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

Enlarge Network Traffic Graph #90

Closed
wants to merge 1 commit into from
Closed

Enlarge Network Traffic Graph #90

wants to merge 1 commit into from

Conversation

RandyMcMillan
Copy link
Contributor

@RandyMcMillan RandyMcMillan commented Sep 14, 2020 •
edited
Loading

This minor GUI change allows for a larger display of the Network Graph.

Before/master:

Screen Shot 2020-09-20 at 1 28 32 AM

After/PR:

Screen Shot 2020-09-20 at 1 36 11 AM

Before/master:

Screen Shot 2020-09-20 at 1 32 07 AM

After/PR:

Screen Shot 2020-09-20 at 1 35 10 AM

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.

@hebasto
Copy link
Member

hebasto commented Sep 15, 2020

@RandyMcMillan

This minor GUI change allows for a larger display of the Network Graph.

On master one can maximize the "Node window" with activated "Network Traffic" at will. How this PR changes that behavior?

I added some accessibility settings as part of an on going personal effort to make the GUI more user friendly for impaired users.

These changes seem unrelated to the PR goal, no?

@RandyMcMillan
Copy link
Contributor Author

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

Screen Shot 2020-09-18 at 4 18 31 PM

This change simply emphasizes the graph and minimizes the drab gray currently occupying a 1/4 of this tabs real estate...

@hebasto
Copy link
Member

hebasto commented Sep 18, 2020

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"?

@RandyMcMillan
Copy link
Contributor Author

RandyMcMillan commented Sep 20, 2020 •
edited
Loading

Before and After added above.

@hebasto - I am currently using

Qt Creator 4.13.0
Based on Qt 5.15.0 (Clang 11.0 (Apple), 64 bit)

Built on Aug 25 2020 10:08:56

I noticed the window icon isn't included when building on macOS.
It appears to be a bug in QT for macOS and hasn't been removed in this PR.

Screen Shot 2020-09-19 at 11 01 55 PM

@maflcko maflcko changed the title gui: Enlarge Network Traffic Graph Enlarge Network Traffic Graph Sep 20, 2020
luke-jr
luke-jr suggested changes Oct 24, 2020
View reviewed changes
src/qt/forms/debugwindow.ui Outdated
Comment on lines 9 to 14
<width>777</width>
<height>475</height>
</rect>
</property>
<property name="windowTitle">
<string>Node window</string>
<string>Node Window</string>
Copy link
Member

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>
Copy link
Member

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

@RandyMcMillan RandyMcMillan mentioned this pull request Oct 25, 2020
Closed
4 tasks
jarolrod
jarolrod suggested changes Nov 5, 2020
View reviewed changes
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>
Copy link
Member

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

@RandyMcMillan
Copy link
Contributor Author

rebased to a commit with the current gui ci configurations

jarolrod
jarolrod approved these changes Nov 12, 2020
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.

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.

@luke-jr
Copy link
Member

luke-jr commented Nov 19, 2020

@RandyMcMillan you thumbs-up'd my review comments, but didn't fix them? O.o

@luke-jr
Copy link
Member

luke-jr commented Nov 20, 2020

Why did you rebase this? Please undo... git reset --hard 7a05be5e47d and force-push.

luke-jr
luke-jr suggested changes Nov 25, 2020
View reviewed changes
src/qt/forms/debugwindow.ui Outdated
<item>
<widget class="QLabel" name="label_24">
<property name="accessibleName">
<string>Recieved</string>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<string>Recieved</string>
<string>Received</string>

Copy link
Contributor Author

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>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<string>Recieved</string>
<string>Received</string>

src/qt/forms/debugwindow.ui Outdated
</size>
</property>
<property name="accessibleName">
<string>Bytes Recieved</string>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<string>Bytes Recieved</string>
<string>Bytes Received</string>

@RandyMcMillan
Copy link
Contributor Author

@luke-jr
The CI timed out. It passes on my travis-ci account.
Please restart.
Thanks

@luke-jr
Copy link
Member

luke-jr commented Nov 30, 2020 •
edited
Loading

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)

@RandyMcMillan
Copy link
Contributor Author

@luke-jr

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
is quite limited. There seems to be room for improvement in qt linting.
Worthwhile work?

@luke-jr
Copy link
Member

luke-jr commented Dec 5, 2020

I only noticed the misspellings because the linter caught them...

@luke-jr
Copy link
Member

luke-jr commented Dec 6, 2020

Again, please don't rebase unless necessary...

git reset --hard 334f0961de8

@RandyMcMillan
Copy link
Contributor Author

RandyMcMillan commented Dec 6, 2020 •
edited
Loading

This PR wasn't passing CI.
All my other PRs are based on 81d5af4 and pass CI with no conflicts.
Help me understand why this PR should stay on a failing commit?
I always welcome feedback - thanks! :)

@hebasto
Copy link
Member

hebasto commented Dec 7, 2020

This PR wasn't passing CI.

Seem all is ok :)

hebasto
hebasto requested changes Dec 25, 2020
View reviewed changes
Copy link
Member

@hebasto hebasto left a 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.

@RandyMcMillan RandyMcMillan marked this pull request as draft January 23, 2021 00:14
@hebasto hebasto mentioned this pull request Apr 30, 2021
Closed
@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

@luke-jr luke-jr luke-jr requested changes

@hebasto hebasto hebasto requested changes

@jarolrod jarolrod jarolrod approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@RandyMcMillan @hebasto @luke-jr @jarolrod