From d2d92cdfd2c1869942cf119b2b8f680750fa7e96 Mon Sep 17 00:00:00 2001 From: Ismael Asensio Date: Thu, 2 May 2024 16:36:08 +0200 Subject: [PATCH] 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 | 56 +++++++++------------------------------- src/rulebooksettings.h | 6 ++--- src/rules.cpp | 42 ++++++++++++++++++++---------- src/rules.h | 12 ++++----- 4 files changed, 49 insertions(+), 67 deletions(-) diff --git a/src/rulebooksettings.cpp b/src/rulebooksettings.cpp index c3a26e6cb5..061b1e4f30 100644 --- a/src/rulebooksettings.cpp +++ b/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) -{ - 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 RuleBookSettings::rules() +QList RuleBookSettings::rules() const { QList result; result.reserve(m_list.count()); @@ -135,6 +93,16 @@ int RuleBookSettings::ruleCount() const return m_list.count(); } +std::optional 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()); diff --git a/src/rulebooksettings.h b/src/rulebooksettings.h index eeecc5ba9d..0ee9c46304 100644 --- a/src/rulebooksettings.h +++ b/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 &); - QList rules(); + QList rules() const; bool usrSave() override; void usrRead() override; bool usrIsSaveNeeded() const; int ruleCount() const; + std::optional indexForId(const QString &id) const; RuleSettings *ruleSettingsAt(int row) const; RuleSettings *insertRuleSettingsAt(int row); void removeRuleSettingsAt(int row); diff --git a/src/rules.cpp b/src/rules.cpp index c94fbb507c..7e7a1f1c77 100644 --- a/src/rules.cpp +++ b/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(config); +} + void RuleBook::load() { deleteAll(); - if (!m_config) { - m_config = KSharedConfig::openConfig(QStringLiteral("kwinrulesrc"), KConfig::NoGlobals); + if (!m_book) { + m_book = std::make_unique(); } else { - m_config->reparseConfiguration(); + m_book->sharedConfig()->reparseConfiguration(); } - RuleBookSettings book(m_config); - book.load(); - m_rules = book.rules(); + m_book->load(); + m_rules = m_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); - settings.setRules(m_rules); - settings.save(); + m_book->save(); } void RuleBook::discardUsed(Window *c, bool withdrawn) { - bool updated = false; for (QList::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(); } } diff --git a/src/rules.h b/src/rules.h index 9e86f71bee..69f761b65f 100644 --- a/src/rules.h +++ b/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) - { - m_config = config; - } + void setConfig(const KSharedConfig::Ptr &config); private Q_SLOTS: void save(); @@ -322,7 +322,7 @@ private: QTimer *m_updateTimer; bool m_updatesDisabled; QList m_rules; - KSharedConfig::Ptr m_config; + std::unique_ptr m_book; }; inline bool RuleBook::areUpdatesDisabled() const