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
