D28152: [RFC] KWinRules KCM Redesign

[Kai Uwe Broulik]

broulik added a comment.


  First of all I must say, I really like the UI. I did not expect that something as complex and old as KWin's rules settings could be turned into such a beauty ;)
  
  UI Comments:
  
  - Do rules have a certain priority order, I don't really see why one would need to re-arrange them? Though the old KCM also had that, so I guess there's a reason to it
  - I think the list view should have an edit button? I hovered an item and instinctively clicked on the "export" button, thinking it would edit the rule, but one can just click the entire delegate. I found this confusing.
  - I think the rule name should be editable in the list rather than being a "Description" field on the window matching section. Took me a while to figure out how to rename a rule
  - Generally we use regular buttons rather than tool buttons below the view for "New"
  - I think the search field should also search rule values, so I can e.g. type the title of the window and end up with the window title rule
  - The "detect window properties" button is somewhat giant
  - Not sure about the use of icons for the window types, perhaps a `ComboBox` with multi selection is better?
  - We generally don't use switches in UI
  
  (Sorry I only came halfway through with code review now)

INLINE COMMENTS

> kcmrules.cpp:72
> +KCMKWinRules::~KCMKWinRules() {
> +    delete m_ruleBook;
> +}

You don't need to `delete` this manually. You pass `this` as `parent` in the constructor and Qt has this parent-child releationship where a parent brutally murders its children on desturction.

> kcmrules.cpp:90
> +    const auto rules = m_ruleBook->rules();
> +    m_ruleList = QList<Rules *>::fromVector(rules);
> +

Why not keep it a `QVector`? We generally try to move away from `QList`

> kcmrules.cpp:140
> +        delete(m_ruleList[m_editingIndex]);
> +        m_ruleList[m_editingIndex] = m_rulesModel->exportToRules();
> +    }

Could this be done in-memory without a temp file?

> kcmrules.cpp:172
> +
> +    m_ruleList.append(new Rules());
> +    updateState();

Looks like you're leaking the rules? Perhaps do a

  qDeleteAll(m_ruleList)

in the destructor

