KWinRules KCM Redesign
Open, Needs TriagePublic

Description

I've been working on a replacement for the KWin Rules KCM based on Kirigami Scrollviews.

This is still very WIP to be merged, but it's now in a state where the main models are working fine, altough some features are still missing, and some feedback would be highly appreciated. The code can be found on the working branch iasensio/kcm_kwinrules_xt and RFC review D28152

The current KCM, dating 2004, uses a custom UI widget with ad-hoc code for each rule property (helped by C-style macros) to interact with the Rules object being edited.

The proposed code implements a new main model (RulesModel) in which rule properties are a list of RuleItems, which can be enabled/disabled and configured. The code required to set-up a new rule property is now quite simpler and descriptive, by just adding a new item to the model. I think this might be handy if, for instance, KWin on Wayland may support different rule properties than in X11.

I couldn't find any mockups on this specific KCM, so I implemented the UI part to my best judgement, mimicking the old kcm when in doubt. VDG opinion is here very welcome.

Now, presenting the new KCM, there are two distinct parts:

  • The rules list: It uses a Kirigami's ScrollViewKCM view that shows the saved kwin rules and allow to create, modify, and edit them.

BEFORE:


AFTER:

  • The rules editor: It is also implemented as a ScrollViewKCM feeding from RulesModel. When editing a rule from the KCM, the rules editor appears as a second page. Instead of tabs, the different categories are now mapped into different headers within the list view.

BEFORE:


AFTER:

By default, only selected "properties" of the rule are shown, presenting a more compact view for casual simple rules. Still, there are lots of information to show and configure, so it's difficult that the UI does not feel "crowded"

A challenging question is that the rules editor has a second entry-point apart from the KCM itself. In the window menu -> Edit Specific Window/App Properties. Currently I have kept this part the same, inserting a QQuickView widget inside the QDialog, to keep the interface with the kwinrules code (which calls this dialog) unchanged.

As you can see, my to-do list is still quite big, but I hope to check them out soon with you help.

  • BUGS
  • New Rule doesn't save the description for the first time
  • Improve KConfig integration
    • Apply/Saving is buggy [X ] Reset/Cancel to previous state without saving
  • PENDING FEATURES
  • Implement rules Import and Export
  • Implement filter model: search and show all
  • Detect Window Properties
  • INTEGRATION WITH KConfigXT (D27682)
  • Rebase to KConfigXT, now on master branch
  • Re-implement features on the new interface
  • Directly store a list of RuleSettings instead of Rules objects
  • Explore integration between KConfigXT and ManagedConfigModule(D24821)
  • Tighter integration between RuleItem and RuleSettings' items (some features overlap)
  • Initialize rule items using the RuleSettings schema (future)
  • CODE
  • Expose enums in the plugin
  • COMMENTS/APIDOX!!!!
  • Unit Tests
  • CMakeList.txt clean-up
  • Use the current module name (kcm_kwinrules)
  • Remove old KCM code
  • Use RuleItem as a template, instead of custom enum Type. (might it require a distinct item to edit in some cases?)
  • Fix QML binding loops
    • InlineMessage.qml:241:13: QML Label: Binding loop detected for property "verticalAlignment"
    • KeySelector (keySequence) (D27795)
    • CoordinateEditor (RowLayout width for items)
    • ScrollViewKCM.qml:73:13: QML Control: Binding loop detected for property "implicitHeight"
  • Remove dependency on QDialog in kwin_rules_dialog executable
  • Use a full model (RuleBookModel) instead of passing a QStringList
    • Use QPersistentModelIndex to store current rule being edited
  • UI / USABILITY
  • VDG suggestions & improvements
  • UI to suggest/select/reject each detected property
  • Add translation domain to qml files (how?)
  • Accessibility / Keyboard navigation
  • Support Right-To-Left layout
  • Better icons
    • Specific icons for rule properties
    • Icons for policies (exact match, regexp.. a la Kate search bar)
  • Better layout for RuleItemDelegate (min/max/preferred size, alignment...)
  • Flat comboboxes (QQC2 style problem?)
  • EXTEND CURRENT BEHAVIOR
    • Use interactive window selection for "Detect window properties" in kcm rules (T5217)
    • Use only one way to disable rules (Is it necessary to have "Do not affect" option?)
    • Separate "window matching" group from the rest ("which windows does it apply" vs "what to do with them")
    • Add/Remove metaphor instead of select/deselect properties
    • Change/expand "window class" to an application selection
    • Instant apply for "apply now / force temporarily"
