KWinRules KCM Redesign
ClosedPublic

Authored by iasensio on Mar 19 2020, 10:49 PM.

Details

Reviewers
ngraham
davidedmundson
zzag
Group Reviewers
Plasma
KWin
VDG
Maniphest Tasks
T7290: Window Rules
Commits
R108:e576412b614c: OptionsComboBox: More accurate TODO Message
R108:a04b40dadbee: KWinRules KCM Redesign
R108:7e3ba978cd60: Simplify OptionsComboBox
R108:4c70102c878c: Edit rule descriptions inline
R108:7a89f9ccaf2b: Fix Export - Correctly set file picker to Save - Use the proper index to…
R108:37c70ad2f626: Disable highlights in editor listview
R108:ffa5d2874738: Update save status when importing
R108:b8228803e7c5: Use a button to edit the rule
R108:d8a1db4a59f9: OptionsModel: use standard role names
R108:be1d7f35f219: Polish rules list drag&drop code
R108:83149504eb19: Use a hint button on empty rules
R108:bc70ca2cf884: Address some comments
R108:28cfcda84ecc: Load values for disabled settings (defaults)
R108:44a3bd376324: Fix position of the drop hint bar
R108:4e8fc74b9681: Use overlay sheet to add rule properties
R108:8c42132bf9b8: Use QVector instead of QList
R108:ddfcd4c754be: Refactor RuleItem
R108:216d775de421: Move the kcm pages management to QML side
R108:8bb6824e3862: Move file picker logic to QML
R108:501d31e8fadc: UI polishing
R108:40933e150e20: Set the proper source model (fixes rules dialog path)
R108:957bdedacfbc: Properly close comboboxes when losing focus
R108:eda55c4664a7: Remove useless file
R108:591c38c4c37b: Remove config file assignation
R108:2986733abaac: Use minimumWidth for percentage label
R108:cdd1b233e5ce: Move `Add` buttons to the bottom-left of lists
R108:3cf3670401ee: Trashcan always visible
R108:f2096fec39b6: Simplify multiple choice combo
R108:70450a019c42: Fix value accepting null QVariant()
R108:0236d4da05f3: Use yes/no radiobuttons instead of switch
R108:4bff5b692ace: Update needsSave status when moving rules
R108:f5259f564c8a: Set default wmclass=ExactMatch (simple)
R108:fabc6993f807: Simplify search field
R108:873c0bb10f63: Use the returned pointer of `addRule()`
R108:11fff83750e3: Almost perfect alignment in RuleItemDelegate
R108:531628e68c7c: Nicer UX when reordering rules
R108:246e4a718a7e: Move filter logic to QML side
R108:8d728b71d956: Implement multiple export
R108:67423ad74e83: Set warning message in C++ side
R108:44b512078989: Remove helper method `updateState()`
R108:0c21ae2769c7: Do not include full QtDbus
R108:bc9704b8071a: Implemet RuleBookModel
R108:593963707b39: Merge branch 'master' (adapt CMake instructions)
R108:90fb25560fb1: Search also in rules' values
R108:ad2286a38936: Push Qt.Quick version to 2.14
R108:4f67bc153d4a: Non-flat buttons for New and Import
R108:a836a9e004c5: Remove unnecesary dependencies
R108:56824de728ef: Merge branch 'master' into iasensio/kcm_kwinrules_xt
R108:9769c49bbd34: Add hints when rules list is empty
R108:f7ec22d3e6fc: Use multiple selection combo for types
R108:a0bbdee04b05: Select items by cliking on its row
R108:498538999dff: Fix alignment between rule delegates
R108:124b6b77e936: Fix RuleBookModel::moveRows() when count > 1
R108:2c8a15615192: Code style: CamelCase
Summary

Replacement KCM to configure kwin rules, using a QML-based UI.

After some work on the task T12729, it is almost feature-par with the previous module, and adapted to the recent move to KConfigXT.

Test Plan


Diff Detail

Repository
R108 KWin
Branch
iasensio/kcm_kwinrules_xt
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 24122
Build 24140: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
  • OptionsComboBox: More accurate TODO Message

This is a huge improvement over the UI of the old one, very nice! However I can't actually get it to work. I target Kate, select "Exact Match" on the window class, and apply some settings, but they don't take effect. Am I doing something wrong? I've restarted KWin after applying this patch.

kcmkwin/kwinrules/package/contents/ui/OptionsComboBox.qml
70

would be nice if clicking anywhere in the row activated that row's checkbox

kcmkwin/kwinrules/package/contents/ui/RuleItemDelegate.qml
27

These listitems shouldn't change their background colors on hover, since nothing happens when they're selected. You can see how I did that in https://cgit.kde.org/plasma-desktop.git/commit/?id=5a5e0bc0880b6b21c60b5f874f7eddc2a7939a48

kcmkwin/kwinrules/package/contents/ui/RulesEditor.qml
44

Perhaps this should be checked when creating a new rule, or else it looks like there aren't any properties you can apply to the window when creating the rule

kcmkwin/kwinrules/package/contents/ui/RulesList.qml
55

Add a centered placeholder message using a disabled level 3 Heading in here when there are no rules

104

Disable this when there are no rules

116

All button text needs to begin with an action verb and end with an ellipsis if more actions are required to complete the action. So this should be "Add New..."

kcmkwin/kwinrules/package/contents/ui/ValueEditor.qml
137

Give this a minimum width, or else the rowlayout gets wider with every digit added to the number and resizes the combobox next to it

kcmkwin/kwinrules/rulesmodel.cpp
216

Maybe "Exact Match" should be the default settting so this message isn't always shown for every new rule you add

iasensio updated this revision to Diff 80240.Apr 15 2020, 10:45 PM
iasensio marked 6 inline comments as done.

Address some comments:

  • Select items by cliking on its row
  • Disable highlights in editor listview
  • Add hints when rules list is empty
  • Use minimumWidth for percentage label
iasensio added inline comments.Apr 16 2020, 6:04 PM
kcmkwin/kwinrules/package/contents/ui/RulesEditor.qml
44

I have tried it, but it was a bit messy since the editor page does not differentiate when it's a new rule or any other. If I set it whenever no optional settings are enabled, it gives a confusing feeling with all the rows just appearing and dissapearing on a single check.

Maye a hint like this suffice?

iasensio updated this revision to Diff 80306.Apr 16 2020, 6:25 PM
iasensio marked an inline comment as done.

Improve UX for new rules:

  • Set default wmclass policy to ExactMatch
  • Show hint text when settings list is empty
iasensio added inline comments.Apr 16 2020, 6:31 PM
kcmkwin/kwinrules/rulesmodel.cpp
216

Agree. I have set it in a simple way for new rules added.

I would like though, to set this directly on the config schema defaults, but for that to work I'll need to modify some bits of the KConfigXT helper classes, which I have planned for a later patch along some other changes to improve integration.

Something changed because now with the default System Setting window size, when I click on the create new rule button, I see this:

Also when I click on Detect window properties, it always enters the text "navigator" into the window class text field, but adds the window title into the window title text box which is hidden by default. Is that expected?

With respect to how to make the view less overwhelming, here's an idea: make each section header collapsible, and collapse all sections by default except for the Window Matching header, whose contents would be entirely visible by default. Then remember the expanded/collapsed state for each section globally, so a person who expands them all will always see them expanded in the future.

kcmkwin/kwinrules/package/contents/ui/OptionsComboBox.qml
70

Hmm, this is not fixed yet for me. Probably you need to put a MouseArea inside the ItemDelegate that toggles the checkbox if clicked.

kcmkwin/kwinrules/package/contents/ui/RuleItemDelegate.qml
44

Same here; would be nice if clicking on the icon or text also activated the checkbox.

kcmkwin/kwinrules/package/contents/ui/RulesList.qml
106

Give it ellipses ("Import...")

kcmkwin/kwinrules/package/contents/ui/ValueEditor.qml
140

Needs to be a bit bigger; the text still expands when set to 100%.

Something changed because now with the default System Setting window size, when I click on the create new rule button, I see this:

I can reproduce it. I'm making the ConfigModule.columnWidth a bit wider to fix it for the default SySe size.
I'd like to set a different width for each page, but that's not possible currently, I think.

Also when I click on Detect window properties, it always enters the text "navigator" into the window class text field, but adds the window title into the window title text box which is hidden by default. Is that expected?

This is expected, yes. The detected properties only fill those fields that are disabled or empty, to not overwrite user's settings. The previous KCM had the same behavior, but also showed a (quite ugly) selection window only for the "matching" section. It's on my future TODO to add some UI to let the user apply or discard the detected properties.

With respect to how to make the view less overwhelming, here's an idea: make each section header collapsible, and collapse all sections by default except for the Window Matching header, whose contents would be entirely visible by default. Then remember the expanded/collapsed state for each section globally, so a person who expands them all will always see them expanded in the future.

I'll give it a try, but the current model is linear (, and grouped by a section role. It might be difficult to achieve it without fully changing model.
I wouldn't like to miss that by default, only the selected properties for each rule are shown, as properties "added" to a list. The con, of course is that it can be a bit harder to start editing a new rule.

