Peers window `Peer id` improvements by jonatack · Pull Request #290 · bitcoin-core/gui

jonatack · 2021-04-23T14:50:38Z

This patch:

renames the peers tab column header from Peer Id to Peer to allow resizing the column more tightly (this will be particularly useful after Save/restore column sizes of the tables in the Peers tab #256) and does the same for the peer details area.
center-aligns the entries in the Peer column to align them under the column header rather than to the left (I also tried right-alignment but the values are unreadably close to the Address values and it seems more esthetic to be aligned under the header 🗄️)

to allow resizing the column more tightly

This aligns them under the column header.

RandyMcMillan · 2021-04-23T18:42:13Z

Tested on MacOS 10.14.6 (18G103)

Talkless

tACK e14d79f, tested on Debian Sid with Qt 5.15.2.

Talkless · 2021-04-25T13:32:23Z

renaming to Id instead of Peer would work also but I'm not sure about translateability

What are the concerns exactly?

kristapsk · 2021-04-25T13:39:40Z

renaming to Id instead of Peer would work also but I'm not sure about translateability

"Id" seems to be a short for "Identifier" in most of the languages I checked. Are there exceptions for that and languages where "Id" can't be translated to something similar? https://en.wikipedia.org/wiki/ID

jonatack · 2021-04-25T14:59:07Z

"Id" is actually incorrect (I think you are referring to "ID"). Outside of databases and developers, "Id" is a somewhat confusing word to use because it has many meanings. See https://www.thefreedictionary.com/id. The link you gave also lists many definitions. It is also an acronym for many different things. For these reasons, I proposed "Peer" to see what reviewers think.

jonatack · 2021-04-25T15:02:16Z

(Dropped the "Id" part from the PR description.)

jarolrod · 2021-04-25T18:05:37Z

- case NetNodeId:
 case Address:
 return {};
+ case NetNodeId:


In e14d79f
Why move the position of NetNodeId here? In the other areas of the code, NetNodeId always comes before Address in our switch statements. I don't think we should move it here.
You can keep it where it originally is and do:
Suggested change
case NetNodeId:
case NetNodeId:
return QVariant(Qt::AlignCenter);

Generally I prefer to propose the smaller, simpler diff. The order doesn't matter here and it seems better to group the cases by alignment.

It's not much of a difference, plus this makes it easier to reason about. You are explicitly stating that you are making it AlignCenter. I'd prefer we keep the same order as our other switch statements, but it's not a strong opinion.

Always keeping the same order of enum elements in switch statement could decrease mental burden for some ppl :)

Ditto, diff is only for "once" (for review), while people will read this switch multiple times in the future, so I'm also for same ordering.

jarolrod · 2021-04-29T19:33:20Z

ACK e14d79f

% nit about switch statement order

hebasto · 2021-04-30T08:51:53Z

renaming to Id instead of Peer would work also but I'm not sure about translateability

btw, a good translation of a noun "peer" that means "a participant of the peer-to-peer network" could be a real challenge, at least in some languages 😃

maflcko · 2021-04-30T08:59:57Z

Is there a way to tell translators synonyms? "Connection" might be easier to translate.

jonatack · 2021-04-30T09:01:12Z

Is there a way to tell translators synonyms? "Connection" might be easier to translate.

Yes, could mention synonyms like "connection, number" in the translator comment.

hebasto · 2021-04-30T09:04:23Z

One more opinion:

Removing "Peer" from the "Peer Id" column would allow for more prudent use of space.
Otherwise it should be translatable. A simple "ID" or "id" would be better. I think all caps "ID" is better imo.

hebasto · 2021-04-30T09:05:55Z

Is there a way to tell translators synonyms? "Connection" might be easier to translate.
Yes, could mention synonyms like "connection, number" in the translator comment.

We could mention the meaning of a term :)

hebasto · 2021-04-30T10:02:44Z

Heh, interesting. Should I mention "ID" in a translation comment? (I'm not against using "ID" instead if reviewers prefer but to my mind it reminds me of "papers, please" police_officer)

If we concern brevity, that the "Peer" word is redundant because the tab is called "Peers".
If we concern consistency, the getpeerinfo output has the id field.

Anyway, translator comments are always welcome.

IIRC, there were some comments about aligning of the "Peer Id" column in this repo, especially when numbers of id go up. But I cannot find them :(

Maybe our fellow designers @Bosch-0 and @GBKS could share their opinions about this PR?

jonatack · 2021-04-30T15:34:53Z

-netinfo also uses "id", but ISTM the command line / JSON-RPC interface follows a different paradigm than the GUI:

JSON lowercase or snakecase / Capitalized Titles
language for developers / language for laypeople
brevity / full words
untranslated / translated

As for data alignment in the column, note that I discussed what I tried and found in the PR description.

jonatack · 2021-04-30T16:13:54Z

It looks like "peers" and "peer" are already translated in qt/locale (if I understand them correctly) in a number of places; here are just a few of them:

 <message>
<source>&amp;Peers</source>
<translation>&amp;Gegenstellen</translation>
</message>
<message>
<source>Banned peers</source>
<translation>Gesperrte Gegenstellen</translation>
</message>
<message>
<source>Select a peer to view detailed information.</source>
<translation>Gegenstelle auswählen, um detaillierte Informationen zu erhalten.</translation>
</message>

 <message>
<source>&amp;Peers</source>
<translation>&amp;Пиры</translation>
</message>
<message>
<source>Banned peers</source>
<translation>Заблокированные пиры</translation>
</message>
<message>
<source>Select a peer to view detailed information.</source>
<translation>Выберите пира для просмотра детальной информации.</translation>
</message>

jonatack · 2021-04-30T20:25:18Z

I've decided to close this.

jonatack · 2021-05-01T10:50:44Z

Maybe our fellow designers @Bosch-0 and @GBKS could share their opinions about this PR?

One thought: designer, UI, and UX can be different specialties, a bit like front end / back end / dev ops / DBA / pentest / embedded, etc. There are designers (say, of icons, logos, typography or illustrations) who wouldn't expect to be asked to make a call about a UI question, and others who might be more comfortable with that.

jonatack · 2021-05-01T16:04:44Z

To clarify, I think this is a good change but it's too much time involved on a very tiny change that may never be merged in a repo that isn't my main focus. That said, I'd be happy for someone more focused on the GUI to fix it.

jarolrod · 2021-05-26T15:18:55Z

@hebasto This is no longer up for grabs as the idea of changing Peer id to Peer was picked up in #311 and the idea of improving the alignment of the Peer Number was picked up in #325

657b33e qt: add translator comments for peers table columns (Jarol Rodriguez) 73a91c6 gui: rename "Peer Id" to "Peer" in tab column and details area (Jon Atack) Pull request description: Picking up bitcoin-core/gui#290 **Original PR Description:** - renames the peers tab column header from `Peer Id` to `Peer` to allow resizing the column more tightly (this will be particularly useful after #256) and does the same for the peer details area. While here, we also add Qt translator comments for the Peer Table columns. | Master | PR | | ----------- | ----------- | | ![Screen Shot 2021-05-03 at 1 23 05 AM](https://user-images.githubusercontent.com/23396902/116843818-20a14b00-abaf-11eb-913e-ddff11cda5cd.png) | ![Screen Shot 2021-05-05 at 4 08 45 AM](https://user-images.githubusercontent.com/23396902/117112825-a2cc7380-ad57-11eb-939b-1aceb4214ad1.png) | ACKs for top commit: jonatack: utACK 657b33e hebasto: re-ACK 657b33e Tree-SHA512: f50116f7ca8719cadf1f95f45e3594b3b92bde9c43eb954f3e963ed10629dd9406efdb5e96aa1f750a926e24a96321d824ed3780bd9cd748774e0b85fd0c9535

jonatack added 2 commits April 23, 2021 16:21

gui: rename "Peer Id" to "Peer" in tab column and details area
b12165b
to allow resizing the column more tightly

gui: center-align the entries in the peers tab "Peer" column
e14d79f
This aligns them under the column header.

hebasto added the Design label Apr 23, 2021

RandyMcMillan mentioned this pull request Apr 23, 2021
Network Graph layout - debug window improvement. #291
Closed

Talkless approved these changes Apr 25, 2021
View reviewed changes

jarolrod reviewed Apr 25, 2021
View reviewed changes

jonatack closed this Apr 30, 2021

jonatack deleted the peer-id-improvements branch April 30, 2021 20:26

hebasto added the Up for grabs label May 1, 2021

jarolrod mentioned this pull request May 3, 2021
Peers Window rename 'Peer id' to 'Peer' #311
Merged

hebasto removed the Up for grabs label May 26, 2021

hebasto added UI All about "look and feel" and removed Design labels May 26, 2021

bitcoin-core locked as resolved and limited conversation to collaborators Aug 16, 2022

	case NetNodeId:
	case NetNodeId:
	return QVariant(Qt::AlignCenter);

Uh oh!

Conversation

jonatack commented Apr 23, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RandyMcMillan commented Apr 23, 2021

Uh oh!

Talkless left a comment

Choose a reason for hiding this comment

Uh oh!

Talkless commented Apr 25, 2021

Uh oh!

kristapsk commented Apr 25, 2021

Uh oh!

jonatack commented Apr 25, 2021

Uh oh!

jonatack commented Apr 25, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jarolrodApr 25, 2021

Choose a reason for hiding this comment

Uh oh!

jonatackApr 25, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jarolrodApr 29, 2021

Choose a reason for hiding this comment

Uh oh!

hebastoApr 30, 2021

Choose a reason for hiding this comment

Uh oh!

TalklessMay 3, 2021

Choose a reason for hiding this comment

Uh oh!

jarolrod commented Apr 29, 2021

Uh oh!

hebasto commented Apr 30, 2021

Uh oh!

maflcko commented Apr 30, 2021

Uh oh!

jonatack commented Apr 30, 2021

Uh oh!

hebasto commented Apr 30, 2021

Uh oh!

hebasto commented Apr 30, 2021

Uh oh!

hebasto commented Apr 30, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack commented Apr 30, 2021

Uh oh!

jonatack commented Apr 30, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack commented Apr 30, 2021

Uh oh!

jonatack commented May 1, 2021

Uh oh!

jonatack commented May 1, 2021

Uh oh!

jarolrod commented May 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jonatack commented Apr 23, 2021•
edited
Loading

jonatack commented Apr 25, 2021•
edited
Loading

jonatackApr 25, 2021•
edited
Loading

hebasto commented Apr 30, 2021•
edited
Loading

jonatack commented Apr 30, 2021•
edited
Loading