Add `Type` column to peers window, update peer details name/tooltip by jonatack · Pull Request #179 · bitcoin-core/gui

jonatack · 2021-01-09T21:27:48Z

This pull:

adds a sortable Type column to the GUI Peers tab window
updates the peer details row to Direction/Type, so the Type column without a direction makes sense (the tooltip is also updated)

jarolrod

ACK 09aa053

I love this! I was just thinking about how this would be a good feature today, you beat me to it. Code looks good to me. Tested on macOS Big Sur 11.1 with Qt 5.15.2

jonatack · 2021-01-10T09:32:34Z

Thanks! Though I should drop the relay type from inbound or check if full or block (same in -netinfo).

hebasto · 2021-01-10T09:41:41Z

Concept ACK.

jonatack · 2021-01-10T12:06:17Z

Rebased after merge of #163, added relay type for inbounds, and inversed the sort for the Direction column to be inbound first.

jonatack · 2021-01-10T14:12:27Z

src/qt/peertablemodel.cpp

+    case PeerTableModel::Direction:
+        return pLeft->fInbound > pRight->fInbound;
+    case PeerTableModel::ConnectionType:
+        return pLeft->m_conn_type < pRight->m_conn_type;


Memo to myself, if we prefer to sort alphabetically by type name rather than by enum order:

- return pLeft->m_conn_type < pRight->m_conn_type; + return GUIUtil::ConnectionTypeToShortQString(pLeft->m_conn_type, pLeft->fRelayTxes) < GUIUtil::ConnectionTypeToShortQString(pRight->m_conn_type, pRight->fRelayTxes);

src/qt/peertablemodel.h

src/qt/guiutil.cpp

jonatack · 2021-01-10T18:25:29Z

Thank you @hebasto for your feedback! Updated with your suggestions per git diff a657b0c 9f76ba6

hebasto

ACK 9f76ba6, tested on Linux Mint 20.1 (Qt 5.12.8):

jarolrod · 2021-01-10T22:07:50Z

re-ACK 9f76ba6, tested on macOS Big Sur 11.1 (Qt 5.15.2)

luke-jr · 2021-01-13T19:10:12Z

Why are Outbound/Inbound strings better than arrows?

jonatack · 2021-01-13T19:38:06Z

Why are Outbound/Inbound strings better than arrows?

Good question. I proposed words instead of arrows for these reasons:

The peer details area uses words. When it displays, say, "Outbound Full Relay", it seems more coherent for the column data to also display "Outbound" and "Full Relay", than up-arrow and "Full Relay"
-getinfo and -netinfo both also use words for the direction rather than arrows

luke-jr · 2021-01-13T22:58:55Z

Words use a lot more space than arrows... It's not unreasonable to have details show more .. detail :p

hebasto · 2021-01-13T23:06:13Z

Why are Outbound/Inbound strings better than arrows?

Words use a lot more space than arrows... It's not unreasonable to have details show more .. detail :p

Actually, words simplify the GUI for new-comers, as an "arrow up" does not imply "outbound" at all.

jonatack · 2021-01-14T07:32:16Z

Agree. And the GUI uses words for the direction in the main info tab.

jonatack · 2021-01-14T08:59:36Z

(If space is an issue we could use in/out instead of inbound/outbound, as that is the wording in the GUI info tab, -getinfo, and -netinfo)

hebasto · 2021-01-14T09:18:26Z

(If space is an issue we could use in/out instead of inbound/outbound, as that is the wording in the GUI info tab, -getinfo, and -netinfo)

I was thinking about it from the beginning, but it won't work for some translations.

jarolrod · 2021-01-17T01:20:15Z

(If space is an issue we could use in/out instead of inbound/outbound, as that is the wording in the GUI info tab, -getinfo, and -netinfo)

I think we should stick with inbound/outbound. Even if you changed to in/out wouldn't Direction keep the tab at about the same size

luke-jr · 2021-01-17T17:43:21Z

I was thinking something more like this:

jonatack/gui@add-peers-dir-and-type-columns...luke-jr:tmp_gui_peers_dir_arrows

jarolrod · 2021-01-17T20:51:14Z

I was thinking something more like this:

jonatack/[email protected]:tmp_gui_peers_dir_arrows

Below is a screenshot of what your branch looks like for me, font issue with the arrows. Anyways, my vote is still for inbound/outbound

luke-jr · 2021-01-17T22:24:01Z

