D28744: Rewrite of the global shortcuts kcm

[Nathaniel Graham]

ngraham added a comment.


  This is excellent work. A radical improvement over the existing KCM. In addition to the inline comments, here are some more general ones:
  
  - When a shortcut is to launch an app, and that shortcut's name would be identical to the app name, maybe we should automatically change it to "Launch [app name]. Otherwise you get this following: F8234080: Screenshot_20200413_094908.png <https://phabricator.kde.org/F8234080>
  - I'm seeing some duplicate entries here, and some with missing icons that aren't missing in the current version: F8234212: Screenshot_20200413_114058.png <https://phabricator.kde.org/F8234212>
  - I notice that `ShortcutActionDelegate.qml` looks suspiciously like the `ExpandableListItem` item I just added to PlasmaExtras. Now I'm wondering if it should be in Kirigami instead...

INLINE COMMENTS

> kcm_keys.desktop:2
>  [Desktop Entry]
> -Exec=kcmshell5 keys
> +Exec=kcmshell5 kcm_keys
>  Icon=preferences-desktop-keyboard-shortcut

If we're changing this and breaking compatibility with this old name, we might as well improve the string as well (e.g `kcm_globalshortcuts`). Alternatively I guess we could not change it.

> ShortcutActionDelegate.qml:50
> +            width: shortcutsList.width
> +            onClicked: shortcutsList.currentIndex = index
> +            contentItem: RowLayout {

should should display the pointing hand cursor when hovered so people know the whole thing will expand when clicked as with the ExpandableListItem.

> ShortcutActionDelegate.qml:56
> +                    Layout.fillWidth: true
> +                    text: {
> +                        if (model.activeShortcuts.length != 0) {

The way you're (ab)using `xi18n()` to change the text weight of different parts of the string, it feels like this text would prefer to be two labels. This would facilitate right-aligning the right-most part too.

> ShortcutActionDelegate.qml:93
> +                    QQC2.ToolButton {
> +                        Layout.alignment: Qt.AlignRight
> +                        icon.name: "arrow-up"

Align top-right so it doesn't jump around vertically when the list item is collapsed

> ShortcutActionDelegate.qml:144
> +                                QQC2.Button {
> +                                    icon.name: "list-remove"
> +                                    onClicked: kcm.shortcutsModel.disableShortcut(originalIndex, modelData)

Since this actually removes a shortcut, I'd probably make use the trashcan icon (`edit-delete`)

> ShortcutActionDelegate.qml:147
> +                                    QQC2.ToolTip {
> +                                        text: i18n("Disable this shortcut")
> +                                    }

This isn't accurate; it actually deletes the shortcut

> main.qml:31
> +    implicitWidth: 1000
> +    implicitHeight: 400
> +    enabled: kcm.lastError == ""

The ratio between these is kind of unpleasant; I would recommend 800x600

> main.qml:62
> +            visible: exportActive
> +            text: i18n("Select the components that should be included in the exported scheme")
> +            type: Kirigami.MessageType.Information

Maybe `Select the components below...` for extra clarity?

> main.qml:65
> +            showCloseButton: true
> +            actions: [
> +                Kirigami.Action {

Probably needs a "select all" action to make exporting everything not be incredibly frustrating

> main.qml:81
> +            Layout.fillWidth: true
> +            onAccepted: kcm.filteredModel.filter = text
> +        }

Would be nice to filter immediately as the user types rather than having to hit the return key

> main.qml:126
> +                onRootIndexChanged: shortcutsList.currentIndex = -1
> +                ListView {
> +                    clip:true

Need to do one of the following:

- Make this view never empty because something in the left view is always selected
- Add a standard-looking placeholder message that appears in the center of this view when nothing is selected that says something like "Select an item from the list to view its shortcuts here"

> metadata.desktop:4
> +
> +Comment=Global Keyboard Shortcuts
> +

Needs to be a sentence starting with an action verb (e.g. "Configure global keyboard shortcuts")

REPOSITORY
  R119 Plasma Desktop

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

To: davidre, #vdg, #plasma
Cc: ngraham, iasensio, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20200413/bedf3a3e/attachment-0001.html>