Add `--idle-timeout-when-locked` for idle timeout when session is locked #3546

hbatagelo · 2024-08-12T19:28:03Z

This PR adds a new configuration option, --idle-timeout-when-locked, that allows the compositor to apply a separate idle timeout when the session is locked. The default value is 0, meaning the display will stay on indefinitely when the session is locked.

Examples showing its interaction with the existing --idle-timeout option:

--idle-timeout=60 --idle-timeout-when-locked=10: The display turns off after 60s of idle time when the session is unlocked, and after 10s of idle time when the session is locked.
--idle-timeout=0 --idle-timeout-when-locked=10: The display never turns off when idle, but it turns off after 10s of idle time when the session is locked.
--idle-timeout=60 --idle-timeout-when-locked=0: The display turns off after 60s of idle time but never when the session is locked.
--idle-timeout=0 --idle-timeout-on-lock=0: This is the default behavior. The display never turns off, wether the session is locked or unlocked.

Implementation details:

BasicIdleHandler now registers a session lock listener via the SessionLockListener class, which takes on_lock and on_unlock callbacks to track the state of the session lock. When the idle handlers (the dimmer and the power setter) are registered, they will use the timeout value from --idle-timeout-when-locked when the session is locked, and from --idle-timeout when the session is not locked.

Saviq

I think this needs to be a little more flexible. Particularly, we want to give time for any lockscreen to render, so that on resume it's already there.

Not sure if we need it to be configurable, maybe just hardcoding a second-long delay would be appropriate…?

@AlanGriffiths @mattkae thoughts?

AlanGriffiths · 2024-08-13T08:39:53Z

src/server/shell/basic_idle_handler.cpp

@@ -215,6 +267,16 @@ void msh::BasicIdleHandler::set_display_off_timeout(std::optional<time::Duration
            display_config_controller, mir_power_mode_off, multiplexer);
        observers.push_back(power_setter);
        idle_hub->register_interest(power_setter, off_timeout);
+        if (off_timeout == time::Duration{} && previous_off_timeout)


Maybe it is "just me", but I find that off_timeout == time::Duration{0} better expresses a comparison to zero. (I know comparing to "default constructed" has the same meaning to the compiler but it takes more thought to read)

AlanGriffiths · 2024-08-13T08:50:55Z

I think this needs to be a little more flexible. Particularly, we want to give time for any lockscreen to render, so that on resume it's already there.

Not sure if we need it to be configurable, maybe just hardcoding a second-long delay would be appropriate…?

@AlanGriffiths @mattkae thoughts?

I don't think turning the screen off will actually prevent the lockscreen rendering. Is this actually down to our "smooth boot" changes? I.e. showing the preexisting display buffer until we have something to composite. If so, we could also avoid flashing the old content by just blanking the display before poweroff.

RAOF · 2024-08-13T11:04:48Z

I think this needs to be a little more flexible. Particularly, we want to give time for any lockscreen to render, so that on resume it's already there.
Not sure if we need it to be configurable, maybe just hardcoding a second-long delay would be appropriate…?
@AlanGriffiths @mattkae thoughts?

I don't think turning the screen off will actually prevent the lockscreen rendering.

I agree. I think it might be a little bit less jarring if we let the lockscreen render¹ between locking and turning the display off, though? For reference, GNOME lets the lockscreen animate in before turning the display off.

¹: Probably just a 5sec delay; while we could, I don't think we need to actually check that the lockscreen has submitted buffers for each output.

mattkae · 2024-08-13T19:36:42Z

I am 👍 for a timer that will idle the screen X seconds after a lock. That way, the lock client has some time to respond before the screen goes dim.

I also think that the X seconds here should be configurable by the compositor implementer. Different authors may want this behavior to happen on a different timing (or not at all)

RAOF · 2024-08-22T02:13:01Z

When talking with Harlen yesterday talking about the appropriate behaviour here, we had something of an idea:
We don't particularly want to change the screen blanking time when locking the session¹. What we want is for the screen to turn off shortly after the session is locked. The suggestion is that, when the SessionLockObserver notifies us of a session lock we arm a 5-second Alarm that will turn off the display, and register a 0-timeout idle handler with the IdleHub whose active() handler will cancel that Alarm.

This might be worth supporting directly in IdleHub, as a one-shot run_if_inactive_for(timeout, std::function<> functor) that fires functor if there is no activity event before timeout.

¹: In the degenerate case where we want the screen to turn off immediately on session lock, we very much don't want the screen off timer to continue to fire immediately, because that will ensure the screen turns off even while the user is trying to unlock their session!

hbatagelo · 2024-09-23T16:01:58Z

I've updated the title and top comment to reflect the latest changes.

While the new behavior aligns with Chris's description, I kept the logic in BasicIdleHandler. Directly using IdleHub could achieve the same results but would require changing its interface. Also, keeping the logic in BasicIdleHandler makes it easier to reuse the existing dimmer and power-setter observers for both --idle-timeout and --idle-timeout-on-lock: the display will dim if either timeout exceeds 20s (a pre-existing hardcoded value).

AlanGriffiths

Some quick things to fix. I'll look at the functionality tomorrow

AlanGriffiths · 2024-09-23T16:48:48Z

src/platform/symbols.map

-MIR_PLATFORM_2.18 {
+MIR_PLATFORM_2.19 {
 global:


I don't think we're breaking the mirplatform ABI: idle_timeout_on_lock_opt should be in a new 2.19 stanza

AlanGriffiths · 2024-09-23T16:50:30Z

src/include/server/mir/shell/idle_handler.h

+    /// set_display_off_timeout while the session is locked and remains idle. If Mir becomes active after the session
+    /// is locked, this timeout is disabled. If the session is already locked when this function is called, the new
+    /// timeout will only take effect after the next session lock.
+    virtual void set_display_off_timeout_on_lock(std::optional<time::Duration> timeout) = 0;


[Aside] This is breaking the mirserver ABI. We need to bump accordingly (either now or before 2.19)

Don't worry about it here - there's other stuff breaking things in the pipeline

Oops, I didn't notice it before because the symbols checking tool doesn't detect changes in the vtable.
If it's just a matter of bumping MIR_VERSION_MINOR and MIRSERVER_ABI, we can do it now. Is it enough to follow the how-to?

AlanGriffiths · 2024-09-23T16:51:46Z

src/server/shell/basic_idle_handler.cpp

+    explicit SessionLockListener(std::function<void()> const& on_lock) : on_lock_{on_lock} {};
+
+private:
+    void on_lock() override { on_lock_(); };
+    void on_unlock() override {};


Suggested change

explicit SessionLockListener(std::function<void()> const& on_lock) : on_lock_{on_lock} {};

private:

void on_lock() override { on_lock_(); };

void on_unlock() override {};

explicit SessionLockListener(std::function<void()> const& on_lock) : on_lock_{on_lock} {}

private:

void on_lock() override { on_lock_(); }

void on_unlock() override {}

AlanGriffiths · 2024-09-25T09:12:58Z

--idle-timeout=60 --idle-timeout-on-lock=10: The display turns off after 60s of idle time, but when the session is locked it turns off after 10s of being idle. If Mir becomes active before 10s, the idle timeout reverts to 60s even if the session stays locked.

Have I misunderstood this?

In a VT:

snap refresh miriway --channel-edge/mir-pr3546
/snap/miriway/current/example-configs/alan_g.bash
miriway --idle-timeout-on-lock=10
Use for a bit and then lock the screen with Ctrl-Alt-L

Expect: after 10s the screen dims and turns off
Actual: the screen remains active

In case it matters...

$ grep idle ~/.config/miriway-shell.config
idle-timeout=600

hbatagelo · 2024-09-30T12:04:55Z

I suspect that, by the time you release the modifier keys, you activate the session and restore the 600s timeout. We'll probably need something a bit more elaborate than the 0-timeout idle handler to prevent this immediate reactivation. I'll work on that.

… tests

hbatagelo · 2024-09-30T15:44:57Z

I changed the 0-timeout idle handler to a 200ms timeout. This should serve as a grace period to prevent immediate reactivation when the session is locked using a keyboard shortcut.

AlanGriffiths · 2024-10-01T11:08:04Z

I changed the 0-timeout idle handler to a 200ms timeout. This should serve as a grace period to prevent immediate reactivation when the session is locked using a keyboard shortcut.

This is better but doesn't work very well. It too easy to "linger" on a shift key when locking the screen.

Also consider the case when the screen is woken by activity but not unlocked.

In both cases, it would be better to recognize that the screen is still locked and just reset the timeout to idle-timeout-on-lock

hbatagelo · 2024-10-01T11:27:12Z

In both cases, it would be better to recognize that the screen is still locked and just reset the timeout to idle-timeout-on-lock

Agreed. That appears to be the easiest solution as well, since it doesn't require restoring to a timeout that changes the behavior. The other option would be to ignore key releases after the session is locked, but that would be more involved.

AlanGriffiths · 2024-10-01T11:38:58Z

Agreed

In which case, maybe s/idle-timeout-on-lock/idle-timeout-when-locked/g

AlanGriffiths · 2024-10-16T17:05:10Z

@hbatagelo ping!

AlanGriffiths

There's a pre-existing nit about validating on use, not on input, but it is protected by validating on input first

AlanGriffiths · 2024-10-17T14:15:42Z

src/server/shell/basic_idle_handler.cpp

        {
-            fatal_error("BasicIdleHandler given invalid timeout %d, should be >0", off_timeout.count());
+            fatal_error("BasicIdleHandler given invalid timeout %d, should be >0", off_timeout->count());
        }


I think this is the wrong time to be validating input and erroring. (That should be on startup when the input is parsed)

Goes idle just after the session is locked

26b6520

hbatagelo requested a review from a team as a code owner August 12, 2024 19:28

Saviq reviewed Aug 13, 2024

View reviewed changes

AlanGriffiths reviewed Aug 13, 2024

View reviewed changes

Add config option and tests

8ff6274

hbatagelo changed the title ~~Goes idle just after the session is locked~~ Add --idle-timeout-on-lock for idle timeout when session is locked Sep 23, 2024

hbatagelo changed the title ~~Add --idle-timeout-on-lock for idle timeout when session is locked~~ Add --idle-timeout-on-lock for idle timeout when session is locked Sep 23, 2024

hbatagelo changed the title ~~Add --idle-timeout-on-lock for idle timeout when session is locked~~ Add --idle-timeout-on-lock for idle timeout when session is locked Sep 23, 2024

AlanGriffiths requested changes Sep 23, 2024

View reviewed changes

Add new stanza for platform ABI + remove extra semicolons

a5b087c

Prevent immediate activation after the session is locked; add related…

0863da2

… tests

hbatagelo force-pushed the MIRENG-454/idle-timeout-on-lock branch from e989b38 to 705f236 Compare October 17, 2024 12:09

hbatagelo enabled auto-merge October 17, 2024 12:26

Implement idle-timeout-when-locked

004fcd3

hbatagelo force-pushed the MIRENG-454/idle-timeout-on-lock branch from 705f236 to 004fcd3 Compare October 17, 2024 12:31

hbatagelo changed the title ~~Add --idle-timeout-on-lock for idle timeout when session is locked~~ Add --idle-timeout-when-locked for idle timeout when session is locked Oct 17, 2024

tarek-y-ismail requested a review from AlanGriffiths October 17, 2024 14:37

AlanGriffiths approved these changes Oct 17, 2024

View reviewed changes

hbatagelo added this pull request to the merge queue Oct 17, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 17, 2024

hbatagelo added this pull request to the merge queue Oct 17, 2024

Merged via the queue into main with commit f66bcae Oct 17, 2024
30 of 38 checks passed

hbatagelo deleted the MIRENG-454/idle-timeout-on-lock branch October 17, 2024 18:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `--idle-timeout-when-locked` for idle timeout when session is locked #3546

Add `--idle-timeout-when-locked` for idle timeout when session is locked #3546

hbatagelo commented Aug 12, 2024 •

edited

Loading

Saviq left a comment

AlanGriffiths Aug 13, 2024

AlanGriffiths commented Aug 13, 2024 •

edited

Loading

RAOF commented Aug 13, 2024

mattkae commented Aug 13, 2024

RAOF commented Aug 22, 2024

hbatagelo commented Sep 23, 2024

AlanGriffiths left a comment

AlanGriffiths Sep 23, 2024

AlanGriffiths Sep 23, 2024

hbatagelo Sep 23, 2024

AlanGriffiths Sep 23, 2024

AlanGriffiths commented Sep 25, 2024

hbatagelo commented Sep 30, 2024

hbatagelo commented Sep 30, 2024

AlanGriffiths commented Oct 1, 2024

hbatagelo commented Oct 1, 2024

AlanGriffiths commented Oct 1, 2024

AlanGriffiths commented Oct 16, 2024

AlanGriffiths left a comment

AlanGriffiths Oct 17, 2024

Add --idle-timeout-when-locked for idle timeout when session is locked #3546

Add --idle-timeout-when-locked for idle timeout when session is locked #3546

Conversation

hbatagelo commented Aug 12, 2024 • edited Loading

Implementation details:

Saviq left a comment

Choose a reason for hiding this comment

AlanGriffiths Aug 13, 2024

Choose a reason for hiding this comment

AlanGriffiths commented Aug 13, 2024 • edited Loading

RAOF commented Aug 13, 2024

mattkae commented Aug 13, 2024

RAOF commented Aug 22, 2024

hbatagelo commented Sep 23, 2024

AlanGriffiths left a comment

Choose a reason for hiding this comment

AlanGriffiths Sep 23, 2024

Choose a reason for hiding this comment

AlanGriffiths Sep 23, 2024

Choose a reason for hiding this comment

hbatagelo Sep 23, 2024

Choose a reason for hiding this comment

AlanGriffiths Sep 23, 2024

Choose a reason for hiding this comment

AlanGriffiths commented Sep 25, 2024

hbatagelo commented Sep 30, 2024

hbatagelo commented Sep 30, 2024

AlanGriffiths commented Oct 1, 2024

hbatagelo commented Oct 1, 2024

AlanGriffiths commented Oct 1, 2024

AlanGriffiths commented Oct 16, 2024

AlanGriffiths left a comment

Choose a reason for hiding this comment

AlanGriffiths Oct 17, 2024

Choose a reason for hiding this comment

Add `--idle-timeout-when-locked` for idle timeout when session is locked #3546

Add `--idle-timeout-when-locked` for idle timeout when session is locked #3546

hbatagelo commented Aug 12, 2024 •

edited

Loading

AlanGriffiths commented Aug 13, 2024 •

edited

Loading