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

Remove "Type" column from peers window #372

Closed

Conversation

jonatack
Copy link
Member

@jonatack jonatack commented Jun 26, 2021

This PR removes the "Type" column from the peers main window that was added in #179, as it is confusing to users and does not make sense to have a Type column without its corresponding Direction column immediately preceding it to indicate Inbound or Outbound, per our current connection type naming (one of the connection types is "Inbound" and is not ever displayed currently).

Three attempts to add the Direction column in #179 (original version), #289, and #317 have seen opposition from some reviewers.

Therefore, before the v22.0 release, remove the Type column until such time as there is rough consensus to add the Direction Inbound/Outbound column with it (or better yet, merge #317).

jonatack added 2 commits June 26, 2021 18:37
as it is confusing to users and does not make sense
to have a Type column without a Direction column
immediately preceding it to indicate Inbound or Outbound.

Several attempts to add the Direction column, as
described, have seen opposition from reviewers.

Therefore, remove the Type column until such time as
there is rough consensus to add the Direction
Inbound/Outbound column with it.
@jarolrod jarolrod added the UX All about "how to get things done" label Jun 26, 2021
Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, it is a sub-optimal situation. My opinion is that there is still value in maintaining the Type column. I am mainly concerned about removing this, and then going through opposition to reintroduce it next release cycle.

While we may not have a separate and sortable Direction column, we do still have arrows prepended before the Address. This has been the default behavior for three years now (4132ad3) so user's know to look here for the direction of the selected peer. While the situation is not optimal; this means the Type information does have orientation context in regards to Direction.

@jonatack
Copy link
Member Author

It's too confusing for users with the current halfway implementation. One type (Inbound) isn't even displayed, and the empty type looks like a bug.

At least this PR and its predecessors can be pointed to when more user bug reports arrive, if this isn't fixed before the release: "Look, we tried 3 times." So this had to be proposed.

But it would be better to resolve it before the release, one way or the other.

@ghost
Copy link

ghost commented Jul 29, 2021

But it would be better to resolve it before the release, one way or the other

We should add directions column

Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on Ubuntu 20.04

The PR claims to remove the type column from the PR table and is successful in doing so. Along with the type column, the PR is also removing the direction arrows preceding the node's address, which makes sense to do.

Though I understand the approach of op to solve this problem, I don’t think it is optimal. As @jarolrod already mentioned, removing the type column will inevitably result in a debate over bringing it back. The optimal approach will be merging pr317, or something similar to 317 if there is a debate there, as suggested by op.

@hebasto
Copy link
Member

hebasto commented Aug 11, 2021

Closing, as #317 has been merged.

@hebasto hebasto closed this Aug 11, 2021
@jonatack jonatack deleted the remove-peers-type-column branch August 11, 2021 14:00
@jonatack
Copy link
Member Author

Closing, as #317 has been merged.

Thanks!

@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
UX All about "how to get things done"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants