Fix For BlockClock Min Size #385

D33r-Gee · 2023-12-20T18:59:19Z

Issue and Fix:

This fixes #382

GBKS reported an issue where the blockclock on their Galaxy A32 5G phone displayed incorrectly under the min size of 200 pixels.
They provided a fix implemented in this pull request:

Math.max(Math.min(200, root.parentWidth - 30), Math.min((root.parentWidth * dial.scale), (root.parentHeight * dial.scale)))

Verification:

I tested the fix on both a virtual device Galaxy A32 5G in Android Studio and a physical Galaxy A13 5G (armv7) device. The fix successfully resolves the pixel-related display issue on both devices.

johnny9

Remove the commented out line of code

johnny9 · 2023-12-21T04:51:13Z

Tested the qml logic on desktop by removing the minimum window restraints
Before:

After

Looks like it fixes the issue.

GBKS · 2023-12-21T07:38:32Z

Awesome. I realized there's one thing missing from the code I proposed. I only ensures that the block fits the horizontal screen width, but not the vertical height. So here's the logic.

// The maximum size.
// This is our hardest constraint - the physical screen size.
screenConstraintSize = Math.min(root.parentWidth, root.parentHeight) - 30

// The minimum size (for legibility).
minimumSize = 200

// The dynamic target size based on our display mode scale.
// Basically just scaling up for larger non-mobile screens.
scaleTargetSize = Math.min(root.parentWidth, root.parentHeight) * dial.scale

// Mashing them all up
result = Math.min(screenConstraintSize, Math.max(minimumSize, scaleTargetSize))

// Or in one line
result = Math.min(Math.min(root.parentWidth, root.parentHeight) - 30, Math.max(200, Math.min(root.parentWidth, root.parentHeight) * dial.scale))

@jarolrod you probably spent the most time on this, how does that logic look to you?

johnny9 · 2023-12-21T13:24:13Z

Good catch with the missing minimum height factor

Current PR	Updated Logic	Main

Updated code can be

        width: {
            minimumSize = Math.min(200, Math.min(root.parentWidth - 30, root.parentHeight - 30)
            scaledSize = Math.min((root.parentWidth * dial.scale), (root.parentHeight * dial.scale))
            return Math.max(minimumSize, scaledSize)
        }

or

        width: {
            Math.max(Math.min(200, Math.min(root.parentWidth - 30, root.parentHeight - 30)),
                     Math.min((root.parentWidth * dial.scale), (root.parentHeight * dial.scale)))
        }

GBKS · 2023-12-21T14:15:34Z

Phew, what are those screenshots from? Android split-screen? That gets really tiny with a huge signet indicator. We might even need something custom for that, like this design:

Would that be possible?

D33r-Gee · 2023-12-21T17:59:13Z

Remove the commented out line of code

Done in new commit

D33r-Gee · 2023-12-21T18:03:00Z

Thanks @GBKS and @johnny9 for looking into the math.

I have update the PR with a new commit 3f455ee which replaces the line suggested by @GBKS with @johnny9's:

Math.max(Math.min(200, Math.min(root.parentWidth - 30, root.parentHeight - 30)),
Math.min((root.parentWidth * dial.scale), (root.parentHeight * dial.scale)))

D33r-Gee · 2023-12-21T18:06:23Z

Also here are some screenshots of the new commit running on different platforms/devices:

Ubuntu 22.04 showcase mode:

Ubuntu 22.04 compact mode:

D33r-Gee · 2023-12-21T18:08:43Z

Galaxy A13 5G physical device screenshots:

Portrait:

Landscape:

D33r-Gee · 2023-12-21T18:10:08Z

Virtual device Galaxy A32 5G:

Landscape:

Portrait:

D33r-Gee · 2023-12-21T18:11:17Z

Changing the mode from compact to showcase didn't have a noticeable effect on the Android builds.

johnny9

One last thing. You should include the context in your git commit message header. In this case, "qml". So the commit message can be "qml: fix the BlockClock's minimum size"

johnny9 · 2023-12-22T04:42:47Z

src/qml/components/BlockClock.qml

@@ -42,7 +42,7 @@ Item {
        id: dial
        anchors.horizontalCenter: root.horizontalCenter
        scale: Theme.blockclocksize
-        width: Math.min((root.parentWidth * dial.scale), (root.parentHeight * dial.scale))
+        width: Math.max(Math.min(200, Math.min(root.parentWidth - 30, root.parentHeight - 30)), Math.min((root.parentWidth * dial.scale), (root.parentHeight * dial.scale)))


Can we break this up into two lines? It's difficult to read.

Yes on both counts... new commit with changes d6331aa

D33r-Gee · 2023-12-22T17:41:15Z

@GBKS regarding your question about the signet indicator:

Would that be possible?

Yes, would you like to explore this in this PR or should we create a new one?

johnny9

ACK d6331aa

pablomartin4btc

tACK d6331aa

Tested on an Android Wear smart watch and experienced a significant improvement.

	Light Theme	Dark Theme
Before
After

As it can be noticed, the network indicator has almost disappeared in such devices, therefor as @GBKS pointed out perhaps the network indicator could be moved up to the top-left, but that's where the "back" button is when we navigate through the settings, maybe it can be accommodated a bit up at the bottom reducing its size?

Another observation, out of scope of this PR, the settings button it's almost at the rounded border and it has been cut a bit, but I'm not sure if we support any/ all devices.

pablomartin4btc · 2023-12-30T13:57:23Z

src/qml/components/BlockClock.qml

+        width: {Math.max(Math.min(200, Math.min(root.parentWidth - 30, root.parentHeight - 30)), 
+                Math.min((root.parentWidth * dial.scale), (root.parentHeight * dial.scale)))}


nit: I would extract the formula to a function where I'd structure the logic as @GBKS suggested above.

How about this...
**
--- a/src/qml/components/BlockClock.qml +++ b/src/qml/components/BlockClock.qml @@ -44,7 +44,7 @@ Item { id: dial anchors.horizontalCenter: root.horizontalCenter scale: Theme.blockclocksize - width: Math.min((root.parentWidth * dial.scale), (root.parentHeight * dial.scale)) + width: calculateConstrainedWidth() height: dial.width penWidth: dial.width / 50 timeRatioList: chainModel.timeRatioList @@ -239,6 +239,24 @@ Item { } ] + function calculateConstrainedWidth() { + // Margin size used to create a buffer around the constrained area. + var marginSize = 30; + + // The maximum size. + // This is our hardest constraint - the physical screen size. + var screenConstraintSize = Math.min(root.parentWidth, root.parentHeight) - marginSize; + + // The minimum size (for legibility). + var minimumSize = 200; + + // The dynamic target size based on our display mode scale. + // Basically just scaling up for larger non-mobile screens. + var scaleTargetSize = Math.min(root.parentWidth, root.parentHeight) * dial.scale; + + // Mashing them all up + return Math.min(screenConstraintSize, Math.max(minimumSize, scaleTargetSize)) + } function formatProgressPercentage(progress) { if (progress >= 1) {

thanks for testing on a wearable device, maybe let’s open a new PR if this feels like a priority, what do you think @GBKS?

Including the explicit logic makes it easier to read, but since I have no clue about the established code standards in this project, my opinion here should not weigh much.

We should address the separate layout at tiny scales in a different PR. I'll try to mock up all the variations, and also include them in the design docs page, probably early next week.

I mocked things up and added them to the design docs, see here. Feedback appreciated.

Also had a question about how this works on Android Wear. Is the full app running on the watch? Or does it run in some sort of stripped down way? I'd be surprised of watches are powerful enough. I also assume we can detect if the app runs on a watch and potentially adjust the layout accordingly (specifically for the rounded frame)?

Is the full app running on the watch? Or does it run in some sort of stripped down way?

I haven't passed the "Connecting..." phase, even the watch is connected to the internet and everything, I'd need to debug and check deeper, but I doubt it, I was just going a bit further with the testing of this PR and since I was going thru the Android guide it seems I got a bit overexcited 😆.

I'd be surprised of watches are powerful enough.

Well, this is an old one, 2017-ish, nowadays resources on smartwatches are still limited but they are getting there.

I also assume we can detect if the app runs on a watch and potentially adjust the layout accordingly (specifically for the rounded frame)?

I'd think so, I'll check when I do something related with it.

D33r-Gee · 2024-01-02T17:38:35Z

Thanks @GBKS , @johnny9 , and @pablomartin4btc for testing and working on this PR

Thanks @hebasto for merging it!

D33r-Gee mentioned this pull request Dec 20, 2023

Set minimum block clock size to 200px width #382

Closed

1 task

johnny9 suggested changes Dec 21, 2023

View reviewed changes

D33r-Gee force-pushed the min-blockclock-size branch from 69b1ec2 to 3f455ee Compare December 21, 2023 17:57

johnny9 suggested changes Dec 22, 2023

View reviewed changes

qml: fix the BlockClock's minimum size

d6331aa

D33r-Gee force-pushed the min-blockclock-size branch from 3f455ee to d6331aa Compare December 22, 2023 17:36

johnny9 approved these changes Dec 23, 2023

View reviewed changes

pablomartin4btc reviewed Dec 30, 2023

View reviewed changes

hebasto merged commit 533a9fc into bitcoin-core:main Jan 1, 2024

GBKS mentioned this pull request Jan 8, 2024

Add layout logic diagrams for the block clock BitcoinDesign/Bitcoin-Core-App#89

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix For BlockClock Min Size #385

Fix For BlockClock Min Size #385

D33r-Gee commented Dec 20, 2023 •

edited by hebasto

johnny9 left a comment

johnny9 commented Dec 21, 2023

GBKS commented Dec 21, 2023

johnny9 commented Dec 21, 2023

GBKS commented Dec 21, 2023

D33r-Gee commented Dec 21, 2023

D33r-Gee commented Dec 21, 2023

D33r-Gee commented Dec 21, 2023

D33r-Gee commented Dec 21, 2023

D33r-Gee commented Dec 21, 2023

D33r-Gee commented Dec 21, 2023

johnny9 left a comment •

edited

johnny9 Dec 22, 2023

D33r-Gee Dec 22, 2023

D33r-Gee commented Dec 22, 2023

johnny9 left a comment

pablomartin4btc left a comment

pablomartin4btc Dec 30, 2023

D33r-Gee Jan 2, 2024

GBKS Jan 3, 2024

GBKS Jan 8, 2024

GBKS Jan 8, 2024

pablomartin4btc Jan 9, 2024

D33r-Gee commented Jan 2, 2024

		width: {Math.max(Math.min(200, Math.min(root.parentWidth - 30, root.parentHeight - 30)),
		Math.min((root.parentWidth * dial.scale), (root.parentHeight * dial.scale)))}

Fix For BlockClock Min Size #385

Fix For BlockClock Min Size #385

Conversation

D33r-Gee commented Dec 20, 2023 • edited by hebasto

johnny9 left a comment

Choose a reason for hiding this comment

johnny9 commented Dec 21, 2023

GBKS commented Dec 21, 2023

johnny9 commented Dec 21, 2023

GBKS commented Dec 21, 2023

D33r-Gee commented Dec 21, 2023

D33r-Gee commented Dec 21, 2023

D33r-Gee commented Dec 21, 2023

D33r-Gee commented Dec 21, 2023

D33r-Gee commented Dec 21, 2023

D33r-Gee commented Dec 21, 2023

johnny9 left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

D33r-Gee commented Dec 22, 2023

johnny9 left a comment

Choose a reason for hiding this comment

pablomartin4btc 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

D33r-Gee commented Jan 2, 2024

D33r-Gee commented Dec 20, 2023 •

edited by hebasto

johnny9 left a comment •

edited