Details
Diff Detail
- Repository
- R108 KWin
- Lint
Lint Skipped - Unit
Unit Tests Skipped - Build Status
Buildable 22968 Build 22986: arc lint + arc unit
kcmkwin/kwinrules/rulesettingsmanager.h | ||
---|---|---|
2 ↗ | (On Diff #76481) | Please add a copyright header. |
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 ↗ | (On Diff #76481) | 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 ↗ | (On Diff #76481) | Please rename the files to rulesettingsbase.kcfg and rulesettingsbase.kcfgc |
5 ↗ | (On Diff #76481) | Change to RuleSettingsBase |
kcmkwin/kwinrules/rulesettingsmanager.cpp | ||
42 ↗ | (On Diff #76481) | We don't do "this->" |
52 ↗ | (On Diff #76481) | Ditto |
kcmkwin/kwinrules/rulesettingsmanager.h | ||
2 ↗ | (On Diff #76481) | Looks like those include guards should be changed to match the filename, and please rename that to rulesettings.h (+ rulesettings.cpp) of course. |
5 ↗ | (On Diff #76481) | Will become rulesettingsbase.h |
11 ↗ | (On Diff #76481) | Please remove this |
13 ↗ | (On Diff #76481) | 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 ↗ | (On Diff #76481) | 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 ↗ | (On Diff #76481) | or better yet use qDeleteAll(m_rules) |
rules.cpp | ||
101 ↗ | (On Diff #76481) | So I guess it'll change ;-) |
307 ↗ | (On Diff #76481) | int v, the const is mostly useless here |
1034 ↗ | (On Diff #76481) | Yeah this lack of separation between General and parameterized groups really feels wrong |
1035 ↗ | (On Diff #76481) | Space before * not after |
1048–1049 ↗ | (On Diff #76481) | declare groups as const or wrap it in qAsConst here |
1053 ↗ | (On Diff #76481) | qAsConst(m_rules) |
kcmkwin/kwinrules/rulesettingsmanager.h | ||
---|---|---|
2 ↗ | (On Diff #76481) | FWIW, we (KWin devs) are okay with using #pragma once. |
kcmkwin/kwinrules/rulesettingsmanager.h | ||
---|---|---|
2 ↗ | (On Diff #76481) | So that's an option I guess. |
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 |
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]
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 ↗ | (On Diff #76481) | With my proposal it'd become: m_settings->load(); m_rules = m_settings->rules(); |
213 ↗ | (On Diff #76481) | Would need a qAsConst around m_rules |
226 ↗ | (On Diff #76481) | With my proposal it'd become: m_settings->setRules(m_rules); m_settings->save(); |
rules.cpp | ||
1034 ↗ | (On Diff #76481) | I wonder if that would still make sense if we got a getter for the list. |
rules.h | ||
105 ↗ | (On Diff #76481) | I'd expect it via pointer since RuleSettings isn't a data type. |
142 ↗ | (On Diff #76481) | Same thing, I'd expect a pointer not a reference. |
199 ↗ | (On Diff #76481) | And here as well. |
kcmkwin/kwinrules/ruleslist.h | ||
---|---|---|
53–54 ↗ | (On Diff #76481) | 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 ↗ | (On Diff #76481) | We no longer read rules here. Maybe rename this method to match what it does now? |
rules.h | ||
392 ↗ | (On Diff #76481) | Unrelated change. Please revert it. |
Minor things now (the one about the enum can be ignored of course).
kcmkwin/kwinrules/ruleslist.cpp | ||
---|---|---|
32 ↗ | (On Diff #76481) | I still think this should be switched to using a pointer, or if not the this should be dropped as parameter. |
rules.h | ||
105 ↗ | (On Diff #76481) | Can drop the const now |
136 ↗ | (On Diff #76481) | Might be worth testing without it, since C++11 this should be guaranteed to be an int anyway. |
rules.h | ||
---|---|---|
105 ↗ | (On Diff #76481) | const does make sense here I think |