Merge lp:~raof/mir/notify-shell-of-display-configuration into lp:mir

[Chris Halse Rogers]

Add a notification mechanism for display configuration changes.

I noticed that we don't currently have any way to get notified when the display configuration has changed; this is one of the major pieces of awkwardness in our display configuration tests.

This is also going to be awkward for shells. Indeed, adding this is a prerequisite for fixing LP: 1556142.

[Alan Griffiths (alan-griffiths)]

"I noticed that we don't currently have any way to get notified when the display configuration has changed; this is one of the major pieces of awkwardness in our display configuration tests."

Not entirely true: I have successfully implemented miral::ActiveOutputsMonitor to provide notifications when the display configuration changes.

[Alan Griffiths (alan-griffiths)]

Apart from the sequencing[*] in MediatingDisplayChanger::apply_config(), how does this improve on implementing DisplayConfigurationReport::new_configuration()?

[*] which is dubious at present and I prefer where you've put it. But that is a different change.

review: Needs Information

[Mir CI Bot (mir-ci-bot)]

PASSED: Continuous integration, rev:3695
https://mir-jenkins.ubuntu.com/job/mir-ci/1669/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/2090
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/2152
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/2143
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/2143
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/2143
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2118
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2118/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2118
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2118/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2118
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2118/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2118
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2118/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2118
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2118/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/1669/rebuild

review: Approve (continuous-integration)

[Chris Halse Rogers (raof)]

Huh. I *guess* you could use the logging interface for detecting display configuration changes.

But I interpret the various mir::report::* interfaces we have as fundamentally debugging aids. In turn, that means that I think we should be able to remove all calls to anything under mir::report:: and still have correct system behaviour.

So I think this improves on implementing DisplayConfigurationReport::new_configuration() in discoverability (Gerry has also been looking for a way to do this, and didn't think to do that), and explicitness.

It's also likely to help preserve the utility of the Reports - if users of Mir start using reports to introspect state like this they need to ensure they wrap the existing report otherwise it becomes useless for reporting.

(I would not object to calling DisplayConfigurationReport::new_configuration() in a wrapper *from* the configured_callback).

[Chris Halse Rogers (raof)]

I guess that also means that I don't think that reports should be visible to the shell *at all*. If shells want to pipe debug logs elsewhere they can replace the Logger.

[Chris Halse Rogers (raof)]

Hm.

That being said, since MirAL is the future of the shell-visible Mir API if you've already implemented notification there it may not be worthwhile adding this?

[Daniel van Vugt (vanvugt)]

The shell (Unity8) is just a client. Why don't we do this via an event using the client API?

There are use cases for normal clients too, like telling a game that its secondary monitor was unplugged.

review: Needs Information

[Chris Halse Rogers (raof)]

We have existing client API for this, but Unity8 is not a client. It
doesn't have a(n accessible) MirConnection.

[Alan Griffiths (alan-griffiths)]

> Huh. I *guess* you could use the logging interface for detecting display
> configuration changes.

You have that backwards. The reason we have the report interfaces is that there are things that need reports from Mir that are not logging.

I remember long discussions when we introduced reports about why we didn't just log directly. This is an example of something that isn't logging or lttng.

> But I interpret the various mir::report::* interfaces we have as fundamentally
> debugging aids.

While the integrated log/lttng reports are all about recording stuff for later human analysis that isn't the whole of the purpose of providing a way to monitor the system state.

> In turn, that means that I think we should be able to remove
> all calls to anything under mir::report:: and still have correct system
> behaviour.

*mirserver* should behave correctly.

> So I think this improves on implementing
> DisplayConfigurationReport::new_configuration() in discoverability (Gerry has
> also been looking for a way to do this, and didn't think to do that), and
> explicitness.
>
> It's also likely to help preserve the utility of the Reports - if users of Mir
> start using reports to introspect state like this they need to ensure they
> wrap the existing report otherwise it becomes useless for reporting.

That's on my list of things to improve after working downstream for a while. My intention is to provide "add report" - which would also allow both logging and lttng reports to be used.

> (I would not object to calling DisplayConfigurationReport::new_configuration()
> in a wrapper *from* the configured_callback).

I don't see the need to add another layer of indirection.

review: Disapprove

[Daniel van Vugt (vanvugt)]

review: Abstain

[Cemil Azizoglu (cemil-azizoglu)]

I don't really think reports are a good way to monitor system behaviour. But I guess it's pre-existing. I still don't mind landing this and doing away with "reports-as-observers".

review: Approve

[Alan Griffiths (alan-griffiths)]

> I don't really think reports are a good way to monitor system behaviour. But I
> guess it's pre-existing. I still don't mind landing this and doing away with
> "reports-as-observers".

I disapprove of having two ways to do the same job.

I won't block renaming the "report" to an "observer".

review: Disapprove

[Daniel van Vugt (vanvugt)]

I hear RAOF is working on the alternative approach. Is this branch formally rejected now?

[Chris Halse Rogers (raof)]

Yeah, let's call it rejected.

Unmerged revisions

3695. By Chris Halse Rogers on 2016-09-08

...and moke set_configured_callback threadsafe.

3694. By Chris Halse Rogers on 2016-09-08

More complete and correct set_configured_callback()

3693. By Chris Halse Rogers on 2016-09-08

Add (start of) a configured-notification to msh::DisplayConfigurationController().

There are all sorts of times that it'd be useful in our test code to know when a configuration
change had occurred, and I'm sure that shells will find it equally useful.

=== modified file 'include/server/mir/shell/display_configuration_controller.h'
--- include/server/mir/shell/display_configuration_controller.h	2016-01-29 08:18:22 +0000
+++ include/server/mir/shell/display_configuration_controller.h	2016-09-08 08:25:23 +0000
@@ -49,6 +49,23 @@
      */
     virtual void set_base_configuration(
         std::shared_ptr<graphics::DisplayConfiguration> const& conf) = 0;
+
+    /**
+     * Set the display configuration notification delegate
+     *
+     * This function is called immediately after a display configuration change has
+     * completed. This is called for any configuration change, from whatever source.
+     * This includes configuration changes from the shell, from changes in client
+     * focus where a client has a display configuration set, and client changes to the base
+     * display configuration.
+     *
+     *
+     * \param [in] delegate Functor to call after every display configuration change. This
+     *                      function is called with a reference to the just-applied display
+     *                      configuration.
+     */
+    virtual void set_configured_callback(
+        std::function<void(graphics::DisplayConfiguration const&)> const& delegate) = 0;
 };
 }
 }

=== modified file 'src/server/scene/mediating_display_changer.cpp'
--- src/server/scene/mediating_display_changer.cpp	2016-08-18 07:30:18 +0000
+++ src/server/scene/mediating_display_changer.cpp	2016-09-08 08:25:23 +0000
@@ -124,7 +124,8 @@
       base_configuration_{display->configuration()},
       base_configuration_applied{true},
       region{region},
-      alarm_factory{alarm_factory}
+      alarm_factory{alarm_factory},
+      configured_callback{std::make_shared<decltype(configured_callback)::element_type>([](auto&){})}
 {
     session_event_handler_register->register_focus_change_handler(
         [this](std::shared_ptr<ms::Session> const& session)
@@ -350,6 +351,13 @@
     }
     update_input_rectangles(*conf);
 
+    decltype(configured_callback) configured_callback_copy;
+    {
+        std::lock_guard<std::mutex> lock{configured_callback_mutex};
+        configured_callback_copy = configured_callback;
+    }
+    (*configured_callback_copy)(*conf);
+
     base_configuration_applied = false;
 }
 
@@ -438,3 +446,10 @@
     });
     region->set_input_rectangles(rectangles);
 }
+
+void ms::MediatingDisplayChanger::set_configured_callback(
+    std::function<void(mg::DisplayConfiguration const&)> const& delegate)
+{
+    std::lock_guard<std::mutex> lock{configured_callback_mutex};
+    configured_callback = std::make_shared<decltype(configured_callback)::element_type>(delegate);
+}

=== modified file 'src/server/scene/mediating_display_changer.h'
--- src/server/scene/mediating_display_changer.h	2016-08-18 07:30:18 +0000
+++ src/server/scene/mediating_display_changer.h	2016-09-08 08:25:23 +0000
@@ -96,6 +96,8 @@
 
     /* From shell::DisplayConfigurationController */
     void set_base_configuration(std::shared_ptr<graphics::DisplayConfiguration> const &conf) override;
+    void set_configured_callback(
+        std::function<void(graphics::DisplayConfiguration const&)> const& delegate) override;
 
 private:
     void focus_change_handler(std::shared_ptr<Session> const& session);
@@ -127,6 +129,9 @@
     std::shared_ptr<time::AlarmFactory> const alarm_factory;
     std::unique_ptr<time::Alarm> preview_configuration_timeout;
     std::weak_ptr<frontend::Session> currently_previewing_session;
+
+    std::mutex configured_callback_mutex;
+    std::shared_ptr<std::function<void(graphics::DisplayConfiguration const&)>> configured_callback;
 };
 
 }

=== modified file 'tests/unit-tests/scene/test_mediating_display_changer.cpp'
--- tests/unit-tests/scene/test_mediating_display_changer.cpp	2016-08-29 23:00:22 +0000
+++ tests/unit-tests/scene/test_mediating_display_changer.cpp	2016-09-08 08:25:23 +0000
@@ -34,6 +34,7 @@
 #include "mir/test/fake_shared.h"
 #include "mir/test/display_config_matchers.h"
 #include "mir/test/doubles/fake_alarm_factory.h"
