Introduce the BlockClock control by shaavan · Pull Request #148 · bitcoin-core/gui-qml

shaavan · 2022-08-09T12:29:21Z

This PR introduces the Block clock control and adds it to the stub.qml to display.
This PR implements the syncing state of the Block Clock.
The implementation follows the design guide and uses this prototype as the reference.

Screenshot:

Things to note for the current implementation:

Since this is the prototype for the Block clock, I focused on the implementation part and took some liberties with the nuances, which I would like to document here.

The color values for the: Remaining time text and Background color of the block clock ring didn't match the exact color values in the Theme.qml file. So I have used the closet matching Theme.color values to avoid hardcoded values and maintain consistency between light and dark mode.
Since there was no "yellow" color in the Theme.qml, I have used a hardcoded value for the progress indicator.
I have used the "bitcoin app" icon instead of the Black and White bitcoin icon, as this required additions to the resources and would be best done in a separate PR.
I have used static dots as the peer indicator. PR Introduce PeersIndicator component #127 introduces an implementation of the peer indicator and has a largely positive review. This PR could be updated with Introduce PeersIndicator component #127's implementation when that gets merged.

jarolrod · 2022-08-09T13:02:16Z

  qml/components/StorageLocations.qml \
  qml/components/StorageOptions.qml \
  qml/components/StorageSettings.qml \
+  qml/controls/BlockClock.qml \


this is a component. not a control

The next update of my PR intends to make the BlockClock.qml file a purely abstract structure and introduce a component file that uses it to paint different states of the block clock as needed. So I think moving BlockClock.qml to components now would be a wasted effort.

Should you still think I shall move the BlockClock.qml to controls, or if there is a better approach for achieving the final BlockClock state, I would love to hear that.

jarolrod · 2022-08-10T03:43:53Z

+        }
+    }
+
+    Canvas {


instead of canvas, this should be the simpler ShapePath: https://doc.qt.io/qt-6/qml-qtquick-shapes-shapepath.html

I looked through using the ShapePath to paint the BlockClock. However, using this would require us to introduce a new dependency, qml-module-qtquick-shapes for building the GUI.

So I think this suggestion needs further discussion before implementing it.

A friendly ping, @hebasto

What do you think about this suggestion?

Unfortunately, from my experiments, ShapePath and related items have issues. The most annoying issue happens with small curves where tessellation yields bad curve smoothness. Another issue is that with complex drawings you end up with lots of items and bindings everywhere (especially with Repeaters), which is also not that performant.

Going forward a stack/composition of Canvas is the best option, like photoshop layers. For instance, the Canvas on the bottom could be static or updated less often than the one on the top.

Thanks, @promag, for sharing your thoughts and suggestion for using Canvas.

For instance, the Canvas on the bottom could be static or updated less often than the one on the top.

I think this would be good optimization. However, I might be putting a temporary hold on this optimization suggestion for now, as I am currently getting the Block Clock to display the block tracker, which involves some significant changes to the Canvas as well. So I shall be taking up this suggestion along with the other suggestion that shall come forth with the updated branch.

shaavan · 2022-09-27T16:22:52Z

Updated from 96311e3 to 055e0c0 (pr148.01 -> pr148.02)

Changes:

Introduces BlockClockComponent to change the displayed block clock states as needed.
Introduces another state of the Block clock: The block tracker.

Screenshot:

Note:
Currently, the time tracking is happening according to GMT and doesn't update with the locale. I am going to address this with the next update.
The main focus of this update is to get a general review of the approach I have taken for implementation.

jarolrod

Branch doesn't build

jarolrod · 2022-09-27T16:29:34Z

+
+    const CBlockIndex* tip;
+    node::NodeContext* context = m_node.context();
+    ChainstateManager* chainman = context->chainman.get();


this is too low level of an object to be handling here when we have interfaces::chain

Let me take a look into replacing this low-level object handling with interfaces::chain

jarolrod · 2022-09-27T16:31:43Z

    double verificationProgress() const { return m_verification_progress; }
    void setVerificationProgress(double new_progress);
+    QVariantList blockTimeList() const { return m_block_time_list; }
+    bool setBlockTimeList(int new_block_time);


why should the nodemodel, a proxy class for our internal node, keep something like a blocklist?

Considering your previous suggestion about using the interfaces::chain, I think I can create a separate .cpp file handling chain-related parameters such as blockTimeList

jarolrod · 2022-09-27T16:37:55Z

+import QtQuick.Controls 2.15
+import QtQuick.Layouts 1.15
+
+Item {


This is the implementation of the block clock, and the file residing at src/qml/components/BlockClockComponent.qml is just a controller for this. A better architecture is to have a controller that loads the different states the block clock can be in.

You'd be able to split up this file, as it is very long, between a simple circular progress bar that shows the verification progress while in IBD, then another dial like progress bar for the blocks. Then load these into the controller.

Let me try rephrasing your suggestion to ensure I understood it correctly.
You suggest I create a separate component file for each block clock state, create a controller file in qml/control, and switch to loading different block clock component files based on the requirement.

Have I understood it correctly?

jarolrod · 2022-09-27T16:38:59Z

+            sourceSize.height: 30
+            Layout.bottomMargin: 15
+        }
+        Text {


isn't this recreating our header component?

Yes, it is. Thanks for pointing that out. I shall fix this with the next update of the PR.

jarolrod · 2022-09-27T16:40:02Z

+    return true;
+}
+
+void NodeModel::setBlockTimeListInitial(const CBlockIndex* pblockindex)


Yeah this is definitely too low level for us to be passing around here

Let me experiment with interfaces::chain and devise an approach that avoids low-level object handling.

shaavan · 2022-09-28T13:50:31Z

Updated from 621eae1 to 1f134fd (pr148.02 -> pr148.03, diff)
Addressed @jarolrod comment

Update:

Made changes to fix the problem of the branch not building. Thanks, @jarolrod, for bringing that to notice and helping figure out the root of the problem.

shaavan · 2022-11-28T17:25:18Z

Updated from 23f8a83 to 621eae1 (pr148.03 -> pr148.04)

Changes:

Introduced a new chainmodel class to use the interfaces::chain and directly use the chain info without going too low level in the code.
The new Block Clock displays the timezone-dependent block time data.
Added property to Header.qml so it can be used directly in the BlockClock.qml file.

Here is the demo of the latest iteration of the block clock:

Block_Clock_demo.mov

shaavan · 2022-12-12T14:37:54Z

Updated from 621eae1 to 2a5e998 (pr148.04 -> pr148.05)

Changes:

Rebased over main.
Added the Connecting and Paused state for the block clock.
Integrated the first three commits of Introduce PeersIndicator component #127. This allows the PeerIndicator dots to be animated and correctly show the peer connection state.

Demo:

Screen.Recording.2022-12-12.at.7.21.57.PM.mov

GBKS · 2022-12-13T09:26:22Z

Thanks for the updates. It looks like the artifacts are not building due a lint issue (the Android build is only 22 bytes). Could you please address this? I'd love to get hands-on with this update.

hebasto

To fix the lint task, suggesting the patch as follows:

diff --git a/src/qml/chainmodel.h b/src/qml/chainmodel.h
index 9ab25f35c..2943c9c16 100644
--- a/src/qml/chainmodel.h
+++ b/src/qml/chainmodel.h
@@ -2,8 +2,8 @@
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
-#ifndef BITCOIN_QML_CHAIN_MODEL_H
-#define BITCOIN_QML_CHAIN_MODEL_H
+#ifndef BITCOIN_QML_CHAINMODEL_H
+#define BITCOIN_QML_CHAINMODEL_H
 
 #include <interfaces/chain.h>
 
@@ -47,4 +47,4 @@ private:
     interfaces::Chain& m_chain;
 };
 
-#endif // BITCOIN_QML_CHAIN_MODEL_H
\ No newline at end of file
+#endif // BITCOIN_QML_CHAINMODEL_H

shaavan · 2022-12-15T19:01:06Z

Updated from 2a5e998 to 0f5461e (pr148.05 -> pr148.06)

Addressed @hebasto @jarolrod @GBKS comments.

Changes:

Switch the Bitcoin app icon for the Bitcoin-circle icon.
Fixed QObject::moveToThread: Cannot move objects with a parent warning.
Correction Font size and margins according to design docs.
Removed .vscode/settings.json file.
Applied the patch suggested by @hebasto to fix the lint warning.

Screenshots:

pr148.05	pr148.06

shaavan · 2022-12-15T19:20:24Z

Updated from 0f5461e to 0fbe6c8 (pr148.06 -> pr148.07)

Rebased the branch over the main.

shaavan · 2022-12-15T19:28:13Z

