Added new Suspend then Hibernate option
ClosedPublic

Authored by avaldes on Oct 25 2018, 12:27 PM.

Details

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

FEATURE: 399727

Diff Detail

Repository
R122 Powerdevil
Lint
Lint Skipped
Unit
Unit Tests Skipped
There are a very large number of changes, so older changes are hidden. Show Older Changes
ngraham requested changes to this revision.Apr 11 2019, 2:03 PM
ngraham added inline comments.
daemon/actions/bundled/suspendsessionconfig.cpp
82
  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")
  1. 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"
This revision now requires changes to proceed.Apr 11 2019, 2:03 PM

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!

avaldes updated this revision to Diff 56023.Apr 11 2019, 10:50 PM
avaldes edited the summary of this revision. (Show Details)

Rebasing changes to master, adding latest comments

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

avaldes updated this revision to Diff 56024.Apr 11 2019, 10:58 PM

Removed CMakeList change

So is everything now working for you, or not?

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.

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

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.

meven edited the summary of this revision. (Show Details)Apr 13 2019, 6:56 AM
meven added a subscriber: meven.Apr 13 2019, 7:12 AM

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.

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.

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.

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:


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.

meven added a comment.May 8 2019, 2:13 PM

This is how the module looks with the new option:


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 ?

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.

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.

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
``
avaldes updated this revision to Diff 58044.May 14 2019, 1:48 AM

Improved messages for suspend session

This is how it looks now @ngraham :

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.

avaldes updated this revision to Diff 58091.May 14 2019, 4:11 PM

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


FEATURE: 399727

Reviewers: broulik, ngraham

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

Tags: Plasma

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

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.

Hi,

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

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.

ngraham accepted this revision.May 17 2019, 4:34 PM

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?

daemon/powerdevilpowermanagement.cpp
207 ↗(On Diff #58091)

space after if

210 ↗(On Diff #58091)

space after if

This revision is now accepted and ready to land.May 17 2019, 4:34 PM
avaldes updated this revision to Diff 58273.May 19 2019, 2:20 AM

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

Reviewers: broulik, ngraham

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

Tags: Plasma

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

avaldes marked 3 inline comments as done.May 19 2019, 3:11 AM

Updated with latest comments.

Please land, I don't have permissions.

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

abalaji added inline comments.May 29 2019, 9:49 PM
daemon/actions/bundled/suspendsession.cpp
137

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

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

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

daemon/actions/bundled/suspendsessionconfig.cpp
57

Again

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

Stray line swap

121

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

avaldes updated this revision to Diff 58969.Jun 1 2019, 2:50 AM

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

FEATURE: 399727

Reviewers: broulik, ngraham

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

Tags: Plasma

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

avaldes marked 5 inline comments as done.Jun 1 2019, 2:57 AM

Added changes based on recent comments.

daemon/actions/bundled/suspendsessionconfig.cpp
106

Done on purpose to change the UI:

Automatically, m_comboBox, m_idleTime

121

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

abalaji added inline comments.Jun 4 2019, 4:11 AM
daemon/actions/bundled/suspendsessionconfig.cpp
106

Oh i see, that actually makes sense

121

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.

122

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

avaldes updated this revision to Diff 59170.Jun 5 2019, 3:21 AM
avaldes marked 2 inline comments as done.

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

Reviewers: broulik, ngraham

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

Tags: Plasma

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

avaldes marked 5 inline comments as done.Jun 5 2019, 3:23 AM

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

abalaji added inline comments.Jun 5 2019, 3:50 AM
daemon/actions/bundled/suspendsessionconfig.cpp
45

Move this to the initializer

avaldes added inline comments.Jun 5 2019, 10:02 PM
daemon/actions/bundled/suspendsessionconfig.cpp
45

something like this?

: ActionConfig(parent),
  m_suspendThenHibernateEnabled(nullptr)
abalaji added inline comments.Jun 6 2019, 2:37 AM
daemon/actions/bundled/suspendsessionconfig.cpp
45

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

avaldes updated this revision to Diff 59241.Jun 6 2019, 3:59 AM

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

Reviewers: broulik, ngraham

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

Tags: Plasma

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

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.

avaldes marked 3 inline comments as done.Jun 6 2019, 10:25 PM

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.

meven added a comment.Jun 27 2019, 3:09 PM

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
meven accepted this revision.Jun 29 2019, 10:12 AM

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

davidedmundson accepted this revision.Jul 1 2019, 1:08 PM
davidedmundson added a subscriber: davidedmundson.

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"

sitter accepted this revision.Jul 1 2019, 1:30 PM
sitter added a subscriber: sitter.

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!

avaldes updated this revision to Diff 60926.Jul 1 2019, 2:06 PM

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

Reviewers: broulik, ngraham

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

Tags: Plasma

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

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

meven added a comment.Jul 1 2019, 5:32 PM

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 !

This revision was automatically updated to reflect the committed changes.
eseifert removed a subscriber: eseifert.
jfalke awarded a token.Tue, Oct 8, 9:05 AM
jfalke added a subscriber: jfalke.