D28152: KWinRules KCM Redesign

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

> OptionsComboBox.qml:61
> +        }
> +        return i18np("1 selected", "%1 selected", selectionCount);
> +    }

Use `%1` instead of the hardcoded number 1 for the singular case (there are languages where e.g. the number 21 uses the singular form)

> OptionsComboBox.qml:68
> +
> +        LayoutMirroring.enabled: Qt.application.layoutDirection === Qt.RightToLeft
> +        LayoutMirroring.childrenInherit: true

Is this necessary to manually assign? I thought `LayoutMirroring.enabled` got set automatically?

> OptionsComboBox.qml:108
> +
> +        Component.onCompleted: {
> +            optionsCombo.popup.width = Math.max(implicitWidth, optionsCombo.width, optionsCombo.popup.width);

Can you add a TODO or FIXME comment indicating that this works around https://bugs.kde.org/show_bug.cgi?id=403153?

> OptionsComboBox.qml:113
> +        onFocusChanged: {
> +            if (!focus) popup.close();
> +        }

Add braces, either on the same line, or in the more conventional multi-line style

> RuleItemDelegate.qml:58
> +            horizontalAlignment: Text.AlignLeft
> +            elide: Text.ElideRight
> +        }

elision only works when a label's maximum width is defined; you probably want to set that somehow, or make it not elide

> RulesEditor.qml:84
> +            id: delaySpin
> +            Layout.minimumWidth: Kirigami.Units.gridUnit * 6
> +            from: 0

In English at least, this minimum width isn't high enough to avoid the spinbox getting wider when going from 1 to 2.

> RulesEditor.qml:89
> +                return (value == 0) ? i18n("Instantly")
> +                                    : i18np("After 1 second", "After %1 seconds", value)
> +            }

Same deal for the singular case here; use `%1` instead of `1`

> RulesEditor.qml:137
> +
> +            delegate: Kirigami.AbstractListItem {
> +                id: propertyDelegate

always manually set width to the list view's width on delegates (e.g. `width: overlayModel.width`)

> RulesEditor.qml:148
> +                        Layout.preferredWidth: Kirigami.Units.iconSizes.smallMedium
> +                        Layout.leftMargin: Kirigami.Units.smallSpacing
> +                    }

due to a Qt bug, left and right margins are not reversed in RTL mode. So whenever you set one of those, yo need to explicitly handle the RTL case like this:

  Layout.leftMargin: !LayoutMirroring.enabled? Kirigami.Units.smallSpacing : 0
  Layout.rightMargin: LayoutMirroring.enabled? Kirigami.Units.smallSpacing : 0

(same for all other instances of using left and right margins on an item in a layout)

> RulesEditor.qml:173
> +                    }
> +                    QQC2.ToolButton {
> +                        icon.name: (model.enabled) ? "dialog-ok-apply" : "list-add-symbolic"

Needs to be vertically centered, or else this happens: F8245478: Screenshot_20200418_111722.png <https://phabricator.kde.org/F8245478>

(probably do the same for other items in this rowlayout as right now their being vertically centered is by chance rather than done explicitly)

> RulesEditor.qml:175
> +                        icon.name: (model.enabled) ? "dialog-ok-apply" : "list-add-symbolic"
> +                        opacity: propertyDelegate.hovered ? 1 : 0
> +                        onClicked: propertyDelegate.clicked()

Is there a reason to use `opacity` rather than `visible` here?

> RulesEditor.qml:194
> +            if (sheetOpen) {
> +                searchField.focus = true; // FIXME: It doesn't set the focus to the search field
> +            } else {

This doesn't do what you think it does.

`searchField.focus` doesn't mean, "focus the search field", it means "allow the search field to *become* focused. The property is terribly mis-named. :(

You probably want to set `focus: true` on the search field itself, and do `searchField.forceActiveFocus()` here.

Then again I would imagine that the search field internally has `focus: true` set so setting it again here may be unnecessary.

> iasensio wrote in RulesEditor.qml:93-96
> The spinbox is not editable, I would just copy the default implementation.
> It's also required in this case?

That's correct; it's not required unless the spinbox is editable.

> RulesList.qml:169
> +                        case Qt.Key_Enter:
> +                        case Qt.Key_Return:
> +                            ruleBookItem.focus = true;

Could we handle the tab key here too?

REPOSITORY
  R108 KWin

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

To: iasensio, #plasma, #kwin, #vdg
Cc: ngraham, davidedmundson, hchain, broulik, zzag, kwin, dmenig, manueljlin, Orage, cacarry, LeGast00n, The-Feren-OS-Dev, cblack, konkinartem, ian, jguidon, Ghost6, jraleigh, zachus, fbampaloukas, squeakypancakes, alexde, IohannesPetros, GB_2, mkulinski, trickyricky26, ragreen, jackyalcine, iodelay, crozbo, ndavis, bwowk, ZrenBot, firef, alexeymin, skadinna, himcesjf, lesliezhai, ali-mohamed, hardening, romangg, jensreuterberg, aaronhoneycutt, abetts, sebas, apol, ahiemstra, mbohlender, mart
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kwin/attachments/20200418/fa6154c9/attachment-0001.html>