[WIP] KWinRules KCM Redesign
Needs ReviewPublic

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

Diff Detail

Repository
R108 KWin
Branch
wip/inline
Lint
No Linters Available
Unit
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
kcmkwin/kwinrules/package/contents/ui/OptionsComboBox.qml
42

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

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

Needed?

31

property bool

36

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

64

Perhaps set elide: Text.ElideRight

68

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

84

Remove

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

There's now a Kirigami.SearchField

62

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

67

You could also us a Binding for that:

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

Capitalize

116

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

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

I think it does that for you already?

kcmkwin/kwinrules/rulesmodel.cpp
58

No need to delete those

150

Check if you changed the value before emitting this for optimization

178

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)

317

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

324

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

774

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

776

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

784

You can also do QModelIndex::data()

801

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

if (m_showAll == showAll) {
    return;
}
...
kcmkwin/kwinrules/rulesmodel.h
48

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
kcmkwin/kwinrules/rulesdialog.h
3

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.
kcmkwin/kwinrules/kcmrules.cpp
141

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.

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

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

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

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

68

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

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

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

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

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

kcmkwin/kwinrules/ruleitem.h
40

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.

kcmkwin/kwinrules/rulesdialog.cpp
35

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

Agree.

  • 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
kcmkwin/kwinrules/rulesmodel.cpp
774

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?

776

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
kcmkwin/kwinrules/kcmrules.cpp
25

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

kcmkwin/kwinrules/regenerateindex.sh
1 β†—(On Diff #78260)

This file is just for testing, I presume?

kcmkwin/kwinrules/rulesdialog.cpp
35

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

kcmkwin/kwinrules/rulesmodel.cpp
44

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
kcmkwin/kwinrules/kcmrules.cpp
25

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

kcmkwin/kwinrules/regenerateindex.sh
1 β†—(On Diff #78260)

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

kcmkwin/kwinrules/rulesmodel.cpp
44

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
kcmkwin/kwinrules/ruleitem.h
33

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.