Add `Type` column to peers window, update peer details name/tooltip

jonatack commented at 9:27 pm on January 9, 2021: contributor

This pull:

adds a sortable Type column to the GUI Peers tab window
updates the peer details row to Direction/Type, so the Type column without a direction makes sense (the tooltip is also updated)

Screenshot from 2021-02-06 22-53-11

jonatack renamed this:
~~gui: add direction and type columns to peers window~~
Add direction and type columns to peers window
on Jan 9, 2021

jarolrod commented at 1:51 am on January 10, 2021: member

ACK 09aa053dc5a83826265aa575ee61f99f0f76287a

I love this! I was just thinking about how this would be a good feature today, you beat me to it. Code looks good to me. Tested on macOS Big Sur 11.1 with Qt 5.15.2

jonatack commented at 9:32 am on January 10, 2021: contributor

Thanks! Though I should drop the relay type from inbound or check if full or block (same in -netinfo).

hebasto commented at 9:41 am on January 10, 2021: member

Concept ACK.

DrahtBot added the label Needs rebase on Jan 10, 2021

jonatack force-pushed on Jan 10, 2021

DrahtBot removed the label Needs rebase on Jan 10, 2021

jonatack force-pushed on Jan 10, 2021

jonatack commented at 12:06 pm on January 10, 2021: contributor

Rebased after merge of #163, added relay type for inbounds, and inversed the sort for the Direction column to be inbound first.

in src/qt/peertablemodel.cpp:33 in a657b0c4c1 outdated

28@@ -29,6 +29,10 @@ bool NodeLessThan::operator()(const CNodeCombinedStats &left, const CNodeCombine
29         return pLeft->nodeid < pRight->nodeid;
30     case PeerTableModel::Address:
31         return pLeft->addrName.compare(pRight->addrName) < 0;
32+    case PeerTableModel::Direction:
33+        return pLeft->fInbound > pRight->fInbound;
34+    case PeerTableModel::ConnectionType:
35+        return pLeft->m_conn_type < pRight->m_conn_type;

jonatack commented at 2:12 pm on January 10, 2021:

Memo to myself, if we prefer to sort alphabetically by type name rather than by enum order:

0-        return pLeft->m_conn_type < pRight->m_conn_type;
1+        return GUIUtil::ConnectionTypeToShortQString(pLeft->m_conn_type, pLeft->fRelayTxes) < GUIUtil::ConnectionTypeToShortQString(pRight->m_conn_type, pRight->fRelayTxes);

in src/qt/peertablemodel.h:68 in ce52716d48 outdated

68+        Direction = 2,
69+        Network = 3,
70+        Ping = 4,
71+        Sent = 5,
72+        Received = 6,
73+        Subversion = 7

hebasto commented at 5:56 pm on January 10, 2021:

To improve maintainability maybe explicitly assign a value to the first element only? It will make future insert/remove diffs smaller.

jonatack commented at 6:22 pm on January 10, 2021:

done

in src/qt/guiutil.cpp:783 in 13e8676204 outdated

776@@ -777,6 +777,19 @@ QString ConnectionTypeToQString(ConnectionType conn_type)
777     assert(false);
778 }
779 
780+QString ConnectionTypeToShortQString(ConnectionType conn_type, bool relay_txes)
781+{
782+    switch (conn_type) {
783+    case ConnectionType::INBOUND: return QObject::tr(relay_txes ? "Full Relay" : "Block Relay");

hebasto commented at 6:01 pm on January 10, 2021:

0    case ConnectionType::INBOUND: return relay_txes ? QObject::tr("Full Relay") : QObject::tr("Block Relay");

jonatack commented at 6:23 pm on January 10, 2021:

This seems equivalent to me, but thanks for looking! done.

hebasto commented at 6:54 pm on January 10, 2021:

It is not equivalent for the lupdate tool:

When Qt’s translation tool, lupdate, is used to process a set of source files, the text wrapped in tr() calls is stored in a section of the translation file that corresponds to its translation context.

jonatack force-pushed on Jan 10, 2021

jonatack commented at 6:25 pm on January 10, 2021: contributor

Thank you @hebasto for your feedback! Updated with your suggestions per git diff a657b0c 9f76ba6

hebasto approved

hebasto commented at 8:07 pm on January 10, 2021: member

ACK 9f76ba6597cd5d984bac0fa5c076e6b123dba22c, tested on Linux Mint 20.1 (Qt 5.12.8):

Screenshot from 2021-01-10 22-01-41

jarolrod commented at 10:07 pm on January 10, 2021: member

re-ACK 9f76ba6597cd5d984bac0fa5c076e6b123dba22c, tested on macOS Big Sur 11.1 (Qt 5.15.2)

luke-jr commented at 7:10 pm on January 13, 2021: member

Why are Outbound/Inbound strings better than arrows?

jonatack commented at 7:38 pm on January 13, 2021: contributor

Why are Outbound/Inbound strings better than arrows?

Good question. I proposed words instead of arrows for these reasons:

The peer details area uses words. When it displays, say, “Outbound Full Relay”, it seems more coherent for the column data to also display “Outbound” and “Full Relay”, than up-arrow and “Full Relay”
-getinfo and -netinfo both also use words for the direction rather than arrows

luke-jr commented at 10:58 pm on January 13, 2021: member

Words use a lot more space than arrows… It’s not unreasonable to have details show more .. detail :p

hebasto commented at 11:06 pm on January 13, 2021: member

Why are Outbound/Inbound strings better than arrows?

Words use a lot more space than arrows… It’s not unreasonable to have details show more .. detail :p

Actually, words simplify the GUI for new-comers, as an “arrow up” does not imply “outbound” at all.

jonatack commented at 7:32 am on January 14, 2021: contributor

Agree. And the GUI uses words for the direction in the main info tab.

jonatack commented at 8:59 am on January 14, 2021: contributor

(If space is an issue we could use in/out instead of inbound/outbound, as that is the wording in the GUI info tab, -getinfo, and -netinfo)

hebasto commented at 9:18 am on January 14, 2021: member

(If space is an issue we could use in/out instead of inbound/outbound, as that is the wording in the GUI info tab, -getinfo, and -netinfo)

I was thinking about it from the beginning, but it won’t work for some translations.

jarolrod commented at 1:20 am on January 17, 2021: member

(If space is an issue we could use in/out instead of inbound/outbound, as that is the wording in the GUI info tab, -getinfo, and -netinfo)

I think we should stick with inbound/outbound. Even if you changed to in/out wouldn’t Direction keep the tab at about the same size

luke-jr commented at 5:43 pm on January 17, 2021: member

I was thinking something more like this:

https://github.com/jonatack/gui/compare/add-peers-dir-and-type-columns...luke-jr:tmp_gui_peers_dir_arrows

jarolrod commented at 8:51 pm on January 17, 2021: member

I was thinking something more like this:

jonatack/gui@add-peers-dir-and-type-columns…luke-jr:tmp_gui_peers_dir_arrows

Below is a screenshot of what your branch looks like for me, font issue with the arrows. Anyways, my vote is still for inbound/outbound lukejr-arrows

luke-jr commented at 10:24 pm on January 17, 2021: member

I suppose we could also do icons with an arrow and tiny in/out text under it

luke-jr commented at 5:10 pm on January 18, 2021: member

There, how does this look? https://github.com/jonatack/gui/compare/add-peers-dir-and-type-columns...luke-jr:tmp_gui_peers_dir_arrows

jarolrod commented at 5:48 pm on January 18, 2021: member

Screenshot is taken from Luke-jr’s branch

I guess we should get some designer opinion on this: @Bosch-0 what do you think about this icon vs inbound/outbound text.

I think this idea has a lot of potential, but as this iteration I would still vote for inbound/outbound text. I think it’s more informative and fits in better with translations. Also I think Direction and Type should remain adjacent.

jonatack commented at 6:05 pm on January 18, 2021: contributor

Thanks @luke-jr for the proposals and to @jarolrod for the screenshot renderings (a thought, no need to hide addresses if you test on signet or testnet, and signet starts up the fastest of all the chains.)

I agree the direction column should be next to the type and network (but no strong opinion on placement of the address column; maybe after the network column would be better)
The direction header could be shortened to “Dir”, or “<->” like in -netinfo, or use icons/symbols to represent <->
The column header and column values are trivial to change and I suggest leaving that for a follow-up (not by me ;)

This has 2 tested ACKs; @luke-jr if you could ACK as well this could go in and open the door for your proposed follow-up.

luke-jr commented at 6:45 pm on January 18, 2021: member

I don’t like the text form… As-is, IIRC this creates horizontal scrolling because it doesn’t all fit in the window.

jonatack commented at 7:08 pm on January 18, 2021: contributor

I think it depends on your window size state which since #165 is now stored and recalled, so (I believe) this is not an issue after the first resize to your preferred state.

hebasto commented at 7:17 pm on January 18, 2021: member

I suppose we could also do icons with an arrow and tiny in/out text under it

Any text within icons causes translation issues, no?

RandyMcMillan commented at 9:43 pm on January 18, 2021: contributor

@jonatack @hebasto

This worked for me…

https://github.com/bitcoin-core/gui/pull/135/files#diff-e721b02fefce2a90ae95da0841adfdcc610ffccf93236ef236f81c1baead8e01R163

Most of the issues you may run into - I already figured out…

#135

jonatack commented at 9:51 pm on January 18, 2021: contributor

This current PR with 2 ACKs is my preferred version among the various proposals I have seen.

jonatack commented at 9:54 pm on January 18, 2021: contributor

I propose that further refinements be done in a follow-up (not by me ;).

RandyMcMillan commented at 10:08 pm on January 18, 2021: contributor

Something I didn’t get to yet - was to separate the uacomment and give it its own column. This could be very useful for some other ideas Ive been thinking about.

luke-jr commented at 11:33 pm on January 18, 2021: member

https://github.com/jonatack/gui/compare/add-peers-dir-and-type-columns...luke-jr:tmp_gui_peers_dir_arrows is now translation-friendly…

I’m still weak concept nack on inbound/outbound as strings in the table. I think it’s worse than the status quo, so would rather this not be merged as-is.

jonatack marked this as a draft on Jan 29, 2021

jonatack commented at 3:49 pm on January 29, 2021: contributor

Moving this back to Draft status until #201 is resolved.

gui: allow ConnectionTypeToQString to prepend direction optionally 6fc72bd6f0

gui: add "Type" column to Peers main window 151888383a

gui: update to "Direction/Type" peer details name/tooltip be4cf4832f

jonatack force-pushed on Feb 6, 2021

jonatack marked this as ready for review on Feb 6, 2021

jonatack commented at 10:15 pm on February 6, 2021: contributor

Ready for review again. Three changes from the previous version:

It no longer replaces the arrows with a Direction column, which had 2 ACKs but also a light NACK and which can be decided later in another pull
For now, inbound peers do not display anything in the Type column, following the logic of merged #203
In the details area, the Connection Type field is renamed to Direction/Type, so the Type column without a direction makes sense. Its tooltip is also updated.

I hope the translations work correctly in this latest push. @hebasto, @jarolrod, @luke-jr, @RandyMcMillan, would you mind having another look?

jonatack commented at 10:17 pm on February 6, 2021: contributor

Updated the PR description and screenshot with this one:

Screenshot from 2021-02-06 22-53-11

jonatack renamed this:
~~Add direction and type columns to peers window~~
Add `Type` column to peers window, update peer details name/tooltip
on Feb 6, 2021

jarolrod commented at 2:16 pm on February 17, 2021: member

ACK be4cf4832f117ab5f97e1367f7bdcb5361de0dae

Tested with Spanish translations. The text added in the tooltip and text associated with the Type column will need translations.

leonardojobim approved

leonardojobim commented at 6:44 am on February 21, 2021: none

Tested ACK https://github.com/bitcoin-core/gui/commit/be4cf4832f117ab5f97e1367f7bdcb5361de0dae on Ubuntu 20.04 on VMWare.

RandyMcMillan commented at 0:27 am on February 22, 2021: contributor

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.

Maybe Type and Network should be left justified?

NOTE: #202

PR #202 is going to allow for more info/columns to be added - and more info displayed “at a glance”. As more columns are added - the columns can be adjusted for a tighter fit and/or column stretching in a resize event.

ACK 6fc72bd6f030c8b49e8b9f68188413766a239d73

macOS 10.14.6 (18G103)

Screen Shot 2021-02-21 at 7 22 01 PM

MarcoFalke merged this on Feb 22, 2021

MarcoFalke closed this on Feb 22, 2021

jonatack deleted the branch on Feb 22, 2021

sidhujag referenced this in commit ed9729095d on Feb 22, 2021

gwillen referenced this in commit fe1e52f667 on Jun 28, 2022

bitcoin-core locked this on Aug 16, 2022

Add Type column to peers window, update peer details name/tooltip #179

Add `Type` column to peers window, update peer details name/tooltip #179