[WIP] KWinRules KCM Redesign
Needs ReviewPublic

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

Diff Detail

R108 KWin
No Linters Available
No Unit Test Coverage
Build Status
Buildable 24370
Build 24388: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
broulik added inline comments.Fri, Mar 20, 4:42 PM

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




property bool


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


Perhaps set elide: Text.ElideRight


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




There's now a Kirigami.SearchField


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


You could also us a Binding for that:

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



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


I think it does that for you already?


No need to delete those


Check if you changed the value before emitting this for optimization


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)


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


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


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


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


You can also do QModelIndex::data()


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

if (m_showAll == showAll) {

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?

iasensio updated this revision to Diff 78122.Fri, Mar 20, 8:05 PM
iasensio marked 3 inline comments as done.
  • Fix copyright dates
  • Use QVector instead of QList
iasensio added inline comments.Fri, Mar 20, 8:06 PM

Oh, time flies lately!

iasensio updated this revision to Diff 78127.Sat, Mar 21, 12:09 AM
iasensio marked 22 inline comments as done.
  • Address some comments
  • Use a button to edit the rule
iasensio added inline comments.

Yes, I want to talk to @hchain and see to expand the RuleBookSettings class to work with a list of RuleSettings objects instead of Rules, and avoid this intermediate step.


That approach fails for me (QCoreApplication::postEvent: Unexpected null receiver), even with some protections


Not really. In fact keyboard navigation works slightly better without setting focus


This helps keeping the description sizes more constant between delegates, so it looks more like a real table.

Setting Layout.fillWidth:true on the Labelproduces this


I will probably set this text on the C++ side as you suggested in another comment.
This feels a bit alien here.


Quite, but not fully. If clip is not set here, the content seems to overlap the frame.
Maybe that's a bug in ScrollViewKCM.


I definitely want to go with a nicer approach for this thing, but for now is the simplest way I found. @hchain suggested to use a template, ideally, using the type on the KConfigXT schema.

Currently RuleItem::Type has a double meaning: it sets the stored type and the edit field in the UI. And anyway, Coordinate just maps to a QPoint or QSize, and Shortcut a QKeySequence.


It's called by kwin when selecting Edit Specific Window/App Properties from the window menu. Here I just kept the interface with that entry point unchanged, but it is within a custom executable, so it should probably be replaced by a full QML dialog by changing main.cpp.

First of all, thanks @broulik for such a thorough review and feedback!

About some of your many UI comments, I've already implemented some, while I'm torn on some others, but will leave it to you VDG people decide.

  • 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

AFAIK, the order affects when a window can be affected by a more general and a more specific rule, but I also don't know the details

  • 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 had had doubts about the edit button vs clicking, but agree:

  • 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:

  • The "detect window properties" button is somewhat giant


  • 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.

(Sorry I only came halfway through with code review now)

No rush, lucky me, since now there is no place where I can run and hide πŸ˜„

iasensio updated this revision to Diff 78167.Sat, Mar 21, 3:42 PM
  • Not expand 'Detect Window' button to full width
  • RTL support: fix remaining items
iasensio updated this revision to Diff 78195.Sun, Mar 22, 12:21 AM
iasensio marked an inline comment as done.
  • Search also in rules' values
iasensio added inline comments.Sun, Mar 22, 12:22 AM

That seems very handy. I've been trying to, but apparently there is no way to invalidateFilter() from the QML side, to add the showAll condition.
Maybe can it be extended on KItemModels?


Added fully custom search, to include name and value properties

iasensio updated this revision to Diff 78226.Sun, Mar 22, 2:47 PM
iasensio marked 2 inline comments as done.
  • Move filter logic to QML side
iasensio updated this revision to Diff 78235.Sun, Mar 22, 3:43 PM
  • Fix or operator
iasensio updated this revision to Diff 78260.Sun, Mar 22, 10:07 PM
iasensio marked an inline comment as done.
  • Update needsSave status when moving rules
  • Move file picker logic to QML
broulik added inline comments.Mon, Mar 23, 8:23 AM

Don't include the entire module, just include whatever QDBus class you're using, but it appears unused in this file anyway?

1 β†—(On Diff #78260)

This file is just for testing, I presume?


Ah! Okay, alright then. I thought KWin would just run kcmshell5 then. Maybe it makes sense in a future step to change this so KWin just opens the KCM


I think this should be registered in the KCM constructor, not inside of another type

iasensio updated this revision to Diff 78312.Mon, Mar 23, 9:01 PM
iasensio marked 2 inline comments as done.
  • Remove useless file
  • Do not include full QtDbus
iasensio added inline comments.Mon, Mar 23, 9:01 PM

It's used in the save() method, to make kwin reload the configuration

1 β†—(On Diff #78260)

Yeah, it was on my first attempts when I was figuring everything out, and never noticed it again since. Removed.


It's related to the 2nd entry point (kwin_rules_dialog). Since it does not include the kcm, but just the model, this is the most outer object common to both paths.

iasensio updated this revision to Diff 78313.Mon, Mar 23, 9:02 PM
  • Include order
iasensio updated this revision to Diff 78318.Mon, Mar 23, 10:42 PM
  • Remove config file assignation
iasensio updated this revision to Diff 78578.Thu, Mar 26, 5:07 PM
iasensio marked 3 inline comments as done.
  • Set warning message in C++ side
  • Simplify search field
iasensio updated this revision to Diff 78581.Thu, Mar 26, 6:52 PM
iasensio marked 6 inline comments as done.
  • Refactor RuleItem
  • Use the returned pointer of addRule()
iasensio added inline comments.Thu, Mar 26, 7:12 PM

It helped me at the beginning to set-up the model items... but you're probably right now it's just an inconvenience

iasensio updated this revision to Diff 78592.Thu, Mar 26, 9:06 PM
  • Set module name to kcm_kwinrules
iasensio edited the summary of this revision. (Show Details)Thu, Mar 26, 9:06 PM
iasensio updated this revision to Diff 78723.Sat, Mar 28, 1:43 PM
  • Merge branch 'master'
  • Remove unnecesary dependencies
  • Edit rule descriptions inline

Now the rules description can be edited inline in the main list view, but I find it a bit quirky due to how TextField behaves, and I might need some hints on that

iasensio retitled this revision from [RFC] KWinRules KCM Redesign to [WIP] KWinRules KCM Redesign.Sat, Mar 28, 1:44 PM

Make sure you remember to remove "WIP" from the phab request when you think we're ready for merging, otherwise no-one will do any approving.