D27784: KCM KWinTabBox manage KCModule states

ervin requested changes to this revision.
ervin added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> kwintabboxconfigform.cpp:40
> +    , m_type(type)
> +    , ui(new Ui::KWinTabBoxConfigForm)
> +{

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

> kwintabboxconfigform.cpp:90
> +    m_actionCollection->setConfigGlobal(true);
> +    QAction* a;
> +    if (TabboxType::Main == m_type) {

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.

> kwintabboxconfigform.cpp:259
> +    QString action;
> +    auto resetShortcut = [this](KKeySequenceWidget *widget, const QKeySequence &sequence = QKeySequence()) {
> +        const QString action = widget->property("shortcutAction").toString();

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

REPOSITORY
  R108 KWin

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

To: crossi, #kwin, ervin, bport, meven, zzag
Cc: kwin, Orage, cacarry, 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