Merge lp:~afrantzis/mir/fix-1511798-alternative into lp:mir

[Alexandros Frantzis]

shell: fix updating of display configuration when applications and nested servers exit

shell: fix updating of display configuration when applications and nested servers exit

The tests for nested mir behavior were cherrypicked from Alan's original proposal.

[Alan Griffiths (alan-griffiths)]

I'm confused.

When I try the scenario in lp:1511798 I don't see an attempt to restore the host config after exiting the client. (But that's what display_configuration_reset_when_application_exits is supposed to test.)

I need to check what is different between the automated and manual test scenarios.

review: Abstain

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:3093
http://jenkins.qa.ubuntu.com/job/mir-ci/5517/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/4816
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/3723
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/4758/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-wily-touch/716/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1671
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1671/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/716
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/716/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4759
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4759/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/7362/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/24964
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-wily-armhf/717/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5517/rebuild

review: Needs Fixing (continuous-integration)

[Alan Griffiths (alan-griffiths)]

> I'm confused.
>
> When I try the scenario in lp:1511798 I don't see an attempt to restore the
> host config after exiting the client. (But that's what
> display_configuration_reset_when_application_exits is supposed to test.)
>
> I need to check what is different between the automated and manual test
> scenarios.

I understand the difference - AbstractShell::focus_next_session() is selecting the dying surface's titlebar. (And that doesn't exist in the core WM that gets tested.)

I have to think about the right solution here.

[Alan Griffiths (alan-griffiths)]

I think the right solution is to move the "session->destroy_surface(surface);" call from AbstractShell::destroy_surface() to <Policy>::handle_delete_surface(). (There are four of these, plus DefaultWindowManager::remove_surface() in playground.)

It seems like a PITA, but this allows the window management policy control over the point at which the surface and decorations are destroyed.

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

Like this: lp:~alan-griffiths/mir/fix-1511798-another-alternative

[Alexandros Frantzis (afrantzis)]

Rejected in favor of the other alternative.

=== modified file 'src/server/scene/mediating_display_changer.cpp'
--- src/server/scene/mediating_display_changer.cpp	2015-11-05 12:54:02 +0000
+++ src/server/scene/mediating_display_changer.cpp	2015-11-06 10:57:51 +0000
@@ -148,7 +148,7 @@
 {
     std::lock_guard<std::mutex> lg{configuration_mutex};
 
-    return base_configuration_;
+    return base_configuration_->clone();
 }
 
 void ms::MediatingDisplayChanger::configure_for_hardware_change(

=== modified file 'src/server/shell/abstract_shell.cpp'
--- src/server/shell/abstract_shell.cpp	2015-11-04 07:43:28 +0000
+++ src/server/shell/abstract_shell.cpp	2015-11-06 10:57:51 +0000
@@ -107,8 +107,11 @@
     mf::SurfaceId surface)
 {
     report->destroying_surface(*session, surface);
-    window_manager->remove_surface(session, session->surface(surface));
+    // Order matters. The surface needs to be destroyed before calling the WM,
+    // so the WM and its tools can make decisions based on up-to-date information.
+    auto weak_surface = std::weak_ptr<ms::Surface>{session->surface(surface)};
     session->destroy_surface(surface);
+    window_manager->remove_surface(session, weak_surface);
 }
 
 std::shared_ptr<ms::PromptSession> msh::AbstractShell::start_prompt_session_for(
@@ -163,16 +166,22 @@
 void msh::AbstractShell::focus_next_session()
 {
     std::unique_lock<std::mutex> lock(focus_mutex);
-    auto session = focus_session.lock();
-
-    session = session_coordinator->successor_of(session);
-
-    std::shared_ptr<ms::Surface> surface;
-
-    if (session)
-        surface = session->default_surface();
-
-    set_focus_to_locked(lock, session, surface);
+    auto const focused_session = focus_session.lock();
+    auto successor = session_coordinator->successor_of(focused_session);
+
+    while (successor != nullptr &&
+           successor != focused_session &&
+           successor->default_surface() == nullptr)
+    {
+        successor = session_coordinator->successor_of(successor);
+    }
+
+    auto const surface = successor ? successor->default_surface() : nullptr;
+
+    if (!surface)
+        successor = nullptr;
+
+    set_focus_to_locked(lock, successor, surface);
 }
 
 std::shared_ptr<ms::Session> msh::AbstractShell::focused_session() const

=== modified file 'tests/acceptance-tests/test_display_configuration.cpp'
--- tests/acceptance-tests/test_display_configuration.cpp	2015-10-26 03:33:22 +0000
+++ tests/acceptance-tests/test_display_configuration.cpp	2015-11-06 10:57:51 +0000
@@ -22,7 +22,7 @@
 #include "mir/graphics/event_handler_register.h"
 #include "mir/shell/display_configuration_controller.h"
 
-#include "mir_test_framework/connected_client_headless_server.h"
+#include "mir_test_framework/connected_client_with_a_surface.h"
 #include "mir/test/doubles/null_platform.h"
 #include "mir/test/doubles/null_display.h"
 #include "mir/test/doubles/null_display_sync_group.h"
@@ -143,13 +143,13 @@
 }
 }
 
