Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GUI: Peer table: Visualize inbound/outbound state for every row #13537

Merged
merged 1 commit into from Jul 1, 2018

Conversation

Projects
None yet
6 participants
@wodry
Copy link
Contributor

wodry commented Jun 25, 2018

Fixes #13483

The address in the network peer table is prefixed with an up-arrow symbolizing an outbound connection, or an down-array symbolizing an inbound connection. See screenshot.

The user has an easy visual confirmation about the connection direction state. I really like it :)
Impact to columns sorting is grouping by inbound/outbound first, which in my opinion is an advantage, too.
bildschirmfoto

@wodry wodry force-pushed the wodry:peer-table-show-inbound-outbound-symbol branch Jun 25, 2018

@fanquake fanquake added GUI P2P labels Jun 25, 2018

@promag
Copy link
Member

promag left a comment

Concept ACK.

src/qt/peertablemodel.cpp Outdated
switch(index.column())
{
case NetNodeId:
return (qint64)rec->nodeStats.nodeid;
case Address:
return QString::fromStdString(rec->nodeStats.addrName);
rec->nodeStats.fInbound ? address_output_prefix = "↓ " : address_output_prefix = "↑ ";

This comment has been minimized.

@promag

promag Jun 26, 2018

Member

Prefer:

address_output_prefix = rec->nodeStats.fInbound ? "" : "";
src/qt/peertablemodel.cpp Outdated
switch(index.column())
{
case NetNodeId:
return (qint64)rec->nodeStats.nodeid;
case Address:
return QString::fromStdString(rec->nodeStats.addrName);
rec->nodeStats.fInbound ? address_output_prefix = "↓ " : address_output_prefix = "↑ ";
return QString::fromStdString(rec->nodeStats.addrName).prepend(address_output_prefix);

This comment has been minimized.

@promag

promag Jun 26, 2018

Member

Could inline like:

return QString(rec->nodeStats.fInbound ? "" : "") + QString::fromStdString(rec->nodeStats.addrName);

and remove declaration above.

@wodry wodry force-pushed the wodry:peer-table-show-inbound-outbound-symbol branch to 4132ad3 Jun 26, 2018

@wodry

This comment has been minimized.

Copy link
Contributor Author

wodry commented Jun 26, 2018

@promag Thanks, cleaner and shorter code. Rebased and squashed.

@@ -162,7 +162,8 @@ QVariant PeerTableModel::data(const QModelIndex &index, int role) const
case NetNodeId:
return (qint64)rec->nodeStats.nodeid;
case Address:
return QString::fromStdString(rec->nodeStats.addrName);
// prepend to peer address down-arrow symbol for inbound connection and up-arrow for outbound connection
return QString(rec->nodeStats.fInbound ? "" : "") + QString::fromStdString(rec->nodeStats.addrName);

This comment has been minimized.

@jonasschnelli

jonasschnelli Jun 26, 2018

Member

Does those unicode char (86 93 and 86 91) work on all supported platforms (especially older windows version)?
Would using an Icon make more sense?

This comment has been minimized.

@wodry

wodry Jun 26, 2018

Author Contributor

Could You define 'older windows version'?

This comment has been minimized.

@fanquake

fanquake Jun 27, 2018

Member

Windows 7 or later is what we are currently supporting.

This comment has been minimized.

@wodry

wodry Jun 29, 2018

Author Contributor

I can not test for Windows, since I do not have any Windows. Maybe someone with Windows testing capabilities can apply this one-liner and check?

This comment has been minimized.

@laanwj

laanwj Jun 29, 2018

Member

These characters have been in unicode since time immemorial [Unicode 1.1.0 (June, 1993], I wouldn't be afraid of them not being supported.

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Jun 26, 2018

Nice and small change. Concept ACK

@jb55

jb55 approved these changes Jun 26, 2018

Copy link
Contributor

jb55 left a comment

utACK 4132ad3

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Jun 29, 2018

utACK 4132ad3

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Jun 29, 2018

The address in the network peer table is prefixed with an up-arrow symbolizing an outbound connection, or an down-array symbolizing an inbound connection. See screenshot.

Thank you for doing it this way, instead of adding another column.
utACK 4132ad3

@fanquake

This comment has been minimized.

Copy link
Member

fanquake commented Jul 1, 2018

tACK 4132ad3

macOS 10.13.5

master:

macos-master

4132ad3:

macos-13537

Windows 10

4132ad3:

windows10-13537

The symbol could be a bit hard to make out on Windows, but Windows really needs an overhaul in terms of font/sizing anyways, so I don't think that should block anything here.

@jonasschnelli jonasschnelli merged commit 4132ad3 into bitcoin:master Jul 1, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

jonasschnelli added a commit that referenced this pull request Jul 1, 2018

Merge #13537: GUI: Peer table: Visualize inbound/outbound state for e…
…very row

4132ad3 Show symbol for inbound/outbound in peer table (wodry)

Pull request description:

  Fixes #13483

  The address in the network peer table is prefixed with an up-arrow symbolizing an outbound connection, or an down-array symbolizing an inbound connection. See screenshot.

  The user has an easy visual confirmation about the connection direction state. I really like it :)
  Impact to columns sorting is grouping by inbound/outbound first, which in my opinion is an advantage, too.
  ![bildschirmfoto](https://user-images.githubusercontent.com/8447873/41862752-13803eb2-78a5-11e8-9126-a52385f5ec19.png)

Tree-SHA512: d355f679d34c3006743c06750be5f36a083c1a8376da8f5f35045fcd9df964153409946fdde5007734f23bd692c91355962dc42df31122cdcf88e4affce8bc0e

@wodry wodry deleted the wodry:peer-table-show-inbound-outbound-symbol branch Jul 1, 2018

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Jul 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.