peers-tab: add connection duration column to tableview

RandyMcMillan commented at 2:14 am on February 3, 2022: contributor

This change adds an “Age” column to the peers table view, which displays the duration of each peer’s connection.

RandyMcMillan commented at 2:15 am on February 3, 2022: contributor

RandyMcMillan commented at 11:57 am on February 3, 2022: contributor

commit 363f181 - the column is resizing to accommodate 1 h 1 m 20 s time format.

Screen Shot 2022-02-03 at 7 52 49 AM

RandyMcMillan commented at 1:35 pm on February 3, 2022: contributor

commit 281b65b69e894bff136533770ee8eb57d53646a9

we make adjustments so that the peers tab collapses down to a useful “desktop widget”.

Screen Shot 2022-02-03 at 8 32 21 AM

RandyMcMillan commented at 1:41 pm on February 3, 2022: contributor

This PR and #540 will work well together. Both the network graph and peers tab will be useful in a minimized window size and present information in a well formatted way. Screen Shot 2022-02-03 at 8 40 42 AM

in src/qt/peertablemodel.h:95 in 281b65b69e outdated

90@@ -90,6 +91,9 @@ public Q_SLOTS:
91         tr("Direction"),
92         /*: Title of Peers Table column which describes the type of
93             peer connection. The "type" describes why the connection exists. */
94+        tr("Duration"),
95+        /*: Title of Peers Table column which describes the duration of

jarolrod commented at 6:14 am on February 4, 2022:

You added this translator comment in reverse, the comment on lines 95 and 96 should be above line 94 in this diff.

RandyMcMillan commented at 11:57 am on February 4, 2022:

Thanks for your attention on this.

jarolrod commented at 6:14 am on February 4, 2022: member

What is the reason to add this column to the peers table? Why is it a useful metric for a user of the GUI?

RandyMcMillan commented at 11:27 am on February 4, 2022: contributor

What is the reason to add this column to the peers table? Why is it a useful metric for a user of the GUI?

I guess one could ask - why peer id address direction type network ping sent received or user agent are useful.

And the fact that there is so much visual real estate NOT BEING USED FOR ANYTHING AT ALL - I am not even sure what the concern or hesitation to adding more columns to the table view would be. On wider screens more than half of the available real-estate is LITERALLY EMPTY SPACE.

Screen Shot 2022-02-04 at 6 09 10 AM

RandyMcMillan commented at 11:42 am on February 4, 2022: contributor

If - by your assertion - the connection duration is NOT USEFUL - why track it? Why not rip it out of the Client Model - a system could use that memory and resources for something else.

RandyMcMillan commented at 12:33 pm on February 4, 2022: contributor

The command line equivalent bitcoin-cli -netinfo 5 we report age to the user. So adding “Duration” to the gui tableview is a parallel to the command line report. Changing the column name to age seems more appropriate than debating whether or not it should be added. I follow the naming convention in the code using Duration GUIUtil::formatDurationStr(). Screen Shot 2022-02-04 at 7 22 57 AM

NOTE: I agree with using a 3 char column title in the command line equivalent - but in the gui - Duration is more appropriate IMO.

RandyMcMillan commented at 12:58 pm on February 4, 2022: contributor

Note: I like the idea of verbosity in the command line version. bitcoin-cli -netinfo bitcoin-cli -netinfo 1 bitcoin-cli -netinfo 2 etc…

If defending the nuance and minutia of gui design changes wasn’t so ridiculous (in this repo) - we could consider adding similar functionality to the gui (levels of verbosity to the table) - but I literally don’t get paid enough (approaching nothing) to deal with that type of design process unless every one else was already on board.

ghost commented at 6:49 pm on February 4, 2022: none

Concept ACK

The connection duration is a useful metric to see when viewing the peer table - sorting the tableview with the connection duration is made possible.

Duration is also useful combined with other things because users can see how much data was sent and received in that duration.

jonatack commented at 6:51 pm on February 4, 2022: contributor

Concept ACK. I do look frequently at age in -netinfo. The direction and type columns should remain together, as combined they form the connection type, like <-> and type in -netinfo, see enum class ConnectionType in src/net.h. I see age/duration as being next to the id column, like in -netinfo, or the data sent/recv ones as @prayank23 mentioned. It may be good to make the output less verbose somehow (seconds seems unneeded). If you use signet, you won’t need to black out sensitive parts of your screenshots.

Edit: the commits seem overly fine-grained, maybe combine the first three or four into one.

in src/qt/peertablemodel.h:94 in 16c9ebf5d4 outdated

88@@ -88,6 +89,9 @@ public Q_SLOTS:
89         /*: Title of Peers Table column which indicates the direction
90             the peer connection was initiated from. */
91         tr("Direction"),
92+        /*: Title of Peers Table column which indicates the duration (length of time)
93+            since the peer connection started. */
94+        tr("Duration"),

jonatack commented at 6:55 pm on February 4, 2022:

I like Age, but if you don’t, maybe consider Uptime.

RandyMcMillan commented at 2:55 am on February 5, 2022:

used Age as column name

in src/qt/rpcconsole.cpp:727 in 16c9ebf5d4 outdated

723@@ -723,6 +724,7 @@ void RPCConsole::setClientModel(ClientModel *model, int bestblock_height, int64_
724             ui->banlistWidget->setColumnWidth(BanTableModel::Address, BANSUBNET_COLUMN_WIDTH);
725             ui->banlistWidget->setColumnWidth(BanTableModel::Bantime, BANTIME_COLUMN_WIDTH);
726         }
727+        ui->banlistWidget->horizontalHeader()->setSectionResizeMode( 0, QHeaderView::ResizeToContents);

jonatack commented at 6:55 pm on February 4, 2022:

Here and line 694 as well

0        ui->banlistWidget->horizontalHeader()->setSectionResizeMode(0, QHeaderView::ResizeToContents);

RandyMcMillan commented at 2:55 am on February 5, 2022:

removed white space

RandyMcMillan commented at 11:24 pm on February 4, 2022: contributor

@jonatack - thanks for the constructive feedback - I appreciate your reasoning. I will re-implement accordingly.

RandyMcMillan force-pushed on Feb 5, 2022

RandyMcMillan commented at 2:57 am on February 5, 2022: contributor

commit 22b1a40 implements suggested changes. Squashed commits per feedback

RandyMcMillan commented at 3:14 am on February 5, 2022: contributor

Screenshots of 22b1a40

Screen Shot 2022-02-04 at 10 09 00 PM

If anybody wants to send a patch truncating the time format - I will gladly add it with attribution. Note: We could also wait - There are a couple things we could still add to the table view. For example we could add a version field - that lends itself to sorting…and we aren’t really short on column space…

Screen Shot 2022-02-04 at 10 31 44 PM

unknown approved

unknown commented at 6:45 am on February 5, 2022: none

utACK https://github.com/bitcoin-core/gui/pull/543/commits/22b1a40537ca7a62e271cb3962c04cdf2066edec

luke-jr commented at 10:37 pm on February 5, 2022: member

Concept ACK

I do note the sort will be the same as sorting by ID…

RandyMcMillan commented at 7:37 pm on February 8, 2022: contributor

@luke-jr - good observation - thanks again for your great feed back - (my apologize for the other thing - I will make the mempool-tab repo more “normal” 😄 )

jonatack commented at 9:07 pm on February 8, 2022: contributor

Debug build is clean and seems to be working. Can you update, squash and fix the commit names, descriptions, and code organization? i.e. make this ready for more final review.

It would be nice to remove the seconds display–seems noisy, distracting and unneeded. Nodes can be up for weeks, months or longer.

sorting the tableview with the connection duration is made possible.

Suggest removing the above from the pull description, as it is the same as the existing sort by peer id. Albeit with the current version, the sort orders of Peer and Age are inversed.

hebasto renamed this:
~~gui: peers-tab: add connection duration column to tableview~~
peers-tab: add connection duration column to tableview
on Feb 9, 2022

hebasto added the label Feature on Feb 9, 2022

RandyMcMillan force-pushed on Feb 10, 2022

RandyMcMillan force-pushed on Feb 11, 2022

RandyMcMillan commented at 4:38 am on February 11, 2022: contributor

Commit: d01c0b36d11ebd761054bec2c51599ee141238fc

Cleaned up commit structure and messages. The Age column now displays the connection duration in seconds for the first minute - then switches to hour/minute format when connection age is greater than 60 seconds.

Screen Shot 2022-02-10 at 11 34 03 PM

in src/qt/peertablemodel.cpp:71 in d01c0b36d1 outdated

64@@ -65,12 +65,19 @@ QVariant PeerTableModel::data(const QModelIndex& index, int role) const
65         return QVariant();
66 
67     CNodeCombinedStats *rec = static_cast<CNodeCombinedStats*>(index.internalPointer());
68-
69+    const auto time_now{GetTime<std::chrono::seconds>()};

jonatack commented at 12:39 pm on February 11, 2022:

0af493d include chrono

0--- a/src/qt/peertablemodel.cpp
1+++ b/src/qt/peertablemodel.cpp
2@@ -9,11 +9,14 @@
3 
4 #include <interfaces/node.h>
5 
6+#include <chrono>
7 #include <utility>

RandyMcMillan commented at 7:33 pm on February 11, 2022:

applied in commit 4cacb8c0463307dfc02f077af5df2f79974ff9d2 with attribution.

jonatack commented at 11:10 pm on February 12, 2022:

suggested changes in the first commit, if you repush:

initialize localvars in order of first use

 0     CNodeCombinedStats *rec = static_cast<CNodeCombinedStats*>(index.internalPointer());
 1-    const auto time_now{GetTime<std::chrono::seconds>()};
 2     const auto column = static_cast<ColumnIndex>(index.column());
 3+    const auto time_now{GetTime<std::chrono::seconds>()};
 4     if (role == Qt::DisplayRole) {
 5         switch (column) {
 6         case NetNodeId:
 7             return (qint64)rec->nodeStats.nodeid;
 8         case Age:
 9             if (time_now - rec->nodeStats.m_connected < 60s) {
10                 return GUIUtil::formatDurationStr(time_now - rec->nodeStats.m_connected);

spaces around minus sign

0-            return GUIUtil::formatDurationStr(time_now-rec->nodeStats.m_connected);
1+            return GUIUtil::formatDurationStr(time_now - rec->nodeStats.m_connected);

in src/qt/peertablemodel.cpp:83 in d01c0b36d1 outdated

76+            if(std::chrono::duration_cast<std::chrono::minutes>(time_now-rec->nodeStats.m_connected) < std::chrono::seconds{60}){
77+                /* display connection "Age" in seconds for first minute */
78+                return GUIUtil::formatDurationStr(std::chrono::duration_cast<std::chrono::seconds>(time_now-rec->nodeStats.m_connected));
79+            }else{
80+                return GUIUtil::formatDurationStr(std::chrono::duration_cast<std::chrono::minutes>(time_now-rec->nodeStats.m_connected));
81+            }

jonatack commented at 12:44 pm on February 11, 2022:

d01c0b36 suggestions

clang-format
use chrono literals, e.g. 60s instead of std::chrono::seconds{60}
drop unneeded casts and comment that echoes the code (and shouldn’t be in doxygen format here)

 0+using namespace std::chrono_literals;
 1+
 2 PeerTableModel::PeerTableModel(interfaces::Node& node, QObject* parent) :
 3     QAbstractTableModel(parent),
 4     m_node(node),
 5@@ -72,11 +75,11 @@ QVariant PeerTableModel::data(const QModelIndex& index, int role) const
 6         case NetNodeId:
 7             return (qint64)rec->nodeStats.nodeid;
 8         case Age:
 9-            if(std::chrono::duration_cast<std::chrono::minutes>(time_now-rec->nodeStats.m_connected) < std::chrono::seconds{60}){
10-                /* display connection "Age" in seconds for first minute */
11-                return GUIUtil::formatDurationStr(std::chrono::duration_cast<std::chrono::seconds>(time_now-rec->nodeStats.m_connected));
12-            }else{
13-                return GUIUtil::formatDurationStr(std::chrono::duration_cast<std::chrono::minutes>(time_now-rec->nodeStats.m_connected));
14+            if (time_now - rec->nodeStats.m_connected < 60s) {
15+                return GUIUtil::formatDurationStr(time_now - rec->nodeStats.m_connected);
16+            } else {
17+                return GUIUtil::formatDurationStr(
18+                    std::chrono::duration_cast<std::chrono::minutes>(time_now - rec->nodeStats.m_connected));
19             }

If this becomes larger, say, to display in DD:HH format after 24 hours it may make sense to extract it to a helper function.

RandyMcMillan commented at 7:43 pm on February 11, 2022:

The ResizeToContents settings should accommodate the duration string as the connection persists longer than 24 hours.

NOTE: The fact that <chrono> is formatting the string for # s, # m then # h # m suggests that it will format for # d # h # m. I guess we could spoof the time to prove/test this.

I applied your suggested diff with attribution.

jonatack commented at 11:01 pm on February 12, 2022:

The fact that <chrono> is formatting the string for # s, # m then # h # m suggests that it will format for # d # h # m. I guess we could spoof the time to prove/test this.

Tested with a few different values and you’re right: 23 h 59 s, 1 d, 1 d 1 h 59 m, 1 d 2 h, 1 d 2 h 1 m, etc. The spacing makes the column wider and a bit noisier than necessary. Could be improved here or as a follow-up.

0-    const auto time_now{GetTime<std::chrono::seconds>()};
1+    const auto time_now{GetTime<std::chrono::seconds>() + 86380s};

in src/qt/peertablesortproxy.cpp:28 in d01c0b36d1 outdated

23@@ -24,6 +24,8 @@ bool PeerTableSortProxy::lessThan(const QModelIndex& left_index, const QModelInd
24     switch (static_cast<PeerTableModel::ColumnIndex>(left_index.column())) {
25     case PeerTableModel::NetNodeId:
26         return left_stats.nodeid < right_stats.nodeid;
27+    case PeerTableModel::Age:
28+        return left_stats.m_connected > right_stats.m_connected;

jonatack commented at 12:48 pm on February 11, 2022:

The first commit doesn’t build cleanly without the second commit adding this line; they should be squashed into the same commit.

0qt/peertablesortproxy.cpp:24:13: warning: enumeration value 'Age' not handled in switch [-Wswitch]
1    switch (static_cast<PeerTableModel::ColumnIndex>(left_index.column())) {
2            ^
31 warning generated.

RandyMcMillan commented at 7:37 pm on February 11, 2022:

I re-reviewed CONTRIBUTING.md - it has been awhile - and will adjust my workflow and PR structure accordingly - thanks for the correction.

jonatack commented at 8:00 pm on February 11, 2022:

Great. Yes, each commit should be hygienic (clean debug build and all tests green).

in src/qt/rpcconsole.cpp:731 in d01c0b36d1 outdated

723@@ -723,6 +724,7 @@ void RPCConsole::setClientModel(ClientModel *model, int bestblock_height, int64_
724             ui->banlistWidget->setColumnWidth(BanTableModel::Address, BANSUBNET_COLUMN_WIDTH);
725             ui->banlistWidget->setColumnWidth(BanTableModel::Bantime, BANTIME_COLUMN_WIDTH);
726         }
727+        ui->banlistWidget->horizontalHeader()->setSectionResizeMode(0, QHeaderView::ResizeToContents);

jonatack commented at 12:52 pm on February 11, 2022:

da2dc046 for each of these two lines, it might be good to comment which column (Age, IP/Netmask) it corresponds to and to update the value if the column order changes (if I understand correctly)

RandyMcMillan commented at 7:31 pm on February 11, 2022:

I added a generic reference to qt/peertablemodel.h - so future column changes don’t require endless revisions to this comment.

jonatack commented at 12:57 pm on February 11, 2022: contributor

Tested approach ACK, this seems much improved.

RandyMcMillan force-pushed on Feb 11, 2022

jonatack commented at 11:15 pm on February 12, 2022: contributor

ACK 795a03da76b64e898fdeb5f36be2eb9a4600546e

jonatack commented at 7:28 pm on February 13, 2022: contributor

@RandyMcMillan I propose these commits for this pull: https://github.com/jonatack/gui/commits/gui-peers-tab-age-column. It formats the column denominated by the most relevant unit (sec, min, hour, day). I think it’s a much cleaner display.

RandyMcMillan commented at 8:18 pm on February 13, 2022: contributor

@jonatack - I agree with your changes - I will add them asap! 😄

RandyMcMillan force-pushed on Feb 13, 2022

RandyMcMillan commented at 10:46 pm on February 13, 2022: contributor

Commit a2e44c1 add reusable FormatPeerAge() utility helper - simplifying over all implementation.

in src/qt/peertablemodel.cpp:106 in a2e44c1e38 outdated

 97@@ -96,10 +98,12 @@ QVariant PeerTableModel::data(const QModelIndex& index, int role) const
 98     } else if (role == Qt::TextAlignmentRole) {
 99         switch (column) {
100         case NetNodeId:
101+        case Age:
102             return QVariant(Qt::AlignRight | Qt::AlignVCenter);
103         case Address:
104             return {};
105         case Direction:
106+            return QVariant(Qt::AlignCenter);

jonatack commented at 9:39 am on February 14, 2022:

f25a6bae sorry for not noticing before; this line can be dropped

jonatack commented at 9:43 am on February 14, 2022: contributor

ACK a2e44c1e3838195d0c0187a5f112015fbb1a8fd6

modulo one comment

unknown approved

unknown commented at 12:29 pm on February 14, 2022: none

tACK https://github.com/bitcoin-core/gui/pull/543/commits/a2e44c1e3838195d0c0187a5f112015fbb1a8fd6

in src/qt/guiutil.cpp:746 in a2e44c1e38 outdated

741+    if (age < 60s) return QObject::tr("%1 s").arg(age.count());
742+    const auto hours{std::chrono::duration_cast<std::chrono::hours>(age).count()};
743+    if (hours >= 24) return QObject::tr("%1 d").arg(hours / 24);
744+    if (hours) return QObject::tr("%1 h").arg(hours);
745+    const auto minutes{std::chrono::duration_cast<std::chrono::minutes>(age).count()};
746+    return QObject::tr("%1 m").arg(minutes);

jonatack commented at 4:08 pm on February 14, 2022:

@RandyMcMillan, looking at this further, I think the function in 349c597475d68e90596d165ee1cd5f08bbe59e2b can be simplified and improved as follows:

 0@@ -736,12 +736,10 @@ QString FormatPeerAge(std::chrono::seconds time_connected)
 1 {
 2     const auto time_now{GetTime<std::chrono::seconds>()};
 3     const auto age{time_now - time_connected};
 4-    if (age < 60s) return QObject::tr("%1 s").arg(age.count());
 5-    const auto hours{std::chrono::duration_cast<std::chrono::hours>(age).count()};
 6-    if (hours >= 24) return QObject::tr("%1 d").arg(hours / 24);
 7-    if (hours) return QObject::tr("%1 h").arg(hours);
 8-    const auto minutes{std::chrono::duration_cast<std::chrono::minutes>(age).count()};
 9-    return QObject::tr("%1 m").arg(minutes);
10+    if (age >= 24h) return QObject::tr("%1 d").arg(age / 24h);
11+    if (age >= 1h) return QObject::tr("%1 h").arg(age / 1h);
12+    if (age >= 1min) return QObject::tr("%1 m").arg(age / 1min);
13+    return QObject::tr("%1 s").arg(age / 1s);
14 }

jonatack commented at 4:15 pm on February 14, 2022:

I repushed an update to https://github.com/jonatack/gui/commits/gui-peers-tab-age-column with the changes in #543 (review) and #543 (review) if you want to cherry-pick them.

luke-jr commented at 11:56 pm on February 14, 2022:

Should probably add translator notes too

jonatack commented at 12:29 pm on February 15, 2022:

Should probably add translator notes too

Agree, am doing this for several of these (FormatPeerAge, formatDurationStr, TimeDurationField, etc.) as a follow-up.

RandyMcMillan force-pushed on Feb 14, 2022

RandyMcMillan commented at 6:48 pm on February 14, 2022: contributor

Rebased to 25a91a571a4f3453f7e0e9299ee7a40a61d04f19 commit abf93ddcdf5802f9b76e747a7669bc65bbda92cc includes a more compact implementation of the QString FormatPeerAge(std::chrono::seconds time_connected) helper function.

jonatack commented at 8:21 pm on February 14, 2022: contributor

ACK 35d685a7db407161d267abb699b3181f05e998bb

I like that the Peer and Age columns have the opposite default sort direction, displaying both the newest and oldest peers by default.

Note that there is a Connection Time in the peer details that displays the Age in a more verbose format. I think it’s ok as-is, as the formats are different, or maybe might propose renaming the former to Age (Connection Time).

unknown approved

unknown commented at 9:18 pm on February 14, 2022: none

reACK https://github.com/bitcoin-core/gui/pull/543/commits/35d685a7db407161d267abb699b3181f05e998bb

RandyMcMillan commented at 10:28 pm on February 14, 2022: contributor

This has been a fun collaborative effort. 😄

I enjoyed watching a rough idea get polished into an “ACK-able” state.

shaavan approved

shaavan commented at 11:56 am on February 15, 2022: contributor

ACK 35d685a7db407161d267abb699b3181f05e998bb

I agree with the concept of adding an Age column in the Node window table because I agree with all the reasons for its approval discussed above. To briefly reiterate them:

We have unused screen real estate which could be used more productively.
The Age column is displayed in the -netinfo command for bitcoin-cli. It makes logical sense to keep our GUI consistent with the cli.
Age column is frequently used by users through -netinfo command. So it would be a good idea to add the GUI.

Regarding the code:

I like the structuring of the FormatPeerPage function. The code is clean and easy to understand.
I like the idea of displaying only the highest degree of time, instead of precise time. This gives users a general idea of how a node has been contacted to them, while also not taking too much screen real estate, and user’s attention.
I think naming the column “Age” seems appropriate to me. This keeps it consistent with bitcoin-cli’s naming.

Here are some screenshots I have added showing the correct working of this PR, on Ubuntu 20.04

Master	PR

w0xlt approved

w0xlt commented at 3:49 pm on February 18, 2022: contributor

tACK 35d685a on Ubuntu 21.10 Qt 5.15.2

in src/qt/guiutil.cpp:732 in abf93ddcdf outdated

733@@ -732,6 +734,16 @@ QString formatDurationStr(std::chrono::seconds dur)
734     return strList.join(" ");
735 }
736 
737+QString FormatPeerAge(std::chrono::seconds time_connected)

promag commented at 11:18 pm on February 22, 2022:

abf93ddcdf5802f9b76e747a7669bc65bbda92cc

I think this could be idempotent QString FormatAge(std::chrono::seconds age), to avoid GetTime.

Note that PeerTableModel has a timer to refresh the data, just call GetTime once there.

jonatack commented at 4:30 pm on March 6, 2022:

@promag I agree this would be an improvement and am looking into it, but am not sure how to integrate GetTime into the peertablemodel.cpp QTimer code. Pointers welcome.

in src/qt/peertablemodel.cpp:75 in 852b036a7c outdated

70@@ -71,6 +71,8 @@ QVariant PeerTableModel::data(const QModelIndex& index, int role) const
71         switch (column) {
72         case NetNodeId:
73             return (qint64)rec->nodeStats.nodeid;
74+        case Age:
75+            return GUIUtil::FormatPeerAge(rec->nodeStats.m_connected);

promag commented at 11:20 pm on February 22, 2022:

852b036a7cc0d38f00558125e04528d4cd0fa2dd

Following my previous comment, this could be

0return GUIUtil::FormatAge(m_now - rec->nodeStats.m_connected);

promag commented at 11:21 pm on February 22, 2022: contributor

Concept ACK.

jonatack commented at 11:59 am on March 11, 2022: contributor

This pull has 4 ACKs and could perhaps be merged now that v23 has been branched off, which would enable moving forward on planned follow-ups like #543 (review), while still allowing a possible improvement per #543 (review).

in src/qt/rpcconsole.cpp:697 in 35d685a7db outdated

692@@ -693,6 +693,8 @@ void RPCConsole::setClientModel(ClientModel *model, int bestblock_height, int64_
693             ui->peerWidget->setColumnWidth(PeerTableModel::Subversion, SUBVERSION_COLUMN_WIDTH);
694             ui->peerWidget->setColumnWidth(PeerTableModel::Ping, PING_COLUMN_WIDTH);
695         }
696+        // reference qt/peertablemodel.h for column indexes
697+        ui->peerWidget->horizontalHeader()->setSectionResizeMode(1, QHeaderView::ResizeToContents);

hebasto commented at 8:50 am on March 12, 2022:

Drop “magic” numbers?

0        ui->peerWidget->horizontalHeader()->setSectionResizeMode(PeerTableModel::Age, QHeaderView::ResizeToContents);

in src/qt/rpcconsole.cpp:731 in 35d685a7db outdated

726@@ -725,6 +727,8 @@ void RPCConsole::setClientModel(ClientModel *model, int bestblock_height, int64_
727             ui->banlistWidget->setColumnWidth(BanTableModel::Address, BANSUBNET_COLUMN_WIDTH);
728             ui->banlistWidget->setColumnWidth(BanTableModel::Bantime, BANTIME_COLUMN_WIDTH);
729         }
730+        // reference qt/peertablemodel.h for column indexes
731+        ui->banlistWidget->horizontalHeader()->setSectionResizeMode(0, QHeaderView::ResizeToContents);

hebasto commented at 8:51 am on March 12, 2022:

Drop “magic” numbers?

0        ui->banlistWidget->horizontalHeader()->setSectionResizeMode(BanTableModel::Address, QHeaderView::ResizeToContents);

hebasto changes_requested

RandyMcMillan requested review from hebasto on Mar 12, 2022

RandyMcMillan commented at 7:29 pm on March 12, 2022: contributor

7ddff98 merged requested changes. Will squash after re-review.

jonatack commented at 1:44 pm on March 13, 2022: contributor

ACK. Squashing the changes into the relevant commit (in this case, the last commit 35d685a7db407161d267abb699b3181f05e998bb) when re-pushing an update can save time for reviewers and speed up the merge. See the “Squashing Commits” section of CONTRIBUTING.md.

gui: add FormatPeerAge() utility helper

Co-authored-by: randymcmillan <randy.lee.mcmillan@gmail.com>

127de22c5f

gui: add Age column to peers tab

Co-authored-by: Jon Atack <jon@atack.com>

209301a442

gui: peersWidget - ResizeToContents Age and IP/Netmask columns

Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>

51708c4516

RandyMcMillan force-pushed on Mar 16, 2022

RandyMcMillan commented at 9:02 am on March 16, 2022: contributor

51708c4516cb9d52e84dc8850d93f556dda1a75b squashed merged requested changes. rebased on top of 310ba924949b0052105a7937ac69d65a3864692b

jonatack commented at 8:04 pm on March 16, 2022: contributor

re-ACK 51708c4516cb9d52e84dc8850d93f556dda1a75b

Jamewood commented at 4:10 am on March 17, 2022: none

(PR #543)

Jamewood commented at 4:50 am on March 17, 2022: none

51708c4

On Wed, Mar 16, 2022, 9:05 PM Jon Atack @.***> wrote:

re-ACK 51708c4 https://github.com/bitcoin-core/gui/commit/51708c4516cb9d52e84dc8850d93f556dda1a75b

— Reply to this email directly, view it on GitHub https://github.com/bitcoin-core/gui/pull/543#issuecomment-1069564641, or unsubscribe https://github.com/notifications/unsubscribe-auth/AWXCCBYJ3XSYP7MP53GSSL3VAI5G3ANCNFSM5NNV222A . You are receiving this because you are subscribed to this thread.Message ID: @.***>

RandyMcMillan commented at 9:54 pm on April 1, 2022: contributor

@Jamewood - thank you for the ACK if this was your intention - please ACK in the appropriate format - and remove the other comments. 😀

shaavan approved

shaavan commented at 7:49 am on April 9, 2022: contributor

reACK 51708c4516cb9d52e84dc8850d93f556dda1a75b

Changes since my last review:

Dropped “Magic numbers”. i.e., 1 → PeerTableModel::Age 0 → BanTableModel::Address
Rebased over master.

I agree with the idea of dropping “magic” numbers because:

It makes code easier to understand and reason with.
It makes code robust to change in enum ordering under the class’s definition.

hebasto approved

hebasto commented at 10:42 pm on April 12, 2022: member

ACK 51708c4516cb9d52e84dc8850d93f556dda1a75b, I have reviewed the code and it looks OK, I agree it can be merged.

hebasto merged this on Apr 12, 2022

hebasto closed this on Apr 12, 2022

sidhujag referenced this in commit 41277662d2 on Apr 13, 2022

bitcoin-core locked this on Apr 12, 2023

peers-tab: add connection duration column to tableview #543