-struct DisplayConfigurationTest : mtf::ConnectedClientHeadlessServer
+struct DisplayConfigurationTest : mtf::ConnectedClientWithASurface
 {
     void SetUp() override
     {
         server.override_the_session_authorizer([this] { return mt::fake_shared(stub_authorizer); });
         preset_display(mt::fake_shared(mock_display));
-        mtf::ConnectedClientHeadlessServer::SetUp();
+        mtf::ConnectedClientWithASurface::SetUp();
     }
 
     testing::NiceMock<MockDisplay> mock_display;

=== modified file 'tests/acceptance-tests/test_nested_mir.cpp'
--- tests/acceptance-tests/test_nested_mir.cpp	2015-11-04 17:59:20 +0000
+++ tests/acceptance-tests/test_nested_mir.cpp	2015-11-06 10:57:51 +0000
@@ -714,3 +714,85 @@
     mir_surface_release_sync(surface);
     mir_connection_release(connection);
 }
+
+// lp:1511798
+TEST_F(NestedServer, display_configuration_reset_when_application_exits)
+{
+    NestedMirRunner nested_mir{new_connection()};
+    ignore_rebuild_of_egl_context();
+
+    auto const connection = mir_connect_sync(nested_mir.new_connection().c_str(), __PRETTY_FUNCTION__);
+
+    // Need a painted surface to have focus
+    auto const painted_surface = make_and_paint_surface(connection);
+
+    auto const configuration = mir_connection_create_display_config(connection);
+
+    configuration->outputs->used = false;
+
+    mt::WaitCondition initial_condition;
+
+    EXPECT_CALL(*the_mock_display_configuration_report(), new_configuration(_))
+        .WillRepeatedly(InvokeWithoutArgs([&] { initial_condition.wake_up_everyone(); }));
+
+    mir_wait_for(mir_connection_apply_display_config(connection, configuration));
+
+    mir_display_config_destroy(configuration);
+    mir_surface_release_sync(painted_surface);
+
+    // Wait for initial config to be applied
+    initial_condition.wait_for_at_most_seconds(1);
+    Mock::VerifyAndClearExpectations(the_mock_display_configuration_report().get());
+    ASSERT_TRUE(initial_condition.woken());
+
+    mt::WaitCondition condition;
+    EXPECT_CALL(*the_mock_display_configuration_report(), new_configuration(_))
+        .WillRepeatedly(InvokeWithoutArgs([&] { condition.wake_up_everyone(); }));
+
+    mir_connection_release(connection);
+
+    condition.wait_for_at_most_seconds(1);
+    EXPECT_TRUE(condition.woken());
+    Mock::VerifyAndClearExpectations(the_mock_display_configuration_report().get());
+}
+
+// lp:1511798
+TEST_F(NestedServer, display_configuration_reset_when_nested_server_exits)
+{
+    mt::WaitCondition condition;
+
+    {
+        NestedMirRunner nested_mir{new_connection()};
+        ignore_rebuild_of_egl_context();
+
+        auto const connection = mir_connect_sync(nested_mir.new_connection().c_str(), __PRETTY_FUNCTION__);
+
+        // Need a painted surface to have focus
+        auto const painted_surface = make_and_paint_surface(connection);
+
+        std::shared_ptr<mg::DisplayConfiguration> const new_config{nested_mir.server.the_display()->configuration()};
+        new_config->for_each_output([](mg::UserDisplayConfigurationOutput& output)
+                                        { output.orientation = mir_orientation_inverted;});
+
+        mt::WaitCondition initial_condition;
+
+        EXPECT_CALL(*the_mock_display_configuration_report(), new_configuration(_))
+            .WillRepeatedly(InvokeWithoutArgs([&] { initial_condition.wake_up_everyone(); }));
+
+        nested_mir.server.the_display_configuration_controller()->set_base_configuration(new_config);
+
+        // Wait for initial config to be applied
+        initial_condition.wait_for_at_most_seconds(1);
+        Mock::VerifyAndClearExpectations(the_mock_display_configuration_report().get());
+        ASSERT_TRUE(initial_condition.woken());
+
+        EXPECT_CALL(*the_mock_display_configuration_report(), new_configuration(_))
+            .WillRepeatedly(InvokeWithoutArgs([&] { condition.wake_up_everyone(); }));
+
+        mir_surface_release_sync(painted_surface);
+        mir_connection_release(connection);
+    }
+
+    condition.wait_for_at_most_seconds(1);
+    EXPECT_TRUE(condition.woken());
+}

=== modified file 'tests/unit-tests/scene/test_abstract_shell.cpp'
--- tests/unit-tests/scene/test_abstract_shell.cpp	2015-11-04 07:43:28 +0000
+++ tests/unit-tests/scene/test_abstract_shell.cpp	2015-11-06 10:57:51 +0000
@@ -31,7 +31,6 @@
 
 #include "mir/test/doubles/mock_window_manager.h"
 #include "mir/test/doubles/mock_surface_coordinator.h"
-#include "mir/test/doubles/mock_session_listener.h"
 #include "mir/test/doubles/mock_surface.h"
 #include "mir/test/doubles/null_event_sink.h"
 #include "mir/test/doubles/null_snapshot_strategy.h"
@@ -107,7 +106,6 @@
     NiceMock<mtd::MockSurfaceCoordinator> surface_coordinator;
     NiceMock<MockSessionContainer> session_container;
     NiceMock<MockSessionEventSink> session_event_sink;
-    NiceMock<mtd::MockSessionListener> session_listener;
     NiceMock<MockSurfaceFactory> surface_factory;
     mtd::StubDisplay display{3};
 
@@ -118,7 +116,7 @@
         mt::fake_shared(session_container),
         std::make_shared<mtd::NullSnapshotStrategy>(),
         mt::fake_shared(session_event_sink),
-        mt::fake_shared(session_listener),
+        std::make_shared<ms::NullSessionListener>(),
         mt::fake_shared(display),
         std::make_shared<mtd::NullANRDetector>()};
 
@@ -367,6 +365,9 @@
     msh::FocusController& focus_controller = shell;
     auto session = shell.open_session(__LINE__, "XPlane", std::shared_ptr<mf::EventSink>());
     auto session1 = shell.open_session(__LINE__, "Bla", std::shared_ptr<mf::EventSink>());
+    shell.create_surface(session, ms::a_surface(), nullptr);
+    shell.create_surface(session1, ms::a_surface(), nullptr);
+
     focus_controller.set_focus_to(session, {});
 
     EXPECT_CALL(session_container, successor_of(session)).
@@ -381,6 +382,9 @@
 {
     auto session = shell.open_session(__LINE__, "XPlane", std::shared_ptr<mf::EventSink>());
     auto session1 = shell.open_session(__LINE__, "Bla", std::shared_ptr<mf::EventSink>());
+    shell.create_surface(session, ms::a_surface(), nullptr);
+    shell.create_surface(session1, ms::a_surface(), nullptr);
+
     EXPECT_CALL(session_container, successor_of(session1)).
         WillOnce(Return(session));
 
@@ -440,6 +444,49 @@
     EXPECT_THAT(focus_controller.surface_at(cursor), Eq(surface));
 }
 
+TEST_F(AbstractShell, as_focus_controller_focus_next_session_skips_surfaceless_sessions)
+{
+    msh::FocusController& focus_controller = shell;
+    auto session = shell.open_session(__LINE__, "XPlane", std::shared_ptr<mf::EventSink>());
+    auto session1 = shell.open_session(__LINE__, "Surfaceless", std::shared_ptr<mf::EventSink>());
+    auto session2 = shell.open_session(__LINE__, "Bla", std::shared_ptr<mf::EventSink>());
+    auto surface_id = shell.create_surface(session, ms::a_surface(), nullptr);
+    shell.create_surface(session2, ms::a_surface(), nullptr);
+
+    focus_controller.set_focus_to(session, session->surface(surface_id));
+
+    EXPECT_CALL(session_container, successor_of(session)).
+        WillOnce(Return(session1));
+    EXPECT_CALL(session_container, successor_of(session1)).
+        WillOnce(Return(session2));
+
+    EXPECT_CALL(session_event_sink, handle_focus_change(session2));
+
+    focus_controller.focus_next_session();
+}
+
+TEST_F(AbstractShell,
+       as_focus_controller_focus_next_session_does_not_focus_any_session_if_no_session_has_surfaces)
+{
+    msh::FocusController& focus_controller = shell;
+    auto session = shell.open_session(__LINE__, "XPlane", std::shared_ptr<mf::EventSink>());
+    auto session1 = shell.open_session(__LINE__, "Surfaceless", std::shared_ptr<mf::EventSink>());
+    auto surface_id = shell.create_surface(session, ms::a_surface(), nullptr);
+
+    focus_controller.set_focus_to(session, session->surface(surface_id));
+
+    shell.destroy_surface(session, surface_id);
+
+    EXPECT_CALL(session_container, successor_of(session)).
+        WillOnce(Return(session1));
+    EXPECT_CALL(session_container, successor_of(session1)).
+        WillOnce(Return(session));
+
+    EXPECT_CALL(session_event_sink, handle_no_focus());
+
+    focus_controller.focus_next_session();
+}
+
 TEST_F(AbstractShell, modify_surface_with_only_streams_doesnt_call_into_wm)
 {
     std::shared_ptr<ms::Session> session =