Add "Direction" column to peers tab

jonatack commented at 1:51 pm on April 23, 2021: contributor

Screenshot from 2021-04-24 13-15-54

This was initially proposed in #179 and saw an enthusiastic reception with several ACKs. One contributor was a weak NACK due to the horizontal space and so this was temporarily dropped, but since then #202 (and soon #256 and #293) resolve the issue. Moreover, the current state of displaying only a connection Type column without a connection Direction column is confusing (e.g. https://github.com/bitcoin/bitcoin/issues/21747#issuecomment-825072559), particularly as both the direction and the type are displayed in the peer details area with the Direction/Type row.

This patch fixes it and completes the initial proposal in #179 by adding a Direction column, making the peers tab the same as the Direction/Type row in the peer details and the direction and type columns in our other user-facing peer connections table in -netinfo.

Users can now sort the peers table by direction. The default sort is set to inbound, then outbound.

This patch also makes the direction and type translations the same for the 3 places where they are used (the peers column, the peer details row, and its tooltip), adds translator comments to link them together, and adds QStringBuilder fast QString concatenation with the % operator.

in src/qt/peertablemodel.cpp:168 in d508c3fd27 outdated

162@@ -161,8 +163,9 @@ QVariant PeerTableModel::data(const QModelIndex &index, int role) const
163         case NetNodeId:
164             return (qint64)rec->nodeStats.nodeid;
165         case Address:
166-            // prepend to peer address down-arrow symbol for inbound connection and up-arrow for outbound connection
167-            return QString(rec->nodeStats.fInbound ? "↓ " : "↑ ") + QString::fromStdString(rec->nodeStats.addrName);
168+            return QString::fromStdString(rec->nodeStats.addrName);
169+        case Direction:
170+            return QString(rec->nodeStats.fInbound ? "Inbound" : "Outbound");

jarolrod commented at 7:59 pm on April 23, 2021:

@hebasto: Would this return the translated string?

Wouldn’t we need to do

0            return QString(rec->nodeStats.fInbound ? tr("Inbound") : tr("Outbound"));

hebasto commented at 8:17 pm on April 23, 2021:

Also we could add translator comments now (see #287 for details).

jonatack commented at 11:42 am on April 24, 2021:

Done.

jarolrod commented at 8:02 pm on April 23, 2021: member

Concept ACK

I think this is an improvement over the current little arrows next to the address. But, this improvement is dependent on getting translations done.

What if we kept the arrow next to the Inbound and Outbound text. That way, if there is no translation available, the arrows provide some context to go on.

jonatack force-pushed on Apr 24, 2021

jonatack commented at 11:57 am on April 24, 2021: contributor

What if we kept the arrow next to the Inbound and Outbound text. That way, if there is no translation available, the arrows provide some context to go on. @jarolrod Thanks for reviewing! I added the arrows yesterday but then removed them today, because the Direction and Type columns should display the same thing as the Direction/Type details row. I also made the direction and type translations the same for the three places where they are used (peers column, peer details row, and its tooltip) and added translator comments.

RandyMcMillan commented at 9:44 pm on April 24, 2021: contributor

@jonatack - In this previous PR #135 I did something similar…

The goal was to actually embed unicode arrows in the column and not use text for direction at all.

This may be better visually and save column space.

The column sorting function should sort by unicode value with little to no additional work.

I just wanted to mention that since this PR is focusing on the direction column.

jonatack commented at 9:53 pm on April 24, 2021: contributor

Thanks – yes, I know but don’t prefer a column with just arrows.

More objectively, there is an issue described in the PR description above: the Inbound/Outbound direction is displayed in the peer details area (the Direction/Type row) but not in the peers windows itself, which is confusing to users. Now that space is less of an issue thanks to #202 and #256, we can complete what was started in #179 and properly display the Direction and the Type next to each other as we are already doing in the peer details.

RandyMcMillan commented at 11:41 pm on April 24, 2021: contributor

#293 should fit with #202 #256 & #179 as well…

What do you think leave the Ampersands or take them out?

jonatack commented at 1:05 pm on April 25, 2021: contributor

What do you think leave the Ampersands or take them out?

The current space-ampersand-space, or comma-space, seem like good separators.

jonatack force-pushed on Apr 25, 2021

jonatack commented at 1:50 pm on April 25, 2021: contributor

Update: after adding the arrows yesterday, I removed them today because the Direction and Type columns should display the same thing as the Direction/Type details row. In a new second commit, I made the direction and type translations the same for the three places where they are used (peers column, peer details row, and its tooltip) and added translator comments to link them together. It’s not clear to me, however, if the translator comments should refer to places in the codebase or to where the translations are displayed to users.

in src/qt/rpcconsole.cpp:467 in d04ebdaab7 outdated

466-        tr("Outbound Full Relay: default"),
467-        tr("Outbound Block Relay: does not relay transactions or addresses"),
468-        tr("Outbound Manual: added using RPC %1 or %2/%3 configuration options")
469+        //: The connection direction (Inbound/Outbound) is also used in ConnectionTypeToQString() and PeerTableModel::data().
470+        //: The connection types (Full Relay, Block Relay, Manual, Feeler, Address Fetch) are also used in ConnectionTypeToQString().
471+        tr("Inbound") + ": " + tr("initiated by peer"),

Talkless commented at 1:58 pm on April 25, 2021:

nit: Now there’s lot’s of string concatenations, maybe worth using QStringBuilder’s % and wrapping " " and “: " with QLatin1String ?

jonatack commented at 3:40 pm on April 25, 2021:

Thanks @Talkless. Added QLatin1String constants in 73cf0fdc8edacf9294f9facc8248fb078298b2d7. Let me know if you think using % would still improve the diff and what you’d suggest.

Talkless approved

Talkless commented at 1:58 pm on April 25, 2021: none

tACK d04ebdaab74493bb0cba79fbb81b8225e8bbb8de, tested on Debian Sid with Qt 5.15.2.

jonatack force-pushed on Apr 25, 2021

hebasto added the label Design on Apr 25, 2021

in src/qt/addressbookpage.cpp:19 in cee6761b06 outdated

15@@ -16,6 +16,7 @@
16 #include <qt/platformstyle.h>
17 
18 #include <QIcon>
19+#include <QLatin1String>

Talkless commented at 4:50 pm on April 25, 2021:

Uhm, I’m a bit lost, why do we have QLatin1String is included a lot in this PR?

jonatack commented at 4:57 pm on April 25, 2021:

Added the header where it is used but header is missing.

in src/qt/rpcconsole.cpp:58 in cee6761b06 outdated

53@@ -52,6 +54,9 @@ const int INITIAL_TRAFFIC_GRAPH_MINS = 30;
54 const QSize FONT_RANGE(4, 40);
55 const char fontSizeSettingsKey[] = "consoleFontSize";
56 
57+const QLatin1String COLON_SPACE{": "};

Talkless commented at 4:51 pm on April 25, 2021:

Interesting idea. It actually could be constexpr.

jonatack commented at 4:59 pm on April 25, 2021:

That was the first thing I tried but the compiler doesn’t like it.

Talkless commented at 6:04 pm on April 26, 2021:

That’s strange, QLatin1String’s constructors are constexpr-marked since at least 5.3: https://code.qt.io/cgit/qt/qtbase.git/tree/src/corelib/tools/qstring.h?h=5.3#n89

Sure that wasn’t type or something? What kind of message was that? And what compiler?

jonatack commented at 11:05 pm on April 28, 2021:

This seemed relevant: https://stackoverflow.com/questions/56201976/qt-vs-constexpr-string-literal

Clang 9 and GCC 10.2.1:

 0  CXX      qt/libbitcoinqt_a-rpcconsole.o
 1qt/rpcconsole.cpp:58:32: error: constexpr variable 'COLON_SPACE' must be initialized by a constant expression
 2static constexpr QLatin1String COLON_SPACE{": "};
 3                               ^~~~~~~~~~~~~~~~~
 4/usr/include/x86_64-linux-gnu/qt5/QtCore/qstring.h:91:93: note: non-constexpr function 'strlen' cannot be used in a constant expression
 5    Q_DECL_CONSTEXPR inline explicit QLatin1String(const char *s) noexcept : m_size(s ? int(strlen(s)) : 0), m_data(s) {}
 6                                                                                            ^
 7qt/rpcconsole.cpp:58:32: note: in call to 'QLatin1String(&": "[0])'
 8static constexpr QLatin1String COLON_SPACE{": "};
 9                               ^
10qt/rpcconsole.cpp:59:32: error: constexpr variable 'SPACE' must be initialized by a constant expression
11static constexpr QLatin1String SPACE{" "};
12                               ^~~~~~~~~~
13/usr/include/x86_64-linux-gnu/qt5/QtCore/qstring.h:91:93: note: non-constexpr function 'strlen' cannot be used in a constant expression
14    Q_DECL_CONSTEXPR inline explicit QLatin1String(const char *s) noexcept : m_size(s ? int(strlen(s)) : 0), m_data(s) {}
15                                                                                            ^
16qt/rpcconsole.cpp:59:32: note: in call to 'QLatin1String(&" "[0])'
17static constexpr QLatin1String SPACE{" "};
18                               ^
192 errors generated.

in src/qt/rpcconsole.cpp:47 in cee6761b06 outdated

41 #include <QMessageBox>
42 #include <QScreen>
43 #include <QScrollBar>
44 #include <QSettings>
45 #include <QString>
46+#include <QStringBuilder>

Talkless commented at 4:53 pm on April 25, 2021:

Not sure if it helps only by including (without using operator %), unless whole bitcoin-qt would be built with QT_USE_QSTRINGBUILDER.

jonatack commented at 4:56 pm on April 25, 2021:

Added because this file already uses %.

Talkless commented at 6:03 pm on April 26, 2021:

I see, QLatin1String and QStringBuilder included because where simply missing, not because of new usage. New usage would be nice though, but I don’t want to be too-QString-nazi..

jonatack commented at 11:29 pm on April 28, 2021:

Thanks for the nudge, @Talkless. Looked up https://wiki.qt.io/Using_QString_Effectively and updated. Please let me know what you think.

DrahtBot added the label Needs rebase on Apr 28, 2021

gui: add Direction column to peers tab 678b57227e

jonatack force-pushed on Apr 28, 2021

jonatack commented at 7:21 pm on April 28, 2021: contributor

Rebased due to the merge of #18. Thanks for the reviews @Talkless!

DrahtBot removed the label Needs rebase on Apr 28, 2021

gui: improve connection type translations and translator comments 842f4e834d

gui: use SPACE constant, QStringBuilder concat in rpcconsole.cpp ad92e04968

gui: add missing QLatin1String, QStringBuilder headers 2bf3f5aee0

jonatack force-pushed on Apr 28, 2021

ghost commented at 5:11 am on April 30, 2021: none

Concept ACK

in src/qt/peertablemodel.cpp:119 in 678b57227e outdated

112@@ -113,8 +113,10 @@ QVariant PeerTableModel::data(const QModelIndex &index, int role) const
113         case NetNodeId:
114             return (qint64)rec->nodeStats.nodeid;
115         case Address:
116-            // prepend to peer address down-arrow symbol for inbound connection and up-arrow for outbound connection
117-            return QString(rec->nodeStats.fInbound ? "↓ " : "↑ ") + QString::fromStdString(rec->nodeStats.addrName);
118+            return QString::fromStdString(rec->nodeStats.addrName);
119+        case Direction:
120+            //: Connection direction. Also used in ConnectionTypeToQString() and CONNECTION_TYPE_DOC.
121+            return QString(rec->nodeStats.fInbound ? tr("Inbound") : tr("Outbound"));

unknown commented at 5:13 am on April 30, 2021:

Can we use IN and OUT in caps instead of Inbound and Outbound?

jarolrod commented at 7:11 am on April 30, 2021:

Here is how that would look, not a bad idea. But, a) I think this change is ok as is b) would like to hear other opinions.

Screen Shot 2021-04-30 at 3 09 34 AM

jonatack commented at 7:15 am on April 30, 2021:

The goal is to unify the peers tab Direction and Type columns with the Direction/Type row in the peer details sidebar, so if the side bar displays “Outbound Full Relay”, the main table also displays “Outbound Full Relay” with the ability to sort by either Direction or Type.

jonatack commented at 7:25 am on April 30, 2021:

(I also tried “In/Out” while working on this PR, and the horizontal space isn’t reduced unless the column header is “Dir” instead of “Direction”. But the other headers are full words.)

ghost commented at 5:22 am on April 30, 2021: none

How do you have 3 Block Relay connections in this screenshot? Isn’t MAX 2?

jarolrod commented at 7:03 am on April 30, 2021: member

ACK 2bf3f5aee014ab4e0ff72e6bdbb33ada02dec314

This patch works well and is a UX improvement. Tested on macOS 11.3 Qt 5.15.2

Thanks for working on this and adding in translator comments.

Screen Shot 2021-04-30 at 2 59 31 AM

jonatack commented at 7:22 am on April 30, 2021: contributor

How do you have 3 Block Relay connections in this screenshot? Isn’t MAX 2?

We very often have a short-lived third extra block relay connection since the merge of https://github.com/bitcoin/bitcoin/pull/19858.

See this open PR that updates our docs about this: https://github.com/bitcoin/bitcoin/pull/21709/files.

hebasto commented at 12:24 pm on April 30, 2021: member

Concept ACK on removing direction info from the “Address” column. @luke-jr wrt your comment #179 (comment) and new circumstances, did you change your mind?

jonatack commented at 8:34 pm on April 30, 2021: contributor

Up for grabs.

jonatack closed this on Apr 30, 2021

hebasto added the label up for grabs on Apr 30, 2021

jonatack deleted the branch on Apr 30, 2021

ghost commented at 8:04 pm on May 2, 2021: none

I can work on this. But I am not sure why is this closed and “Up for grabs” because @jonatack already made changes required and reviews looked positive as well.

hebasto commented at 8:18 pm on May 2, 2021: member

@prayank23

I can work on this.

You’re also welcome to join the ##bitcoin-core-gui (with two #) channel on IRC :)

jonatack commented at 8:45 pm on May 2, 2021: contributor

Hi @prayank23, sorry for the confusion. I opened this to finish what was started with #179 but decided to not spend further time on GUI/design/QT/UI/translation things. Everything takes time to do well, and discussing each word, arrow, indentation, translation, and consistency question and explanation until everyone is happy is completely normal, but it takes bandwidth in an area that isn’t my focus. Cheers.

jarolrod commented at 3:16 pm on May 26, 2021: member

@hebasto this is no longer up for grabs as it was picked up in #317

hebasto removed the label Design on May 26, 2021

hebasto removed the label up for grabs on May 26, 2021

hebasto added the label UI on May 26, 2021

hebasto removed the label UI on May 26, 2021

hebasto added the label UX on May 26, 2021

hebasto referenced this in commit 77e23ca945 on Aug 11, 2021

hebasto referenced this in commit ccc4b9125a on Sep 29, 2021

bitcoin-core locked this on Aug 16, 2022

Add “Direction” column to peers tab #289