D28744: Rewrite of the global shortcuts kcm

Kai Uwe Broulik noreply at phabricator.kde.org
Tue Apr 14 09:41:02 BST 2020


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("kde at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20200414/2897016e/attachment-0001.html>


More information about the Plasma-devel mailing list