Updated from 0fbe6c8 to 29f019e

Fixed the trailing whitespace lint failure.

shaavan · 2022-12-18T13:22:11Z

Updated from 29f019e to 8173bf6 (pr148.08 -> pr148.09)

Moved the simplified block_clock changes from Simplified version of Block Clock #212 to this PR to preserve the comment history for the block_clock and prevent splitting of discussion in two separate PRs.

Differences as explained in #212 Description:

The PeersIndicator has been replaced with plain rectangles to keep the UI intact.

The conns property representing if the number of connections is greater than 0, has been set to always positive. With this > change, the "connecting" page, though implemented, becomes inaccessible through the UI.

johnny9

Views look nice and render correctly. Tested pause/unpause as well as a full sync on signet at got the following results.

A few QML errors when running that need to be resolved before merging. I've left comments for those errors at the corresponding line numbers.

tail -f debug.log | grep GUI to watch for issues when running bitcoin-qt.

shaavan · 2022-12-18T15:44:35Z

Updated from 8173bf6 to bcbc80b (pr148.09 -> pr148.10)

Addressed @johnny9 comments.

Changes:

Fixed Type Errors as pointed out in this comment.
Added space before and after operator in BlockClock.qml
Added a new line at the end of the file in chainmodel.cpp and chainmodel.h.
Removed the redundant ColumnLayout in Noderunner.qml.

hebasto · 2022-12-19T11:50:30Z

The CI lint task error:

fatal: Invalid revision range fd4e57f63654e2d8e7d0de0aafc335715a53a5cc..c4e1ce262ffb5e6956a7b912f2dcf7560823682e

looks a bit weird as I failed to find a branch the c4e1ce2 commit belongs to.

I hope rebasing will help.

@shaavan And please make sure your local branch does not contain any merge commits.

- This method would allow to get the blocktime value of the block at the given height.

shaavan · 2022-12-19T13:01:50Z

Updated from bcbc80b to 68fa33b (pr148.10 -> pr148.11)

Rebased PR over current main.

johnny9

QML error when going into the paused state still exists. Other issues are resolved.

shaavan · 2022-12-20T12:49:35Z

Updated from 68fa33b to 20b4ed5 (pr148.11 -> pr148.12, diff)

Addressed @johnny9 comment.

Fixed TypeError: Value is undefined and could not be converted to an object error.
Also addressed one formatting issue, present in displaying Block Counter.

Explaination:

For example, for the block count of 122010, difference:

pr148.11	pr148.12

shaavan · 2022-12-20T13:04:14Z

Updated from 20b4ed5 to abb0bef (pr148.12 -> pr148.13, diff)

Modified timeRatioList so that blockClock start displaying from the "0" point.

Screenshot:

pr148.12	pr148.13

johnny9

Good work on this Shashwat. All of the QML errors appear to be resolved now. Views render as expected

hebasto · 2022-12-20T17:47:06Z

+    int64_t getBlockTime(int height) override
+    {
+        LOCK(::cs_main);
+        return Assert(chainman().ActiveChain()[height])->GetBlockTime();


fec6a0e

In Chain interface, probably better to use a type-safe CBlockIndex::Time()

gui-qml/src/chain.h

Lines 279 to 282 in fd4e57f

NodeSeconds Time() const

{

return NodeSeconds{std::chrono::seconds{nTime}};

}

Anyway, it will take a broader discussion when it will about to merge into the main repo.

Sure, let me look through it.

it will take a broader discussion when it will about to merge into the main repo.

Let me include that, along with other future suggestions for this file.

hebasto · 2022-12-20T17:56:19Z

+    timer = new QTimer();
+    connect(timer, &QTimer::timeout, this, &ChainModel::setCurrentTimeRatio);
+    timer->start(1000);
+
+    QThread* timer_thread = new QThread;
+    timer->moveToThread(timer_thread);
+    timer_thread->start();


a287f9e

Sorry, if I missed any previous discussion about that, but why is calling of ChainModel::setCurrentTimeRatio implemented like that?

The currentTimeRatio represents the ratio of 12 hours that has been passed.
So, for example, at 4 am or pm, the currenTimeRatio would be 0.3.

We need to update this information in the blockClock each second so that the blockClock always shows the correct blockTime.

So that's why I created a separate 1-sec timer thread to call this function each second and update the currentTimeRatio.

- This file allowing using of Chain interfaces to get information about the blockchain. - This class declares and defines the function that will be used in for the Block Clock.

1. Added a new Q_PROPERTY to track the remaining time till the IBD completioni. 2. Added two signals to later connect to the chainmodel class function. 3. Added new member variable, m_pause, which will keep track of if the block clock is in paused state or not, and will accordingly disconnect or reconnect the user to network.

- Also made appropriate connections with the nodemodel signals

- This property will later be used for the BlockClock.qml file

- The BlockClock.qml files provides the control for BlockClock and determines how the elements would be displayed. - The BlockClockComponent.qml file uses the aforementioned control to display the value, and determines what will be displayed.

shaavan · 2022-12-21T13:46:02Z

Updated from abb0bef to 67d12a6 (pr148.13 -> pr148.14, diff)

Addressed @hebasto comments.

Changes:

Changed 1-sec QTimer from a ~~global~~ private member variable to a local variable.
Changed names of variables mentioned in this comment from camel-style naming to snake-style naming. For full diff, check diff.
Added descriptive comment for m_time_ratio_list, and the values it stores.

hebasto · 2023-01-25T10:59:53Z

Closing in favor of #220.

6701ad2 qml: introduce the BlockClock (johnny9) 60bd8cb qml: introduce ChainModel (johnny9) eb945ac qml: add sync time related properties to NodeModel (johnny9) 8db7f0f qml: add additional properties in Header (johnny9) 094a2c4 qml: add ability to pause NodeModel (johnny9) 1fce853 Add getBlockTime interface method in src/interfaces/chain.h (shaavan) Pull request description: This is a continuation of #148. The goal of this PR is to get the working features from #148 merged in using a QQuickPainterItem implementation of the dial instead of the javascript canvas. Connection state as well as the animating icon will be implemented in a later PR. Refactoring and clean up from the removal of the Canvas code still remains. [![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/220) [![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/220) [![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/220) [![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/220) ACKs for top commit: jarolrod: ACK 6701ad2 Tree-SHA512: 43b99d1fd69b67b285e91e05d3864c7044917cd6feae97b4c226f3a7b499ca24f6e786575dbc700461b1f6c58c0273746e4ea5834c554fa4dc916868199698df

4f9bfbc qml: introduce the BlockClock (johnny9) e57e7ff qml: introduce ChainModel (johnny9) e38c565 qml: add sync time related properties to NodeModel (johnny9) 9c45b09 qml: add additional properties in Header (johnny9) fcee831 qml: add ability to pause NodeModel (johnny9) 1fce853 Add getBlockTime interface method in src/interfaces/chain.h (shaavan) Pull request description: This is a continuation of bitcoin-core#148. The goal of this PR is to get the working features from bitcoin-core#148 merged in using a QQuickPainterItem implementation of the dial instead of the javascript canvas. Connection state as well as the animating icon will be implemented in a later PR. Refactoring and clean up from the removal of the Canvas code still remains. [![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/220) [![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/220) [![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/220) [![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/220) ACKs for top commit: jarolrod: ACK 4f9bfbc Tree-SHA512: 43b99d1fd69b67b285e91e05d3864c7044917cd6feae97b4c226f3a7b499ca24f6e786575dbc700461b1f6c58c0273746e4ea5834c554fa4dc916868199698df

jarolrod reviewed Aug 9, 2022

View reviewed changes

jarolrod reviewed Aug 10, 2022

View reviewed changes

hebasto added the UX Designers' opinions are required label Aug 12, 2022

hebasto added the Needs rebase label Aug 29, 2022

shaavan force-pushed the 220806-the_block_clock branch from 96311e3 to 621eae1 Compare September 27, 2022 16:22

jarolrod reviewed Sep 27, 2022

View reviewed changes

hebasto removed the Needs rebase label Sep 28, 2022

shaavan force-pushed the 220806-the_block_clock branch from 621eae1 to 1f134fd Compare September 28, 2022 13:50

shaavan force-pushed the 220806-the_block_clock branch from 1f134fd to 23f8a83 Compare November 28, 2022 17:24

shaavan force-pushed the 220806-the_block_clock branch from 23f8a83 to 2a5e998 Compare December 12, 2022 14:37

hebasto reviewed Dec 13, 2022

View reviewed changes

Comment thread .vscode/settings.json Outdated

jarolrod reviewed Dec 14, 2022

View reviewed changes

Comment thread src/qml/controls/BlockClock.qml Outdated

jarolrod reviewed Dec 14, 2022

View reviewed changes

Comment thread src/qml/chainmodel.cpp

jarolrod mentioned this pull request Dec 14, 2022

[DO NOT MERGE] 148 rebased over recent changes #208

Closed

shaavan force-pushed the 220806-the_block_clock branch from 2a5e998 to 0f5461e Compare December 15, 2022 19:00

shaavan force-pushed the 220806-the_block_clock branch from 0f5461e to 0fbe6c8 Compare December 15, 2022 19:20

shaavan force-pushed the 220806-the_block_clock branch from 0fbe6c8 to 29f019e Compare December 15, 2022 19:26

shaavan mentioned this pull request Dec 16, 2022

Simplified version of Block Clock #212

Closed

shaavan force-pushed the 220806-the_block_clock branch from 29f019e to 8173bf6 Compare December 18, 2022 13:22

johnny9 suggested changes Dec 18, 2022

View reviewed changes

Comment thread src/qml/components/BlockClockComponent.qml

Comment thread src/qml/controls/BlockClock.qml Outdated

Comment thread src/qml/pages/node/NodeRunner.qml Outdated

Comment thread src/qml/chainmodel.h Outdated

Comment thread src/qml/chainmodel.cpp Outdated

shaavan force-pushed the 220806-the_block_clock branch from 8173bf6 to bcbc80b Compare December 18, 2022 15:44

hebasto added the Needs rebase label Dec 19, 2022

Add getBlockTime interface method in src/interfaces/chain.h

fec6a0e

- This method would allow to get the blocktime value of the block at the given height.

shaavan force-pushed the 220806-the_block_clock branch from bcbc80b to 68fa33b Compare December 19, 2022 13:01

hebasto removed the Needs rebase label Dec 19, 2022

johnny9 suggested changes Dec 19, 2022

View reviewed changes

Comment thread src/qml/controls/BlockClock.qml Outdated

shaavan force-pushed the 220806-the_block_clock branch 2 times, most recently from 20b4ed5 to abb0bef Compare December 20, 2022 13:04

johnny9 reviewed Dec 20, 2022

View reviewed changes

hebasto reviewed Dec 20, 2022

View reviewed changes

shaavan added 6 commits December 21, 2022 19:06

qml: Introduced a new class chainmodel

14a4e8c

- This file allowing using of Chain interfaces to get information about the blockchain. - This class declares and defines the function that will be used in for the Block Clock.

qml: Initialised the chainmodel in src/bitcoin.cpp

d0d4b03

- Also made appropriate connections with the nodemodel signals

qml: Added descriptionColor and descriptionBold properties in Header.qml

7eb7939

- This property will later be used for the BlockClock.qml file

qml: Added BlockClockComponent to NodeRunner.qml

67d12a6

shaavan force-pushed the 220806-the_block_clock branch from abb0bef to 67d12a6 Compare December 21, 2022 13:45

mouxdesign mentioned this pull request Jan 3, 2023

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

Open

hebasto added the Needs rebase label Jan 17, 2023

johnny9 mentioned this pull request Jan 19, 2023

The Block Clock #220

Merged

hebasto closed this Jan 25, 2023

	NodeSeconds Time() const
	{
	return NodeSeconds{std::chrono::seconds{nTime}};
	}

Conversation

shaavan commented Aug 9, 2022

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!

promag Sep 8, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

shaavan commented Sep 27, 2022

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!

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!

shaavan commented Sep 28, 2022

Uh oh!

shaavan commented Nov 28, 2022

Uh oh!

shaavan commented Dec 12, 2022

Uh oh!

GBKS commented Dec 13, 2022

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shaavan commented Dec 15, 2022

Uh oh!

shaavan commented Dec 15, 2022

Uh oh!

shaavan commented Dec 15, 2022

Uh oh!

shaavan commented Dec 18, 2022

Uh oh!

johnny9 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shaavan commented Dec 18, 2022

Uh oh!

hebasto commented Dec 19, 2022

Uh oh!

shaavan commented Dec 19, 2022

Uh oh!

johnny9 left a comment

promag Sep 8, 2022 •

edited

Loading

johnny9 left a comment •

edited

Loading

hebasto Dec 20, 2022 •

edited

Loading

shaavan commented Dec 21, 2022 •

edited

Loading