Skip to content

Remove redundant MouseAreas from controls that are Buttons#271

Merged
hebasto merged 1 commit into
bitcoin-core:mainfrom
jarolrod:drop-redundant-mouse-area
Feb 22, 2023
Merged

Remove redundant MouseAreas from controls that are Buttons#271
hebasto merged 1 commit into
bitcoin-core:mainfrom
jarolrod:drop-redundant-mouse-area

Conversation

@jarolrod
Copy link
Copy Markdown
Contributor

@jarolrod jarolrod commented Feb 21, 2023

For controls that are buttons, an additional MouseArea layer is unnecessary as they already have built-in event detection for actions such as pressing and hovering. By eliminating the redundant MouseAreas in the ContinueButton, NavButton, OutlineButton, Setting, and TextButton controls, we make their implementations cleaner. Previously, the MouseArea was utilized to set the controls' state, but now this is achieved through when statements that are bundled with the states themselves.

Windows
Intel macOS
Apple Silicon macOS
ARM64 Android

@jarolrod jarolrod force-pushed the drop-redundant-mouse-area branch from 5fa9522 to b30fefd Compare February 21, 2023 17:55
@jarolrod
Copy link
Copy Markdown
Contributor Author

Updated from 5fa9522 to b30fefd, compare

Changes: updated to fix linter

Copy link
Copy Markdown
Collaborator

@johnny9 johnny9 left a comment

Choose a reason for hiding this comment

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

hoverEnabled: true needs to be set on the buttons for hover to work.

https://doc.qt.io/qt-5/qml-qtquick-controls2-control.html#hoverEnabled-prop

@johnny9
Copy link
Copy Markdown
Collaborator

johnny9 commented Feb 21, 2023

States can be simplified a bit to:

       states: [                                                                                                                                                                                                     
            State {                                                                                                                                                                                                   
                name: "PRESSED"; when: root.pressed                                                                                                                                                                   
                PropertyChanges { target: bg; color: Theme.color.orangeLight2 }                                                                                                                                       
            },                                                                                                                                                                                                        
            State {                                                                                                                                                                                                   
                name: "HOVER"; when: root.hovered                                                                                                                                                                     
                PropertyChanges { target: bg; color: Theme.color.orangeLight1 }                                                                                                                                       
            },                                                                                                                                                                                                        
            State {                                                                                                                                                                                                   
                name: "DEFAULT";                                                                                                                                                                                      
                PropertyChanges { target: bg; color: Theme.color.orange }                                                                                                                                             
            }                                                                                                                                                                                                         
        ]               

when can be thought of like a string of "if-else" statements, order matters

This is just optional, though, being explicit has its benefits in terms of readability

@jarolrod jarolrod force-pushed the drop-redundant-mouse-area branch 2 times, most recently from b86f893 to f68fd69 Compare February 22, 2023 01:59
@jarolrod
Copy link
Copy Markdown
Contributor Author

Updated from b30fefd to b86f893, compare

Changes:

  • Simplify when states are triggered per review feedback
  • Removed the "DEFAULT" state as this isn't triggered without a specific condition. When components are not on a state they go back to the definitions given when the component was created, so the "DEFAULT" case is now just what where the definitions on creation.
  • Reverted the changes to NavButton as that control does need the MouseArea to achieve proper animation of the pressed state

Then updated from b86f893 to f68fd69, compare

Changes:

  • Specified hoverEnabled: true per review feedback
  • Removed an unnecessary extra condition from the Setting control "HOVER" state

@jarolrod jarolrod force-pushed the drop-redundant-mouse-area branch from f68fd69 to 80392a5 Compare February 22, 2023 02:59
@jarolrod
Copy link
Copy Markdown
Contributor Author

Updated from f68fd69 to 80392a5, compare

Changes:

  • reversed changes to Setting control because it needs a custom mouseArea for the same reasons that NavButton needs it
  • Ensure that Setting and NavButton are explicit with hoverEnabled: true

Copy link
Copy Markdown
Collaborator

@johnny9 johnny9 left a comment

Choose a reason for hiding this comment

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

ACK 80392a5

@hebasto hebasto merged commit 7b168da into bitcoin-core:main Feb 22, 2023
johnny9 pushed a commit to johnny9/bitcoin-core-app that referenced this pull request Jul 4, 2025
…rols that are Buttons

ab6cd13 qml: remove redundant MouseAreas from controls that are Buttons (Jarol Rodriguez)

Pull request description:

  For controls that are buttons, an additional `MouseArea` layer is unnecessary as they already have built-in event detection for actions such as [pressing](https://doc.qt.io/qt-6/qml-qtquick-controls2-abstractbutton.html#pressed-prop) and [hovering](https://doc.qt.io/qt-6/qml-qtquick-controls2-control.html#hovered-prop). By eliminating the redundant MouseAreas in the `ContinueButton`, `NavButton`, `OutlineButton`, `Setting`, and `TextButton` controls, we make their implementations cleaner. Previously, the `MouseArea` was utilized to set the controls' state, but now this is achieved through `when` statements that are bundled with the states themselves.

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

ACKs for top commit:
  johnny9:
    ACK ab6cd13

Tree-SHA512: 1bebc46d6a3b2449ae028e799611711197756c212899f831005b35881281b7f1a2a4d93b8cb39d683b9b90621928421e99c26491195a870b99eb260d08ce21b7
tx-signer450 added a commit to tx-signer450/gui-qml that referenced this pull request Oct 20, 2025
…rols that are Buttons

ab6cd136ba9e977eb92a2234290e7c5f572ecfd7 qml: remove redundant MouseAreas from controls that are Buttons (Jarol Rodriguez)

Pull request description:

  For controls that are buttons, an additional `MouseArea` layer is unnecessary as they already have built-in event detection for actions such as [pressing](https://doc.qt.io/qt-6/qml-qtquick-controls2-abstractbutton.html#pressed-prop) and [hovering](https://doc.qt.io/qt-6/qml-qtquick-controls2-control.html#hovered-prop). By eliminating the redundant MouseAreas in the `ContinueButton`, `NavButton`, `OutlineButton`, `Setting`, and `TextButton` controls, we make their implementations cleaner. Previously, the `MouseArea` was utilized to set the controls' state, but now this is achieved through `when` statements that are bundled with the states themselves.

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

ACKs for top commit:
  johnny9:
    ACK ab6cd136ba9e977eb92a2234290e7c5f572ecfd7

Tree-SHA512: 1bebc46d6a3b2449ae028e799611711197756c212899f831005b35881281b7f1a2a4d93b8cb39d683b9b90621928421e99c26491195a870b99eb260d08ce21b7
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.

3 participants