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

[luke-jr]

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.

[ghost]

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

[jarolrod]

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:

diff --git a/src/qt/peertablemodel.cpp b/src/qt/peertablemodel.cpp
index 9188cc32f..d20de25f5 100644
--- a/src/qt/peertablemodel.cpp
+++ b/src/qt/peertablemodel.cpp
@@ -73,9 +73,14 @@ QVariant PeerTableModel::data(const QModelIndex& index, int role) const
         switch (column) {
         case NetNodeId:
             return (qint64)rec->nodeStats.nodeid;
+        case Direction:
+            return QString(rec->nodeStats.fInbound ?
+                               //: An Inbound Connection from a Peer.
+                               "↓ " + tr("IN") :
+                               //: An Outbound Connection to a Peer.
+                               "↑ " + tr("OUT"));
         case Address:
-            // prepend to peer address down-arrow symbol for inbound connection and up-arrow for outbound connection
-            return QString::fromStdString((rec->nodeStats.fInbound ? "↓ " : "↑ ") + rec->nodeStats.addrName);
+            return QString::fromStdString(rec->nodeStats.addrName);
         case ConnectionType:
             return GUIUtil::ConnectionTypeToQString(rec->nodeStats.m_conn_type, /* prepend_direction */ false);
         case Network:
@@ -94,6 +99,7 @@ QVariant PeerTableModel::data(const QModelIndex& index, int role) const
         switch (column) {
         case NetNodeId:
             return QVariant(Qt::AlignRight | Qt::AlignVCenter);
+        case Direction:
         case Address:
             return {};
         case ConnectionType:
diff --git a/src/qt/peertablemodel.h b/src/qt/peertablemodel.h
index a02472550..07e2ebfc1 100644
--- a/src/qt/peertablemodel.h
+++ b/src/qt/peertablemodel.h
@@ -48,6 +48,7 @@ public:
 
     enum ColumnIndex {
         NetNodeId = 0,
+        Direction,
         Address,
         ConnectionType,
         Network,
@@ -86,6 +87,9 @@ private:
         /*: Title of Peers Table column which contains a
             unique number used to identify a connection. */
         tr("Peer"),
+        /*: Title of Peers Table Column which indicates
+            the direction of the peer connection. */
+        tr("Direction"),
         /*: Title of Peers Table column which contains the
             IP/Onion/I2P address of the connected peer. */
         tr("Address"),
diff --git a/src/qt/peertablesortproxy.cpp b/src/qt/peertablesortproxy.cpp
index 78932da8d..f92eef48f 100644
--- a/src/qt/peertablesortproxy.cpp
+++ b/src/qt/peertablesortproxy.cpp
@@ -26,6 +26,8 @@ bool PeerTableSortProxy::lessThan(const QModelIndex& left_index, const QModelInd
         return left_stats.nodeid < right_stats.nodeid;
     case PeerTableModel::Address:
         return left_stats.addrName.compare(right_stats.addrName) < 0;
+    case PeerTableModel::Direction:
+        return left_stats.fInbound > right_stats.fInbound; // default sort Inbound, then Outbound
     case PeerTableModel::ConnectionType:
         return left_stats.m_conn_type < right_stats.m_conn_type;
     case PeerTableModel::Network:

Screenshots comparing patch sets:

This PR	PR Alternative Diff

[rebroad]

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]

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]

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

[jarolrod]

I'm NACK on this

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

[shaavan]

Concept 0
Tested 41c881c 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.

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

1. The left and right arrows, do not have the same strong sense of incoming and outgoing as the up and down arrows.
2. 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.
3. The arrows don’t look aesthetically pleasing. I would aesthetics of up and down arrows as suggested here (under the PR alternative heading)

[shaavan]

@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]

Rebased and description updated to current state of master

[hebasto]

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

[DrahtBot]

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]

🐙 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".}

[hebasto]

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.