broulik added a comment.
Very nice! I really like the default shortcuts with checkboxes with additional ones to the right. INLINE COMMENTS > filteredmodel.cpp:40 > + const QModelIndex index = sourceModel()->index(source_row, 0, > source_parent); > + bool displayMatches = > index.data(Qt::DisplayRole).toString().contains(m_filter, > Qt::CaseInsensitive); > + if (!source_parent.isValid() || displayMatches) { `const bool` > filteredmodel.cpp:41 > + bool displayMatches = > index.data(Qt::DisplayRole).toString().contains(m_filter, > Qt::CaseInsensitive); > + if (!source_parent.isValid() || displayMatches) { > + return displayMatches; Why this `!source_parent.isValid()` check? > filteredmodel.cpp:45 > + > + if (index.parent().data(Qt::DisplayRole).toString().contains(m_filter, > Qt::CaseInsensitive)) { > + return true; Shouldn't recursive filtering take care of this? > filteredmodel.cpp:50 > + const auto &defaultShortcuts = > index.data(ShortcutsModel::DefaultShortcutsRole); > + for (const auto& shortcut : > defaultShortcuts.value<QSet<QKeySequence>>()) { > + if (shortcut.toString(QKeySequence::NativeText).contains(m_filter, > Qt::CaseInsensitive)) { Put the `.value<QSet<...>>` in the line above outside the `for` > filteredmodel.h:26 > + > +class FilteredShortcutsModel : public QSortFilterProxyModel { > + Q_OBJECT Coding style, `{` on next line > filteredmodel.h:32 > +public: > + FilteredShortcutsModel(QObject *parent); > + `explicit` > filteredmodel.h:34 > + > + bool filterAcceptsRow(int source_row, const QModelIndex & source_parent) > const override; > + Coding style: `const QModelIndex &soure_parent` > kcm_keys.cpp:51 > + KAboutData *about = new KAboutData(QStringLiteral("kcm_keys"), > i18n("Global Shortcuts"), > + QStringLiteral("1.0"), QString(), KAboutLicense::GPL); > + about->addAuthor(i18n("David Redondo"), QString(), > QStringLiteral("k...@david-redondo.de")); I guess this can be 2.0 at this point :) > kcm_keys.cpp:106 > +{ > + return m_lastError; > +} Not a huge fan of this `lastError` bookkeeping in the KCM > kcm_keys.cpp:153 > +{ > + auto dialog = new KOpenWithDialog; > + dialog->hideRunInTerminal(); Set a transient parent on the dialog like so: void KCMKeys::addApplication(QQuickItem *ctx) { auto *dialog = new KOpenWithDialog; if (ctx && ctx->window()) { dialog->winId(); // so it creates windowHandle dialog->windowHandle()->setTransientParent(QQuickRenderControl::renderWindowFor(ctx->window())); dialog->setWindowModality(Qt::WindowModal); } ... } Then call in QML like `onClicked: kcm.addApplication(this)` Also, you might want to set `dialog->setAttribute(Qt::WA_DeleteOnClose);` so you don't need to `deleteLater()` yourself. (I believe the `QQuickRenderControl` use may not be necessary when in the same process but I recall it behaving funky in `QQuickWidget` in some cases on Wayland otherwise) > kcm_keys.cpp:159 > + const KService::Ptr service = dialog->service(); > + QString desktopFileName = service->entryPath().split('/').last(); > + if (m_shortcutsModel->match(m_shortcutsModel->index(0, 0), > ShortcutsModel::ComponentRole, desktopFileName).isEmpty()) { Perhaps use `KService::desktopEntryName` > kcm_keys.h:51 > + Q_INVOKABLE void loadScheme(const QUrl &url); > + Q_INVOKABLE QVariantList defaultSchemes(); > + `const` > kcm_keys.h:54 > + Q_INVOKABLE void addApplication(); > + Q_INVOKABLE QString keySequenceToString(const QKeySequence &keySequence); > + I presume `QKeySequence` is opaque to QML? Also, `const` > ShortcutActionDelegate.qml:30 > + property int oldHeight: height > + sourceComponent: ListView.isCurrentItem ? editRepresentation : > compactRepresentation > + ListView.onIsCurrentItemChanged: { I find the transition between editing and display state somewhat jarring > ShortcutActionDelegate.qml:58 > + if (model.activeShortcuts.length != 0) { > + return model.display + ": " + > model.activeShortcuts.map(s => kcm.keySequenceToString(s)).join(", ") > + } else { Please use `i18n` > main.qml:33 > + enabled: kcm.lastError == "" > + property bool exportActive: exportInfo.visible > + ColumnLayout{ This appears to loop? `exportInfo`'s `visible` is bound to this > main.qml:34 > + property bool exportActive: exportInfo.visible > + ColumnLayout{ > + // otherwise we get scrollbars Coding style: `ColumnLayout {` > main.qml:36 > + // otherwise we get scrollbars > + height: root.availableHeight - Kirigami.Units.smallSpacing > + width: root.availableWidth Huh? > main.qml:40 > + Layout.fillWidth: true > + visible: kcm.lastError != "" > + text: kcm.lastError Coding style, also `!==` > main.qml:53 > + enabled: exportWarning.visible > + function onNeedsSaveChanged () { > + exportWarning.visible = kcm.needsSave What Qt version was that changed, 5.14? Also why not just bind? > main.qml:87 > + Component.onCompleted: background.visible = true > + Layout.preferredWidth: 300 > + Layout.fillHeight:true `Kirigami.Units.gridUnit` and/or maybe proportional to the window width? > main.qml:94 > + delegate: Kirigami.AbstractListItem { > + RowLayout { > + Kirigami.Icon { Is `contentItem` the default property? > main.qml:102 > + Layout.fillWidth: true > + text: model.display > + } Make sure to replicate the color behavior on hover and when selected, right now text stays black with the blue selection background behind it > main.qml:114 > + section.delegate: Kirigami.ListSectionHeader { > + label: section > + } Perhaps you could put a checkbox here so one check all items in a section > main.qml:141 > + icon.name: "list-add" > + text: i18n("Add application...") > + onClicked: { Capitalize: "Add Application..." How do I remove applications again, though? > main.qml:157 > + icon.name: "document-export" > + text:i18n("Export Scheme...") > + onClicked: { I kinda think this button should turn into a "Cancel Export" button when exporting? Not sure that closing the message widget is obvious enough to the user? > main.qml:160 > + if (kcm.needsSave) { > + exportWarning.visible = true > + } else { You're doing a `Connections` above already. > main.qml:163 > + search.text = "" > + kcm.filteredModel.filter = "" > + exportInfo.visible = true This should just be bound to the `search.text` and work automatically > main.qml:164 > + kcm.filteredModel.filter = "" > + exportInfo.visible = true > + } You seem to love being overly explicit with this? :P You have a binding above already... Perhaps run the application with `QT_LOGGING_RULES="qt.qml.binding.removal.info=true"` to clean those up > main.qml:224 > + } > + importSheet.close() > + } Found it a bit odd that the overlaysheet suddenly closed as a result. I would have expected clicking "Select File", choosing one, and then it being added to and selected in the combox list and then confirming this by clicking "Import" > main.qml:30 > + id: root > + implicitWidth: 1000 > + implicitHeight: 400 `Kirigami.Units.gridUnit` > shortcutsmodel.cpp:38 > +{ > + QStringList actionId{"", "", "", ""}; > + actionId[KGlobalAccel::ComponentUnique] = componentUnique; Hmm... how about QStringList actionId; actionId.reserve(4); > shortcutsmodel.cpp:60 > + m_components.clear(); > + QDBusReply<QList<QStringList>> componentsReply = > m_globalAccelInterface->allMainComponents(); > + if (!componentsReply.isValid()) { Do this asynchronously with `QDBusPendingCallWatcher`? Not sure if it really matters here > shortcutsmodel.cpp:66 > + const auto components = componentsReply.value(); > + for(const auto &component : components) { > + m_components.push_back(loadComponent(component)); Coding style, space after for `for (const auto &...)` > shortcutsmodel.cpp:70 > + std::sort(m_components.begin(), m_components.end(), [](const Component > &c1, const Component &c2){ > + return c1.type != c2.type ? c1.type < c2.type : c1.friendlyName < > c2.friendlyName; > + }); for comparing `friendlyName` use `QCollator`? > shortcutsmodel.cpp:103 > + shortcut.friendlyName = actionFriendly; > + QDBusReply<QList<int>> shortcutsReply = > m_globalAccelInterface->shortcut(action); > + if (!shortcutsReply.isValid()) { I kinda feel this should be done on demand or asynchronously as it causes a significant load time of the KCM > shortcutsmodel.cpp:130 > + std::sort(c.shortcuts.begin(), c.shortcuts.end(), [] (const Shortcut > &s1, const Shortcut &s2) { > + return s1.friendlyName < s2.friendlyName; > + }); Use `QCollator` > shortcutsmodel.cpp:140 > + if (shortcut.initialShortcuts != shortcut.activeShortcuts) { > + QStringList actionId = buildActionId(component.uniqueName, > component.friendlyName, > + shortcut.uniqueName, shortcut.friendlyName); `const` > shortcutsmodel.cpp:194 > +{ > + if (row < 0 ) { > + return QModelIndex(); Also check `if (column != 0) {` (Also, coding style) > shortcutsmodel.cpp:232 > +{ > + if (!index.isValid()) { > + return QVariant(); `checkIndex()`? > shortcutsmodel.cpp:251 > + } > + default: > + return QVariant(); No `default` case please in case we add new roles. Just `return` outside the `switch`, and below > shortcutsmodel.cpp:260 > + case Qt::DecorationRole: > + return QIcon::fromTheme(component.icon); > + case SectionRole: Do we want a `QIcon` here? I guess just returning an `iconName` and using a `Kirigami.Icon` in the UI should suffice > shortcutsmodel.cpp:274 > +{ > + if (!checkIndex(index) || index.parent().isValid() || role != > CheckedRole) { > + return false; Makes me wonder if `CheckedRole` should be the `Qt::EditableRole` > shortcutsmodel.cpp:298 > +{ > + if (!checkIndex(index) || !index.parent().isValid()) { > + return; You can pass also `ParentIsInvalid` as flag to `checkIndex` > shortcutsmodel.cpp:403 > + } > + emit this->error(i18n("Error while communicating with the global > shortcuts daemon")); > +} perhaps "service" instead of "daemon"? > shortcutsmodel.h:54 > + > +class ShortcutsModel : public QAbstractItemModel { > + Q_OBJECT Coding style, `{` on new line > shortcutsmodel.h:58 > +public: > + friend FilteredShortcutsModel; > + enum Roles { It doesnt appear to use any private members? > shortcutsmodel.h:83 > + void save(); > + bool needsSave(); > + bool isDefault(); `const` and below > shortcutsmodel.h:95 > +Q_SIGNALS: > + void error(QString); > + `const QString &`, also perhaps `errorOccurred`? REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D28744 To: davidre, #vdg, #plasma Cc: broulik, davidedmundson, nicolasfella, 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