I suppose we could also do icons with an arrow and tiny in/out text under it

RandyMcMillan · 2021-01-18T21:43:13Z

@jonatack @hebasto

This worked for me...

https://github.com/bitcoin-core/gui/pull/135/files#diff-e721b02fefce2a90ae95da0841adfdcc610ffccf93236ef236f81c1baead8e01R163

Most of the issues you may run into - I already figured out...

#135

jonatack · 2021-01-18T21:51:44Z

This current PR with 2 ACKs is my preferred version among the various proposals I have seen.

jonatack · 2021-01-18T21:54:56Z

I propose that further refinements be done in a follow-up (not by me ;).

RandyMcMillan · 2021-01-18T22:08:03Z

Something I didn't get to yet - was to separate the uacomment and give it its own column.
This could be very useful for some other ideas Ive been thinking about.

luke-jr · 2021-01-18T23:33:35Z

jonatack/gui@add-peers-dir-and-type-columns...luke-jr:tmp_gui_peers_dir_arrows is now translation-friendly...

I'm still weak concept nack on inbound/outbound as strings in the table. I think it's worse than the status quo, so would rather this not be merged as-is.

jonatack · 2021-01-29T15:49:50Z

Moving this back to Draft status until #201 is resolved.

jonatack · 2021-02-06T22:15:40Z

Ready for review again. Three changes from the previous version:

It no longer replaces the arrows with a Direction column, which had 2 ACKs but also a light NACK and which can be decided later in another pull
For now, inbound peers do not display anything in the Type column, following the logic of merged Display plain "Inbound" in peer details #203
In the details area, the Connection Type field is renamed to Direction/Type, so the Type column without a direction makes sense. Its tooltip is also updated.

I hope the translations work correctly in this latest push.

@hebasto, @jarolrod, @luke-jr, @RandyMcMillan, would you mind having another look?

jonatack · 2021-02-06T22:17:17Z

Updated the PR description and screenshot with this one:

jarolrod · 2021-02-17T14:16:32Z

ACK be4cf48

Tested with Spanish translations. The text added in the tooltip and text associated with the Type column will need translations.

leonardojobim

Tested ACK be4cf48 on Ubuntu 20.04 on VMWare.

RandyMcMillan · 2021-02-22T00:27:14Z

Removing "Peer" from the "Peer Id" column would allow for more prudent use of space.
Otherwise it should be translatable. A simple "ID" or "id" would be better. I think all caps "ID" is better imo.

Maybe Type and Network should be left justified?

NOTE: #202

PR #202 is going to allow for more info/columns to be added - and more info displayed "at a glance". As more columns are added - the columns can be adjusted for a tighter fit and/or column stretching in a resize event.

ACK 6fc72bd

macOS 10.14.6 (18G103)

