Differential D28152 Diff 79846 kcmkwin/kwinrules/kcmrules.cpp

Changeset View

Standalone View

View Options

kcmkwin/kwinrules/kcmrules.cpp

  • This file was added.
1​/*
2​ * Copyright (c) 2004 Lubos Lunak <l.lunak@kde.org>
3​ * Copyright (c) 2020 Ismael Asensio <isma.af@gmail.com>
4​ *
5​ * This program is free software; you can redistribute it and/or
6​ * modify it under the terms of the GNU General Public License as
7​ * published by the Free Software Foundation; either version 2 of
8​ * the License or (at your option) version 3 or any later version
9​ * accepted by the membership of KDE e.V. (or its successor approved
10​ * by the membership of KDE e.V.), which shall act as a proxy
11​ * defined in Section 14 of version 3 of the license.
12​ *
13​ * This program is distributed in the hope that it will be useful,
14​ * but WITHOUT ANY WARRANTY; without even the implied warranty of
15​ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
16​ * GNU General Public License for more details.
17​ *
18​ * You should have received a copy of the GNU General Public License
19​ * along with this program. If not, see <http://www.gnu.org/licenses/>.
20​ */
21​
22​#include "kcmrules.h"
23​
24​#include <QDBusConnection>
25​#include <QDBusMessage>
broulikUnsubmitted
Done

Don't include the entire module, just include whatever QDBus class you're using, but it appears unused in this file anyway?

broulik: Don't include the entire module, just include whatever `QDBus` class you're using, but it…
iasensioAuthorUnsubmitted
Not Done

It's used in the save() method, to make kwin reload the configuration

