Peers window: Show direction in a new column, with clearer icon

luke-jr commented at 8:59 pm on June 12, 2021: member

Replaces the space-wasting “Inbound”/“Outbound” strings with left/right pointing to/from the address, and a tiny text label (which can be translated) overlayed.

luke-jr cross-referenced this on Jun 12, 2021 from issue Add Direction column to Peers Tab by jarolrod

jarolrod added the label UX on Jun 13, 2021

ghost commented at 9:45 am on June 14, 2021: none

Would be helpful to attach screenshot before/after code change.

in src/qt/peertablemodel.cpp:135 in c55a2e7d8d outdated

128+                CheckSize(icon_out, label_out, /* align_right= */ true)) break;
129+        }
130+        icon_in_painter .drawText(0, 0, SIZE, SIZE, Qt::AlignLeft  | Qt::AlignBottom, label_in);
131+        icon_out_painter.drawText(0, 0, SIZE, SIZE, Qt::AlignRight | Qt::AlignBottom, label_out);
132+    }
133+    m_icon_conn_in  = m_platform_style.TextColorIcon(QIcon(QPixmap::fromImage(icon_in)));

jarolrod commented at 7:46 pm on June 17, 2021:

This patchset does not work with macOS theme switching. You need to actually watch for a change event and repaint the icon/text according to the changed platform style.

Start	Switch To Dark

Start	Switch To Light

luke-jr commented at 0:01 am on June 22, 2021:

Fixed, I think (no way to test)

in src/qt/rpcconsole.cpp:671 in c55a2e7d8d outdated

