peers-tab: cleaner presentation - more functionality

RandyMcMillan commented at 11:44 pm on November 24, 2020: contributor

Initial Presentation:

Screen Shot 2020-11-24 at 6 27 28 PM

peer table populated:

Screen Shot 2020-11-24 at 6 27 36 PM

Detail View of selected peer:

Screen Shot 2020-11-24 at 6 27 50 PM

Window enlarged: Peertable and detail view: Screen Shot 2020-11-24 at 6 30 57 PM

Additional Notes:

The widgets were renamed to offer a “self documenting” naming convention during the development.

Screen Shot 2020-11-24 at 6 39 18 PM

DrahtBot added the label Needs rebase on Nov 25, 2020

RandyMcMillan marked this as a draft on Nov 25, 2020

RandyMcMillan marked this as ready for review on Nov 25, 2020

RandyMcMillan renamed this:
~~[WIP] peers-tab cleaner presentation - more info - functionality improvements~~
peers-tab cleaner presentation - more info - functionality improvements
on Nov 25, 2020

RandyMcMillan renamed this:
~~peers-tab cleaner presentation - more info - functionality improvements~~
peers-tab: cleaner presentation - more info - functionality improvements
on Nov 25, 2020

RandyMcMillan marked this as a draft on Nov 26, 2020

in src/qt/forms/debugwindow.ui:39 in 2bf8c2489f outdated

35@@ -36,7 +36,7 @@
36    <item>
37     <widget class="QTabWidget" name="tabWidget">
38      <property name="currentIndex">
39-      <number>0</number>
40+      <number>3</number>

luke-jr commented at 6:13 pm on November 30, 2020:

Leave this alone…

luke-jr commented at 6:20 pm on November 30, 2020:

Leave this alone…

RandyMcMillan commented at 9:37 pm on December 2, 2020:

This is Qt Creator diff junk - I will correct this if/when ready for merge. Thanks

in src/qt/forms/debugwindow.ui:937 in 2bf8c2489f outdated

929+             <property name="whatsThis">
930+              <string/>
931+             </property>
932+             <property name="accessibleName">
933+              <string/>
934+             </property>

luke-jr commented at 6:53 pm on November 30, 2020:

This shouldn’t be here

RandyMcMillan commented at 9:37 pm on December 2, 2020:

This is Qt Creator diff junk - I will correct this if/when ready for merge. Thanks

RandyMcMillan commented at 8:49 pm on December 3, 2020:

#135 (review)

in src/qt/forms/debugwindow.ui:1033 in 2bf8c2489f outdated

1024@@ -1001,45 +1025,45 @@
1025             <height>0</height>
1026            </size>
1027           </property>
1028+          <property name="toolTip">
1029+           <string/>
1030+          </property>
1031+          <property name="statusTip">
1032+           <string/>
1033+          </property>

luke-jr commented at 6:54 pm on November 30, 2020:

More pointless code

RandyMcMillan commented at 8:49 pm on December 3, 2020:

#135 (review)

in src/qt/forms/debugwindow.ui:1033 in 2bf8c2489f outdated

1031+          <property name="statusTip">
1032+           <string/>
1033+          </property>
1034+          <property name="whatsThis">
1035+           <string>Peer Detail</string>
1036+          </property>

luke-jr commented at 6:54 pm on November 30, 2020:

Not sure this is useful

in src/qt/forms/debugwindow.ui:1060 in 2bf8c2489f outdated

1071              </property>
1072-             <property name="alignment">
1073-              <set>Qt::AlignHCenter|Qt::AlignTop</set>
1074+             <property name="whatsThis">
1075+              <string>Detailed Info of Selected Node</string>
1076              </property>

luke-jr commented at 6:55 pm on November 30, 2020:

Is this reachable? (Didn’t we remove the “What’s This” button?)

in src/qt/forms/debugwindow.ui:1056 in 2bf8c2489f outdated

1065-              <cursorShape>IBeamCursor</cursorShape>
1066-             </property>
1067-             <property name="text">
1068-              <string>Select a peer to view detailed information.</string>
1069+             <property name="statusTip">
1070+              <string>Select a Node from List</string>

luke-jr commented at 6:56 pm on November 30, 2020:

Where is this visible?

in src/qt/forms/debugwindow.ui:1229 in 2bf8c2489f outdated

1242                  <property name="cursor">
1243                   <cursorShape>IBeamCursor</cursorShape>
1244                  </property>
1245+                 <property name="accessibleName">
1246+                  <string/>
1247+                 </property>

luke-jr commented at 7:07 pm on November 30, 2020:

blank

in src/qt/forms/debugwindow.ui:1380 in 2bf8c2489f outdated

1393                  <property name="cursor">
1394                   <cursorShape>IBeamCursor</cursorShape>
1395                  </property>
1396+                 <property name="accessibleName">
1397+                  <string/>
1398+                 </property>

luke-jr commented at 7:08 pm on November 30, 2020:

blank

RandyMcMillan commented at 8:49 pm on December 3, 2020:

#135 (review)

in src/qt/guiutil.cpp:784 in 2bf8c2489f outdated

