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.

Sign up for GitHub

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

Jump to bottom

qt: peers-tab resizeColumnsToContents #338

Closed
wants to merge 1 commit into from
Closed

qt: peers-tab resizeColumnsToContents #338

wants to merge 1 commit into from

Conversation

RandyMcMillan
Copy link
Contributor

No description provided.

@RandyMcMillan
Copy link
Contributor Author

Related to #256

This PR handles resizing column widths programmatically.

@RandyMcMillan
Copy link
Contributor Author

Fields that commonly need to be resized to view are resized based on content length.

Screen Shot 2021-05-17 at 2 35 09 AM

@hebasto hebasto added the UI All about "look and feel" label May 17, 2021
jarolrod
jarolrod reviewed May 17, 2021
View reviewed changes
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.

I'm failing to understand what this is trying to address, can you clarify a bit? 🤔

src/qt/bantablemodel.cpp
@@ -120,6 +122,8 @@ QVariant BanTableModel::data(const QModelIndex &index, int role) const
const auto column = static_cast<ColumnIndex>(index.column());
if (role == Qt::DisplayRole) {
switch (column) {
case Bumper:
return QString::fromStdString("");
Copy link
Member

Choose a reason for hiding this comment

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

why does this need to be QString::fromStdString("")

src/qt/peertablemodel.cpp
return {};
} // no default case, so the compiler can warn about missing cases
case Subversion:
return QVariant(Qt::AlignLeft | Qt::AlignVCenter);
Copy link
Member

Choose a reason for hiding this comment

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

isn't this unrelated to what you want to do?

src/qt/peertablesortproxy.cpp
@@ -38,6 +38,8 @@ bool PeerTableSortProxy::lessThan(const QModelIndex& left_index, const QModelInd
return left_stats.nRecvBytes < right_stats.nRecvBytes;
case PeerTableModel::Subversion:
return left_stats.cleanSubVer.compare(right_stats.cleanSubVer) < 0;
case PeerTableModel::Bumper:
return left_stats.nodeid < right_stats.nodeid;
Copy link
Member

Choose a reason for hiding this comment

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

Why this sorting scheme?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorting by nodeid was simply to have sorting functionality for that column as opposed to no sorting functionality.

@promag
Copy link
Contributor

promag commented May 17, 2021

Please describe the goal, what is trying to improve and how it relates to #256. Looks like #256 conflicts with QHeaderView::ResizeToContents?

@RandyMcMillan
Copy link
Contributor Author

Goal of this PR/Discussion
  • Ideally formatting the initial column widths can reduce the number of click events the user needs to perform to be able to view the data presented.
  • Reducing click events where ever possible is ideal for normal users & tremendously valuable for scenarios where the user doesn't interact with this software primarily with mouse/touch methods.
Motivations for /gui fork as far as I can tell...
  • It is rooted in improving UI/UX (User Experience)
  • REVIEW ISO 9241-210:2019(en)
  • It is really impractical to require /gui PRs have to restate basic GUI/UI/UX motivations every time.

@hebasto
Copy link
Member

hebasto commented May 23, 2021

Concept NACK.

There are some concerns about QHeaderView::ResizeToContents.

  1. It increases complexity in such a way that could lead, at some scale, to deteriorating the GUI responsiveness
  2. From Qt docs:

The size cannot be changed by the user...

Usually, in this project, we strive do not make decisions instead of users in such questions :)

  1. Some users could feel uncomfortable with possible continuous reflowing of the peers table. I'm among of them.

@jarolrod
Copy link
Member

Concept NACK

This unnecessarily restricts the user-configurability of the GUI. Testing this PR, I am no longer able to resize the tabs on my own to my liking. Per Qt docs, this is because of the use of QHeaderView::ResizeToContents.

In general, we should ONLY restrict user freedom when there is good reason and motivation to do so. Tables are meant to be resized to one's liking; not everyone wants to see the User Agent string, and some may not want to see the full Address string (this is one reason why we have #276).

Users of the GUI do want to customize the sizing of various aspects of the interface and have that saved and restored. This is a precedent that has been set by the direction GUI development has taken, See #165, #194, #205, and #256.

@DrahtBot
Copy link
Contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".

@RandyMcMillan RandyMcMillan marked this pull request as draft May 26, 2021 23:14
@rebroad
Copy link
Contributor

rebroad commented Jun 26, 2021

Can we make it do this only until the user tries to manually resize a column?

@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.
Reviewers

@jarolrod jarolrod jarolrod left review comments

Assignees
No one assigned
Labels
Needs rebase UI All about "look and feel"
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@RandyMcMillan @promag @hebasto @jarolrod @DrahtBot @rebroad