Created attachment 5780 Patch I made changes to xfce4-power-manager to support the hybrid sleep feature of newer Linux kernels, currently located here: https://github.com/nkreipke/xfce4-power-manager It works well on my system, maybe it is interesting enough to include in the official version.
Created attachment 5941 Rebased patch Hey Nico, Thanks for your patch. I've rebased it against current Git. I have some questions for you: 1. in xfpm-xfconf.c, why is it that suspend/hibernate are not yet installed but you install HYBRID_SLEEP? Could it be that there is a better time to install this value to the property of lid-related actions later? How are suspend and hibernate added right now? 2. in xfpm_settings_on_ac, you originally did not expose hybrid sleep. Was that an omission or was there a reason I'm missing? My patch adds it. 3. if I select hybrid sleep on the "System" tab, "System sleep mode" line, I get: (xfce4-power-manager:6940): GLib-GObject-WARNING **: value "5" of type 'guint' is invalid or out of range for property 'inactivity-sleep-mode-on-battery' of type 'guint' (xfce4-power-manager:6940): GLib-GObject-WARNING **: value "5" of type 'guint' is invalid or out of range for property 'inactivity-sleep-mode-on-ac' of type 'guint' Thanks.
Hi Steve, thanks for looking into this. I'll try to answer your questions: 1. g_param_spec_uint expects a minimum and maximum value. I had to raise its previous maximum of LID_TRIGGER_LOCK_SCREEN to LID_TRIGGER_HYBRID_SLEEP as setting it to this value would otherwise have failed. Values for suspend and hibernate do not need to be explicitly installed. 2. I think you missed my change in this one and exposed the value at the wrong position. My change was intended for the lid switch settings and can be found in line 1162. 3. The value for hybrid sleep is only valid for the lid switch settings. This error must be caused by the same problem as in question 2. Yours, Nico
Hey, I did swap around some values because I didn't understand why you used a constant from a different enum than for other actions (i.e. you did not use XFPM_DO_HYBRID_SLEEP). I wasn't sure given the patch's age and possible changes in the coding style of xfpm what the difference was due to. I rebuilt and the patch applies without errors and does not cause runtime errors so I might have made a mistake before. Your patch was meant to apply as a setting for when the computer has been inactive, whether on ac or battery, wasn't it? There were only two exposure points in the settings? I quite like having the option but we should really propose it everywhere in the settings UI if we're going to propose it. I'm leaving this as is for now, quite busy elsewhere especially as the patch can't make it for 4.12 now :-( If there's any chance that you add menu items for the other menus, it'd be very kind of you :-) To anyone reviewing the patch: please ask yourselves whether xfpm-enum-glib.h is ok. There might be need to add a XFPM_TRIGGER_SHUTDOWN for consistency or to add an explicit comment explaining if/why the values between both enums should be synchronised (also why are there two enums at all!?).
Okay, the reason why I had to synchronize the values between the two enums in xfpm-enum-glib.h is that in xfpm-manager.c, line 382, there is a value of type XfpmLidTriggerAction passed to xfpm_manager_sleep_request which expects an XfpmShutdownRequest. This previously just "happend to not fail" because the enum members were mapped to the same integer value. As the enums are also used to store the desired action in the user settings, my way was the simplest solution that did not break backwards compatibility. I did not understand either why this is the case and why we need multiple enums which apparently do the same thing, but differently, but this must be a pain to maintain. I originally intended hybrid sleep to only be available when the laptop lid is closed because it did not make sense in other situations, which admittedly was not well thought out. I'll try to do something about xfpm-enum-glib.h first before I update the patch.
Thanks for your reply Nico. I do hope you're not put off from improving the patch and pushing it into Xfce, as the feature itself would be awesome to have. I just think it's better if we are all convinced the newly added code is robust :) (ps: feel free to assign the bug to yourself so we know you're working on it)
Since we could theoretically merge this in the 1.5 cycle, any news on this?
Created attachment 8423 Patch to allow Hybrid Sleep in Powermanager on power button etc This bug is duplicate of https://bugzilla.xfce.org/show_bug.cgi?id=13076
Created attachment 8425 XFCE4 power manager with patch for Hybrid sleep applied (on laptop)
Created attachment 8426 diff -u Patch to allow Hybrid Sleep in Powermanager on power button etc patch made with diff -u
While nobody's asked to support it, I've noticed systemd-sleep does support a fourth sleep mode beyond suspend, hibernate and hybrid-sleep: suspend-then-hibernate - A low power state where the system is initially suspended (the state is stored in RAM). If not interrupted within the delay specified by HibernateDelaySec=, the system will be woken using an RTC alarm and hibernated (the state is then stored on disk). I can only guess if nobody's asked for this because they don't need it or don't know that there is such an option. I guess Xfce4 could support this, not that I see a need. Supporting hybrid-sleep first is probably a good idea regardless.
*** Bug 13076 has been marked as a duplicate of this bug. ***
I still have to let logind handle the lid switch for hybrid sleep when closing the lid. It would be nice, if the power-manager could handle this.
I am also a user of this feature (and above patch) and would like to see it included. Is there any way I can help?
Created attachment 9409 Hybrid Suspend patch
Created attachment 9411 git format-patch
Thanks for the patch ! Seems to work fine on my laptop. So bsd's will not see the option at all ? I'll review the code this / next evening and see if I can push it.
The BSD case is one I did not at all solve. They will, with that patch, get an option called Hybrid Suspend which results in Hibernate. Perhaps that part should be handled differently. #ifdef BACKEND_TYPE_FREEBSD #define UP_BACKEND_SUSPEND_COMMAND "/usr/sbin/acpiconf -s 3" +#define UP_BACKEND_HYBRID_SLEEP_COMMAND "/usr/sbin/acpiconf -s 4" #define UP_BACKEND_HIBERNATE_COMMAND "/usr/sbin/acpiconf -s 4" #ifdef BACKEND_TYPE_OPENBSD #define UP_BACKEND_SUSPEND_COMMAND "/usr/sbin/zzz" +#define UP_BACKEND_HYBRID_SLEEP_COMMAND "/usr/sbin/ZZZ" #define UP_BACKEND_HIBERNATE_COMMAND "/usr/sbin/ZZZ" It looks like there is no hybrid sleep method for FreeBSD, https://www.dragonflybsd.org/cgi/web-man?command=acpiconf and I did not find any such method for OpenBSD. I was assuming that it was possible and that someone from freeBSD and OpenBSD would tell me how to do those things so I used the same commands as Hibernate but it appears that there is no such support at all right now. Do note that I have not actually tested the patch on BSD or BSD at all, ever. I should probably try a BSD sometime.
Finally found some time to check the code ... besides the BSD case, it looks very good to me ! In xfpm_suspend_can_hybrid_sleep (xfpm-suspend.c:161) : >#ifdef BACKEND_TYPE_FREEBSD > return freebsd_supports_sleep_state ("S3"); >#endif >#ifdef BACKEND_TYPE_OPENBSD > return TRUE; >#endif So it looks like there can be some kind of hybrid_sleep support on FREEBSD ... or is S3 just "hibernate" ? If BSD's dont support hybrid_sleep, I would expect to get FALSE from this method when using a BSD. (As you said above .. probably better to dont provide the possibility to pick hybrid-sleep at all on BSDs ) In xfpm-systemd.c:78, copypasted the string from hibernate: "org.freedesktop.login1.hibernate". Forgot to replace, with hybrid-sleep, or was it intentional ? If we would not show "hybrid-sleep" as an enum value on BSD's, it would be possible to test the BSD usecase without even running a BSD ... just directly return FALSE on "xfpm_suspend_can_hybrid_sleep", and you theoretically should see the same behavior like on a BSD. Some very minor thing: When apllying the patch, git found a trailing whitespace .git/rebase-apply/patch:371: trailing whitespace. I asked gaston in IRC if he can take a look regarding BSDs .. possibly he knows details.
The reason I went with "org.freedesktop.login1.hibernate" in xfpm-systemd.c:78 is really stupid yet I did so for a (stupid) reason: There is no polkit hybrid sleep method, none is listed on https://www.freedesktop.org/wiki/Software/systemd/logind/ and there is none listed in /usr/share/polkit-1/actions/org.freedesktop.login1.policy What we _do_ have is a systemd-logind method CanHybridSleep Simon Steinbeiss fixed the very same problem in xfce4-session by switching from polkit to systemd-logind, https://git.xfce.org/xfce/xfce4-session/commit/xfce4-session/xfsm-systemd.c?id=e10ac91be3301c92900168855b265101ba9884e2 The same switch from polkit to systemd-logind should ideally/eventually be done in xfpm-systemd.c too. However, that is not what I tried to accomplish with this patch and my thinking was that it would be simplest to leave it using polkit and add the Hybrid Sleep functionality - and perhaps do a separate patch for polkit -> systemd-logind later. I am not very familiar with contributing to free software projects so I do not know if that is the right way of doing things, I just assumed that a patch touching all kinds of code which is unrelated to hybrid sleep would be a confuse to look at.
i dunno for freebsd but there's definitely no hybrid sleep for now on openbsd, i'll try to have a look at the patch in the coming days (but dont wait for me..)
(In reply to Öyvind Saether from comment #19) > The reason I went with "org.freedesktop.login1.hibernate" in > xfpm-systemd.c:78 is really stupid yet I did so for a (stupid) reason: > > There is no polkit hybrid sleep method, none is listed on > https://www.freedesktop.org/wiki/Software/systemd/logind/ and there is none > listed in /usr/share/polkit-1/actions/org.freedesktop.login1.policy > > What we _do_ have is a systemd-logind method CanHybridSleep > > Simon Steinbeiss fixed the very same problem in xfce4-session by switching > from polkit to systemd-logind, > https://git.xfce.org/xfce/xfce4-session/commit/xfce4-session/xfsm-systemd. > c?id=e10ac91be3301c92900168855b265101ba9884e2 > > The same switch from polkit to systemd-logind should ideally/eventually be > done in xfpm-systemd.c too. However, that is not what I tried to accomplish > with this patch and my thinking was that it would be simplest to leave it > using polkit and add the Hybrid Sleep functionality - and perhaps do a > separate patch for polkit -> systemd-logind later. I am not very familiar > with contributing to free software projects so I do not know if that is the > right way of doing things, I just assumed that a patch touching all kinds of > code which is unrelated to hybrid sleep would be a confuse to look at. Ok, fine for me ... could you please put a comment on that line of code, explaining that in a few words ? (Not sure if we can purge polkit in xfce4-power-manager, iirc, the BSD's dont support systemd-logind .. or do they ? Anyhow, you are right, thats a different issue)
Created attachment 9414 Add HybridSleep support. Has comment about PolicyKit workaround due to lack of method to check for hybridsleep permission.
(In reply to Öyvind Saether from comment #22) > Created attachment 9414 > Add HybridSleep support. > > Has comment about PolicyKit workaround due to lack of method to check for > hybridsleep permission. Thanks, that will help a bit for later contributors. What still is missing, is the return value of "xfpm_suspend_can_hybrid_sleep" for BSDs, like proposed in c#18 - The method should just return FALSE for both BSDs, since it is not supported - "hybrid sleep" should not be selectable on BSDs in the GUI I tried to "mock" a BSD by just returning FALSE directly, but the enum value is still shown.
Created attachment 9419 v2 turns out I had the order wrong in xfpm-enum-glib.h so on lib event hybrid sleep would hibernate and hibernate would hybrid sleep, very embarrassing. does not fix bsd yet, will fix later
Created attachment 9420 V2.1 Add Hybrid Sleep support. Works on Linux. Returns FALSE on BSD
(In reply to Öyvind Saether from comment #19) > The reason I went with "org.freedesktop.login1.hibernate" in > xfpm-systemd.c:78 is really stupid yet I did so for a (stupid) reason: > > There is no polkit hybrid sleep method, none is listed on > https://www.freedesktop.org/wiki/Software/systemd/logind/ and there is none > listed in /usr/share/polkit-1/actions/org.freedesktop.login1.policy > > What we _do_ have is a systemd-logind method CanHybridSleep > > Simon Steinbeiss fixed the very same problem in xfce4-session by switching > from polkit to systemd-logind, > https://git.xfce.org/xfce/xfce4-session/commit/xfce4-session/xfsm-systemd. > c?id=e10ac91be3301c92900168855b265101ba9884e2 > > The same switch from polkit to systemd-logind should ideally/eventually be > done in xfpm-systemd.c too. However, that is not what I tried to accomplish > with this patch and my thinking was that it would be simplest to leave it > using polkit and add the Hybrid Sleep functionality - and perhaps do a > separate patch for polkit -> systemd-logind later. I am not very familiar > with contributing to free software projects so I do not know if that is the > right way of doing things, I just assumed that a patch touching all kinds of > code which is unrelated to hybrid sleep would be a confuse to look at. Current code is buggy. logind Can* methods will return false (na) if user is not allowed to preform the action *or* the action is not supported! Hence please use logind Can* methods. logind does polkit checks internally. See: https://bugzilla.xfce.org/show_bug.cgi?id=13699 https://bugzilla.xfce.org/show_bug.cgi?id=14707 https://bugzilla.xfce.org/show_bug.cgi?id=15331
(In reply to Öyvind Saether from comment #25) > Created attachment 9420 > V2.1 Add Hybrid Sleep support. Works on Linux. Returns FALSE on BSD Thanks for the fix & sorry for the long delay ! Just re-tested your patch. What I still dont understand: Even if I hard-code to return FALSE on "xfpm_suspend_can_hybrid_sleep" and "xfpm_systemd_get_property" for hybrid sleep, why do I still have the possibility to select "hybrid sleep" in the drop-down on lid-close in the xfce4-power-manager-settings ? Not sure if I do something wrong in pretending that my system cannot hybrid sleep, or if there still is a problem in the code. Does that specific part work for you ? How do you test it ?
Created attachment 9732 hybrid sleep patch with small fix for the consolekit2 case alexxcons, the reason setting return FALSE on "xfpm_suspend_can_hybrid_sleep" makes no difference on your system is likely because src/xfpm-power.c checks if (LOGIND_RUNNING()) and POLKIT_AUTH_HIBERNATE_LOGIND defined in common/xfpm-power-common.h is called in that case because there is no org.freedesktop.login1.hybridsleep defined in https://www.freedesktop.org/wiki/Software/systemd/logind/ If the system doesn't have logind then consolekit2 is used and the "xfpm internal suspend backend" is only used if there is no logind or consolekit2 present (which is probably the case on *BSD). As Marcos Mello points out, larger parts of xfce4-power-manager should be re-written so it uses systemd-logind methods since this patch allows hybrid sleep if hibernate is allowed when logind is used since polkit doesn't have and never will have any hybrid sleep check.
Created attachment 9738 ops
Created attachment 9739 Hybrid sleep patch, works on linux laptop. added +#define POLKIT_AUTH_HYBRIDSLEEP_LOGIND "org.freedesktop.login1.hibernate" even though hibernate isn't the right check since polkit doesn't have one.
> +/* Policykit does not support any method for hybrid sleep > + * A require to use systemd-logind and CanHybridSleep would be required > + * to properly test for permission. Mention the polkit tests are incomplete, because they do not test if the actions are *supported* by the system (user may have permission, but system does not have ACPI for example). It is not just CanHybridSleep. All other checks are wrong too and should be replaced by logind's CanXXX methods, which take care of authentication checks internally.
-- GitLab Migration Automatic Message -- This bug has been migrated to xfce.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.xfce.org/xfce/xfce4-power-manager/-/issues/7. Please create an account or use an existing account on one of our supported OAuth providers. If you want to fork to submit patches and merge requests please continue reading here: https://docs.xfce.org/contribute/dev/git/start#gitlab_forks_and_merge_requests Also feel free to reach out to us on the mailing list https://mail.xfce.org/mailman/listinfo/xfce4-dev