This adds a hover effect so the user knows it's a button.
Creates a reuseable SmallToolButton which overlays an IconItem on a ToolButton ignoring the svg's margins and ToolButton's padding.
Changes the spacing between the rows to 1px (0px causes the ColumnLayout to reflow and the label sometimes shifts).
| Lint Skipped |
| Unit Tests Skipped |
Looks very nice. I also don't think that the hover line looks weird. It's just a little bit stupid, that for these small buttons we always have to use the workaround with the IconItem. Can't we introduce a parameter in ToolButton to reduce the margins from its native icon? But this is unrelated to this patch. So accepted from my side. @drosca ?
I don't like it, the context menu button is enough hacks for one applet. Also it doesn't have spacing so it looks wrong compared to other ToolButtons (contex menu have spacing, but the icon size is still iconSizes.small).
If we want to use ToolButtons in this way, it should be fixed in ToolButton.
I don't like it, the context menu button is enough hacks for one applet.
The "hack" is consistent with the one of context menu button.
I think the improved discoverability outweighs your objections. We can use this here until someone diggs deeper and improves ToolButton in this regard.
@Zren: Can you tell me why your icon looks different from mine and why on my I still have a small (and nice) margin:
The "hack" is consistent with the one of context menu button.
It's not, context menu button have spacing
I think the improved discoverability outweighs your objections. We can use this here until someone diggs deeper and improves ToolButton in this regard.
If we go with this, it must be moved into a separate item, let's say SmallToolButton.qml
@Zren: Can you tell me why your icon looks different from mine and why on my I still have a small (and nice) margin:
Because you use scaling and 22px version of the icon has bigger margins.
Yeah, it's probably dpi scaling.
And I didn't want to mess with the height of the ListItemBase. On further testing, it seems adding smallSpacing margins isn't as bad as I thought.
Back and Forth Gif: https://streamable.com/0ld55
If the increase in height is acceptable, I'll make a SmallToolButton
PlasmaComponents.ToolButton {
id: smallToolButton
property int iconSize: units.iconSizes.small
property int padding: units.smallSpacing
implicitWidth: iconSize + padding * 2
implicitHeight: iconSize + padding * 2
Layout.preferredWidth: implicitWidth
Layout.preferredHeight: implicitHeight
property alias icon: icon.source
PlasmaCore.IconItem {
id: icon
anchors.fill: parent
anchors.margins: parent.padding
visible: false
// From Plasma's ToolButtonStyle:
active: parent.hovered
colorGroup: parent.hovered ? PlasmaCore.Theme.ButtonColorGroup : PlasmaCore.ColorScope.colorGroup
}
}Ah, ignore the visible: false in that, was testing which variables ToolButton uses (iconSource. iconName).
Another thing we could do to keep things small vertically is set spacing: 0 in the ColumnLayout { id: column }.
Create a reusuble SmallToolButton class.
Add smallSpacing around the mute button.
Remove spacing between the two rows to counter the increase in height.
| applet/contents/ui/SmallToolButton.qml | ||
|---|---|---|
| 11 | This won't work when not in Layout, I think you can use implicitWidth/Height to have the same effect. | |
Before tackling the implicitWidth/height stuff, I noticed that my microphone's Heading label was getting moved on the first click. Adding height: contextMenuButton.height fixes it but that doesn't feel right.
I've seen that, but that is unrelated to this change, so please move it into separate review.
I've seen that, but that is unrelated to this change, so please move it into separate review.
Sorry. I hit a wall on that bug and it drove me crazy. I just found out that changing spacing: 0 to spacing: 1 will cause things to layout correctly the first time.
My theory is that clicking the button will change the ToolButton's svg, which probably causes the naturalHeight to be set, causing the ColumnLayout to recalculate the height of the rows. Whatever is loading wrong at initialization is corrected, causes the reflow, which makes the text shift.
My theory is that clicking the button will change the ToolButton's svg, which probably causes the naturalHeight to be set, causing the ColumnLayout to recalculate the height of the rows. Whatever is loading wrong at initialization is corrected, causes the reflow, which makes the text shift.
No, it's a bug in PlasmaComponents.Label, it sets:
height: Math.round(Math.max(paintedHeight, theme.mSize(theme.defaultFont).height*1.6))
If you do height: paintedHeight in our code, the issue is fixed.
Should I go ahead and commit this?
Yes, thanks.