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

pull ghost wants to merge 1 commits into bitcoin-core:master from changing 1 files +5 −1

ghost commented at 7:28 am on August 8, 2021: none

Fixes #397

~This re-establishes a functionality that was lost I guess since 0.21.~

Peer address column sort with current master: Peers 27 and 51 are sorted without honoring the connection direction.

Peer address column sort with this PR: Peers 16 and 22 are sorted honoring the connection direction.

I would be grateful for implementation improvement suggestions.
Add inbound/outbound arrow prefix to addrName to include it in sorting fb6bc6b5e1
hebasto added the label UX on Aug 8, 2021
hebasto commented at 10:15 am on August 8, 2021: member

This re-establishes a functionality that was lost I guess since 0.21.

This is not true: https://github.com/bitcoin-core/gui/blob/4c097f9669a28420da74b159a4d61e509da80d33/src/qt/peertablemodel.cpp#L26-L27

The implementation remains unchanged since v0.10.0 when it was introduced in https://github.com/bitcoin/bitcoin/pull/4225.

UPDATE: @wodry The arrows were added to addresses in your https://github.com/bitcoin/bitcoin/pull/13537 (since v0.17.0).
ghost commented at 12:25 pm on August 8, 2021: none

OK thanks, then my guess was wrong and i did really not stumble upon this issue by chance. I thought it might have been lost with the new sortProxy.
shaavan commented at 2:40 pm on August 9, 2021: contributor

Tested on Ubuntu 20.04

The sorting of addresses in Master is purely lexicographical, irrespective of the direction of peer. This PR adds the functionality of taking the direction of peers as a prior consideration while sorting the addresses. The PR still sorts addresses in lexicographical order for the same direction peers. I was able to compile and test the PR on Ubuntu 20.04 successfully.

Though I like the approach, since we want #317, it is not worth merging this PR because PR #317 would revert these changes.
jarolrod commented at 3:20 am on August 10, 2021: member

The issue here is that the meaning of arrows are ambiguous, I am personally concept 0 on this in favor of #317. These PR’s are essentially competing as #317 is just going to remove the arrows.
hebasto commented at 2:02 pm on August 11, 2021: member

Closing, as #317 has been merged.
hebasto closed this on Aug 11, 2021
unknown deleted the branch on Aug 12, 2021
bitcoin-core locked this on Aug 16, 2022

Contributors
ghost hebasto shaavan jarolrod

Labels
UX