iasensio created this task.Feb 21 2020, 7:22 PM
ngraham added a subscriber: ngraham.

Super awesome!

I don't know if it would be best at this stage to create a RFC code review or upload it to a new <user> branch.

Either works

iasensio updated the task description. (Show Details)Feb 22 2020, 12:57 AM
iasensio updated the task description. (Show Details)Feb 23 2020, 2:15 AM

Let me know if you want me to look at any of these specific tasks.

felixernst added a subscriber: felixernst.EditedFeb 23 2020, 12:40 PM

This looks much better! Only showing preferences that are part of the rule by default for already existing rules is a good idea too imo.

The biggest UI issue I see with window rules is not introduced by this change but maybe this can now easily be solved:
I think the biggest problem users will have with figuring out how to use window rules to change a specific behaviour of a window is understanding the window matching part. I think for non-technical folks it is unintuitive that the question "What does this rule apply to?" is part of the rule itself. This problem is made more grave by the string "window matching" that doesn't necessarily convey that this decides what the rule is applied to. Overall the window matching section is quite hard to understand because terminologies like "window class", "window types" and "window role" aren't easily understood.

Here is a mockup that contains some ideas to solve this:


*Edit: It should read "Apply rule to windows matching all of these conditions:". Ignore the non-existent anti-aliasing on text.

  • Separated "window matching" from the rest.
  • Changed "window class" to an application selection
  • Only display conditions that are in use: I am a bit unhappy that I solved this differently than with the "Show all rules"-button. I went with the approach of adding/removing conditions because I thought having a checkbox to enable/disable conditions wasn't easy enough to understand. I especially thought this would be a problem when a new rule is created and the user is faced with five unchecked conditions.

I really like your mock-up! Some of the ideas also crossed my mind when I first approached the KCM but I couldn't visualize them in a nice way like you did. I'm adding you're ideas to the task to-do list, although maybe they'll have to wait (with some others) until we get a first functional patch, and be added in the "polish" phase.

  • Separated "window matching" from the rest.

Technically, I think is not too hard to achieve using the same backend model, but just having two views with different proxy filters, one for "Window Matching" properties and another one for the effects.

  • Changed "window class" to an application selection

I have more doubts about this one, since I don't know easy or feasible is to map applications to its wmclass string, but anyway a must addition.

  • Only display conditions that are in use: I am a bit unhappy that I solved this differently than with the "Show all rules"-button. I went with the approach of adding/removing conditions because I thought having a checkbox to enable/disable conditions wasn't easy enough to understand. I especially thought this would be a problem when a new rule is created and the user is faced with five unchecked conditions.

I find your approach (Add/Remove) better from an user point of view, although the problem I see is how to present the possible effects that can be added to the user, that is, what happens when you press "+ Add.... Have you something in mind?

Let me know if you want me to look at any of these specific tasks.

Thanks! I'm currently struggling a bit with the KConfig stuff and how to undo the changes to a previous state (before Apply).
I think I don't understand how it really works, and maybe my approach here is not the best one. That would be my first milestone for considering it at minimal working state and submit a RFC patch.

iasensio updated the task description. (Show Details)Feb 24 2020, 9:29 PM
iasensio updated the task description. (Show Details)Feb 24 2020, 9:44 PM

I'm adding you're ideas to the task to-do list

I am happy to hear that!

I don't know easy or feasible is to map applications to its wmclass string

I was wondering about that. Hopefully there is an easy way. But ye, this is one of the things that can be done any time later I think.

  • Only display conditions that are in use: I am a bit unhappy that I solved this differently than with the "Show all rules"-button. I went with the approach of adding/removing conditions because I thought having a checkbox to enable/disable conditions wasn't easy enough to understand. I especially thought this would be a problem when a new rule is created and the user is faced with five unchecked conditions.

I find your approach (Add/Remove) better from an user point of view, although the problem I see is how to present the possible effects that can be added to the user, that is, what happens when you press "+ Add.... Have you something in mind?

I was thinking about this some more but didn't come up with a way to solve this better than in my mockup. I think it might be fine though. For the "conditions" list the "Add conditions" button would have a dropdown with entries like "Add application condition", "Add window title condition", "Add window type condition", … This way no secondary window is necessary to create a rule. I would guess that most of the time only one or two conditions would be added this way and because there are only a few possible conditions to choose from a dropdown is a great fit here.
For the "rule effects" list the current behaviour has the same benefit of not making a secondary window necessary. The big list of possible "effects" needs to be somewhere and putting it in another window that spawns when clicking an "Add …" button wouldn't make the UI more understandable IMO (although this wouldn't be a bad way to solve this either). The workflow of activating effects in a rule by clicking its checkbox also makes sense IMO (contrary to the "conditions" list).
So yea, although it is a bit unfortunate to have two different ways to add objects to a list in the same window it might still be the best solution for the window rule modification/creation.

iasensio updated the task description. (Show Details)Feb 28 2020, 11:26 PM
iasensio updated the task description. (Show Details)Mar 2 2020, 10:24 PM
hchain added a subscriber: hchain.Mar 3 2020, 1:01 PM

Hi @iasensio, great work on the UI redesign! I currently have a patch https://phabricator.kde.org/D27682 to port the existing kwinrules KCM to KConfig XT. Hopefully this can help bridge existing configuration with your model, as well as import/export.

Hi there! Thanks, I've been watching your patch and currently following it to see how it evolves.

The current models here extract the info directly by directly reading the KConfig groups and properties. I think that having a specific settings object like RuleBookSettings can simplify this, and make it more coherent, but I don't know much about KConfigXT.
I've seen that it generates a pair of methods for each config property (prop()/setProp()) Does it also provide a similar API to KConfig for the properties like readEntry(prop) / writeEntry(prop)? If that is the case, then I think the integration with your patch can be quite straightforward.

I also wonder if the XML schema for KConfigXT can be somehow extended to include extra custom properties. Then, the model could import the rules' properties directly from the schema making it fully flexible. But that is a discussion for the future.

I'm going to try closing the pending features one at a time and then see which is the best path forward.

iasensio updated the task description. (Show Details)Sun, Mar 8, 9:56 PM
ognarb added a subscriber: ognarb.Sun, Mar 8, 11:38 PM

KConfig XT is based on KConfig, and there is an underlying KConfig Object with entries/groups. But the entire point of KConfig XT is not to use them, and use the auto generated accessors instead. The kconfig compiler can also make RuleBookSettings a QObject with Q_PROPERTIES and signals that you can connect to your model.

iasensio updated the task description. (Show Details)Tue, Mar 10, 11:37 PM
iasensio updated the task description. (Show Details)Wed, Mar 11, 11:57 PM
iasensio updated the task description. (Show Details)Sat, Mar 14, 10:35 PM
iasensio updated the task description. (Show Details)Mon, Mar 16, 12:23 AM

Hi @iasensio , I can help with porting your code to KConfig XT . Do you mind if I rebase your branch and make some modifications ?

iasensio added a comment.EditedThu, Mar 19, 1:58 PM

Hi there, sorry, I just read your comment.

Thanks for your help! I have already rebased it locally and started the porting.
Currently, the rule edition and the new and remove methods are working.
I'm now just re-adding the functionallity bit by bit, following by move, import and export.

Editing to add that there are also some small refactoring bits that I wanted to do before, and seem to be easier now.

These evening, after that, I'll upload the new branch, and I will reach you for some help on the following :)

