Make QPushButton/QToolButton the same height as QLineEdit
Needs RevisionPublic

Authored by guoyunhe on Nov 18 2019, 9:04 PM.

Details

Reviewers
ngraham
ndavis
Group Reviewers
Breeze
Plasma
Summary

BUG 411811

Test Plan
  1. Open KCM Font Management
  2. Check the font filter: height of the text box and menu button.

Diff Detail

Repository
R31 Breeze
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 18921
Build 18939: arc lint + arc unit
guoyunhe created this revision.Nov 18 2019, 9:04 PM
Restricted Application added a project: Plasma. · View Herald TranscriptNov 18 2019, 9:04 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
guoyunhe requested review of this revision.Nov 18 2019, 9:04 PM

+1 conceptually, this will be nice to finally have fixed.

But might the opposite make more sense? If we make buttons shorter, we're slightly reducing their click targets, but if we make line edits taller, all we're doing it reducing unused whitespace.

guoyunhe edited the summary of this revision. (Show Details)Nov 18 2019, 9:07 PM
guoyunhe edited the test plan for this revision. (Show Details)
GB_2 added a subscriber: GB_2.Nov 18 2019, 9:08 PM
This comment was removed by GB_2.
guoyunhe edited the summary of this revision. (Show Details)Nov 18 2019, 9:08 PM
guoyunhe added reviewers: Breeze, Plasma.
GB_2 added a comment.EditedNov 18 2019, 9:09 PM

Plus in the screenshot it looks like it's still not quite the same height.

In D25381#564378, @GB_2 wrote:

Plus in the screenshot it looks like it's still not quite the same height.

Yes, still 1px taller :(

+1 conceptually, this will be nice to finally have fixed.

But might the opposite make more sense? If we make buttons shorter, we're slightly reducing their click targets, but if we make line edits taller, all we're doing it reducing unused whitespace.

From my personal experience, I didn't have any difficulty to click buttons of new sizes. And it seems the same height as buttons in Phabricator.

ndavis added a subscriber: ndavis.EditedNov 18 2019, 10:34 PM

If we do make flat toolbuttons buttons (autoRaise == true) and non-flat buttons the same size, we will need to change the default icon size for all buttons to 22px, or we will have a ton of UI regressions. Any non-flat button with a fixed icon size is going to have a regression. Anyone who was using non-flat buttons with 22px icons to get a bigger clickable area will need to switch to 32px icons. This also means that breeze-icons' problem with 32px icon style consistency will show itself more often.

If we do make flat toolbuttons buttons (autoRaise == true) and non-flat buttons the same size, we will need to change the default icon size for all buttons to 22px, or we will have a ton of UI regressions. Any non-flat button with a fixed icon size is going to have a regression. Anyone who was using non-flat buttons with 22px icons to get a bigger clickable area will need to switch to 32px icons. This also means that breeze-icons' problem with 32px icon style consistency will show itself more often.

Thanks for the remind!

By default, QToolButton with 22px icons look good:

When change to 32px, still okay:

Maybe this is because Dolphin has all icons in 32px. Do you know any application that miss 32px icons in toolbar? Thanks!

ndavis added a comment.EditedNov 19 2019, 9:48 AM

If we do make flat toolbuttons buttons (autoRaise == true) and non-flat buttons the same size, we will need to change the default icon size for all buttons to 22px, or we will have a ton of UI regressions. Any non-flat button with a fixed icon size is going to have a regression. Anyone who was using non-flat buttons with 22px icons to get a bigger clickable area will need to switch to 32px icons. This also means that breeze-icons' problem with 32px icon style consistency will show itself more often.

Thanks for the remind!

By default, QToolButton with 22px icons look good:

When change to 32px, still okay:

Maybe this is because Dolphin has all icons in 32px. Do you know any application that miss 32px icons in toolbar? Thanks!

This issue is most likely to show up in 3rd party apps. Maybe we shouldn't let an issue like that dictate the design of Breeze widgets though since I expect either Breeze icons or the XDG icon specs will change one day to fix that issue. For now, I think we should stick to reducing the size of non-flat buttons or increasing the size of comboboxes and line edits. If we make non-flat button sizes match flat button sizes, we should do that in another patch.

I personally think the height of line edits and comboboxes should be increased to 32px (using default font and icon settings). This also makes it so we won't have to change how flat button sizes work if/when the non-flat buttons are made to match them.

In QML applications, buttons and line edits are already same height, the same height as QLineEdit in Qt Widgets applications. If we increase the height of QLineEdit, we have to change QML components' height, too. The amount of work will be bigger. If a view contains a lot of QLineEdit/QComboBox rows (like many KCM views), the content might overflow. So it is safer to make button height shorter.

If this patch makes the sizes match those in Kirigami/QML apps, I guess that seems fine, since nobody has noticed any problems with the sized there, right?

Left: JuK (Qt Widgets), Right: Elisa (Qt QML)

guoyunhe edited the summary of this revision. (Show Details)Nov 20 2019, 12:49 PM
guoyunhe added a comment.EditedNov 20 2019, 12:54 PM

The remaining 1px difference between QPushButton and QLineEdit is caused by content. Depending on fonts, the content size are always different.

Even in QML applications, like Elisa, we can notice the difference if we use different fonts:

Button height: 32px, LineEdit height: 34px. (Using Noto Sans SC fonts)

When applying size.setHeight(34); to each sizing function, they have exact same size. But the size doesn't respect users' font customization.

The best approach is to calculate font size directly, instead of relying on content size.

guoyunhe updated this revision to Diff 70059.EditedNov 20 2019, 4:30 PM

Force height consistency

Now they should have exactly same height (QPushButton, QToolButton, QLineEdit, QComboBox, QSpinBox):

guoyunhe edited the summary of this revision. (Show Details)Nov 20 2019, 4:30 PM
ngraham requested changes to this revision.Nov 20 2019, 6:02 PM

Things seem noticeably too short to me now, and QWidgets things are smaller than their QML versions. Observe:

Above, QWidgets. Below, QML:

This seems like too invasive a change. I think we should fix this bug, but not by resizing everything.

This revision now requires changes to proceed.Nov 20 2019, 6:02 PM

@ngraham which applications are they? I didn't see similar thing in my system.

QWidgets KCMs in English.

I checked all KCM modules, the bottom row of buttons are all in same height (like the smaller button in your screenshot). Have you tried reboot?

ndavis requested changes to this revision.Nov 21 2019, 11:33 AM

This is what happens when icon size is set to 32px or more (48px here):


size.setHeight(toolButtonOption->fontMetrics.height()); doesn't take icon size into account.
Also, still -1 to making flat buttons and non-flat buttons the same size at the same icon sizes in this patch. That change has potential consequences that can reach much farther than the button vs line edit height inconsistency that originally needed to be solved.

Yes, this patch needs minimization. What is the software in your screenshot? @ndavis

Yes, this patch needs minimization. What is the software in your screenshot? @ndavis

oxygen-demo5

oxygen-demo5

Thanks for the tip!