Differential D26046 Diff 73411 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 10 Lines
60} 62​}
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) {
110​ _q->registerSettings(skeleton);
111​ }
112​}
113​
114​void ManagedConfigModule::settingsChanged()
115​{
116​ bool needsSave = false;
117​ bool representsDefaults = true;
118​ for (const auto &skeleton : qAsConst(d->_skeletons)) {
119​ if (skeleton) {
120​ needsSave |= skeleton->isSaveNeeded();
121​ representsDefaults &= skeleton->isDefaults();
122​ }
123​ }
102 124​
103 _skeletons = _q->findChildren<KCoreConfigSkeleton*>(); 125​ if (!needsSave) {
104 for (auto skeleton : qAsConst(_skeletons)) { 126​ needsSave = isSaveNeeded();
105 QObject::connect(skeleton, &KCoreConfigSkeleton::configChanged, _q, &ManagedConfigModule::settingsChanged); 127​ }
128​
129​ if (representsDefaults) {
130​ representsDefaults = isDefaults();
131​ }
132​
133​ setRepresentsDefaults(representsDefaults);
134​ setNeedsSave(needsSave);
135​}
136​
137​void ManagedConfigModule::registerSettings(KCoreConfigSkeleton *skeleton)
138​{
139​ if (!skeleton || d->_skeletons.contains(skeleton)) {
140​ return;
141​ }
142​
143​ d->_skeletons.append(skeleton);
144​
145​ auto settingsChangedSlotIndex = metaObject()->indexOfMethod("settingsChanged()");
146​ 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
147​
148​ QObject::connect(skeleton, &KCoreConfigSkeleton::configChanged, this, &ManagedConfigModule::settingsChanged);
106 149​
107 const auto items = skeleton->items(); 150​ const auto items = skeleton->items();
108 for (auto item : items) { 151​ for (auto item : items) {
109 auto signallingItem = dynamic_cast<KConfigCompilerSignallingItem*>(item); 152​ auto signallingItem = dynamic_cast<KConfigCompilerSignallingItem*>(item);
110 if (!signallingItem) { 153​ if (!signallingItem) {
111 continue; 154​ continue;
112 } 155​ }
113 156​
114 QString name = signallingItem->name(); 157​ QString name = signallingItem->name();
115 if (name.at(0).isUpper()) 158​ if (name.at(0).isUpper()) {
116 name[0] = name[0].toLower(); 159​ name[0] = name[0].toLower();
160​ }
117 161​
118 const auto metaObject = skeleton->metaObject(); 162​ const auto metaObject = skeleton->metaObject();
119 const auto propertyIndex = metaObject->indexOfProperty(name.toUtf8().constData()); 163​ const auto propertyIndex = metaObject->indexOfProperty(name.toUtf8().constData());
120 const auto property = metaObject->property(propertyIndex); 164​ const auto property = metaObject->property(propertyIndex);
121 if (!property.hasNotifySignal()) { 165​ if (!property.hasNotifySignal()) {
122 continue; 166​ continue;
123 } 167​ }
124 168​
125 const auto changedSignal = property.notifySignal(); 169​ const auto changedSignal = property.notifySignal();
126 QObject::connect(skeleton, changedSignal, _q, settingsChangedSlot); 170​ QObject::connect(skeleton, changedSignal, this, settingsChangedSlot);
127 } ​
128 } 171​ }
129 172​
130 _q->settingsChanged(); 173​ for (auto it = d->_skeletons.begin(); it != d->_skeletons.end(); ++it) {
174​ if (it->isNull()) {
175​ d->_skeletons.erase(it);
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…
131} 176​ }
132 ​
133void ManagedConfigModule::settingsChanged() ​
134{ ​
135 bool needsSave = false; ​
136 bool representsDefaults = true; ​
137 for (const auto skeleton : qAsConst(d->_skeletons)) { ​
138 needsSave |= skeleton->isSaveNeeded(); ​
139 representsDefaults &= skeleton->isDefaults(); ​
140 } ​
141 ​
142 if (!needsSave) { ​
143 needsSave = isSaveNeeded(); ​
144 } 177​ }
145 178​
146 if (representsDefaults) { 179​ QMetaObject::invokeMethod(this, "settingsChanged", Qt::QueuedConnection);
147 representsDefaults = isDefaults(); ​
148 } ​
149 ​
150 setRepresentsDefaults(representsDefaults); ​
151 setNeedsSave(needsSave); ​
152} 180​}
153 181​
154} 182​}
155 183​
156#include "moc_managedconfigmodule.cpp" 184​#include "moc_managedconfigmodule.cpp"
157 185​