Implement idle-timeout-when-locked

include/platform/mir/options/configuration.h

@@ -46,7 +46,7 @@ extern char const* const wayland_extensions_opt;
 extern char const* const add_wayland_extensions_opt;
 extern char const* const drop_wayland_extensions_opt;
 extern char const* const idle_timeout_opt;
-extern char const* const idle_timeout_on_lock_opt;
+extern char const* const idle_timeout_when_locked_opt;
 
 extern char const* const enable_key_repeat_opt;
 

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

@@ -52,12 +52,10 @@ class IdleHandler : public ObserverRegistrar<IdleHandlerObserver>
     /// is sent the display is never turned off or dimmed, which is the default.
     virtual void set_display_off_timeout(std::optional<time::Duration> timeout) = 0;
 
-    /// Duration Mir will remain idle before the display is turned off after the session is locked. The display may dim
-    /// some time before this. If not nullopt, this idle timeout takes precedence over the timeout set with
-    /// 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;
+    /// Duration Mir will remain idle before the display is turned off when the session is locked. The display may dim
+    /// some time before this. If nullopt is sent, the display is never turned off or dimmed during session lock, which
+    /// is the default.
+    virtual void set_display_off_timeout_when_locked(std::optional<time::Duration> timeout) = 0;
 
 private:
     IdleHandler(IdleHandler const&) = delete;

src/platform/options/default_configuration.cpp

@@ -44,7 +44,7 @@ char const* const mo::wayland_extensions_opt      = "wayland-extensions";
 char const* const mo::add_wayland_extensions_opt  = "add-wayland-extensions";
 char const* const mo::drop_wayland_extensions_opt = "drop-wayland-extensions";
 char const* const mo::idle_timeout_opt            = "idle-timeout";
-char const* const mo::idle_timeout_on_lock_opt    = "idle-timeout-on-lock";
+char const* const mo::idle_timeout_when_locked_opt = "idle-timeout-when-locked";
 
 char const* const mo::off_opt_value = "off";
 char const* const mo::log_opt_value = "log";
@@ -178,13 +178,11 @@ mo::DefaultConfiguration::DefaultConfiguration(
         (enable_key_repeat_opt, po::value<bool>()->default_value(true),
              "Enable server generated key repeat")
         (idle_timeout_opt, po::value<int>()->default_value(0),
-            "Time (in seconds) Mir will remain idle before turning off the display, "
-            "or 0 to keep display on forever.")
-        (idle_timeout_on_lock_opt, po::value<int>()->default_value(-1),
             "Time (in seconds) Mir will remain idle before turning off the display "
-            "after the session is locked. If Mir becomes active while the session is "
-            "locked, the timeout falls back to the one set with --idle-timeout. "
-            "Default: a negative value disables this timeout.")
+            "when the session is not locked, or 0 to keep display on forever.")
+        (idle_timeout_when_locked_opt, po::value<int>()->default_value(0),
+            "Time (in seconds) Mir will remain idle before turning off the display "
+            "when the session is locked, or 0 to keep the display on forever.")
         (fatal_except_opt, "On \"fatal error\" conditions [e.g. drivers behaving "
             "in unexpected ways] throw an exception (instead of a core dump)")
         (debug_opt, "Enable extra development debugging. "

src/platform/symbols.map

@@ -205,7 +205,7 @@ MIR_PLATFORM_2.18 {
 MIR_PLATFORM_2.19 {
  global:
   extern "C++" {
-    mir::options::idle_timeout_on_lock_opt;
+    mir::options::idle_timeout_when_locked_opt;
  };
  local: *;
 } MIR_PLATFORM_2.18;

src/server/shell/basic_idle_handler.cpp

@@ -24,7 +24,6 @@
 #include "mir/shell/display_configuration_controller.h"
 
 #include <mutex>
-#include <utility>
 
 namespace ms = mir::scene;
 namespace mg = mir::graphics;
@@ -35,7 +34,6 @@ namespace msh = mir::shell;
 
 namespace
 {
-auto const min_off_timeout_on_lock = std::chrono::milliseconds{200};
 auto const dim_time_before_off = std::chrono::seconds{10};
 unsigned char const black_pixel_data[4] = {0, 0, 0, 255};
 int const coverage_size = 100000;
@@ -173,37 +171,6 @@ struct PowerModeSetter : ms::IdleStateObserver
 };
 }
 
-class msh::BasicIdleHandler::TimeoutRestorer : public ms::IdleStateObserver
-{
-public:
-    explicit TimeoutRestorer(
-        msh::BasicIdleHandler* idle_handler)
-        : idle_handler{idle_handler}
-    {
-    }
-
-    void active() override {
-        // Since this observer is registered when Mir is already active, this callback is invoked immediately upon
-        // registration. To restore the off timeout, Mir must transition to idle before becoming active again.
-        if (was_idle && !restored)
-        {
-            restored = true;
-            idle_handler->restore_off_timeout();
-        }
-    }
-
-    void idle() override
-    {
-        was_idle = true;
-    }
-
-private:
-    msh::BasicIdleHandler* const idle_handler;
-
-    bool was_idle{false};
-    bool restored{false};
-};
-
 class msh::BasicIdleHandler::SessionLockListener : public ms::SessionLockObserver
 {
 public:
@@ -213,12 +180,12 @@ class msh::BasicIdleHandler::SessionLockListener : public ms::SessionLockObserve
 
     void on_lock() override
     {
-        idle_handler->on_lock();
+        idle_handler->on_session_lock();
     }
 
     void on_unlock() override
     {
-        idle_handler->on_unlock();
+        idle_handler->on_session_unlock();
     }
 
 private:
@@ -245,10 +212,7 @@ msh::BasicIdleHandler::~BasicIdleHandler()
 {
     session_lock->unregister_interest(*session_lock_monitor);
     std::lock_guard lock{mutex};
-    for (auto const& observer : observers)
-    {
-        idle_hub->unregister_interest(*observer);
-    }
+    clear_observers(lock);
 }
 
 void msh::BasicIdleHandler::set_display_off_timeout(std::optional<time::Duration> timeout)
@@ -257,66 +221,69 @@ void msh::BasicIdleHandler::set_display_off_timeout(std::optional<time::Duration
     if (timeout != current_off_timeout)
     {
         current_off_timeout = timeout;
-        clear_observers(lock);
-        register_observers(lock);
+        if (!session_locked)
+        {
+            clear_observers(lock);
+            register_observers(lock);
+        }
     }
 }
 
-void msh::BasicIdleHandler::set_display_off_timeout_on_lock(std::optional<time::Duration> timeout)
+void msh::BasicIdleHandler::set_display_off_timeout_when_locked(std::optional<time::Duration> timeout)
 {
     std::lock_guard lock{mutex};
-    if (timeout)
+    if (timeout != current_off_timeout_when_locked)
     {
-        timeout = timeout < min_off_timeout_on_lock ? min_off_timeout_on_lock : timeout;
+        current_off_timeout_when_locked = timeout;
+        if (session_locked)
+        {
+            clear_observers(lock);
+            register_observers(lock);
+        }
     }
-    current_off_timeout_on_lock = timeout;
 }
 
-void  msh::BasicIdleHandler::on_lock()
+void  msh::BasicIdleHandler::on_session_lock()
 {
     std::lock_guard lock{mutex};
-    if (auto const timeout_on_lock{current_off_timeout_on_lock})
+    session_locked = true;
+    if (current_off_timeout_when_locked != current_off_timeout)
     {
-        if (timeout_on_lock != current_off_timeout)
-        {
-            previous_off_timeout = std::exchange(current_off_timeout, timeout_on_lock);
-            clear_observers(lock);
-            register_observers(lock);
-
-            // Register an observer to restore the off timeout when Mir transitions from idle to active
-            // while the session is locked
-            timeout_restorer = std::make_shared<TimeoutRestorer>(this);
-            observers.push_back(timeout_restorer);
-            idle_hub->register_interest(timeout_restorer, min_off_timeout_on_lock);
-        }
+        clear_observers(lock);
+        register_observers(lock);
     }
-
 }
-void  msh::BasicIdleHandler::on_unlock()
+
+void  msh::BasicIdleHandler::on_session_unlock()
 {
-    restore_off_timeout();
+    std::lock_guard lock{mutex};
+    session_locked = false;
+    if (current_off_timeout_when_locked != current_off_timeout)
+    {
+        clear_observers(lock);
+        register_observers(lock);
+    }
 }
 
 void msh::BasicIdleHandler::register_observers(ProofOfMutexLock const&)
 {
-    if (current_off_timeout)
+    if (auto const off_timeout{session_locked ? current_off_timeout_when_locked : current_off_timeout})
     {
-        auto const off_timeout = current_off_timeout.value();
-        if (off_timeout < time::Duration{0})
+        if (*off_timeout <= time::Duration{0})
         {
-            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());
         }
-        if (off_timeout >= dim_time_before_off * 2)
+        if (*off_timeout >= dim_time_before_off * 2)
         {
-            auto const dim_timeout = off_timeout - dim_time_before_off;
+            auto const dim_timeout = *off_timeout - dim_time_before_off;
             auto const dimmer = std::make_shared<Dimmer>(input_scene, allocator, multiplexer);
             observers.push_back(dimmer);
             idle_hub->register_interest(dimmer, dim_timeout);
         }
         auto const power_setter = std::make_shared<PowerModeSetter>(
             display_config_controller, mir_power_mode_off, multiplexer);
         observers.push_back(power_setter);
-        idle_hub->register_interest(power_setter, off_timeout);
+        idle_hub->register_interest(power_setter, *off_timeout);
     }
 }
 
@@ -331,21 +298,6 @@ void msh::BasicIdleHandler::clear_observers(ProofOfMutexLock const&)
     observers.clear();
 }
 
-void msh::BasicIdleHandler::restore_off_timeout()
-{
-    std::lock_guard lock{mutex};
-    if (std::ranges::find(observers, timeout_restorer) != observers.end())
-    {
-        // Remove the timeout observer upfront to prevent it from being activated
-        idle_hub->unregister_interest(*timeout_restorer);
-        std::erase(observers, timeout_restorer);
-
-        current_off_timeout = previous_off_timeout;
-        clear_observers(lock);
-        register_observers(lock);
-    }
-}
-
 void msh::BasicIdleHandler::register_interest(std::weak_ptr<IdleHandlerObserver> const& observer)
 {
     multiplexer.register_interest(observer);

src/server/shell/basic_idle_handler.h

@@ -59,7 +59,7 @@ class BasicIdleHandler : public IdleHandler
 
     void set_display_off_timeout(std::optional<time::Duration> timeout) override;
 
-    void set_display_off_timeout_on_lock(std::optional<time::Duration> timeout) override;
+    void set_display_off_timeout_when_locked(std::optional<time::Duration> timeout) override;
 
     void register_interest(std::weak_ptr<IdleHandlerObserver> const&) override;
 
@@ -75,14 +75,12 @@ class BasicIdleHandler : public IdleHandler
 
 private:
     class SessionLockListener;
-    class TimeoutRestorer;
 
-    void on_lock();
-    void on_unlock();
+    void on_session_lock();
+    void on_session_unlock();
 
     void register_observers(ProofOfMutexLock const&);
     void clear_observers(ProofOfMutexLock const&);
-    void restore_off_timeout();
 
     std::shared_ptr<scene::IdleHub> const idle_hub;
     std::shared_ptr<input::Scene> const input_scene;
@@ -93,10 +91,9 @@ class BasicIdleHandler : public IdleHandler
 
     std::mutex mutex;
     std::optional<time::Duration> current_off_timeout;
-    std::optional<time::Duration> previous_off_timeout;
-    std::optional<time::Duration> current_off_timeout_on_lock;
+    std::optional<time::Duration> current_off_timeout_when_locked;
+    bool session_locked{false};
     std::vector<std::shared_ptr<scene::IdleStateObserver>> observers;
-    std::shared_ptr<TimeoutRestorer> timeout_restorer;
 
     class BasicIdleHandlerObserverMultiplexer: public ObserverMultiplexer<IdleHandlerObserver>
     {

src/server/shell/default_configuration.cpp

@@ -123,14 +123,23 @@ auto mir::DefaultServerConfiguration::the_idle_handler() -> std::shared_ptr<msh:
                 idle_handler->set_display_off_timeout(std::chrono::seconds{idle_timeout_seconds});
             }
 
-            int const idle_timeout_on_lock = options->get<int>(options::idle_timeout_on_lock_opt);
-            if (idle_timeout_on_lock < 0)
+            int const idle_timeout_when_locked = options->get<int>(options::idle_timeout_when_locked_opt);
+            if (idle_timeout_when_locked < 0)
             {
-                idle_handler->set_display_off_timeout_on_lock(std::nullopt);
+                throw mir::AbnormalExit(
+                    "Invalid " +
+                    std::string{options::idle_timeout_when_locked_opt} +
+                    " value " +
+                    std::to_string(idle_timeout_when_locked) +
+                    ", must be > 0");
+            }
+            if (idle_timeout_when_locked == 0)
+            {
+                idle_handler->set_display_off_timeout_when_locked(std::nullopt);
             }
             else
             {
-                idle_handler->set_display_off_timeout_on_lock(std::chrono::seconds{idle_timeout_on_lock});
+                idle_handler->set_display_off_timeout_when_locked(std::chrono::seconds{idle_timeout_when_locked});
             }
 
             return idle_handler;