[Differential] [Commented On] D3706: WIP: Global Menu Applet

[broulik (Kai Uwe Broulik)]

broulik added inline comments.

INLINE COMMENTS

> appmenuapplet.cpp:91
> +                ctx->window()->mouseGrabberItem()->ungrabMouse();
> +            }
> +

Somehow your indentation is a bit off?

> configGeneral.qml:44
> +            id: fullRepresentationRadio
> +            text: i18n("Full Representation")
> +            exclusiveGroup: representationGroup

Not too happy with the naming here. The old applet just had a checkbox for "Use single button" for menu or something like that, that would also simplify the settings page a lot ;)

> configGeneral.qml:56
> +        if (plasmoid.configuration.fullRepresentation) {
> +             fullRepresentationRadio.checked = true
> +        } else {

I *think* you can just in the ExclusiveGroup above bind to the "current" property:

  Controls.ExclusiveGroup {
      id: representationGroup
      current: plasmoid.configuration.fullRepresentation ? fullRepresentationRadio : compactRepresentationRadio
  }

or you could perhaps just bind "checked" in the RadioButtons

  checked: plasmoid.configuration.fullRepresentation

> main.qml:25
> +import org.kde.plasma.components 2.0 as PlasmaComponents
> +import org.kde.plasma.private.appmenu 1.0 as AppMenuPrivate
> +

Can you perhaps look into making it a c++ applet rather than using a private import – we somewhat try to get away from this

> main.qml:37-38
> +
> +    Layout.minimumWidth: units.iconSizes.large
> +    Layout.minimumHeight: units.iconSizes.large
> +

Perhaps base that on font size (or just units.gridUnit)

> main.qml:42
> +
> +    Plasmoid.compactRepresentation: Component {
> +        PlasmaComponents.ToolButton {

I don't think this needs to be wrapped in Component

> main.qml:43
> +    Plasmoid.compactRepresentation: Component {
> +        PlasmaComponents.ToolButton {
> +            Layout.fillWidth: root.vertical

Also, I find it pretty cool that you just use compact and full rep for that :) I would not have thought of that

> main.qml:67-73
> +                var count = buttonRepeater.count
> +                var idx = index;
> +                if (idx >= count) {
> +                    idx = 0;
> +                } else if (idx < 0) {
> +                    idx = count - 1;
> +                }

var idx = Math.max(0, Math.min(buttonRepeater.count - 1, index))

(too bad there's no qBound in QML)

> main.qml:77
> +                if (button) {
> +                    plasmoid.nativeInterface.trigger(button, button.buttonData, idx);
> +                }

You could just do button.clicked() which will end up executing the onClicked below instead of duplicating the logic here

> main.qml:88
> +                readonly property int buttonIndex: index
> +                readonly property var buttonData: activeActions
> +

buttonData is pretty generic of a name

REPOSITORY
  R120 Plasma Workspace

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: chinmoyr, #plasma, broulik
Cc: plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20161216/8a37b339/attachment-0001.html>