Align numbers in the "Peer Id" column to the right#325
Align numbers in the "Peer Id" column to the right#325hebasto merged 1 commit intobitcoin-core:masterfrom
Conversation
| ui->peerWidget->setColumnWidth(PeerTableModel::Subversion, SUBVERSION_COLUMN_WIDTH); | ||
| ui->peerWidget->setColumnWidth(PeerTableModel::Ping, PING_COLUMN_WIDTH); | ||
| ui->peerWidget->horizontalHeader()->setStretchLastSection(true); | ||
| ui->peerWidget->setItemDelegateForColumn(PeerTableModel::NetNodeId, new PeerIdViewDelegate(this)); |
There was a problem hiding this comment.
I don't have a strong opinion on this. I think with and without the PeerIdViewDelegate both look fine:
| With Delegate | Without Delegate |
|---|---|
 |
 |
There was a problem hiding this comment.
Thanks for bringing this patch to my attention @jarolrod and for this helpful comparison. I prefer with the added separation between the right-aligned peer number and the left-aligned address as proposed here (which is why I originally proposed center-aligned to have the spacing as well and aligned under the header that is also center-aligned, but the version proposed in this PR seems good too).
There was a problem hiding this comment.
ACK 69b8b5d
Having the Peer Id numbers right-aligned is a UX improvement in my opinion.
Through testing and resizing columns, it's possible for the Peer id and Address columns to look a bit cluttered.
This PR's approach to prevent this clutter is to use a QStyledItemDelegate, named PeerIdViewDelegate, which gets around this by drawing the value with a few spaces after the number.
It's an important distinction that these spaces are being drawn and not appended to the ID value. We could instead just append these spaces to the following line directly and not use a delegate:
Line 114 in b295395
But, that would mess up cases where we access the NetNodeId value such as here:
Line 1155 in b295395
If other reviewers think the clutter is a non-issue, or the added complexity of a QStyledItemDelegate is not worth it, then this PR can just be comprised of this line:
Line 135 in 69b8b5d
See my comparison of what this PR looks like with and without the delegate here: https://github.com/bitcoin-core/gui/pull/325/files#r631517595
|
Concept ACK. I wanted to change this column away from left-alignment as well, so thank you for proposing this. |
jonatack
left a comment
jonatack
left a comment
There was a problem hiding this comment.
Tested rebased to master ("Peer" header, this PR could be renamed) and with the arrows removed from the neighboring address column to test the case of them being replaced by a dedicated Direction column.
Center-aligned:
Right-aligned per this patch:
I think this is an improvement, though maybe a bit more separation from the Address column might be good and I do prefer center-aligned for overall readability and balance in the design.
ACK 69b8b5d happy to re-ack with more separation or center-aligned
Let's collect some feedback with current one :)
I'm really opposite against center-aligned ids, as it ends with such a weird layout:
|
4 participants
On master (6b49d88):
With this PR:
