Introduce `PeersIndicator` component by hebasto · Pull Request #127 · bitcoin-core/gui-qml

hebasto · 2022-06-03T17:59:02Z

The reference design: https://bitcoindesign.github.io/Bitcoin-Core-App/assets/images/block-clock-connection-states-big.png

Design features implementation:

a single blinking dot
dots are lit up proportional to the count of the outbound peers

Looking for designers' Concept (N)ACK, and developers' notes regarding QML code.

jarolrod

the design specifies spacing of 5 between each connection dot, and that each connection dot be 3x3, see: https://www.figma.com/file/GaCoOSNHB2yMB9ThiDtred/Bitcoin-Core-App?node-id=3288%3A70945

With these changes, it'll look a bit small in its current demo. But it should look fine when it's a part of the block clock.

jarolrod · 2022-06-03T18:31:57Z

+    implicitHeight: 16
+
+    Row {
+        spacing: 6


Suggested change

spacing: 6

spacing: 5

jarolrod · 2022-06-03T18:32:05Z

+        Repeater {
+            model: root.size
+            delegate: Rectangle {
+                width: 8


Suggested change

width: 8

width: 3

jarolrod · 2022-06-03T18:32:12Z

+            model: root.size
+            delegate: Rectangle {
+                width: 8
+                height: 8


Suggested change

height: 8

height: 3

hebasto · 2022-06-03T19:28:37Z

Fixed circular dependency.

the design specifies spacing of 5 between each connection dot, and that each connection dot be 3x3, see: https://www.figma.com/file/GaCoOSNHB2yMB9ThiDtred/Bitcoin-Core-App?node-id=3288%3A70945

Thanks! Updated.

But now dots look like square pixels :)

cc @GBKS

jarolrod · 2022-06-03T19:38:03Z

@hebasto yes that is true, if we need to scale it, we can scale it from the current proportions. The orginal implementation looked a lot like the PageIndicator we had before.

hebasto · 2022-06-03T19:47:35Z

we can scale it from the current proportions

Talking about scaling, we should scale down, rather up.

shaavan

Concept ACK

The added code changes effectively introduce the progress indicator.
I was able to successfully compile and run the PR on Ubuntu 22.04 with Qt version 5.15.3
As @jarolrod mentioned, following the design guide precisely regarding sizing and spacing leads to dots being relatively smaller than the ones displayed in PR.

Demo:

According to this PR	According design guide

I have attached my notes for this PR.

Notes:

The first commit replaces the “notify only about the total number of nodes change” to “notify about change in any type of nodes”. This change is necessary for the progress indicator component.
1. This is done by introducing a new struct PeersNumByType, which tracks all the types of nodes along with the total number of nodes.
The second commit introduces the variable to track the number of outbound peers connected and the total number of outbound peers that can connect.
The third commit introduces the PageIndicator.qml file, which utilizes the above two variables to display a real-time indicator for the number of peers connected to our node.
Commit four, including the PageIndicator.qml to the stub.qml file, allowing it to be displayed on the screen.

promag · 2022-06-04T10:21:14Z

+    int num_block_relay{0};
+    int num_manual{0};
+    int num_inbound{0};
+    for (const auto* node : nodes_copy) {


36e20da

Correct me if I'm wrong but I think it's safe to use node without AddRef/Release because DeleteNode isn't called concurrently with CConnman::NotifyNumConnectionsChanged.

Do you mind elaborating? What change do you suggest?

That's my point, I think the correct code is fine. Initially, I wondered if some AddRef/Release was needed.

promag · 2022-06-04T10:28:55Z

+import QtQuick 2.15
+import "../controls"
+
+Item {


b0b36d6

This can be Row? Then can drop width and height bindings below.

Otherwise below should bind implicitWidth and implicitHeight.

This can be Row?

Yes, it can. But using Item provides better encapsulation and hiding implementation details. For example, PeersIndicator { padding: 42 } will fail in the current branch.

Otherwise below should bind implicitWidth and implicitHeight.

Thanks! Updated.

hebasto · 2022-06-04T11:32:40Z

Updated 88b99c9 -> aad0ff4 (pr127.02 -> pr127.03, diff):

addressed @promag's comment:

Otherwise below should bind implicitWidth and implicitHeight.

Co-authored-by: Christoph Ono <chri@sto.ph>

hebasto · 2022-06-05T22:32:21Z

Updated aad0ff4 -> 179ca23 (pr127.03 -> pr127.04, diff):

addressed @jarolrod's comment
added blinking of the first dot according to the design
other small improvements (I hope)

jarolrod · 2022-06-06T02:25:51Z

concept ACK on node changes and QML changes. Node changes seem ready to open upstream

jarolrod · 2022-08-05T03:24:03Z

+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+// The PeersIndicator component.


I guess there's no need for this line, just look at the path and name of the file

jarolrod · 2022-08-05T03:24:27Z

+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+// The PeersIndicator component.
+// See reference design: https://bitcoindesign.github.io/Bitcoin-Core-App/assets/images/block-clock-connection-states-big.png


Perhaps every component with a one to one link to a design file should have a link for easy reference to the design spec?

jarolrod · 2023-02-01T00:19:05Z

replaced by #236

hebasto · 2023-02-01T13:37:22Z

replaced by #236

Closed in favour of #236.

c7c73b8 qml: add PeersIndicator to BlockClock (jarolrod) 4f3368a qml: Add `PeersIndicator` component (Hennadii Stepanov) b22b88b qml: Add `NodeModel::numOutboundPeers` property (Hennadii Stepanov) 475c63e net: Notify about connection number by type (Hennadii Stepanov) Pull request description: This is a rebased version of, and replaces, #127 Introduces the actual `PeersIndicator` component and adds it to the `BlockClock` | Dark Mode | Light Mode | | --------- | ---------- | | <img width="752" alt="Screen Shot 2023-01-31 at 12 48 23 AM" src="https://user-images.githubusercontent.com/23396902/215676831-6573a94f-2030-4bf2-b45b-9d325be42600.png"> | <img width="752" alt="Screen Shot 2023-01-31 at 12 49 09 AM" src="https://user-images.githubusercontent.com/23396902/215676882-96f82c03-43ff-449d-b5d9-d7016fb0540b.png"> | [![Windows](https://img.shields.io/badge/OS-Windows-green)](https://api.cirrus-ci.com/v1/artifact/github/bitcoin-core/gui-qml/win64/insecure_win_gui.zip?branch=pull/236) [![Intel macOS](https://img.shields.io/badge/OS-Intel%20macOS-green)](https://api.cirrus-ci.com/v1/artifact/github/bitcoin-core/gui-qml/macos/insecure_mac_gui.zip?branch=pull/236) [![Apple Silicon macOS](https://img.shields.io/badge/OS-Apple%20Silicon%20macOS-green)](https://api.cirrus-ci.com/v1/artifact/github/bitcoin-core/gui-qml/macos_arm64/insecure_mac_arm64_gui.zip?branch=pull/236) [![ARM64 Android](https://img.shields.io/badge/OS-Android-green)](https://api.cirrus-ci.com/v1/artifact/github/bitcoin-core/gui-qml/android/insecure_android_apk.zip?branch=pull/236) ACKs for top commit: johnny9: ACK c7c73b8 Tree-SHA512: f5af2cd53bcecdb9441f412e94ad6557c48aad7998c2da69f1e0174682a0761151ca72b27a094b12d3f9055dff61694b9205297f5a4db9559bd0ed0ff3c4fdca

…dd to BlockClock fc34996 qml: add PeersIndicator to BlockClock (jarolrod) 5d0ea0e qml: Add `PeersIndicator` component (Hennadii Stepanov) f5c2cd0 qml: Add `NodeModel::numOutboundPeers` property (Hennadii Stepanov) 475c63ef4752f7b79eca647072208f3a5be8d56e net: Notify about connection number by type (Hennadii Stepanov) Pull request description: This is a rebased version of, and replaces, bitcoin-core/gui-qml#127 Introduces the actual `PeersIndicator` component and adds it to the `BlockClock` | Dark Mode | Light Mode | | --------- | ---------- | | <img width="752" alt="Screen Shot 2023-01-31 at 12 48 23 AM" src="https://user-images.githubusercontent.com/23396902/215676831-6573a94f-2030-4bf2-b45b-9d325be42600.png"> | <img width="752" alt="Screen Shot 2023-01-31 at 12 49 09 AM" src="https://user-images.githubusercontent.com/23396902/215676882-96f82c03-43ff-449d-b5d9-d7016fb0540b.png"> | [![Windows](https://img.shields.io/badge/OS-Windows-green)](https://api.cirrus-ci.com/v1/artifact/github/bitcoin-core/gui-qml/win64/insecure_win_gui.zip?branch=pull/236) [![Intel macOS](https://img.shields.io/badge/OS-Intel%20macOS-green)](https://api.cirrus-ci.com/v1/artifact/github/bitcoin-core/gui-qml/macos/insecure_mac_gui.zip?branch=pull/236) [![Apple Silicon macOS](https://img.shields.io/badge/OS-Apple%20Silicon%20macOS-green)](https://api.cirrus-ci.com/v1/artifact/github/bitcoin-core/gui-qml/macos_arm64/insecure_mac_arm64_gui.zip?branch=pull/236) [![ARM64 Android](https://img.shields.io/badge/OS-Android-green)](https://api.cirrus-ci.com/v1/artifact/github/bitcoin-core/gui-qml/android/insecure_android_apk.zip?branch=pull/236) ACKs for top commit: johnny9: ACK fc34996 Tree-SHA512: f5af2cd53bcecdb9441f412e94ad6557c48aad7998c2da69f1e0174682a0761151ca72b27a094b12d3f9055dff61694b9205297f5a4db9559bd0ed0ff3c4fdca

…dd to BlockClock fc34996357b8cb7830b1fbc26bc8f91575f88cff qml: add PeersIndicator to BlockClock (jarolrod) 5d0ea0ed5768109a60a86ec1a22e146812a1e021 qml: Add `PeersIndicator` component (Hennadii Stepanov) f5c2cd044833e8365224f382692d27ddcfa8f3d9 qml: Add `NodeModel::numOutboundPeers` property (Hennadii Stepanov) 475c63ef4752f7b79eca647072208f3a5be8d56e net: Notify about connection number by type (Hennadii Stepanov) Pull request description: This is a rebased version of, and replaces, bitcoin-core/gui-qml#127 Introduces the actual `PeersIndicator` component and adds it to the `BlockClock` | Dark Mode | Light Mode | | --------- | ---------- | | <img width="752" alt="Screen Shot 2023-01-31 at 12 48 23 AM" src="https://user-images.githubusercontent.com/23396902/215676831-6573a94f-2030-4bf2-b45b-9d325be42600.png"> | <img width="752" alt="Screen Shot 2023-01-31 at 12 49 09 AM" src="https://user-images.githubusercontent.com/23396902/215676882-96f82c03-43ff-449d-b5d9-d7016fb0540b.png"> | [![Windows](https://img.shields.io/badge/OS-Windows-green)](https://api.cirrus-ci.com/v1/artifact/github/bitcoin-core/gui-qml/win64/insecure_win_gui.zip?branch=pull/236) [![Intel macOS](https://img.shields.io/badge/OS-Intel%20macOS-green)](https://api.cirrus-ci.com/v1/artifact/github/bitcoin-core/gui-qml/macos/insecure_mac_gui.zip?branch=pull/236) [![Apple Silicon macOS](https://img.shields.io/badge/OS-Apple%20Silicon%20macOS-green)](https://api.cirrus-ci.com/v1/artifact/github/bitcoin-core/gui-qml/macos_arm64/insecure_mac_arm64_gui.zip?branch=pull/236) [![ARM64 Android](https://img.shields.io/badge/OS-Android-green)](https://api.cirrus-ci.com/v1/artifact/github/bitcoin-core/gui-qml/android/insecure_android_apk.zip?branch=pull/236) ACKs for top commit: johnny9: ACK fc34996357b8cb7830b1fbc26bc8f91575f88cff Tree-SHA512: f5af2cd53bcecdb9441f412e94ad6557c48aad7998c2da69f1e0174682a0761151ca72b27a094b12d3f9055dff61694b9205297f5a4db9559bd0ed0ff3c4fdca

hebasto changed the title ~~Introduced PeersIndicator component~~ Introduce PeersIndicator component Jun 3, 2022

jarolrod suggested changes Jun 3, 2022

View reviewed changes

net: Notify about connection number by type

36e20da

hebasto force-pushed the 220603-peers branch from 07b0eb3 to 88b99c9 Compare June 3, 2022 19:26

shaavan reviewed Jun 3, 2022

View reviewed changes

Comment thread src/qml/components/PeersIndicator.qml Outdated

promag reviewed Jun 4, 2022

View reviewed changes

hebasto force-pushed the 220603-peers branch from 88b99c9 to aad0ff4 Compare June 4, 2022 11:30

jarolrod reviewed Jun 5, 2022

View reviewed changes

Comment thread src/qml/nodemodel.cpp

hebasto and others added 3 commits June 6, 2022 00:25

qml: Add NodeModel::numOutboundPeers property

5692f16

qml: Add PeersIndicator component

df8e4da

Co-authored-by: Christoph Ono <chri@sto.ph>

qml: Add a PeersIndicator control demo to stub.qml

179ca23

hebasto force-pushed the 220603-peers branch from aad0ff4 to 179ca23 Compare June 5, 2022 22:29

mouxdesign mentioned this pull request Jun 7, 2022

Introduce PeersIndicator component - An alternative design #128

Closed

GBKS mentioned this pull request Jul 13, 2022

Create onboarding flow #6

Closed

hebasto added the UX Designers' opinions are required label Jul 27, 2022

jarolrod reviewed Aug 5, 2022

View reviewed changes

shaavan mentioned this pull request Aug 9, 2022

Introduce the BlockClock control #148

Closed

hebasto added the Needs rebase label Aug 29, 2022

shaavan mentioned this pull request Dec 16, 2022

Simplified version of Block Clock #212

Closed

jarolrod mentioned this pull request Jan 31, 2023

Introduce PeersIndicator component, Add to BlockClock #236

Merged

hebasto closed this Feb 1, 2023

Conversation

hebasto commented Jun 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jarolrod left a comment

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!

hebasto commented Jun 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jarolrod commented Jun 3, 2022

Uh oh!

hebasto commented Jun 3, 2022

Uh oh!

shaavan left a comment

Choose a reason for hiding this comment

Uh oh!

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!

hebasto commented Jun 4, 2022

Uh oh!

Uh oh!

hebasto commented Jun 5, 2022

Uh oh!

jarolrod commented Jun 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jarolrod commented Feb 1, 2023

Uh oh!

hebasto commented Feb 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hebasto commented Jun 3, 2022 •

edited

Loading

hebasto commented Jun 3, 2022 •

edited

Loading

jarolrod commented Jun 6, 2022 •

edited

Loading