… peer details name/tooltip be4cf48 gui: update to "Direction/Type" peer details name/tooltip (Jon Atack) 1518883 gui: add "Type" column to Peers main window (Jon Atack) 6fc72bd gui: allow ConnectionTypeToQString to prepend direction optionally (Jon Atack) Pull request description: This pull: - adds a sortable `Type` column to the GUI Peers tab window - updates the peer details row to `Direction/Type`, so the `Type` column without a direction makes sense (the tooltip is also updated) ![Screenshot from 2021-02-06 22-53-11](https://user-images.githubusercontent.com/2415484/107130646-973bee80-68c7-11eb-9025-b18394ac5c93.png) ACKs for top commit: jarolrod: ACK be4cf48 leonardojobim: Tested ACK bitcoin-core/gui@be4cf48 on Ubuntu 20.04 on VMWare. Tree-SHA512: 6c6d1dbe7d6bdb616acff0aaf8f4223546f1d2524566e9cd6e5b1b3bed2be1e9b20b1bc52ed3b627df53ba1f2fe0bc76f036cf16ad934d8a446b515d9bece3b1

jonatack changed the title ~~gui: add direction and type columns to peers window~~ Add direction and type columns to peers window Jan 9, 2021

jarolrod reviewed Jan 10, 2021

View reviewed changes

DrahtBot added the Needs rebase label Jan 10, 2021

jonatack mentioned this pull request Jan 10, 2021

Peer details: replace Direction with Connection Type #163

Merged

jonatack force-pushed the add-peers-dir-and-type-columns branch 2 times, most recently from 5e605a4 to 816fc10 Compare January 10, 2021 11:28

DrahtBot removed the Needs rebase label Jan 10, 2021

jonatack force-pushed the add-peers-dir-and-type-columns branch from 816fc10 to a657b0c Compare January 10, 2021 12:05

jonatack commented Jan 10, 2021

View reviewed changes

hebasto reviewed Jan 10, 2021

View reviewed changes

src/qt/peertablemodel.h Outdated Show resolved Hide resolved

hebasto reviewed Jan 10, 2021

View reviewed changes

src/qt/guiutil.cpp Outdated Show resolved Hide resolved

jonatack force-pushed the add-peers-dir-and-type-columns branch from a657b0c to 9f76ba6 Compare January 10, 2021 18:22

hebasto approved these changes Jan 10, 2021

View reviewed changes

jonatack mentioned this pull request Jan 11, 2021

Peer details: connection type follow-ups #180

Merged

jonatack marked this pull request as draft January 29, 2021 15:48

jonatack added 3 commits February 6, 2021 22:21

gui: allow ConnectionTypeToQString to prepend direction optionally

6fc72bd

gui: add "Type" column to Peers main window

1518883

gui: update to "Direction/Type" peer details name/tooltip

be4cf48

jonatack force-pushed the add-peers-dir-and-type-columns branch from 9f76ba6 to be4cf48 Compare February 6, 2021 22:04

jonatack marked this pull request as ready for review February 6, 2021 22:07

jonatack changed the title ~~Add direction and type columns to peers window~~ Add Type column to peers window, update peer details name/tooltip Feb 6, 2021

leonardojobim approved these changes Feb 21, 2021

View reviewed changes

maflcko merged commit 02fda82 into bitcoin-core:master Feb 22, 2021

This was referenced Feb 22, 2021

refactor: Use enum type as switch argument in *TableModel #166

Merged

Add peertablesortproxy module #18

Merged

Handle peer addition/removal in a right way #164

Merged

jonatack deleted the add-peers-dir-and-type-columns branch February 22, 2021 13:13

maflcko mentioned this pull request Apr 22, 2021

gui: Node appears as Unroutable in peers list bitcoin/bitcoin#21747

Closed

jonatack mentioned this pull request Apr 23, 2021

Add "Direction" column to peers tab #289

Closed

hebasto mentioned this pull request Apr 30, 2021

Peers window Peer id improvements #290

Closed

jonatack mentioned this pull request Jun 26, 2021

Remove "Type" column from peers window #372

Closed

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 28, 2022

Merge 02fda82 into merged_master (Bitcoin PR bitcoin-core/gui#179)

fe1e52f

bitcoin-core locked as resolved and limited conversation to collaborators Aug 16, 2022

Conversation

jonatack commented Jan 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jarolrod left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack commented Jan 10, 2021

Uh oh!

hebasto commented Jan 10, 2021

Uh oh!

jonatack commented Jan 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack Jan 10, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jonatack commented Jan 10, 2021

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

jarolrod commented Jan 10, 2021

Uh oh!

luke-jr commented Jan 13, 2021

Uh oh!

jonatack commented Jan 13, 2021

Uh oh!

luke-jr commented Jan 13, 2021

Uh oh!

hebasto commented Jan 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack commented Jan 14, 2021

Uh oh!

jonatack commented Jan 14, 2021

Uh oh!

hebasto commented Jan 14, 2021

Uh oh!

jarolrod commented Jan 17, 2021

Uh oh!

luke-jr commented Jan 17, 2021

Uh oh!

jarolrod commented Jan 17, 2021

Uh oh!

luke-jr commented Jan 17, 2021

Uh oh!

RandyMcMillan commented Jan 18, 2021

Uh oh!

jonatack commented Jan 18, 2021

Uh oh!

jonatack commented Jan 18, 2021

Uh oh!

RandyMcMillan commented Jan 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luke-jr commented Jan 18, 2021

Uh oh!

jonatack commented Jan 29, 2021

Uh oh!

jonatack commented Feb 6, 2021

Uh oh!

jonatack commented Feb 6, 2021

Uh oh!

jarolrod commented Feb 17, 2021

Uh oh!

leonardojobim left a comment

Choose a reason for hiding this comment

Uh oh!

RandyMcMillan commented Feb 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

jonatack commented Jan 9, 2021 •

edited

Loading

jonatack commented Jan 10, 2021 •

edited

Loading

hebasto commented Jan 13, 2021 •

edited

Loading

RandyMcMillan commented Jan 18, 2021 •

edited

Loading