Differential D26046 Diff 71673 src/quickaddons/managedconfigmodule.cpp

Changeset View

Standalone View

View Options

src/quickaddons/managedconfigmodule.cpp

Show All 16 Lines1/*
17 along with this library; see the file COPYING.LIB. If not, write to 17​ along with this library; see the file COPYING.LIB. If not, write to
18 the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, 18​ the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
19 Boston, MA 02110-1301, USA. 19​ Boston, MA 02110-1301, USA.
20 20​
21*/ 21​*/
22 22​
23#include "managedconfigmodule.h" 23​#include "managedconfigmodule.h"
24 24​
25​#include <QPointer>
26​
25#include <KConfigCore/KCoreConfigSkeleton> 27​#include <KConfigCore/KCoreConfigSkeleton>
26 28​
27namespace KQuickAddons { 29​namespace KQuickAddons {
28 30​
29class ManagedConfigModulePrivate 31​class ManagedConfigModulePrivate
30{ 32​{
31public: 33​public:
32 ManagedConfigModulePrivate(ManagedConfigModule *module): 34​ ManagedConfigModulePrivate(ManagedConfigModule *module):
33 _q(module) 35​ _q(module)
34 { 36​ {
35 QMetaObject::invokeMethod(_q, "_k_registerSettings", Qt::QueuedConnection); 37​ QMetaObject::invokeMethod(_q, "_k_registerSettings", Qt::QueuedConnection);
36 } 38​ }
37 39​
38 void _k_registerSettings(); 40​ void _k_registerSettings();
39 41​
40 ManagedConfigModule *_q; 42​ ManagedConfigModule *_q;
41 QList<KCoreConfigSkeleton*> _skeletons; 43​ QList<QPointer<KCoreConfigSkeleton>> _skeletons;
davidedmundsonUnsubmitted
Not Done

Any reason for doing this approach rather than connecting to QObject:: destroyed and cleaning the list as we go?

davidedmundson: Any reason for doing this approach rather than connecting to QObject:: destroyed and cleaning…
crossiAuthorUnsubmitted
Not Done

Sort of a guard if one registers manually a KCoreConfigSkeleton and then deallocate it.
It is not intended to manage the deallocation as KCoreConfigSkeleton are normally registered in the QObject hierarchy.

crossi: Sort of a guard if one registers manually a KCoreConfigSkeleton and then deallocate it. It is…
ervinUnsubmitted
Not Done

I think that was David's point. If the object is deallocated it'll emit the destroyed signal before passing away. By connecting to that signal you could remove the pointer from the list (instead of having a list potentially containing null pointers like now). Personally I don't mind much between both approaches though. QPointer generally leads to less code, you just have to be aware of the null case like for any weak reference pointer.

Still you never clean up that list, it shouldn't grow large, but I'd remove the null pointers from time to time. Possibly at the next registerSettings call or so.

ervin: I think that was David's point. If the object is deallocated it'll emit the destroyed signal…
42}; 44​};
43 45​
44ManagedConfigModule::ManagedConfigModule(const KAboutData *aboutData, QObject *parent, const QVariantList &args) 46​ManagedConfigModule::ManagedConfigModule(const KAboutData *aboutData, QObject *parent, const QVariantList &args)
45 : ConfigModule(aboutData, parent, args), 47​ : ConfigModule(aboutData, parent, args),
46 d(new ManagedConfigModulePrivate(this)) 48​ d(new ManagedConfigModulePrivate(this))
47{ 49​{
48} 50​}
49 51​
Show All 11 Lines
61 63​
62ManagedConfigModule::~ManagedConfigModule() 64​ManagedConfigModule::~ManagedConfigModule()
63{ 65​{
64 delete d; 66​ delete d;
65} 67​}
66 68​
67void ManagedConfigModule::load() 69​void ManagedConfigModule::load()
68{ 70​{
69 for (const auto skeleton : qAsConst(d->_skeletons)) { 71​ for (const auto skeleton : qAsConst(d->_skeletons)) {
ervinUnsubmitted
Not Done

Should be const auto &skeleton now that it's not a simple raw pointer anymore.

ervin: Should be const auto &skeleton now that it's not a simple raw pointer anymore.
72​ if (skeleton) {
70 skeleton->load(); 73​ skeleton->load();
71 } 74​ }
72} 75​ }
76​}
73 77​
74void ManagedConfigModule::save() 78​void ManagedConfigModule::save()
75{ 79​{
76 for (const auto skeleton : qAsConst(d->_skeletons)) { 80​ for (const auto skeleton : qAsConst(d->_skeletons)) {
ervinUnsubmitted
Not Done

ditto

ervin: ditto
81​ if (skeleton) {
77 skeleton->save(); 82​ skeleton->save();
78 } 83​ }
79} 84​ }
85​}
80 86​
81void ManagedConfigModule::defaults() 87​void ManagedConfigModule::defaults()
82{ 88​{
83 for (const auto skeleton : qAsConst(d->_skeletons)) { 89​ for (const auto skeleton : qAsConst(d->_skeletons)) {
ervinUnsubmitted
Not Done

ditto

ervin: ditto
90​ if (skeleton) {
84 skeleton->setDefaults(); 91​ skeleton->setDefaults();
85 } 92​ }
86} 93​ }
94​}
87 95​
88bool ManagedConfigModule::isSaveNeeded() const 96​bool ManagedConfigModule::isSaveNeeded() const
89{ 97​{
90 return false; 98​ return false;
91} 99​}
92 100​
93bool ManagedConfigModule::isDefaults() const 101​bool ManagedConfigModule::isDefaults() const
94{ 102​{
95 return true; 103​ return true;
96} 104​}
97 105​
98void ManagedConfigModulePrivate::_k_registerSettings() 106​void ManagedConfigModulePrivate::_k_registerSettings()
99{ 107​{
100 auto settingsChangedSlotIndex = _q->metaObject()->indexOfMethod("settingsChanged()"); 108​ const auto skeletons = _q->findChildren<KCoreConfigSkeleton*>();
101 auto settingsChangedSlot = _q->metaObject()->method(settingsChangedSlotIndex); 109​ for (auto skeleton : skeletons) {
102 110​ _q->registerSettings(skeleton);
103 _skeletons = _q->findChildren<KCoreConfigSkeleton*>(); ​
104 for (auto skeleton : qAsConst(_skeletons)) { ​
105 QObject::connect(skeleton, &KCoreConfigSkeleton::configChanged, _q, &ManagedConfigModule::settingsChanged); ​
106 ​
107 const auto items = skeleton->items(); ​
108 for (auto item : items) { ​
109 auto signallingItem = dynamic_cast<KConfigCompilerSignallingItem*>(item); ​
110 if (!signallingItem) { ​
111 continue; ​
112 } ​
113 ​
114 QString name = signallingItem->name(); ​
115 if (name.at(0).isUpper()) ​
116 name[0] = name[0].toLower(); ​
117 ​
118 const auto metaObject = skeleton->metaObject(); ​
119 const auto propertyIndex = metaObject->indexOfProperty(name.toUtf8().constData()); ​
120 const auto property = metaObject->property(propertyIndex); ​
121 if (!property.hasNotifySignal()) { ​
122 continue; ​
123 } ​
124 ​
125 const auto changedSignal = property.notifySignal(); ​
126 QObject::connect(skeleton, changedSignal, _q, settingsChangedSlot); ​
127 } ​
128 } 111​ }
129 112​
130 _q->settingsChanged(); 113​ _q->settingsChanged();
131} 114​}
132 115​
133void ManagedConfigModule::settingsChanged() 116​void ManagedConfigModule::settingsChanged()
134{ 117​{
135 bool needsSave = false; 118​ bool needsSave = false;
136 bool representsDefaults = true; 119​ bool representsDefaults = true;
137 for (const auto skeleton : qAsConst(d->_skeletons)) { 120​ for (const auto skeleton : qAsConst(d->_skeletons)) {
121​ if (skeleton) {
138 needsSave |= skeleton->isSaveNeeded(); 122​ needsSave |= skeleton->isSaveNeeded();
139 representsDefaults &= skeleton->isDefaults(); 123​ representsDefaults &= skeleton->isDefaults();
140 } 124​ }
125​ }
141 126​
142 if (!needsSave) { 127​ if (!needsSave) {
143 needsSave = isSaveNeeded(); 128​ needsSave = isSaveNeeded();
144 } 129​ }
145 130​
146 if (representsDefaults) { 131​ if (representsDefaults) {
147 representsDefaults = isDefaults(); 132​ representsDefaults = isDefaults();
148 } 133​ }
149 134​
150 setRepresentsDefaults(representsDefaults); 135​ setRepresentsDefaults(representsDefaults);
151 setNeedsSave(needsSave); 136​ setNeedsSave(needsSave);
152} 137​}
153 138​
139​void ManagedConfigModule::registerSettings(KCoreConfigSkeleton *skeleton)
140​{
141​ if (!skeleton || d->_skeletons.contains(skeleton)) {
142​ return;
143​ }
144​
145​ d->_skeletons.append(skeleton);
146​
147​ auto settingsChangedSlotIndex = metaObject()->indexOfMethod("settingsChanged()");
148​ auto settingsChangedSlot = metaObject()->method(settingsChangedSlotIndex);
bportUnsubmitted
Not Done

Those calls are done for each skeleton perhaps we can do only one call

bport: Those calls are done for each skeleton perhaps we can do only one call
bportUnsubmitted
Not Done

registerSettings is not on the hot path, so it will be ok

bport: registerSettings is not on the hot path, so it will be ok
149​
150​ QObject::connect(skeleton, &KCoreConfigSkeleton::configChanged, this, &ManagedConfigModule::settingsChanged);
151​
152​ const auto items = skeleton->items();
153​ for (auto item : items) {
154​ auto signallingItem = dynamic_cast<KConfigCompilerSignallingItem*>(item);
155​ if (!signallingItem) {
156​ continue;
157​ }
158​
159​ QString name = signallingItem->name();
160​ if (name.at(0).isUpper()) {
161​ name[0] = name[0].toLower();
162​ }
163​
164​ const auto metaObject = skeleton->metaObject();
165​ const auto propertyIndex = metaObject->indexOfProperty(name.toUtf8().constData());
166​ const auto property = metaObject->property(propertyIndex);
167​ if (!property.hasNotifySignal()) {
168​ continue;
169​ }
170​
171​ const auto changedSignal = property.notifySignal();
172​ QObject::connect(skeleton, changedSignal, this, settingsChangedSlot);
173​ }
174​}
175​
154} 176​}
155 177​
ervinUnsubmitted
Not Done

Careful there you should use the return value of erase(). It invalidates iterators so it's probably working just by change here (I'd suspect it might crash for instance if it happens to remove the last one in the list).

Even better yet you could spare that loop completely and use remove_if + erase, see:
https://en.wikipedia.org/wiki/Erase-remove_idiom

ervin: Careful there you should use the return value of erase(). It invalidates iterators so it's…
ervinUnsubmitted
Not Done

Nitpicking but in function call we generally don't have commas at the start of line but at the end of the line before. Also there should be a space after the closing parenthesis of the lambda declaration.

ervin: Nitpicking but in function call we generally don't have commas at the start of line but at the…
156#include "moc_managedconfigmodule.cpp" 178​#include "moc_managedconfigmodule.cpp"
157 179​