Following D27323, manage KCModule states for reinitialize/defaults/apply
Also, this should solve :
BUG: 414567
BUG: 387160
FIXED-IN: 5.19
Lint Skipped |
Unit Tests Skipped |
Buildable 23925 | |
Build 23943: arc lint + arc unit |
Most of it is really nitpicking at that point, I was about to hit accept and let you decide if you wanted to deal with them or not. That being said, it turns out there are two comments around unmanagedStateChange() which give me a bit the creeps (that's why I don't hit accept, not sure it's worth rejecting though, your call in the end).
kcmkwin/kwintabbox/kwintabboxconfigform.h | ||
---|---|---|
34 | Since it's quite some changed lines already, maybe it'd be a good opportunity to move from inheritance to delegation for the Ui::KWinTabBoxConfigForm (ui pointer and all that, it's a better pattern and leads to better incremental compilation). | |
45 | = nullptr missing for the parent parameter | |
kcmkwin/kwintabbox/main.cpp | ||
220 | Risk seems low... but just in case, I'd use the four variant connect in this method still and pass this as third parameter just before the lambda. Granted in case of crash the bug would be somewhere else (as in the reason why ui would outlive this...) but if I would be the future developer having to debug such a regression I'd rather have another symptom than a crash deep within Qt signal emission implementation. :-) | |
222 | Probably works, that being said, we're not supposed to temper with the config object in those slots (this is a KCModule not a Plasma::ConfigModule, they unfortunately both have very different approaches). If we want to stay true to the KCModule approach, we'd rather connect to updateUnmanagedState() directly, and inside updateUnmanagedState() compare the widget value with the corresponding setting values. | |
256 | If you address my comment above, implementation here is likely to change quite a bit. | |
346 | I'd expect those save calls to be done by the KCModule::save() call. Maybe what you want is instead of have the KCModule::save() call done before the dbug message being sent out. | |
420 | dad that's cute :-) | |
421 | What!? dad isn't my dad? It's my grand-dad? Or wait my grand-grand-daddy? This is gross. Clearly it's a complicated family with dark secrets. ;-) What about calling it ancestor? | |
kcmkwin/kwintabbox/main.h | ||
69–70 | Since you touched that line anyway and the pointer isn't in the ctor initialization list, why not append a = nullptr here? (likewise for m_alternativeTabBoxUi, m_actionCollection and m_editor) Avoiding uninitialized members is generally a good idea. |
Move Ui from inheritance to delegation.
Rework KCModule to modify settings only on save().
This diff is almost a rework of the previous one in ordre to move Ui class from inhertitance to delegation and handle unmanaged settings according to KCModule philosophy.
kcmkwin/kwintabbox/kwintabboxconfigform.cpp | ||
---|---|---|
41 | This is leaked, you got to have a dtor now. It can be empty (or = default) if you use QScopedPointer, otherwise it'll have to contain delete ui;. | |
91 | Space on the wrong side of the *, also probably a good idea to initialize with nullptr. Or even better maybe adjust the macro to declare a scope and have the QAction declared and initialized within the scope. This would avoid this odd namespace pollution. | |
260 | man this is so much better than the old macros ;-) (not saying you have to clean them up, it's just that seeing both like this the contrast is telling). |
kcmkwin/kwintabbox/main.cpp | ||
---|---|---|
420–422 | Could we maybe drop ancestor? e.g. auto form = qobject_cast<KWinTabBoxConfigForm *>(sender()); |
kcmkwin/kwintabbox/main.cpp | ||
---|---|---|
418–419 | Too bad you lost the Q_ASSERT(form) in the process. It's all the more important now that we're using the evil sender(). :-) Could we please have the assert back? |
kcmkwin/kwintabbox/main.cpp | ||
---|---|---|
402 | Oops, should be form->layoutName() instead, otherwise it won't save the new layout. |