D16425: Added new Suspend then Hibernate option

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

D16425: Added new Suspend then Hibernate option

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

D16425: Added new Suspend then Hibernate option

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

D16425: Added new Suspend then Hibernate option

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

D16425: Added new Suspend then Hibernate option

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 wor

D16425: Added new Suspend then Hibernate option

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 bett

D16425: Added new Suspend then Hibernate option

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,

D16425: Added new Suspend then Hibernate option

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 CanSusp

D16425: Added new Suspend then Hibernate option

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

D16425: Added new Suspend then Hibernate option

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://

D16425: Added new Suspend then Hibernate option

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

D16425: Added new Suspend then Hibernate option

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

D16425: Added new Suspend then Hibernate option

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 htt

D16425: Added new Suspend then Hibernate option

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, br

D16425: Added new Suspend then Hibernate option

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

D16425: Added new Suspend then Hibernate option

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

D16425: Added new Suspend then Hibernate option

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

D16425: Added new Suspend then Hibernate option

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

D16425: Added new Suspend then Hibernate option

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

D16425: Added new Suspend then Hibernate option

abalaji added inline comments. INLINE COMMENTS > suspendsession.cpp:137 > Q_EMIT aboutToSuspend(); > -suspendJob = > backend()->suspend(PowerDevil::BackendInterface::ToRam); > +if (m_suspendThenHibernateEnabled) { > +suspendJob = > backend()-

D16425: Added new Suspend then Hibernate option

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, sori

D16425: Added new Suspend then Hibernate option

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: reveren

D16425: Added new Suspend then Hibernate option

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 th

D16425: Added new Suspend then Hibernate option

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

D16425: Added new Suspend then Hibernate option

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?

D16425: Added new Suspend then Hibernate option

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

D16425: Added new Suspend then Hibernate option

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 las

D16425: Added new Suspend then Hibernate option

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 f

D16425: Added new Suspend then Hibernate option

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/D

D16425: Added new Suspend then Hibernate option

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,

D16425: Added new Suspend then Hibernate option

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

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 FormL

D16425: Added new Suspend then Hibernate option

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

D16425: Added new Suspend then Hibernate option

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 m

D16425: Added new Suspend then Hibernate option

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

D16425: Added new Suspend then Hibernate option

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 plas

D16425: Added new Suspend then Hibernate option

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

D16425: Added new Suspend then Hibernate option

avaldes added a comment. In D16425#448828 , @ngraham wrote: > In D16425#448822 , @avaldes wrote: > > > In D16425#448820 , @ngraham wrote: > > > > > So

D16425: Added new Suspend then Hibernate option

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

D16425: Added new Suspend then Hibernate option

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 t

D16425: Added new Suspend then Hibernate option

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,

D16425: Added new Suspend then Hibernate option

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://phabr

D16425: Added new Suspend then Hibernate option

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

D16425: Added new Suspend then Hibernate option

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

D16425: Added new Suspend then Hibernate option

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, n

D16425: Added new Suspend then Hibernate option

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

D16425: Added new Suspend then Hibernate option

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: av

D16425: Added new Suspend then Hibernate option

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

D16425: Added new Suspend then Hibernate option

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 th

D16425: Added new Suspend then Hibernate option

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://p

D16425: Added new Suspend then Hibernate option

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

D16425: Added new Suspend then Hibernate option

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: [comb

D16425: Added new Suspend then Hibernate option

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

D16425: Added new Suspend then Hibernate option

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

D16425: Added new Suspend then Hibernate option

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

D16425: Added new Suspend then Hibernate option

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 "

D16425: Added new Suspend then Hibernate option

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 CanSuspendThenHib

D16425: Added new Suspend then Hibernate option

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

D16425: Added new Suspend then Hibernate option

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