Port kwinrules kcm to kconfigxt
ClosedPublic

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

Diff Detail

Repository
R108 KWin
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 22967
Build 22985: arc lint + arc unit
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
2

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
443

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
2

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

5

Change to RuleSettingsBase

kcmkwin/kwinrules/rulesettingsmanager.cpp
42

We don't do "this->"

52

Ditto

kcmkwin/kwinrules/rulesettingsmanager.h
2

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

5

Will become rulesettingsbase.h

11

Please remove this

13

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
32

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

or better yet use qDeleteAll(m_rules)

rules.cpp
101

So I guess it'll change ;-)

307

int v, the const is mostly useless here

1034

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

1035

Space before * not after

1048–1049

declare groups as const or wrap it in qAsConst here

1053

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
2

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

ervin added inline comments.Feb 27 2020, 1:04 PM
kcmkwin/kwinrules/rulesettingsmanager.h
2

So that's an option I guess.

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

Add second kcfg, clean class hierachy

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

Maybe you want to coordinate with T12729

crossi added inline comments.Mar 3 2020, 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.Mar 3 2020, 12:57 PM

add copyright

hchain marked 13 inline comments as done.Mar 3 2020, 1:10 PM
zzag added a comment.Mar 4 2020, 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.Mar 5 2020, 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
212–213

With my proposal it'd become:

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

Would need a qAsConst around m_rules

226

With my proposal it'd become:

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

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

rules.h
105

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

142

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

199

And here as well.

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

split loadAllInto/saveAllFrom

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

qAsConst

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

Move rulesettings* to source root

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

remove unecessary group deletion

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

fix stack overflow, export

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

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

QVector<Rules *> m_rules;
rulebooksettings.cpp
27 ↗(On Diff #77300)

Seems redundant. Please delete this line.

rulebooksettings.h
31 ↗(On Diff #77300)

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

rules.cpp
307

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

rules.h
392

Unrelated change. Please revert it.

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

cleanup

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

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

kcmkwin/kwinrules/ruleslist.cpp
32

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

rules.h
105

Can drop the const now

136

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.Mar 16 2020, 8:39 AM
hchain marked an inline comment as done.Mar 16 2020, 9:24 AM
hchain updated this revision to Diff 77712.Mar 16 2020, 10:20 AM

Remove double delete smell

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

const does make sense here I think

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