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
