D25381: Make QPushButton/QToolButton the same height as QLineEdit

[Yunhe Guo]

guoyunhe added a comment.


  In D25381#566128 , @ndavis wrote:
  
  > `oxygen-demo5`
  
  
  Thanks for the tip!

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D25381

To: guoyunhe, #breeze, #plasma, ngraham, ndavis
Cc: ndavis, GB_2, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25381: Make QPushButton/QToolButton the same height as QLineEdit

[Noah Davis]

ndavis added a comment.


  In D25381#566100 , @guoyunhe wrote:
  
  > Yes, this patch needs minimization. What is the software in your 
screenshot? @ndavis
  
  
  `oxygen-demo5`

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D25381

To: guoyunhe, #breeze, #plasma, ngraham, ndavis
Cc: ndavis, GB_2, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25381: Make QPushButton/QToolButton the same height as QLineEdit

[Yunhe Guo]

guoyunhe added a comment.


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

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D25381

To: guoyunhe, #breeze, #plasma, ngraham, ndavis
Cc: ndavis, GB_2, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25381: Make QPushButton/QToolButton the same height as QLineEdit

[Noah Davis]

ndavis requested changes to this revision.
ndavis added a comment.


  This is what happens when icon size is set to 32px or more (48px here): 
F7776520: Screenshot_20191121_062030.png 
  `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.

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D25381

To: guoyunhe, #breeze, #plasma, ngraham, ndavis
Cc: ndavis, GB_2, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25381: Make QPushButton/QToolButton the same height as QLineEdit

[Yunhe Guo]

guoyunhe added a comment.


  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?

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D25381

To: guoyunhe, #breeze, #plasma, ngraham
Cc: ndavis, GB_2, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25381: Make QPushButton/QToolButton the same height as QLineEdit

[Nathaniel Graham]

ngraham added a comment.


  QWidgets KCMs in English.

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D25381

To: guoyunhe, #breeze, #plasma, ngraham
Cc: ndavis, GB_2, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25381: Make QPushButton/QToolButton the same height as QLineEdit

[Yunhe Guo]

guoyunhe added a comment.


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

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D25381

To: guoyunhe, #breeze, #plasma, ngraham
Cc: ndavis, GB_2, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25381: Make QPushButton/QToolButton the same height as QLineEdit

[Nathaniel Graham]

ngraham requested changes to this revision.
ngraham added a comment.
This revision now requires changes to proceed.


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

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

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D25381

To: guoyunhe, #breeze, #plasma, ngraham
Cc: ndavis, GB_2, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25381: Make QPushButton/QToolButton the same height as QLineEdit

[Yunhe Guo]

guoyunhe updated this revision to Diff 70059.
guoyunhe added a comment.


  Force height consistency

REPOSITORY
  R31 Breeze

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25381?vs=69954=70059

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D25381

AFFECTED FILES
  kstyle/breezestyle.cpp

To: guoyunhe, #breeze, #plasma
Cc: ndavis, GB_2, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25381: Make QPushButton/QToolButton the same height as QLineEdit

[Yunhe Guo]

guoyunhe added a comment.


  The remaining 1px difference between QPushButton and QLineEdit is caused by 
content. QLineEdit directly expands from its content size, while QPushButton 
calculate size of text metrics and icons.

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D25381

To: guoyunhe, #breeze, #plasma
Cc: ndavis, GB_2, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25381: Make QPushButton/QToolButton the same height as QLineEdit

[Yunhe Guo]

guoyunhe added a comment.


  Left: JuK (Qt Widgets), Right: Elisa (Qt QML)
  
  F7771662: image.png 

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D25381

To: guoyunhe, #breeze, #plasma
Cc: ndavis, GB_2, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25381: Make QPushButton/QToolButton the same height as QLineEdit

[Nathaniel Graham]

ngraham added a comment.


  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?

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D25381

To: guoyunhe, #breeze, #plasma
Cc: ndavis, GB_2, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25381: Make QPushButton/QToolButton the same height as QLineEdit

[Yunhe Guo]

guoyunhe added a comment.


  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.

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D25381

To: guoyunhe, #breeze, #plasma
Cc: ndavis, GB_2, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25381: Make QPushButton/QToolButton the same height as QLineEdit

[Noah Davis]

ndavis added a comment.


  In D25381#564512 , @guoyunhe wrote:
  
  > In D25381#564404 , @ndavis wrote:
  >
  > > 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:
  >
  > F7771214: image.png 
  >
  > When change to 32px, still okay:
  >
  > F7771218: image.png 
  >
  > 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. 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.

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D25381

To: guoyunhe, #breeze, #plasma
Cc: ndavis, GB_2, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25381: Make QPushButton/QToolButton the same height as QLineEdit

[Yunhe Guo]

guoyunhe added a comment.


  In D25381#564404 , @ndavis wrote:
  
  > 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:
  
  F7771214: image.png 
  
  When change to 32px, still okay:
  
  F7771218: image.png 
  
  Maybe this is because Dolphin has all icons in 32px. Do you know any 
application that miss 32px icons in toolbar? Thanks!

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D25381

To: guoyunhe, #breeze, #plasma
Cc: ndavis, GB_2, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25381: Make QPushButton/QToolButton the same height as QLineEdit

[Noah Davis]

ndavis added a comment.


  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. Also, 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.

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D25381

To: guoyunhe, #breeze, #plasma
Cc: ndavis, GB_2, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25381: Make QPushButton/QToolButton the same height as QLineEdit

[Yunhe Guo]

guoyunhe added a comment.


  In D25381#564370 , @ngraham wrote:
  
  > +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.

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D25381

To: guoyunhe, #breeze, #plasma
Cc: GB_2, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25381: Make QPushButton/QToolButton the same height as QLineEdit

[Yunhe Guo]

guoyunhe added a comment.


  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 :(

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D25381

To: guoyunhe, #breeze, #plasma
Cc: GB_2, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25381: Make QPushButton/QToolButton the same height as QLineEdit

[Björn Feber]

GB_2 added a comment.


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

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D25381

To: guoyunhe, #breeze, #plasma
Cc: GB_2, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25381: Make QPushButton/QToolButton the same height as QLineEdit

[Björn Feber]

GB_2 added a comment.


  In D25381#564370 , @ngraham wrote:
  
  > +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.

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D25381

To: guoyunhe
Cc: GB_2, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25381: Make QPushButton/QToolButton the same height as QLineEdit

[Nathaniel Graham]

ngraham added a comment.


  +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.

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D25381

To: guoyunhe
Cc: ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, 
GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25381: Make QPushButton/QToolButton the same height as QLineEdit

[Yunhe Guo]

guoyunhe created this revision.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
guoyunhe requested review of this revision.

REVISION SUMMARY
  BUG 411811

REPOSITORY
  R31 Breeze

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D25381

AFFECTED FILES
  kstyle/breezestyle.cpp

To: guoyunhe
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart