davidedmundson requested changes to this revision.
davidedmundson added a comment.
This revision now requires changes to proceed.


  Showing a disabled checkbox for rollover doesn't make much sense.
  IMHO we should either have it as a separate option from kwin and have a 
checkbox, or not have a checkbox at all.

INLINE COMMENTS

> totto wrote in desktop.cpp:36
> The rollover option is from another kde component (kwin), is there a way to 
> get notified whenever this config changes?

I have this coming: 
https://phabricator.kde.org/D13034

> desktop.cpp:122
> +        QCheckBox *item = new QCheckBox(widget);
> +        item->setText(i18nc(e.cfgKey.toStdString().c_str(),
> +                            e.name.toStdString().c_str()));

Unforunately, that's not how our i18n works.

a script greps for i18n("some text here") to generate the english translations.

> desktop.h:54
>  
> +        // QSharedPtr has non-explicit operator bool which is bugprone, work 
> around that
> +        struct explicitBool { bool val; };

or just use a regular bool and copy the value in configurationAccepted

Long term, having simpler code tends to be much better than clever code.

REPOSITORY
  R120 Plasma Workspace

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

To: totto, hein, broulik, #plasma, davidedmundson
Cc: davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

Reply via email to