rules/RuleBook: Optimize saving discarded rules to config

After porting to KConfigXT settings some time ago, there was still an
inefficient and error-prone codepath between the `RuleBook` (which keeps
the runtime list of `Rules`) and the `RuleBookSettings` (responsible for
config reads and saves), in the form of the `setRules()` method.

We can eliminate the `setRules()` codepath, reducing unnecessary
runtime process and file access operations, and instead:
- Keep track of the config `id` in the `Rules` objects
- Keep a single `RuleBookSettings` object as a member
- Modify or delete the discarded rules settings directly
- Save when necessary

This also fixes two bugs/pitfalls of the previous solution:
- the config group id for each rule is now preserved instead of creating
  new ones
- no leftovers on the config file for the discarded groups and entries

Setting custom configs for the integration tests still works unchanged.

BUG: 446381
FIXED-IN: 6.1

src/rulebooksettings.cpp

@@ -20,18 +20,8 @@ RuleBookSettings::RuleBookSettings(KSharedConfig::Ptr config, QObject *parent)
 {
 }
 
-RuleBookSettings::RuleBookSettings(const QString &configname, KConfig::OpenFlags flags, QObject *parent)
-    : RuleBookSettings(KSharedConfig::openConfig(configname, flags), parent)
-{
-}
-
-RuleBookSettings::RuleBookSettings(KConfig::OpenFlags flags, QObject *parent)
-    : RuleBookSettings(QStringLiteral("kwinrulesrc"), flags, parent)
-{
-}
-
 RuleBookSettings::RuleBookSettings(QObject *parent)
-    : RuleBookSettings(KConfig::FullConfig, parent)
+    : RuleBookSettings(KSharedConfig::openConfig(QStringLiteral("kwinrulesrc"), KConfig::NoGlobals), parent)
 {
 }
 
@@ -40,39 +30,7 @@ RuleBookSettings::~RuleBookSettings()
     qDeleteAll(m_list);
 }
 
-void RuleBookSettings::setRules(const QList<Rules *> &rules)
+QList<Rules *> RuleBookSettings::rules() const
-{
-    mCount = rules.count();
-    mRuleGroupList.clear();
-    mRuleGroupList.reserve(rules.count());
-
-    int i = 0;
-    const int list_length = m_list.length();
-    for (const auto &rule : rules) {
-        RuleSettings *settings;
-        if (i < list_length) {
-            // Optimization. Reuse RuleSettings already created
-            settings = m_list.at(i);
-            settings->setDefaults();
-        } else {
-            // If there are more rules than in cache
-            settings = new RuleSettings(this->sharedConfig(), QString::number(i + 1), this);
-            m_list.append(settings);
-        }
-
-        rule->write(settings);
-        mRuleGroupList.append(settings->currentGroup());
-
-        i++;
-    }
-
-    for (int j = m_list.count() - 1; j >= rules.count(); j--) {
-        delete m_list[j];
-        m_list.removeAt(j);
-    }
-}
-
-QList<Rules *> RuleBookSettings::rules()
 {
     QList<Rules *> result;
     result.reserve(m_list.count());
@@ -135,6 +93,16 @@ int RuleBookSettings::ruleCount() const
     return m_list.count();
 }
 
+std::optional<int> RuleBookSettings::indexForId(const QString &id) const
+{
+    for (int i = 0; i < m_list.count(); i++) {
+        if (m_list.at(i)->currentGroup() == id) {
+            return i;
+        }
+    }
+    return std::nullopt;
+}
+
 RuleSettings *RuleBookSettings::ruleSettingsAt(int row) const
 {
     Q_ASSERT(row >= 0 && row < m_list.count());

src/rulebooksettings.h

@@ -22,19 +22,17 @@ class RuleBookSettings : public RuleBookSettingsBase
 {
 public:
     RuleBookSettings(KSharedConfig::Ptr config, QObject *parent = nullptr);
-    RuleBookSettings(const QString &configname, KConfig::OpenFlags, QObject *parent = nullptr);
-    RuleBookSettings(KConfig::OpenFlags, QObject *parent = nullptr);
     RuleBookSettings(QObject *parent = nullptr);
     ~RuleBookSettings();
 
-    void setRules(const QList<Rules *> &);
+    QList<Rules *> rules() const;
-    QList<Rules *> rules();
 
     bool usrSave() override;
     void usrRead() override;
     bool usrIsSaveNeeded() const;
 
     int ruleCount() const;
+    std::optional<int> indexForId(const QString &id) const;
     RuleSettings *ruleSettingsAt(int row) const;
     RuleSettings *insertRuleSettingsAt(int row);
     void removeRuleSettingsAt(int row);

src/rules.cpp

@@ -98,6 +98,7 @@ Rules::Rules(const RuleSettings *settings)
 
 void Rules::readFromSettings(const RuleSettings *settings)
 {
+    m_id = settings->currentGroup();
     description = settings->description();
     if (description.isEmpty()) {
         description = settings->descriptionLegacy();
@@ -166,6 +167,11 @@ void Rules::readFromSettings(const RuleSettings *settings)
 #undef READ_FORCE_RULE
 #undef READ_FORCE_RULE2
 
+QString Rules::id() const
+{
+    return m_id;
+}
+
 #define WRITE_MATCH_STRING(var, capital, force) \
     settings->set##capital##match(var##match);  \
     if (!var.isEmpty() || force) {              \
@@ -186,6 +192,8 @@ void Rules::readFromSettings(const RuleSettings *settings)
 
 void Rules::write(RuleSettings *settings) const
 {
+    settings->setDefaults();
+
     settings->setDescription(description);
     // always write wmclass
     WRITE_MATCH_STRING(wmclass, Wmclass, true);
@@ -944,51 +952,59 @@ void RuleBook::edit(Window *c, bool whole_app)
     p->start();
 }
 
+void RuleBook::setConfig(const KSharedConfig::Ptr &config)
+{
+    m_book = std::make_unique<RuleBookSettings>(config);
+}
+
 void RuleBook::load()
 {
     deleteAll();
-    if (!m_config) {
+    if (!m_book) {
-        m_config = KSharedConfig::openConfig(QStringLiteral("kwinrulesrc"), KConfig::NoGlobals);
+        m_book = std::make_unique<RuleBookSettings>();
     } else {
-        m_config->reparseConfiguration();
+        m_book->sharedConfig()->reparseConfiguration();
     }
-    RuleBookSettings book(m_config);
+    m_book->load();
-    book.load();
+    m_rules = m_book->rules();
-    m_rules = book.rules();
 }
 
 void RuleBook::save()
 {
     m_updateTimer->stop();
-    if (!m_config) {
+    if (!m_book) {
         qCWarning(KWIN_CORE) << "RuleBook::save invoked without prior invocation of RuleBook::load";
         return;
     }
-    RuleBookSettings settings(m_config);
+    m_book->save();
-    settings.setRules(m_rules);
-    settings.save();
 }
 
 void RuleBook::discardUsed(Window *c, bool withdrawn)
 {
-    bool updated = false;
     for (QList<Rules *>::Iterator it = m_rules.begin();
          it != m_rules.end();) {
         if (c->rules()->contains(*it)) {
+            const auto index = m_book->indexForId((*it)->id());
             if ((*it)->discardUsed(withdrawn)) {
-                updated = true;
+                if (index) {
+                    RuleSettings *setting = m_book->ruleSettingsAt(index.value());
+                    (*it)->write(setting);
+                }
             }
             if ((*it)->isEmpty()) {
                 c->removeRule(*it);
                 Rules *r = *it;
                 it = m_rules.erase(it);
                 delete r;
+                if (index) {
+                    m_book->removeRuleSettingsAt(index.value());
+                }
                 continue;
             }
         }
         ++it;
     }
-    if (updated) {
+    if (m_book->usrIsSaveNeeded()) {
         requestDiskStorage();
     }
 }

src/rules.h

@@ -25,6 +25,7 @@ class Window;
 class Output;
 class Rules;
 class RuleSettings;
+class RuleBookSettings;
 class VirtualDesktop;
 
 #ifndef KCMRULES // only for kwin core
@@ -143,6 +144,8 @@ public:
     };
     void write(RuleSettings *) const;
     bool isEmpty() const;
+    QString id() const;
+
 #ifndef KCMRULES
     bool discardUsed(bool withdrawn);
     bool match(const Window *c) const;
@@ -208,6 +211,7 @@ private:
 #endif
     enum Layer layer;
     ForceRule layerrule;
+    QString m_id;
     QString description;
     QString wmclass;
     StringMatch wmclassmatch;
@@ -308,11 +312,7 @@ public:
     void load();
     void edit(Window *c, bool whole_app);
     void requestDiskStorage();
-
+    void setConfig(const KSharedConfig::Ptr &config);
-    void setConfig(const KSharedConfig::Ptr &config)
-    {
-        m_config = config;
-    }
 
 private Q_SLOTS:
     void save();
@@ -322,7 +322,7 @@ private:
     QTimer *m_updateTimer;
     bool m_updatesDisabled;
     QList<Rules *> m_rules;
-    KSharedConfig::Ptr m_config;
+    std::unique_ptr<RuleBookSettings> m_book;
 };
 
 inline bool RuleBook::areUpdatesDisabled() const