kcmkwin/kwinrules/package/contents/ui/OptionsComboBox.qml
70

oh, silly me! I did that on the main list selection when exporting.. will repeat it here

kcmkwin/kwinrules/package/contents/ui/ValueEditor.qml
140

Ok. I might have a slightly smaller font.
Is there a not very hackish way the get the width for "100%" with user's font?

iasensio updated this revision to Diff 80317.Apr 16 2020, 9:38 PM

Small UI fixes

ngraham added inline comments.Apr 16 2020, 9:53 PM
kcmkwin/kwinrules/package/contents/ui/ValueEditor.qml
140

Allow me to introduce you to TextMetrics! It can be used for exactly this. Elisa uses it all over the place for just this purpose.

iasensio updated this revision to Diff 80337.Apr 16 2020, 11:43 PM
iasensio marked 4 inline comments as done.

More UI polishing

iasensio added inline comments.Apr 17 2020, 12:01 AM
kcmkwin/kwinrules/package/contents/ui/RuleItemDelegate.qml
44

I'm not so sure here. Since this is a complex item, I find toggling selection on text click unexpected, and any misclick can be confusing. Deselecting will hide the item (it's like removing it from the list), and only activating on click would be very inconsistent.
So I'd rather keep the selection action here to the checkbox.

iasensio updated this revision to Diff 80472.Apr 18 2020, 4:20 PM

As suggested by @ngraham in the VDG channel, this changes the metaphor to select new properti>
Instead of a Show All filter, now we can add properties to the rule using an overlay sheet.
The Detect window properties option also uses the sheet to show and select them, which was a>

There are some UI/QML quirks with the overlay I'm not able to get rid of:

  • The first item in the ListView goes under the section header until it's repainted
  • Some items misaling the right button
  • Cannot set the focus to the search field

Adding an overview of the overlay
Add Properties:

Detect Window Properties:

Wow, this is waaaaaay better. The workflow is really smooth. It's, like, incredibly great now. Also, the rules actually work for me now! So that's nice.

I really only have nitpicks:

kcmkwin/kwinrules/package/contents/ui/RuleItemDelegate.qml
64

let's use edit-delete for this, as that uses a trashcan icon and we're moving towards using that everywhere

kcmkwin/kwinrules/package/contents/ui/RulesEditor.qml
43

the list items are back to changing their background on hover, which isn't necessary

49

Might be a good idea to move the "Add properties... button right below this text so it's obvious to the user now they add more properties. When the placeholder message is not shown, the button could just be there at the bottom of the list, and maybe could change its text to "Add more properties..."

The reason for this is to make it easy for the user to understand what they need to do. We do this in the online accounts KCM:

74

I would make this footer a header to mimic the natural flow. Typically you'll click the "detectwindow properties" button first to target a window, which means it should be at the top of the window to suggest that it's the first thing you want to click on.

76

Button titles use Title Case ("Detect Window Properties")

97

use Title Case and add ellipses ("Add Properties...")

ngraham added inline comments.Apr 18 2020, 6:05 PM
kcmkwin/kwinrules/package/contents/ui/OptionsComboBox.qml
62

Use %1 instead of the hardcoded number 1 for the singular case (there are languages where e.g. the number 21 uses the singular form)

69

Is this necessary to manually assign? I thought LayoutMirroring.enabled got set automatically?

109

Can you add a TODO or FIXME comment indicating that this works around https://bugs.kde.org/show_bug.cgi?id=403153?

114

Add braces, either on the same line, or in the more conventional multi-line style

kcmkwin/kwinrules/package/contents/ui/RuleItemDelegate.qml
59

elision only works when a label's maximum width is defined; you probably want to set that somehow, or make it not elide

kcmkwin/kwinrules/package/contents/ui/RulesEditor.qml
85

In English at least, this minimum width isn't high enough to avoid the spinbox getting wider when going from 1 to 2.

90

Same deal for the singular case here; use %1 instead of 1

94–97

That's correct; it's not required unless the spinbox is editable.

138

always manually set width to the list view's width on delegates (e.g. width: overlayModel.width)

149

due to a Qt bug, left and right margins are not reversed in RTL mode. So whenever you set one of those, yo need to explicitly handle the RTL case like this:

Layout.leftMargin: !LayoutMirroring.enabled? Kirigami.Units.smallSpacing : 0
Layout.rightMargin: LayoutMirroring.enabled? Kirigami.Units.smallSpacing : 0

(same for all other instances of using left and right margins on an item in a layout)

174

Needs to be vertically centered, or else this happens:

(probably do the same for other items in this rowlayout as right now their being vertically centered is by chance rather than done explicitly)

