Allow ManagedConfigModule derived class to register explicitly KCoreConfigSkeleton.
ClosedPublic

Authored by crossi on Dec 16 2019, 3:42 PM.

Details

Summary

Helpful for derived class that do not store KConfig settings as children items.

Test Plan

Should not break binary compatibility. Same behaviour as before.

Diff Detail

Repository
R296 KDeclarative
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
crossi created this revision.Dec 16 2019, 3:42 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptDec 16 2019, 3:42 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
crossi requested review of this revision.Dec 16 2019, 3:42 PM
meven accepted this revision.Dec 16 2019, 4:19 PM
meven added a subscriber: meven.

Code looks sane to me.
One nitpick

src/quickaddons/managedconfigmodule.h
205

Mention importance of emitting configChanged signal to automatically call settingsChanged as alternative to calling settingsChanged.

This revision is now accepted and ready to land.Dec 16 2019, 4:19 PM

Generally +1

src/quickaddons/managedconfigmodule.cpp
43

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

meven resigned from this revision.Dec 16 2019, 4:57 PM

Leave the first reviewer position to someone else.

This revision now requires review to proceed.Dec 16 2019, 4:57 PM
crossi added inline comments.Dec 17 2019, 7:57 AM
src/quickaddons/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.

crossi added inline comments.Dec 17 2019, 12:33 PM
src/quickaddons/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.

ervin requested changes to this revision.Dec 23 2019, 12:15 PM
ervin added inline comments.
src/quickaddons/managedconfigmodule.cpp
43

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.

70–71

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

79–80

ditto

88–89

ditto

src/quickaddons/managedconfigmodule.h
205

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?

207

Space before * not after

This revision now requires changes to proceed.Dec 23 2019, 12:15 PM
ervin added inline comments.Jan 8 2020, 4:37 PM
src/quickaddons/managedconfigmodule.h
206

Now needs @since 5.67

crossi updated this revision to Diff 73411.Jan 13 2020, 2:24 PM

API documentation. Schedule settingsChanged(). Clean up the list. Code style.

ervin requested changes to this revision.Jan 20 2020, 1:36 PM
ervin added inline comments.
src/quickaddons/managedconfigmodule.cpp
175

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

This revision now requires changes to proceed.Jan 20 2020, 1:36 PM
crossi abandoned this revision.Jan 22 2020, 9:40 AM
ervin added a comment.Jan 22 2020, 9:45 AM

Well, even though you didn't have a direct use anymore it doesn't mean this patch was useless. I think it'd be worth fixing the last issue I pointed out and merging it. I seem to remember someone else requested this feature.

crossi reclaimed this revision.Jan 22 2020, 9:59 AM
This revision now requires changes to proceed.Jan 22 2020, 9:59 AM
crossi planned changes to this revision.Jan 22 2020, 9:59 AM
crossi updated this revision to Diff 74125.Jan 22 2020, 2:57 PM

Clean the list properly

ervin requested changes to this revision.Jan 22 2020, 3:29 PM

Just a tiny nitpick left.

src/quickaddons/managedconfigmodule.cpp
175

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.

This revision now requires changes to proceed.Jan 22 2020, 3:29 PM
crossi updated this revision to Diff 74128.Jan 22 2020, 3:35 PM

code style

ervin accepted this revision.Jan 22 2020, 4:11 PM
This revision is now accepted and ready to land.Jan 22 2020, 4:11 PM
bport accepted this revision.Wed, Feb 5, 2:02 PM
bport added inline comments.Wed, Feb 5, 2:09 PM
src/quickaddons/managedconfigmodule.cpp
145–146

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

bport accepted this revision.Wed, Feb 5, 2:13 PM
bport added inline comments.
src/quickaddons/managedconfigmodule.cpp
145–146

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

This revision was automatically updated to reflect the committed changes.