KCM KWinTabBox manage KCModule states
ClosedPublic

Authored by crossi on Mar 2 2020, 3:13 PM.

Details

Summary

Following D27323, manage KCModule states for reinitialize/defaults/apply

Also, this should solve :

BUG: 414567
BUG: 387160
FIXED-IN: 5.19

Diff Detail

Repository
R108 KWin
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 23925
Build 23943: arc lint + arc unit
crossi created this revision.Mar 2 2020, 3:13 PM
Restricted Application added a project: KWin. · View Herald TranscriptMar 2 2020, 3:13 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
crossi requested review of this revision.Mar 2 2020, 3:13 PM
crossi edited the summary of this revision. (Show Details)Mar 2 2020, 4:04 PM
meven added inline comments.Mar 4 2020, 8:32 AM
kcmkwin/kwintabbox/main.cpp
218

Too bad we have to use old syntax for private slots

429

s/dispplay/display

crossi updated this revision to Diff 76905.Mar 4 2020, 8:38 AM

misspelling

crossi marked an inline comment as done.Mar 4 2020, 8:38 AM
zzag added inline comments.Mar 4 2020, 8:44 AM
kcmkwin/kwintabbox/kwintabboxconfigform.cpp
119

s/Qt::UserRole + 2/AddonEffect/?

kcmkwin/kwintabbox/kwintabboxconfigform.h
56

Q_SIGNALS:

crossi updated this revision to Diff 76906.Mar 4 2020, 8:53 AM

code style

crossi updated this revision to Diff 76907.Mar 4 2020, 8:55 AM

code style

crossi marked 2 inline comments as done.Mar 4 2020, 8:56 AM
zzag accepted this revision.Mar 4 2020, 9:21 AM
This revision is now accepted and ready to land.Mar 4 2020, 9:21 AM
meven accepted this revision.Mar 4 2020, 11:47 AM
ervin added a comment.Mar 5 2020, 2:47 PM

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.

crossi updated this revision to Diff 77291.Mar 9 2020, 4:03 PM
crossi marked 4 inline comments as done.

Move Ui from inheritance to delegation.
Rework KCModule to modify settings only on save().

crossi marked 6 inline comments as done.Mar 9 2020, 4:09 PM

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.

zzag added inline comments.Mar 10 2020, 4:23 PM
kcmkwin/kwintabbox/kwintabboxconfigform.cpp
260

RESET_SHORTCUT is not a macro. So maybe resetShortcut?

kcmkwin/kwintabbox/main.cpp
420

Heh, we have a variable somewhere in KWin core called "daddy". :D

crossi marked an inline comment as done.Mar 11 2020, 3:23 PM
ervin requested changes to this revision.Mar 16 2020, 3:59 PM
ervin added inline comments.
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).

This revision now requires changes to proceed.Mar 16 2020, 3:59 PM
crossi updated this revision to Diff 77994.Mar 19 2020, 12:49 PM

fix leak. replace macros.

crossi marked 3 inline comments as done.Mar 19 2020, 12:50 PM
zzag added inline comments.Mar 19 2020, 12:56 PM
kcmkwin/kwintabbox/main.cpp
420–422

Could we maybe drop ancestor? e.g.

auto form = qobject_cast<KWinTabBoxConfigForm *>(sender());
crossi updated this revision to Diff 77996.Mar 19 2020, 1:27 PM

drop ancestor.

crossi marked an inline comment as done.Mar 19 2020, 2:32 PM
ervin added inline comments.Mar 27 2020, 4:32 PM
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?

crossi updated this revision to Diff 78689.Mar 27 2020, 6:06 PM

restore Q_ASSERT

crossi marked an inline comment as done.Mar 27 2020, 6:07 PM
ervin accepted this revision.Mar 27 2020, 9:12 PM
This revision is now accepted and ready to land.Mar 27 2020, 9:12 PM
crossi added inline comments.Apr 1 2020, 8:48 AM
kcmkwin/kwintabbox/main.cpp
402

Oops, should be form->layoutName() instead, otherwise it won't save the new layout.

meven edited the summary of this revision. (Show Details)Apr 1 2020, 8:57 AM
crossi updated this revision to Diff 79027.Apr 1 2020, 8:59 AM

fix error

crossi marked an inline comment as done.Apr 1 2020, 9:00 AM
This revision was automatically updated to reflect the committed changes.