D22261: Add a global shortcut action to turn off the screen

[Martin Blumenstingl]

mblumenstingl added a comment.


  In D22261#502044 , @broulik wrote:
  
  > Just checking, are you still up for making this kscreenlocker change? Given 
the heatwave right now I would have loved to be able to turn off my screens on 
the lock screen explicitly :D
  
  
  I just created D22748 
  unfortunately the patch is not working for me, probably because there's some 
other issue on my system with kscreenlocker. see the description for details

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

D22261: Add a global shortcut action to turn off the screen

[Kai Uwe Broulik]

broulik added a comment.


  Just checking, are you still up for making this kscreenlocker change? Given 
the heatwave right now I would have loved to be able to turn off my screens on 
the lock screen explicitly :D

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

D22261: Add a global shortcut action to turn off the screen

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R122:119943e6b883: Add a global shortcut action to turn off 
the screen (authored by mblumenstingl, committed by broulik).

REPOSITORY
  R122 Powerdevil

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22261?vs=61124=61127

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

AFFECTED FILES
  daemon/actions/dpms/CMakeLists.txt
  daemon/actions/dpms/powerdevildpmsaction.cpp

To: mblumenstingl, #plasma, broulik
Cc: plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, 
ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D22261: Add a global shortcut action to turn off the screen

[Martin Blumenstingl]

mblumenstingl added a comment.


  In D22261#490680 , @broulik wrote:
  
  > Alright, very nice!
  >  I suppose you don't have commit access, so I need an email address from 
you to add you as commit author. Thanks!
  
  
  I once had commit access but I'm not sure if I still have.
  either way, I would be more comfortable with you committing this with the 
following author information: Martin Blumenstingl 

  
  >> I tried adding |Turn Off Screen to the powerdevil QRegularExpression but I 
couldn't turn off the display from the lock screen
  > 
  > You probably need to log out and back in for the new screenlocker binary to 
be loaded (or run the kscreenlocker_test binary in the build/bin directory)
  
  alright, I'll give this another go and open another review

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

D22261: Add a global shortcut action to turn off the screen

[Kai Uwe Broulik]

broulik accepted this revision.
broulik added a comment.
This revision is now accepted and ready to land.


  Alright, very nice!
  I suppose you don't have commit access, so I need an email address from you 
to add you as commit author. Thanks!
  
  > I tried adding |Turn Off Screen to the powerdevil QRegularExpression but I 
couldn't turn off the display from the lock screen
  
  You probably need to log out and back in for the new screenlocker binary to 
be loaded (or run the kscreenlocker_test binary in the build/bin directory)

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

D22261: Add a global shortcut action to turn off the screen

[Martin Blumenstingl]

mblumenstingl added a comment.


  In D22261#490660 , @broulik wrote:
  
  > 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.
  
  
  I tried adding `|Turn Off Screen` to the powerdevil QRegularExpression but I 
couldn't turn off the display from the lock screen
  my test plan for this is:
  
  - original test plan
  - move mouse so the screen turns on again
  - application launcher -> Leave -> Lock -> confirm
  - press my shortcut
  - (nothing happens)

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

D22261: Add a global shortcut action to turn off the screen

[Martin Blumenstingl]

mblumenstingl added inline comments.

INLINE COMMENTS

> broulik wrote in powerdevildpmsaction.cpp:83
> I suppose there's no `Qt::Key_` enum for that, given laptops typically bypass 
> the operating system here anyway.

I couldn't find any appropriate `Qt::Key_` so I'm leaving it up to the user to 
choose one

> broulik wrote in powerdevildpmsaction.cpp:84
> Use new style connect, you can probably just use a lambda
> 
>   connect(globalAction, ::triggered, this, [this] {
>   if (m_helper) {
>   m_helper->trigger(QStringLiteral("TurnOff"));
>   }
>   });

that if (m_helper) is probably the overloaded != nullptr from 
https://doc.qt.io/qt-5/qscopedpointer.html#operator-not-eq-1
thanks for this hint, it's been a long time since I last did some c++/Qt

> broulik wrote in powerdevildpmsaction.h:57
> For `SLOT(...)` syntax to work this must be in a section `private Q_SLOTS`, 
> but note my comment below about using a lambda instead

sorry for that, I missed that when I moved from a public slot to a private one
your lambda expression is working fine so I'm going with that 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

D22261: Add a global shortcut action to turn off the screen

[Martin Blumenstingl]

mblumenstingl updated this revision to Diff 61124.
mblumenstingl marked 5 inline comments as done.
mblumenstingl added a comment.


  updated patch to address the comments from @broulik

REPOSITORY
  R122 Powerdevil

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22261?vs=61118=61124

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

AFFECTED FILES
  daemon/actions/dpms/CMakeLists.txt
  daemon/actions/dpms/powerdevildpmsaction.cpp

To: mblumenstingl, #plasma, broulik
Cc: plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, 
ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D22261: Add a global shortcut action to turn off the screen

[Kai Uwe Broulik]

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 
> +

Use `#include ` 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());
> +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());
> +connect(globalAction, SIGNAL(triggered(bool)), SLOT(turnOffScreen()));
>  }

Use new style connect, you can probably just use a lambda

  connect(globalAction, ::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

D22261: Add a global shortcut action to turn off the screen

[Martin Blumenstingl]

mblumenstingl created this revision.
mblumenstingl added a reviewer: Plasma.
mblumenstingl added a project: Plasma.
Herald added a subscriber: plasma-devel.
mblumenstingl requested review of this revision.

REVISION SUMMARY
  PowerDevilDPMSAction already provides implementations for X11 and Wayland 
which allow turning off the screen.
  For X11 there was a way to do this on the command line (with "xset dpms force 
off"), but there's no such replacement for Wayland yet.
  
  Add an action - to which users can assign a shortcut - to turn off the screen.

TEST PLAN
  start a plasma wayland session
  open systemsettings5 -> Shortcuts -> Global Shortcuts -> Power Management
  assign a shortcut to "Turn Off Screen"
  press this shortcut
  (screen must now turn off)

REPOSITORY
  R122 Powerdevil

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

AFFECTED FILES
  daemon/actions/dpms/CMakeLists.txt
  daemon/actions/dpms/powerdevildpmsaction.cpp
  daemon/actions/dpms/powerdevildpmsaction.h

To: mblumenstingl, #plasma
Cc: plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, 
ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart