Add "Direction" column to peers tab #289

jonatack · 2021-04-23T13:51:53Z

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. bitcoin/bitcoin#21747 (comment)), 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.

jarolrod

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.

src/qt/peertablemodel.cpp

jonatack · 2021-04-24T11:57:47Z

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 · 2021-04-24T21:44:05Z

@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 · 2021-04-24T21:53:02Z

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 · 2021-04-24T23:41:18Z

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

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

jonatack · 2021-04-25T13:05:28Z

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

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

jonatack · 2021-04-25T13:50:55Z

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.

Talkless

tACK d04ebda, tested on Debian Sid with Qt 5.15.2.

src/qt/rpcconsole.cpp

src/qt/addressbookpage.cpp

Talkless · 2021-04-25T16:51:52Z

src/qt/rpcconsole.cpp

@@ -52,6 +54,9 @@ const int INITIAL_TRAFFIC_GRAPH_MINS = 30;
 const QSize FONT_RANGE(4, 40);
 const char fontSizeSettingsKey[] = "consoleFontSize";

+const QLatin1String COLON_SPACE{": "};


Interesting idea. It actually could be constexpr.

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

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?

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

Clang 9 and GCC 10.2.1:

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

Talkless · 2021-04-25T16:53:46Z

src/qt/rpcconsole.cpp

 #include <QMenu>
 #include <QMessageBox>
 #include <QScreen>
 #include <QScrollBar>
 #include <QSettings>
 #include <QString>
+#include <QStringBuilder>


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

Added because this file already uses %.

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

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

jonatack · 2021-04-28T19:21:19Z

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

ghost · 2021-04-30T05:11:10Z

Concept ACK

ghost · 2021-04-30T05:13:32Z

src/qt/peertablemodel.cpp

+            return QString::fromStdString(rec->nodeStats.addrName);
+        case Direction:
+            //: Connection direction. Also used in ConnectionTypeToQString() and CONNECTION_TYPE_DOC.
+            return QString(rec->nodeStats.fInbound ? tr("Inbound") : tr("Outbound"));


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

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.

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.