+#include "mir/test/signal.h"
 
 #include <mutex>
 
@@ -950,3 +951,143 @@
     changer->configure(session,
                        mt::fake_shared(conf));
 }
+
+TEST_F(MediatingDisplayChangerTest, configured_callback_is_triggered_on_base_configuration_change)
+{
+    using namespace testing;
+    using namespace std::chrono_literals;
+
+    auto config = changer->base_configuration();
+    config->for_each_output(
+        [](mg::UserDisplayConfigurationOutput& output)
+        {
+            output.used = !output.used;
+        });
+
+    bool callback_triggered{false};
+    changer->set_configured_callback([&callback_triggered](auto const&) { callback_triggered = true; });
+
+    changer->set_base_configuration(config);
+
+    EXPECT_TRUE(callback_triggered);
+}
+
+TEST_F(MediatingDisplayChangerTest, configured_callback_is_triggered_on_preview_base_configuration)
+{
+    using namespace testing;
+    using namespace std::chrono_literals;
+
+    auto config =
+        changer->base_configuration();
+    config->for_each_output(
+        [](mg::UserDisplayConfigurationOutput& output)
+        {
+            output.used = !output.used;
+        });
+
+    bool callback_triggered{false};
+    changer->set_configured_callback([&callback_triggered](auto const&) { callback_triggered = true; });
+
+    auto session = std::make_shared<mtd::StubSession>();
+    changer->preview_base_configuration(session, config, 10s);
+
+    EXPECT_TRUE(callback_triggered);
+}
+
+TEST_F(MediatingDisplayChangerTest, configured_callback_is_triggered_on_cancel_base_configuration)
+{
+    using namespace testing;
+    using namespace std::chrono_literals;
+
+    auto config =
+        changer->base_configuration();
+    config->for_each_output(
+        [](mg::UserDisplayConfigurationOutput& output)
+        {
+            output.used = !output.used;
+        });
+
+    int callback_count{0};
+    changer->set_configured_callback([&callback_count](auto const&) { callback_count++; });
+
+    auto session = std::make_shared<mtd::StubSession>();
+    changer->preview_base_configuration(session, config, 10min);
+
+    EXPECT_THAT(callback_count, Eq(1));
+
+    changer->cancel_base_configuration_preview(session);
+    EXPECT_THAT(callback_count, Eq(2));
+}
+
+TEST_F(MediatingDisplayChangerTest, configured_callback_is_called_when_focusing_a_session_with_attached_config)
+{
+    using namespace testing;
+    auto config =
+        changer->base_configuration();
+    config->for_each_output(
+        [](mg::UserDisplayConfigurationOutput& output)
+        {
+            output.used = !output.used;
+        });
+
+    bool callback_triggered{false};
+    changer->set_configured_callback([&callback_triggered](auto const&) { callback_triggered = true; });
+
+    auto session = std::make_shared<mtd::StubSession>();
+
+    stub_session_container.insert_session(session);
+    changer->configure(session, config);
+
+    EXPECT_FALSE(callback_triggered);
+
+    session_event_sink.handle_focus_change(session);
+
+    EXPECT_TRUE(callback_triggered);
+}
+
+TEST_F(MediatingDisplayChangerTest, configured_callback_is_called_when_a_focused_session_changes_config)
+{
+    using namespace testing;
+    auto config =
+        changer->base_configuration();
+    config->for_each_output(
+        [](mg::UserDisplayConfigurationOutput& output)
+        {
+            output.used = !output.used;
+        });
+
+    bool callback_triggered{false};
+    changer->set_configured_callback([&callback_triggered](auto const&) { callback_triggered = true; });
+
+    auto session = std::make_shared<mtd::StubSession>();
+
+    stub_session_container.insert_session(session);
+    session_event_sink.handle_focus_change(session);
+
+    changer->configure(session, config);
+
+    EXPECT_TRUE(callback_triggered);
+}
+
+TEST_F(MediatingDisplayChangerTest, configured_callback_is_not_called_when_an_unfocused_session_changes_config)
+{
+    using namespace testing;
+    auto config =
+        changer->base_configuration();
+    config->for_each_output(
+        [](mg::UserDisplayConfigurationOutput& output)
+        {
+            output.used = !output.used;
+        });
+
+    bool callback_triggered{false};
+    changer->set_configured_callback([&callback_triggered](auto const&) { callback_triggered = true; });
+
+    auto session = std::make_shared<mtd::StubSession>();
+
+    stub_session_container.insert_session(session);
+
+    changer->configure(session, config);
+
+    EXPECT_FALSE(callback_triggered);
+}