D27784: KCM KWinTabBox manage KCModule states

[Kevin Ottens]

ervin added a comment.


  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).

INLINE COMMENTS

> kwintabboxconfigform.h:33
> +
> +class KWinTabBoxConfigForm : public QWidget, public Ui::KWinTabBoxConfigForm
> +{

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).

> kwintabboxconfigform.h:44
> +
> +    explicit KWinTabBoxConfigForm(QWidget *parent);
> +

`= nullptr` missing for the parent parameter

> main.cpp:246
> +
> +    connect(ui, &KWinTabBoxConfigForm::filterScreenChanged, [this, config](int value) {
> +        config->setMultiScreenMode(value);

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. :-)

> main.cpp:248
> +        config->setMultiScreenMode(value);
> +        updateUnmanagedState();
> +    });

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.

> main.cpp:282
> +{
> +    unmanagedWidgetChangeState(m_tabBoxConfig->isSaveNeeded() || m_tabBoxAlternativeConfig->isSaveNeeded());
> +    unmanagedWidgetDefaultState(m_tabBoxConfig->isDefaults() && m_tabBoxAlternativeConfig->isDefaults());

If you address my comment above, implementation here is likely to change quite a bit.

> main.cpp:373
>  
> +    m_tabBoxConfig->save();
> +    m_tabBoxAlternativeConfig->save();

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.

> main.cpp:439
> +    KWinTabBoxConfigForm *ui = nullptr;
> +    QObject *dad = sender();
> +    while (!ui && (dad = dad->parent())) {

`dad` that's cute :-)

> main.cpp:440
> +    QObject *dad = sender();
> +    while (!ui && (dad = dad->parent())) {
> +        ui = qobject_cast<KWinTabBoxConfigForm *>(dad);

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`?

> main.h:69
>  private:
> -    enum Mode {
> -        CoverSwitch = 0,
> -        FlipSwitch = 1,
> -        Layout = 2
> -    };
> -    KWinTabBoxConfigForm* m_primaryTabBoxUi;
> -    KWinTabBoxConfigForm* m_alternativeTabBoxUi;
> +    KWinTabBoxConfigForm *m_primaryTabBoxUi;
> +    KWinTabBoxConfigForm *m_alternativeTabBoxUi;

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.

REPOSITORY
  R108 KWin

REVISION DETAIL
  https://phabricator.kde.org/D27784

To: crossi, #kwin, ervin, bport, meven, zzag
Cc: kwin, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, mkulinski, ragreen, jackyalcine, iodelay, crozbo, bwowk, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, hardening, romangg, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kwin/attachments/20200305/4d6f1199/attachment-0001.html>