ervin requested changes to this revision.
ervin added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> crossi wrote in managedconfigmodule.cpp:43
> 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.
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.
> managedconfigmodule.cpp:71
> {
> for (const auto skeleton : qAsConst(d->_skeletons)) {
> + if (skeleton) {
Should be const auto &skeleton now that it's not a simple raw pointer anymore.
> managedconfigmodule.cpp:80
> {
> for (const auto skeleton : qAsConst(d->_skeletons)) {
> + if (skeleton) {
ditto
> managedconfigmodule.cpp:89
> {
> for (const auto skeleton : qAsConst(d->_skeletons)) {
> + if (skeleton) {
ditto
> crossi wrote in managedconfigmodule.h:205
> It is not an alternative, configChanged is a KCoreConfigSkeleton's signal
> that will trigger a settingsChanged.
> The proper way to use it, is register several settings (KCoreConfigSkeleton),
> then call settingsChanged that will perform a check on all registered
> settings.
It may be needed? Or it is mandatory to call settingsChanged() for proper
behavior?
I'm wondering because I'm tempted to say this shall not be necessary and we
should perhaps schedule a settingsChanged() call instead when we go back in the
event loop?
> managedconfigmodule.h:207
> + */
> + void registerSettings(KCoreConfigSkeleton* skeleton);
> +
Space before * not after
REPOSITORY
R296 KDeclarative
REVISION DETAIL
https://phabricator.kde.org/D26046
To: crossi, #plasma, #frameworks, ervin, bport, davidedmundson, mart
Cc: meven, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
