From 004fcd377ffd8a7c84904348f3c0e87d46a4d2a7 Mon Sep 17 00:00:00 2001 From: Harlen Batagelo Date: Thu, 17 Oct 2024 09:03:25 -0300 Subject: [PATCH] Implement idle-timeout-when-locked --- include/platform/mir/options/configuration.h | 2 +- src/include/server/mir/shell/idle_handler.h | 10 +- .../options/default_configuration.cpp | 12 +- src/platform/symbols.map | 2 +- src/server/shell/basic_idle_handler.cpp | 120 ++++++------------ src/server/shell/basic_idle_handler.h | 13 +- src/server/shell/default_configuration.cpp | 17 ++- .../shell/test_basic_idle_handler.cpp | 79 ++++-------- 8 files changed, 92 insertions(+), 163 deletions(-) diff --git a/include/platform/mir/options/configuration.h b/include/platform/mir/options/configuration.h index a970bdd0ceb..4383db21c69 100644 --- a/include/platform/mir/options/configuration.h +++ b/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; diff --git a/src/include/server/mir/shell/idle_handler.h b/src/include/server/mir/shell/idle_handler.h index b615cc76136..cc49b18a88c 100644 --- a/src/include/server/mir/shell/idle_handler.h +++ b/src/include/server/mir/shell/idle_handler.h @@ -52,12 +52,10 @@ class IdleHandler : public ObserverRegistrar /// is sent the display is never turned off or dimmed, which is the default. virtual void set_display_off_timeout(std::optional 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 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 timeout) = 0; private: IdleHandler(IdleHandler const&) = delete; diff --git a/src/platform/options/default_configuration.cpp b/src/platform/options/default_configuration.cpp index 39ff1ebfc2d..3729ea727e1 100644 --- a/src/platform/options/default_configuration.cpp +++ b/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()->default_value(true), "Enable server generated key repeat") (idle_timeout_opt, po::value()->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()->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()->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. " diff --git a/src/platform/symbols.map b/src/platform/symbols.map index 9b436394ded..c99ae8b62fb 100644 --- a/src/platform/symbols.map +++ b/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; diff --git a/src/server/shell/basic_idle_handler.cpp b/src/server/shell/basic_idle_handler.cpp index 22c433410f6..ba96fb3c130 100644 --- a/src/server/shell/basic_idle_handler.cpp +++ b/src/server/shell/basic_idle_handler.cpp @@ -24,7 +24,6 @@ #include "mir/shell/display_configuration_controller.h" #include -#include 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 timeout) @@ -257,58 +221,61 @@ void msh::BasicIdleHandler::set_display_off_timeout(std::optional timeout) +void msh::BasicIdleHandler::set_display_off_timeout_when_locked(std::optional 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(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(input_scene, allocator, multiplexer); observers.push_back(dimmer); idle_hub->register_interest(dimmer, dim_timeout); @@ -316,7 +283,7 @@ void msh::BasicIdleHandler::register_observers(ProofOfMutexLock const&) auto const power_setter = std::make_shared( 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 const& observer) { multiplexer.register_interest(observer); diff --git a/src/server/shell/basic_idle_handler.h b/src/server/shell/basic_idle_handler.h index 8971e65b83c..e247557d9ce 100644 --- a/src/server/shell/basic_idle_handler.h +++ b/src/server/shell/basic_idle_handler.h @@ -59,7 +59,7 @@ class BasicIdleHandler : public IdleHandler void set_display_off_timeout(std::optional timeout) override; - void set_display_off_timeout_on_lock(std::optional timeout) override; + void set_display_off_timeout_when_locked(std::optional timeout) override; void register_interest(std::weak_ptr 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 const idle_hub; std::shared_ptr const input_scene; @@ -93,10 +91,9 @@ class BasicIdleHandler : public IdleHandler std::mutex mutex; std::optional current_off_timeout; - std::optional previous_off_timeout; - std::optional current_off_timeout_on_lock; + std::optional current_off_timeout_when_locked; + bool session_locked{false}; std::vector> observers; - std::shared_ptr timeout_restorer; class BasicIdleHandlerObserverMultiplexer: public ObserverMultiplexer { diff --git a/src/server/shell/default_configuration.cpp b/src/server/shell/default_configuration.cpp index 43b1b62a589..34285311286 100644 --- a/src/server/shell/default_configuration.cpp +++ b/src/server/shell/default_configuration.cpp @@ -123,14 +123,23 @@ auto mir::DefaultServerConfiguration::the_idle_handler() -> std::shared_ptrset_display_off_timeout(std::chrono::seconds{idle_timeout_seconds}); } - int const idle_timeout_on_lock = options->get(options::idle_timeout_on_lock_opt); - if (idle_timeout_on_lock < 0) + int const idle_timeout_when_locked = options->get(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; diff --git a/tests/unit-tests/shell/test_basic_idle_handler.cpp b/tests/unit-tests/shell/test_basic_idle_handler.cpp index 268600c05e5..8464dcb873b 100644 --- a/tests/unit-tests/shell/test_basic_idle_handler.cpp +++ b/tests/unit-tests/shell/test_basic_idle_handler.cpp @@ -141,124 +141,99 @@ TEST_F(BasicIdleHandler, does_not_register_observers_by_default) mt::fake_shared(session_lock)}; } -TEST_F(BasicIdleHandler, off_timeout_on_lock_is_used_when_session_is_locked) +TEST_F(BasicIdleHandler, off_timeout_when_locked_is_used_when_session_is_locked) { + handler.set_display_off_timeout(30s); + handler.set_display_off_timeout_when_locked(10s); + EXPECT_CALL(idle_hub, register_interest(_, Eq(30s))) - .Times(1); + .Times(0); EXPECT_CALL(idle_hub, register_interest(_, Eq(10s))) .Times(1); - handler.set_display_off_timeout(30s); - handler.set_display_off_timeout_on_lock(10s); session_lock.lock(); } -TEST_F(BasicIdleHandler, off_timeout_on_lock_is_not_used_when_session_is_not_locked) +TEST_F(BasicIdleHandler, off_timeout_when_locked_is_not_used_when_session_is_not_locked) { EXPECT_CALL(idle_hub, register_interest(_, Eq(30s))) .Times(1); EXPECT_CALL(idle_hub, register_interest(_, Eq(10s))) .Times(0); handler.set_display_off_timeout(30s); - handler.set_display_off_timeout_on_lock(10s); + handler.set_display_off_timeout_when_locked(10s); } -TEST_F(BasicIdleHandler, off_timeout_is_restored_when_locked_session_becomes_active) +TEST_F(BasicIdleHandler, off_timeout_is_restored_when_session_is_unlocked) { - EXPECT_CALL(idle_hub, register_interest(_, Eq(30s))) - .Times(2); - EXPECT_CALL(idle_hub, register_interest(_, Eq(10s))) - .Times(1); handler.set_display_off_timeout(30s); - handler.set_display_off_timeout_on_lock(10s); + handler.set_display_off_timeout_when_locked(10s); session_lock.lock(); - auto const observer = observer_for(200ms); - observer->idle(); - observer->active(); -} - -TEST_F(BasicIdleHandler, off_timeout_is_restored_when_session_is_unlocked) -{ EXPECT_CALL(idle_hub, register_interest(_, Eq(30s))) - .Times(2); - EXPECT_CALL(idle_hub, register_interest(_, Eq(10s))) .Times(1); - handler.set_display_off_timeout(30s); - handler.set_display_off_timeout_on_lock(10s); - session_lock.lock(); session_lock.unlock(); } -TEST_F(BasicIdleHandler, off_timeout_set_on_session_lock_is_used_when_session_becomes_active) +TEST_F(BasicIdleHandler, off_timeout_set_on_session_lock_is_not_used_when_session_is_locked) { - EXPECT_CALL(idle_hub, register_interest(_, Eq(30s))) - .Times(1); + handler.set_display_off_timeout_when_locked(10s); + EXPECT_CALL(idle_hub, register_interest(_, Eq(10s))) .Times(1); EXPECT_CALL(idle_hub, register_interest(_, Eq(5s))) - .Times(1); - handler.set_display_off_timeout(30s); - handler.set_display_off_timeout_on_lock(10s); + .Times(0); session_lock.lock(); handler.set_display_off_timeout(5s); - - auto const observer = observer_for(200ms); - observer->idle(); - observer->active(); } TEST_F(BasicIdleHandler, off_timeout_set_on_session_lock_is_used_when_session_unlocks) { - EXPECT_CALL(idle_hub, register_interest(_, Eq(30s))) - .Times(1); + handler.set_display_off_timeout_when_locked(10s); + EXPECT_CALL(idle_hub, register_interest(_, Eq(10s))) .Times(1); EXPECT_CALL(idle_hub, register_interest(_, Eq(5s))) .Times(1); - handler.set_display_off_timeout(30s); - handler.set_display_off_timeout_on_lock(10s); session_lock.lock(); handler.set_display_off_timeout(5s); session_lock.unlock(); } -TEST_F(BasicIdleHandler, off_timeout_on_lock_set_on_session_lock_is_used_only_in_the_next_lock) +TEST_F(BasicIdleHandler, off_timeout_when_locked_set_on_session_lock_is_used_when_session_is_locked) { - EXPECT_CALL(idle_hub, register_interest(_, Eq(30s))) - .Times(2); + handler.set_display_off_timeout_when_locked(10s); + EXPECT_CALL(idle_hub, register_interest(_, Eq(10s))) .Times(1); EXPECT_CALL(idle_hub, register_interest(_, Eq(5s))) .Times(1); - handler.set_display_off_timeout(30s); - handler.set_display_off_timeout_on_lock(10s); - session_lock.lock(); - handler.set_display_off_timeout_on_lock(5s); - session_lock.unlock(); session_lock.lock(); + handler.set_display_off_timeout_when_locked(5s); } -TEST_F(BasicIdleHandler, off_timeout_on_lock_is_not_used_if_equal_to_off_timeout) +TEST_F(BasicIdleHandler, off_timeout_when_locked_is_not_used_if_equal_to_off_timeout) { EXPECT_CALL(idle_hub, register_interest(_, Eq(30s))) .Times(1); handler.set_display_off_timeout(30s); - handler.set_display_off_timeout_on_lock(30s); + handler.set_display_off_timeout_when_locked(30s); session_lock.lock(); } -TEST_F(BasicIdleHandler, off_timeout_on_lock_can_be_disabled) +TEST_F(BasicIdleHandler, off_timeout_when_locked_can_be_disabled) { - handler.set_display_off_timeout_on_lock(30s); + handler.set_display_off_timeout_when_locked(30s); session_lock.lock(); auto const observer = observer_for(30s); + EXPECT_CALL(idle_hub, register_interest(_, _)) + .Times(0); EXPECT_CALL(idle_hub, unregister_interest(Not(Ref(*observer)))) - .Times(AnyNumber()); + .Times(1); EXPECT_CALL(idle_hub, unregister_interest(Ref(*observer))) .Times(1); + handler.set_display_off_timeout_when_locked(std::nullopt); session_lock.unlock(); - handler.set_display_off_timeout_on_lock(std::nullopt); session_lock.lock(); }