iasensio: It's used in the `save()` method, to make kwin reload the configuration
26​
27​#include <KAboutData>
28​#include <KConfig>
29​#include <KLocalizedString>
30​#include <KPluginFactory>
31​
32​
33​namespace KWin
34​{
35​
36​KCMKWinRules::KCMKWinRules(QObject *parent, const QVariantList &arguments)
37​ : KQuickAddons::ConfigModule(parent, arguments)
38​ , m_ruleBookModel(new RuleBookModel(this))
39​ , m_rulesModel(new RulesModel(this))
40​{
41​ auto about = new KAboutData(QStringLiteral("kcm_kwinrules"),
42​ i18n("Window Rules"),
43​ QStringLiteral("1.0"),
44​ QString(),
45​ KAboutLicense::GPL);
46​ about->addAuthor(i18n("Ismael Asensio"),
47​ i18n("Author"),
48​ QStringLiteral("isma.af@gmail.com"));
49​ setAboutData(about);
50​
51​ setQuickHelp(i18n("<p><h1>Window-specific Settings</h1> Here you can customize window settings specifically only"
52​ " for some windows.</p>"
53​ " <p>Please note that this configuration will not take effect if you do not use"
54​ " KWin as your window manager. If you do use a different window manager, please refer to its documentation"
55​ " for how to customize window behavior.</p>"));
56​
57​ connect(m_rulesModel, &RulesModel::descriptionChanged, this, [this]{
58​ if (m_editIndex.isValid()) {
59​ m_ruleBookModel->setDescriptionAt(m_editIndex.row(), m_rulesModel->description());
60​ }
61​ } );
62​ connect(m_rulesModel, &RulesModel::dataChanged, this, &KCMKWinRules::updateNeedsSave);
63​ connect(m_ruleBookModel, &RulesModel::dataChanged, this, &KCMKWinRules::updateNeedsSave);
64​}
65​
66​void KCMKWinRules::load()
67​{
68​ m_ruleBookModel->load();
69​
70​ m_editIndex = QModelIndex();
71​ emit editIndexChanged();
72​
73​ setNeedsSave(false);
broulikUnsubmitted
Done

You don't need to delete this manually. You pass this as parent in the constructor and Qt has this parent-child releationship where a parent brutally murders its children on desturction.

broulik: You don't need to `delete` this manually. You pass `this` as `parent` in the constructor and Qt…
74​}
75​
76​void KCMKWinRules::save()
77​{
78​ saveCurrentRule();
79​
80​ m_ruleBookModel->save();
81​
82​ // Notify kwin to reload configuration
83​ QDBusMessage message = QDBusMessage::createSignal("/KWin", "org.kde.KWin", "reloadConfig");
84​ QDBusConnection::sessionBus().send(message);
85​}
86​
87​void KCMKWinRules::updateNeedsSave()
88​{
89​ setNeedsSave(true);
90​ emit needsSaveChanged();
91​}
broulikUnsubmitted
Done

Why not keep it a QVector? We generally try to move away from QList

broulik: Why not keep it a `QVector`? We generally try to move away from `QList`
92​
93​void KCMKWinRules::saveCurrentRule()
94​{
95​ if (m_editIndex.isValid() && needsSave()) {
96​ m_ruleBookModel->setRuleAt(m_editIndex.row(), m_rulesModel->exportToRules());
97​ }
98​}
99​
100​int KCMKWinRules::editIndex() const
101​{
102​ if (!m_editIndex.isValid()) {
103​ return -1;
104​ }
105​ return m_editIndex.row();
106​}
107​
108​
109​void KCMKWinRules::setRuleDescription(int index, const QString &description)
110​{
111​ if (index < 0 || index >= m_ruleBookModel->rowCount()) {
112​ return;
113​ }
114​
115​ if (m_editIndex.row() == index) {
116​ m_rulesModel->setDescription(description);
117​ return;
118​ }
119​ m_ruleBookModel->setDescriptionAt(index, description);
120​
121​ updateNeedsSave();
122​}
123​
124​
125​void KCMKWinRules::editRule(int index)
126​{
127​ if (index < 0 || index >= m_ruleBookModel->rowCount()) {
128​ return;
129​ }
130​ saveCurrentRule();
131​
132​ m_editIndex = m_ruleBookModel->index(index);
133​ emit editIndexChanged();
134​
135​ m_rulesModel->importFromRules(m_ruleBookModel->ruleAt(m_editIndex.row()));
136​
137​ // Set the active page to rules editor (0:RulesList, 1:RulesEditor)
138​ setCurrentIndex(1);
139​}
140​
141​void KCMKWinRules::createRule()
broulikUnsubmitted
Not Done

Could this be done in-memory without a temp file?

broulik: Could this be done in-memory without a temp file?
iasensioAuthorUnsubmitted
Not Done

Yes, I want to talk to @hchain and see to expand the RuleBookSettings class to work with a list of RuleSettings objects instead of Rules, and avoid this intermediate step.

iasensio: Yes, I want to talk to @hchain and see to expand the `RuleBookSettings` class to work with a…
142​{
143​ const int newIndex = m_ruleBookModel->rowCount();
144​ m_ruleBookModel->insertRow(newIndex);
145​
146​ updateNeedsSave();
147​
148​ editRule(newIndex);
149​}
150​
151​void KCMKWinRules::removeRule(int index)
152​{
153​ if (index < 0 || index >= m_ruleBookModel->rowCount()) {
154​ return;
155​ }
156​
157​ m_ruleBookModel->removeRow(index);
158​
159​ emit editIndexChanged();
160​ updateNeedsSave();
161​}
162​
163​void KCMKWinRules::moveRule(int sourceIndex, int destIndex)
164​{
165​ const int lastIndex = m_ruleBookModel->rowCount() - 1;
166​ if (sourceIndex == destIndex
167​ || (sourceIndex < 0 || sourceIndex > lastIndex)
168​ || (destIndex < 0 || destIndex > lastIndex)) {
169​ return;
170​ }
171​
172​ m_ruleBookModel->moveRow(QModelIndex(), sourceIndex, QModelIndex(), destIndex);
173​
broulikUnsubmitted
Done

Looks like you're leaking the rules? Perhaps do a

qDeleteAll(m_ruleList)

in the destructor

broulik: Looks like you're leaking the rules? Perhaps do a ``` qDeleteAll(m_ruleList) ``` in the…
174​ emit editIndexChanged();
175​ updateNeedsSave();
176​}
177​
178​void KCMKWinRules::exportToFile(const QUrl &path, int index)
179​{
180​ Q_ASSERT(index >= 0 && index < m_ruleBookModel->rowCount());
181​
182​ saveCurrentRule();
183​
184​ const auto config = KSharedConfig::openConfig(path.toLocalFile(), KConfig::SimpleConfig);
broulikUnsubmitted
Done

I think doing

if (index < 0 || index >= m_ruleList.count()) {
    return;
}

is clear enough

broulik: I think doing ``` if (index < 0 || index >= m_ruleList.count()) { return; } ``` is clear…
185​ const Rules *rule = m_ruleBookModel->ruleAt(index);
186​
187​ RuleSettings settings(config, rule->description);
188​ settings.setDefaults();
189​ rule->write(&settings);
190​ settings.save();
191​}
192​
193​void KCMKWinRules::importFromFile(const QUrl &path)
194​{
195​ const auto config = KSharedConfig::openConfig(path.toLocalFile(), KConfig::SimpleConfig);
196​ const QStringList groups = config->groupList();
197​ if (groups.isEmpty()) {
198​ return;
199​ }
200​
201​ for (const QString &groupName : groups) {
202​ RuleSettings settings(config, groupName);
203​
204​ const bool remove = settings.deleteRule();
205​ const QString importDescription = settings.description();
206​ if (importDescription.isEmpty()) {
207​ continue;
208​ }
209​
210​ // Try to find a rule with the same description to replace
211​ int newIndex = -2;
212​ for (int index = 0; index < m_ruleBookModel->rowCount(); index++) {
213​ if (m_ruleBookModel->descriptionAt(index) == importDescription) {
214​ newIndex = index;
215​ break;
216​ }
217​ }
218​
219​ if (remove) {
220​ m_ruleBookModel->removeRow(newIndex);
221​ continue;
222​ }
223​
224​ if (newIndex < 0) {
225​ newIndex = m_ruleBookModel->rowCount();
226​ m_ruleBookModel->insertRow(newIndex);
227​ }
228​
229​ m_ruleBookModel->setRuleAt(newIndex, new Rules(&settings));
230​
231​ // Reset rule editor if the current rule changed when importing
232​ if (m_editIndex.row() == newIndex) {
233​ m_rulesModel->importFromRules(m_ruleBookModel->ruleAt(m_editIndex.row()));
234​ }
235​ }
236​
237​ updateNeedsSave();
238​}
239​
240​K_PLUGIN_CLASS_WITH_JSON(KCMKWinRules, "kcm_kwinrules.json");
241​
242​} // namespace
243​
244​#include "kcmrules.moc"
broulikUnsubmitted
Done

Don't use this in conjunction with QML! This creates a nested event loop which will cause all sorts of hard to find crashes. You can probably just do the file selection on the QML side, like we do in most other settings modules:

import QtQuick.Dialogs 1.0 as QtDialogs

    Loader {
        id: fileDialogLoader
        active: false
        sourceComponent: QtDialogs.FileDialog {
            title: i18n("Import Rule")
            folder: shortcuts.home
            nameFilters: [ i18n("KWin Rules (*.kwinrule)") ]
            Component.onCompleted: open()
            onAccepted: {
                kcm.importRuleFromFile(fileUrls[0])
                fileDialogLoader.active = false
            }
            onRejected: fileDialogLoader.active = false
        }
    }

This is copied from another KCM and adjusted. It's behind a Loader as creation of this item is pretty heavy, so we want to just do it on demand. And on the import button you just do onClicked: fileDialogLoader.active = true. Perhaps you could share it for open and save, or just use two separate ones. I'm sure you'll figure it out :)

broulik: Don't use this in conjunction with QML! This creates a nested event loop which will cause all…