Add Direction column to Peers Tab #317

jarolrod · 2021-05-05T08:03:41Z

Picking up #289

This adds a Direction column, making the peers tab the same as the Direction/Type row in the peer details and the direction and type columns in our other user-facing peer connections table in -netinfo.

Users can now sort the peers table by direction. The default sort is set to inbound, then outbound.

Master	PR

src/qt/peertablesortproxy.cpp

src/qt/guiutil.cpp

promag · 2021-05-10T13:40:43Z

No strong opinion, just think that with arrows it was more compact and clean.

jonatack · 2021-05-10T22:33:15Z

Currently, direction isn't a sortable column, and it's potentially confusing to users to display both connection direction and type in the details area but only the the connection type without the direction (and so no type at all for inbounds) in the main table. It was the reason I reopened the parent PR after an issue was filed in bitcoin/bitcoin where this came up.

jarolrod · 2021-05-26T03:47:50Z

updated from a44eec9 -> 5d7463a (pr317.01, pr317.02, diff)

Changes:

drop the last three commits (f4bde8c, 8090db4, a44eec9) inherited from picking up Add "Direction" column to peers tab #289 in order to make the PR focused on adding a Direction column. The work done in those commits will be picked up in another PR.
Now based on the last commit of Peers Window rename 'Peer id' to 'Peer' #311 so that we are consistent with adding the new Direction column and the translator comment for it
Address Add Direction column to Peers Tab #317 (comment) by removing the (possibly confusing) comment
Fix translator comment for the Direction column in PeerTableModel::data

jonatack

Light code review ACK 5d7463a

src/qt/peertablesortproxy.cpp

jarolrod · 2021-05-26T20:31:26Z

updated from 5d7463a -> 0183415 (pr317.02 -> pr317.03)

Changes:

rebased over master due to conflict with qt: Optimize string concatenation by default #313
Addressed @jonatacks suggestion. For the purposes of clarity, reintroduce comment about ordering

Co-authored-by: Jarol Rodriguez <jarolrod@tutanota.com>

jarolrod · 2021-05-26T21:39:54Z

updated from 0183415 -> 6971e79 (pr317.03 -> pr317.04, diff)

Changes:

rebase over conflict with Peers Window rename 'Peer id' to 'Peer' #311 (drop commit from 311 this was based on)

jonatack · 2021-05-26T21:46:54Z

Tested ACK 6971e79

jonatack · 2021-06-05T13:32:47Z

@hebasto mind having a look into this? Would be nice to have it in v22.

luke-jr · 2021-06-12T20:59:10Z

This still takes up an annoying chunk of horizontal size. #363 ?

jonatack · 2021-06-12T21:28:11Z

This still takes up an annoying chunk of horizontal size. #363 ?

Recently merged PRs gained us a lot of horizontal space so I thought it might no longer be an obstacle.

The issue with arrows is they would not resolve the discrepancy between the connection direction and type, which are displayed in full in the details area ("Outbound Full Relay") but lack the direction in the main table ("Full Relay"). This causes confusion, e.g. for a bitcoin dev in a bitcoin/bitcoin issue who thought it was a bug.

rebroad · 2021-06-26T14:21:39Z

NACK - I prefer the existing functionality. It could be possible to have a 4-click sort method on the existing column though if you wanted to choose to sort by the direction.

jonatack · 2021-06-26T15:27:26Z

Given the opposition to having a direction column, I wonder if the connection type column should be removed. Direction and type of connection go together and displaying only half of the two has been seen by experienced users as a bug. I think that's the wrong direction, but it is what reviewers seem to want. Edit: done in #372.

ghost · 2021-06-27T05:54:59Z

Users can now sort the peers table by direction.

With the up/down arrow before the address in the same column, you can already sort by direction. The column is sorted (or you could also say "grouped" like in bitcoin/bitcoin#13537 where the arrow prefix was introduced) by direction first (since the arrow is the first character), then by address (the following characters). You only cannot sort by address regardless of the direction, but I wonder what the use case could be for that.
I only thought that would be the case. See issue #397

jonatack · 2021-07-04T09:00:10Z

Note that #372 (which reverts the addition of the "type" column) or this PR ought to be merged for v22.0 to avoid confusing users with a half-implemented feature that may look like a bug.

jarolrod · 2021-07-06T03:30:30Z

@luke-jr

This still takes up an annoying chunk of horizontal size. #363 ?

#256 now allows a user to customize the widths of the table columns. It does not take up an annoying chunk of horizontal size if the user has resized the columns to their preference.

@rebroad

NACK - I prefer the existing functionality.

The existing functionality of no Direction column? 🤔

shaavan

tACK 6971e79

Compiled and tested successfully on Ubuntu 20.04

Displaying Connection Direction would help clarify some confusion regarding connection direction, as was suggested in #289. I like the approach of this PR over #363, because for the following reasons:

The horizontal arrows do not have a strong connotation of IN and OUT as compared to vertical arrows.
Without mentioning the purpose of the left and right arrows in the column header, it creates more confusion to a new user than clearing it.
Finally, the aesthetics of the arrow in Peers window: Show direction in a new column, with clearer icon #363 does not match with the rest of the table.

I would, however, like to make a suggestion. Instead of removing the arrow, they should be moved to the Direction Column. And also‘OUTBOUND’ and ‘INBOUND’ should be replaced by ‘OUT’ and ‘IN” as it is done here (under PR alternative diff header in the table) as it would convey the information correctly at the same time being aesthetically more pleasing.

hebasto · 2021-08-08T10:28:16Z

@promag

No strong opinion, just think that with arrows it was more compact and clean.

Arrows "up" and "down" have no meaning about the connection direction.

hebasto · 2021-08-08T10:41:32Z

@wodry

You only cannot sort by address regardless of the direction, but I wonder what the use case could be for that.

To check if one's node has more than one peers from a particular subnet.

hebasto · 2021-08-08T10:44:18Z

@rebroad

NACK - I prefer the existing functionality. It could be possible to have a 4-click sort method on the existing column though if you wanted to choose to sort by the direction.

What is a "4-click sort method"?

hebasto

ACK 6971e79, tested on Linux Mint 20.2 (Qt 5.12.8).

This change is an improvement for the following reasons:

both "Direction" and "Type" columns are coherent with peer details
the "Address" column is free from independent data
it is possible to sort by address and by direction independently
"up" and "down" arrows with no implicit or ubiquitous semantics of "inbound" and "outbound" have been replaced with the texts that are clear and understandable for everyone, including new users

I do not consider taking horizontal space by the new "Direction" column as a blocker for approving this PR.

A screenshot from my system:

It looks really great!

ghost · 2021-08-09T05:44:31Z

I like #401 very much, please see my suggestion B.3 there.

An empty connection type I find confusing. If the user sees (like I did with 22.0rc2), that always only all inbound connections have an empty type, the user thinks, OK I guess it's not detectable or something for inbound. But it leaves uncertainty, is confusing.

RandyMcMillan · 2021-08-09T07:59:03Z

Here is an example of a simple arrow based direction column for comparison.

Using that much visual real estate for direction seems impractical.

bitcoincore-dev#1

()

hebasto · 2021-08-09T08:02:05Z

@RandyMcMillan

Here is an example of a simple arrow based direction column for comparison.

What does "↑" mean in the column with unnamed header title?

RandyMcMillan · 2021-08-09T08:29:01Z

one possible option is to put a unicode earth symbol in the column title.

🜨

This is both prudent on visual real-estate and universally recognized.

arrow up (as in upload) means outbound data to most people these days.

arrow down (as in download) means inbound data these days.

hebasto · 2021-08-09T10:35:29Z

@RandyMcMillan

arrow up (as in upload) means outbound data to most people these days.

arrow down (as in download) means inbound data these days.

Upload/download are about a direction of data transferring.
Inbound/outbound are about a connection initiator.

Let's don't add more mess :)

RandyMcMillan · 2021-08-09T18:33:28Z

I was using the analogy to express an idea and possible approach to visual communication with the node operator.
I apologize if something is getting lost in translation.
I agree with you - a mess it is.

RandyMcMillan · 2021-08-09T18:47:43Z

I suspect the long term solution is to leverage existing symbolic/unicode libraries to communicate with the node operator.
This is provide a lot of information about the node "at a glance" and minimize the need to translate.
Using a symbolic library has the added benefit of being prudent in its use of gui real-estate.
This will also affect on-going Android mobile GUI design.

Reference: https://google.github.io/material-design-icons/

Explaining fundamental GUI designs and concepts is fun. :) (not really)

ghost · 2021-08-09T20:08:22Z

Please see my updated #401 (comment) in the brainstorming issue, with a small commit and a screenshot. Please comment what you think of it.

RandyMcMillan · 2021-08-09T21:30:11Z

As I mentioned in the comment below - drawing inspiration from the established iconography of git is going to go a long way.
In fact reusing the established icons for some gui elements may avoid reinventing the wheel on this...

#401 (comment)

