-
Notifications
You must be signed in to change notification settings - Fork 267
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
Enlarge Network Traffic Graph #90
Conversation
On master one can maximize the "Node window" with activated "Network Traffic" at will. How this PR changes that behavior?
These changes seem unrelated to the PR goal, no? |
@hebasto - this change has to do with better use of space within the confines of the tab itself. As you can see - there is a lot of unused space in the presentation... This change simply emphasizes the graph and minimizes the drab gray currently occupying a 1/4 of this tabs real estate... |
Concept ACK on reordering child widgets. @RandyMcMillan Could you place a screenshot "before/master" in the OP to visually compare with the screenshot of "after/PR"? |
Before and After added above. @hebasto - I am currently using
I noticed the window icon isn't included when building on macOS. |
src/qt/forms/debugwindow.ui
Outdated
<width>777</width> | ||
<height>475</height> | ||
</rect> | ||
</property> | ||
<property name="windowTitle"> | ||
<string>Node window</string> | ||
<string>Node Window</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please leave these alone
src/qt/forms/debugwindow.ui
Outdated
@@ -36,7 +36,7 @@ | |||
<item> | |||
<widget class="QTabWidget" name="tabWidget"> | |||
<property name="currentIndex"> | |||
<number>0</number> | |||
<number>2</number> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to change this
src/qt/forms/debugwindow.ui
Outdated
@@ -1074,8 +1087,8 @@ | |||
<rect> | |||
<x>0</x> | |||
<y>0</y> | |||
<width>300</width> | |||
<height>426</height> | |||
<width>274</width> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are you including changes to tab_peers in this? While the peers tab should be redesigned to maximize space, I don't think this PR (focused on the network traffic graph) should carry any changes to tab_peers
rebased to a commit with the current gui ci configurations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested ACK 5036d9a. Built and visually verified on Arch Linux (Kernel 5.8.16) and macOS 10.15.7 both with Qt 5.15. Everything looks good to me.
@RandyMcMillan you thumbs-up'd my review comments, but didn't fix them? O.o |
Why did you rebase this? Please undo... |
src/qt/forms/debugwindow.ui
Outdated
<item> | ||
<widget class="QLabel" name="label_24"> | ||
<property name="accessibleName"> | ||
<string>Recieved</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<string>Recieved</string> | |
<string>Received</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you - I value your input.
src/qt/forms/debugwindow.ui
Outdated
<string>Recieved</string> | ||
</property> | ||
<property name="accessibleDescription"> | ||
<string>Recieved</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<string>Recieved</string> | |
<string>Received</string> |
src/qt/forms/debugwindow.ui
Outdated
</size> | ||
</property> | ||
<property name="accessibleName"> | ||
<string>Bytes Recieved</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<string>Bytes Recieved</string> | |
<string>Bytes Received</string> |
@luke-jr |
I don't know how to restart CI on this GUI repo... but presumably if you fix those misspellings, it will re-run... (please don't rebase in the process) |
I realized that I can trigger my own rebuilds after I posted. Really useful. Also: I noticed codespell ignores the /forms folder and test/lint/lint-qt.sh |
I only noticed the misspellings because the linter caught them... |
Again, please don't rebase unless necessary...
|
This PR wasn't passing CI. |
Seem all is ok :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have reviewed the code (8b79225), and tested it.
I think that adding accessibleName
and accessibleDescription
properties is out of scope of this PR, and they should be removed.
This minor GUI change allows for a larger display of the Network Graph.
Before/master:
After/PR:
Before/master:
After/PR:
I capitalized "Window" in the title per standard "Human Interface Guidelines" and common sense.
I added some accessibility settings as part of an on going personal effort to make the GUI more user friendly for impaired users.