D26046: Allow ManagedConfigModule derived class to register explicitly KCoreConfigSkeleton.

[Kevin Ottens]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20191223/b4dfd09a/attachment.html>