Network Graph layout - debug window improvement. #291

pull RandyMcMillan wants to merge 1 commits into bitcoin-core:master from bitcoincore-dev:net-graph-layout changing 1 files +69 −81
  1. RandyMcMillan commented at 11:03 pm on April 23, 2021: contributor

    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

  2. RandyMcMillan commented at 11:54 pm on April 23, 2021: contributor
  3. RandyMcMillan commented at 0:33 am on April 24, 2021: contributor
  4. Bosch-0 commented at 2:16 am on April 24, 2021: none

    tACK https://github.com/bitcoin-core/gui/pull/291/commits/99f6f43bdd05ed83fca462c0514d6157bad1025b on

    Windows 10.

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

    Frame 35

  5. in src/qt/forms/debugwindow.ui:663 in 99f6f43bdd outdated
    656@@ -657,6 +657,181 @@
    657          </item>
    658          <item>
    659           <layout class="QHBoxLayout" name="horizontalLayout_2">
    660+           <item>
    661+            <widget class="QGroupBox" name="groupBox_2">
    662+             <property name="toolTip">
    663+              <string extracomment="Totals"/>
    


    jonatack commented at 3:51 pm on April 24, 2021:
    How does a user see this tooltip?

    RandyMcMillan commented at 6:05 pm on April 24, 2021:
    thanks - removed

    RandyMcMillan commented at 6:09 pm on April 24, 2021:
    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.

    jonatack commented at 10:21 pm on April 24, 2021:
    Thanks, makes sense and is unrelated to this change. Will re-review tomorrow.
  6. jonatack commented at 3:54 pm on April 24, 2021: contributor

    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.

  7. RandyMcMillan force-pushed on Apr 24, 2021
  8. Talkless commented at 1:13 pm on April 25, 2021: none
    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.
  9. RandyMcMillan commented at 3:13 pm on April 25, 2021: contributor
    there is plenty of room for more tabs. 😄
  10. hebasto added the label Design on Apr 25, 2021
  11. jonatack commented at 4:52 pm on April 25, 2021: contributor
    ACK 1cdcdb30ce57ab3847a839dcb71570c2c5ef0e36
  12. Talkless commented at 5:45 pm on April 26, 2021: none

    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.

  13. jarolrod commented at 4:56 am on April 30, 2021: member

    tACK 1cdcdb30ce57ab3847a839dcb71570c2c5ef0e36

    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

  14. hebasto commented at 11:28 am on April 30, 2021: member

    ~Concept ACK.~ Withdrawn in #291 (comment).

    IIUC this is a reincarnation of #90.

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

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

  15. in src/qt/forms/debugwindow.ui:854 in 1cdcdb30ce outdated
    857-              </size>
    858-             </property>
    859-             <property name="alignment">
    860-              <set>Qt::AlignCenter</set>
    861+             <property name="text">
    862+              <string>Range</string>
    


    hebasto commented at 11:37 am on April 30, 2021:

    Why this change?

    This label will be overwritten in https://github.com/bitcoin-core/gui/blob/480bf01c295527bd212964efe4df3bb886db5654/src/qt/rpcconsole.cpp#L1006

    and it is unnecessary burden to translate the “Range” string.

  16. hebasto commented at 11:58 am on April 30, 2021: member

    horizontalLayout_3 is useless now:

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

    With this change

     0--- a/src/qt/forms/debugwindow.ui
     1+++ b/src/qt/forms/debugwindow.ui
     2@@ -642,9 +642,7 @@
     3       <attribute name="title">
     4        <string>&amp;Network Traffic</string>
     5       </attribute>
     6-      <layout class="QHBoxLayout" name="horizontalLayout_3">
     7-       <item>
     8-        <layout class="QVBoxLayout" name="verticalLayout_4">
     9+      <layout class="QVBoxLayout" name="verticalLayout_4">
    10          <item>
    11           <widget class="TrafficGraphWidget" name="trafficGraph" native="true">
    12            <property name="sizePolicy">
    13@@ -867,8 +865,6 @@
    14            </item>
    15           </layout>
    16          </item>
    17-        </layout>
    18-       </item>
    19       </layout>
    20      </widget>
    21      <widget class="QWidget" name="tab_peers">
    

    we have

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

  17. RandyMcMillan force-pushed on Apr 30, 2021
  18. hebasto commented at 7:58 pm on April 30, 2021: member

    ~Approach ACK 0174e68fd59460b53a187bf396cb54e523185eb1~ 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:

     0            <widget class="QLabel" name="lblGraphRange">
     1             <property name="minimumSize">
     2              <size>
     3               <width>100</width>
     4               <height>0</height>
     5              </size>
     6             </property>
     7             <property name="alignment">
     8              <set>Qt::AlignCenter</set>
     9             </property>
    10            </widget>
    

    This PR:

    0            <widget class="QLabel" name="lblGraphRange">
    1            </widget>
    
  19. RandyMcMillan commented at 8:47 pm on April 30, 2021: contributor

    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.

  20. RandyMcMillan force-pushed on Apr 30, 2021
  21. Talkless commented at 5:44 pm on May 3, 2021: none

    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?

  22. RandyMcMillan commented at 8:54 pm on May 5, 2021: contributor
    yes @Talkless - that is an option. I will take a look at that.
  23. RandyMcMillan force-pushed on May 16, 2021
  24. RandyMcMillan commented at 7:35 pm on May 16, 2021: contributor

    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

  25. RandyMcMillan force-pushed on May 16, 2021
  26. RandyMcMillan force-pushed on May 16, 2021
  27. in src/qt/forms/debugwindow.ui:802 in 1f373f93a6 outdated
    840            </layout>
    841           </widget>
    842          </item>
    843+         <item>
    844+          <widget class="QSlider" name="sldGraphRange">
    845+           <property name="minimumSize">
    


    RandyMcMillan commented at 7:58 pm on May 16, 2021:
    (re) setting minimum size to address sizing issue when time interval changes
  28. in src/qt/forms/debugwindow.ui:827 in 1f373f93a6 outdated
    865+           </property>
    866+          </widget>
    867+         </item>
    868+         <item>
    869+          <widget class="QLabel" name="lblGraphRange">
    870+           <property name="minimumSize">
    


    RandyMcMillan commented at 7:58 pm on May 16, 2021:
    (re) setting minimum size to address sizing issue when time interval changes
  29. RandyMcMillan commented at 8:08 pm on May 16, 2021: contributor

    Slider Issue Fixed:

    slider

  30. RandyMcMillan marked this as a draft on Jul 22, 2021
  31. shaavan commented at 8:13 pm on August 6, 2021: contributor

    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
  32. hebasto commented at 12:05 pm on August 7, 2021: member

    I’ve reconsidered my opinion, and withdraw my ACKs (https://github.com/bitcoin-core/gui/pull/291#issuecomment-830029755, #291#pullrequestreview-649577005).

    Agree with @Talkless’s point (https://github.com/bitcoin-core/gui/pull/291#issuecomment-826322546, #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.

  33. rebroad commented at 11:03 am on August 25, 2021: contributor

    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?

  34. in src/qt/forms/debugwindow.ui:638 in 1f373f93a6 outdated
    726+            <bool>true</bool>
    727+           </property>
    728+           <layout class="QVBoxLayout" name="verticalLayout_9">
    729             <item>
    730-             <layout class="QHBoxLayout" name="horizontalLayout_4">
    731+             <layout class="QHBoxLayout" name="RecievedBox">
    


    luke-jr commented at 8:52 pm on October 10, 2021:
    typo here and 2 more

    luke-jr commented at 8:52 pm on October 10, 2021:
    Recieved ==> Received
  35. RandyMcMillan force-pushed on Nov 10, 2021
  36. RandyMcMillan force-pushed on Nov 14, 2021
  37. RandyMcMillan force-pushed on Nov 21, 2021
  38. RandyMcMillan force-pushed on Nov 21, 2021
  39. Network Graph layout 500841e49d
  40. RandyMcMillan force-pushed on Nov 21, 2021
  41. RandyMcMillan marked this as ready for review on Nov 21, 2021
  42. RandyMcMillan commented at 0:40 am on November 21, 2021: contributor

    Looks good with PR #473 as well…

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

  43. RandyMcMillan commented at 0:42 am on November 21, 2021: contributor

    Minimum size presentation:

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

  44. rebroad commented at 9:04 pm on November 24, 2021: contributor

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

  45. RandyMcMillan closed this on Dec 14, 2021

  46. bitcoin-core locked this on Dec 14, 2022

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/gui. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-23 02:20 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me