From 26b6520cf953aff0defab3d71648742d5e1b9a04 Mon Sep 17 00:00:00 2001 From: Harlen Batagelo Date: Mon, 12 Aug 2024 16:20:04 -0300 Subject: [PATCH 1/5] Goes idle just after the session is locked --- src/server/shell/basic_idle_handler.cpp | 70 +++++++++++++++++-- src/server/shell/basic_idle_handler.h | 8 ++- src/server/shell/default_configuration.cpp | 3 +- .../shell/test_basic_idle_handler.cpp | 8 ++- 4 files changed, 81 insertions(+), 8 deletions(-) diff --git a/src/server/shell/basic_idle_handler.cpp b/src/server/shell/basic_idle_handler.cpp index 51057549a79..56a36b3d044 100644 --- a/src/server/shell/basic_idle_handler.cpp +++ b/src/server/shell/basic_idle_handler.cpp @@ -17,6 +17,7 @@ #include "basic_idle_handler.h" #include "mir/fatal.h" #include "mir/scene/idle_hub.h" +#include "mir/scene/session_lock.h" #include "mir/graphics/renderable.h" #include "mir/renderer/sw/pixel_source.h" #include "mir/input/scene.h" @@ -168,22 +169,72 @@ struct PowerModeSetter : ms::IdleStateObserver MirPowerMode const power_mode; msh::IdleHandlerObserver& observer; }; + +struct TimeoutRestorer : ms::IdleStateObserver +{ + explicit TimeoutRestorer(msh::BasicIdleHandler* idle_handler, mir::time::Duration timeout) + : idle_handler{idle_handler}, timeout{timeout} + { + } + + void active() override + { + if (!is_active && !restored) + { + restored = true; + idle_handler->set_display_off_timeout(timeout); + } + is_active = true; + } + + void idle() override + { + is_active = false; + } + +private: + mir::shell::BasicIdleHandler* const idle_handler; + mir::time::Duration const timeout; + + bool is_active{true}; + bool restored{false}; +}; } +class msh::BasicIdleHandler::SessionLockListener : public ms::SessionLockObserver +{ +public: + explicit SessionLockListener(BasicIdleHandler* idle_handler) : idle_handler{idle_handler} {}; + +private: + void on_lock() override + { + idle_handler->set_display_off_timeout(time::Duration{}); + }; + void on_unlock() override {}; + + BasicIdleHandler* const idle_handler; +}; + msh::BasicIdleHandler::BasicIdleHandler( std::shared_ptr const& idle_hub, std::shared_ptr const& input_scene, std::shared_ptr const& allocator, - std::shared_ptr const& display_config_controller) + std::shared_ptr const& display_config_controller, + std::shared_ptr const& session_lock) : idle_hub{idle_hub}, input_scene{input_scene}, allocator{allocator}, - display_config_controller{display_config_controller} + display_config_controller{display_config_controller}, + session_lock{session_lock}, + session_lock_monitor{std::make_shared(this)} { + session_lock->register_interest(session_lock_monitor); } msh::BasicIdleHandler::~BasicIdleHandler() { + session_lock->unregister_interest(*session_lock_monitor); std::lock_guard lock{mutex}; clear_observers(lock); } @@ -195,14 +246,15 @@ void msh::BasicIdleHandler::set_display_off_timeout(std::optional0", off_timeout.count()); + fatal_error("BasicIdleHandler given invalid timeout %d, should be >=0", off_timeout.count()); } if (off_timeout >= dim_time_before_off * 2) { @@ -215,6 +267,16 @@ void msh::BasicIdleHandler::set_display_off_timeout(std::optionalregister_interest(power_setter, off_timeout); + if (off_timeout == time::Duration{} && previous_off_timeout) + { + // A zero timeout cannot be set through configuration options. However, + // it can be set internally to immediately turn off the display when + // the session is locked. In this case, the original timeout from the + // config options will be restored the next time Mir wakes from idle. + auto const timeout_restorer = std::make_shared(this, previous_off_timeout.value()); + observers.push_back(timeout_restorer); + idle_hub->register_interest(timeout_restorer, off_timeout); + } } } diff --git a/src/server/shell/basic_idle_handler.h b/src/server/shell/basic_idle_handler.h index d569674eb6b..6ee1aea8f30 100644 --- a/src/server/shell/basic_idle_handler.h +++ b/src/server/shell/basic_idle_handler.h @@ -39,6 +39,7 @@ namespace scene { class IdleHub; class IdleStateObserver; +class SessionLock; } namespace shell { @@ -51,7 +52,8 @@ class BasicIdleHandler : public IdleHandler std::shared_ptr const& idle_hub, std::shared_ptr const& input_scene, std::shared_ptr const& allocator, - std::shared_ptr const& display_config_controller); + std::shared_ptr const& display_config_controller, + std::shared_ptr const& session_lock); ~BasicIdleHandler(); @@ -70,12 +72,16 @@ class BasicIdleHandler : public IdleHandler void unregister_interest(IdleHandlerObserver const&) override; private: + class SessionLockListener; + void clear_observers(ProofOfMutexLock const&); std::shared_ptr const idle_hub; std::shared_ptr const input_scene; std::shared_ptr const allocator; std::shared_ptr const display_config_controller; + std::shared_ptr const session_lock; + std::shared_ptr const session_lock_monitor; std::mutex mutex; std::optional current_off_timeout; diff --git a/src/server/shell/default_configuration.cpp b/src/server/shell/default_configuration.cpp index d9ebb69f7b5..8515af8b114 100644 --- a/src/server/shell/default_configuration.cpp +++ b/src/server/shell/default_configuration.cpp @@ -100,7 +100,8 @@ auto mir::DefaultServerConfiguration::the_idle_handler() -> std::shared_ptrget(options::idle_timeout_opt); diff --git a/tests/unit-tests/shell/test_basic_idle_handler.cpp b/tests/unit-tests/shell/test_basic_idle_handler.cpp index 7a54879fe21..fa8326d9e17 100644 --- a/tests/unit-tests/shell/test_basic_idle_handler.cpp +++ b/tests/unit-tests/shell/test_basic_idle_handler.cpp @@ -19,6 +19,7 @@ #include "mir/test/doubles/mock_idle_hub.h" #include "mir/test/doubles/stub_input_scene.h" #include "mir/test/doubles/stub_buffer_allocator.h" +#include "mir/test/doubles/stub_session_lock.h" #include "mir/shell/display_configuration_controller.h" #include "mir/test/fake_shared.h" @@ -52,11 +53,13 @@ struct BasicIdleHandler: Test mtd::StubInputScene input_scene; mtd::StubBufferAllocator allocator; NiceMock display; + mtd::StubSessionLock session_lock; msh::BasicIdleHandler handler{ mt::fake_shared(idle_hub), mt::fake_shared(input_scene), mt::fake_shared(allocator), - mt::fake_shared(display)}; + mt::fake_shared(display), + mt::fake_shared(session_lock)}; std::map> observers; void SetUp() @@ -95,7 +98,8 @@ TEST_F(BasicIdleHandler, does_not_register_observers_by_default) mt::fake_shared(idle_hub), mt::fake_shared(input_scene), mt::fake_shared(allocator), - mt::fake_shared(display)}; + mt::fake_shared(display), + mt::fake_shared(session_lock)}; } TEST_F(BasicIdleHandler, turns_display_off_when_idle) From 8ff6274968b124fc4c8fbbbdcab7b7969fcea7b1 Mon Sep 17 00:00:00 2001 From: Harlen Batagelo Date: Mon, 23 Sep 2024 12:53:54 -0300 Subject: [PATCH 2/5] Add config option and tests --- include/platform/mir/options/configuration.h | 1 + src/include/server/mir/shell/idle_handler.h | 7 ++ .../options/default_configuration.cpp | 6 ++ src/platform/symbols.map | 4 +- src/server/shell/basic_idle_handler.cpp | 84 +++++++++-------- src/server/shell/basic_idle_handler.h | 4 + src/server/shell/default_configuration.cpp | 6 ++ .../shell/test_basic_idle_handler.cpp | 89 ++++++++++++++++++- 8 files changed, 163 insertions(+), 38 deletions(-) diff --git a/include/platform/mir/options/configuration.h b/include/platform/mir/options/configuration.h index f4af7a7af28..a970bdd0ceb 100644 --- a/include/platform/mir/options/configuration.h +++ b/include/platform/mir/options/configuration.h @@ -46,6 +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 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 6be983fc9cc..b615cc76136 100644 --- a/src/include/server/mir/shell/idle_handler.h +++ b/src/include/server/mir/shell/idle_handler.h @@ -52,6 +52,13 @@ 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; + private: IdleHandler(IdleHandler const&) = delete; IdleHandler& operator=(IdleHandler const&) = delete; diff --git a/src/platform/options/default_configuration.cpp b/src/platform/options/default_configuration.cpp index b2412781c9d..39ff1ebfc2d 100644 --- a/src/platform/options/default_configuration.cpp +++ b/src/platform/options/default_configuration.cpp @@ -44,6 +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::off_opt_value = "off"; char const* const mo::log_opt_value = "log"; @@ -179,6 +180,11 @@ mo::DefaultConfiguration::DefaultConfiguration( (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.") (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 6cc7ac3e89c..94d01e500fc 100644 --- a/src/platform/symbols.map +++ b/src/platform/symbols.map @@ -1,4 +1,4 @@ -MIR_PLATFORM_2.18 { +MIR_PLATFORM_2.19 { global: extern "C++" { mir::graphics::AtomicFrame::increment*; @@ -125,6 +125,7 @@ MIR_PLATFORM_2.18 { mir::options::enable_key_repeat_opt*; mir::options::fatal_except_opt*; mir::options::idle_timeout_opt; + mir::options::idle_timeout_on_lock_opt; mir::options::input_report_opt*; mir::options::log_opt_value*; mir::options::logind_console; @@ -202,4 +203,3 @@ MIR_PLATFORM_2.18 { }; local: *; }; - diff --git a/src/server/shell/basic_idle_handler.cpp b/src/server/shell/basic_idle_handler.cpp index 56a36b3d044..1eadb401b00 100644 --- a/src/server/shell/basic_idle_handler.cpp +++ b/src/server/shell/basic_idle_handler.cpp @@ -172,31 +172,30 @@ struct PowerModeSetter : ms::IdleStateObserver struct TimeoutRestorer : ms::IdleStateObserver { - explicit TimeoutRestorer(msh::BasicIdleHandler* idle_handler, mir::time::Duration timeout) - : idle_handler{idle_handler}, timeout{timeout} + explicit TimeoutRestorer( + msh::BasicIdleHandler* idle_handler, + std::optional timeout) + : idle_handler{idle_handler}, + timeout{timeout} { } - void active() override - { - if (!is_active && !restored) + void active() override { + if (!restored) { restored = true; idle_handler->set_display_off_timeout(timeout); } - is_active = true; } void idle() override { - is_active = false; } private: mir::shell::BasicIdleHandler* const idle_handler; - mir::time::Duration const timeout; + std::optional const timeout; - bool is_active{true}; bool restored{false}; }; } @@ -204,16 +203,13 @@ struct TimeoutRestorer : ms::IdleStateObserver class msh::BasicIdleHandler::SessionLockListener : public ms::SessionLockObserver { public: - explicit SessionLockListener(BasicIdleHandler* idle_handler) : idle_handler{idle_handler} {}; + explicit SessionLockListener(std::function const& on_lock) : on_lock_{on_lock} {}; private: - void on_lock() override - { - idle_handler->set_display_off_timeout(time::Duration{}); - }; + void on_lock() override { on_lock_(); }; void on_unlock() override {}; - BasicIdleHandler* const idle_handler; + std::function const on_lock_; }; msh::BasicIdleHandler::BasicIdleHandler( @@ -227,7 +223,23 @@ msh::BasicIdleHandler::BasicIdleHandler( allocator{allocator}, display_config_controller{display_config_controller}, session_lock{session_lock}, - session_lock_monitor{std::make_shared(this)} + session_lock_monitor{std::make_shared([this] + { + std::lock_guard lock{mutex}; + if (auto const timeout_on_lock{current_off_timeout_on_lock}) + { + if (timeout_on_lock != current_off_timeout) + { + auto const previous_off_timeout{current_off_timeout}; + current_off_timeout = timeout_on_lock; + clear_observers(lock); + register_observers(lock); + auto const timeout_restorer{std::make_shared(this, previous_off_timeout)}; + observers.push_back(timeout_restorer); + this->idle_hub->register_interest(timeout_restorer, time::Duration{0}); + } + } + })} { session_lock->register_interest(session_lock_monitor); } @@ -236,23 +248,35 @@ msh::BasicIdleHandler::~BasicIdleHandler() { session_lock->unregister_interest(*session_lock_monitor); std::lock_guard lock{mutex}; - clear_observers(lock); + for (auto const& observer : observers) + { + idle_hub->unregister_interest(*observer); + } } void msh::BasicIdleHandler::set_display_off_timeout(std::optional timeout) { std::lock_guard lock{mutex}; - if (timeout == current_off_timeout) + if (timeout != current_off_timeout) { - return; + current_off_timeout = timeout; + clear_observers(lock); + register_observers(lock); } - auto const previous_off_timeout{current_off_timeout}; - current_off_timeout = timeout; - clear_observers(lock); - if (timeout) +} + +void msh::BasicIdleHandler::set_display_off_timeout_on_lock(std::optional timeout) +{ + std::lock_guard lock{mutex}; + current_off_timeout_on_lock = timeout; +} + +void msh::BasicIdleHandler::register_observers(ProofOfMutexLock const&) +{ + if (current_off_timeout) { - auto const off_timeout = timeout.value(); - if (off_timeout < time::Duration{}) + auto const off_timeout = current_off_timeout.value(); + if (off_timeout < time::Duration{0}) { fatal_error("BasicIdleHandler given invalid timeout %d, should be >=0", off_timeout.count()); } @@ -267,16 +291,6 @@ void msh::BasicIdleHandler::set_display_off_timeout(std::optionalregister_interest(power_setter, off_timeout); - if (off_timeout == time::Duration{} && previous_off_timeout) - { - // A zero timeout cannot be set through configuration options. However, - // it can be set internally to immediately turn off the display when - // the session is locked. In this case, the original timeout from the - // config options will be restored the next time Mir wakes from idle. - auto const timeout_restorer = std::make_shared(this, previous_off_timeout.value()); - observers.push_back(timeout_restorer); - idle_hub->register_interest(timeout_restorer, off_timeout); - } } } diff --git a/src/server/shell/basic_idle_handler.h b/src/server/shell/basic_idle_handler.h index 6ee1aea8f30..54352b7bd58 100644 --- a/src/server/shell/basic_idle_handler.h +++ b/src/server/shell/basic_idle_handler.h @@ -59,6 +59,8 @@ 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 register_interest(std::weak_ptr const&) override; void register_interest( @@ -74,6 +76,7 @@ class BasicIdleHandler : public IdleHandler private: class SessionLockListener; + void register_observers(ProofOfMutexLock const&); void clear_observers(ProofOfMutexLock const&); std::shared_ptr const idle_hub; @@ -85,6 +88,7 @@ class BasicIdleHandler : public IdleHandler std::mutex mutex; std::optional current_off_timeout; + std::optional current_off_timeout_on_lock; std::vector> observers; class BasicIdleHandlerObserverMultiplexer: public ObserverMultiplexer diff --git a/src/server/shell/default_configuration.cpp b/src/server/shell/default_configuration.cpp index 8515af8b114..2d5f6c0ef34 100644 --- a/src/server/shell/default_configuration.cpp +++ b/src/server/shell/default_configuration.cpp @@ -123,6 +123,12 @@ 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) + { + idle_handler->set_display_off_timeout_on_lock(std::chrono::seconds{idle_timeout_on_lock}); + } + 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 fa8326d9e17..eb5bbf4f905 100644 --- a/tests/unit-tests/shell/test_basic_idle_handler.cpp +++ b/tests/unit-tests/shell/test_basic_idle_handler.cpp @@ -47,13 +47,51 @@ struct MockDisplayConfigurationController: msh::DisplayConfigurationController MOCK_METHOD1(set_power_mode, void(MirPowerMode)); }; +class FakeSessionLock : public ms::SessionLock +{ +public: + void lock() override + { + auto config_observer = observer.lock(); + EXPECT_THAT(config_observer, testing::NotNull()); + config_observer->on_lock(); + } + void unlock() override + { + auto config_observer = observer.lock(); + EXPECT_THAT(config_observer, testing::NotNull()); + config_observer->on_unlock(); + } + void register_interest(std::weak_ptr const& o) override + { + ASSERT_THAT(observer.lock(), testing::IsNull()) + << "FakeSessionLock does not support multiple observers"; + observer = o; + } + void register_interest(std::weak_ptr const& o, mir::Executor&) override + { + register_interest(o); + } + void register_early_observer(std::weak_ptr const& o, mir::Executor&) override + { + register_interest(o); + } + void unregister_interest(ms::SessionLockObserver const& o) override + { + ASSERT_THAT(observer.lock().get(), testing::Eq(&o)); + observer = std::weak_ptr(); + } +private: + std::weak_ptr observer; +}; + struct BasicIdleHandler: Test { NiceMock idle_hub; mtd::StubInputScene input_scene; mtd::StubBufferAllocator allocator; NiceMock display; - mtd::StubSessionLock session_lock; + FakeSessionLock session_lock; msh::BasicIdleHandler handler{ mt::fake_shared(idle_hub), mt::fake_shared(input_scene), @@ -94,6 +132,7 @@ TEST_F(BasicIdleHandler, does_not_register_observers_by_default) { EXPECT_CALL(idle_hub, register_interest(_, _)) .Times(0); + mtd::StubSessionLock session_lock; msh::BasicIdleHandler local_handler{ mt::fake_shared(idle_hub), mt::fake_shared(input_scene), @@ -102,6 +141,54 @@ 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) +{ + EXPECT_CALL(idle_hub, register_interest(_, Eq(30s))) + .Times(1); + handler.set_display_off_timeout(30s); + + handler.set_display_off_timeout_on_lock(10s); + + EXPECT_CALL(idle_hub, register_interest(_, Eq(10s))) + .Times(1); + session_lock.lock(); +} + +TEST_F(BasicIdleHandler, off_timeout_on_lock_is_not_used_when_session_is_not_locked) +{ + EXPECT_CALL(idle_hub, register_interest(_, Eq(30s))) + .Times(1); + handler.set_display_off_timeout(30s); + + EXPECT_CALL(idle_hub, register_interest(_, Eq(10s))) + .Times(0); + handler.set_display_off_timeout_on_lock(10s); +} + +TEST_F(BasicIdleHandler, off_timeout_is_restored_when_locked_session_becomes_active) +{ + EXPECT_CALL(idle_hub, register_interest(_, Eq(30s))) + .Times(2); + handler.set_display_off_timeout(30s); + handler.set_display_off_timeout_on_lock(10s); + + EXPECT_CALL(idle_hub, register_interest(_, Eq(10s))) + .Times(1); + session_lock.lock(); + + auto const observer = observer_for(0s); + observer->active(); +} + +TEST_F(BasicIdleHandler, off_timeout_on_lock_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); + session_lock.lock(); +} + TEST_F(BasicIdleHandler, turns_display_off_when_idle) { handler.set_display_off_timeout(30s); From a5b087c94b6298d68620fe25414f6d3b0edc22a7 Mon Sep 17 00:00:00 2001 From: Harlen Batagelo Date: Mon, 23 Sep 2024 16:40:59 -0300 Subject: [PATCH 3/5] Add new stanza for platform ABI + remove extra semicolons --- src/platform/symbols.map | 12 +++++++++--- src/server/shell/basic_idle_handler.cpp | 6 +++--- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/src/platform/symbols.map b/src/platform/symbols.map index 94d01e500fc..9b436394ded 100644 --- a/src/platform/symbols.map +++ b/src/platform/symbols.map @@ -1,4 +1,4 @@ -MIR_PLATFORM_2.19 { +MIR_PLATFORM_2.18 { global: extern "C++" { mir::graphics::AtomicFrame::increment*; @@ -125,7 +125,6 @@ MIR_PLATFORM_2.19 { mir::options::enable_key_repeat_opt*; mir::options::fatal_except_opt*; mir::options::idle_timeout_opt; - mir::options::idle_timeout_on_lock_opt; mir::options::input_report_opt*; mir::options::log_opt_value*; mir::options::logind_console; @@ -201,5 +200,12 @@ MIR_PLATFORM_2.19 { vtable?for?mir::options::Option; vtable?for?mir::options::ProgramOption; }; - local: *; }; + +MIR_PLATFORM_2.19 { + global: + extern "C++" { + mir::options::idle_timeout_on_lock_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 1eadb401b00..7cf81e5e3d9 100644 --- a/src/server/shell/basic_idle_handler.cpp +++ b/src/server/shell/basic_idle_handler.cpp @@ -203,11 +203,11 @@ struct TimeoutRestorer : ms::IdleStateObserver class msh::BasicIdleHandler::SessionLockListener : public ms::SessionLockObserver { public: - explicit SessionLockListener(std::function const& on_lock) : on_lock_{on_lock} {}; + explicit SessionLockListener(std::function const& on_lock) : on_lock_{on_lock} {} private: - void on_lock() override { on_lock_(); }; - void on_unlock() override {}; + void on_lock() override { on_lock_(); } + void on_unlock() override {} std::function const on_lock_; }; From 0863da21c59327607644ad26ebf196bff375c4ed Mon Sep 17 00:00:00 2001 From: Harlen Batagelo Date: Mon, 30 Sep 2024 12:22:06 -0300 Subject: [PATCH 4/5] Prevent immediate activation after the session is locked; add related tests --- src/server/shell/basic_idle_handler.cpp | 105 ++++++++++++------ src/server/shell/basic_idle_handler.h | 7 ++ src/server/shell/default_configuration.cpp | 6 +- .../shell/test_basic_idle_handler.cpp | 87 +++++++++++++-- 4 files changed, 165 insertions(+), 40 deletions(-) diff --git a/src/server/shell/basic_idle_handler.cpp b/src/server/shell/basic_idle_handler.cpp index 7cf81e5e3d9..22c433410f6 100644 --- a/src/server/shell/basic_idle_handler.cpp +++ b/src/server/shell/basic_idle_handler.cpp @@ -24,6 +24,7 @@ #include "mir/shell/display_configuration_controller.h" #include +#include namespace ms = mir::scene; namespace mg = mir::graphics; @@ -34,6 +35,7 @@ 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; @@ -169,47 +171,58 @@ struct PowerModeSetter : ms::IdleStateObserver MirPowerMode const power_mode; msh::IdleHandlerObserver& observer; }; +} -struct TimeoutRestorer : ms::IdleStateObserver +class msh::BasicIdleHandler::TimeoutRestorer : public ms::IdleStateObserver { +public: explicit TimeoutRestorer( - msh::BasicIdleHandler* idle_handler, - std::optional timeout) - : idle_handler{idle_handler}, - timeout{timeout} + msh::BasicIdleHandler* idle_handler) + : idle_handler{idle_handler} { } void active() override { - if (!restored) + // 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->set_display_off_timeout(timeout); + idle_handler->restore_off_timeout(); } } void idle() override { + was_idle = true; } private: - mir::shell::BasicIdleHandler* const idle_handler; - std::optional const timeout; + msh::BasicIdleHandler* const idle_handler; + bool was_idle{false}; bool restored{false}; }; -} class msh::BasicIdleHandler::SessionLockListener : public ms::SessionLockObserver { public: - explicit SessionLockListener(std::function const& on_lock) : on_lock_{on_lock} {} + explicit SessionLockListener(msh::BasicIdleHandler* idle_handler) : idle_handler{idle_handler} + { + } -private: - void on_lock() override { on_lock_(); } - void on_unlock() override {} + void on_lock() override + { + idle_handler->on_lock(); + } - std::function const on_lock_; + void on_unlock() override + { + idle_handler->on_unlock(); + } + +private: + msh::BasicIdleHandler* const idle_handler; }; msh::BasicIdleHandler::BasicIdleHandler( @@ -223,23 +236,7 @@ msh::BasicIdleHandler::BasicIdleHandler( allocator{allocator}, display_config_controller{display_config_controller}, session_lock{session_lock}, - session_lock_monitor{std::make_shared([this] - { - std::lock_guard lock{mutex}; - if (auto const timeout_on_lock{current_off_timeout_on_lock}) - { - if (timeout_on_lock != current_off_timeout) - { - auto const previous_off_timeout{current_off_timeout}; - current_off_timeout = timeout_on_lock; - clear_observers(lock); - register_observers(lock); - auto const timeout_restorer{std::make_shared(this, previous_off_timeout)}; - observers.push_back(timeout_restorer); - this->idle_hub->register_interest(timeout_restorer, time::Duration{0}); - } - } - })} + session_lock_monitor{std::make_shared(this)} { session_lock->register_interest(session_lock_monitor); } @@ -268,9 +265,38 @@ void msh::BasicIdleHandler::set_display_off_timeout(std::optional timeout) { std::lock_guard lock{mutex}; + if (timeout) + { + timeout = timeout < min_off_timeout_on_lock ? min_off_timeout_on_lock : timeout; + } current_off_timeout_on_lock = timeout; } +void msh::BasicIdleHandler::on_lock() +{ + std::lock_guard lock{mutex}; + if (auto const timeout_on_lock{current_off_timeout_on_lock}) + { + 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); + } + } + +} +void msh::BasicIdleHandler::on_unlock() +{ + restore_off_timeout(); +} + void msh::BasicIdleHandler::register_observers(ProofOfMutexLock const&) { if (current_off_timeout) @@ -305,6 +331,21 @@ 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 54352b7bd58..8971e65b83c 100644 --- a/src/server/shell/basic_idle_handler.h +++ b/src/server/shell/basic_idle_handler.h @@ -75,9 +75,14 @@ class BasicIdleHandler : public IdleHandler private: class SessionLockListener; + class TimeoutRestorer; + + void on_lock(); + void on_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; @@ -88,8 +93,10 @@ 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::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 2d5f6c0ef34..43b1b62a589 100644 --- a/src/server/shell/default_configuration.cpp +++ b/src/server/shell/default_configuration.cpp @@ -124,7 +124,11 @@ auto mir::DefaultServerConfiguration::the_idle_handler() -> std::shared_ptrget(options::idle_timeout_on_lock_opt); - if (idle_timeout_on_lock >= 0) + if (idle_timeout_on_lock < 0) + { + idle_handler->set_display_off_timeout_on_lock(std::nullopt); + } + else { idle_handler->set_display_off_timeout_on_lock(std::chrono::seconds{idle_timeout_on_lock}); } diff --git a/tests/unit-tests/shell/test_basic_idle_handler.cpp b/tests/unit-tests/shell/test_basic_idle_handler.cpp index eb5bbf4f905..268600c05e5 100644 --- a/tests/unit-tests/shell/test_basic_idle_handler.cpp +++ b/tests/unit-tests/shell/test_basic_idle_handler.cpp @@ -145,12 +145,10 @@ TEST_F(BasicIdleHandler, off_timeout_on_lock_is_used_when_session_is_locked) { EXPECT_CALL(idle_hub, register_interest(_, Eq(30s))) .Times(1); - handler.set_display_off_timeout(30s); - - handler.set_display_off_timeout_on_lock(10s); - 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(); } @@ -158,10 +156,9 @@ TEST_F(BasicIdleHandler, off_timeout_on_lock_is_not_used_when_session_is_not_loc { EXPECT_CALL(idle_hub, register_interest(_, Eq(30s))) .Times(1); - handler.set_display_off_timeout(30s); - EXPECT_CALL(idle_hub, register_interest(_, Eq(10s))) .Times(0); + handler.set_display_off_timeout(30s); handler.set_display_off_timeout_on_lock(10s); } @@ -169,17 +166,78 @@ TEST_F(BasicIdleHandler, off_timeout_is_restored_when_locked_session_becomes_act { 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(); + + 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) +{ + EXPECT_CALL(idle_hub, register_interest(_, Eq(30s))) + .Times(1); 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); - auto const observer = observer_for(0s); + 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); + 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) +{ + EXPECT_CALL(idle_hub, register_interest(_, Eq(30s))) + .Times(2); + 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(); +} + TEST_F(BasicIdleHandler, off_timeout_on_lock_is_not_used_if_equal_to_off_timeout) { EXPECT_CALL(idle_hub, register_interest(_, Eq(30s))) @@ -189,6 +247,21 @@ TEST_F(BasicIdleHandler, off_timeout_on_lock_is_not_used_if_equal_to_off_timeout session_lock.lock(); } +TEST_F(BasicIdleHandler, off_timeout_on_lock_can_be_disabled) +{ + handler.set_display_off_timeout_on_lock(30s); + session_lock.lock(); + auto const observer = observer_for(30s); + + EXPECT_CALL(idle_hub, unregister_interest(Not(Ref(*observer)))) + .Times(AnyNumber()); + EXPECT_CALL(idle_hub, unregister_interest(Ref(*observer))) + .Times(1); + session_lock.unlock(); + handler.set_display_off_timeout_on_lock(std::nullopt); + session_lock.lock(); +} + TEST_F(BasicIdleHandler, turns_display_off_when_idle) { handler.set_display_off_timeout(30s); From 004fcd377ffd8a7c84904348f3c0e87d46a4d2a7 Mon Sep 17 00:00:00 2001 From: Harlen Batagelo Date: Thu, 17 Oct 2024 09:03:25 -0300 Subject: [PATCH 5/5] 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(); }