176

Is there a reason to use opacity rather than visible here?

195

This doesn't do what you think it does.

searchField.focus doesn't mean, "focus the search field", it means "allow the search field to *become* focused. The property is terribly mis-named. :(

You probably want to set focus: true on the search field itself, and do searchField.forceActiveFocus() here.

Then again I would imagine that the search field internally has focus: true set so setting it again here may be unnecessary.

kcmkwin/kwinrules/package/contents/ui/RulesList.qml
170

Could we handle the tab key here too?

kcmkwin/kwinrules/package/contents/ui/RulesEditor.qml
149

We shouldn't be working round Qt bugs.

Also..if you do that, what will happen when Qt does fix it?

195

searchField.focus doesn't mean, "focus the search field", it means "allow the search field to *become* focused. The property is terribly mis-named. :(

That's not right.

focus does mean "become focussed".

However, it means "become focussed within the parent focus scope".

Focus Scopes are a super simple concept that are simultaneously very very very confusing, to the point that I can't explain them well.

In this case ListView is our focus scope, so we also need to make the listview focussed in order for it to have any effect.

forceActiveFocus() iterates through the tree doing that.

iasensio updated this revision to Diff 80512.Apr 19 2020, 1:03 AM
iasensio marked 21 inline comments as done.
  • UI polishing: Address comments

I'm not 100% sure about this two, but they are uploaded now so you people can say:

  • Trashcan always visible
  • Use a hint button on empty rules
iasensio added inline comments.Apr 19 2020, 1:06 AM
kcmkwin/kwinrules/package/contents/ui/OptionsComboBox.qml
69

You're right, it works automatically now!
It didn't previously for me, so maybe something was fixed for Qt5.14

kcmkwin/kwinrules/package/contents/ui/RuleItemDelegate.qml
68

I reworked all the Layout hints to use preferredWidth instead of max/mins and it seems to work much better, including the elidedlabel.

kcmkwin/kwinrules/package/contents/ui/RulesEditor.qml
43

Having hoverEnabled: false does not allow to show the delete button on hover only.
The other option is to have it always visible, which is not as a bad as I though, and is indeed cleaner than having the background changing colors

149

As a kind of middle way, I set the two margins to the same value

176

It is to preserve the space of the button in the Layout when hidden

176

I'm not too proud of this, but it was required to avoid this overlapping

iasensio updated this revision to Diff 80513.EditedApr 19 2020, 3:39 AM
  • UI fix: Almost perfect alignment in RuleItemDelegate
davidedmundson added inline comments.Apr 19 2020, 2:21 PM
kcmkwin/kwinrules/package/contents/ui/RulesEditor.qml
149
iasensio updated this revision to Diff 80694.Apr 20 2020, 8:32 PM

UI refining:

  • Move Add buttons to the bottom-left of lists (T10384)
  • Fix alignment between rule delegates

ngraham accepted this revision.Apr 20 2020, 9:51 PM

Boom, perfect.

Wonderful work.

This revision is now accepted and ready to land.Apr 20 2020, 9:51 PM
davidedmundson accepted this revision.Apr 22 2020, 11:06 AM

If you haven't done so already, I would suggest attaching QAbstractItemModelTester to rulesModel and move stuff about. It's really good at finding technical quirks.

But in general +++ I couldn't find any real technical problems \o/

I say we try and merge it early so we can get some wider testing.

kcmkwin/kwinrules/rulesmodel.cpp
706

This won't work on wayland, we will want to copy whatever we do in the virtual desktop KCM.

I think this is not regression, as the current code does the same.
At which point I'm happy to merge now and revist it later.

zzag accepted this revision.Apr 22 2020, 12:28 PM

Awesome work!

kcmkwin/kwinrules/package/contents/ui/RulesEditor.qml
94–97

Hmm, okay. I just restated what Qt docs say.

This revision was automatically updated to reflect the committed changes.
iasensio added inline comments.Apr 22 2020, 7:42 PM
kcmkwin/kwinrules/rulesmodel.cpp
706

I'll look into it. Thanks!

zzag added a comment.EditedApr 24 2020, 6:38 AM

@iasensio BTW, since this change landed, can we remove origin/iasensio/kcm_kwinrules?

EDIT: and origin/iasensio/kcm_kwinrules_xt

iasensio added a comment.EditedApr 24 2020, 6:51 AM
In D28152#656181, @zzag wrote:

@iasensio BTW, since this change landed, can we remove origin/iasensio/kcm_kwinrules?

EDIT: and origin/iasensio/kcm_kwinrules_xt

Sure. I didn't know how to remove them.
EDIT: I googled it, and those are now removed :)