Add network to peers window and peer details

jonatack commented at 10:54 am on December 24, 2020: contributor

and rename peers window column headers from NodeId and Node/Service to Peer Id and Address.

Screenshot from 2020-12-27 14-45-31

hebasto commented at 10:56 am on December 24, 2020: member

Concept ACK.

Sjors commented at 11:30 am on December 24, 2020: member

tACK b1704ead912ed813eaa08e80af5e14a2c087574e

Especially handy because IPv6 addresses are turned into ellipses until you expand it:

This column might be a more natural place for the inbound/outbound arrow.

MarcoFalke renamed this:
~~gui: add network to peers window and in peer details~~
add network to peers window and in peer details
on Dec 24, 2020

jonatack renamed this:
~~add network to peers window and in peer details~~
Add network to peers window and peer details
on Dec 24, 2020

jonatack commented at 12:33 pm on December 24, 2020: contributor

This column might be a more natural place for the inbound/outbound arrow.

Thanks @Sjors. Good idea, done.

RandyMcMillan commented at 1:13 pm on December 24, 2020: contributor

@jonatack

#135

In this PR the direction has its own column. This makes the direction sortable. Having the network type and the direction individually sortable would be great.

Note: I found that offsetting the arrow (based on direction) is a useful visual cue. And this makes seeing the direction (at a glance) easier.

jonatack commented at 1:47 pm on December 24, 2020: contributor

@RandyMcMillan not a problem, will just drop the last commit if that PR is merged

in src/qt/forms/debugwindow.ui:1106 in 79e997e826 outdated

1099@@ -1100,13 +1100,39 @@
1100                 </widget>
1101                </item>
1102                <item row="2" column="0">
1103+                <widget class="QLabel" name="peerNetworkLabel">
1104+                 <property name="toolTip">
1105+                  <string>The network protocol this peer is connected through.</string>
1106+                 </property>

hebasto commented at 10:18 am on December 25, 2020:

b1704ead912ed813eaa08e80af5e14a2c087574e

It is inconsistent to have a tooltip only for this label among others.

jonatack commented at 12:20 pm on December 25, 2020:

There is one for the Mapped AS field that was added in https://github.com/bitcoin/bitcoin/pull/18402. Probably best to add some of the missing ones but not in this pull.

in src/qt/guiutil.cpp:766 in 79e997e826 outdated

754+    if (network == "ipv4") return "IPv4";
755+    if (network == "ipv6") return "IPv6";
756+    if (network == "onion") return "Onion";
757+    return "";
758+}
759+

hebasto commented at 10:39 am on December 25, 2020:

97d5b82beaf35761bfcc31562f525f4bc52d6529

It seems strange to me to add a new function for the sake of a such functionality. And string interface is fragile.

Why do not expose Network in CNodeStats (the same approach you use in #163)?

jonatack commented at 12:22 pm on December 25, 2020:

Agreed, I wanted to both here and in #163 but I didn’t think it would be accepted by reviewers. Updating to switch on the enum instead and add i2p and cjdns.

jonatack commented at 2:17 pm on December 25, 2020:

done – I agree it’s a shame to add a function but the GUI UI is non all-lowercase like the RPC/CLI interface

in src/qt/peertablemodel.cpp:109 in 79e997e826 outdated

105@@ -104,7 +106,7 @@ PeerTableModel::PeerTableModel(interfaces::Node& node, QObject* parent) :
106     m_node(node),
107     timer(nullptr)
108 {
109-    columns << tr("NodeId") << tr("Node/Service") << tr("Ping") << tr("Sent") << tr("Received") << tr("User Agent");
110+    columns << tr("Node Id") << tr("Node/Service") << tr("Network") << tr("Ping") << tr("Sent") << tr("Received") << tr("User Agent");

hebasto commented at 10:48 am on December 25, 2020:

c772533ce5f471b5aa968ac864cf8a2173900fac As you touching the headers, mind considering the following patch

 0--- a/src/qt/peertablemodel.cpp
 1+++ b/src/qt/peertablemodel.cpp
 2@@ -106,7 +106,6 @@ PeerTableModel::PeerTableModel(interfaces::Node& node, QObject* parent) :
 3     m_node(node),
 4     timer(nullptr)
 5 {
 6-    columns << tr("Node Id") << tr("Node/Service") << tr("Network") << tr("Ping") << tr("Sent") << tr("Received") << tr("User Agent");
 7     priv.reset(new PeerTablePriv());
 8 
 9     // set up timer for auto refresh
10diff --git a/src/qt/peertablemodel.h b/src/qt/peertablemodel.h
11index 39cbe16df..61a0132e0 100644
12--- a/src/qt/peertablemodel.h
13+++ b/src/qt/peertablemodel.h
14@@ -83,7 +83,7 @@ public Q_SLOTS:
15 
16 private:
17     interfaces::Node& m_node;
18-    QStringList columns;
19+    const QStringList columns{tr("Peer Id"), tr("Address"), tr("Network"), tr("Ping"), tr("Sent"), tr("Received"), tr("User Agent")};
20     std::unique_ptr<PeerTablePriv> priv;
21     QTimer *timer;
22 };

?

jonatack commented at 2:16 pm on December 25, 2020:

sure, done in c2cd8267cb21d496bcf66070b3b9b18d5f181ea5 (can drop the clang-formatting changes in that commit but some of the conditionals have dangerous, error-prone formatting)

hebasto changes_requested

hebasto commented at 10:49 am on December 25, 2020: member

I have reviewed the code (79e997e826b93595896839fd513a052b7a4d001d), and tested it.

UX notes.

Disclaimer: I’m not a design expert, and my opinions are based exclusively on my experience.

In this PR the direction has its own column. This makes the direction sortable. Having the network type and the direction individually sortable would be great.

I’ve been waiting for this feature for many years :)

This column might be a more natural place for the inbound/outbound arrow.

Thanks @Sjors. Good idea, done.

I don’t think this change is valuable. It is not possible to sort out all connections through a particular network.

jonatack force-pushed on Dec 25, 2020

jonatack commented at 2:23 pm on December 25, 2020: contributor

Thanks for the feedback. I dropped the arrows commit (I think there is a better approach possible) and took most of @hebasto’s suggestions. (Can drop the clang-formatting changes in the refactoring commit but some of the conditionals have error-prone formatting.)

hebasto commented at 2:56 pm on December 25, 2020: member

(Can drop the clang-formatting changes in the refactoring commit but some of the conditionals have error-prone formatting.)

The most of the re-formatted code could go away in #18 :)

in src/qt/peertablemodel.cpp:40 in c2cd8267cb outdated

44     case PeerTableModel::Received:
45         return pLeft->nRecvBytes < pRight->nRecvBytes;
46-    }
47+    case PeerTableModel::Subversion:
48+        return pLeft->cleanSubVer.compare(pRight->cleanSubVer) < 0;
49+    } // no default case, so the compiler can warn about missing cases

hebasto commented at 2:58 pm on December 25, 2020:

I think this comment is not true, as the switch parameter is an int, not a enum.

jonatack commented at 3:07 pm on December 25, 2020:

You’re right–thanks!

in src/qt/guiutil.cpp:755 in 67e8b636a8 outdated

748@@ -749,6 +749,21 @@ QString boostPathToQString(const fs::path &path)
749     return QString::fromStdString(path.string());
750 }
751 
752+QString NetworkToQString(Network net)
753+{
754+    switch (net) {
755+    case NET_UNROUTABLE: return "Unroutable";

hebasto commented at 3:02 pm on December 25, 2020:

0    case NET_UNROUTABLE: return tr("Unroutable");

jonatack commented at 3:24 pm on December 25, 2020:

thanks, done QObject::tr("Unroutable")

in src/qt/guiutil.cpp:761 in 67e8b636a8 outdated

756+    case NET_IPV4: return "IPv4";
757+    case NET_IPV6: return "IPv6";
758+    case NET_ONION: return "Onion";
759+    case NET_I2P: return "I2P";
760+    case NET_CJDNS: return "CJDNS";
761+    case NET_INTERNAL: return "Internal";

hebasto commented at 3:02 pm on December 25, 2020:

0    case NET_INTERNAL: return tr("Internal");

jonatack commented at 3:24 pm on December 25, 2020:

thanks, done

hebasto approved

hebasto commented at 3:03 pm on December 25, 2020: member

ACK 67e8b636a88daf47acf01f04e1e1b62b6f200554

hebasto commented at 3:16 pm on December 25, 2020: member

@jonatack Could this patch make things look better

 0--- a/src/qt/peertablemodel.cpp
 1+++ b/src/qt/peertablemodel.cpp
 2@@ -168,6 +168,8 @@ QVariant PeerTableModel::data(const QModelIndex& index, int role) const
 3         }
 4     } else if (role == Qt::TextAlignmentRole) {
 5         switch (index.column()) {
 6+        case Network:
 7+            return QVariant(Qt::AlignCenter);
 8         case Ping:
 9         case Sent:
10         case Received:

?

hebasto commented at 3:22 pm on December 25, 2020: member

Due to the header titles are changed this patch seems required:

 0--- a/src/qt/rpcconsole.cpp
 1+++ b/src/qt/rpcconsole.cpp
 2@@ -1092,7 +1092,7 @@ void RPCConsole::updateDetailWidget()
 3     const CNodeCombinedStats *stats = clientModel->getPeerTableModel()->getNodeStats(selected_rows.first().row());
 4     // update the detail ui with latest node information
 5     QString peerAddrDetails(QString::fromStdString(stats->nodeStats.addrName) + " ");
 6-    peerAddrDetails += tr("(node id: %1)").arg(QString::number(stats->nodeStats.nodeid));
 7+    peerAddrDetails += tr("(peer id: %1)").arg(QString::number(stats->nodeStats.nodeid));
 8     if (!stats->nodeStats.addrLocal.empty())
 9         peerAddrDetails += "<br />" + tr("via %1").arg(QString::fromStdString(stats->nodeStats.addrLocal));
10     ui->peerHeading->setText(peerAddrDetails);

jonatack force-pushed on Dec 25, 2020

jonatack commented at 4:22 pm on December 25, 2020: contributor

Thanks for the good feedback @hebasto. Added translation, changed the network column align to center, added a commit in your name for the header renaming + initialize columns in the .h, fixed the broken doxygen formatting, dropped the clang formatting, and improved the network tooltip. Updated the PR description and screenshot.

hebasto approved

hebasto commented at 6:53 pm on December 25, 2020: member

ACK 12212e2b30b87110f81787d6e10259d02d807279, tested on Linux Mint 20 (x86_64) + Qt 5.12.8.

Looks great: Screenshot from 2020-12-25 20-26-53

Also I’ve verified that the “network” field in the getpeerinfo RPC has not changed behavior.

With this patch

 0--- a/src/net.cpp
 1+++ b/src/net.cpp
 2@@ -566,7 +566,7 @@ void CNode::copyStats(CNodeStats &stats, const std::vector<bool> &m_asmap)
 3     X(nServices);
 4     X(addr);
 5     X(addrBind);
 6-    stats.m_network = ConnectedThroughNetwork();
 7+    stats.m_network = static_cast<enum Network>(stats.nodeid % NET_MAX);
 8     stats.m_network_name = GetNetworkName(stats.m_network);
 9     stats.m_mapped_as = addr.GetMappedAS(m_asmap);
10     if (m_tx_relay != nullptr) {

I’ve verified that items in the “Network” column are sorted in lexicographical order: Screenshot from 2020-12-25 21-05-16

TBH, I don’t like it, but it could be changed in a followup.

jonatack commented at 7:07 pm on December 25, 2020: contributor

Thanks! The sort order was deliberate–my thinking was that users expect this to be sorted by alphabetical order as there isn’t an obvious alternative sort expectation, so I went with sort by name rather than by enum id (which may be more what you’re thinking of, e.g. ipv4, then ipv6, then onion, then i2p, then cjdns, but that may violate the principle of least surprise to users). I don’t mind updating if there is consensus on sort by enum id.

hebasto commented at 7:17 pm on December 25, 2020: member

After #18 a filter feature could be easy added, and the sort order will do not matter :)

jonatack commented at 7:19 pm on December 25, 2020: contributor

SGTM, will review PRs 161 -> 18 ->164.

net, rpc: change CNodeStats::m_network from string to Network af9103cc79

gui: create GUIUtil::NetworkToQString() utility function 0d5613f9de

gui: fix broken doxygen formatting in src/qt/guiutil.h e0e55060bf

gui: add network column in peers tab/window 05c08c696a

gui: rename peer tab column headers, initialize in .h 9136953073

jonatack force-pushed on Dec 27, 2020

gui: display network in peer details e262a19b0b

jonatack force-pushed on Dec 27, 2020

jonatack commented at 1:42 pm on December 27, 2020: contributor

Updated the first commit to remove the now-redundant network string (same idea as @promag’s suggestion in the connection type pull) and updated the network column ordering by enum id rather than by network name (@hebasto I agree it’s better, though maybe more surprising to some users, see screenshot with your patch to show all the Network enum members). Will open this in the main repo as it now touches RPC code.

Screenshot from 2020-12-27 14-21-28

jonatack closed this on Dec 28, 2020

jonatack commented at 9:46 pm on December 28, 2020: contributor

Re-opening here, leaving the main repo pull closed. The changes are minor enough to not warrant moving it to the main repo.

jonatack reopened this on Dec 28, 2020

laanwj commented at 10:56 pm on December 28, 2020: member

ACK e262a19b0ba07d8d2334551f49ca1577ab2999fc

laanwj merged this on Dec 28, 2020

laanwj closed this on Dec 28, 2020

jonatack deleted the branch on Dec 28, 2020

in src/qt/forms/debugwindow.ui:1105 in e262a19b0b

1099@@ -1100,13 +1100,39 @@
1100                 </widget>
1101                </item>
1102                <item row="2" column="0">
1103+                <widget class="QLabel" name="peerNetworkLabel">
1104+                 <property name="toolTip">
1105+                  <string>The network protocol this peer is connected through: IPv4, IPv6, Onion, I2P, or CJDNS.</string>

hebasto commented at 4:41 pm on December 30, 2020:

The suggestion for a follow-up is to remove untranslatable network names from the translatable tooltip string.

hebasto commented at 4:41 pm on December 30, 2020: member

post-merge ACK e262a19b0ba07d8d2334551f49ca1577ab2999fc

bitcoin-core locked this on Dec 30, 2020

Add network to peers window and peer details #162