D27682: Port kwinrules kcm to kconfigxt

[Kevin Ottens]

ervin requested changes to this revision.
ervin added a comment.
This revision now requires changes to proceed.


  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.

INLINE COMMENTS

> rulesettings.kcfg:442
> +  </group>
> +  <group name="General">
> +    <entry name="count" type="int">

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.

> rulesettings.kcfgc:1
> +File=rulesettings.kcfg
> +IncludeFiles=\"rules.h\",\"placement.h\",netwm_def.h

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

> rulesettings.kcfgc:4
> +NameSpace=KWin
> +ClassName=RuleSettings
> +UseEnumTypes=true

Change to RuleSettingsBase

> rulesettingsmanager.cpp:41
> +            // If there are more rules than in cache
> +            settings = new RuleSettings(this->sharedConfig(), QString::number(i), this);
> +            m_list.append(settings);

We don't do "this->"

> rulesettingsmanager.cpp:51
> +    for (i = rules.length() + 1; i <= mCount; i++) {
> +        this->sharedConfig()->deleteGroup(QString::number(i));
> +    }

Ditto

> rulesettingsmanager.h:1
> +#ifndef RULESSETTINGSLIST_H
> +#define RULESSETTINGSLIST_H

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

> rulesettingsmanager.h:4
> +
> +#include "rulesettings.h"
> +#include <KSharedConfig>

Will become rulesettingsbase.h

> rulesettingsmanager.h:10
> +class Rules;
> +// class RuleSettings;
> +

Please remove this

> rulesettingsmanager.h:12
> +
> +class RuleSettingsManager : public RuleSettings
> +{

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.

> ruleslist.cpp:32
>      : QWidget(parent)
> +    , m_settings(new RuleSettingsManager(this))
>  {

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 ;-)

> ruleslist.cpp:61
>  {
> -    for (QVector< Rules* >::Iterator it = rules.begin();
> -            it != rules.end();
> -            ++it)
> +    for (QVector<Rules *>::Iterator it = m_rules.begin(); it != m_rules.end(); ++it)
>          delete *it;

or better yet use `qDeleteAll(m_rules)`

> rules.cpp:101
> +    auto cfg = KSharedConfig::openConfig(file.fileName(), KConfig::SimpleConfig);
> +    // not completely sure this works, because group name will be an empty non null string
> +    readFromSettings(RuleSettings(cfg, QString()));

So I guess it'll change ;-)

> rules.cpp:314
>  
> -Rules::SetRule Rules::readSetRule(const KConfigGroup& cfg, const QString& key)
> +Rules::ForceRule Rules::readForceRule(const int v)
>  {

`int v`, the const is mostly useless here

> rules.cpp:1025
> +    for (int i = 1; i <= settings.count(); ++i) {
> +        RuleSettings settings(m_config, QString::number(i));
> +        Rules* rule = new Rules(settings);

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

> rules.cpp:1026
> +        RuleSettings settings(m_config, QString::number(i));
> +        Rules* rule = new Rules(settings);
>          m_rules.append(rule);

Space before * not after

> rules.cpp:1039
>      QStringList groups = m_config->groupList();
> -    for (QStringList::ConstIterator it = groups.constBegin();
> -            it != groups.constEnd();
> -            ++it)
> -        m_config->deleteGroup(*it);
> -    m_config->group("General").writeEntry("count", m_rules.count());
> +    for (const auto &group : groups)
> +        m_config->deleteGroup(group);

declare groups as const or wrap it in qAsConst here

> rules.cpp:1044
> +    settings.setCount(m_rules.count());
> +    for (const auto &rule: m_rules) {
> +        RuleSettings settings(m_config, QString::number(i));

`qAsConst(m_rules)`

REPOSITORY
  R108 KWin

REVISION DETAIL
  https://phabricator.kde.org/D27682

To: hchain, meven, crossi, bport, ervin, #kwin
Cc: zzag, kwin, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, mkulinski, ragreen, jackyalcine, iodelay, crozbo, bwowk, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, hardening, romangg, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kwin/attachments/20200227/8e240a3b/attachment-0001.html>