Differential D27682 Diff 76481 kcmkwin/kwinrules/ruleslist.cpp

Changeset View

Standalone View

View Options

kcmkwin/kwinrules/ruleslist.cpp

Show All 23 Lines
24 24​
25#include "ruleswidget.h" 25​#include "ruleswidget.h"
26 26​
27namespace KWin 27​namespace KWin
28{ 28​{
29 29​
30KCMRulesList::KCMRulesList(QWidget* parent) 30​KCMRulesList::KCMRulesList(QWidget* parent)
31 : QWidget(parent) 31​ : QWidget(parent)
32​ , m_settings(new RuleSettingsManager(this))
ervinUnsubmitted
Done

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

ervin: I don't think this is doing what you expect, you didn't declare m_settings as a pointer. I…
ervinUnsubmitted
Done

I still think this should be switched to using a pointer, or if not the this should be dropped as parameter.

ervin: I still think this should be switched to using a pointer, or if not the this should be dropped…
32{ 33​{
33 setupUi(this); 34​ setupUi(this);
34 // connect both current/selected, so that current==selected (stupid QListBox :( ) 35​ // connect both current/selected, so that current==selected (stupid QListBox :( )
35 connect(rules_listbox, SIGNAL(itemChanged(QListWidgetItem*)), 36​ connect(rules_listbox, SIGNAL(itemChanged(QListWidgetItem*)),
36 SLOT(activeChanged())); 37​ SLOT(activeChanged()));
37 connect(rules_listbox, SIGNAL(itemSelectionChanged()), 38​ connect(rules_listbox, SIGNAL(itemSelectionChanged()),
38 SLOT(activeChanged())); 39​ SLOT(activeChanged()));
39 connect(new_button, SIGNAL(clicked()), 40​ connect(new_button, SIGNAL(clicked()),
Show All 12 Lines52 connect(import_button, SIGNAL(clicked()),
52 SLOT(importClicked())); 53​ SLOT(importClicked()));
53 connect(rules_listbox, SIGNAL(itemDoubleClicked(QListWidgetItem*)), 54​ connect(rules_listbox, SIGNAL(itemDoubleClicked(QListWidgetItem*)),
54 SLOT(modifyClicked())); 55​ SLOT(modifyClicked()));
55 load(); 56​ load();
56} 57​}
57 58​
58KCMRulesList::~KCMRulesList() 59​KCMRulesList::~KCMRulesList()
59{ 60​{
60 for (QVector< Rules* >::Iterator it = rules.begin(); 61​ for (QVector<Rules *>::Iterator it = m_rules.begin(); it != m_rules.end(); ++it)
ervinUnsubmitted
Done

or better yet use qDeleteAll(m_rules)

ervin: or better yet use `qDeleteAll(m_rules)`
61 it != rules.end(); ​
62 ++it) ​
63 delete *it; 62​ delete *it;
64 rules.clear(); 63​ m_rules.clear();
65} 64​}
66 65​
67void KCMRulesList::activeChanged() 66​void KCMRulesList::activeChanged()
68{ 67​{
69 QListWidgetItem *item = rules_listbox->currentItem(); 68​ QListWidgetItem *item = rules_listbox->currentItem();
70 int itemRow = rules_listbox->row(item); 69​ int itemRow = rules_listbox->row(item);
71 70​
72 if (item != nullptr) // make current==selected 71​ if (item != nullptr) // make current==selected
Show All 9 Lines
82{ 81​{
83 RulesDialog dlg(this); 82​ RulesDialog dlg(this);
84 Rules* rule = dlg.edit(nullptr, {}, false); 83​ Rules* rule = dlg.edit(nullptr, {}, false);
85 if (rule == nullptr) 84​ if (rule == nullptr)
86 return; 85​ return;
87 int pos = rules_listbox->currentRow() + 1; 86​ int pos = rules_listbox->currentRow() + 1;
88 rules_listbox->insertItem(pos , rule->description); 87​ rules_listbox->insertItem(pos , rule->description);
89 rules_listbox->setCurrentRow(pos, QItemSelectionModel::ClearAndSelect); 88​ rules_listbox->setCurrentRow(pos, QItemSelectionModel::ClearAndSelect);
90 rules.insert(rules.begin() + pos, rule); 89​ m_rules.insert(m_rules.begin() + pos, rule);
91 emit changed(true); 90​ emit changed(true);
92} 91​}
93 92​
94void KCMRulesList::modifyClicked() 93​void KCMRulesList::modifyClicked()
95{ 94​{
96 int pos = rules_listbox->currentRow(); 95​ int pos = rules_listbox->currentRow();
97 if (pos == -1) 96​ if (pos == -1)
98 return; 97​ return;
99 RulesDialog dlg(this); 98​ RulesDialog dlg(this);
100 Rules* rule = dlg.edit(rules[ pos ], {}, false); 99​ Rules *rule = dlg.edit(m_rules[pos], {}, false);
101 if (rule == rules[ pos ]) 100​ if (rule == m_rules[pos])
102 return; 101​ return;
103 delete rules[ pos ]; 102​ delete m_rules[pos];
104 rules[ pos ] = rule; 103​ m_rules[pos] = rule;
105 rules_listbox->item(pos)->setText(rule->description); 104​ rules_listbox->item(pos)->setText(rule->description);
106 emit changed(true); 105​ emit changed(true);
107} 106​}
108 107​
109void KCMRulesList::deleteClicked() 108​void KCMRulesList::deleteClicked()
110{ 109​{
111 int pos = rules_listbox->currentRow(); 110​ int pos = rules_listbox->currentRow();
112 Q_ASSERT(pos != -1); 111​ Q_ASSERT(pos != -1);
113 delete rules_listbox->takeItem(pos); 112​ delete rules_listbox->takeItem(pos);
114 rules.erase(rules.begin() + pos); 113​ m_rules.erase(m_rules.begin() + pos);
115 emit changed(true); 114​ emit changed(true);
116} 115​}
117 116​
118void KCMRulesList::moveupClicked() 117​void KCMRulesList::moveupClicked()
119{ 118​{
120 int pos = rules_listbox->currentRow(); 119​ int pos = rules_listbox->currentRow();
121 Q_ASSERT(pos != -1); 120​ Q_ASSERT(pos != -1);
122 if (pos > 0) { 121​ if (pos > 0) {
123 QListWidgetItem * item = rules_listbox->takeItem(pos); 122​ QListWidgetItem * item = rules_listbox->takeItem(pos);
124 rules_listbox->insertItem(pos - 1 , item); 123​ rules_listbox->insertItem(pos - 1 , item);
125 rules_listbox->setCurrentItem(item, QItemSelectionModel::ClearAndSelect); 124​ rules_listbox->setCurrentItem(item, QItemSelectionModel::ClearAndSelect);
126 Rules* rule = rules[ pos ]; 125​ Rules *rule = m_rules[pos];
127 rules[ pos ] = rules[ pos - 1 ]; 126​ m_rules[pos] = m_rules[pos - 1];
128 rules[ pos - 1 ] = rule; 127​ m_rules[pos - 1] = rule;
129 } 128​ }
130 emit changed(true); 129​ emit changed(true);
131} 130​}
132 131​
133void KCMRulesList::movedownClicked() 132​void KCMRulesList::movedownClicked()
134{ 133​{
135 int pos = rules_listbox->currentRow(); 134​ int pos = rules_listbox->currentRow();
136 Q_ASSERT(pos != -1); 135​ Q_ASSERT(pos != -1);
137 if (pos < int(rules_listbox->count()) - 1) { 136​ if (pos < int(rules_listbox->count()) - 1) {
138 QListWidgetItem * item = rules_listbox->takeItem(pos); 137​ QListWidgetItem * item = rules_listbox->takeItem(pos);
139 rules_listbox->insertItem(pos + 1 , item); 138​ rules_listbox->insertItem(pos + 1 , item);
140 rules_listbox->setCurrentItem(item, QItemSelectionModel::ClearAndSelect); 139​ rules_listbox->setCurrentItem(item, QItemSelectionModel::ClearAndSelect);
141 Rules* rule = rules[ pos ]; 140​ Rules *rule = m_rules[pos];
142 rules[ pos ] = rules[ pos + 1 ]; 141​ m_rules[pos] = m_rules[pos + 1];
143 rules[ pos + 1 ] = rule; 142​ m_rules[pos + 1] = rule;
144 } 143​ }
145 emit changed(true); 144​ emit changed(true);
146} 145​}
147 146​
148void KCMRulesList::exportClicked() 147​void KCMRulesList::exportClicked()
149{ 148​{
150 int pos = rules_listbox->currentRow(); 149​ int pos = rules_listbox->currentRow();
151 Q_ASSERT(pos != -1); 150​ Q_ASSERT(pos != -1);
152 QString path = QFileDialog::getSaveFileName(this, i18n("Export Rules"), QDir::home().absolutePath(), 151​ QString path = QFileDialog::getSaveFileName(this, i18n("Export Rules"), QDir::home().absolutePath(),
153 i18n("KWin Rules (*.kwinrule)")); 152​ i18n("KWin Rules (*.kwinrule)"));
154 if (path.isEmpty()) 153​ if (path.isEmpty())
155 return; 154​ return;
156 KConfig config(path, KConfig::SimpleConfig); 155​ const auto cfg = KSharedConfig::openConfig(path, KConfig::SimpleConfig);
157 KConfigGroup group(&config, rules[pos]->description); 156​ RuleSettings settings(cfg, m_rules[pos]->description);
158 group.deleteGroup(); 157​ settings.setDefaults();
159 rules[pos]->write(group); 158​ m_rules[pos]->write(settings);
160} 159​}
161 160​
162void KCMRulesList::importClicked() 161​void KCMRulesList::importClicked()
163{ 162​{
164 QString path = QFileDialog::getOpenFileName(this, i18n("Import Rules"), QDir::home().absolutePath(), 163​ QString path = QFileDialog::getOpenFileName(this, i18n("Import Rules"), QDir::home().absolutePath(),
165 i18n("KWin Rules (*.kwinrule)")); 164​ i18n("KWin Rules (*.kwinrule)"));
166 if (path.isEmpty()) 165​ if (path.isEmpty())
167 return; 166​ return;
168 KConfig config(path, KConfig::SimpleConfig); 167​ const auto config = KSharedConfig::openConfig(path, KConfig::SimpleConfig);
169 QStringList groups = config.groupList(); 168​ QStringList groups = config->groupList();
170 if (groups.isEmpty()) 169​ if (groups.isEmpty())
171 return; 170​ return;
172 171​
173 int pos = qMax(0, rules_listbox->currentRow()); 172​ int pos = qMax(0, rules_listbox->currentRow());
174 foreach (const QString &group, groups) { 173​ for (const QString &group : groups) {
175 KConfigGroup grp(&config, group); 174​ RuleSettings settings(config, group);
176 const bool remove = grp.readEntry("DeleteRule", false); 175​ const bool remove = settings.deleteRule();
177 Rules* new_rule = new Rules(grp); 176​ Rules *new_rule = new Rules(settings);
178 177​
179 // try to replace existing rule first 178​ // try to replace existing rule first
180 for (int i = 0; i < rules.count(); ++i) { 179​ for (int i = 0; i < m_rules.count(); ++i) {
181 if (rules[i]->description == new_rule->description) { 180​ if (m_rules[i]->description == new_rule->description) {
182 delete rules[i]; 181​ delete m_rules[i];
183 if (remove) { 182​ if (remove) {
184 rules.remove(i); 183​ m_rules.remove(i);
185 delete rules_listbox->takeItem(i); 184​ delete rules_listbox->takeItem(i);
186 delete new_rule; 185​ delete new_rule;
187 pos = qMax(0, rules_listbox->currentRow()); // might have changed! 186​ pos = qMax(0, rules_listbox->currentRow()); // might have changed!
188 } 187​ } else
189 else 188​ m_rules[i] = new_rule;
190 rules[i] = new_rule; ​
191 new_rule = nullptr; 189​ new_rule = nullptr;
192 break; 190​ break;
193 } 191​ }
194 } 192​ }
195 193​
196 // don't add "to be deleted" if not present 194​ // don't add "to be deleted" if not present
197 if (remove) { 195​ if (remove) {
198 delete new_rule; 196​ delete new_rule;
199 new_rule = nullptr; 197​ new_rule = nullptr;
200 } 198​ }
201 199​
202 // plain insertion 200​ // plain insertion
203 if (new_rule) { 201​ if (new_rule) {
204 rules.insert(pos, new_rule); 202​ m_rules.insert(pos, new_rule);
205 rules_listbox->insertItem(pos++, new_rule->description); 203​ rules_listbox->insertItem(pos++, new_rule->description);
206 } 204​ }
207 } 205​ }
208 emit changed(true); 206​ emit changed(true);
209} 207​}
210 208​
211void KCMRulesList::load() 209​void KCMRulesList::load()
212{ 210​{
213 rules_listbox->clear(); 211​ rules_listbox->clear();
214 for (QVector< Rules* >::Iterator it = rules.begin(); 212​ m_settings.loadAllInto(m_rules);
215 it != rules.end(); 213​ for (const auto rule : m_rules) {
ervinUnsubmitted
Not Done

Would need a qAsConst around m_rules

ervin: Would need a qAsConst around m_rules
ervinUnsubmitted
Not Done

With my proposal it'd become:

m_settings->load();
m_rules = m_settings->rules();
ervin: With my proposal it'd become: ``` m_settings->load(); m_rules = m_settings->rules(); ```
216 ++it) ​
217 delete *it; ​
218 rules.clear(); ​
219 KConfig _cfg("kwinrulesrc"); ​
220 KConfigGroup cfg(&_cfg, "General"); ​
221 int count = cfg.readEntry("count", 0); ​
222 rules.reserve(count); ​
223 for (int i = 1; ​
224 i <= count; ​
225 ++i) { ​
226 cfg = KConfigGroup(&_cfg, QString::number(i)); ​
227 Rules* rule = new Rules(cfg); ​
228 rules.append(rule); ​
229 rules_listbox->addItem(rule->description); 214​ rules_listbox->addItem(rule->description);
230 } 215​ }
231 if (rules.count() > 0) 216​
217​ if (m_rules.count() > 0)
232 rules_listbox->setCurrentItem(rules_listbox->item(0)); 218​ rules_listbox->setCurrentItem(rules_listbox->item(0));
233 else 219​ else
234 rules_listbox->setCurrentItem(nullptr); 220​ rules_listbox->setCurrentItem(nullptr);
235 activeChanged(); 221​ activeChanged();
236} 222​}
237 223​
238void KCMRulesList::save() 224​void KCMRulesList::save()
239{ 225​{
240 KConfig cfg(QLatin1String("kwinrulesrc")); 226​ m_settings.saveAllFrom(m_rules);
ervinUnsubmitted
Not Done

With my proposal it'd become:

m_settings->setRules(m_rules);
m_settings->save();
ervin: With my proposal it'd become: ``` m_settings->setRules(m_rules); m_settings->save(); ```
241 QStringList groups = cfg.groupList(); ​
242 for (QStringList::ConstIterator it = groups.constBegin(); ​
243 it != groups.constEnd(); ​
244 ++it) ​
245 cfg.deleteGroup(*it); ​
246 cfg.group("General").writeEntry("count", rules.count()); ​
247 int i = 1; ​
248 for (QVector< Rules* >::ConstIterator it = rules.constBegin(); ​
249 it != rules.constEnd(); ​
250 ++it) { ​
251 KConfigGroup cg(&cfg, QString::number(i)); ​
252 (*it)->write(cg); ​
253 ++i; ​
254 } ​
255} 227​}
256 228​
257void KCMRulesList::defaults() 229​void KCMRulesList::defaults()
258{ 230​{
259 load(); 231​ load();
260} 232​}
261 233​
262} // namespace 234​} // namespace
263 235​