peers-tab: cleaner presentation - more functionality by RandyMcMillan · Pull Request #135 · bitcoin-core/gui

RandyMcMillan · 2020-11-24T23:44:28Z

Initial Presentation:

peer table populated:

Detail View of selected peer:

Window enlarged:
Peertable and detail view:

Additional Notes:

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

luke-jr · 2020-11-30T18:13:53Z

src/qt/forms/debugwindow.ui

    <widget class="QTabWidget" name="tabWidget">
     <property name="currentIndex">
-      <number>0</number>
+      <number>3</number>


Leave this alone...

luke-jr · 2020-11-30T18:20:21Z

src/qt/forms/debugwindow.ui

    <widget class="QTabWidget" name="tabWidget">
     <property name="currentIndex">
-      <number>0</number>
+      <number>3</number>


Leave this alone...

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

luke-jr · 2020-11-30T18:53:34Z

src/qt/forms/debugwindow.ui

+             <property name="statusTip">
+              <string/>
+             </property>
+             <property name="whatsThis">
+              <string/>
+             </property>
+             <property name="accessibleName">
+              <string/>
+             </property>


This shouldn't be here

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

#135 (comment)

luke-jr · 2020-11-30T18:54:16Z

src/qt/forms/debugwindow.ui

+          <property name="toolTip">
+           <string/>
+          </property>
+          <property name="statusTip">
+           <string/>
+          </property>


More pointless code

#135 (comment)

luke-jr · 2020-11-30T18:54:45Z

src/qt/forms/debugwindow.ui

+          <property name="whatsThis">
+           <string>Peer Detail</string>
+          </property>


Not sure this is useful

luke-jr · 2020-11-30T18:55:34Z

src/qt/forms/debugwindow.ui

+             <property name="whatsThis">
+              <string>Detailed Info of Selected Node</string>
             </property>


Is this reachable? (Didn't we remove the "What's This" button?)

luke-jr · 2020-11-30T19:08:18Z

src/qt/forms/debugwindow.ui

+                 <property name="accessibleName">
+                  <string/>
+                 </property>


#135 (comment)

luke-jr · 2020-11-30T19:09:24Z

src/qt/guiutil.cpp

+        if (flag == "NETWORK_LIMITED"){
+        strList.append(QString::fromStdString("LIMITED"));


If we're going to start renaming these, "recent-blocks" makes more sense than "LIMITED"...

luke-jr · 2020-11-30T19:10:14Z

src/qt/guiutil.cpp


    if (strList.size())
-        return strList.join(" & ");
+        return strList.join(" ");


This seems less clear

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

luke-jr · 2020-11-30T19:11:09Z

src/qt/guiutil.cpp

+    else
+        return QObject::tr(""); //silence is golden


This conditional seems pointless

luke-jr · 2020-11-30T19:11:45Z

src/qt/guiutil.cpp

+{
+    QStringList strList;
+
+    for (const auto& flag : serviceFlagsToStr(mask)) {


Probably better to re-implement serviceFlagsToStr here...

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.

jonasschnelli · 2020-12-01T10:00:17Z

Nice. Concept ACK.

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.

RandyMcMillan

Autogenerated gui elements are necessary for VALID code.

RandyMcMillan · 2020-12-03T20:31:29Z

src/qt/forms/debugwindow.ui

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


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

hebasto · 2020-12-25T11:08:38Z

@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 (#162 (comment), #162 (review)).

It would help make progress :)

jonatack · 2020-12-25T19:34:50Z

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 · 2020-12-28T23:02:14Z

🐙 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".}

DrahtBot added the Needs rebase label Nov 25, 2020

RandyMcMillan marked this pull request as draft November 25, 2020 00:48

RandyMcMillan marked this pull request as ready for review November 25, 2020 07:10

RandyMcMillan changed the title ~~[WIP] peers-tab cleaner presentation - more info - functionality improvements~~ peers-tab cleaner presentation - more info - functionality improvements Nov 25, 2020

RandyMcMillan changed the title ~~peers-tab cleaner presentation - more info - functionality improvements~~ peers-tab: cleaner presentation - more info - functionality improvements Nov 25, 2020

RandyMcMillan marked this pull request as draft November 26, 2020 00:23

luke-jr reviewed Nov 30, 2020

View reviewed changes

luke-jr suggested changes Nov 30, 2020

View reviewed changes

promag reviewed Dec 2, 2020

View reviewed changes

RandyMcMillan marked this pull request as ready for review December 3, 2020 04:20

RandyMcMillan changed the title ~~peers-tab: cleaner presentation - more info - functionality improvements~~ peers-tab: cleaner presentation - more functionality Dec 3, 2020

RandyMcMillan marked this pull request as draft December 3, 2020 05:27

RandyMcMillan commented Dec 3, 2020

View reviewed changes

RandyMcMillan marked this pull request as ready for review December 6, 2020 20:22

DrahtBot removed the Needs rebase label Dec 6, 2020

RandyMcMillan mentioned this pull request Dec 7, 2020

about:version:help clean up add Bitcoin branding #140

Closed

promag mentioned this pull request Dec 9, 2020

splash: New layout #147

Closed

DrahtBot added the Needs rebase label Dec 22, 2020

RandyMcMillan mentioned this pull request Dec 24, 2020

Add network to peers window and peer details #162

Merged

RandyMcMillan marked this pull request as draft December 24, 2020 19:35

peers-tab: cleaner presentation more functionality

91b7e23

RandyMcMillan marked this pull request as ready for review December 25, 2020 01:24

DrahtBot removed the Needs rebase label Dec 25, 2020

DrahtBot added the Needs rebase label Dec 28, 2020

RandyMcMillan mentioned this pull request Jan 18, 2021

Add Type column to peers window, update peer details name/tooltip #179

Merged

hebasto mentioned this pull request Jan 20, 2021

User Agent header flickers when trying to resize any column #191

Closed

RandyMcMillan marked this pull request as draft January 23, 2021 00:10

RandyMcMillan closed this Jan 23, 2021

RandyMcMillan mentioned this pull request Apr 24, 2021

Add "Direction" column to peers tab #289

Closed

RandyMcMillan mentioned this pull request May 12, 2021

Peers Window rename 'Peer id' to 'Peer' #311

Merged

bitcoin-core locked as resolved and limited conversation to collaborators Aug 16, 2022

		if (flag == "NETWORK_LIMITED"){
		strList.append(QString::fromStdString("LIMITED"));

Conversation

RandyMcMillan commented Nov 24, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Dec 1, 2020

Uh oh!

promag left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RandyMcMillan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto commented Dec 25, 2020

Uh oh!

jonatack commented Dec 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Dec 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

promag left a comment •

edited

Loading

jonatack commented Dec 25, 2020 •

edited

Loading