D26046: Allow ManagedConfigModule derived class to register explicitly KCoreConfigSkeleton.
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
D26046: Allow ManagedConfigModule derived class to register explicitly KCoreConfigSkeleton.
ervin added inline comments. INLINE COMMENTS > managedconfigmodule.h:206 > + * it may be needed to call settingsChanged() > + */ > +void registerSettings(KCoreConfigSkeleton* skeleton); Now needs @since 5.67 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
D26046: Allow ManagedConfigModule derived class to register explicitly KCoreConfigSkeleton.
crossi updated this revision to Diff 73411. crossi added a comment. API documentation. Schedule settingsChanged(). Clean up the list. Code style. REPOSITORY R296 KDeclarative CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D26046?vs=71673&id=73411 REVISION DETAIL https://phabricator.kde.org/D26046 AFFECTED FILES src/quickaddons/managedconfigmodule.cpp src/quickaddons/managedconfigmodule.h To: crossi, #plasma, #frameworks, ervin, bport, davidedmundson, mart Cc: meven, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D26046: Allow ManagedConfigModule derived class to register explicitly KCoreConfigSkeleton.
ervin requested changes to this revision.
ervin added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> managedconfigmodule.cpp:175
> +if (it->isNull()) {
> +d->_skeletons.erase(it);
> +}
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
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
D26046: Allow ManagedConfigModule derived class to register explicitly KCoreConfigSkeleton.
crossi abandoned this revision. 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
D26046: Allow ManagedConfigModule derived class to register explicitly KCoreConfigSkeleton.
ervin added a comment. 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. 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
D26046: Allow ManagedConfigModule derived class to register explicitly KCoreConfigSkeleton.
crossi reclaimed this revision. This revision now requires changes to proceed. 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
D26046: Allow ManagedConfigModule derived class to register explicitly KCoreConfigSkeleton.
crossi planned changes to this revision. 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
D26046: Allow ManagedConfigModule derived class to register explicitly KCoreConfigSkeleton.
crossi updated this revision to Diff 74125. crossi added a comment. Clean the list properly REPOSITORY R296 KDeclarative CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D26046?vs=73411&id=74125 REVISION DETAIL https://phabricator.kde.org/D26046 AFFECTED FILES src/quickaddons/managedconfigmodule.cpp src/quickaddons/managedconfigmodule.h To: crossi, #plasma, #frameworks, ervin, bport, davidedmundson, mart Cc: meven, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D26046: Allow ManagedConfigModule derived class to register explicitly KCoreConfigSkeleton.
ervin requested changes to this revision.
ervin added a comment.
This revision now requires changes to proceed.
Just a tiny nitpick left.
INLINE COMMENTS
> managedconfigmodule.cpp:175
> + , d->_skeletons.end()
> + , [](const QPointer
> &value){ return value.isNull(); });
> +d->_skeletons.erase(toRemove, d->_skeletons.end());
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.
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
D26046: Allow ManagedConfigModule derived class to register explicitly KCoreConfigSkeleton.
crossi updated this revision to Diff 74128. crossi added a comment. code style REPOSITORY R296 KDeclarative CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D26046?vs=74125&id=74128 REVISION DETAIL https://phabricator.kde.org/D26046 AFFECTED FILES src/quickaddons/managedconfigmodule.cpp src/quickaddons/managedconfigmodule.h To: crossi, #plasma, #frameworks, ervin, bport, davidedmundson, mart Cc: meven, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D26046: Allow ManagedConfigModule derived class to register explicitly KCoreConfigSkeleton.
ervin accepted this revision. This revision is now accepted and ready to land. 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
D26046: Allow ManagedConfigModule derived class to register explicitly KCoreConfigSkeleton.
bport accepted this revision. 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
D26046: Allow ManagedConfigModule derived class to register explicitly KCoreConfigSkeleton.
bport added inline comments.
INLINE COMMENTS
> managedconfigmodule.cpp:145-146
> +
> +auto settingsChangedSlotIndex =
> metaObject()->indexOfMethod("settingsChanged()");
> +auto settingsChangedSlot =
> metaObject()->method(settingsChangedSlotIndex);
> +
Those calls are done for each skeleton perhaps we can do only one call
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
D26046: Allow ManagedConfigModule derived class to register explicitly KCoreConfigSkeleton.
crossi updated this revision to Diff 75052. crossi added a comment. rebase REPOSITORY R296 KDeclarative CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D26046?vs=74128&id=75052 REVISION DETAIL https://phabricator.kde.org/D26046 AFFECTED FILES src/quickaddons/managedconfigmodule.cpp src/quickaddons/managedconfigmodule.h To: crossi, #plasma, #frameworks, ervin, bport, davidedmundson, mart Cc: meven, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D26046: Allow ManagedConfigModule derived class to register explicitly KCoreConfigSkeleton.
bport accepted this revision. bport added inline comments. INLINE COMMENTS > bport wrote in managedconfigmodule.cpp:145-146 > Those calls are done for each skeleton perhaps we can do only one call registerSettings is not on the hot path, so it will be ok 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
D26046: Allow ManagedConfigModule derived class to register explicitly KCoreConfigSkeleton.
This revision was automatically updated to reflect the committed changes. Closed by commit R296:7c4f46aded18: Allow ManagedConfigModule derived class to register explicitly… (authored by crossi). REPOSITORY R296 KDeclarative CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D26046?vs=75052&id=75053 REVISION DETAIL https://phabricator.kde.org/D26046 AFFECTED FILES src/quickaddons/managedconfigmodule.cpp src/quickaddons/managedconfigmodule.h To: crossi, #plasma, #frameworks, ervin, bport, davidedmundson, mart Cc: meven, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D26046: Allow ManagedConfigModule derived class to register explicitly KCoreConfigSkeleton.
crossi created this revision. crossi added reviewers: Plasma, Frameworks, ervin, bport, davidedmundson, mart. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. crossi requested review of this revision. REVISION 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. REPOSITORY R296 KDeclarative REVISION DETAIL https://phabricator.kde.org/D26046 AFFECTED FILES src/quickaddons/managedconfigmodule.cpp src/quickaddons/managedconfigmodule.h To: crossi, #plasma, #frameworks, ervin, bport, davidedmundson, mart Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D26046: Allow ManagedConfigModule derived class to register explicitly KCoreConfigSkeleton.
meven accepted this revision. meven added a comment. This revision is now accepted and ready to land. Code looks sane to me. One nitpick INLINE COMMENTS > managedconfigmodule.h:205 > + * After manual registration on the fly, > + * it may be needed to call settingsChanged() > + */ Mention importance of emitting configChanged signal to automatically call settingsChanged as alternative to calling settingsChanged. REPOSITORY R296 KDeclarative REVISION DETAIL https://phabricator.kde.org/D26046 To: crossi, #plasma, #frameworks, ervin, bport, davidedmundson, mart, meven Cc: meven, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D26046: Allow ManagedConfigModule derived class to register explicitly KCoreConfigSkeleton.
davidedmundson added a comment. Generally +1 INLINE COMMENTS > managedconfigmodule.cpp:43 > ManagedConfigModule *_q; > -QList _skeletons; > +QList> _skeletons; > }; Any reason for doing this approach rather than connecting to QObject:: destroyed and cleaning the list as we go? REPOSITORY R296 KDeclarative REVISION DETAIL https://phabricator.kde.org/D26046 To: crossi, #plasma, #frameworks, ervin, bport, davidedmundson, mart, meven Cc: meven, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D26046: Allow ManagedConfigModule derived class to register explicitly KCoreConfigSkeleton.
meven resigned from this revision. meven added a comment. This revision now requires review to proceed. Leave the first reviewer position to someone else. 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
D26046: Allow ManagedConfigModule derived class to register explicitly KCoreConfigSkeleton.
crossi added inline comments. INLINE COMMENTS > davidedmundson wrote in managedconfigmodule.cpp:43 > Any reason for doing this approach rather than connecting to QObject:: > destroyed and cleaning the list as we go? 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. 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
D26046: Allow ManagedConfigModule derived class to register explicitly KCoreConfigSkeleton.
crossi added inline comments. INLINE COMMENTS > meven wrote in managedconfigmodule.h:205 > Mention importance of emitting configChanged signal to automatically call > settingsChanged as alternative to calling settingsChanged. 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. 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