666@@ -666,7 +667,13 @@ void RPCConsole::setClientModel(ClientModel *model, int bestblock_height, int64_
667         ui->peerWidget->setContextMenuPolicy(Qt::CustomContextMenu);
668 
669         if (!ui->peerWidget->horizontalHeader()->restoreState(m_peer_widget_header_state)) {
670+            const QFontMetrics fm = ui->peerWidget->fontMetrics();
671+            ui->peerWidget->setColumnWidth(PeerTableModel::NetNodeId, GUIUtil::TextWidth(fm, "99999"));

jarolrod commented at 7:46 pm on June 17, 2021:

in c55a2e7d8da7ced22f517447668681e1453a7803, there’s no good reason to touch the other table widths. Should just supply the new Direction column width.

Additionally, this line:

0ui->peerWidget->setColumnWidth(PeerTableModel::NetNodeId, GUIUtil::TextWidth(fm, "99999"));

makes it so that the Peer column is shown with the word Peer slightly truncated

luke-jr commented at 0:02 am on June 22, 2021:

Reduced and squashed into previous commit

in src/qt/peertablemodel.h:54 in c55a2e7d8d outdated

49     void stopAutoRefresh();
50 
51+    // See also RPCConsole::ColumnWidths in rpcconsole.h
52     enum ColumnIndex {
53         NetNodeId = 0,
54+        Direction,

jarolrod commented at 8:07 pm on June 17, 2021:

PR description states:

Uses a new column so sorting is possible.

it’s possible, but this PR doesn’t implement it.

Because this PR doesn’t implement it, the GUI will crash if you click on this column header:

 0Application Specific Information:
 1Assertion failed: (false), function lessThan, file qt/peertablesortproxy.cpp, line 42.
 2 
 3
 4Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
 50   libsystem_kernel.dylib        	0x00007fff2031e946 __pthread_kill + 10
 61   libsystem_pthread.dylib       	0x00007fff2034d615 pthread_kill + 263
 72   libsystem_c.dylib             	0x00007fff202a2411 abort + 120
 83   libsystem_c.dylib             	0x00007fff202a17e8 __assert_rtn + 314
 94   bitcoin-qt                    	0x000000010ca378d6 PeerTableSortProxy::lessThan(QModelIndex const&, QModelIndex const&) const + 918 (peertablesortproxy.cpp:42)
105   org.qt-project.QtCore         	0x000000010dfbf56e 0x10ddf9000 + 1860974
116   org.qt-project.QtCore         	0x000000010dfb3c4d 0x10ddf9000 + 1813581
127   org.qt-project.QtCore         	0x000000010dfb4308 0x10ddf9000 + 1815304
138   org.qt-project.QtCore         	0x000000

Here’s a diff to implement sorting for this column:

 0diff --git a/src/qt/peertablesortproxy.cpp b/src/qt/peertablesortproxy.cpp
 1index 78932da8d..dd29ddd28 100644
 2--- a/src/qt/peertablesortproxy.cpp
 3+++ b/src/qt/peertablesortproxy.cpp
 4@@ -24,6 +24,8 @@ bool PeerTableSortProxy::lessThan(const QModelIndex& left_index, const QModelInd
 5     switch (static_cast<PeerTableModel::ColumnIndex>(left_index.column())) {
 6     case PeerTableModel::NetNodeId:
 7         return left_stats.nodeid < right_stats.nodeid;
 8+    case PeerTableModel::Direction:
 9+        return left_stats.fInbound > right_stats.fInbound; // default sort Inbound, then Outbound
10     case PeerTableModel::Address:
11         return left_stats.addrName.compare(right_stats.addrName) < 0;
12     case PeerTableModel::ConnectionType:

luke-jr commented at 0:03 am on June 22, 2021:

Apparently I already fixed this but forgot to push, oops :)

(Pushed along with other fixes)

in src/qt/peertablemodel.cpp:70 in c55a2e7d8d outdated

65+    };
66+    DrawArrow(0, icon_in_painter);
67+    DrawArrow(SIZE-1, icon_out_painter);
68+
69+    {
70+        const QString label_in  = tr("IN" , "Label on inbound connection icon");

jarolrod commented at 8:16 pm on June 17, 2021:

We do not use disambiguation strings anymore. This would need to be made into a translator comment.

diff for Line 70 and 71:

 0
 1diff --git a/src/qt/peertablemodel.cpp b/src/qt/peertablemodel.cpp
 2index 58998781c..8f65f3e75 100644
 3--- a/src/qt/peertablemodel.cpp
 4+++ b/src/qt/peertablemodel.cpp
 5@@ -67,8 +67,10 @@ void PeerTableModel::DrawIcons()
 6     DrawArrow(SIZE-1, icon_out_painter);
 7 
 8     {
 9-        const QString label_in  = tr("IN" , "Label on inbound connection icon");
10-        const QString label_out = tr("OUT", "Label on outbound connection icon");
11+        //: Label on inbound connection icon
12+        const QString label_in  = tr("IN");
13+        //: Label on outbound connection icon
14+        const QString label_out = tr("OUT");
15         QImage scratch(SIZE, SIZE, QImage::Format_Alpha8);
16         QPainter scratch_painter(&scratch);
17         QFont font;  // NOTE: Application default font

luke-jr commented at 0:03 am on June 22, 2021:

k

jarolrod commented at 8:31 pm on June 17, 2021: member

I don’t agree with this patchset and I think #317 is the way to go. If it appeases everyone, #317 can keep the arrows next to the address.

Some notes: This patchset is quite complicated for what it seeks to accomplish. It can be condensed down into this diff:

 0diff --git a/src/qt/peertablemodel.cpp b/src/qt/peertablemodel.cpp
 1index 9188cc32f..d20de25f5 100644
 2--- a/src/qt/peertablemodel.cpp
 3+++ b/src/qt/peertablemodel.cpp
 4@@ -73,9 +73,14 @@ QVariant PeerTableModel::data(const QModelIndex& index, int role) const
 5         switch (column) {
 6         case NetNodeId:
 7             return (qint64)rec->nodeStats.nodeid;
 8+        case Direction:
 9+            return QString(rec->nodeStats.fInbound ?
10+                               //: An Inbound Connection from a Peer.
11+                               "↓ " + tr("IN") :
12+                               //: An Outbound Connection to a Peer.
13+                               "↑ " + tr("OUT"));
14         case Address:
15-            // prepend to peer address down-arrow symbol for inbound connection and up-arrow for outbound connection
16-            return QString::fromStdString((rec->nodeStats.fInbound ? "↓ " : "↑ ") + rec->nodeStats.addrName);
17+            return QString::fromStdString(rec->nodeStats.addrName);
18         case ConnectionType:
19             return GUIUtil::ConnectionTypeToQString(rec->nodeStats.m_conn_type, /* prepend_direction */ false);
20         case Network:
21@@ -94,6 +99,7 @@ QVariant PeerTableModel::data(const QModelIndex& index, int role) const
22         switch (column) {
23         case NetNodeId:
24             return QVariant(Qt::AlignRight | Qt::AlignVCenter);
25+        case Direction:
26         case Address:
27             return {};
28         case ConnectionType:
29diff --git a/src/qt/peertablemodel.h b/src/qt/peertablemodel.h
30index a02472550..07e2ebfc1 100644
31--- a/src/qt/peertablemodel.h
32+++ b/src/qt/peertablemodel.h
33@@ -48,6 +48,7 @@ public:
34 
35     enum ColumnIndex {
36         NetNodeId = 0,
37+        Direction,
38         Address,
39         ConnectionType,
40         Network,
41@@ -86,6 +87,9 @@ private:
42         /*: Title of Peers Table column which contains a
43             unique number used to identify a connection. */
44         tr("Peer"),
45+        /*: Title of Peers Table Column which indicates
46+            the direction of the peer connection. */
47+        tr("Direction"),
48         /*: Title of Peers Table column which contains the
49             IP/Onion/I2P address of the connected peer. */
50         tr("Address"),
51diff --git a/src/qt/peertablesortproxy.cpp b/src/qt/peertablesortproxy.cpp
52index 78932da8d..f92eef48f 100644
53--- a/src/qt/peertablesortproxy.cpp
54+++ b/src/qt/peertablesortproxy.cpp
55@@ -26,6 +26,8 @@ bool PeerTableSortProxy::lessThan(const QModelIndex& left_index, const QModelInd
56         return left_stats.nodeid < right_stats.nodeid;
57     case PeerTableModel::Address:
58         return left_stats.addrName.compare(right_stats.addrName) < 0;
59+    case PeerTableModel::Direction:
60+        return left_stats.fInbound > right_stats.fInbound; // default sort Inbound, then Outbound
61     case PeerTableModel::ConnectionType:
62         return left_stats.m_conn_type < right_stats.m_conn_type;
63     case PeerTableModel::Network:

Screenshots comparing patch sets:

This PR	PR Alternative Diff

luke-jr force-pushed on Jun 22, 2021

rebroad commented at 2:20 pm on June 26, 2021: contributor

NACK - takes up more space - left and right don’t obviously correspond to in or out, unless it’s in the same column as something and then perhaps it would.

jonatack commented at 3:51 pm on June 26, 2021: contributor

The Direction column needs to be right before the Type column because Direction and Type are the two aspects of the current connection type naming, e.g. Outbound Full Relay, etc. It also should be “Inbound” or “Outbound” per the same naming. Another option is to drop the Type column completely, which I reckon is the wrong direction but the current state is confusing to users who have reported it as a bug.

luke-jr commented at 3:54 pm on June 26, 2021: member

@rebroad Left/right point away/toward the IP column. There’s also the text part to avoid any uncertainty.

jarolrod commented at 3:25 am on July 6, 2021: member

I’m NACK on this

Can you elaborate on why you would want this approach versus #317?

shaavan commented at 6:16 pm on August 2, 2021: contributor

Concept 0 Tested 41c881c8a780f19f7e5a7d01f67684c14609afe1 on Ubuntu 20.04

This PR is adding the left and right arrows as a representation of if the signal is incoming and outgoing. This PR also removes the up and down arrows from the address column. This is achieved by adding a new column with custom-drawn arrows for the left and right directions. And simplifying the address column to just displaying address instead of both address and direction. The PR is successful in doing what it says. Here is the screenshot of the PR’s peertable. I have compiled the PR and ran it on the signet chain. Screenshot from 2021-08-02 23-22-02

I can see how op is approaching this problem. However, I prefer its alternative #317 over this PR. The reasons being:

The left and right arrows, do not have the same strong sense of incoming and outgoing as the up and down arrows.
There is no particular heading for the arrow column in this PR. Which could lead to confusion for a new user for the application of this column. Basically, it would decrease the user’s experience.
The arrows don’t look aesthetically pleasing. I would aesthetics of up and down arrows as suggested here (under the PR alternative heading)

hebasto cross-referenced this on Aug 7, 2021 from issue Peer address column sorting does not honor the inbound/outbound arrow prefix by ghost

hebasto cross-referenced this on Aug 8, 2021 from issue Summary of Direction/Type columns problem and a new suggested trade-off by hebasto

DrahtBot added the label Needs rebase on Aug 11, 2021

shaavan commented at 2:43 pm on August 14, 2021: contributor

@luke-jr, @hebasto. Just a reminder. One of the alternatives of this PR, #317, has been merged. So, can this PR be closed?

luke-jr force-pushed on Aug 31, 2021

luke-jr commented at 11:37 pm on August 31, 2021: member

Rebased and description updated to current state of master

DrahtBot removed the label Needs rebase on Sep 1, 2021

hebasto commented at 6:50 pm on September 7, 2021: member

Would be helpful to attach screenshot before/after code change.

DrahtBot added the label Needs rebase on Dec 23, 2021

luke-jr force-pushed on Apr 21, 2022

GUI: Pass PlatformStyle through ClientModel into PeerTableModel bab1c9a0a4

GUI: Peers: Add direction table column before "Address" with arrow

Arrow icon drawn at startup to make text translatable

262f4fd088

GUI: Make Peers table aware of runtime palette change

This change fixes the GUI when changing appearance on macOS.

4e2fe6b987

luke-jr force-pushed on Apr 21, 2022

DrahtBot removed the label Needs rebase on Apr 22, 2022

DrahtBot cross-referenced this on May 20, 2022 from issue Unify bitcoin-qt and bitcoind persistent settings by ryanofsky

DrahtBot cross-referenced this on May 20, 2022 from issue refactor: Pass interfaces::Node references to OptionsModel constructor by ryanofsky

DrahtBot commented at 8:54 am on May 20, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#602 (Unify bitcoin-qt and bitcoind persistent settings by ryanofsky)
#601 (refactor: Pass interfaces::Node references to OptionsModel constructor by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot cross-referenced this on May 21, 2022 from issue Add settings.json prune-prev, proxy-prev, onion-prev settings by ryanofsky

DrahtBot commented at 9:23 am on May 24, 2022: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

DrahtBot added the label Needs rebase on May 24, 2022

hebasto commented at 3:44 pm on October 26, 2022: member

Closing this as it has not had any activity in a while. If you are interested in continuing work on this, please leave a comment so that it can be reopened.

hebasto closed this on Oct 26, 2022

bitcoin-core locked this on Oct 26, 2023

Peers window: Show direction in a new column, with clearer icon #363

Conflicts