REF: https://github.com/primer/octicons/tree/main/icons

6971e79 gui: add Direction column to peers tab (Jon Atack) Pull request description: Picking up #289 This adds a `Direction column`, making the peers tab the same as the `Direction/Type` row in the peer details and the direction and type columns in our other user-facing peer connections table in `-netinfo`. Users can now sort the peers table by direction. The default sort is set to inbound, then outbound. | Master | PR | | ----------- | ----------- | | ![Screen Shot 2021-05-05 at 3 51 09 AM](https://user-images.githubusercontent.com/23396902/117111864-38ff9a00-ad56-11eb-889d-f1c838c845e6.png) | ![Screen Shot 2021-05-05 at 3 35 40 AM](https://user-images.githubusercontent.com/23396902/117111892-4157d500-ad56-11eb-82b1-5bd3e88a4cff.png) | ACKs for top commit: jonatack: Tested ACK 6971e79 ShaMan239: tACK 6971e79 hebasto: ACK 6971e79, tested on Linux Mint 20.2 (Qt 5.12.8). Tree-SHA512: 9716cdedd435f88245a097fed6d4b2b486104d0dd09df739bdb4f2bfad709cbd9c9a231168cc3326e94fa5fddc77dd68f992f20417d04d94930db9fccdbb7de1

rebroad · 2021-09-01T12:21:49Z

src/qt/peertablemodel.cpp

+        case Direction:
+            return QString(rec->nodeStats.fInbound ?
+                               //: An Inbound Connection from a Peer.
+                               tr("Inbound") :


why not continue using the up and down arrows?

@rebroad

How could the "down arrow" could mean a "connection initiated by a peer" for a new user?

3ec061d qt: Add "Copy address" item to the context menu in the Peers table (Hennadii Stepanov) Pull request description: Picking up #264 This adds a `Copy Address` context menu action to the `Peers Tab`. Based on the first commit of PR #317 so that we can use `Qt::DisplayRole` in the `copyEntryData` function. | Master | PR | | ----------- | ----------- | | ![Screen Shot 2021-05-05 at 4 51 11 AM](https://user-images.githubusercontent.com/23396902/117117822-fb067400-ad5d-11eb-9466-228456108e52.png) | ![Screen Shot 2021-05-05 at 4 49 15 AM](https://user-images.githubusercontent.com/23396902/117117835-fe99fb00-ad5d-11eb-8de0-f6a9acdbf40e.png) | ACKs for top commit: shaavan: tACK 3ec061d luke-jr: utACK 3ec061d hebasto: ACK 3ec061d, tested on Linux Mint 20.2 (Qt 5.12.8): Tree-SHA512: be0d7324592aae3928fa3cc522294f17226419fe8cbe3587df12a36bd4fa9c81bead377b13051e950b9a3fcd290b273861e70d6c76b75cdf76eaf58224b834cd

jarolrod changed the title ~~Direction col peers~~ Peers Tab Direction Column May 5, 2021

jarolrod mentioned this pull request May 5, 2021

Add Copy address Peers Tab Context Menu Action #318

Merged

hebasto reviewed May 6, 2021

View reviewed changes

src/qt/peertablesortproxy.cpp Show resolved Hide resolved

hebasto reviewed May 7, 2021

View reviewed changes

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

hebasto added the UX All about "how to get things done" label May 9, 2021

jarolrod force-pushed the direction-col-peers branch from a44eec9 to 5d7463a Compare May 26, 2021 03:37

jonatack reviewed May 26, 2021

View reviewed changes

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

DrahtBot added the Needs rebase label May 26, 2021

jarolrod mentioned this pull request May 26, 2021

Add "Direction" column to peers tab #289

Closed

jarolrod force-pushed the direction-col-peers branch from 5d7463a to 0183415 Compare May 26, 2021 20:27

gui: add Direction column to peers tab

6971e79

Co-authored-by: Jarol Rodriguez <jarolrod@tutanota.com>

jarolrod force-pushed the direction-col-peers branch from 0183415 to 6971e79 Compare May 26, 2021 21:36

DrahtBot removed the Needs rebase label May 26, 2021

luke-jr mentioned this pull request Jun 12, 2021

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

Closed

jonatack mentioned this pull request Jun 26, 2021

Remove "Type" column from peers window #372

Closed

shaavan reviewed Jul 27, 2021

View reviewed changes

hebasto mentioned this pull request Aug 7, 2021

Peer address column sorting does not honor the inbound/outbound arrow prefix #397

Closed

hebasto mentioned this pull request Aug 8, 2021

Summary of Direction/Type columns problem and a new suggested trade-off #401

Closed

hebasto approved these changes Aug 8, 2021

View reviewed changes

hebasto changed the title ~~Peers Tab Direction Column~~ Add Direction column to Peers Tab Aug 11, 2021

hebasto merged commit 77e23ca into bitcoin-core:master Aug 11, 2021

hebasto mentioned this pull request Aug 11, 2021

Honor inbound/outbound arrow prefix when sorting peer address column #400

Closed

jonatack mentioned this pull request Aug 11, 2021

Connection Type Translator Comments #345

Merged

rebroad reviewed Sep 1, 2021

View reviewed changes

bitcoin-core locked as resolved and limited conversation to collaborators Sep 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Direction column to Peers Tab #317

Add Direction column to Peers Tab #317

jarolrod commented May 5, 2021 •

edited

Loading

promag commented May 10, 2021 •

edited

Loading

jonatack commented May 10, 2021

jarolrod commented May 26, 2021

jonatack left a comment

jarolrod commented May 26, 2021 •

edited

Loading

jarolrod commented May 26, 2021

jonatack commented May 26, 2021

jonatack commented Jun 5, 2021

luke-jr commented Jun 12, 2021

jonatack commented Jun 12, 2021

rebroad commented Jun 26, 2021

jonatack commented Jun 26, 2021 •

edited

Loading

ghost commented Jun 27, 2021 •

edited by ghost

Loading

jonatack commented Jul 4, 2021

jarolrod commented Jul 6, 2021

shaavan left a comment

hebasto commented Aug 8, 2021

hebasto commented Aug 8, 2021

hebasto commented Aug 8, 2021

hebasto left a comment

ghost commented Aug 9, 2021 •

edited by ghost

Loading

RandyMcMillan commented Aug 9, 2021

hebasto commented Aug 9, 2021 •

edited

Loading

RandyMcMillan commented Aug 9, 2021

hebasto commented Aug 9, 2021 •

edited

Loading

RandyMcMillan commented Aug 9, 2021

RandyMcMillan commented Aug 9, 2021

ghost commented Aug 9, 2021

RandyMcMillan commented Aug 9, 2021 •

edited

Loading

rebroad Sep 1, 2021

hebasto Sep 3, 2021

Add Direction column to Peers Tab #317

Add Direction column to Peers Tab #317

Conversation

jarolrod commented May 5, 2021 • edited Loading

promag commented May 10, 2021 • edited Loading

jonatack commented May 10, 2021

jarolrod commented May 26, 2021

jonatack left a comment

Choose a reason for hiding this comment

jarolrod commented May 26, 2021 • edited Loading

jarolrod commented May 26, 2021

jonatack commented May 26, 2021

jonatack commented Jun 5, 2021

luke-jr commented Jun 12, 2021

jonatack commented Jun 12, 2021

rebroad commented Jun 26, 2021

jonatack commented Jun 26, 2021 • edited Loading

ghost commented Jun 27, 2021 • edited by ghost Loading

jonatack commented Jul 4, 2021

jarolrod commented Jul 6, 2021

shaavan left a comment

Choose a reason for hiding this comment

hebasto commented Aug 8, 2021

hebasto commented Aug 8, 2021

hebasto commented Aug 8, 2021

hebasto left a comment

Choose a reason for hiding this comment

ghost commented Aug 9, 2021 • edited by ghost Loading

RandyMcMillan commented Aug 9, 2021

hebasto commented Aug 9, 2021 • edited Loading

RandyMcMillan commented Aug 9, 2021

hebasto commented Aug 9, 2021 • edited Loading

RandyMcMillan commented Aug 9, 2021

RandyMcMillan commented Aug 9, 2021

ghost commented Aug 9, 2021

RandyMcMillan commented Aug 9, 2021 • edited Loading

rebroad Sep 1, 2021

Choose a reason for hiding this comment

hebasto Sep 3, 2021

Choose a reason for hiding this comment

jarolrod commented May 5, 2021 •

edited

Loading

promag commented May 10, 2021 •

edited

Loading

jarolrod commented May 26, 2021 •

edited

Loading

jonatack commented Jun 26, 2021 •

edited

Loading

ghost commented Jun 27, 2021 •

edited by ghost

Loading

ghost commented Aug 9, 2021 •

edited by ghost

Loading

hebasto commented Aug 9, 2021 •

edited

Loading

hebasto commented Aug 9, 2021 •

edited

Loading

RandyMcMillan commented Aug 9, 2021 •

edited

Loading