Add network to peers window and peer details #162

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

Concept ACK.

tACK b1704ead912ed813eaa08e80af5e14a2c087574e

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

<img width="812" alt="Schermafbeelding 2020-12-24 om 12 29 34" src="https://user-images.githubusercontent.com/10217/103086035-b7e40980-45e3-11eb-9fa6-f483b6f4696e.png">

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

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

Thanks @Sjors. Good idea, done.

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

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

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.

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

(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 :)

ACK 67e8b636a88daf47acf01f04e1e1b62b6f200554

@jonatack Could this patch make things look better

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

?

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

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

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.

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

Looks great:

Also I've verified that the "network" field in the getpeerinfo RPC has not changed behavior.

With this patch

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

I've verified that items in the "Network" column are sorted in lexicographical order:

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

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.

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

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

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.

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

ACK e262a19b0ba07d8d2334551f49ca1577ab2999fc

post-merge ACK e262a19b0ba07d8d2334551f49ca1577ab2999fc