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"

Summary:
See bug 399727 for a good description of what this code is for.

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.

Summary:
See bug 399727 for a good description of what this code is for.

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 new Suspend then Hibernate option

Added changes based on recent comments.

Added new Suspend then Hibernate option

Updated with latest comments.

Added new Suspend then Hibernate option

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.

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.

Added new Suspend then Hibernate option

This is how it looks now @ngraham :

Improved messages for suspend session

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.

I can't land it, please merge @jgrulich

In D21111#463270, @jgrulich wrote:

In D21111#463216, @avaldes wrote:

networkmanager-openconnect has added support in master for global protect, so now we can use it from plasma.

This was tested in my machine with Plasma 5.15.5 and the lastest networkmanager-openconnect.

The VPN works but I'm having an issue with the existing configuration options:

Unselecting the option "Use only for resources on this connection" in IPv4 > Routes doesn't work, I can access the network resources only available at the VPN but all my internet connections goes to the default internet route. I was able to solve this temporally by modifying the kernel routes. Is this intended?

Do you mean this change didn't apply and when you opened the configuration again was still selected? If the change did apply and after you open the configuration again it was set as it was supposed to be, then it's most likely a NM issue.

I also tried changing the priority of the VPN to 0 and the default network connection to 2 but it didn't work.

It's autoconnect priority, this means that a connection with a higher priority will be automatically activated after restart instead of any other connection with a lower priority if available.

• Changes based on code review

networkmanager-openconnect has added support in master for global protect, so now we can use it from plasma.

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.

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

In D16425#448820, @ngraham wrote:

So is everything now working for you, or not?

Removed CMakeList change

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

Rebasing changes to master, adding latest comments

I have changed the message.

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 exaaaactly the same thing, but I think the principle applies here.

I have added a new checkbox inside the suspend session delay and in button event handling.

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