bitcoin-core / gui-qml

Bitcoin GUI (experimental QML-based fork)
MIT License
106 stars 40 forks source link

Fix For BlockClock Min Size #385

Closed D33r-Gee closed 6 months ago

D33r-Gee commented 6 months ago

Issue and Fix:

This fixes https://github.com/bitcoin-core/gui-qml/issues/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 commented 6 months ago

Tested the qml logic on desktop by removing the minimum window restraints Before: Screenshot from 2023-12-20 23-49-00 After Screenshot from 2023-12-20 23-48-43

Looks like it fixes the issue.

GBKS commented 6 months ago

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 commented 6 months ago

Good catch with the missing minimum height factor

Current PR Updated Logic Main
current pr extreme updated pr extreme main extreme

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 commented 6 months ago

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:

image

Would that be possible?

D33r-Gee commented 6 months ago

Remove the commented out line of code

Done in new commit

D33r-Gee commented 6 months ago

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

I have update the PR with a new commit https://github.com/bitcoin-core/gui-qml/pull/385/commits/3f455ee10807a0663b833afd94f864a7b7c23c75 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 commented 6 months ago

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

Ubuntu 22.04 showcase mode:

Ubuntu2204_showcase

Ubuntu 22.04 compact mode:

Ubuntu2204_compact

D33r-Gee commented 6 months ago

Galaxy A13 5G physical device screenshots:

Portrait:

physical_GalaxyA13_portrait

Landscape:

physical_GalaxyA13_landscape

D33r-Gee commented 6 months ago

Virtual device Galaxy A32 5G:

Landscape:

virtual_GalaxyA32_landscape

Portrait:

virtual_GalaxyA32_portrait

D33r-Gee commented 6 months ago

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

D33r-Gee commented 6 months ago

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

D33r-Gee commented 6 months ago

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

Thanks @hebasto for merging it!