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

peers-tab: cleaner presentation - more functionality #135

Closed
wants to merge 1 commit into from
Closed

peers-tab: cleaner presentation - more functionality #135

wants to merge 1 commit into from

Conversation

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

@RandyMcMillan RandyMcMillan marked this pull request as draft November 25, 2020 00:48
@RandyMcMillan RandyMcMillan marked this pull request as ready for review November 25, 2020 07:10
@RandyMcMillan 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 RandyMcMillan changed the title peers-tab cleaner presentation - more info - functionality improvements peers-tab: cleaner presentation - more info - functionality improvements Nov 25, 2020
@RandyMcMillan RandyMcMillan marked this pull request as draft November 26, 2020 00:23
@@ -36,7 +36,7 @@
<item>
<widget class="QTabWidget" name="tabWidget">
<property name="currentIndex">
<number>0</number>
<number>3</number>
Copy link
Member

Choose a reason for hiding this comment

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

Leave this alone...

@@ -36,7 +36,7 @@
<item>
<widget class="QTabWidget" name="tabWidget">
<property name="currentIndex">
<number>0</number>
<number>3</number>
Copy link
Member

Choose a reason for hiding this comment

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

Leave this alone...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Comment on lines 926 to 937
<property name="statusTip">
<string/>
</property>
<property name="whatsThis">
<string/>
</property>
<property name="accessibleName">
<string/>
</property>
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines 1028 to 1033
<property name="toolTip">
<string/>
</property>
<property name="statusTip">
<string/>
</property>
Copy link
Member

Choose a reason for hiding this comment

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

More pointless code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines 1034 to 1033
<property name="whatsThis">
<string>Peer Detail</string>
</property>
Copy link
Member

Choose a reason for hiding this comment

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

Not sure this is useful

Comment on lines 1058 to 1060
<property name="whatsThis">
<string>Detailed Info of Selected Node</string>
</property>
Copy link
Member

Choose a reason for hiding this comment

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

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

Comment on lines 1390 to 1380
<property name="accessibleName">
<string/>
</property>
Copy link
Member

Choose a reason for hiding this comment

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

blank

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines +783 to +784
if (flag == "NETWORK_LIMITED"){
strList.append(QString::fromStdString("LIMITED"));
Copy link
Member

Choose a reason for hiding this comment

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

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

}

if (strList.size())
return strList.join(" & ");
return strList.join(" ");
Copy link
Member

Choose a reason for hiding this comment

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

This seems less clear

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

else
return QObject::tr(""); //silence is golden
Copy link
Member

Choose a reason for hiding this comment

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

This conditional seems pointless

{
QStringList strList;

for (const auto& flag : serviceFlagsToStr(mask)) {
Copy link
Member

Choose a reason for hiding this comment

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

Probably better to re-implement serviceFlagsToStr here...

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

@jonasschnelli
Copy link
Contributor

Nice. Concept ACK.

Copy link
Contributor

@promag promag 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. 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 RandyMcMillan marked this pull request as ready for review December 3, 2020 04:20
@RandyMcMillan RandyMcMillan changed the title peers-tab: cleaner presentation - more info - functionality improvements peers-tab: cleaner presentation - more functionality Dec 3, 2020
@RandyMcMillan RandyMcMillan marked this pull request as draft December 3, 2020 05:27
Copy link
Contributor Author

@RandyMcMillan RandyMcMillan left a comment

Choose a reason for hiding this comment

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

Autogenerated gui elements are necessary for VALID code.

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

Choose a reason for hiding this comment

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

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 RandyMcMillan marked this pull request as ready for review December 25, 2020 01:24
@hebasto
Copy link
Member

hebasto commented Dec 25, 2020

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

jonatack commented Dec 25, 2020

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
Copy link
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 RandyMcMillan marked this pull request as draft January 23, 2021 00:10
@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants