Merge lp:~andreas-pokorny/mir/fix-1580774 into lp:mir

[Andreas Pokorny]

Use the connected status of the display output to configure pointer confinement

The virtual display is powered and available but not connected on startup, hence it would be accounted as reachable display area. And the mouse would be allowed to leave the first output, which leads to a termination of input events being sent to the nested session..

This is the hotfix that will probably be released within 0.22.1. and maybe also integrated into 0.23.

The virtual display is created 'on' has a valid mode but is not connected on startup. On M10 the screen is 1200x1920, and the virtual display is 1920x1080. As a result the allowed input area was calculated to be 1920x1920. In the coordinate system of usc, the surface of unity8 ends at x = 1200 moving beyond that point makes the cursor leave unity8. No more mouse events reach unity8. Unity8 maps that movement along the other axis, so there is a phantom border on the right that you cannot cross..

[Daniel van Vugt (vanvugt)]

+ if (output.connected && output.power_mode == mir_power_mode_on &&

Should be just 'if (output.used &&'. Because we don't want to include unused but connected outputs, and we DO want to include used but sleeping outputs (they should turn on when the cursor moves anyway).

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

Same fix needs applying to lp:mir/0.22

[Daniel van Vugt (vanvugt)]

I still think power_mode needs to be ignored. 'used' is the only field that matters. If you're moving the mouse then your used displays should be already on or turning on as a result of the mouse movement.

A monitor that is 'used' but also 'off' (which probably never exists anyway) should not in theory disallow the cursor moving to that monitor. In fact, you need the cursor to be able to reach it to ensure it turns on.

review: Needs Fixing

[Andreas Pokorny (andreas-pokorny)]

> I still think power_mode needs to be ignored. 'used' is the only field that
> matters. If you're moving the mouse then your used displays should be already
> on or turning on as a result of the mouse movement.
>
> A monitor that is 'used' but also 'off' (which probably never exists anyway)
> should not in theory disallow the cursor moving to that monitor. In fact, you
> need the cursor to be able to reach it to ensure it turns on.

Hmm I fully agree. But I am unsure if that works out - especially since unity8 and usc may have a different sense of cursor placement. Unity8 only extracts the relative motion, and may have a different resulting location.. Yes this is wrong we should have a better way to express that. So I think we should only do that change on lp:mir and let 0.22 and 0.23 use the logic as is.

* Needs discussion * Who would turn on the screen in that scenario?

[Andreas Pokorny (andreas-pokorny)]

^ discussion topic above ..

review: Needs Information

[Kevin DuBois (kdub)]

fix probably has to be ported to lp:mir/0.23 release as well

[Kevin DuBois (kdub)]

If the logic should just be to check for 'used' IMO, we may as well change it for 0.22/0.23.
If its prohibitively difficult to test the change for the fix (maybe its on a device we dont have?), then I'd be onboard with landing the existing code. So I guess my question is: Is it prohibitively tricky to quickly test the fix to unblock OTA11? If yes, then approve, else needs-fixing

review: Needs Information

[Kevin DuBois (kdub)]

From quick irc chat, seems that making usc more aware of the poitner might take some time, so +1.

review: Approve

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

PASSED: Continuous integration, rev:3508
https://mir-jenkins.ubuntu.com/job/mir-ci/981/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/1063
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/1110
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/1101
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/1101
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/1073
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/1073/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/1073
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/1073/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/1073
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/1073/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/1073
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/1073/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/1073
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/1073/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Cemil Azizoglu (cemil-azizoglu)]

Ok.

kdub, is it too late to cherry pick this into 0.23?

review: Approve

[Daniel van Vugt (vanvugt)]

If you assume your shell turns on all monitors whenever the cursor moves at all, then this new version will work. I was only pushing for a simpler version with fewer preconditions because it's possible, and always desirable.

review: Abstain

=== modified file 'src/server/scene/mediating_display_changer.cpp'
--- src/server/scene/mediating_display_changer.cpp	2016-03-29 07:30:50 +0000
+++ src/server/scene/mediating_display_changer.cpp	2016-05-16 07:49:33 +0000
@@ -353,11 +353,10 @@
 void ms::MediatingDisplayChanger::update_input_rectangles(mg::DisplayConfiguration const& config)
 {
     geometry::Rectangles rectangles;
-    config.for_each_output(
-        [&rectangles](mg::DisplayConfigurationOutput const& output)
-        {
-            if (output.power_mode == mir_power_mode_on && output.current_mode_index < output.modes.size())
-                rectangles.add(geometry::Rectangle(output.top_left, output.modes[output.current_mode_index].size));
-        });
+    config.for_each_output([&rectangles](mg::DisplayConfigurationOutput const& output) {
+        if (output.used && output.connected && output.power_mode == mir_power_mode_on &&
+            output.current_mode_index < output.modes.size())
+            rectangles.add(geometry::Rectangle(output.top_left, output.modes[output.current_mode_index].size));
+    });
     region->set_input_rectangles(rectangles);
 }

=== modified file 'tests/unit-tests/scene/test_mediating_display_changer.cpp'
--- tests/unit-tests/scene/test_mediating_display_changer.cpp	2016-03-29 07:30:50 +0000
+++ tests/unit-tests/scene/test_mediating_display_changer.cpp	2016-05-16 07:49:33 +0000
@@ -45,12 +45,52 @@
 namespace mf = mir::frontend;
 namespace ms = mir::scene;
 namespace mg = mir::graphics;
+namespace geom = mir::geometry;
 
 using namespace testing;
 
 namespace
 {
 
+auto display_output(
+    mg::DisplayConfigurationOutputId id, geom::Point pos, geom::Size size, bool connected, bool used, MirPowerMode mode)
+    -> mg::DisplayConfigurationOutput
+{
+        return mg::DisplayConfigurationOutput{id,
+                                              mg::DisplayConfigurationCardId{0},
+                                              mg::DisplayConfigurationOutputType::lvds,
+                                              std::vector<MirPixelFormat>{mir_pixel_format_abgr_8888},
+                                              {mg::DisplayConfigurationMode{size, 60}},
+                                              0,
+                                              geom::Size{40, 40},
+                                              connected,
+                                              used,
+                                              pos,
+                                              0,
+                                              mir_pixel_format_abgr_8888,
+                                              mode,
+                                              mir_orientation_normal,
+                                              1.0f,
+                                              mir_form_factor_phone
+        };
+}
+
+struct TestDisplayConfiguration : mtd::NullDisplayConfiguration
+{
+    std::vector<mg::DisplayConfigurationOutput> const outputs;
+    TestDisplayConfiguration(std::vector<mg::DisplayConfigurationOutput> const& items)
+        : outputs{items} {}
+    void for_each_output(std::function<void(mg::DisplayConfigurationOutput const&)> fun) const override
+    {
+        for (auto const& output : outputs)
+            fun(output);
+    }
+    std::unique_ptr<mg::DisplayConfiguration> clone() const override
+    {
+        return std::make_unique<TestDisplayConfiguration>(outputs);
+    }
+};
+
 class MockDisplayConfigurationPolicy : public mg::DisplayConfigurationPolicy
 {
 public:
@@ -806,3 +846,104 @@
     ASSERT_THAT(received_configuration, Not(Eq(nullptr)));
     EXPECT_THAT(*received_configuration, mt::DisplayConfigMatches(std::cref(*new_config)));
 }
+
+TEST_F(MediatingDisplayChangerTest, input_region_skipps_not_connected_displays)
+{
+    using namespace testing;
+
+    auto const connected = true;
+    auto const disconnected = false;
+    auto const used = true;
+    mir::geometry::Rectangles expected_rectangles;
+    expected_rectangles.add(geom::Rectangle{geom::Point{0,0}, geom::Size{100,100}});
+
+    TestDisplayConfiguration conf{{display_output(mg::DisplayConfigurationOutputId{0},
+                                                  geom::Point{0, 0},
+                                                  geom::Size{100, 100},
+                                                  connected,
+                                                  used,
+                                                  mir_power_mode_on),
+                                   display_output(mg::DisplayConfigurationOutputId{1},
+                                                  geom::Point{100, 0},
+                                                  geom::Size{100, 100},
+                                                  disconnected,
+                                                  used,
+                                                  mir_power_mode_on)}};
+
+    EXPECT_CALL(mock_input_region, set_input_rectangles(expected_rectangles));
+
+    auto session = std::make_shared<mtd::StubSession>();
+
+    session_event_sink.handle_focus_change(session);
+    changer->configure(session,
+                       mt::fake_shared(conf));
+}
+
+TEST_F(MediatingDisplayChangerTest, input_region_accumulates_powered_and_connected_displays)
+{
+    using namespace testing;
+
+    auto const connected = true;
+    auto const first_monitor = geom::Rectangle{geom::Point{0, 0}, geom::Size{ 40, 40 }};
+    auto const second_monitor = geom::Rectangle{geom::Point{40, 0}, geom::Size{ 10, 10}};
+    auto const used = true;
+    mir::geometry::Rectangles expected_rectangles;
+    expected_rectangles.add(first_monitor);
+    expected_rectangles.add(second_monitor);
+
+    TestDisplayConfiguration conf{{display_output(mg::DisplayConfigurationOutputId{0},
+                                                  first_monitor.top_left,
+                                                  first_monitor.size,
+                                                  connected,
+                                                  used,
+                                                  mir_power_mode_on),
+                                   display_output(mg::DisplayConfigurationOutputId{1},
+                                                  second_monitor.top_left,
+                                                  second_monitor.size,
+                                                  connected,
+                                                  used,
+                                                  mir_power_mode_on)}};
+
+    EXPECT_CALL(mock_input_region, set_input_rectangles(expected_rectangles));
+
+    auto session = std::make_shared<mtd::StubSession>();
+
+    session_event_sink.handle_focus_change(session);
+    changer->configure(session,
+                       mt::fake_shared(conf));
+}
+
+
+TEST_F(MediatingDisplayChangerTest, input_region_accumulates_powered_connected_skips_unused_displays)
+{
+    using namespace testing;
+
+    auto const connected = true;
+    auto const first_monitor = geom::Rectangle{geom::Point{0, 0}, geom::Size{ 40, 40 }};
+    auto const second_monitor = geom::Rectangle{geom::Point{40, 0}, geom::Size{ 10, 10}};
+    auto const not_used = false;
+    auto const used = true;
+    mir::geometry::Rectangles expected_rectangles;
+    expected_rectangles.add(second_monitor);
+
+    TestDisplayConfiguration conf{{display_output(mg::DisplayConfigurationOutputId{0},
+                                                  first_monitor.top_left,
+                                                  first_monitor.size,
+                                                  connected,
+                                                  not_used,
+                                                  mir_power_mode_on),
+                                   display_output(mg::DisplayConfigurationOutputId{1},
+                                                  second_monitor.top_left,
+                                                  second_monitor.size,
+                                                  connected,
+                                                  used,
+                                                  mir_power_mode_on)}};
+
+    EXPECT_CALL(mock_input_region, set_input_rectangles(expected_rectangles));
+
+    auto session = std::make_shared<mtd::StubSession>();
+
+    session_event_sink.handle_focus_change(session);
+    changer->configure(session,
+                       mt::fake_shared(conf));
+}