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!

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

So is everything now working for you, or not?

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.

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

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.

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.

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.

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.

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:

In D16425#449330, @avaldes wrote:

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.

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.

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

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.

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.

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

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.

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

Added changes based on recent comments.

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

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.

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.

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

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

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 !