Helpful for derived class that do not store KConfig settings as children items.
Details
- Reviewers
ervin bport davidedmundson mart meven - Group Reviewers
Plasma Frameworks - Commits
- R296:7c4f46aded18: Allow ManagedConfigModule derived class to register explicitly…
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.
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. |
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? |
src/quickaddons/managedconfigmodule.cpp | ||
---|---|---|
43 | Sort of a guard if one registers manually a KCoreConfigSkeleton and then deallocate it. |
src/quickaddons/managedconfigmodule.h | ||
---|---|---|
205 | It is not an alternative, configChanged is a KCoreConfigSkeleton's signal that will trigger a settingsChanged. |
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 |
src/quickaddons/managedconfigmodule.h | ||
---|---|---|
206 | Now needs @since 5.67 |
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: |
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.
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. |
src/quickaddons/managedconfigmodule.cpp | ||
---|---|---|
145–146 | Those calls are done for each skeleton perhaps we can do only one call |
src/quickaddons/managedconfigmodule.cpp | ||
---|---|---|
145–146 | registerSettings is not on the hot path, so it will be ok |