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

Network Graph layout - debug window improvement. #291

Closed

Conversation

RandyMcMillan
Copy link
Contributor

@RandyMcMillan RandyMcMillan commented Apr 23, 2021

Rearrange the views for (imo) better layout and debug window resizing options.

Screen Shot 2021-04-23 at 6 29 30 PM

Removing "lblGraphRange" minimumSize enables the view to be reduced to the size of a desktop "widget" :)

https://github.com/bitcoin-core/gui/pull/291/files#diff-a24601363160c5ffd048f45a763e702c988b067f96e48a816c36f855f9820826L681

Screen Shot 2021-04-23 at 6 28 00 PM

The peers tab resizes if the detail view is revealed.

Screen Shot 2021-04-23 at 6 29 15 PM

Screen Shot 2021-04-23 at 6 29 21 PM

Info tab still useful at the "widget" size.

Screen Shot 2021-04-23 at 6 28 11 PM

Console tab still functional at "widget" size.

Screen Shot 2021-04-23 at 6 29 04 PM

@RandyMcMillan
Copy link
Contributor Author

@RandyMcMillan
Copy link
Contributor Author

@Bosch-0
Copy link

Bosch-0 commented Apr 24, 2021

tACK 99f6f43 on

Windows 10.

Looks good to me, agree this does look slightly more clean. Other windows seem fine. Before / After screens:

Frame 35

Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

Tested Concept ACK. This seems like a nice design improvement.

Suggest reviewers look at the diff with colorMoved = dimmed-zebra and colorMovedWs = allow-indentation-change.

Is the slider working correctly? It seems like I lose the traffic history if I move it. Edit: bug(?) seems to be the same on current master.

<item>
<widget class="QGroupBox" name="groupBox_2">
<property name="toolTip">
<string extracomment="Totals"/>
Copy link
Member

Choose a reason for hiding this comment

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

How does a user see this tooltip?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks - removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The traffic history always took time to repaint.
I doubt there is an easy fix - it would require caching excessive data in anticipation of a resolution change.
I would lean toward prudent disk space usage over this inconvenience.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, makes sense and is unrelated to this change. Will re-review tomorrow.

@Talkless
Copy link

Idunno about Network Info tab. It's now inconsistent with Peers tab, where details are shown on the right.
What if we get more Network metrics in the future, not just received/sent? Keeping right pane allows for future growth.

@RandyMcMillan
Copy link
Contributor Author

there is plenty of room for more tabs. 😄

@hebasto hebasto added the Design label Apr 25, 2021
@jonatack
Copy link
Member

ACK 1cdcdb3

@Talkless
Copy link

there is plenty of room for more tabs. smile

What I mean is if there's more data series needed (in addition to "Sent", and "Received") on the same graph, not about new tabs.

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.

tACK 1cdcdb3

Screen Shot 2021-04-30 at 12 52 39 AM

Tested on macOS 11.3 Qt 5.15.2. I agree that this is a better use of space.

@RandyMcMillan
You may want to fix your commit author; it's currently authored by git. You may want to change that back to you.

@Talkless

What I mean is if there's more data series needed (in addition to "Sent", and "Received") on the same graph, not about new tabs.

That can be figured out when new data series are proposed

@hebasto
Copy link
Member

hebasto commented Apr 30, 2021

Concept ACK. Withdrawn in #291 (comment).

IIUC this is a reincarnation of #90.

$ git log -1
commit 1cdcdb30ce57ab3847a839dcb71570c2c5ef0e36 (HEAD -> pr291-0430)
Author: git <git@gits-Air.deepspace.dynalias.org>
Date:   Mon Sep 14 17:21:44 2020 -0400

    Network Graph layout

^ this (the authorship) should be fixed before merging.

@hebasto
Copy link
Member

hebasto commented Apr 30, 2021

horizontalLayout_3 is useless now:

Screenshot from 2021-04-30 14-45-45

With this change

--- a/src/qt/forms/debugwindow.ui
+++ b/src/qt/forms/debugwindow.ui
@@ -642,9 +642,7 @@
       <attribute name="title">
        <string>&amp;Network Traffic</string>
       </attribute>
-      <layout class="QHBoxLayout" name="horizontalLayout_3">
-       <item>
-        <layout class="QVBoxLayout" name="verticalLayout_4">
+      <layout class="QVBoxLayout" name="verticalLayout_4">
          <item>
           <widget class="TrafficGraphWidget" name="trafficGraph" native="true">
            <property name="sizePolicy">
@@ -867,8 +865,6 @@
            </item>
           </layout>
          </item>
-        </layout>
-       </item>
       </layout>
      </widget>
      <widget class="QWidget" name="tab_peers">

we have

Screenshot from 2021-04-30 14-51-15

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.

Approach ACK 0174e68 Withdrawn in #291 (comment).

A slider glitching was introduced:

gui291-0430-1

I think the root of this bug is the changed layout of the sldGraphRange, lblGraphRange and btnClearTrafficGraph widgets.

On master:

            <widget class="QLabel" name="lblGraphRange">
             <property name="minimumSize">
              <size>
               <width>100</width>
               <height>0</height>
              </size>
             </property>
             <property name="alignment">
              <set>Qt::AlignCenter</set>
             </property>
            </widget>

This PR:

            <widget class="QLabel" name="lblGraphRange">
            </widget>

@RandyMcMillan
Copy link
Contributor Author

Great catch! In #90 the peers tab toggle detailView wasn't merged yet! So this wasn't an issue.
Re-adding the properties now.

NOTE: I would have reused and rebased PR #90 - For some reason gh wouldn't let me reopen it.

@Talkless
Copy link

Talkless commented May 3, 2021

What I mean is if there's more data series needed (in addition to "Sent", and "Received") on the same graph, not about new tabs.

That can be figured out when new data series are proposed

So we gonna move back to the right? :) . Maybe adding a splitter, making right pane collapsible would help if someone wants "full screen like" graph?

@RandyMcMillan
Copy link
Contributor Author

yes @Talkless - that is an option. I will take a look at that.

@RandyMcMillan
Copy link
Contributor Author

RandyMcMillan commented May 16, 2021

Update: 1f373f9

Landscape Layout:

Screen Shot 2021-05-16 at 3 32 09 PM

Widget Layout:

Screen Shot 2021-05-16 at 3 32 00 PM

@RandyMcMillan RandyMcMillan force-pushed the net-graph-layout branch 2 times, most recently from 8cc2bba to 1f373f9 Compare May 16, 2021 19:55
</layout>
</widget>
</item>
<item>
<widget class="QSlider" name="sldGraphRange">
<property name="minimumSize">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

(re) setting minimum size to address sizing issue when time interval changes

</item>
<item>
<widget class="QLabel" name="lblGraphRange">
<property name="minimumSize">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

(re) setting minimum size to address sizing issue when time interval changes

@RandyMcMillan
Copy link
Contributor Author

Slider Issue Fixed:

slider

@RandyMcMillan RandyMcMillan marked this pull request as draft July 22, 2021 17:12
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.

Concept ACK
Tested Successfully on Ubuntu 20.04

The New Layout for the Network Traffic Window looks much better than the current master.
It is also not affecting the functionality of other Node Windows.
Nice Work!
The bug discussed here is also fixed. This was done by op by setting a minimum size of 60px for the lblGraphRange.
But I think op should increase the minimum size a little more because it is causing some overflow issues. Adding a screenshot of the problem:
Screenshot from 2021-08-07 01-06-56(1)
Overflowing of time (in the blue box near the slider)

Other than this the PR seems to work excellently
Adding some screenshots for comparison.

On Maximum Size:

Master PR
Screenshot from 2021-08-07 00-11-07 Screenshot from 2021-08-07 00-10-02

On Minimum Size:

Master PR
Screenshot from 2021-08-07 00-11-00(2) Screenshot from 2021-08-07 00-09-48

Difference in Layout (Screenshot from Qt Designer):

Master PR
Screenshot from 2021-08-07 00-52-00 Screenshot from 2021-08-07 00-47-58

@hebasto
Copy link
Member

hebasto commented Aug 7, 2021

I've reconsidered my opinion, and withdraw my ACKs (#291 (comment), #291 (review)).

Agree with @Talkless's point (#291 (comment), #291 (comment), #291 (comment)).

One of such changes, that require the current layout, are "Received" and "Send" counters separated by data type: blocks, transactions, addresses, service messages. Especially, this could be helpful with upcoming Erlay protocol (at least for me, of course).

So, for now, Concept NACK from me.

@rebroad
Copy link
Contributor

rebroad commented Aug 25, 2021

Can we make it so the default is 3 hours not 30 minutes? Or make it so that this can be configured somewhere?

Also, can we make it so that when the time range is changed that the data is not lost?

<item>
<layout class="QHBoxLayout" name="horizontalLayout_4">
<layout class="QHBoxLayout" name="RecievedBox">
Copy link
Member

Choose a reason for hiding this comment

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

typo here and 2 more

Copy link
Member

Choose a reason for hiding this comment

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

Recieved ==> Received

@RandyMcMillan RandyMcMillan force-pushed the net-graph-layout branch 2 times, most recently from fef97d9 to f044190 Compare November 14, 2021 18:19
@RandyMcMillan RandyMcMillan force-pushed the net-graph-layout branch 2 times, most recently from a170c84 to 21ed56e Compare November 21, 2021 00:35
@RandyMcMillan RandyMcMillan marked this pull request as ready for review November 21, 2021 00:38
@RandyMcMillan
Copy link
Contributor Author

Looks good with PR #473 as well...

Screen Shot 2021-11-20 at 7 39 55 PM

@RandyMcMillan
Copy link
Contributor Author

Minimum size presentation:

Screen Shot 2021-11-20 at 7 41 06 PM

@rebroad
Copy link
Contributor

rebroad commented Nov 24, 2021

Minimum size presentation:

Screen Shot 2021-11-20 at 7 41 06 PM

I like this new format - also, it makes it more possible to add a button above/below reset which could be used to toggle between linear and non-linear (rather than clicking on the graph).

@bitcoin-core bitcoin-core locked and limited conversation to collaborators Dec 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants