D5144: Change the volume icon/mute button into a ToolButton

This revision was automatically updated to reflect the committed changes. Closed by commit R115:2ac365e96b70: Change the volume icon/mute button into a ToolButton (authored by Zren). REPOSITORY R115 Plasma Audio Volume Applet CHANGES SINCE LAST UPDATE

D5144: Change the volume icon/mute button into a ToolButton

drosca accepted this revision. drosca added a comment. This revision is now accepted and ready to land. > 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.

D5144: Change the volume icon/mute button into a ToolButton

Zren added a comment. Should I go ahead and commit this? REPOSITORY R115 Plasma Audio Volume Applet REVISION DETAIL https://phabricator.kde.org/D5144 To: Zren, #plasma, subdiff, drosca Cc: mart, subdiff, drosca, plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts,

D5144: Change the volume icon/mute button into a ToolButton

Zren updated this revision to Diff 13070. Zren edited the summary of this revision. REPOSITORY R115 Plasma Audio Volume Applet CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D5144?vs=12746=13070 REVISION DETAIL https://phabricator.kde.org/D5144 AFFECTED FILES

D5144: Change the volume icon/mute button into a ToolButton

Zren added a comment. > 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.

D5144: Change the volume icon/mute button into a ToolButton

drosca added a comment. In https://phabricator.kde.org/D5144#97171, @Zren wrote: > 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

D5144: Change the volume icon/mute button into a ToolButton

mart added a comment. i think it looks better (tough only how it looks on roman's machine) REPOSITORY R115 Plasma Audio Volume Applet REVISION DETAIL https://phabricator.kde.org/D5144 To: Zren, #plasma, subdiff, drosca Cc: mart, subdiff, drosca, plasma-devel, progwolff, lesliezhai,

D5144: Change the volume icon/mute button into a ToolButton

Zren added a comment. 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. https://streamable.com/07hc0 REPOSITORY R115 Plasma

D5144: Change the volume icon/mute button into a ToolButton

drosca added inline comments. INLINE COMMENTS > SmallToolButton.qml:11 > +property int padding: units.smallSpacing > +Layout.preferredWidth: iconSize + padding * 2 > +Layout.preferredHeight: iconSize + padding * 2 This won't work when not in Layout, I think you can use

D5144: Change the volume icon/mute button into a ToolButton

Zren updated this revision to Diff 12746. Zren added a comment. Create a reusuble SmallToolButton class. Add smallSpacing around the mute button. Remove spacing between the two rows to counter the increase in height. REPOSITORY R115 Plasma Audio Volume Applet CHANGES SINCE LAST UPDATE

D5144: Change the volume icon/mute button into a ToolButton

Zren added a comment. Another thing we could do to keep things small vertically is set `spacing: 0` in the `ColumnLayout { id: column }`. F3154383: 2017-03-23___11-50-47.png REPOSITORY R115 Plasma Audio Volume Applet REVISION DETAIL

D5144: Change the volume icon/mute button into a ToolButton

Zren added a comment. Ah, ignore the `visible: false` in that, was testing which variables ToolButton uses (iconSource. iconName). REPOSITORY R115 Plasma Audio Volume Applet REVISION DETAIL https://phabricator.kde.org/D5144 To: Zren, #plasma, subdiff, drosca Cc: subdiff, drosca,

D5144: Change the volume icon/mute button into a ToolButton

Zren added a comment. 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 F3154040:

D5144: Change the volume icon/mute button into a ToolButton

drosca added a comment. > 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

D5144: Change the volume icon/mute button into a ToolButton

subdiff added a comment. > 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

D5144: Change the volume icon/mute button into a ToolButton

drosca requested changes to this revision. drosca added a comment. This revision now requires changes to proceed. 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,

D5144: Change the volume icon/mute button into a ToolButton

subdiff added subscribers: drosca, subdiff. subdiff accepted this revision. subdiff added a comment. This revision is now accepted and ready to land. 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

D5144: Change the volume icon/mute button into a ToolButton

subdiff added a reviewer: Plasma. REPOSITORY R115 Plasma Audio Volume Applet REVISION DETAIL https://phabricator.kde.org/D5144 To: Zren, #plasma Cc: plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

D5144: Change the volume icon/mute button into a ToolButton

Zren created this revision. Restricted Application added a project: Plasma. Restricted Application added a subscriber: plasma-devel. REVISION SUMMARY This adds a hover effect so the user knows it's a button. Unlike the application-menu button, I didn't add the smallSpacing around it so