-
Notifications
You must be signed in to change notification settings - Fork 38.8k
Removes forward slash (/) from user agent in peers tab UI. #7640
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| return strResult; | ||
| } | ||
|
|
||
| /// Formats the network peer user agent text (or subversion) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please put the comment in the h file. And use the formatting according to the guideline (CONTRIBUTING.md)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will move my comments to the header file and squash the commits into one if the concept is ACK.
|
I like making the GUI cleaner but isn't the slash part of the string? https://github.com/bitcoin/bips/blob/master/bip-0014.mediawiki#proposal |
|
It looks like BIP0014 uses the forward slashes as delimiters or separator. One node doesn't have many user agents, so the delimiter is not needed in this case.
Displaying a separator or delimiter in the UI is unnecessary and goes against Jakob Nielsen heuristics: https://www.nngroup.com/articles/ten-usability-heuristics/:
|
|
It's a debug window... it's full of information which is irrelevant and rarely needed. NACK on the concept |
|
@pstratem, I don't think your NACK would be considered sound technical justification. The UI is displaying a delimiter for no reason. |
|
This also affects the RPC |
|
On second thought: no, I don't like this. I like the debug window to report exactly the data that goes over the network. This may make the string prettier but makes it less useful too. |
|
@CryptoDJ I think it's a matter of taste which looks better:
As the current way is exactly the way how BIP 14 defines it, you can't really say it is "irrelevant or rarely needed". Personallly, I'd suggest we just stick with BIP 14 for simplicity and consistency. |
|
The member variable is called cleanSubVer and it is put through a sanitize function. See the comment in net.h:
When consuming the API message, the / in cleanSubVer is kept and displayed to end users. Web sites display it b/c who want to further sanitize the string? Also, why have strSubVer and cleanSubVer? |
|
https://bitnodes.21.co/nodes/ I think BIP 14 pertains to the wire format, NOT the UI. |
Correct. However, the reason for the debug window is troubleshooting. For technical diagnostics, it is important to have accurate information. Granted, removing a slash is not a big deal, but we shouldn't make a habit out of presenting things differently than they are on the wire. In any case: if the goal of this is to affect the UI only, this pull request should touch only UI code. |
|
This is adding code which is for all intents doing nothing. NACK on the simple principle that less code is better. |
|
My problem is with BIP 14 and I will pull request to change it. The user-agent string example begins and ends with delimiters (the forward slash or /) that have no use to humans or computers and therefore should be eliminated. Eliminating these superfluous delimiters will slightly decreased the data size transmitted to peers, stored when consuming API messages and make the UI look better. So far, no one has been able to explain the benefit of the leading and trailing forwards slashes except that it is specified in BIP 14. Edit: BIP 14 pull request: bitcoin/bips#352 |
The user-agent string example begins and ends with delimiters (the forward slash or /) that have no use to humans or computers and therefore should be eliminated. Eliminating these superfluous delimiters will slightly decreased the data size transmitted to peers, stored when consuming API messages and make the UI look better. So far, no one has been able to explain the benefit of the leading and trailing forwards slashes except that it is specified in this BIP. Please see this pull request: bitcoin/bitcoin#7640
|
I'm closing this, there doesn't seem to be agreement to do this. |

Formats the network peer user agent text (or clean subversion) by removing the forward slash prefix and suffix. Example: /Satoshi:0.11.2/ --> Satoshi:0.11.2
The forward slashes in the User Agent text are occupying two spaces and seem unnecessary for the Qt wallet UI.
This code is forked from my original pull for Dark Silk: https://github.com/SCDeveloper/DarkSilk-Release-Candidate/pull/138