D27682: Port kwinrules kcm to kconfigxt

ervin requested changes to this revision.
ervin added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> rulebooksettings.h:41
> +    void saveAllFrom(const QVector<Rules *> &);
> +    void loadAllInto(QVector<Rules *> &);
> +

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.

> rulebooksettings.h:41
> +    void saveAllFrom(const QVector<Rules *> &);
> +    void loadAllInto(QVector<Rules *> &);
> +

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.

> rulebooksettingsbase.kcfg:14
> +</kcfg>
> \ No newline at end of file


Phab seems to not like that. :-)

> ruleslist.cpp:211
>      rules_listbox->clear();
> -    for (QVector< Rules* >::Iterator it = rules.begin();
> -            it != rules.end();
> -            ++it)
> -        delete *it;
> -    rules.clear();
> -    KConfig _cfg("kwinrulesrc");
> -    KConfigGroup cfg(&_cfg, "General");
> -    int count = cfg.readEntry("count", 0);
> -    rules.reserve(count);
> -    for (int i = 1;
> -            i <= count;
> -            ++i) {
> -        cfg = KConfigGroup(&_cfg, QString::number(i));
> -        Rules* rule = new Rules(cfg);
> -        rules.append(rule);
> +    m_settings.loadAllInto(m_rules);
> +    for (const auto rule : m_rules) {

With my proposal it'd become:

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

> ruleslist.cpp:212
> +    m_settings.loadAllInto(m_rules);
> +    for (const auto rule : m_rules) {
>          rules_listbox->addItem(rule->description);

Would need a qAsConst around m_rules

> ruleslist.cpp:225
>  {
> -    KConfig cfg(QLatin1String("kwinrulesrc"));
> -    QStringList groups = cfg.groupList();
> -    for (QStringList::ConstIterator it = groups.constBegin();
> -            it != groups.constEnd();
> -            ++it)
> -        cfg.deleteGroup(*it);
> -    cfg.group("General").writeEntry("count", rules.count());
> -    int i = 1;
> -    for (QVector< Rules* >::ConstIterator it = rules.constBegin();
> -            it != rules.constEnd();
> -            ++it) {
> -        KConfigGroup cg(&cfg, QString::number(i));
> -        (*it)->write(cg);
> -        ++i;
> -    }
> +    m_settings.saveAllFrom(m_rules);
>  }

With my proposal it'd become:

  m_settings->setRules(m_rules);
  m_settings->save();

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

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

> rules.h:104
>      Rules();
> -    explicit Rules(const KConfigGroup&);
> +    explicit Rules(const RuleSettings&);
>      Rules(const QString&, bool temporary);

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

> rules.h:141
> +    };
> +    void write(RuleSettings&) const;
>      bool isEmpty() const;

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

> rules.h:194
>      bool matchClientMachine(const QByteArray& match_machine, bool local) const;
> -    enum SetRule {
> -        UnusedSetRule = Unused,
> -        SetRuleDummy = 256   // so that it's at least short int
> -    };
> -    enum ForceRule {
> -        UnusedForceRule = Unused,
> -        ForceRuleDummy = 256   // so that it's at least short int
> -    };
> -    void readFromCfg(const KConfigGroup& cfg);
> -    static SetRule readSetRule(const KConfigGroup&, const QString& key);
> -    static ForceRule readForceRule(const KConfigGroup&, const QString& key);
> -    static NET::WindowType readType(const KConfigGroup&, const QString& key);
> -    static QString readDecoColor(const KConfigGroup &cfg);
> +    void readFromSettings(const RuleSettings& settings);
> +    static ForceRule readForceRule(int v);

And here as well.

REPOSITORY
  R108 KWin

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

To: hchain, meven, crossi, bport, ervin, #kwin
Cc: ognarb, 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