Skip to content
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

Make console buttons look clickable #329

Merged
merged 1 commit into from Jun 5, 2021

Conversation

jarolrod
Copy link
Member

@jarolrod jarolrod commented May 14, 2021

On master, for macOS, the console buttons' hitboxes are quite small. This makes clicking on the button with your mouse a little more tedious than it should be. The Issue is related to recent versions of Qt (>5.9.8) not playing so nice on macOS when there are "incorrect" width and height values set for a QPushButton (here is another example: #319 (review)).

This fixes this small hitbox issue by converting the buttons from QPushButton to QToolButton, which in turn makes the buttons look explicitly clickable. This approach was chosen as it helps us avoid having to play around with width and height values until we find values that play nice with macOS and look good on Linux & Windows. Also, QToolButton is an appropriate class for these buttons.

Per Qt Docs:

A tool button is a special button that provides quick-access to specific commands or options. As opposed to a normal command button, a tool button usually doesn't show a text label, but shows an icon instead.

Since we are changing the type of the buttons, we need to change the respective actions connection logic in rpcconsole. Instead of plugging in QToolButton, we abstract it to the base class: QAbstractButton.

per Qt Dev Notes

Use base class functions as this makes the code more general, e.g., use QAbstractButton::clicked instead of QPushButton::clicked.

While here, we also update the size of the icons to 22x22 to be consistent with other tool buttons.

macOS: Master vs PR:

Master PR
master-ss-macos pr-ss-macos

Linux: Master vs PR:

Master PR
master-ss-linux pr-ss-linux

@hebasto hebasto added the UI All about "look and feel" label May 15, 2021
@hebasto
Copy link
Member

hebasto commented May 15, 2021

Concept ACK on making clickable elements explicit (see #217).

On master, for macOS, the console buttons' hitboxes are quite small

How do your changes increase their sizes?

@jarolrod
Copy link
Member Author

@hebasto

How do your changes increase their sizes?

It seems a better name for this PR would be "Make Console Buttons Look Clickable" as that is what it really does for all systems. The PR was motivated by the small hitbox bug on macOS, this is fixed by making the buttons a QToolButton instead of finding appropriate width and height values for aQPushButton.

Change the type for the console's buttons to QToolButton which will make them look explicitly clickable, which in turn fixes the small hitbox issue for macOS.
With this change, we need to generalize the respective action connect logic from QPushButton to QAbstractButton.
While here, update width and height of icon for consistency with other tool buttons.
@jarolrod jarolrod changed the title fix console buttons hitbox on macOS Make console buttons look clickable May 15, 2021
@jarolrod
Copy link
Member Author

updated from 05808da -> 8b419b5:

Changes:

  • Change naming of commit and pr to better represent what the PR is doing

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK 8b419b5, tested on Linux Mint 20.1 (Qt 5.12.8).

@promag
Copy link
Contributor

promag commented May 24, 2021

Not a NACK, personally I think they are fine as is.

Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

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

Tested ACK 8b419b5 on macOS Big Sur M1, this drops only relevant usages to flat buttons.

nit, could drop the following:

<property name="flat">
<bool>false</bool>
</property>

@promag
Copy link
Contributor

promag commented May 31, 2021

Played around and with

          </item>
          <item>
           <widget class="QPushButton" name="fontSmallerButton">
-           <property name="maximumSize">
-            <size>
-             <width>24</width>
-             <height>24</height>
-            </size>
-           </property>
            <property name="toolTip">
             <string>Decrease font size</string>
            </property>
@@ -490,7 +484,7 @@
            <property name="iconSize">
             <size>
              <width>24</width>
-             <height>16</height>
+             <height>24</height>
             </size>
            </property>
            <property name="autoDefault">

I get this
Screenshot 2021-06-01 at 00 30 31
and when clicking
Screenshot 2021-06-01 at 00 30 50
So it seems that maximumSize is screwing the button layout.

@hebasto hebasto merged commit 916f45e into bitcoin-core:master Jun 5, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 6, 2021
8b419b5 qt: make console buttons look clickable (Jarol Rodriguez)

Pull request description:

  On master, for macOS, the console buttons' hitboxes are quite small. This makes clicking on the button with your mouse a little more tedious than it should be. The Issue is related to recent versions of Qt (>5.9.8) not playing so nice on macOS when there are "incorrect" `width` and `height` values set for a `QPushButton` (here is another example: bitcoin-core/gui#319 (review)).

  This fixes this small hitbox issue by converting the buttons from `QPushButton` to `QToolButton`, which in turn makes the buttons look explicitly clickable. This approach was chosen as it helps us avoid having to play around with `width` and `height` values until we find values that play nice with macOS and look good on Linux & Windows. Also, `QToolButton` is an appropriate class for these buttons.

  Per [Qt Docs](https://doc.qt.io/qt-5/qtoolbutton.html#details):
  > A tool button is a special button that provides quick-access to specific commands or options. As opposed to a normal command button, a tool button usually doesn't show a text label, but shows an icon instead.

  Since we are changing the type of the buttons, we need to change the respective actions connection logic in `rpcconsole`. Instead of plugging in `QToolButton`, we abstract it to the base class: `QAbstractButton`.

  per [Qt Dev Notes](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Developer-Notes-for-Qt-Code#inherited-signals-and-slot)
  > Use base class functions as this makes the code more general, e.g., use QAbstractButton::clicked instead of QPushButton::clicked.

  While here, we also update the size of the icons to `22x22` to be consistent with other tool buttons.

  **macOS: Master vs PR:**

  | Master        | PR               |
  | ----------- | ----------- |
  | ![master-ss-macos](https://user-images.githubusercontent.com/23396902/118339460-e9079c80-b4e6-11eb-864b-d394aca5df61.png) | ![pr-ss-macos](https://user-images.githubusercontent.com/23396902/118339468-ec9b2380-b4e6-11eb-9a9e-30620216750e.png) |

  **Linux: Master vs PR:**

  | Master        | PR               |
  | ----------- | ----------- |
  | ![master-ss-linux](https://user-images.githubusercontent.com/23396902/118339520-13595a00-b4e7-11eb-86d0-96dd1264c198.png) | ![pr-ss-linux](https://user-images.githubusercontent.com/23396902/118339533-1c4a2b80-b4e7-11eb-8d7f-f733d999c8fd.png) |

ACKs for top commit:
  hebasto:
    ACK 8b419b5, tested on Linux Mint 20.1 (Qt 5.12.8).
  promag:
    Tested ACK 8b419b5 on macOS Big Sur M1, this drops only relevant usages to `flat` buttons.

Tree-SHA512: 3f3cdcbe83398136a1d1ee8fc2835be8681f2ed39e79db1e939cab6a00a779f528343d54992807a845cc84d9ef13591affb7a6dbca9e5753a2b8665b0af4d611
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022
@bitcoin-core bitcoin-core locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
UI All about "look and feel"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants