Add abitlity to resize peerTable and banTable by shaavan · Pull Request #457 · bitcoin-core/gui

shaavan · 2021-10-25T18:17:23Z

Till now, peerTable and banTable under the peerTab were fixed in height and cannot be resized. This creates unnecessary inconvenience, especially when you have many items on one table and not many on another.

This PR proposes to add a splitter between these two tables, allowing them to be resized freely.

Master	PR

Saibato · 2021-10-26T17:53:21Z

concept ACK

I like the resize. Are they connected or active traffic?

shaavan · 2021-10-27T11:59:41Z

Are they connected or active traffic?

I think they both mean the same thing in this context.
Connected peers, implies the peer that we are currently connected to, indicating the active traffic.

jarolrod

This shouldn't display until there is a Banned Peers table

Additionally, I shouldn't be able to hide any of the tables

src/qt/forms/debugwindow.ui

luke-jr

The new label is redundant: the tab name itself indicates what the user is viewing.

Concept ACK to adding a splitter.

shaavan · 2021-11-01T12:32:52Z

Updated 5e54cfe to c09e6ad (pr457.01 -> pr457.02)
Addressed @jarolrod comments

Updates:

Considering that naming peerTable was not gathering a lot of support. I have removed that change from this PR.
Made the peerTable and banTable non-collapsible.
Increased splitter’s height from 3px to 4px.

This shouldn't display until there is a Banned Peers table

@jarolrod. As you rightly mentioned, showing splitter when no BanTable was looking kind of off. But in the new iteration of this PR, the splitter seems like an element of peerTable and looks aesthetically pleasing even when there is no banTable. I have updated the PR description with the new Screenshots. You can look at them to better get the feel of what I am trying to say.

jarolrod

Getting there, but the splitter should behave like the splitter that currently exists between the Peers Table and the Peer Information Pane.

When I select a Peer, I get the Peer Information Pane, as well as a splitter where I can resize as needed. When the peer is de-selected, the splitter disappears; because there is nothing to resize.

Similarly, until I have a banned peer and a banned peer table, the splitter being introduced here should not be displayed at all because there is nothing to resize. Additionally, if i unban a peer and subsequently no longer have a banned peers table, the splitter should disappear as well.

shaavan · 2021-11-03T13:45:55Z

Updated from c09e6ad to aa6229e (pr457.02 -> pr457.03, diff)
Addressed @jarolrod comments

Changes:

Instead of hiding individual children of banTable. The code is now updated to hide the banTable widget. This will make sure the splitter will not be displayed when there is no banTable.
Updated sizePolicy to better reflect the peerWidget and the banTable widget.
Added code to save and restore the state (position) of the splitter. The state is remembered even in the case when GUI is shut down and restarted.

shaavan · 2021-11-05T03:30:01Z

cc @jarolrod. PR ready for another review!

src/qt/forms/debugwindow.ui

kristapsk

ACK aa6229e (tested under Linux / Xfce4)

promag

Concept ACK.

promag · 2021-11-11T18:09:43Z

src/qt/forms/debugwindow.ui

-              <set>Qt::NoTextInteraction</set>
+            <widget class="QSplitter" name="tableSplitter">
+             <property name="styleSheet">
+              <string notr="true">QSplitter::handle{background: #a6a6a6;}</string>


Why this style?

I did this to make the splitter visible and looks aesthetically pleasing.
Do you recommend keeping the splitter invisible?

What do you think about this? @promag

Do you recommend keeping the splitter invisible?

I don't think the hardcoded color will work for any platform and appearance (dark/light), no?

I don't think the hardcoded color will work for any platform and appearance (dark/light), no?

The spiller looks fine for the default light mode (in Linux) and dark mode in macOS, which can be seen in the screenshots of the following comments: light; dark.

However, this doesn't guarantee that this will work for any custom appearance. Do you recommend removing the hardcoded color and making the splitter invisible again?

Why not keeping the same look as for the horizontal splitter?

Do you recommend keeping the splitter invisible?

What does make you think so?

Do you recommend keeping the splitter invisible?

By this, I meant to say, keeping it the same as the horizontal splitter.

Why not keep the same look as for horizontal splitter?

I think this will be a more optimal approach to go with.

But the horizontal splitter is not invisible :)

And having different looks for similar GUI elements does not as an optimal design for me.

At least, introducing a new GUI element, and changing GUI element style across the GUI are two different tasks.

Fixed splitter color to default palette in 0c36420

w0xlt

tACK aa6229e . Tested on Ubuntu 21.10.

hebasto · 2021-11-21T21:04:15Z

Concept ACK.

hebasto · 2021-12-27T11:09:13Z

src/qt/rpcconsole.cpp

+        //Setup Peer and Ban Table Splitter
+        ui->tableSplitter->setChildrenCollapsible(false);
+


This code could be a part of the debugwindow.ui file as it is for the horizontal splitter.

Qt Designer tool could very helpful for such tasks.

Addressed in 0c36420

hebasto · 2021-12-27T11:09:52Z

src/qt/forms/debugwindow.ui

-      <number>0</number>
+      <number>3</number>


Why this change?

This was an auto-generated change by QtCreator. Fixed in 0c36420

DrahtBot · 2021-12-30T10:53:51Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#540 (network graph - show/hide panels based on window width/height by RandyMcMillan)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

shaavan · 2022-01-07T18:27:57Z

Updated from aa6229e to 0c36420(pr457.04 -> pr457.05)
Addressed @hebasto and @randymacmilan comments

Removed custom color of the added splitter
Moved childrenCollapsible=False to debugwindow.ui file.
Removed auto-generated changes by QtCreator.

hebasto

Approach ACK 0c36420.

Could you please separate a refactoring commit which introduces the banTable widget?

hebasto · 2022-06-26T18:16:22Z

src/qt/forms/debugwindow.ui

           </size>
          </property>
-          <layout class="QVBoxLayout" name="verticalLayout_7">
+          <layout class="QVBoxLayout" name="verticalLayout_9">


Is this change required?

Since this PR introduced a new sub-Layout, the introduced sub-Layout was named in the chronological order of its introduction. And since verticalLayout_7 already exists (on Line 935), giving this the same name leads to a warning message while compiling:

qt/forms/debugwindow.ui: Warning: The name 'verticalLayout_7' (QVBoxLayout) is already in use, defaulting to 'verticalLayout_71'.

So, I am letting this change be as it is for now, and I welcome any suggestions you might have.

- This is a pure refactoring change and is done so that the table splitter can be added between peerTable and banTable in the following commit.

This commit adds a splitter between the peerWidget and the banTable widget. This adds the ability to resize these windows to the user’s needs.

shaavan · 2022-07-02T13:24:58Z

Updated from 0c36420 to 5a9bdce (pr457.05 -> pr457.06, diff)

Addressed @hebasto comment

Changes:

Rebased PR over the current master.
Separated adding of banTable and tableSplitter in separate commits.
Removed unnecessary width/height changes.

jarolrod

While the resizing works, the UI here does not. The UI element representing the splitter is too small and is pinned to the bottom of peerTable.

linux	macOS

hebasto · 2022-10-26T15:54:17Z

Closing this as it has not had any activity in a while. If you are interested in continuing work on this, please leave a comment so that it can be reopened.

jarolrod added the Feature label Oct 26, 2021

jarolrod reviewed Oct 28, 2021

View reviewed changes

src/qt/forms/debugwindow.ui Outdated Show resolved Hide resolved

luke-jr suggested changes Oct 29, 2021

View reviewed changes

shaavan force-pushed the peer-table-splitter branch from 5e54cfe to c09e6ad Compare November 1, 2021 12:31

jarolrod reviewed Nov 2, 2021

View reviewed changes

shaavan force-pushed the peer-table-splitter branch from c09e6ad to aa6229e Compare November 3, 2021 13:45

kristapsk reviewed Nov 9, 2021

View reviewed changes

src/qt/forms/debugwindow.ui Show resolved Hide resolved

kristapsk approved these changes Nov 10, 2021

View reviewed changes

promag reviewed Nov 11, 2021

View reviewed changes

w0xlt approved these changes Nov 15, 2021

View reviewed changes

hebasto added the UI All about "look and feel" label Nov 21, 2021

hebasto changed the title ~~qt: Add abitlity to resize peerTable and banTable~~ Add abitlity to resize peerTable and banTable Dec 14, 2021

hebasto reviewed Dec 27, 2021

View reviewed changes

DrahtBot mentioned this pull request Dec 30, 2021

Hide peers details #505

Merged

shaavan force-pushed the peer-table-splitter branch from aa6229e to 0c36420 Compare January 7, 2022 18:25

hebasto reviewed Jun 26, 2022

View reviewed changes

shaavan added 2 commits July 2, 2022 18:19

qt, refactor: Group banHeading and banlistWidget in debugwindow.ui

4b9f349

- This is a pure refactoring change and is done so that the table splitter can be added between peerTable and banTable in the following commit.

qt: Add ability to resize Peer and Ban table

5a9bdce

This commit adds a splitter between the peerWidget and the banTable widget. This adds the ability to resize these windows to the user’s needs.

shaavan force-pushed the peer-table-splitter branch from 0c36420 to 5a9bdce Compare July 2, 2022 13:24

DrahtBot mentioned this pull request Jul 3, 2022

network graph - show/hide panels based on window width/height #540

Closed

jarolrod reviewed Aug 6, 2022

View reviewed changes

hebasto closed this Oct 26, 2022

bitcoin-core locked and limited conversation to collaborators Oct 26, 2023

		//Setup Peer and Ban Table Splitter
		ui->tableSplitter->setChildrenCollapsible(false);

Comments

Conversation

shaavan commented Oct 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Saibato commented Oct 26, 2021

Uh oh!

shaavan commented Oct 27, 2021

Uh oh!

jarolrod left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

shaavan commented Nov 1, 2021

Uh oh!

jarolrod left a comment

Choose a reason for hiding this comment

Uh oh!

shaavan commented Nov 3, 2021

Uh oh!

shaavan commented Nov 5, 2021

Uh oh!

Uh oh!

kristapsk left a comment

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto Dec 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto Dec 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto Dec 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto commented Nov 21, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Dec 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

shaavan commented Jan 7, 2022

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

shaavan commented Oct 25, 2021 •

edited

Loading

hebasto Dec 27, 2021 •

edited

Loading

hebasto Dec 27, 2021 •

edited

Loading

hebasto Dec 27, 2021 •

edited

Loading

DrahtBot commented Dec 30, 2021 •

edited

Loading