Skip to content

The Block Clock #220

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

Merged
merged 6 commits into from
Jan 29, 2023
Merged

The Block Clock #220

merged 6 commits into from
Jan 29, 2023

Conversation

johnny9
Copy link
Collaborator

@johnny9 johnny9 commented Jan 19, 2023

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
Intel macOS
Apple Silicon macOS
ARM64 Android

- This method would allow to get the blocktime value of the block at
  the given height.
@johnny9 johnny9 marked this pull request as draft January 19, 2023 23:20
@johnny9 johnny9 force-pushed the the-block-clock branch 2 times, most recently from 90469cb to e9b5ddd Compare January 19, 2023 23:26
@johnny9
Copy link
Collaborator Author

johnny9 commented Jan 19, 2023

Update from 90469cb to e9b5dd:

  • Replace tabs with spaces

@johnny9
Copy link
Collaborator Author

johnny9 commented Jan 19, 2023

Update branch from e9b5ddd to 96e47f8:

  • Fix painting of last block. index 0 of timeRatioList is actually the current time.

@johnny9
Copy link
Collaborator Author

johnny9 commented Jan 20, 2023

Update from 96e47f8 to 63792bf:

  • Fix Makefile includes

@johnny9 johnny9 force-pushed the the-block-clock branch 2 times, most recently from 37b6257 to 8515fdf Compare January 20, 2023 02:17
@johnny9
Copy link
Collaborator Author

johnny9 commented Jan 20, 2023

Update branch from 37b6257 to 8515fdf:

  • Added comment for calculating the "gap" between painted blocks.

@johnny9 johnny9 marked this pull request as ready for review January 20, 2023 12:44
@johnny9 johnny9 force-pushed the the-block-clock branch 3 times, most recently from fa214b0 to 26d57ff Compare January 20, 2023 13:01
@johnny9
Copy link
Collaborator Author

johnny9 commented Jan 20, 2023

I noticed that these changes require the QtGraphicalEffects module (qml-module-graphicaleffects ubuntu package). I'll likely have to update the depends to include that module or change how the icon is forced into a specific color. Looking for an opinion one way or the other.

Copy link
Member

@jarolrod jarolrod 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

Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

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

Let's squash some of the history to have a clean commit record, but also I'm seeing some slight misalignment with the design file.

Below, the more visible layer is the design file, and below that is this PR on signet (hence diff block heights). Looks like the block height, the words "Blocktime" and peers indicator should be a bit higher up. Also fine with leaving this fixup for a follow-up!

Screen Shot 2023-01-25 at 12 05 42 AM

@johnny9
Copy link
Collaborator Author

johnny9 commented Jan 27, 2023

Update branch from 8ecfae6 to 76a504e:

  • Squashed all BlockClock control commits into one

@johnny9
Copy link
Collaborator Author

johnny9 commented Jan 28, 2023

Update branch from 76a504e to 2e5a0d8:

  • Adjusted alignment of labels to better match the figma spec

Screenshot from 2023-01-28 10-37-08

@johnny9
Copy link
Collaborator Author

johnny9 commented Jan 28, 2023

Update. Remove a missed trailing whitespace

@johnny9
Copy link
Collaborator Author

johnny9 commented Jan 28, 2023

Update from 3fd696d to 33eb4af:

  • Fixed potential initialization error in ChainModel, possible root cause of an issue Jarol discovered with the dial not lining up with 12 o'clock.

Additional properties to Header will allow configuring Header's
description color and bold values as well as Header's header-text
bold value.

Co-authored-by: shaavan <[email protected]>
johnny9 and others added 3 commits January 28, 2023 21:06
The properties will allow the gui to show an estimate for the
time remaining before the node is synced.

Co-authored-by: shaavan <[email protected]>
The ChainModel is responsible for managing the block information
that the gui components need to show. In this case, the ChainModel
will provide a list of ratios for the block times of the last 12 hours
to be rendered on the block clock's dial.

Co-authored-by: shaavan <[email protected]>
Implements a few of the BlockClock dial states. Sync progress while
downloading, rendering blocks after sync, and pausing/unpausing the
node. An additional Model is added, ChainModel, to provide the dial
block information. Changes to NodeModel are also made to support the
pausing/unpausing Dial feature.

Co-authored-by: shaavan <[email protected]>
@johnny9
Copy link
Collaborator Author

johnny9 commented Jan 29, 2023

Update branch to 6701ad2:

  • Fixed code style issue in blockclock.cpp/.h
  • Moved BlockClock.qml to components
  • Split BlockClock commit up to clarify history

Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

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

ACK 6701ad2

This works, with improvements to be made in follow-ups

const QRectF bounds = getBoundsForPen(pen);
painter->setPen(pen);

QColor confirmationColors[] = {
Copy link
Member

Choose a reason for hiding this comment

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

in a follow-up all of these colors should be defined in our theme.qml, because the colors would actually be slightly different when on dark or light mode. We'll circle back with @GBKS about these values.


states: [
State {
name: "intialBlockDownload"; when: !synced && !paused && conns
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer all caps short names for states. in the codebase like, can be changed afterwards

  • "IBD"
  • "BLOCKCLOCK"
  • "PAUSED"
  • "CONNECTING"

// Paint blocks
for (int i = 1; i < numberOfBlocks; i++) {
if (numberOfBlocks - i <= 6) {
QPen pen(confirmationColors[numberOfBlocks - i - 1]);
Copy link
Member

Choose a reason for hiding this comment

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

can this be initialized outside of the loop, then have its properties updated?


// The gap is calculated here and is used to create a
// one pixel spacing between each block
double gap = degreesPerPixel();
Copy link
Member

Choose a reason for hiding this comment

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

does this need to be calculated each time the paintBlocks function is called, or can it be cached when the object is constructed?

update();
}

QRectF BlockClockDial::getBoundsForPen(const QPen & pen)
Copy link
Member

Choose a reason for hiding this comment

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

I think this could be simplified into:

QRectF BlockClockDial::getBoundsForPen(const QPen & pen)
{
    const QRectF bounds = boundingRect();
    QRectF rect = bounds.marginsRemoved(QMarginsF(pen.widthF() / 2.0, pen.widthF() / 2.0, pen.widthF() / 2.0, pen.widthF() / 2.0));
    return rect.toAlignedRect();
}
This PR My Suggestion
Screen Shot 2023-01-29 at 12 23 42 AM Screen Shot 2023-01-29 at 12 24 12 AM

}

MouseArea {
anchors.fill: dial
Copy link
Member

Choose a reason for hiding this comment

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

could add, since the paused state is implemented

Suggested change
anchors.fill: dial
anchors.fill: dial
cursorShape: Qt.PointingHandCursor

@@ -27,14 +28,60 @@ void NodeModel::setBlockTipHeight(int new_height)
}
}

void NodeModel::setRemainingSyncTime(double new_progress)
Copy link
Member

Choose a reason for hiding this comment

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

This function is pretty confusing in general, we'll heavily change this in follow-ups

Copy link
Member

Choose a reason for hiding this comment

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

should probably do an early test on new_progress to make sure it's a valid value

int timeDelta = 0;
int remainingMSecs = 0;
double remainingProgress = 1.0 - new_progress;
for (int i = 1; i < m_block_process_time.size(); i++) {
Copy link
Member

Choose a reason for hiding this comment

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

starting from 1, isn't it skipping the first value in the list?

for (int i = 1; i < m_block_process_time.size(); i++) {
QPair<int, double> sample = m_block_process_time[i];

// take first sample after 500 seconds or last available one
Copy link
Member

Choose a reason for hiding this comment

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

but currentTime was set in milliseconds in line 33, now we're doing math with seconds values?

}
static const int MAX_SAMPLES = 5000;
if (m_block_process_time.count() > MAX_SAMPLES) {
m_block_process_time.remove(1, m_block_process_time.count() - 1);
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 why we're removing the first element here; but in follow-ups its better if this is a queue, instead of a vector that we manually remove elements from

@hebasto hebasto merged commit a56361d into bitcoin-core:main Jan 29, 2023
hebasto added a commit that referenced this pull request Feb 9, 2023
759d2f0 qml: move BlockClock confirmation colors into Theme.qml (jarolrod)
0b6d32e qml: capitalize the BlockClock component's state names (jarolrod)
fd40538 qml: set BlockClock mouseArea cursorShape to pointing cursor (jarolrod)

Pull request description:

  This implements the following review comments from #220 (review)
  - Make cursor into pointing cursor when over the BlockClock to make the BlockClock look like it's clickable #220 (comment)
  - Make BlockClock state names capital #220 (comment)
  - Move out BlockClock confirmation colors into our Theme.qml #220 (comment)

  [![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/250)
  [![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/250)
  [![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/250)
  [![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/250)

ACKs for top commit:
  johnny9:
    ACK 759d2f0

Tree-SHA512: 7a7919a47f65872656a63fd7cac7cc94d95cfe60bf714d09295995296fd81b11c10e58ff4e7280c67bc74f6df7f3f1aa5c09975fa04e9c71d947fffc3df4b50b
hebasto pushed a commit to hebasto/gui-qml that referenced this pull request Jun 8, 2025
- This method would allow to get the blocktime value of the block at
  the given height.

Github-Pull: bitcoin-core#220
Rebased-From: 1fce853
hebasto pushed a commit to hebasto/gui-qml that referenced this pull request Jun 8, 2025
Github-Pull: bitcoin-core#220
Rebased-From: 094a2c4

Co-authored-by: shaavan <[email protected]>
hebasto pushed a commit to hebasto/gui-qml that referenced this pull request Jun 8, 2025
Additional properties to Header will allow configuring Header's
description color and bold values as well as Header's header-text
bold value.

Github-Pull: bitcoin-core#220
Rebased-From: 8db7f0f

Co-authored-by: shaavan <[email protected]>
hebasto pushed a commit to hebasto/gui-qml that referenced this pull request Jun 8, 2025
The properties will allow the gui to show an estimate for the
time remaining before the node is synced.

Github-Pull: bitcoin-core#220
Rebased-From: eb945ac

Co-authored-by: shaavan <[email protected]>
hebasto pushed a commit to hebasto/gui-qml that referenced this pull request Jun 8, 2025
The ChainModel is responsible for managing the block information
that the gui components need to show. In this case, the ChainModel
will provide a list of ratios for the block times of the last 12 hours
to be rendered on the block clock's dial.

Github-Pull: bitcoin-core#220
Rebased-From: 60bd8cb

Co-authored-by: shaavan <[email protected]>
hebasto pushed a commit to hebasto/gui-qml that referenced this pull request Jun 8, 2025
Implements a few of the BlockClock dial states. Sync progress while
downloading, rendering blocks after sync, and pausing/unpausing the
node. An additional Model is added, ChainModel, to provide the dial
block information. Changes to NodeModel are also made to support the
pausing/unpausing Dial feature.

Github-Pull: bitcoin-core#220
Rebased-From: 6701ad2

Co-authored-by: shaavan <[email protected]>
hebasto pushed a commit to hebasto/gui-qml that referenced this pull request Jun 9, 2025
- This method would allow to get the blocktime value of the block at
  the given height.

Github-Pull: bitcoin-core#220
Rebased-From: 1fce853
hebasto pushed a commit to hebasto/gui-qml that referenced this pull request Jun 9, 2025
Github-Pull: bitcoin-core#220
Rebased-From: 094a2c4

Co-authored-by: shaavan <[email protected]>
hebasto pushed a commit to hebasto/gui-qml that referenced this pull request Jun 9, 2025
Additional properties to Header will allow configuring Header's
description color and bold values as well as Header's header-text
bold value.

Github-Pull: bitcoin-core#220
Rebased-From: 8db7f0f

Co-authored-by: shaavan <[email protected]>
hebasto pushed a commit to hebasto/gui-qml that referenced this pull request Jun 9, 2025
The properties will allow the gui to show an estimate for the
time remaining before the node is synced.

Github-Pull: bitcoin-core#220
Rebased-From: eb945ac

Co-authored-by: shaavan <[email protected]>
hebasto pushed a commit to hebasto/gui-qml that referenced this pull request Jun 9, 2025
The ChainModel is responsible for managing the block information
that the gui components need to show. In this case, the ChainModel
will provide a list of ratios for the block times of the last 12 hours
to be rendered on the block clock's dial.

Github-Pull: bitcoin-core#220
Rebased-From: 60bd8cb

Co-authored-by: shaavan <[email protected]>
hebasto pushed a commit to hebasto/gui-qml that referenced this pull request Jun 9, 2025
Implements a few of the BlockClock dial states. Sync progress while
downloading, rendering blocks after sync, and pausing/unpausing the
node. An additional Model is added, ChainModel, to provide the dial
block information. Changes to NodeModel are also made to support the
pausing/unpausing Dial feature.

Github-Pull: bitcoin-core#220
Rebased-From: 6701ad2

Co-authored-by: shaavan <[email protected]>
hebasto pushed a commit to hebasto/gui-qml that referenced this pull request Jun 9, 2025
- This method would allow to get the blocktime value of the block at
  the given height.

Github-Pull: bitcoin-core#220
Rebased-From: 1fce853
hebasto pushed a commit to hebasto/gui-qml that referenced this pull request Jun 9, 2025
Github-Pull: bitcoin-core#220
Rebased-From: 094a2c4

Co-authored-by: shaavan <[email protected]>
hebasto pushed a commit to hebasto/gui-qml that referenced this pull request Jun 9, 2025
Additional properties to Header will allow configuring Header's
description color and bold values as well as Header's header-text
bold value.

Github-Pull: bitcoin-core#220
Rebased-From: 8db7f0f

Co-authored-by: shaavan <[email protected]>
hebasto pushed a commit to hebasto/gui-qml that referenced this pull request Jun 9, 2025
The properties will allow the gui to show an estimate for the
time remaining before the node is synced.

Github-Pull: bitcoin-core#220
Rebased-From: eb945ac

Co-authored-by: shaavan <[email protected]>
hebasto pushed a commit to hebasto/gui-qml that referenced this pull request Jun 9, 2025
The ChainModel is responsible for managing the block information
that the gui components need to show. In this case, the ChainModel
will provide a list of ratios for the block times of the last 12 hours
to be rendered on the block clock's dial.

Github-Pull: bitcoin-core#220
Rebased-From: 60bd8cb

Co-authored-by: shaavan <[email protected]>
hebasto pushed a commit to hebasto/gui-qml that referenced this pull request Jun 9, 2025
Implements a few of the BlockClock dial states. Sync progress while
downloading, rendering blocks after sync, and pausing/unpausing the
node. An additional Model is added, ChainModel, to provide the dial
block information. Changes to NodeModel are also made to support the
pausing/unpausing Dial feature.

Github-Pull: bitcoin-core#220
Rebased-From: 6701ad2

Co-authored-by: shaavan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants