Fix context menu button's icon size when on default DPI
ClosedPublic

Authored by Zren on Feb 28 2017, 5:41 PM.

Details

Summary

ToolButton pads the contexts of the label+icon based on the desktop theme's svg margins. Causing the icon to only be 12px, which is scaled down and looks like a solid square with the breeze icon, as seen in D4751 comments. We want this button to be reasonably small, but should have the minimum of a 16px icon. The button itself is 24px atm (based off the slider.height). Since the icon doesn't go right to the edge, we can just overlay the icon on top of the button and apply all the ToolButtonStyle effects.

Test Plan

Tested on 96 DPI and 2x DPI with:
QT_DEVICE_PIXEL_RATIO=2 plasmoidviewer -a applet -l floating -f horizontal

Diff Detail

Repository
R115 Plasma Audio Volume Applet
Lint
Lint Skipped
Unit
Unit Tests Skipped
Zren created this revision.Feb 28 2017, 5:41 PM
Restricted Application added a project: Plasma. · View Herald TranscriptFeb 28 2017, 5:41 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
Zren edited the summary of this revision. (Show Details)Feb 28 2017, 5:43 PM
drosca added inline comments.Feb 28 2017, 5:49 PM
applet/contents/ui/ListItemBase.qml
122

Do we really need this (we certainly don't need the parent.flat part)? It doesn't seem to change the icon at all.

Zren added inline comments.Feb 28 2017, 5:54 PM
applet/contents/ui/ListItemBase.qml
122

Not really. Just wanted to have consistent code with ToolButtonStyle, but I already had to change control => parent so might as well remove it. ToolButton is basically a flat: true Button so that part will always be !true/false.

Zren added inline comments.Feb 28 2017, 6:00 PM
applet/contents/ui/ListItemBase.qml
122

As for active+colorGroup, you'll notice the hover effect if you change the icon to something with color, like source: "plasma". The default hover effect doesn't do much (white overlay on white symbolic icon) with breeze, but might be necessary for other themes.

romangg accepted this revision.Feb 28 2017, 6:16 PM

Tested it and works well. You can omit the !parent.flat condition as @drosca said, since it's always false.

This revision is now accepted and ready to land.Feb 28 2017, 6:16 PM
Zren updated this revision to Diff 11969.Feb 28 2017, 6:26 PM

Remove || !parent.flat

Zren marked 3 inline comments as done.Feb 28 2017, 6:27 PM
broulik edited edge metadata.Feb 28 2017, 7:08 PM

What about making ToolButton's icon size a minimum of small icon size no matter what? I don't see a case where you ever want to have it smaller.

Zren added a comment.Feb 28 2017, 7:46 PM

That's a good idea @broulik .

Slider height for the Desktop themes I have installed (I used this script):

  • air: 20px
  • Arctica: 24px
  • Arezzo: 24px
  • arquetype-dark: 24px
  • Bise: 24px
  • breeze-alphablack: 24px
  • BreezeAlphaBlackNaked: 24px
  • damadamas: 24px
  • Freeze: 24px
  • Fushigi: 12px <==
  • Ghost: 24px
  • Glassified: 16px
  • Glassified: 16px
  • Glaze: 24px
  • Glaze: 24px
  • Helium: 24px
  • Helium: 24px
  • Interceptor: 24px
  • K10ne: 18px
  • K10neBlack: 18px
  • Mild: 24px
  • Professional: 24px
  • SimpleFlow_Modified: 24px
  • slim-glow: 24px
  • Sly: 24px
  • tragedy: 24px
  • Underworld: 16px
  • Volatile: 16px
  • Wave: 24px

Only Fushigi seems to be <16px, but a number of sliders are 16px which means the icon only has 0-1px of padding.

So we probably need to set Layout.preferredHeight: Math.max(slider.height, units.iconSizes.small) in the ToolButton?

@Zren did you get that original problem in plasmashell as well as plasmoidviewer?

The description doesn't make much sense because Plasmashell disables Qt's high DPI stuff.

Zren added a comment.Mar 1 2017, 4:31 PM

@davidedmundson: Yes. I usually test in plasmoidviewer since it's the same (except for emoji rendering).

I get the same effect because you're still trying to stuff a 16px icon into a 12px hole at 1x DPI (96). The original patch was tested with 2x DPI so the icon was scaled up first before being scaled down, which rendered it at 24px and is still recognizable. At 12px, the triple bars squish together to look like a solid square.

aacid requested changes to this revision.Mar 3 2017, 7:15 PM
aacid added a subscriber: aacid.

Patch does not seem to apply

This revision now requires changes to proceed.Mar 3 2017, 7:15 PM
drosca edited edge metadata.Mar 3 2017, 7:31 PM

So we probably need to set Layout.preferredHeight: Math.max(slider.height, units.iconSizes.small) in the ToolButton?

Actually, let's make this and also volume indicator icon (next to theslider) have size = units.iconSizes.small and remove the relative sizing by slider height (which may differ a lot on different themes).

Otherwise, it's good to go.

Zren updated this revision to Diff 12221.Mar 6 2017, 2:03 AM
Zren edited edge metadata.

Fix merge conflict based on removal of x/y parameters in contextmenu.show()
Change height from slider.height to units.iconSizes.small

Zren added a comment.Mar 6 2017, 2:03 AM

Ah, the contextMenu.show(x, y) changed to contextMenu.show().
I've got to comment out roundToIconSize: false in the mute "button" to test now since that probably requires a new version of frameworks.

I changed the slider.height to Layout.preferredHeight: units.iconSizes.small, which makes the button 16px. Screenshot below.

Also including a screenshot of smallMedium which is 22px since it looks closer to breeze's slider.height.

drosca accepted this revision.Mar 6 2017, 2:14 PM
aacid resigned from this revision.Mar 6 2017, 9:00 PM

Abstain

This revision is now accepted and ready to land.Mar 6 2017, 9:00 PM
This revision was automatically updated to reflect the committed changes.