(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 · 2021-04-30T05:22:49Z

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

jarolrod

ACK 2bf3f5a

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.

jonatack · 2021-04-30T07:22:16Z

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 bitcoin/bitcoin#19858.

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

hebasto · 2021-04-30T12:24:38Z

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 · 2021-04-30T20:34:41Z

Up for grabs.

ghost · 2021-05-02T20:04:19Z

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 · 2021-05-02T20:18:50Z

@prayank23

I can work on this.

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

jonatack · 2021-05-02T20:45:04Z

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 · 2021-05-26T15:16:36Z

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

6971e79 gui: add Direction column to peers tab (Jon Atack) Pull request description: Picking up #289 This adds 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. | Master | PR | | ----------- | ----------- | | ![Screen Shot 2021-05-05 at 3 51 09 AM](https://user-images.githubusercontent.com/23396902/117111864-38ff9a00-ad56-11eb-889d-f1c838c845e6.png) | ![Screen Shot 2021-05-05 at 3 35 40 AM](https://user-images.githubusercontent.com/23396902/117111892-4157d500-ad56-11eb-82b1-5bd3e88a4cff.png) | ACKs for top commit: jonatack: Tested ACK 6971e79 ShaMan239: tACK 6971e79 hebasto: ACK 6971e79, tested on Linux Mint 20.2 (Qt 5.12.8). Tree-SHA512: 9716cdedd435f88245a097fed6d4b2b486104d0dd09df739bdb4f2bfad709cbd9c9a231168cc3326e94fa5fddc77dd68f992f20417d04d94930db9fccdbb7de1

4832737 qt: connection type translator comments (Jarol Rodriguez) Pull request description: This PR introduces Qt translator comments for `Connection Type` strings in `guiutil.cpp` as well as `rpcconsole.cpp`. This is an alternate implementation of the idea presented in the last three commits of #289. It is especially inspired by commit 842f4e8. Per [Qt Dev Notes](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Developer-Notes-for-Qt-Code), it is better to not break up strings when not necessary. This way we preserve the full context for translators. ACKs for top commit: jonatack: Code review re-ACK 4832737 per `git diff 371e2b9 4832737`, changes are translator comment edits since my review yesterday (thank you for updating) hebasto: ACK 4832737 Tree-SHA512: 67e1741e10a2e30cde6d50d3293eec89f0b7641b34463865dc6909d2926cdcf33a7d8c1dc8055d2f85906ad2002cdaa594d37b184d16e2f06614b6c5ad00c982

jonatack mentioned this pull request Apr 23, 2021

gui: Node appears as Unroutable in peers list bitcoin/bitcoin#21747

Closed

jarolrod reviewed Apr 23, 2021

View reviewed changes

src/qt/peertablemodel.cpp Outdated Show resolved Hide resolved

jonatack force-pushed the add-direction-column-to-peers-tab branch from d508c3f to 8c580ef Compare April 24, 2021 11:41

jonatack force-pushed the add-direction-column-to-peers-tab branch from 8c580ef to d04ebda Compare April 25, 2021 13:43

Talkless approved these changes Apr 25, 2021

View reviewed changes

src/qt/rpcconsole.cpp Outdated Show resolved Hide resolved

jonatack force-pushed the add-direction-column-to-peers-tab branch from d04ebda to cee6761 Compare April 25, 2021 15:38

hebasto added the Design label Apr 25, 2021

Talkless reviewed Apr 25, 2021

View reviewed changes

DrahtBot added the Needs rebase label Apr 28, 2021

gui: add Direction column to peers tab

678b572

jonatack force-pushed the add-direction-column-to-peers-tab branch from cee6761 to 181065c Compare April 28, 2021 19:19

DrahtBot removed the Needs rebase label Apr 28, 2021

jonatack added 3 commits April 29, 2021 01:17

gui: improve connection type translations and translator comments

842f4e8

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

ad92e04

gui: add missing QLatin1String, QStringBuilder headers

2bf3f5a

jonatack force-pushed the add-direction-column-to-peers-tab branch from 181065c to 2bf3f5a Compare April 28, 2021 23:28

ghost reviewed Apr 30, 2021

View reviewed changes

jarolrod reviewed Apr 30, 2021

View reviewed changes

jonatack closed this Apr 30, 2021

hebasto added the Up for grabs label Apr 30, 2021

jonatack deleted the add-direction-column-to-peers-tab branch April 30, 2021 23:31

jarolrod mentioned this pull request May 5, 2021

Add Direction column to Peers Tab #317

Merged

hebasto added UI All about "look and feel" UX All about "how to get things done" and removed Design Up for grabs UI All about "look and feel" labels May 26, 2021

jarolrod mentioned this pull request May 27, 2021

Connection Type Translator Comments #345

Merged

jonatack mentioned this pull request Jun 26, 2021

Remove "Type" column from peers window #372

Closed

hebasto mentioned this pull request Aug 8, 2021

Summary of Direction/Type columns problem and a new suggested trade-off #401

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "Direction" column to peers tab #289

Add "Direction" column to peers tab #289

jonatack commented Apr 23, 2021 •

edited

Loading

jarolrod left a comment

jonatack commented Apr 24, 2021 •

edited

Loading

RandyMcMillan commented Apr 24, 2021

jonatack commented Apr 24, 2021

RandyMcMillan commented Apr 24, 2021

jonatack commented Apr 25, 2021

jonatack commented Apr 25, 2021

Talkless left a comment

Talkless Apr 25, 2021

jonatack Apr 25, 2021

Talkless Apr 26, 2021

jonatack Apr 28, 2021

Talkless Apr 25, 2021

jonatack Apr 25, 2021

Talkless Apr 26, 2021

jonatack Apr 28, 2021

jonatack commented Apr 28, 2021

ghost commented Apr 30, 2021

ghost Apr 30, 2021

jarolrod Apr 30, 2021

jonatack Apr 30, 2021

jonatack Apr 30, 2021

ghost commented Apr 30, 2021

jarolrod left a comment

jonatack commented Apr 30, 2021

hebasto commented Apr 30, 2021

jonatack commented Apr 30, 2021

ghost commented May 2, 2021

hebasto commented May 2, 2021

jonatack commented May 2, 2021

jarolrod commented May 26, 2021

Add "Direction" column to peers tab #289

Add "Direction" column to peers tab #289

Conversation

jonatack commented Apr 23, 2021 • edited Loading

jarolrod left a comment

Choose a reason for hiding this comment

jonatack commented Apr 24, 2021 • edited Loading

RandyMcMillan commented Apr 24, 2021

jonatack commented Apr 24, 2021

RandyMcMillan commented Apr 24, 2021

jonatack commented Apr 25, 2021

jonatack commented Apr 25, 2021

Talkless left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonatack commented Apr 28, 2021

ghost commented Apr 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Apr 30, 2021

jarolrod left a comment

Choose a reason for hiding this comment

jonatack commented Apr 30, 2021

hebasto commented Apr 30, 2021

jonatack commented Apr 30, 2021

ghost commented May 2, 2021

hebasto commented May 2, 2021

jonatack commented May 2, 2021

jarolrod commented May 26, 2021

jonatack commented Apr 23, 2021 •

edited

Loading

jonatack commented Apr 24, 2021 •

edited

Loading