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

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

Closed
ghost opened this issue Aug 7, 2021 · 8 comments
Closed
Labels
Bug Something isn't working UX All about "how to get things done"

Comments

@ghost
Copy link

ghost commented Aug 7, 2021

See this peer address column:
screenshot

Expected behavior
Peer number 196 is sorted below peer number 424, because it is sorted by the first character (arrow) first.

Actual behavior
Peer number 196 is sorted below peer number 110. The first (arrow) character seems not to be relevant for sorting?

To reproduce
Run with both Onion and I2P network enabled and watch the peer table.

System information
22.0rc2
Debian Buster amd_64

@ghost ghost added the Bug Something isn't working label Aug 7, 2021
@ghost ghost changed the title Peer address column is not sortec correctly Peer address column is not sorted correctly Aug 7, 2021
@hebasto hebasto added the UX All about "how to get things done" label Aug 7, 2021
@hebasto
Copy link
Member

hebasto commented Aug 7, 2021

@wodry
Is this sorting order was restored from the previous Bitcoin Core run, or was set manually in the current session?

UPDATE: see #397 (comment)

@hebasto
Copy link
Member

hebasto commented Aug 7, 2021

@wodry
Does sorting work for you without 986bf78?

UPDATE: see #397 (comment)

@ghost
Copy link
Author

ghost commented Aug 7, 2021

I wanted to sort by peer address (to more easily count I2P and Onion inbound/outbound connections), so I clicked manually on the peer address column header (where the little triangle is), to be sure, sorting by address is active. Then I noticed this issue. Clicking more times on the header reverses sorting order as expected, but issue persists.

@hebasto
Copy link
Member

hebasto commented Aug 7, 2021

For now, only address itself in considered during sorting:

case PeerTableModel::Address:
return left_stats.addrName.compare(right_stats.addrName) < 0;

I wanted to sort by peer address (to more easily count inbound/outbound connections), so I clicked manually on the peer address column header (where the little triangle is), to be sure, sorting by address is active. Then I noticed this issue. Clicking more times on the header reverses sorting order as expected, but issue persists.

Maybe you could be interested in #317 or #363?

@ghost ghost changed the title Peer address column is not sorted correctly Peer address column sorting does not honour the inbound/outbound arrow prefix Aug 8, 2021
@ghost ghost changed the title Peer address column sorting does not honour the inbound/outbound arrow prefix Peer address column sorting does not honor the inbound/outbound arrow prefix Aug 8, 2021
@ghost
Copy link
Author

ghost commented Aug 8, 2021

Maybe you could be interested in #317 or #363?

I do not find them reasonable, see my expressions there.

I like the sorting functionality that honors the inbound/outbound arrow prefix. I cannot imagine any use case for not honoring the prefix. Honoring the prefix makes sense, see my use case.

Was the prefix sorting honoring removed? In the past I did not stumble upon this issue, but not sure if I tested it explicitly.

@hebasto
Copy link
Member

hebasto commented Aug 8, 2021

I like the sorting functionality that honors the inbound/outbound arrow prefix. I cannot imagine any use case for not honoring the prefix. Honoring the prefix makes sense, see my use case.

... to more easily count I2P and Onion inbound/outbound connections

Your use case, actually, requires filtering :)

Was the prefix sorting honoring removed?

The implementation remains unchanged since it was introduced in bitcoin/bitcoin#4225.

@ghost
Copy link
Author

ghost commented Aug 9, 2021

This issue would be obsoleted by alternatives like #401 (B.2) or (B.3)

@hebasto
Copy link
Member

hebasto commented Aug 11, 2021

Closing, as #317 has been merged.

@hebasto hebasto closed this as completed Aug 11, 2021
@bitcoin-core bitcoin-core locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug Something isn't working UX All about "how to get things done"
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant