D16425: Added new Suspend then Hibernate option

[Méven Car]

meven added a comment.


  In D16425#488989 , @avaldes wrote:
  
  > In D16425#488963 , 
@davidedmundson wrote:
  >
  > > I'd quite like to get this in as I'll end up moving part of this - and 
we've got too much bikeshedding here.
  > >
  > > > i18n("While asleep, hibernate after 3 hours")
  > >
  > > If you don't know the timeout, don't write it. It's better to be vague 
than lie.
  > >
  > > "Suspend, then hibernate after a period of inactivitiy"
  >
  >
  > I have updated the message. I cannot land it so I need help with that
  
  
  Btw, I think you could have used
  
arc amend
  
  to update your commit message based on the differential description.
  
  I'd be happy to land your change.
  
  Great contribution !

REPOSITORY
  R122 Powerdevil

BRANCH
  arcpatch-D16425_1

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

To: avaldes, broulik, ngraham, meven, davidedmundson, sitter
Cc: sitter, davidedmundson, ericadams, jobauer, reverendhomer, meven, soriano, 
abalaji, graesslin, ngraham, plasma-devel, LeGast00n, jraleigh, fbampaloukas, 
GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Méven Car]

This revision was automatically updated to reflect the committed changes.
Closed by commit R122:daa06ba31ff1: Added new Suspend then Hibernate option 
(authored by avaldes, committed by meven).

REPOSITORY
  R122 Powerdevil

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16425?vs=60926&id=60947

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

AFFECTED FILES
  daemon/actions/bundled/suspendsession.cpp
  daemon/actions/bundled/suspendsession.h
  daemon/actions/bundled/suspendsessionconfig.cpp
  daemon/actions/bundled/suspendsessionconfig.h
  daemon/backends/upower/login1suspendjob.cpp
  daemon/backends/upower/powerdevilupowerbackend.cpp
  daemon/backends/upower/upowersuspendjob.cpp
  daemon/org.freedesktop.PowerManagement.xml
  daemon/powerdevilbackendinterface.h
  daemon/powerdevilfdoconnector.cpp
  daemon/powerdevilfdoconnector.h
  daemon/powerdevilpowermanagement.cpp
  daemon/powerdevilpowermanagement.h

To: avaldes, broulik, ngraham, meven, davidedmundson, sitter
Cc: sitter, davidedmundson, ericadams, jobauer, reverendhomer, meven, soriano, 
abalaji, graesslin, ngraham, plasma-devel, LeGast00n, jraleigh, fbampaloukas, 
GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Alejandro Valdes]

avaldes added a comment.


  In D16425#488963 , @davidedmundson 
wrote:
  
  > I'd quite like to get this in as I'll end up moving part of this - and 
we've got too much bikeshedding here.
  >
  > > i18n("While asleep, hibernate after 3 hours")
  >
  > If you don't know the timeout, don't write it. It's better to be vague than 
lie.
  >
  > "Suspend, then hibernate after a period of inactivitiy"
  
  
  I have updated the message. I cannot land it so I need help with that

REPOSITORY
  R122 Powerdevil

BRANCH
  arcpatch-D16425_1

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

To: avaldes, broulik, ngraham, meven, davidedmundson, sitter
Cc: sitter, davidedmundson, ericadams, jobauer, reverendhomer, meven, soriano, 
abalaji, graesslin, ngraham, plasma-devel, LeGast00n, jraleigh, fbampaloukas, 
GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Alejandro Valdes]

avaldes updated this revision to Diff 60926.
avaldes added a comment.


  Summary:
  See bug 399727  for a good 
description of what this code is for.
  
  The new ui will show a new option like the following image F6349860: 
screenshot.png 
  
  Reviewers: broulik, ngraham
  
  Subscribers: soriano, abalaji, graesslin, ngraham, plasma-devel
  
  Tags: #plasma 
  
  Differential Revision: https://phabricator.kde.org/D16425

REPOSITORY
  R122 Powerdevil

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16425?vs=59241&id=60926

BRANCH
  arcpatch-D16425_1

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

AFFECTED FILES
  daemon/actions/bundled/suspendsession.cpp
  daemon/actions/bundled/suspendsession.h
  daemon/actions/bundled/suspendsessionconfig.cpp
  daemon/actions/bundled/suspendsessionconfig.h
  daemon/backends/upower/login1suspendjob.cpp
  daemon/backends/upower/powerdevilupowerbackend.cpp
  daemon/backends/upower/upowersuspendjob.cpp
  daemon/org.freedesktop.PowerManagement.xml
  daemon/powerdevilbackendinterface.h
  daemon/powerdevilfdoconnector.cpp
  daemon/powerdevilfdoconnector.h
  daemon/powerdevilpowermanagement.cpp
  daemon/powerdevilpowermanagement.h

To: avaldes, broulik, ngraham, meven, davidedmundson, sitter
Cc: sitter, davidedmundson, ericadams, jobauer, reverendhomer, meven, soriano, 
abalaji, graesslin, ngraham, plasma-devel, LeGast00n, jraleigh, fbampaloukas, 
GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Harald Sitter]

sitter accepted this revision.
sitter added a comment.


  FTR on ubuntu, and by extension kubuntu and also neon, hibernation is 
disabled by default to enable it you need to kick the disabling rule out of 
`/var/lib/polkit-1/localauthority/10-vendor.d/com.ubuntu.desktop.pkla`.
  
  The feature works very well!

REPOSITORY
  R122 Powerdevil

BRANCH
  arcpatch-D16425_1

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

To: avaldes, broulik, ngraham, meven, davidedmundson, sitter
Cc: sitter, davidedmundson, ericadams, jobauer, reverendhomer, meven, soriano, 
abalaji, graesslin, ngraham, plasma-devel, LeGast00n, jraleigh, fbampaloukas, 
GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[David Edmundson]

davidedmundson accepted this revision.
davidedmundson added a comment.


  I'd quite like to get this in as I'll end up moving part of this - and we've 
got too much bikeshedding here.
  
  > i18n("While asleep, hibernate after 3 hours")
  
  If you don't know the timeout, don't write it. It's better to be vague than 
lie.
  
  "Suspend, then hibernate after a period of inactivitiy"

REPOSITORY
  R122 Powerdevil

BRANCH
  arcpatch-D16425_1

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

To: avaldes, broulik, ngraham, meven, davidedmundson
Cc: davidedmundson, ericadams, jobauer, reverendhomer, meven, soriano, abalaji, 
graesslin, ngraham, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Méven Car]

meven accepted this revision.
meven added a comment.


  Except that I couldn't test it, the code looks in great shape to me.

REPOSITORY
  R122 Powerdevil

BRANCH
  arcpatch-D16425_1

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

To: avaldes, broulik, ngraham, meven
Cc: ericadams, jobauer, reverendhomer, meven, soriano, abalaji, graesslin, 
ngraham, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, 
ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D16425: Added new Suspend then Hibernate option

[Méven Car]

meven added a comment.


  I tried using it but my system does not suspend-then-hibernate mode.
  
  You can check if your system supports it once you have systemd >= 239 and the 
following command returns true :
  
qdbus org.freedesktop.PowerManagement /org/freedesktop/PowerManagement 
CanSuspendThenHibernate
  
  Or this one should have the same result :
  
qdbus org.freedesktop.PowerManagement /org/freedesktop/PowerManagement 
CanHibernate

REPOSITORY
  R122 Powerdevil

BRANCH
  arcpatch-D16425_1

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

To: avaldes, broulik, ngraham
Cc: ericadams, jobauer, reverendhomer, meven, soriano, abalaji, graesslin, 
ngraham, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, 
ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D16425: Added new Suspend then Hibernate option

[Alejandro Valdes]

avaldes marked 3 inline comments as done.
avaldes added a comment.


  In D16425#475148 , @ericadams 
wrote:
  
  > I apologize if this is the wrong place to add this comment but I have a 
laptop where this applies and would benefit me. I am happy to help test this if 
you find it helpful. Thanks.
  
  
  Feel free to download this patch (download raw diff 
) and apply it to powerdevil 
repo . You need to build the 
package and replace the one in your system.
  
  If you use arch or a derivative I can help you with that.

REPOSITORY
  R122 Powerdevil

BRANCH
  arcpatch-D16425_1

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

To: avaldes, broulik, ngraham
Cc: ericadams, jobauer, reverendhomer, meven, soriano, abalaji, graesslin, 
ngraham, plasma-devel, LeGast00n, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Eric Adams]

ericadams added a comment.


  I apologize if this is the wrong place to add this comment but I have a 
laptop where this applies and would benefit me. I am happy to help test this if 
you find it helpful. Thanks.

REPOSITORY
  R122 Powerdevil

BRANCH
  arcpatch-D16425_1

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

To: avaldes, broulik, ngraham
Cc: ericadams, jobauer, reverendhomer, meven, soriano, abalaji, graesslin, 
ngraham, plasma-devel, LeGast00n, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Alejandro Valdes]

avaldes updated this revision to Diff 59241.
avaldes added a comment.


  Summary:
  See bug 399727  for a good 
description of what this code is for.
  
  The new ui will show a new option like the following image F6349860: 
screenshot.png 
  
  Reviewers: broulik, ngraham
  
  Subscribers: soriano, abalaji, graesslin, ngraham, plasma-devel
  
  Tags: #plasma 
  
  Differential Revision: https://phabricator.kde.org/D16425

REPOSITORY
  R122 Powerdevil

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16425?vs=59170&id=59241

BRANCH
  arcpatch-D16425_1

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

AFFECTED FILES
  daemon/actions/bundled/suspendsession.cpp
  daemon/actions/bundled/suspendsession.h
  daemon/actions/bundled/suspendsessionconfig.cpp
  daemon/actions/bundled/suspendsessionconfig.h
  daemon/backends/upower/login1suspendjob.cpp
  daemon/backends/upower/powerdevilupowerbackend.cpp
  daemon/backends/upower/upowersuspendjob.cpp
  daemon/org.freedesktop.PowerManagement.xml
  daemon/powerdevilbackendinterface.h
  daemon/powerdevilfdoconnector.cpp
  daemon/powerdevilfdoconnector.h
  daemon/powerdevilpowermanagement.cpp
  daemon/powerdevilpowermanagement.h

To: avaldes, broulik, ngraham
Cc: jobauer, reverendhomer, meven, soriano, abalaji, graesslin, ngraham, 
plasma-devel, LeGast00n, ericadams, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Ambareesh Balaji]

abalaji added inline comments.

INLINE COMMENTS

> avaldes wrote in suspendsessionconfig.cpp:45
> something like this?
> 
>   : ActionConfig(parent),
> m_suspendThenHibernateEnabled(nullptr)

Yeah exactly. So a subtle detail of C++ constructors is any data members which 
are not primitive (like structs class instances) get implicitly initialized 
using their default constructor if they don't have an entry in the initializer 
list. So if you do everything in the constructor body you risk double 
initializing stuff, which is unnecessary overhead. Of course this is not the 
case with primitives like pointers which are just integers behind the scenes, 
so what you have is okay, but just for the sake of consistency, better to make 
use of the initializer list

REPOSITORY
  R122 Powerdevil

BRANCH
  arcpatch-D16425_1

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

To: avaldes, broulik, ngraham
Cc: jobauer, reverendhomer, meven, soriano, abalaji, graesslin, ngraham, 
plasma-devel, LeGast00n, ericadams, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Alejandro Valdes]

avaldes added inline comments.

INLINE COMMENTS

> abalaji wrote in suspendsessionconfig.cpp:45
> Move this to the initializer

something like this?

  : ActionConfig(parent),
m_suspendThenHibernateEnabled(nullptr)

REPOSITORY
  R122 Powerdevil

BRANCH
  arcpatch-D16425_1

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

To: avaldes, broulik, ngraham
Cc: jobauer, reverendhomer, meven, soriano, abalaji, graesslin, ngraham, 
plasma-devel, LeGast00n, ericadams, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Ambareesh Balaji]

abalaji added inline comments.

INLINE COMMENTS

> suspendsessionconfig.cpp:45
>  {
> -
> +m_suspendThenHibernateEnabled = nullptr;
>  }

Move this to the initializer

REPOSITORY
  R122 Powerdevil

BRANCH
  arcpatch-D16425_1

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

To: avaldes, broulik, ngraham
Cc: jobauer, reverendhomer, meven, soriano, abalaji, graesslin, ngraham, 
plasma-devel, LeGast00n, ericadams, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Alejandro Valdes]

avaldes marked 5 inline comments as done.
avaldes added a comment.


  Sorry for the constant spam of the original commit message, I cannot make 
arcanist to pick up a new commit for this differential without deleting the old 
one.
  
  Added changes based on code review comments

REPOSITORY
  R122 Powerdevil

BRANCH
  arcpatch-D16425_1

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

To: avaldes, broulik, ngraham
Cc: jobauer, reverendhomer, meven, soriano, abalaji, graesslin, ngraham, 
plasma-devel, LeGast00n, ericadams, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Alejandro Valdes]

avaldes updated this revision to Diff 59170.
avaldes marked 2 inline comments as done.
avaldes added a comment.


  Added new Suspend then Hibernate option
  
  Summary:
  See bug 399727  for a good 
description of what this code is for.
  
  The new ui will show a new option like the following image F6349860: 
screenshot.png 
  
  Reviewers: broulik, ngraham
  
  Subscribers: soriano, abalaji, graesslin, ngraham, plasma-devel
  
  Tags: #plasma 
  
  Differential Revision: https://phabricator.kde.org/D16425

REPOSITORY
  R122 Powerdevil

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16425?vs=58969&id=59170

BRANCH
  arcpatch-D16425_1

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

AFFECTED FILES
  daemon/actions/bundled/suspendsession.cpp
  daemon/actions/bundled/suspendsession.h
  daemon/actions/bundled/suspendsessionconfig.cpp
  daemon/actions/bundled/suspendsessionconfig.h
  daemon/backends/upower/login1suspendjob.cpp
  daemon/backends/upower/powerdevilupowerbackend.cpp
  daemon/backends/upower/upowersuspendjob.cpp
  daemon/org.freedesktop.PowerManagement.xml
  daemon/powerdevilbackendinterface.h
  daemon/powerdevilfdoconnector.cpp
  daemon/powerdevilfdoconnector.h
  daemon/powerdevilpowermanagement.cpp
  daemon/powerdevilpowermanagement.h

To: avaldes, broulik, ngraham
Cc: jobauer, reverendhomer, meven, soriano, abalaji, graesslin, ngraham, 
plasma-devel, LeGast00n, ericadams, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Ambareesh Balaji]

abalaji added inline comments.

INLINE COMMENTS

> suspendsessionconfig.cpp:122
> +int comboBoxMaxWidth = 300;
> +if (m_suspendThenHibernateEnabled) {
> +comboBoxMaxWidth = qMax(comboBoxMaxWidth, 
> m_suspendThenHibernateEnabled->sizeHint().width());

After doing the above, this if block can be merged with the above if block, if 
you know what I mean.

> avaldes wrote in suspendsessionconfig.cpp:106
> Done on purpose to change the UI:
> 
> F6822188: suspendThenHibernate.png 
> 
> Automatically, m_comboBox, m_idleTime

Oh i see, that actually makes sense

> avaldes wrote in suspendsessionconfig.cpp:121
> yeah it's necessary, without it powerdevil crashes at launch

Yeah it crashes because you're probably not doing a null check somewhere. I 
would suggest initializing `m_suspendThenHibernateEnabled ` to nullptr in the 
ctor, moving line 79 into this if block right above, and getting rid of this 
else block altogether, that way you don't wastefully allocate the object. After 
this, check for nullptr in every place you use `m_suspendThenHibernateEnabled`.

REPOSITORY
  R122 Powerdevil

BRANCH
  arcpatch-D16425_1

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

To: avaldes, broulik, ngraham
Cc: jobauer, reverendhomer, meven, soriano, abalaji, graesslin, ngraham, 
plasma-devel, ericadams, jraleigh, GB_2, ragreen, Pitel, ZrenBot, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Alejandro Valdes]

avaldes marked 5 inline comments as done.
avaldes added a comment.


  Added changes based on recent comments.

INLINE COMMENTS

> abalaji wrote in suspendsessionconfig.cpp:106
> Stray line swap

Done on purpose to change the UI:

F6822188: suspendThenHibernate.png 

Automatically, m_comboBox, m_idleTime

> abalaji wrote in suspendsessionconfig.cpp:121
> Not sure if this is necessary, just `delete m_suspendThenHibernateEnabled` 
> might be enough.

yeah it's necessary, without it powerdevil crashes at launch

REPOSITORY
  R122 Powerdevil

BRANCH
  arcpatch-D16425_1

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

To: avaldes, broulik, ngraham
Cc: jobauer, reverendhomer, meven, soriano, abalaji, graesslin, ngraham, 
plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Alejandro Valdes]

avaldes updated this revision to Diff 58969.
avaldes added a comment.


  Added new Suspend then Hibernate option
  
  Summary:
  See bug 399727  for a good 
description of what this code is for.
  
  The new ui will show a new option like the following image F6822188: 
suspendThenHibernate.png 
  
  FEATURE: 399727
  
  Reviewers: broulik, ngraham
  
  Subscribers: soriano, abalaji, graesslin, ngraham, plasma-devel
  
  Tags: #plasma 
  
  Differential Revision: https://phabricator.kde.org/D16425

REPOSITORY
  R122 Powerdevil

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16425?vs=58273&id=58969

BRANCH
  arcpatch-D16425_1

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

AFFECTED FILES
  daemon/actions/bundled/suspendsession.cpp
  daemon/actions/bundled/suspendsession.h
  daemon/actions/bundled/suspendsessionconfig.cpp
  daemon/actions/bundled/suspendsessionconfig.h
  daemon/backends/upower/login1suspendjob.cpp
  daemon/backends/upower/powerdevilupowerbackend.cpp
  daemon/backends/upower/upowersuspendjob.cpp
  daemon/org.freedesktop.PowerManagement.xml
  daemon/powerdevilbackendinterface.h
  daemon/powerdevilfdoconnector.cpp
  daemon/powerdevilfdoconnector.h
  daemon/powerdevilpowermanagement.cpp
  daemon/powerdevilpowermanagement.h

To: avaldes, broulik, ngraham
Cc: jobauer, reverendhomer, meven, soriano, abalaji, graesslin, ngraham, 
plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Ambareesh Balaji]

abalaji added inline comments.

INLINE COMMENTS

> suspendsession.cpp:137
>  Q_EMIT aboutToSuspend();
> -suspendJob = 
> backend()->suspend(PowerDevil::BackendInterface::ToRam);
> +if (m_suspendThenHibernateEnabled) {
> +suspendJob = 
> backend()->suspend(PowerDevil::BackendInterface::SuspendThenHibernate);

How about avoiding the duplicate `suspendJob = backend()->suspend()` calls by 
doing:

  suspendJob = backend()->suspend(m_suspendThenHibernateEnabled 
   ? PowerDevil::BackendInterface::SuspendThenHibernate : 
PowerDevil::BackendInterface::ToRam);

> suspendsession.cpp:177
>  {
>  if (config.isValid() && config.hasKey("idleTime") && 
> config.hasKey("suspendType")) {
>  // Add the idle timeout

Factor the `config.isValid()` out of the two `if` statements

> suspendsessionconfig.cpp:57
>  configGroup().writeEntry("idleTime", m_idleTime->value() * 60 * 1000);
> +if (m_suspendThenHibernateEnabled) {
> +configGroup().writeEntry(

Again

  configGroup().writeEntry("suspendThenHibernate", 
m_suspendThenHibernateEnabled != nullptr && 
m_suspendThenHibernateEnabled->isChecked());

> suspendsessionconfig.cpp:106
>  hlay->addWidget(m_comboBox);
> +hlay->addWidget(m_idleTime);
>  hlay->addStretch();

Stray line swap

> suspendsessionconfig.cpp:121
> +} else {
> +m_suspendThenHibernateEnabled->deleteLater();
> +m_suspendThenHibernateEnabled = nullptr;

Not sure if this is necessary, just `delete m_suspendThenHibernateEnabled` 
might be enough.

REPOSITORY
  R122 Powerdevil

BRANCH
  arcpatch-D16425_1

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

To: avaldes, broulik, ngraham
Cc: reverendhomer, meven, soriano, abalaji, graesslin, ngraham, plasma-devel, 
jraleigh, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Nathaniel Graham]

ngraham added a comment.


  Thanks. Let's wait for @broulik's review once he gets back from vacation (a 
few days, I think).

REPOSITORY
  R122 Powerdevil

BRANCH
  arcpatch-D16425_1

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

To: avaldes, broulik, ngraham
Cc: reverendhomer, meven, soriano, abalaji, graesslin, ngraham, plasma-devel, 
jraleigh, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Alejandro Valdes]

avaldes marked 3 inline comments as done.
avaldes added a comment.


  Updated with latest comments.
  
  Please land, I don't have permissions.

REPOSITORY
  R122 Powerdevil

BRANCH
  arcpatch-D16425_1

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

To: avaldes, broulik, ngraham
Cc: reverendhomer, meven, soriano, abalaji, graesslin, ngraham, plasma-devel, 
jraleigh, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Alejandro Valdes]

avaldes updated this revision to Diff 58273.
avaldes added a comment.


  Added new Suspend then Hibernate option
  

  
  Summary:
  See bug 399727  for a good 
description of what this code is for.
  
  The new ui will show a new option like the following image F6349860: 
screenshot.png 
  

  
  Reviewers: broulik, ngraham
  
   
  
  Subscribers: soriano, abalaji, graesslin, ngraham, plasma-devel
  

  
  Tags: #plasma 
  

  
  Differential Revision: https://phabricator.kde.org/D16425

REPOSITORY
  R122 Powerdevil

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16425?vs=58091&id=58273

BRANCH
  arcpatch-D16425_1

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

AFFECTED FILES
  daemon/actions/bundled/suspendsession.cpp
  daemon/actions/bundled/suspendsession.h
  daemon/actions/bundled/suspendsessionconfig.cpp
  daemon/actions/bundled/suspendsessionconfig.h
  daemon/backends/upower/login1suspendjob.cpp
  daemon/backends/upower/powerdevilupowerbackend.cpp
  daemon/backends/upower/upowersuspendjob.cpp
  daemon/org.freedesktop.PowerManagement.xml
  daemon/powerdevilbackendinterface.h
  daemon/powerdevilfdoconnector.cpp
  daemon/powerdevilfdoconnector.h
  daemon/powerdevilpowermanagement.cpp
  daemon/powerdevilpowermanagement.h

To: avaldes, broulik, ngraham
Cc: reverendhomer, meven, soriano, abalaji, graesslin, ngraham, plasma-devel, 
jraleigh, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Nathaniel Graham]

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


  We just branched 5.16, so this will be 5.17 material. That should leave lots 
of time for testing. Would also be nice to get a review from @broulik once he 
returns from vacation or anyone else in #plasma 
.
  
  Can you land the patch yourself or do you need someone else to do it for you?

INLINE COMMENTS

> powerdevilpowermanagement.cpp:207
> +{
> +if(!d->serviceRegistered) {
> +return;

space after `if`

> powerdevilpowermanagement.cpp:210
> +}
> +if(!d->canSuspendThenHibernate) {
> +return;

space after `if`

REPOSITORY
  R122 Powerdevil

BRANCH
  arcpatch-D16425_1

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

To: avaldes, broulik, ngraham
Cc: reverendhomer, meven, soriano, abalaji, graesslin, ngraham, plasma-devel, 
jraleigh, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Alejandro Valdes]

avaldes added a comment.


  > This patch doesn't seem to work for me. I have Arch Linux, 
powerdevil-5.15.5-1 and your patch applied. After clicking the check-box the 
"Apply" button doesn't get active. Therefore, this setting is not being saved.
  
  Did you enable the Suspend Session option too? you need to enable the Suspend 
Session before using the new checkbox, it is the same behavior of "Even when an 
external monitor is connected" checkbox. Is a limitation on how powerdevil UI 
is built.
  
  > Also, why does the checkbox label say "While asleep, hibernate after 3 
hours"? This value can be overriden in /etc/systemd/sleep.conf
  
  It is the default timeout, there's no easy way to change it using a UI or a 
parameter, unless we want to point the users to modify a root-owned file.

REPOSITORY
  R122 Powerdevil

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

To: avaldes, broulik, ngraham
Cc: reverendhomer, meven, soriano, abalaji, graesslin, ngraham, plasma-devel, 
jraleigh, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Reverend Homer]

reverendhomer added a comment.


  Hi,
  
  In D16425#465325 , @avaldes wrote:
  
  > In D16425#465321 , @ngraham 
wrote:
  >
  > > UI looks good enough for now. But is this the full diff? It seems like 
something got lost. The whole patch should include the changes from all commits 
in your branch, not just the last one.
  >
  >
  > should be fixed now, I'm not sure what happened with arcanist.
  
  
  This patch doesn't seem to work for me. I have Arch Linux, 
powerdevil-5.15.5-1 and your patch applied. After clicking the check-box the 
"Apply" button doesn't get active. Therefore, this setting is not being saved.
  
  Also, why does the checkbox label say "While asleep, hibernate after 3 
hours"? This value can be overriden in /etc/systemd/sleep.conf

REPOSITORY
  R122 Powerdevil

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

To: avaldes, broulik, ngraham
Cc: reverendhomer, meven, soriano, abalaji, graesslin, ngraham, plasma-devel, 
jraleigh, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Alejandro Valdes]

avaldes added a comment.


  In D16425#465321 , @ngraham wrote:
  
  > UI looks good enough for now. But is this the full diff? It seems like 
something got lost. The whole patch should include the changes from all commits 
in your branch, not just the last one.
  
  
  should be fixed now, I'm not sure what happened with arcanist.

REPOSITORY
  R122 Powerdevil

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

To: avaldes, broulik, ngraham
Cc: meven, soriano, abalaji, graesslin, ngraham, plasma-devel, jraleigh, GB_2, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Alejandro Valdes]

avaldes updated this revision to Diff 58091.
avaldes added a comment.


  Added new Suspend then Hibernate option
  

  
  Summary:
  See bug 399727  for a good 
description of what this code is for.
  The new ui will show a new option like the following image F6349860: 
screenshot.png 
  FEATURE: 399727
  
  Reviewers: broulik, ngraham
  
   
  
  Subscribers: soriano, abalaji, graesslin, ngraham, plasma-devel
  

  
  Tags: #plasma 
  

  
  Differential Revision: https://phabricator.kde.org/D16425

REPOSITORY
  R122 Powerdevil

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16425?vs=58044&id=58091

BRANCH
  arcpatch-D16425_1

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

AFFECTED FILES
  daemon/actions/bundled/suspendsession.cpp
  daemon/actions/bundled/suspendsession.h
  daemon/actions/bundled/suspendsessionconfig.cpp
  daemon/actions/bundled/suspendsessionconfig.h
  daemon/backends/upower/login1suspendjob.cpp
  daemon/backends/upower/powerdevilupowerbackend.cpp
  daemon/backends/upower/upowersuspendjob.cpp
  daemon/org.freedesktop.PowerManagement.xml
  daemon/powerdevilbackendinterface.h
  daemon/powerdevilfdoconnector.cpp
  daemon/powerdevilfdoconnector.h
  daemon/powerdevilpowermanagement.cpp
  daemon/powerdevilpowermanagement.h

To: avaldes, broulik, ngraham
Cc: meven, soriano, abalaji, graesslin, ngraham, plasma-devel, jraleigh, GB_2, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Nathaniel Graham]

ngraham added a comment.


  UI looks good enough for now. But is this the full diff? It seems like 
something got lost. The whole patch should include the changes from all commits 
in your branch, not just the last one.

REPOSITORY
  R122 Powerdevil

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

To: avaldes, broulik, ngraham
Cc: meven, soriano, abalaji, graesslin, ngraham, plasma-devel, jraleigh, GB_2, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Alejandro Valdes]

avaldes added a comment.


  This is how it looks now @ngraham :
  
  F6822188: suspendThenHibernate.png 

REPOSITORY
  R122 Powerdevil

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

To: avaldes, broulik, ngraham
Cc: meven, soriano, abalaji, graesslin, ngraham, plasma-devel, jraleigh, GB_2, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Alejandro Valdes]

avaldes updated this revision to Diff 58044.
avaldes added a comment.


  Improved messages for suspend session

REPOSITORY
  R122 Powerdevil

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16425?vs=56024&id=58044

BRANCH
  arcpatch-D16425_1

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

AFFECTED FILES
  daemon/actions/bundled/suspendsessionconfig.cpp

To: avaldes, broulik, ngraham
Cc: meven, soriano, abalaji, graesslin, ngraham, plasma-devel, jraleigh, GB_2, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Nathaniel Graham]

ngraham added a comment.


  In D16425#464725 , @avaldes wrote:
  
  > In D16425#464381 , @ngraham 
wrote:
  >
  > > This thing's UI really needs to be rewritten in QML. Once we do that and 
give it a proper FormLayout style, the string can be shorter, but for now, 
seeing it in context, I feel like  the string should be "Hibernate after 3 
hours of sleep".
  > >
  > > Also I wouldn't mind making the period configurable. If you don't want to 
do that in this patch though, that's fine.
  >
  >
  > The period configuration can only be done by adding a .conf file to any of 
these locations with the parameter HibernateDelaySec, current dbus API doesn't 
have anything to pass or modify this value, so user intervention is required to 
change the default timeout of 3 hours.
  >
  >   /etc/systemd/sleep.conf
  >   /etc/systemd/sleep.conf.d/*.conf
  >   /run/systemd/sleep.conf.d/*.conf
  >   /usr/lib/systemd/sleep.conf.d/*.conf
  >
  >
  > @ngraham there is currently a limitation with my current patch, as a user 
you will always have to enable the suspend after a period of time (Suspend 
Session > After) to enable the Suspend-then-hibernate option. I think this is 
not a good default, I can also move this option to a new group similar to 
"Buttons event handling", which also modifies the suspend behaviour.
  
  
  Maybe something like this?
  
[ ] Automatically sleep [after 30 minutes | v]
[ ] While asleep, hibernate after 3 hours
``

REPOSITORY
  R122 Powerdevil

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

To: avaldes, broulik, ngraham
Cc: meven, soriano, abalaji, graesslin, ngraham, plasma-devel, jraleigh, GB_2, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Alejandro Valdes]

avaldes added a comment.


  In D16425#464381 , @ngraham wrote:
  
  > This thing's UI really needs to be rewritten in QML. Once we do that and 
give it a proper FormLayout style, the string can be shorter, but for now, 
seeing it in context, I feel like  the string should be "Hibernate after 3 
hours of sleep".
  >
  > Also I wouldn't mind making the period configurable. If you don't want to 
do that in this patch though, that's fine.
  
  
  The period configuration can only be done by adding a .conf file to any of 
these locations with the parameter HibernateDelaySec, current dbus API doesn't 
have anything to pass or modify this value, so user intervention is required to 
change the default timeout of 3 hours.
  
/etc/systemd/sleep.conf
/etc/systemd/sleep.conf.d/*.conf
/run/systemd/sleep.conf.d/*.conf
/usr/lib/systemd/sleep.conf.d/*.conf
  
  @ngraham there is currently a limitation with my current patch, as a user you 
will always have to enable the suspend after a period of time (Suspend Session 
> After) to enable the Suspend-then-hibernate option. I think this is not a 
good default, I can also move this option to a new group similar to "Buttons 
event handling", which also modifies the suspend behaviour.

REPOSITORY
  R122 Powerdevil

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

To: avaldes, broulik, ngraham
Cc: meven, soriano, abalaji, graesslin, ngraham, plasma-devel, jraleigh, GB_2, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Nathaniel Graham]

ngraham added a comment.


  This thing's UI really needs to be rewritten in QML. Once we do that and give 
it a proper FormLayout style, the string can be shorter, but for now, seeing it 
in context, I feel like  the string should be "Hibernate after 3 hours of 
sleep".
  
  Also I wouldn't mind making the period configurable. If you don't want to do 
that in this patch though, that's fine.

REPOSITORY
  R122 Powerdevil

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

To: avaldes, broulik, ngraham
Cc: meven, soriano, abalaji, graesslin, ngraham, plasma-devel, jraleigh, GB_2, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Méven Car]

meven added a comment.


  In D16425#449330 , @avaldes wrote:
  
  > This is how the module looks with the new option:
  >  F6770639: image.png 
  >  It looks the same as the "Even when an external monitor is connected", I 
tried using a HBox but it didn't change the alignment, maybe there is a 
restriction on how powerdevil builds the UI, but I'm no expert on that. if 
anyone has any idea on how to change it I'm open to suggestions.
  
  
  The feature looks great.
  
  But I wonder if #vdg  could suggest the 
improvement.
  "Even when an external monitor is connected" is already misaligned.
  But perhaps this setting window would need some redesign later on.
  Do you have an opinion @ngraham ?

REPOSITORY
  R122 Powerdevil

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

To: avaldes, broulik, ngraham
Cc: meven, soriano, abalaji, graesslin, ngraham, plasma-devel, jraleigh, GB_2, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Alejandro Valdes]

avaldes added a comment.


  In D16425#449026 , @meven wrote:
  
  > In D16425#448999 , @avaldes 
wrote:
  >
  > > I applied this patch to v5.15.4 tag and tested with that and it works (my 
machine is running plasma 5.15.4), I can change the brightness with the 
keyboard and set the suspend-then-hibernate option.
  > >
  > > F6769609: image.png 
  >
  >
  > Very nice.
  >  The position of the checkbox in the settings seems odd to me.
  >  It would be great to align it below the "After" field.
  >  Or perhaps the screenshot just does not justice to the UI.
  
  
  This is how the module looks with the new option:
  F6770639: image.png 
  It looks the same as the "Even when an external monitor is connected", I 
tried using a HBox but it didn't change the alignment, maybe there is a 
restriction on how powerdevil builds the UI, but I'm no expert on that. if 
anyone has any idea on how to change it I'm open to suggestions.

REPOSITORY
  R122 Powerdevil

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

To: avaldes, broulik, ngraham
Cc: meven, soriano, abalaji, graesslin, ngraham, plasma-devel, jraleigh, GB_2, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Méven Car]

meven added a comment.


  In D16425#448999 , @avaldes wrote:
  
  > I applied this patch to v5.15.4 tag and tested with that and it works (my 
machine is running plasma 5.15.4), I can change the brightness with the 
keyboard and set the suspend-then-hibernate option.
  >
  > F6769609: image.png 
  
  
  Very nice.
  The position of the checkbox in the settings seems odd to me.
  It would be great to align it below the "After" field.
  Or perhaps the screenshot just does not justice to the UI.

REPOSITORY
  R122 Powerdevil

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

To: avaldes, broulik, ngraham
Cc: meven, soriano, abalaji, graesslin, ngraham, plasma-devel, jraleigh, GB_2, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Alejandro Valdes]

avaldes added a comment.


  In D16425#448828 , @ngraham wrote:
  
  > In D16425#448822 , @avaldes 
wrote:
  >
  > > In D16425#448820 , @ngraham 
wrote:
  > >
  > > > So is everything now working for you, or not?
  > >
  > >
  > > No, I can't use my keyboard to change brightness, but it might be related 
on how I test my changes. I need guidance on how can I test powerdevil changes 
locally.
  >
  >
  > It's a plasma component, so this might be helpful: 
https://community.kde.org/Guidelines_and_HOWTOs/Build_from_source/Test_plasma
  
  
  I applied this patch to v5.15.4 tag and tested with that and it works (my 
machine is running plasma 5.15.4), I can change the brightness with the 
keyboard and set the suspend-then-hibernate option.
  
  F6769609: image.png 

REPOSITORY
  R122 Powerdevil

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

To: avaldes, broulik, ngraham
Cc: soriano, abalaji, graesslin, ngraham, plasma-devel, jraleigh, GB_2, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Nathaniel Graham]

ngraham added a comment.


  In D16425#448822 , @avaldes wrote:
  
  > In D16425#448820 , @ngraham 
wrote:
  >
  > > So is everything now working for you, or not?
  >
  >
  > No, I can't use my keyboard to change brightness, but it might be related 
on how I test my changes. I need guidance on how can I test powerdevil changes 
locally.
  
  
  It's a plasma component, so this might be helpful: 
https://community.kde.org/Guidelines_and_HOWTOs/Build_from_source/Test_plasma

REPOSITORY
  R122 Powerdevil

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

To: avaldes, broulik, ngraham
Cc: soriano, abalaji, graesslin, ngraham, plasma-devel, jraleigh, GB_2, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Alejandro Valdes]

avaldes added a comment.


  In D16425#448820 , @ngraham wrote:
  
  > So is everything now working for you, or not?
  
  
  No, I can't use my keyboard to change brightness, but it might be related on 
how I test my changes. I need guidance on how can I test powerdevil changes 
locally.

REPOSITORY
  R122 Powerdevil

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

To: avaldes, broulik, ngraham
Cc: soriano, abalaji, graesslin, ngraham, plasma-devel, jraleigh, GB_2, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Nathaniel Graham]

ngraham added a comment.


  So is everything now working for you, or not?

REPOSITORY
  R122 Powerdevil

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

To: avaldes, broulik, ngraham
Cc: soriano, abalaji, graesslin, ngraham, plasma-devel, jraleigh, GB_2, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Alejandro Valdes]

avaldes updated this revision to Diff 56024.
avaldes added a comment.


  Removed CMakeList change

REPOSITORY
  R122 Powerdevil

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16425?vs=56023&id=56024

BRANCH
  suspend-then-hibernate (branched from master)

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

AFFECTED FILES
  daemon/actions/bundled/suspendsession.cpp
  daemon/actions/bundled/suspendsession.h
  daemon/actions/bundled/suspendsessionconfig.cpp
  daemon/actions/bundled/suspendsessionconfig.h
  daemon/backends/upower/login1suspendjob.cpp
  daemon/backends/upower/powerdevilupowerbackend.cpp
  daemon/backends/upower/upowersuspendjob.cpp
  daemon/org.freedesktop.PowerManagement.xml
  daemon/powerdevilbackendinterface.h
  daemon/powerdevilfdoconnector.cpp
  daemon/powerdevilfdoconnector.h
  daemon/powerdevilpowermanagement.cpp
  daemon/powerdevilpowermanagement.h

To: avaldes, broulik, ngraham
Cc: soriano, abalaji, graesslin, ngraham, plasma-devel, jraleigh, GB_2, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Alejandro Valdes]

avaldes added a comment.


  @ngraham I updated the patch with the comments, I'm not sure if I'm updating 
it corrrectly, I have tested it on my machine and it can suspend, but for some 
reason the dedicated keys to change the brightness are not working on my 
laptop. I'm still checking if this is related on how I build and test locally 
powerdevil or is related with my changes.

REPOSITORY
  R122 Powerdevil

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

To: avaldes, broulik, ngraham
Cc: soriano, abalaji, graesslin, ngraham, plasma-devel, jraleigh, GB_2, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Alejandro Valdes]

avaldes updated this revision to Diff 56023.
avaldes edited the summary of this revision.
avaldes added a comment.


  Rebasing changes to master, adding latest comments

REPOSITORY
  R122 Powerdevil

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16425?vs=44950&id=56023

BRANCH
  suspend-then-hibernate (branched from master)

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

AFFECTED FILES
  CMakeLists.txt
  daemon/actions/bundled/suspendsession.cpp
  daemon/actions/bundled/suspendsession.h
  daemon/actions/bundled/suspendsessionconfig.cpp
  daemon/actions/bundled/suspendsessionconfig.h
  daemon/backends/upower/login1suspendjob.cpp
  daemon/backends/upower/powerdevilupowerbackend.cpp
  daemon/backends/upower/upowersuspendjob.cpp
  daemon/org.freedesktop.PowerManagement.xml
  daemon/powerdevilbackendinterface.h
  daemon/powerdevilfdoconnector.cpp
  daemon/powerdevilfdoconnector.h
  daemon/powerdevilpowermanagement.cpp
  daemon/powerdevilpowermanagement.h

To: avaldes, broulik, ngraham
Cc: soriano, abalaji, graesslin, ngraham, plasma-devel, jraleigh, GB_2, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Nathaniel Graham]

ngraham added a comment.


  Oh and I'm sorry that this patch has sat idle for so long. Once you make my 
requested change, I'll see if I can help push this along. Thanks for your 
patience!

REPOSITORY
  R122 Powerdevil

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

To: avaldes, broulik, ngraham
Cc: soriano, abalaji, graesslin, ngraham, plasma-devel, jraleigh, GB_2, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Nathaniel Graham]

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

INLINE COMMENTS

> suspendsessionconfig.cpp:82
> +m_suspendThenHibernateCheck = new QCheckBox(
> +i18nc("Hibernate after 3 hours when suspended", "Hibernate after 3 
> hours when suspended"));
>  QWidget *tempWidget = new QWidget;

1. The first argument is the context, which doesn't need to be the same string 
as the actual user-displayed string itself. It's mainly used for very short 
strings where the string itself may not illustrate on its own what the context 
is and how it should be translated. Since this string is descriptive enough, 
I'd just use a regular old `i18n("blablabla")`

2. We now use the word "Sleep" instead of "Suspend", and now that I look at 
this some more, I think a better string would be, "Sleep for 3 hours, then 
hibernate"

REPOSITORY
  R122 Powerdevil

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

To: avaldes, broulik, ngraham
Cc: soriano, abalaji, graesslin, ngraham, plasma-devel, jraleigh, GB_2, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Bryan Soriano]

soriano added a comment.


  Were these changes not committed? I still don't have a suspend-then-hibernate 
option on powerdevil. Is there any way I can enable it? Any suggestions would 
be much appreciated.

REPOSITORY
  R122 Powerdevil

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

To: avaldes, broulik
Cc: soriano, abalaji, graesslin, ngraham, plasma-devel, jraleigh, GB_2, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Kai Uwe Broulik]

broulik added a comment.


  I think that's because we're iterating `m_cookieToBusService` in that method 
and at the same time have `ReleaseInhibition` tamper with it. Perhaps taking a 
copy should fix that already:
  
void PolicyAgent::onServiceUnregistered(const QString& serviceName)
{
// Ouch - the application quit or crashed without releasing its 
inhibitions. Let's fix that.
const auto cookieToBusService = m_cookieToBusService;
for (auto it = cookieToBusService.constBegin(); it != 
cookieToBusService.constEnd(); ++it) {
if (it.value() == serviceName) {
ReleaseInhibition(it.key());
}
}
m_cookieToBusService.clear();
}

REPOSITORY
  R122 Powerdevil

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

To: avaldes, broulik
Cc: graesslin, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Alejandro Valdes]

avaldes updated this revision to Diff 44950.
avaldes added a comment.


  I have changed the message.
  
  Also, if anyone can help me, I'm having a segmentation fault with these 
changes when an application inhibits power saving. This does happen with or 
without having available hibernation in the machine. Is there any manual on how 
can I debug powerdevil? I wasn't able to find much when searching for it.
  
Application: org_kde_powerdevil (org_kde_powerdevil), signal: Segmentation 
fault
Using host libthread_db library "/usr/lib/libthread_db.so.1".
[Current thread is 1 (Thread 0x7fa4c8735840 (LWP 12369))]

Thread 5 (Thread 0x7fa4c4e18700 (LWP 12396)):
#0  0x7fa4ce93d7a4 in read () at /usr/lib/libc.so.6
#1  0x7fa4cced1781 in  () at /usr/lib/libglib-2.0.so.0
#2  0x7fa4ccf21a50 in g_main_context_check () at 
/usr/lib/libglib-2.0.so.0
#3  0x7fa4ccf22e86 in  () at /usr/lib/libglib-2.0.so.0
#4  0x7fa4ccf23f62 in g_main_loop_run () at /usr/lib/libglib-2.0.so.0
#5  0x7fa4c5d46c28 in  () at /usr/lib/libgio-2.0.so.0
#6  0x7fa4cceec3eb in  () at /usr/lib/libglib-2.0.so.0
#7  0x7fa4cdb66a9d in start_thread () at /usr/lib/libpthread.so.0
#8  0x7fa4ce94cb23 in clone () at /usr/lib/libc.so.6

Thread 4 (Thread 0x7fa4c5619700 (LWP 12393)):
#0  0x7fa4ce93d7a4 in read () at /usr/lib/libc.so.6
#1  0x7fa4cced1781 in  () at /usr/lib/libglib-2.0.so.0
#2  0x7fa4ccf21a50 in g_main_context_check () at 
/usr/lib/libglib-2.0.so.0
#3  0x7fa4ccf22e86 in  () at /usr/lib/libglib-2.0.so.0
#4  0x7fa4ccf22fce in g_main_context_iteration () at 
/usr/lib/libglib-2.0.so.0
#5  0x7fa4ccf23022 in  () at /usr/lib/libglib-2.0.so.0
#6  0x7fa4cceec3eb in  () at /usr/lib/libglib-2.0.so.0
#7  0x7fa4cdb66a9d in start_thread () at /usr/lib/libpthread.so.0
#8  0x7fa4ce94cb23 in clone () at /usr/lib/libc.so.6

Thread 3 (Thread 0x7fa4c675e700 (LWP 12388)):
#0  0x7fa4ce93d7a4 in read () at /usr/lib/libc.so.6
#1  0x7fa4cced1781 in  () at /usr/lib/libglib-2.0.so.0
#2  0x7fa4ccf21a50 in g_main_context_check () at 
/usr/lib/libglib-2.0.so.0
#3  0x7fa4ccf22e86 in  () at /usr/lib/libglib-2.0.so.0
#4  0x7fa4ccf22fce in g_main_context_iteration () at 
/usr/lib/libglib-2.0.so.0
#5  0x7fa4cee68fe4 in 
QEventDispatcherGlib::processEvents(QFlags) () 
at /usr/lib/libQt5Core.so.5
#6  0x7fa4cee148cc in 
QEventLoop::exec(QFlags) () at 
/usr/lib/libQt5Core.so.5
#7  0x7fa4cec5deb9 in QThread::exec() () at /usr/lib/libQt5Core.so.5
#8  0x7fa4cf0baba6 in  () at /usr/lib/libQt5DBus.so.5
#9  0x7fa4cec67f65 in  () at /usr/lib/libQt5Core.so.5
#10 0x7fa4cdb66a9d in start_thread () at /usr/lib/libpthread.so.0
#11 0x7fa4ce94cb23 in clone () at /usr/lib/libc.so.6

Thread 2 (Thread 0x7fa4c737d700 (LWP 12378)):
#0  0x7fa4ce941c21 in poll () at /usr/lib/libc.so.6
#1  0x7fa4ce053630 in  () at /usr/lib/libxcb.so.1
#2  0x7fa4ce0552db in xcb_wait_for_event () at /usr/lib/libxcb.so.1
#3  0x7fa4c82e3c5a in  () at /usr/lib/libQt5XcbQpa.so.5
#4  0x7fa4cec67f65 in  () at /usr/lib/libQt5Core.so.5
#5  0x7fa4cdb66a9d in start_thread () at /usr/lib/libpthread.so.0
#6  0x7fa4ce94cb23 in clone () at /usr/lib/libc.so.6

Thread 1 (Thread 0x7fa4c8735840 (LWP 12369)):
[KCrash Handler]
#6  0x7fa4cecb2523 in QHashData::nextNode(QHashData::Node*) () at 
/usr/lib/libQt5Core.so.5
#7  0x7fa4cfe0f8e5 in 
PowerDevil::PolicyAgent::onServiceUnregistered(QString const&) () at 
/usr/lib/libpowerdevilcore.so.2
#8  0x7fa4cfe3ac8a in  () at /usr/lib/libpowerdevilcore.so.2
#9  0x7fa4cee3fa7c in QMetaObject::activate(QObject*, int, int, void**) 
() at /usr/lib/libQt5Core.so.5
#10 0x7fa4cf118626 in QDBusServiceWatcher::serviceUnregistered(QString 
const&) () at /usr/lib/libQt5DBus.so.5
#11 0x7fa4cf118e8b in  () at /usr/lib/libQt5DBus.so.5
#12 0x7fa4cf119293 in 
QDBusServiceWatcher::qt_metacall(QMetaObject::Call, int, void**) () at 
/usr/lib/libQt5DBus.so.5
#13 0x7fa4cf0c69ef in  () at /usr/lib/libQt5DBus.so.5
#14 0x7fa4cee40352 in QObject::event(QEvent*) () at 
/usr/lib/libQt5Core.so.5
#15 0x7fa4cee15c39 in QCoreApplication::notifyInternal2(QObject*, 
QEvent*) () at /usr/lib/libQt5Core.so.5
#16 0x7fa4cee18ccc in 
QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) () at 
/usr/lib/libQt5Core.so.5
#17 0x7fa4cee699d4 in  () at /usr/lib/libQt5Core.so.5
#18 0x7fa4ccf213cf in g_main_context_dispatch () at 
/usr/lib/libglib-2.0.so.0
#19 0x7fa4ccf22f89 in  () at /usr/lib/libglib-2.0.so.0
#20 0x7fa4ccf22fce in g_main_context_iteration () at 
/usr/lib/libglib-2.0.so.0
#21 0x7fa4cee68fc9 in 
QEventDispatcherGlib::processEvents(QFlags) () 
at /

D16425: Added new Suspend then Hibernate option

[Nathaniel Graham]

ngraham added a comment.


  If the default is 3 hours, then we need to either provide a GUI facility to 
change that, or, if we can't, then we need to mention the time in the 
checkbox's label, e.g. `Hibernate after 3 hours when suspended`

REPOSITORY
  R122 Powerdevil

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

To: avaldes, broulik
Cc: graesslin, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Alejandro Valdes]

avaldes added a comment.


  In D16425#349588 , @ngraham wrote:
  
  > We'll need a spinbox to display time options for the amount of delay before 
hibernating if this is the UI we go with. But I kinda like Martin's idea and 
present this in the form of an additional control sort of like this:
  >
  >   While suspended, hibernate after: [combobox with some carefully selected 
intervals, plus "Never", which is the default choice]
  >
  >
  > Also, how does this handle hardware or distro configurations that don't 
support hibernation? I might suggest that it would be best to hide the option 
entirely in such a case. On this subject, the HIG says:
  >
  > > If some of the program’s settings are only applicable in certain 
contexts, do not hide the inapplicable ones. Instead, disable them and hint to 
the user why they’re disabled. **Exception:** it is acceptable to hide settings 
for non-existent hardware. For example, it’s okay to hide the touchpad 
configuration when no touchpad is present.
  >
  > It's not //exctly// the same thing, but I think the principle applies 
here.
  
  
  If the hardware you are running doesn't support hibernation, logind 
canSupportThenHibernate method should return no, I tested this code in a 
machine without hibernation and it currently hides the new option. I added some 
checks before saving so we don't accidentally enable this suspend option by 
error.
  
  On the amount of time to hibernate, afaik this can only be configured by 
modifying /etc/systemd/sleep.conf and adding HibernateDelaySec= with the amount 
of seconds that you want to wait to hibernate, by default the wait time is 3 
hours. I have no knowledge of an API to configure this time.

REPOSITORY
  R122 Powerdevil

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

To: avaldes, broulik
Cc: graesslin, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Nathaniel Graham]

ngraham added a comment.


  We'll need a spinbox to display time options for the amount of delay before 
hibernating if this is the UI we go with. But I kinda like Martin's idea and 
present this in the form of an additional control sort of like this:
  
While suspended, hibernate after: [combobox with some carefully selected 
intervals, plus "Never", which is the default choice]
  
  Also, how does this handle hardware or distro configurations that don't 
support hibernation? I might suggest that it would be best to hide the option 
entirely in such a case. On this subject, the HIG says:
  
  > If some of the program’s settings are only applicable in certain contexts, 
do not hide the inapplicable ones. Instead, disable them and hint to the user 
why they’re disabled. **Exception:** it is acceptable to hide settings for 
non-existent hardware. For example, it’s okay to hide the touchpad 
configuration when no touchpad is present.
  
  It's not //exctly// the same thing, but I think the principle applies 
here.

REPOSITORY
  R122 Powerdevil

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

To: avaldes, broulik
Cc: graesslin, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Alejandro Valdes]

avaldes updated this revision to Diff 44294.
avaldes added a comment.


  I have added a new checkbox inside the suspend session delay and in button 
event handling.
  
  I wasn't able to make it work with the button event handling, but the 
checkbox does work with the logic when Suspend Session is enabled.
  
  Let me know if you know of a better way to do this.
  
  Here's how it looks now: F6364813: sth-checkbox.png 


REPOSITORY
  R122 Powerdevil

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16425?vs=44208&id=44294

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

AFFECTED FILES
  daemon/actions/bundled/handlebuttonevents.cpp
  daemon/actions/bundled/handlebuttonevents.h
  daemon/actions/bundled/handlebuttoneventsconfig.cpp
  daemon/actions/bundled/handlebuttoneventsconfig.h
  daemon/actions/bundled/suspendsession.cpp
  daemon/actions/bundled/suspendsession.h
  daemon/actions/bundled/suspendsessionconfig.cpp
  daemon/actions/bundled/suspendsessionconfig.h
  daemon/org.freedesktop.PowerManagement.xml
  daemon/powerdevilfdoconnector.cpp
  kcmodule/activities/activitywidget.cpp

To: avaldes, broulik
Cc: graesslin, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Martin Flöser]

graesslin added a comment.


  I would suggest to turn "suspend then hibernate" into the new suspend. And 
one just can specify in the ui how long it takes till hibernate (which could 
also be endless).

REPOSITORY
  R122 Powerdevil

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

To: avaldes, broulik
Cc: graesslin, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Nathaniel Graham]

ngraham added a comment.


  In D16425#348775 , @broulik wrote:
  
  > Wouldn't it make sense to have a checkbox in PowerDevil advanced settings 
to have "Suspend" be "Suspend then Hibernate", or do you only want that when 
pressing a button but not use it in other circumstances?
  
  
  I agree, that would be a nicer UI.

REPOSITORY
  R122 Powerdevil

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

To: avaldes, broulik
Cc: ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Kai Uwe Broulik]

broulik added a comment.


  We basically just ask logind and it also handles all the background work for 
it, nothing on our side needed.
  
  I'm just not sure we should treat it as "yet another suspend option". 
Wouldn't it make sense to have a checkbox in PowerDevil advanced settings to 
have "Suspend" be "Suspend then Hibernate", or do you only want that when 
pressing a button but not use it in other circumstances?

REPOSITORY
  R122 Powerdevil

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

To: avaldes, broulik
Cc: ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Alejandro Valdes]

avaldes added a comment.


  @ngraham afaik powerdevil asks logind for system capabilites, with the 
CanSuspend, CanHibernate, etc. defined here 
.
 I added a new option if logind reports yes for CanSuspendThenHibernate option.

REPOSITORY
  R122 Powerdevil

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

To: avaldes, broulik
Cc: ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Nathaniel Graham]

ngraham added a comment.


  Very interesting.  Is there a programmatic way to detect hardware that would 
benefit from this?

REPOSITORY
  R122 Powerdevil

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

To: avaldes, broulik
Cc: ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D16425: Added new Suspend then Hibernate option

[Alejandro Valdes]

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

REVISION SUMMARY
  See bug 399727  for a good 
description of what this code is for.
  
  The new ui will show a new option like the following image F6349860: 
screenshot.png 

REPOSITORY
  R122 Powerdevil

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

AFFECTED FILES
  daemon/actions/bundled/handlebuttoneventsconfig.cpp
  daemon/actions/bundled/suspendsession.cpp
  daemon/actions/bundled/suspendsession.h
  daemon/actions/bundled/suspendsessionconfig.cpp
  daemon/backends/upower/login1suspendjob.cpp
  daemon/backends/upower/powerdevilupowerbackend.cpp
  daemon/backends/upower/upowersuspendjob.cpp
  daemon/org.freedesktop.PowerManagement.xml
  daemon/powerdevilbackendinterface.h
  daemon/powerdevilfdoconnector.cpp
  daemon/powerdevilfdoconnector.h
  daemon/powerdevilpowermanagement.cpp
  daemon/powerdevilpowermanagement.h
  kcmodule/activities/activitywidget.cpp

To: avaldes, broulik
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart