Merge lp:~albaguirre/mir/fix-1483909 into lp:mir

[Alberto Aguirre]

canonical window manager: restore surface visibility
(LP: #1483909)

surface->show() is now called by the window manager when appropriate.

When I modified BasicSurface::configure(mir_surface_attrib_state, state)to no longer modify its visibility state I forgot to actually make the window manager handle the restoration part.

I also addressed the the mir_surface_spec_set_state/mir_surface_apply_spec path as it did not previously propagate surface state changes.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2834
http://jenkins.qa.ubuntu.com/job/mir-ci/4541/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3501
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-wily-amd64-build/361
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/1033
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3449
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/690
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/690/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3449
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3449/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6234
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/22496

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

review: Approve (continuous-integration)

[Chris Halse Rogers (raof)]

Looks sensible to me.

review: Approve

[Alan Griffiths (alan-griffiths)]

LGTM

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/mir-autolanding/1772/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3506
    FAILURE: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-wily-amd64-build/367/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/1039/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3454/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-autolanding/325/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3454
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3454/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6239/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/22505

review: Needs Fixing (continuous-integration)

[Kevin DuBois (kdub)]

lgtm

review: Approve

=== modified file 'examples/server_example_canonical_window_manager.cpp'
--- examples/server_example_canonical_window_manager.cpp	2015-07-27 07:24:50 +0000
+++ examples/server_example_canonical_window_manager.cpp	2015-08-11 22:18:03 +0000
@@ -181,6 +181,19 @@
     }
 }
 
+bool me::CanonicalSurfaceInfoCopy::is_visible() const
+{
+    switch (state)
+    {
+    case mir_surface_state_hidden:
+    case mir_surface_state_minimized:
+        return false;
+    default:
+        break;
+    }
+    return true;
+}
+
 
 me::CanonicalWindowManagerPolicyCopy::CanonicalWindowManagerPolicyCopy(
     Tools* const tools,
@@ -555,6 +568,12 @@
 
         apply_resize(surface, surface_info.titlebar, top_left, new_size);
     }
+
+    if (modifications.state.is_set())
+    {
+        auto const state = handle_set_state(surface, modifications.state.value());
+        surface->configure(mir_surface_attrib_state, state);
+    }
 }
 
 void me::CanonicalWindowManagerPolicyCopy::handle_delete_surface(std::shared_ptr<ms::Session> const& session, std::weak_ptr<ms::Surface> const& surface)
@@ -682,7 +701,12 @@
     // TODO some sensible layout rules.
     move_tree(surface, movement);
 
-    return info.state = value;
+    info.state = value;
+
+    if (info.is_visible())
+        surface->show();
+
+    return info.state;
 }
 
 void me::CanonicalWindowManagerPolicyCopy::drag(Point cursor)

=== modified file 'examples/server_example_canonical_window_manager.h'
--- examples/server_example_canonical_window_manager.h	2015-06-23 06:49:01 +0000
+++ examples/server_example_canonical_window_manager.h	2015-08-11 22:18:03 +0000
@@ -50,6 +50,7 @@
     bool can_morph_to(MirSurfaceType new_type) const;
     bool must_have_parent() const;
     bool must_not_have_parent() const;
+    bool is_visible() const;
 
     void constrain_resize(
         std::shared_ptr<scene::Surface> const& surface,

=== modified file 'src/include/server/mir/shell/canonical_window_manager.h'
--- src/include/server/mir/shell/canonical_window_manager.h	2015-06-26 08:00:59 +0000
+++ src/include/server/mir/shell/canonical_window_manager.h	2015-08-11 22:18:03 +0000
@@ -46,6 +46,7 @@
     bool can_morph_to(MirSurfaceType new_type) const;
     bool must_have_parent() const;
     bool must_not_have_parent() const;
+    bool is_visible() const;
 
     void constrain_resize(
         std::shared_ptr<scene::Surface> const& surface,

=== modified file 'src/server/shell/canonical_window_manager.cpp'
--- src/server/shell/canonical_window_manager.cpp	2015-07-27 07:24:50 +0000
+++ src/server/shell/canonical_window_manager.cpp	2015-08-11 22:18:03 +0000
@@ -166,6 +166,19 @@
     return ::must_not_have_parent(type);
 }
 
+bool msh::CanonicalSurfaceInfo::is_visible() const
+{
+    switch (state)
+    {
+    case mir_surface_state_hidden:
+    case mir_surface_state_minimized:
+        return false;
+    default:
+        break;
+    }
+    return true;
+}
+
 
 msh::CanonicalWindowManagerPolicy::CanonicalWindowManagerPolicy(
     Tools* const tools,
@@ -468,6 +481,12 @@
 
     if (modifications.input_shape.is_set())
         surface->set_input_region(modifications.input_shape.value());
+
+    if (modifications.state.is_set())
+    {
+        auto const state = handle_set_state(surface, modifications.state.value());
+        surface->configure(mir_surface_attrib_state, state);
+    }
 }
 
 void msh::CanonicalWindowManagerPolicy::handle_delete_surface(std::shared_ptr<ms::Session> const& session, std::weak_ptr<ms::Surface> const& surface)
@@ -573,7 +592,12 @@
     // TODO some sensible layout rules.
     move_tree(surface, movement);
 
-    return info.state = value;
+    info.state = value;
+
+    if (info.is_visible())
+        surface->show();
+
+    return info.state;
 }
 
 void msh::CanonicalWindowManagerPolicy::drag(Point cursor)

=== modified file 'tests/acceptance-tests/test_surface_modifications.cpp'
--- tests/acceptance-tests/test_surface_modifications.cpp	2015-06-26 08:00:59 +0000
+++ tests/acceptance-tests/test_surface_modifications.cpp	2015-08-11 22:18:03 +0000
@@ -46,6 +46,7 @@
 public:
     MOCK_METHOD1(renamed, void(char const*));
     MOCK_METHOD1(resized_to, void(Size const& size));
+    MOCK_METHOD1(hidden_set_to, void(bool));
 };
 
 struct SurfaceModifications : mtf::ConnectedClientWithASurface
@@ -135,7 +136,7 @@
 
     MirInputDeviceId const device_id = MirInputDeviceId(7);
     std::chrono::nanoseconds const timestamp = std::chrono::nanoseconds(39);
-    MockSurfaceObserver surface_observer;
+    NiceMock<MockSurfaceObserver> surface_observer;
     std::weak_ptr<ms::Surface> shell_surface;
 };
 
@@ -149,6 +150,30 @@
     return Height(value) == arg.height;
 }
 
+struct StatePair
+{
+    MirSurfaceState from;
+    MirSurfaceState to;
+
+    friend std::ostream& operator<<(std::ostream& out, StatePair const& types)
+        { return out << "from:" << types.from << ", to:" << types.to; }
+};
+
+bool is_visible(MirSurfaceState state)
+{
+    switch (state)
+    {
+    case mir_surface_state_hidden:
+    case mir_surface_state_minimized:
+        return false;
+    default:
+        break;
+    }
+    return true;
+}
+
+struct SurfaceStateCase : SurfaceModifications, ::testing::WithParamInterface<StatePair> {};
+struct SurfaceSpecStateCase : SurfaceModifications, ::testing::WithParamInterface<StatePair> {};
 }
 
 TEST_F(SurfaceModifications, surface_spec_name_is_notified)
@@ -486,3 +511,74 @@
         EXPECT_THAT(actual, Eq(expected_size));
     };
 }
+
+TEST_F(SurfaceModifications, surface_spec_state_affects_surface_visibility)
+{
+    auto const new_state = mir_surface_state_hidden;
+
+    EXPECT_CALL(surface_observer, hidden_set_to(true));
+
+    apply_changes([&](MirSurfaceSpec* spec)
+        {
+            mir_surface_spec_set_state(spec, new_state);
+        });
+}
+
+TEST_P(SurfaceSpecStateCase, set_state_affects_surface_visibility)
+{
+    auto const initial_state = GetParam().from;
+    auto const new_state = GetParam().to;
+
+    EXPECT_CALL(surface_observer, hidden_set_to(is_visible(initial_state)));
+    EXPECT_CALL(surface_observer, hidden_set_to(is_visible(new_state)));
+
+    apply_changes([&](MirSurfaceSpec* spec)
+        {
+            mir_surface_spec_set_state(spec, initial_state);
+        });
+
+    apply_changes([&](MirSurfaceSpec* spec)
+       {
+           mir_surface_spec_set_state(spec, new_state);
+       });
+}
+
+INSTANTIATE_TEST_CASE_P(SurfaceModifications, SurfaceSpecStateCase,
+    Values(
+        StatePair{mir_surface_state_hidden, mir_surface_state_restored},
+        StatePair{mir_surface_state_hidden, mir_surface_state_maximized},
+        StatePair{mir_surface_state_hidden, mir_surface_state_vertmaximized},
+        StatePair{mir_surface_state_hidden, mir_surface_state_horizmaximized},
+        StatePair{mir_surface_state_hidden, mir_surface_state_fullscreen},
+        StatePair{mir_surface_state_minimized, mir_surface_state_restored},
+        StatePair{mir_surface_state_minimized, mir_surface_state_maximized},
+        StatePair{mir_surface_state_minimized, mir_surface_state_vertmaximized},
+        StatePair{mir_surface_state_minimized, mir_surface_state_horizmaximized},
+        StatePair{mir_surface_state_minimized, mir_surface_state_fullscreen}
+    ));
+
+TEST_P(SurfaceStateCase, set_state_affects_surface_visibility)
+{
+    auto const initial_state = GetParam().from;
+    auto const new_state = GetParam().to;
+
+    EXPECT_CALL(surface_observer, hidden_set_to(is_visible(initial_state)));
+    EXPECT_CALL(surface_observer, hidden_set_to(is_visible(new_state)));
+
+    mir_wait_for(mir_surface_set_state(surface, initial_state));
+    mir_wait_for(mir_surface_set_state(surface, new_state));
+}
+
+INSTANTIATE_TEST_CASE_P(SurfaceModifications, SurfaceStateCase,
+    Values(
+        StatePair{mir_surface_state_hidden, mir_surface_state_restored},
+        StatePair{mir_surface_state_hidden, mir_surface_state_maximized},
+        StatePair{mir_surface_state_hidden, mir_surface_state_vertmaximized},
+        StatePair{mir_surface_state_hidden, mir_surface_state_horizmaximized},
+        StatePair{mir_surface_state_hidden, mir_surface_state_fullscreen},
+        StatePair{mir_surface_state_minimized, mir_surface_state_restored},
+        StatePair{mir_surface_state_minimized, mir_surface_state_maximized},
+        StatePair{mir_surface_state_minimized, mir_surface_state_vertmaximized},
+        StatePair{mir_surface_state_minimized, mir_surface_state_horizmaximized},
+        StatePair{mir_surface_state_minimized, mir_surface_state_fullscreen}
+    ));