Port kwinrules kcm to kconfigxt
ClosedPublic

Authored by hchain on Feb 26 2020, 5:32 PM.

Diff Detail

Repository
R108 KWin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes
zzag added inline comments.Feb 27 2020, 10:28 AM
kcmkwin/kwinrules/rulesettingsmanager.h
1 ↗(On Diff #76525)

Please add a copyright header.

ervin requested changes to this revision.Feb 27 2020, 10:59 AM

This needs some design changes I think. Also please update copyright headers in the files you touch and add them in the files you add.

kcmkwin/kwinrules/rulesettings.kcfg
442 ↗(On Diff #76525)

Related to other comments I made, this feels like you should have two kcfg files, one with this trivial General group and one with the parameterized group. Since user code would likely poke one to know how many parameterized groups to look for. Otherwise it forces you to have two very distinct concepts living in the same class.

kcmkwin/kwinrules/rulesettings.kcfgc
1 ↗(On Diff #76525)

Please rename the files to rulesettingsbase.kcfg and rulesettingsbase.kcfgc

4 ↗(On Diff #76525)

Change to RuleSettingsBase

kcmkwin/kwinrules/rulesettingsmanager.cpp
41 ↗(On Diff #76525)

We don't do "this->"

51 ↗(On Diff #76525)

Ditto

kcmkwin/kwinrules/rulesettingsmanager.h
1 ↗(On Diff #76525)

Looks like those include guards should be changed to match the filename, and please rename that to rulesettings.h (+ rulesettings.cpp) of course.

4 ↗(On Diff #76525)

Will become rulesettingsbase.h

10 ↗(On Diff #76525)

Please remove this

12 ↗(On Diff #76525)

Rename RuleSettingsManager to RuleSettings

Note that I find this design looks odd (I suspect it tries to conflate two things which should perhaps be separated). So its a RuleSettings which contains more RuleSettings? How is the user code supposed to decide where to store? Also those RuleSettings are not accessible from the outside but it goes through a list of "Rules"? Is it me or this class only uses count()? This is incredibly meddled (of course I understand the area is muddy already ;-)).

Disclaimer: All the renaming I propose is based on the assumption this inheritance doesn't change, if we decide later on to change it all my comments renaming RuleSettings to RuleSettingsBase might not apply anymore.

kcmkwin/kwinrules/ruleslist.cpp
34

I don't think this is doing what you expect, you didn't declare m_settings as a pointer. I wonder if that couldn't even lead to a double delete... might not be the case a bit by chance (since children are cleaned-up in QObject dtor the KCMRulesList part of the object (including the m_settings member are long gone). Cheer luck ;-)

61–62

or better yet use qDeleteAll(m_rules)

rules.cpp
102

So I guess it'll change ;-)

315

int v, the const is mostly useless here

1025

Yeah this lack of separation between General and parameterized groups really feels wrong

1026

Space before * not after

1033–1034

declare groups as const or wrap it in qAsConst here

1035

qAsConst(m_rules)

This revision now requires changes to proceed.Feb 27 2020, 10:59 AM
zzag added inline comments.Feb 27 2020, 12:41 PM
kcmkwin/kwinrules/rulesettingsmanager.h
1 ↗(On Diff #76525)

FWIW, we (KWin devs) are okay with using #pragma once.

ervin added inline comments.Feb 27 2020, 1:04 PM
kcmkwin/kwinrules/rulesettingsmanager.h
1 ↗(On Diff #76525)

So that's an option I guess.

hchain updated this revision to Diff 76837.Tue, Mar 3, 10:41 AM

Add second kcfg, clean class hierachy

ognarb added a subscriber: ognarb.Tue, Mar 3, 11:45 AM

Maybe you want to coordinate with T12729

crossi added inline comments.Tue, Mar 3, 12:35 PM
kcmkwin/kwinrules/rulebooksettings.cpp
1 ↗(On Diff #76837)

Please add copyright header.

kcmkwin/kwinrules/rulebooksettings.h
1 ↗(On Diff #76837)

Please add copyright header.

kcmkwin/kwinrules/rulebooksettings.kcfg
1 ↗(On Diff #76837)

File should be named rulebooksettingsbase.kcfg

kcmkwin/kwinrules/rulebooksettingsbase.kcfgc
1 ↗(On Diff #76837)

File should be named rulebooksettingsbase.kcfg

hchain updated this revision to Diff 76844.Tue, Mar 3, 12:57 PM

add copyright

hchain marked 13 inline comments as done.Tue, Mar 3, 1:10 PM
zzag added a comment.Wed, Mar 4, 1:08 PM

Hmm, I can't build KWin with this change applied

# kdesrc-build running: '/usr/bin/ninja'
# from directory: /home/vlad/Workspace/KDE/build/kde/workspace/kwin
ninja: error: build.ninja:4074: multiple rules generate kcmkwin/kwinrules/rulesettings.h [-w dupbuild=err]
ervin requested changes to this revision.Thu, Mar 5, 5:16 PM
ervin added inline comments.
kcmkwin/kwinrules/rulebooksettings.h
41 ↗(On Diff #76844)

This is a rather unusual pattern and it prevents declaring the list const in the caller, I'd return a new QVector instead (especially since the the implementation clears the vector anyway).

Thus you could rename both as saveAll and loadAll.

41 ↗(On Diff #76844)

Actually I also wonder if it would make sense to instead separate load/save from getting/setting the rules. It'd be more similar to what we usually if we have a rules()/setRules() pair (which would probably impact the count property automatically).

Then usrRead() and usrSave() could be overridden to deal with loading/saving the actual content of the list when load/save is called on the rule book.

kcmkwin/kwinrules/rulebooksettingsbase.kcfg
14 ↗(On Diff #76844)

Phab seems to not like that. :-)

kcmkwin/kwinrules/ruleslist.cpp
213–215

With my proposal it'd become:

m_settings->load();
m_rules = m_settings->rules();
214

Would need a qAsConst around m_rules

228–229

With my proposal it'd become:

m_settings->setRules(m_rules);
m_settings->save();
rules.cpp
1025

I wonder if that would still make sense if we got a getter for the list.

rules.h
104

I'd expect it via pointer since RuleSettings isn't a data type.

141

Same thing, I'd expect a pointer not a reference.

194–196

And here as well.

This revision now requires changes to proceed.Thu, Mar 5, 5:16 PM
hchain updated this revision to Diff 77116.Fri, Mar 6, 4:13 PM

split loadAllInto/saveAllFrom

hchain marked 6 inline comments as done.Fri, Mar 6, 4:22 PM
hchain updated this revision to Diff 77122.Fri, Mar 6, 4:27 PM

qAsConst

hchain updated this revision to Diff 77129.Fri, Mar 6, 6:26 PM

Move rulesettings* to source root

hchain updated this revision to Diff 77132.Fri, Mar 6, 6:37 PM

remove unecessary group deletion

hchain updated this revision to Diff 77300.Mon, Mar 9, 5:37 PM

fix stack overflow, export

zzag added inline comments.Wed, Mar 11, 11:37 AM
kcmkwin/kwinrules/ruleslist.h
52–53

While you're on this, could you please fix coding style?

QVector<Rules *> m_rules;
rulebooksettings.cpp
28

Seems redundant. Please delete this line.

rulebooksettings.h
32

It appears like we can uncomment this line and remove include for RuleSettings.

rules.cpp
315

We no longer read rules here. Maybe rename this method to match what it does now?

rules.h
389

Unrelated change. Please revert it.

hchain updated this revision to Diff 77563.Fri, Mar 13, 2:51 PM
hchain marked an inline comment as done.

cleanup

hchain marked 6 inline comments as done.Fri, Mar 13, 2:53 PM
ervin requested changes to this revision.Mon, Mar 16, 8:39 AM

Minor things now (the one about the enum can be ignored of course).

kcmkwin/kwinrules/ruleslist.cpp
34

I still think this should be switched to using a pointer, or if not the this should be dropped as parameter.

rules.h
104

Can drop the const now

135

Might be worth testing without it, since C++11 this should be guaranteed to be an int anyway.

This revision now requires changes to proceed.Mon, Mar 16, 8:39 AM
hchain marked an inline comment as done.Mon, Mar 16, 9:24 AM
hchain updated this revision to Diff 77712.Mon, Mar 16, 10:20 AM

Remove double delete smell

hchain marked an inline comment as done.Mon, Mar 16, 11:31 AM
hchain added inline comments.
rules.h
104

const does make sense here I think

ervin accepted this revision.Mon, Mar 16, 12:05 PM
This revision is now accepted and ready to land.Mon, Mar 16, 12:05 PM
This revision was automatically updated to reflect the committed changes.