qml: Add PeerDetails page #387

johnny9 · 2024-01-29T14:55:06Z

This commit introduces a new page, PeerDetails, that shows additional information on a Peer after it is selected from the list of Peers. After clicking on the peer, the new page gets pushed onto the settings stack. Two conditions will cause the view to pop, the user clicking the Back at the top left or the peer disconnecting.

For reviewers, I would very much like comments on the conversion of the Stats struct to the QML compatible object that is used.

This commit does not include the disconnect/ban buttons, the top right buttons to cycle through the peers, or the icons that are adjacent to the values.

Link to github actions build artifacts.

src/qml/controls/SettingsButton.qml

src/qml/controls/PeerRadioButton.qml

jarolrod · 2024-01-29T15:24:09Z

src/qml/bitcoin.cpp

@@ -292,6 +294,8 @@ int QmlGuiMain(int argc, char* argv[])
    qmlRegisterSingletonInstance<AppMode>("org.bitcoincore.qt", 1, 0, "AppMode", &app_mode);
    qmlRegisterType<BlockClockDial>("org.bitcoincore.qt", 1, 0, "BlockClockDial");
    qmlRegisterType<LineGraph>("org.bitcoincore.qt", 1, 0, "LineGraph");
+    qmlRegisterUncreatableType<PeerDetailsModel>("org.bitcoincore.qt", 1, 0, "PeerDetailsModel", "");


for reviewers: https://doc.qt.io/qt-6/qqmlengine.html#qmlRegisterUncreatableType

jarolrod · 2024-01-29T15:29:08Z

src/qml/pages/node/PeerDetails.qml

+        clip: true
+        contentWidth: width
+
+        Column {


we need rules and way to wrap these values if for whatever reason they get too long:

i think a max width with an elipses should cover it. I'll see what that looks like.

values now elide

user might want to view all Services
maybe be good to put the missing ones in a tooltip?

also things like onion address should be fully visible imo
#375 (review)

src/qml/pages/node/PeerDetails.qml

johnny9 · 2024-01-29T15:40:37Z

From 845a878 to a660b9c

Removed unused controls (RadioButton and SettingsButton for disconnect/ban)

johnny9 · 2024-01-29T15:43:11Z

Update from a660b9c to 8c2a918

Replaced color white with color neutral9 for Dark mode switching

johnny9 · 2024-01-30T04:36:10Z

Update from ef6e826 to b5e2de7

Fixed lint issues
Constrained text values with elide

MarnixCroes · 2024-01-30T15:27:50Z

b5e2de7 weird jumping when clicking on peer, at Direction, User agent_, Type, Ip and Network
cannot repro at ID

jumping.webm

johnny9 · 2024-01-30T19:10:46Z

b5e2de7 weird jumping when clicking on peer, at Direction, User agent_, Type, Ip and Network cannot repro at ID
jumping.webm

This appears to be due to using the wrong index for the original model. I have updated the branch with a working solution

johnny9 · 2024-01-30T19:11:36Z

Update from b5e2de7 to b7161c5

Don't use the index from the proxy table, instead find the index for the details from the original table model.

MarnixCroes

what was the motivation behind current shown details?
some things like Connection Time (Age), Network, Last Block, Last Transaction are missing

MarnixCroes · 2024-02-01T11:11:25Z

src/qml/pages/node/PeerDetails.qml

+            CoreText { text: "Information"; bold: true; font.pixelSize: 18; horizontalAlignment: Qt.AlignLeft; }
+            Column {
+                width: parent.width
+                PeerKeyValueRow { key: qsTr("IP"); value: details.address }


Suggested change

PeerKeyValueRow { key: qsTr("IP"); value: details.address }

PeerKeyValueRow { key: qsTr("Address"); value: details.address }

can also be onion service/ other networks

MarnixCroes · 2024-02-01T11:20:09Z

src/qml/pages/node/PeerDetails.qml

+                PeerKeyValueRow { key: qsTr("Version"); value: details.version }
+                PeerKeyValueRow { key: qsTr("User agent"); value: details.userAgent }
+                PeerKeyValueRow { key: qsTr("Services"); value: details.services }
+                PeerKeyValueRow { key: qsTr("Transport relay"); value: details.transactionRelay }


Suggested change

PeerKeyValueRow { key: qsTr("Transport relay"); value: details.transactionRelay }

PeerKeyValueRow { key: qsTr("Transaction relay"); value: details.transactionRelay }

MarnixCroes · 2024-02-01T11:24:43Z

src/qml/pages/node/PeerDetails.qml

+            CoreText { text: "Block data"; bold: true; font.pixelSize: 18; horizontalAlignment: Qt.AlignLeft; }
+            Column {
+                width: parent.width
+                PeerKeyValueRow { key: qsTr("Starting block"); value: details.startingHeight }


Starting Block doesn't seem to work correctly, for me this always shows 0
(testing with a synced signet node, normal bitcoin qt it works fine)

I noticed that the Qt gui uses a value from a different struct. I will switch to that and see if that resolves it.

yes it works now

GBKS · 2024-02-02T12:39:08Z

Great work. I tested it out on Android. Looks great overall, just see some inconsistencies to the design in terms of vertical spacing and missing formatting.

For text that does not fit in one line, I think it should just go multi-line. If you shorten it, you then need a way to expand it (which could be a simple tap).

In the image below, the Android screenshot is on the right, and the Figma mock-ups are on the left.

A few questions:

What does a "Synced headers" value of "-1" mean? That the process is not initialized?
What are permissions? What does "N/A" mean?
What does "Mapped AS" mean?
For sent/received, does it maybe make sense to show the percentage that is of the total send/receive? As an indicator of how much your node relies on this peer?

MarnixCroes · 2024-02-03T15:00:03Z

@GBKS

What does a "Synced headers" value of "-1" mean? That the process is not initialized?

yes, it means that that peer didn't announce that info yet

What are permissions? What does "N/A" mean?

any special permissions that have been granted to that peer. N/A means Not Applicable / Not Assessed

What does "Mapped AS" mean?

Mapped Autonomous System, which can be used for diversifying peer selection

For sent/received, does it maybe make sense to show the percentage that is of the total send/receive? As an indicator of how much your node relies on this peer?

As peers are connecting and disconnecting all the time this is quite hard: Do you measure it for all currently connected peers, since the node is up, since first launch... etc.
and these values aren't that meaningful: a higher amount sent/received doesn't necessarily mean that the node relies more on this peer than another peer with a lower amount.

So, I get what you mean, but this shouldn't be added imo

johnny9 · 2024-02-04T04:41:31Z

Great work. I tested it out on Android. Looks great overall, just see some inconsistencies to the design in terms of vertical spacing and missing formatting.

Ill adjust that spacing. Looks like mine is too tight.

For text that does not fit in one line, I think it should just go multi-line. If you shorten it, you then need a way to expand it (which could be a simple tap).

I'll use word wrap on the next update

A few questions:

* What does a "Synced headers" value of "-1" mean? That the process is not initialized

It means that it hasn't synced any headers with this peer yet. -1 is the default vault. I think this should just probably just be a minus as well until this data updates.

* What are permissions? What does "N/A" mean?

These are all of the permission strings "bloomfilter", "noban", "forcerelay" "relay" "mempool" "download" "addr". N/A just means that none are set. These permissions are assigned by using the "-whitelist" configuration option. Here are the description of these flags.
"bloomfilter (allow requesting BIP37 filtered blocks and transactions)",
"noban (do not ban for misbehavior; implies download)",
"forcerelay (relay transactions that are already in the mempool; implies relay)",
"relay (relay even in -blocksonly mode, and unlimited transaction announcements)",
"mempool (allow requesting BIP35 mempool contents)",
"download (allow getheaders during IBD, no disconnect after maxuploadtarget limit)",
"addr (responses to GETADDR avoid hitting the cache and contain random records with the most up-to-date info)"

* What does "Mapped AS" mean?

Mapped AS means Mapped Autonomous System which is a configuration option that lets you map peers to AS numbers. AS numbers are assigned by top level internet registries to organizations with a bunch of network addresses. You specify a mapping file to bitcoin core with a configuration option.

* For sent/received, does it maybe make sense to show the percentage that is of the total send/receive? As an indicator of how much your node relies on this peer?

Yeah I think there is a lot of value in that.

johnny9 · 2024-02-05T04:53:16Z

Update from b7161c5 to d737ea3

Used correct struct for m_starting_height value to fix "Starting Height"
Changed IP to Address
Adjusted spacing to match figma
Allowed Text to WordWrap

MarnixCroes · 2024-02-05T10:21:49Z

running d737ea3, I now had twice that the peer details page closed itself randomly and returned to the peers page.
peer didn't disconnect, not sure what is causing it.

Changed IP to Address

forgot to add the change?

Allowed Text to WordWrap

this looks good 👍

GBKS · 2024-02-05T12:23:30Z

Thanks for the explainers and tweaking the layout.

These permissions are assigned by using the "-whitelist" configuration option

Does that mean permissions are application-wide and the same for all peers? If so, maybe they can be shown elsewhere...?

I think this should just probably just be a minus as well until this data updates.

Are we using "N/A" and "-" interchangeably? Looks like we need states for

Pending: we're still waiting to find out (maybe "..."?)
Not available: we're not waiting anymore, and have no idea ("N/A")
None: none were set ("-")

Does that seem about right? What's the likelihood of getting "not available"? Is that even possible?

johnny9 · 2024-02-05T16:30:03Z

running d737ea3, I now had twice that the peer details page closed itself randomly and returned to the peers page. peer didn't disconnect, not sure what is causing it.

Yeah im worried about this. I will dig into that and make sure my signals are being triggered correctly

GBKS · 2024-02-06T07:58:57Z

I now had twice that the peer details page closed itself randomly and returned to the peers page

I also had this happen.

yashrajd · 2024-02-06T17:27:45Z

Good stuff guys.

In line with external feedback we received in past Bitcoin Core calls, a little tooltip and explainers for these items would be extremely useful.

Even the tip that @johnny9 provided about how peer permissions can be changed would be useful (on this page or somewhere else)

@GBKS

What does a "Synced headers" value of "-1" mean? That the process is not initialized?

yes, it means that that peer didn't announce that info yet

What are permissions? What does "N/A" mean?

any special permissions that have been granted to that peer. N/A means Not Applicable / Not Assessed

What does "Mapped AS" mean?

Mapped Autonomous System, which can be used for diversifying peer selection

For sent/received, does it maybe make sense to show the percentage that is of the total send/receive? As an indicator of how much your node relies on this peer?

As peers are connecting and disconnecting all the time this is quite hard: Do you measure it for all currently connected peers, since the node is up, since first launch... etc. and these values aren't that meaningful: a higher amount sent/received doesn't necessarily mean that the node relies more on this peer than another peer with a lower amount.

So, I get what you mean, but this shouldn't be added imo

GBKS · 2024-02-07T07:09:10Z

For this PR generally, I'm happy to ACK it whenever the functionality is solid and we can tweak visualization details and tips as we live with it during practical usage.

jarolrod · 2024-02-07T04:53:09Z

src/qml/pages/node/PeerDetails.qml

+            anchors.horizontalCenter: parent.horizontalCenter
+            spacing: 10
+
+            CoreText { text: "Information"; bold: true; font.pixelSize: 18; horizontalAlignment: Qt.AlignLeft; }


Every categories CoreText declaration also needs to specify color as Theme.color.neutral9

MarnixCroes

Transport version and Connection Time with the peer should be added imo

johnny9 · 2024-02-10T02:08:03Z

Transport version and Connection Time with the peer should be added imo

I think we need to re-sync with main before we add transport type but I can add Connection Time to the Network traffic section

johnny9 · 2024-02-10T02:38:53Z

Update from b4f8fc1 to c8344d8

Prevented disconnect signal from triggering twice
Set color correctly on the section titles
Added "Connection time"

MarnixCroes · 2024-02-13T13:17:54Z

Transport version and Connection Time with the peer should be added imo

and the Network (ipv4, onion etc),
sorry forgot about that one

MarnixCroes

apart from some minor things, like the dialog sometimes randomly closing.
I think this is good enough and improvements can be made later on

ACK c8344d8

nice work

jarolrod

Almost there for the basic functionality of this page, some spacing is not adherent to the design file

jarolrod · 2024-02-14T04:19:54Z

src/qml/pages/node/PeerDetails.qml

+                PeerKeyValueRow { key: qsTr("Address"); value: details.address }
+                PeerKeyValueRow { key: qsTr("VIA"); value: details.addressLocal }
+                PeerKeyValueRow { key: qsTr("Type"); value: details.type }
+                PeerKeyValueRow { key: qsTr("Permission"); value: details.permission }


Suggested change

PeerKeyValueRow { key: qsTr("Permission"); value: details.permission }

PeerKeyValueRow { key: qsTr("Permissions"); value: details.permission }

jarolrod · 2024-02-14T04:24:16Z

src/qml/pages/node/PeerDetails.qml

+            }
+            Column {
+                width: parent.width
+                spacing: 5


The spacing between sections of information is actually 15, so we'd want to add a bottom margin of 5

diff --git a/src/qml/pages/node/PeerDetails.qml b/src/qml/pages/node/PeerDetails.qml index 224aef8c7..b32c2a0c7 100644 --- a/src/qml/pages/node/PeerDetails.qml +++ b/src/qml/pages/node/PeerDetails.qml @@ -79,6 +79,7 @@ Page { Column { width: parent.width spacing: 5 + bottomPadding: 5 PeerKeyValueRow { key: qsTr("Address"); value: details.address } PeerKeyValueRow { key: qsTr("VIA"); value: details.addressLocal } PeerKeyValueRow { key: qsTr("Type"); value: details.type } @@ -101,6 +102,7 @@ Page { Column { width: parent.width spacing: 5 + bottomPadding: 5 PeerKeyValueRow { key: qsTr("Starting block"); value: details.startingHeight } PeerKeyValueRow { key: qsTr("Synced headers"); value: details.syncedHeaders } PeerKeyValueRow { key: qsTr("Synced blocks"); value: details.syncedBlocks } @@ -116,6 +118,7 @@ Page { Column { width: parent.width spacing: 5 + bottomPadding: 5 PeerKeyValueRow { key: qsTr("Direction"); value: details.direction } PeerKeyValueRow { key: qsTr("Connection time"); value: details.connectionDuration } PeerKeyValueRow { key: qsTr("Last send"); value: details.lastSend }

See here, this starts to look like the design file

master patch

Design file screenshot of section spacing:

jarolrod · 2024-02-14T04:28:40Z

src/qml/pages/node/PeerDetails.qml

+        Column {
+            width: Math.min(parent.width - 40, 450)
+            anchors.horizontalCenter: parent.horizontalCenter
+            spacing: 10


we want to add top and bottom padding of 30 to adhere to the design file

Suggested change

spacing: 10

spacing: 10

topPadding: 30

bottomPadding: 30

See here, this starts to look like the design file

master patch

Design file screenshot of top padding:

jarolrod · 2024-02-14T04:43:36Z

src/qml/pages/node/PeerDetails.qml

+    component PeerKeyValueRow: Row {
+        width: parent.width
+        property string key: ""
+        property string value: ""


also

Suggested change

property string value: ""

property string value: ""

spacing: 10

johnny9 · 2024-02-14T05:27:04Z

Update from c8344d8 to eb5030f

incorporated all of @jarolrod suggestions (good eye)
set height of PeerKeyValueRow to 21 as specified by the Figma and removed the spacing value for the Columns that hold those Rows.

MarnixCroes

eb5030f the new spacing can cause some overlap

johnny9 · 2024-02-14T14:24:27Z

eb5030f the new spacing can cause some overlap

Thank you, updated with the height attached to the lineCount of the value

component PeerKeyValueRow: Row {
        width: parent.width
        property string key: ""
        property string value: ""
        height: 21 * valueText.lineCount
        spacing: 10
        CoreText {
            color: Theme.color.neutral9;
            text: key;
            width: 125;
            horizontalAlignment: Qt.AlignLeft;
        }
        CoreText {
            id: valueText
            color: Theme.color.neutral9;
            elide: Text.ElideRight;
            wrapMode: Text.WordWrap;
            text: value
            width: parent.width - 125;
            horizontalAlignment: Qt.AlignLeft;
        }
    }

MarnixCroes

ack fa4e080

yashrajd · 2024-02-14T15:44:58Z

Just ran the latest artefact, and it looks pretty good. Didn't see any spacing issues. ACK.

GBKS · 2024-02-15T10:59:33Z

Thank you, updated with the height attached to the lineCount of the value

Do I see it correctly that the line height is hard-coded? Users can typically adjust text size via accessibility settings, which means the line height will also differ. Is there a way to automatically calculate this value?

In Figma and CSS, I usually define the line height as a percentage (see how that is defined in CSS here). Is that possible? Might be easier to work with.

jarolrod · 2024-02-28T09:56:41Z

#388 is proposed as an alternative which handles some of the outstanding issues, implements more functionality

johnny9 · 2024-03-01T02:55:02Z

Closing this as it is superseded with #388

02d3121 qml: Add PeerDetails page (johnny9) 1ed2188 qml: Introduce KeyValueRow control (jarolrod) 052b5be qml: Introduce minus icon (jarolrod) Pull request description: Alternative to #387 which expands to think about a reusable row component, allowing to load any element(s) to the value field, not hardcoding in a height of 21 for a row (but still ensuring it's a minimum of 21), proper icon usage for N/A cases, dummy net indicators in place TODO: - Ban buttons - Hover hints - Disconnected state instead of flying back to the peers table - Smart text fixups <img width="752" alt="Screenshot 2024-02-28 at 3 53 43 AM" src="https://github.com/bitcoin-core/gui-qml/assets/23396902/7b3b8f7f-f490-4233-a540-736e34760116"> Link to github actions build artifacts. [![Build Artifacts](https://img.shields.io/badge/Build%20Artifacts-green )]() ACKs for top commit: MarnixCroes: tACK 02d3121 D33r-Gee: tACK 02d3121 pablomartin4btc: utACK 02d3121 Tree-SHA512: 26e3782e4cac2629bbeef40815edeaaddf59f91ec8f63017e5b8c223c4060c0a535b2640a7dbf34e4b25a20fa887a6bc109f7d57c5864a6d9896832db7f87b9f

jarolrod reviewed Jan 29, 2024

View reviewed changes

src/qml/pages/node/PeerDetails.qml Outdated Show resolved Hide resolved

johnny9 force-pushed the peer-details branch from 845a878 to a660b9c Compare January 29, 2024 15:38

johnny9 force-pushed the peer-details branch from a660b9c to 8c2a918 Compare January 29, 2024 15:42

johnny9 force-pushed the peer-details branch 4 times, most recently from 480c03f to b5e2de7 Compare January 30, 2024 04:27

johnny9 force-pushed the peer-details branch from b5e2de7 to b7161c5 Compare January 30, 2024 19:09

MarnixCroes reviewed Feb 1, 2024

View reviewed changes

johnny9 force-pushed the peer-details branch from b7161c5 to d737ea3 Compare February 5, 2024 04:51

johnny9 force-pushed the peer-details branch from d737ea3 to bea2699 Compare February 7, 2024 03:04

jarolrod reviewed Feb 7, 2024

View reviewed changes

johnny9 force-pushed the peer-details branch from bea2699 to b4f8fc1 Compare February 8, 2024 04:26

MarnixCroes suggested changes Feb 8, 2024

View reviewed changes

johnny9 force-pushed the peer-details branch from b4f8fc1 to c8344d8 Compare February 10, 2024 02:21

MarnixCroes approved these changes Feb 13, 2024

View reviewed changes

jarolrod reviewed Feb 14, 2024

View reviewed changes

johnny9 force-pushed the peer-details branch from c8344d8 to eb5030f Compare February 14, 2024 05:25

MarnixCroes reviewed Feb 14, 2024

View reviewed changes

qml: Add PeerDetails page

fa4e080

johnny9 force-pushed the peer-details branch from eb5030f to fa4e080 Compare February 14, 2024 14:15

MarnixCroes approved these changes Feb 14, 2024

View reviewed changes

jarolrod mentioned this pull request Feb 28, 2024

Add PeerDetails page #388

Merged

johnny9 closed this Mar 1, 2024

mouxdesign mentioned this pull request Mar 1, 2024

Documenting the process BitcoinDesign/Bitcoin-Core-App#24

Open

	PeerKeyValueRow { key: qsTr("IP"); value: details.address }
	PeerKeyValueRow { key: qsTr("Address"); value: details.address }

	PeerKeyValueRow { key: qsTr("Transport relay"); value: details.transactionRelay }
	PeerKeyValueRow { key: qsTr("Transaction relay"); value: details.transactionRelay }

	PeerKeyValueRow { key: qsTr("Permission"); value: details.permission }
	PeerKeyValueRow { key: qsTr("Permissions"); value: details.permission }

	property string value: ""
	property string value: ""
	spacing: 10

qml: Add PeerDetails page #387

qml: Add PeerDetails page #387

Conversation

johnny9 commented Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnny9 commented Jan 29, 2024

johnny9 commented Jan 29, 2024

johnny9 commented Jan 30, 2024

MarnixCroes commented Jan 30, 2024

johnny9 commented Jan 30, 2024

johnny9 commented Jan 30, 2024

MarnixCroes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnny9 Feb 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GBKS commented Feb 2, 2024

MarnixCroes commented Feb 3, 2024

johnny9 commented Feb 4, 2024

johnny9 commented Feb 5, 2024

MarnixCroes commented Feb 5, 2024

GBKS commented Feb 5, 2024

johnny9 commented Feb 5, 2024

GBKS commented Feb 6, 2024

yashrajd commented Feb 6, 2024

GBKS commented Feb 7, 2024

Choose a reason for hiding this comment

MarnixCroes left a comment

Choose a reason for hiding this comment

johnny9 commented Feb 10, 2024

johnny9 commented Feb 10, 2024

MarnixCroes commented Feb 13, 2024

MarnixCroes left a comment

Choose a reason for hiding this comment

jarolrod left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnny9 commented Feb 14, 2024 • edited Loading

MarnixCroes left a comment

Choose a reason for hiding this comment

johnny9 commented Feb 14, 2024 • edited Loading

MarnixCroes left a comment

Choose a reason for hiding this comment

yashrajd commented Feb 14, 2024 • edited Loading

GBKS commented Feb 15, 2024 • edited Loading

jarolrod commented Feb 28, 2024

johnny9 commented Mar 1, 2024

johnny9 commented Jan 29, 2024 •

edited

Loading

johnny9 Feb 5, 2024 •

edited

Loading

johnny9 commented Feb 14, 2024 •

edited

Loading

johnny9 commented Feb 14, 2024 •

edited

Loading

yashrajd commented Feb 14, 2024 •

edited

Loading

GBKS commented Feb 15, 2024 •

edited

Loading