> kcmrules.cpp:183
> +{
> +    const int lastIndex = m_ruleList.count() - 1;
> +    if (index < 0 || index > lastIndex) {

I think doing

  if (index < 0 || index >= m_ruleList.count()) {
      return;
  }

is clear enough

> kcmrules.cpp:246
> +{
> +    QString path = QFileDialog::getOpenFileName(nullptr,
> +                                                i18n("Import Rules"),

Don't use this in conjunction with QML! This creates a nested event loop which will cause all sorts of hard to find crashes. You can probably just do the file selection on the QML side, like we do in most other settings modules:

  import QtQuick.Dialogs 1.0 as QtDialogs
  
      Loader {
          id: fileDialogLoader
          active: false
          sourceComponent: QtDialogs.FileDialog {
              title: i18n("Import Rule")
              folder: shortcuts.home
              nameFilters: [ i18n("KWin Rules (*.kwinrule)") ]
              Component.onCompleted: open()
              onAccepted: {
                  kcm.importRuleFromFile(fileUrls[0])
                  fileDialogLoader.active = false
              }
              onRejected: fileDialogLoader.active = false
          }
      }

This is copied from another KCM and adjusted. It's behind a `Loader` as creation of this item is pretty heavy, so we want to just do it on demand. And on the import button you just do `onClicked: fileDialogLoader.active = true`. Perhaps you could share it for open and save, or just use two separate ones. I'm sure you'll figure it out :)

> kcmrules.h:39
> +
> +    Q_PROPERTY(QStringList ruleBookModel READ ruleBookModel NOTIFY ruleBookModelChanged)
> +    Q_PROPERTY(int editingIndex READ editingIndex NOTIFY editingIndexChanged)

I would prefer if this was a proper model (or `QStandardItemModel`) so we don't end up having the list destroy and recreate its entire state when we change it. This way yo could also move items and it just swaps the delegates without redoing the entire list.
When you signal changes to a `QStringList` bound to a `model` the view resets completely.

(Also, by the name of the variable it wasn't clear to me what it actually is about.)

> kcmrules.h:45
> +    explicit KCMKWinRules(QObject *parent, const QVariantList &arguments);
> +    ~KCMKWinRules();
> +

`override`

> kcmrules.h:51
> +
> +    QStringList ruleBookModel() const;
> +

It's a property already, doesn't need to be a `slot`

> kcmrules.h:56
> +
> +    void newRule();
> +    void removeRule(int index);

`createRule`?
also perhaps make it `Q_INVOKABLE` rather than a slot. At least personally I use that to indicate stuff I want explicitly called from QML

> kcmrules.h:79
> +
> +    int m_editingIndex = -1;
> +};

I think a `QPersistentModelIndex` might be better suited for this, so it automatically adjusts as you add, remove, rearrange rules. You still probably need to signal the changes for it manually, so not sure. Just food for thought.

> optionsmodel.cpp:50
> +{
> +    if (!index.isValid() || index.column() != 0 || index.row() < 0 || index.row() >= int(m_data.size())) {
> +        return QVariant();

There's `checkIndex()` :)

> OptionsComboBox.qml:41
> +
> +    onActivated: (index) => {
> +        currentValue = values[index];

Kudos for using `onActivated` rather than `onCurrentIndexChanged`! ;)

However, you can probably make this a binding

  readonly property var currentValue: values[index]

or perhaps a `string` property, even

> RuleItemDelegate.qml:28
> +    id: ruleDelegate
> +    focus: true
> +

Needed?

> RuleItemDelegate.qml:30
> +
> +    property var ruleEnabled: model.enabled
> +

`property bool`

> RuleItemDelegate.qml:35
> +    RowLayout {
> +        spacing: Kirigami.Units.smallSpacing
> +        anchors {

`RowLayout` has a default spacing of `5` I think which we just leave untouched usually

> RuleItemDelegate.qml:63
> +            text: model.name
> +            Layout.minimumWidth: 12 * Kirigami.Units.gridUnit
> +        }

Perhaps set `elide: Text.ElideRight`

> RuleItemDelegate.qml:67
> +        Item {
> +            Layout.fillWidth: true
> +        }

I think you better set that on the `Label` and remove this item, so the description can always span the full width?

> RuleItemDelegate.qml:83
> +            onActivated: {
> +                print ("Policy changed for rule " + key + ": " + currentValue);
> +                policy = currentValue;

Remove

> RulesEditor.qml:36
> +        id: filterBar
> +        Kirigami.ActionTextField {
> +            id: searchField

There's now a `Kirigami.SearchField`

> RulesEditor.qml:61
> +            icon.name: checked ? 'view-visible' : 'view-hidden'
> +            text: i18n("Show all rules")
> +            checkable: true

I think this needs to use title capitalization: "Show All Rules"

> RulesEditor.qml:66
> +            onToggled: {
> +                rulesModel.filter.showAll = checked;
> +            }

You could also us a `Binding` for that:

  Binding {
      target: rulesModel.filter
      property: "showAll"
      value: showAllButton.checked
  }

> RulesEditor.qml:102
> +            textFromValue: (value, locale) => {
> +                return value == 0 ? i18n("instantly")
> +                                  : i18np("after 1 second", "after %1 seconds", value)

Capitalize

> RulesEditor.qml:115
> +            visible: rulesModel.showWarning
> +            text: i18n("You have specified the window class as unimportant.\n" +
> +                    "This means the settings will possibly apply to windows from all " +

You can't do word puzzles with `i18n` like this. It needs to be one consecuetive string.

> RulesList.qml:42
> +        id: ruleBookView
> +        clip: true
> +        focus: true

I think it does that for you already?

> ruleitem.cpp:36
> +    , m_enabled(false)
> +    , m_flags(0)
> +    {};

We usually prefer initialization in the class definition

> ruleitem.cpp:48
> +    QString m_description;
> +    uint m_flags;
> +

There's `QFlag` you could use

> ruleitem.h:32
> +
> +class RuleItemPrivate;
> +

Since none of these classes are exported, I think you can save yourself the trouble of using private data classes

> ruleitem.h:39
> +public:
> +    enum Type {
> +        Undefined,

I /think/ if you based your class on `QMetaObject::Type` you can generalize it a lot without putting `switch` statements for your types in it? However, there's also `Coordinate` and `Shortcut`, so maybe not.

> ruleitem.h:108
> +    RuleItemPrivate *d;
> +    RulePolicy *p;
> +    OptionsModel *o;

Generally we want to have speaking names, i.e. name this `policy`.
`d` pointer is an exception.

> rulesdialog.cpp:34
> +
> +RulesDialog::RulesDialog(QWidget* parent, const char* name)
> +    : QDialog(parent)

Where is this class being used from?

> rulesmodel.cpp:57
> +{
> +    delete m_filterModel;
> +    delete m_activities;

No need to delete those

> rulesmodel.cpp:149
> +
> +    emit dataChanged(index, index, QVector<int>{role});
> +

Check if you changed the value before emitting this for optimization

> rulesmodel.cpp:177
> +{
> +    return m_rules[key];
> +}

Avoid `operator[]` which may detach and insert new items into the list unexpectedly. (It can't here because this method is `const` but generally we try to avoid it)

> rulesmodel.cpp:316
> +{
> +    m_ruleList.clear();
> +

`qDeleteAll(m_ruleList)` before so you don't leak those rules?

> rulesmodel.cpp:323
> +                         QStringLiteral("entry-edit")));
> +    m_rules["description"]->setFlags(RuleItem::AlwaysEnabled | RuleItem::AffectsDescription);
> +

Just use the return value of `addRule` instead of looking it up in the dict again

> rulesmodel.cpp:773
> +
> +bool RulesFilterModel::filterAcceptsRow(int source_row, const QModelIndex &source_parent) const
> +{

There's now a `KSortFilterProxyModel` in `KItemModels` you can use from QML for simple filtering like this. Just a suggestion.

> rulesmodel.cpp:775
> +{
> +    if (m_isSearching) {
> +        return QSortFilterProxyModel::filterAcceptsRow(source_row, source_parent);

I probably would have stored the search string in a member and done the filtering fully custom here rather than relying on `filterFixedString`

> rulesmodel.cpp:783
> +    const QModelIndex index = sourceModel()->index(source_row, 0);
> +    return sourceModel()->data(index, RulesModel::EnabledRole).toBool();
> +}

You can also do `QModelIndex::data()`

> rulesmodel.cpp:800
> +{
> +    m_showAll = showAll;
> +    invalidateFilter();

Generally we check if a property has actually changed before emitting a change:

  if (m_showAll == showAll) {
      return;
  }
  ...

> rulesmodel.h:47
> +    Q_PROPERTY(QString description READ description NOTIFY descriptionChanged)
> +    Q_PROPERTY(bool showWarning READ isWarningShown NOTIFY showWarningChanged)
> +    Q_PROPERTY(RulesFilterModel *filter MEMBER m_filterModel CONSTANT)

I kinda think this should be a `QString` property and have the info filled on the C++ side. But since it's just used for that one case, maybe name it `showWindowClassWarning` or something?

REPOSITORY
  R108 KWin

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

To: iasensio, #plasma, #kwin, #vdg
Cc: 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, ngraham, 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/20200320/4acbaa86/attachment-0001.html>