Added new Suspend then Hibernate option
Needs ReviewPublic

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

Details

Reviewers
broulik
ngraham
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
Branch
suspend-then-hibernate (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 10746
Build 10764: arc lint + arc unit
avaldes created this revision.Oct 25 2018, 12:27 PM
Restricted Application added a project: Plasma. · View Herald TranscriptOct 25 2018, 12:27 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
avaldes requested review of this revision.Oct 25 2018, 12:27 PM

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

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

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?

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.

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

avaldes updated this revision to Diff 44294.EditedOct 27 2018, 9:27 AM

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

I wasn't able to make it work inside the button event handling config, but the checkbox does work with the logic when added to Suspend Session config and it is enabled.

Let me know if you know of a better way to do this.

Here's how it looks now:

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.

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.

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.

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

avaldes updated this revision to Diff 44950.Nov 6 2018, 9:00 AM

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  0x00007fa4ce93d7a4 in read () at /usr/lib/libc.so.6
#1  0x00007fa4cced1781 in  () at /usr/lib/libglib-2.0.so.0
#2  0x00007fa4ccf21a50 in g_main_context_check () at /usr/lib/libglib-2.0.so.0
#3  0x00007fa4ccf22e86 in  () at /usr/lib/libglib-2.0.so.0
#4  0x00007fa4ccf23f62 in g_main_loop_run () at /usr/lib/libglib-2.0.so.0
#5  0x00007fa4c5d46c28 in  () at /usr/lib/libgio-2.0.so.0
#6  0x00007fa4cceec3eb in  () at /usr/lib/libglib-2.0.so.0
#7  0x00007fa4cdb66a9d in start_thread () at /usr/lib/libpthread.so.0
#8  0x00007fa4ce94cb23 in clone () at /usr/lib/libc.so.6

Thread 4 (Thread 0x7fa4c5619700 (LWP 12393)):
#0  0x00007fa4ce93d7a4 in read () at /usr/lib/libc.so.6
#1  0x00007fa4cced1781 in  () at /usr/lib/libglib-2.0.so.0
#2  0x00007fa4ccf21a50 in g_main_context_check () at /usr/lib/libglib-2.0.so.0
#3  0x00007fa4ccf22e86 in  () at /usr/lib/libglib-2.0.so.0
#4  0x00007fa4ccf22fce in g_main_context_iteration () at /usr/lib/libglib-2.0.so.0
#5  0x00007fa4ccf23022 in  () at /usr/lib/libglib-2.0.so.0
#6  0x00007fa4cceec3eb in  () at /usr/lib/libglib-2.0.so.0
#7  0x00007fa4cdb66a9d in start_thread () at /usr/lib/libpthread.so.0
#8  0x00007fa4ce94cb23 in clone () at /usr/lib/libc.so.6

Thread 3 (Thread 0x7fa4c675e700 (LWP 12388)):
#0  0x00007fa4ce93d7a4 in read () at /usr/lib/libc.so.6
#1  0x00007fa4cced1781 in  () at /usr/lib/libglib-2.0.so.0
#2  0x00007fa4ccf21a50 in g_main_context_check () at /usr/lib/libglib-2.0.so.0
#3  0x00007fa4ccf22e86 in  () at /usr/lib/libglib-2.0.so.0
#4  0x00007fa4ccf22fce in g_main_context_iteration () at /usr/lib/libglib-2.0.so.0
#5  0x00007fa4cee68fe4 in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () at /usr/lib/libQt5Core.so.5
#6  0x00007fa4cee148cc in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () at /usr/lib/libQt5Core.so.5
#7  0x00007fa4cec5deb9 in QThread::exec() () at /usr/lib/libQt5Core.so.5
#8  0x00007fa4cf0baba6 in  () at /usr/lib/libQt5DBus.so.5
#9  0x00007fa4cec67f65 in  () at /usr/lib/libQt5Core.so.5
#10 0x00007fa4cdb66a9d in start_thread () at /usr/lib/libpthread.so.0
#11 0x00007fa4ce94cb23 in clone () at /usr/lib/libc.so.6

Thread 2 (Thread 0x7fa4c737d700 (LWP 12378)):
#0  0x00007fa4ce941c21 in poll () at /usr/lib/libc.so.6
#1  0x00007fa4ce053630 in  () at /usr/lib/libxcb.so.1
#2  0x00007fa4ce0552db in xcb_wait_for_event () at /usr/lib/libxcb.so.1
#3  0x00007fa4c82e3c5a in  () at /usr/lib/libQt5XcbQpa.so.5
#4  0x00007fa4cec67f65 in  () at /usr/lib/libQt5Core.so.5
#5  0x00007fa4cdb66a9d in start_thread () at /usr/lib/libpthread.so.0
#6  0x00007fa4ce94cb23 in clone () at /usr/lib/libc.so.6

Thread 1 (Thread 0x7fa4c8735840 (LWP 12369)):
[KCrash Handler]
#6  0x00007fa4cecb2523 in QHashData::nextNode(QHashData::Node*) () at /usr/lib/libQt5Core.so.5
#7  0x00007fa4cfe0f8e5 in PowerDevil::PolicyAgent::onServiceUnregistered(QString const&) () at /usr/lib/libpowerdevilcore.so.2
#8  0x00007fa4cfe3ac8a in  () at /usr/lib/libpowerdevilcore.so.2
#9  0x00007fa4cee3fa7c in QMetaObject::activate(QObject*, int, int, void**) () at /usr/lib/libQt5Core.so.5
#10 0x00007fa4cf118626 in QDBusServiceWatcher::serviceUnregistered(QString const&) () at /usr/lib/libQt5DBus.so.5
#11 0x00007fa4cf118e8b in  () at /usr/lib/libQt5DBus.so.5
#12 0x00007fa4cf119293 in QDBusServiceWatcher::qt_metacall(QMetaObject::Call, int, void**) () at /usr/lib/libQt5DBus.so.5
#13 0x00007fa4cf0c69ef in  () at /usr/lib/libQt5DBus.so.5
#14 0x00007fa4cee40352 in QObject::event(QEvent*) () at /usr/lib/libQt5Core.so.5
#15 0x00007fa4cee15c39 in QCoreApplication::notifyInternal2(QObject*, QEvent*) () at /usr/lib/libQt5Core.so.5
#16 0x00007fa4cee18ccc in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) () at /usr/lib/libQt5Core.so.5
#17 0x00007fa4cee699d4 in  () at /usr/lib/libQt5Core.so.5
#18 0x00007fa4ccf213cf in g_main_context_dispatch () at /usr/lib/libglib-2.0.so.0
#19 0x00007fa4ccf22f89 in  () at /usr/lib/libglib-2.0.so.0
#20 0x00007fa4ccf22fce in g_main_context_iteration () at /usr/lib/libglib-2.0.so.0
#21 0x00007fa4cee68fc9 in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () at /usr/lib/libQt5Core.so.5
#22 0x00007fa4c8379da2 in  () at /usr/lib/libQt5XcbQpa.so.5
#23 0x00007fa4cee148cc in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () at /usr/lib/libQt5Core.so.5
#24 0x00007fa4cee1cbc6 in QCoreApplication::exec() () at /usr/lib/libQt5Core.so.5
#25 0x000055c94f91d0c6 in  ()
#26 0x00007fa4ce875223 in __libc_start_main () at /usr/lib/libc.so.6
#27 0x000055c94f91d12e in _start ()
[Inferior 1 (process 12369) detached]
broulik added a comment.EditedNov 28 2018, 3:05 PM

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());
        }
    }
    // and here remove them from m_cookieToBusService
}
abalaji added a subscriber: abalaji.

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.

ngraham requested changes to this revision.Thu, Apr 11, 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.Thu, Apr 11, 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.Thu, Apr 11, 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.Thu, Apr 11, 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)Sat, Apr 13, 6:56 AM
meven added a subscriber: meven.Sat, Apr 13, 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.