broulik added a comment.
Thanks a lot for your patch!
I have some minor style nitpicks (you probably just copied them from
elsewhere but if we're adding new code, it should be tidy ;)
Bonus points if you also make a patch for kscreenlocker globalaccel.cpp to
whitelist this new action so that it also works on the lock screen.
INLINE COMMENTS
> powerdevildpmsaction.cpp:42
>
> +#include <kglobalaccel.h>
> +
Use `#include <KGlobalAccel>` and sort it correctly into the other includes
> powerdevildpmsaction.cpp:78
> +
> + KActionCollection* actionCollection = new KActionCollection( this );
> + actionCollection->setComponentDisplayName(i18nc("Name for powerdevil
> shortcuts category", "Power Management"));
Coding style: `KActionCollection *actionCollection` (asterisk after space)
> powerdevildpmsaction.cpp:83
> + globalAction->setText(i18nc("@action:inmenu Global shortcut", "Turn Off
> Screen"));
> + accel->setGlobalShortcut(globalAction, QList<QKeySequence>());
> + connect(globalAction, SIGNAL(triggered(bool)), SLOT(turnOffScreen()));
I suppose there's no `Qt::Key_` enum for that, given laptops typically bypass
the operating system here anyway.
> powerdevildpmsaction.cpp:84
> + accel->setGlobalShortcut(globalAction, QList<QKeySequence>());
> + connect(globalAction, SIGNAL(triggered(bool)), SLOT(turnOffScreen()));
> }
Use new style connect, you can probably just use a lambda
connect(globalAction, &QAction::triggered, this, [this] {
if (m_helper) {
m_helper->trigger(QStringLiteral("TurnOff"));
}
});
> powerdevildpmsaction.h:57
> void setKeyboardBrightnessHelper(int brightness);
> + void turnOffScreen();
>
For `SLOT(...)` syntax to work this must be in a section `private Q_SLOTS`, but
note my comment below about using a lambda instead
REPOSITORY
R122 Powerdevil
REVISION DETAIL
https://phabricator.kde.org/D22261
To: mblumenstingl, #plasma, broulik
Cc: plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel,
ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas,
apol, mart