Great!
I was thinking you might want to use pointer to RuleSettings members instead of config item name strings inside of RuleItem. Also maybe it would be worthwhile to make RuleItem a template (I think it doesn't need to be a QObject) instead of your Type enum.

BTW, are you aware of https://phabricator.kde.org/D24821 ? I could change the kcfgc file to make RuleBookSettings a QObject with signals that a ManagedConfigModule could use to handle the load/save/defaults buttons automatically

iasensio updated the task description. (Show Details)Thu, Mar 19, 7:43 PM
iasensio added a comment.EditedThu, Mar 19, 7:56 PM

I must admit that those suggestions kind of overwhelm me and my skills. 😅

I'm thinking of submitting a RFC patch (on the current working version (still polishing some bits), not to be merged right away, but so more eyes can try it easily, both UI and code-wise. The rest of the integration steps and improvements you suggest, and some others can be added later, on the same feature branch.

Otherwise I fear that I'll start losing any sense of progress. I hope that you understand and it sounds sane to you.

iasensio updated the task description. (Show Details)Thu, Mar 19, 11:08 PM
iasensio updated the task description. (Show Details)Sat, Mar 21, 3:46 PM
iasensio updated the task description. (Show Details)Thu, Mar 26, 9:00 PM

Since they tend to get lost on code review, I'm pasting some of @broulik 's pending comments and my answers here. They are about some UI decisions I would like to check with the rest of the VDG:

  • 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

It's implemented now, but I find it a bit quirky due to how TextField behaves, and I might need some hints on that.

  • Generally we use regular buttons rather than tool buttons below the view for "New"

Here I feel that the non-flat buttons can give a more "busy" feeling when close to the KCM buttons, while flat ones make it more clean, but I'm not strong against it:


  • Not sure about the use of icons for the window types, perhaps a ComboBox with multi selection is better?

I've tried it, but I see a problem about how to show the selected items when the combo is closed
Current:


Using a ComboBox:

  • We generally don't use switches in UI

I know. In this case I think a checkbox is out of the question visually and semantically, so an alternative can be the yes/no radio buttons like in the old KCM. I still slightly prefer the switch, but the radios doesn't look that bad either.


I've tried it, but I see a problem about how to show the selected items when the combo is closed

You could combine the two? Use the icons for display, but the popup for selecting them.

Another way of solving this is to display what is selected as the text of the ComboBox like "'Normal Window' selected" or "5 selected". Here is roughly how this can be done: https://stackoverflow.com/questions/47575880/qcombobox-set-title-text-regardless-of-items (if you copy any code from this the file would need to be GPL 3 as part of a CC-BY-SA 4.0 one-way conversion AFAIK)

Here I feel that the non-flat buttons can give a more "busy" feeling when close to the KCM buttons, while flat ones make it more clean, but I'm not strong against it:

I agree but I think going with what is consistent is more important here.

This is also why I agree with broulik that switches shouldn't be used here. Furthermore switches tend to be used to communicate changes that are instantly applied which isn't really the case here.

I've just implemented the display text for the combo box to show the number of items selected (or the corresponding text if only one is selected) as you suggested.

Anyway, I still think that the UX is more annoying and less informative at a glance than using the array of buttons.

About the other smaller questions, you've both convinced me. It's better to get consistent now, and maybe discuss it later on a more global scale.

iasensio updated the task description. (Show Details)Sun, Mar 29, 5:01 PM

Anyway, I still think that the UX is more annoying and less informative at a glance than using the array of buttons.

Well, the problem is that we can't expect users to be able to identify window types by their icons. Maybe if we had icons as recognisable as for the desktop, the normal and the dialog window for all of them it would be alright but even then users might still want to hover over some of the other icons to be sure they selected the right one(s). This is especially difficult because some of the window types can seem similar like Dialog and Utility or Dock, Toolbar, Torn-Off Menu and Standalone Menubar.
There really needs to be a list with the actual names of the window types somewhere IMO. And while I don't think this ComboBox will win any design awards I do think it works well enough. There might still be a better way of doing this though.
I really like how easily the window type condition can be read when only one type is selected. I hope that covers the vast majority of use cases.

iasensio updated the task description. (Show Details)Tue, Mar 31, 9:18 AM
abetts added a subscriber: abetts.Tue, Mar 31, 6:46 PM

Looking at some of the rules, I see that the language seems very high. Surely a user that spends time on this KCM would have some understanding of what they are getting into. I would suggest bringing the language for labels closer to the one used for KCM options, where we make it conversational, inviting action. For example,

Under Window Matching:

Description

Use:

Add description
Enter description

Under

Window Types [All Selected]

Maybe use

Apply to [all selected] window types