Align numbers in the "Peer Id" column to the right

hebasto commented at 6:56 pm on May 12, 2021: member

On master (6b49d88a5dda97fdbabe363fd7d3c4f1ce29082a): Screenshot from 2021-05-12 21-53-52

With this PR: Screenshot from 2021-05-12 21-48-19

hebasto added the label UI on May 12, 2021

hebasto force-pushed on May 12, 2021

qt: Align numbers in the "Peer Id" column to the right 69b8b5d72c

hebasto force-pushed on May 12, 2021

in src/qt/rpcconsole.cpp:654 in 69b8b5d72c

650@@ -634,6 +651,7 @@ void RPCConsole::setClientModel(ClientModel *model, int bestblock_height, int64_
651         ui->peerWidget->setColumnWidth(PeerTableModel::Subversion, SUBVERSION_COLUMN_WIDTH);
652         ui->peerWidget->setColumnWidth(PeerTableModel::Ping, PING_COLUMN_WIDTH);
653         ui->peerWidget->horizontalHeader()->setStretchLastSection(true);
654+        ui->peerWidget->setItemDelegateForColumn(PeerTableModel::NetNodeId, new PeerIdViewDelegate(this));

jarolrod commented at 2:15 am on May 13, 2021:

I don’t have a strong opinion on this. I think with and without the PeerIdViewDelegate both look fine:

With Delegate	Without Delegate

jonatack commented at 9:54 pm on May 26, 2021:

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).

jarolrod commented at 2:15 am on May 13, 2021: member

Concept ACK

jarolrod commented at 10:09 pm on May 24, 2021: member

ACK 69b8b5d72c47d42a9e69e6666af89606057be15b

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: https://github.com/bitcoin-core/gui/blob/b295395664bd37e26d168c329f238237b34aef8c/src/qt/peertablemodel.cpp#L114

But, that would mess up cases where we access the NetNodeId value such as here: https://github.com/bitcoin-core/gui/blob/b295395664bd37e26d168c329f238237b34aef8c/src/qt/rpcconsole.cpp#L1155

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:

https://github.com/bitcoin-core/gui/blob/69b8b5d72c47d42a9e69e6666af89606057be15b/src/qt/peertablemodel.cpp#L135

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

jonatack commented at 9:56 pm on May 26, 2021: contributor

Concept ACK. I wanted to change this column away from left-alignment as well, so thank you for proposing this.

jonatack commented at 10:04 am on May 28, 2021: contributor

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:

Screenshot from 2021-05-28 11-51-04

Right-aligned per this patch:

Screenshot from 2021-05-28 11-56-23

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 69b8b5d72c47d42a9e69e6666af89606057be15b happy to re-ack with more separation or center-aligned

kristapsk approved