779 {
780     QStringList strList;
781 
782     for (const auto& flag : serviceFlagsToStr(mask)) {
783+        if (flag == "NETWORK_LIMITED"){
784+        strList.append(QString::fromStdString("LIMITED"));

luke-jr commented at 7:09 pm on November 30, 2020:

If we’re going to start renaming these, “recent-blocks” makes more sense than “LIMITED”…

in src/qt/guiutil.cpp:794 in 2bf8c2489f outdated

790+        sortLocaleAware(strList);
791     }
792 
793     if (strList.size())
794-        return strList.join(" & ");
795+        return strList.join(" ");

luke-jr commented at 7:10 pm on November 30, 2020:

This seems less clear

RandyMcMillan commented at 2:01 am on December 3, 2020:

This change allows the services string in the detail view to break. The ampersands are extraneous (imo).

Screen Shot 2020-12-02 at 8 51 38 PM

in src/qt/guiutil.cpp:824 in 2bf8c2489f outdated

821+    }
822+
823+    if (strList.size())
824+        return strList.join("");
825+    else
826+        return QObject::tr(""); //silence is golden

luke-jr commented at 7:11 pm on November 30, 2020:

This conditional seems pointless

in src/qt/guiutil.cpp:803 in 2bf8c2489f outdated

799 
800+QString shortFormatServicesStr(quint64 mask)
801+{
802+    QStringList strList;
803+
804+    for (const auto& flag : serviceFlagsToStr(mask)) {

luke-jr commented at 7:11 pm on November 30, 2020:

Probably better to re-implement serviceFlagsToStr here…

RandyMcMillan commented at 2:10 am on December 3, 2020:

The “Bumper” will be used to add more columns to the peertablemodel. The point of having abbreviated services flags is to conserve space so more information can be displayed in the table view. The underlying design principle is to display as much useful info as possible “at a glance” as well as allowing those columns to be sortable. Connection time and synced blocks will be added in a follow up PR.

Screen Shot 2020-12-02 at 9 03 14 PM

luke-jr changes_requested

jonasschnelli commented at 10:00 am on December 1, 2020: contributor

Nice. Concept ACK.

promag commented at 5:03 pm on December 2, 2020: contributor

Concept ACK. You should split in multiple commits - renames, add more column(s), etc. Splitting in multiple commits also allows to split the PR so part of it can be early merged. You also have unrelated changes, like in RPCConsole::tabShortcut, that should be removed.

RandyMcMillan marked this as ready for review on Dec 3, 2020

RandyMcMillan renamed this:
~~peers-tab: cleaner presentation - more info - functionality improvements~~
peers-tab: cleaner presentation - more functionality
on Dec 3, 2020

RandyMcMillan marked this as a draft on Dec 3, 2020

in src/qt/forms/debugwindow.ui:922 in 44173c76dd outdated

919@@ -920,9 +920,30 @@
920           <layout class="QVBoxLayout" name="verticalLayout_7">
921            <item>
922             <widget class="QTableView" name="peerWidget">

RandyMcMillan commented at 8:31 pm on December 3, 2020:

Upon further investigation I see that Qt Creator validates these types of files and generates these elements when Qt expects them. If they are not present when Qt expects them they generate an innocuous log - So they aren’t unnecessary code - these elements are created by Qt when it expects them and is VALID code. I recommend stop considering these auto generated elements as “unnecessary code”.

Screen Shot 2020-12-03 at 3 26 46 PM

RandyMcMillan commented at 8:39 pm on December 3, 2020: contributor

Autogenerated gui elements are necessary for VALID code.

RandyMcMillan marked this as ready for review on Dec 6, 2020

DrahtBot removed the label Needs rebase on Dec 6, 2020

DrahtBot added the label Needs rebase on Dec 22, 2020

RandyMcMillan marked this as a draft on Dec 24, 2020

peers-tab: cleaner presentation more functionality 91b7e23053

RandyMcMillan marked this as ready for review on Dec 25, 2020

DrahtBot removed the label Needs rebase on Dec 25, 2020

hebasto commented at 11:08 am on December 25, 2020: member

@promag

Concept ACK. You should split in multiple commits - renames, add more column(s), etc. Splitting in multiple commits also allows to split the PR so part of it can be early merged. You also have unrelated changes, like in RPCConsole::tabShortcut, that should be removed.

I’m completely agree. @RandyMcMillan For example, you could split out the I/O column adding into a separated PR (https://github.com/bitcoin-core/gui/pull/162#issuecomment-750879842, #162#pullrequestreview-558799912).

It would help make progress :)

jonatack commented at 7:34 pm on December 25, 2020: contributor

I agree with @promag and @hebasto. Smaller, more-focused commits and PRs would be a good idea here. Make it easy for people to review and approve your changes (you’re not alone, I struggle with this myself).

DrahtBot added the label Needs rebase on Dec 28, 2020

DrahtBot commented at 11:02 pm on December 28, 2020: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

RandyMcMillan marked this as a draft on Jan 23, 2021

RandyMcMillan closed this on Jan 23, 2021

bitcoin-core locked this on Aug 16, 2022

peers-tab: cleaner presentation - more functionality #135