Mir

Merge lp:~alan-griffiths/mir/fix-1506846 into lp:mir

Proposed by Alan Griffiths on 2015-11-10
Status: Merged
Merged at revision: 3103
Proposed branch: lp:~alan-griffiths/mir/fix-1506846
Merge into: lp:mir
Diff against target: 211 lines (+65/-14)
7 files modified
include/server/mir/shell/display_layout.h (+4/-2)
include/server/mir/shell/system_compositor_window_manager.h (+9/-0)
src/server/shell/CMakeLists.txt (+2/-0)
src/server/shell/graphics_display_layout.cpp (+2/-6)
src/server/shell/graphics_display_layout.h (+4/-4)
src/server/shell/system_compositor_window_manager.cpp (+43/-1)
tests/acceptance-tests/test_system_compositor_window_manager.cpp (+1/-1)
To merge this branch: bzr merge lp:~alan-griffiths/mir/fix-1506846
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration 2015-11-10 Needs Fixing on 2015-11-13
Alexandros Frantzis (community) 2015-11-10 Approve on 2015-11-12
Review via email: mp+277092@code.launchpad.net

This proposal supersedes a proposal from 2015-11-10.

Commit message

shell: system compositors should update fullscreen windows when the output is reconfigured.

Description of the change

shell: system compositors should update fullscreen windows when the output is reconfigured.

This avoids the problem seen in lp:1506846 of the nested servers surfaces being placed according to a display configuration that has become obsolete.

To post a comment you must log in.
Alexandros Frantzis (afrantzis) wrote : Posted in a previous version of this proposal

I think this will break the phone startup by showing the unity8 session too early, while it is still blank. We could fix this by reintroducing similar logic as the one removed (i.e. a SurfaceReadyObserver) in USC's window manager.

review: Needs Information
Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal

> I think this will break the phone startup by showing the unity8 session too
> early, while it is still blank.

I see your point but...

> We could fix this by reintroducing similar
> logic as the one removed (i.e. a SurfaceReadyObserver) in USC's window
> manager.

...that would then break this fix.

Alexandros Frantzis (afrantzis) wrote : Posted in a previous version of this proposal

... and this also brings up a few questions: What does it mean for a session to be ready? Does it make sense for readiness to differ depending on the WM?

review: Needs Information
Alexandros Frantzis (afrantzis) wrote : Posted in a previous version of this proposal

> > We could fix this by reintroducing similar
> > logic as the one removed (i.e. a SurfaceReadyObserver) in USC's window
> > manager.
>
> ...that would then break this fix.

Ah, correct... The purpose of this fix is to focus the session early so its display config is applied.

Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal

Alternatively, what does "fullscreen_on_output" mean for a session that has set a display configuration that changes that output?

PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Alexandros Frantzis (afrantzis) wrote :

Looks good.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'include/server/mir/shell/display_layout.h'
--- include/server/mir/shell/display_layout.h 2013-08-28 03:41:48 +0000
+++ include/server/mir/shell/display_layout.h 2015-11-12 18:48:46 +0000
@@ -53,12 +53,14 @@
53 virtual void size_to_output(geometry::Rectangle& rect) = 0;53 virtual void size_to_output(geometry::Rectangle& rect) = 0;
5454
55 /**55 /**
56 * Places a rectangle in an particular output.56 * Places a rectangle in an particular output if the display is known,
57 * otherwise does nothing.
57 *58 *
58 * @param [in] id the id of the output to place the rectangle in59 * @param [in] id the id of the output to place the rectangle in
59 * @param [in,out] rect the rectangle to place60 * @param [in,out] rect the rectangle to place
61 * @return true iff the display id is recognised
60 */62 */
61 virtual void place_in_output(graphics::DisplayConfigurationOutputId id,63 virtual bool place_in_output(graphics::DisplayConfigurationOutputId id,
62 geometry::Rectangle& rect) = 0;64 geometry::Rectangle& rect) = 0;
6365
64protected:66protected:
6567
=== modified file 'include/server/mir/shell/system_compositor_window_manager.h'
--- include/server/mir/shell/system_compositor_window_manager.h 2015-10-29 22:07:47 +0000
+++ include/server/mir/shell/system_compositor_window_manager.h 2015-11-12 18:48:46 +0000
@@ -20,6 +20,10 @@
20#define MIR_SHELL_SYSTEM_COMPOSITOR_WINDOW_MANAGER_H_20#define MIR_SHELL_SYSTEM_COMPOSITOR_WINDOW_MANAGER_H_
2121
22#include "mir/shell/window_manager.h"22#include "mir/shell/window_manager.h"
23#include "mir/graphics/display_configuration.h"
24
25#include <map>
26#include <mutex>
2327
24namespace mir28namespace mir
25{29{
@@ -96,6 +100,11 @@
96 std::shared_ptr<scene::Surface> const& surface,100 std::shared_ptr<scene::Surface> const& surface,
97 MirSurfaceAttrib attrib,101 MirSurfaceAttrib attrib,
98 int value) override;102 int value) override;
103
104 using OutputMap = std::map<std::weak_ptr<scene::Surface>, graphics::DisplayConfigurationOutputId, std::owner_less<std::weak_ptr<scene::Surface>>>;
105
106 std::mutex mutable mutex;
107 OutputMap output_map;
99};108};
100}109}
101}110}
102111
=== modified file 'src/server/shell/CMakeLists.txt'
--- src/server/shell/CMakeLists.txt 2015-10-29 02:08:24 +0000
+++ src/server/shell/CMakeLists.txt 2015-11-12 18:48:46 +0000
@@ -5,7 +5,9 @@
5 canonical_window_manager.cpp5 canonical_window_manager.cpp
6 frontend_shell.cpp6 frontend_shell.cpp
7 graphics_display_layout.cpp7 graphics_display_layout.cpp
8 graphics_display_layout.h
8 default_configuration.cpp9 default_configuration.cpp
10 null_host_lifecycle_event_listener.h
9 shell_wrapper.cpp11 shell_wrapper.cpp
10 surface_ready_observer.cpp12 surface_ready_observer.cpp
11 system_compositor_window_manager.cpp13 system_compositor_window_manager.cpp
1214
=== modified file 'src/server/shell/graphics_display_layout.cpp'
--- src/server/shell/graphics_display_layout.cpp 2015-04-28 07:54:10 +0000
+++ src/server/shell/graphics_display_layout.cpp 2015-11-12 18:48:46 +0000
@@ -24,9 +24,6 @@
24#include "mir/geometry/rectangles.h"24#include "mir/geometry/rectangles.h"
25#include "mir/geometry/displacement.h"25#include "mir/geometry/displacement.h"
2626
27#include <boost/throw_exception.hpp>
28#include <stdexcept>
29
30namespace msh = mir::shell;27namespace msh = mir::shell;
31namespace mg = mir::graphics;28namespace mg = mir::graphics;
32namespace geom = mir::geometry;29namespace geom = mir::geometry;
@@ -68,7 +65,7 @@
68 rect = output;65 rect = output;
69}66}
7067
71void msh::GraphicsDisplayLayout::place_in_output(68bool msh::GraphicsDisplayLayout::place_in_output(
72 graphics::DisplayConfigurationOutputId id,69 graphics::DisplayConfigurationOutputId id,
73 geometry::Rectangle& rect)70 geometry::Rectangle& rect)
74{71{
@@ -87,8 +84,7 @@
87 }84 }
88 });85 });
8986
90 if (!placed)87 return placed;
91 BOOST_THROW_EXCEPTION(std::runtime_error("Failed to place surface in requested output"));
92}88}
9389
94geom::Rectangle msh::GraphicsDisplayLayout::get_output_for(geometry::Rectangle& rect)90geom::Rectangle msh::GraphicsDisplayLayout::get_output_for(geometry::Rectangle& rect)
9591
=== modified file 'src/server/shell/graphics_display_layout.h'
--- src/server/shell/graphics_display_layout.h 2014-03-06 06:05:17 +0000
+++ src/server/shell/graphics_display_layout.h 2015-11-12 18:48:46 +0000
@@ -37,10 +37,10 @@
37public:37public:
38 GraphicsDisplayLayout(std::shared_ptr<graphics::Display> const& display);38 GraphicsDisplayLayout(std::shared_ptr<graphics::Display> const& display);
3939
40 void clip_to_output(geometry::Rectangle& rect);40 void clip_to_output(geometry::Rectangle& rect) override;
41 void size_to_output(geometry::Rectangle& rect);41 void size_to_output(geometry::Rectangle& rect) override;
42 void place_in_output(graphics::DisplayConfigurationOutputId output_id,42 bool place_in_output(graphics::DisplayConfigurationOutputId output_id,
43 geometry::Rectangle& rect);43 geometry::Rectangle& rect) override;
4444
45private:45private:
46 geometry::Rectangle get_output_for(geometry::Rectangle& rect);46 geometry::Rectangle get_output_for(geometry::Rectangle& rect);
4747
=== modified file 'src/server/shell/system_compositor_window_manager.cpp'
--- src/server/shell/system_compositor_window_manager.cpp 2015-10-29 22:07:47 +0000
+++ src/server/shell/system_compositor_window_manager.cpp 2015-11-12 18:48:46 +0000
@@ -28,6 +28,8 @@
28#include "mir/scene/surface.h"28#include "mir/scene/surface.h"
29#include "mir/scene/surface_creation_parameters.h"29#include "mir/scene/surface_creation_parameters.h"
3030
31#include <boost/throw_exception.hpp>
32
31namespace mf = mir::frontend;33namespace mf = mir::frontend;
32namespace ms = mir::scene;34namespace ms = mir::scene;
33namespace msh = mir::shell;35namespace msh = mir::shell;
@@ -60,6 +62,9 @@
60{62{
61 mir::geometry::Rectangle rect{params.top_left, params.size};63 mir::geometry::Rectangle rect{params.top_left, params.size};
6264
65 if (!params.output_id.as_value())
66 BOOST_THROW_EXCEPTION(std::runtime_error("An output ID must be specified"));
67
63 display_layout->place_in_output(params.output_id, rect);68 display_layout->place_in_output(params.output_id, rect);
6469
65 auto placed_parameters = params;70 auto placed_parameters = params;
@@ -78,6 +83,9 @@
78 surface);83 surface);
7984
80 surface->add_observer(session_ready_observer);85 surface->add_observer(session_ready_observer);
86
87 std::lock_guard<decltype(mutex)> lock{mutex};
88 output_map[surface] = params.output_id;
8189
82 return result;90 return result;
83}91}
@@ -89,16 +97,50 @@
89{97{
90 if (modifications.name.is_set())98 if (modifications.name.is_set())
91 surface->rename(modifications.name.value());99 surface->rename(modifications.name.value());
100
101 if (modifications.output_id.is_set())
102 {
103 auto const output_id = modifications.output_id.value();
104
105 mir::geometry::Rectangle rect{surface->top_left(), surface->size()};
106
107 if (display_layout->place_in_output(output_id, rect))
108 {
109 surface->move_to(rect.top_left);
110 surface->resize(rect.size);
111 }
112
113 std::lock_guard<decltype(mutex)> lock{mutex};
114 output_map[surface] = output_id;
115 }
92}116}
93117
94void msh::SystemCompositorWindowManager::remove_surface(118void msh::SystemCompositorWindowManager::remove_surface(
95 std::shared_ptr<ms::Session> const& /*session*/,119 std::shared_ptr<ms::Session> const& /*session*/,
96 std::weak_ptr<ms::Surface> const& /*surface*/)120 std::weak_ptr<ms::Surface> const& surface)
97{121{
122 output_map.erase(surface);
98}123}
99124
100void msh::SystemCompositorWindowManager::add_display(mir::geometry::Rectangle const& /*area*/)125void msh::SystemCompositorWindowManager::add_display(mir::geometry::Rectangle const& /*area*/)
101{126{
127 std::lock_guard<decltype(mutex)> lock{mutex};
128
129 for (auto const& so : output_map)
130 {
131 if (auto surface = so.first.lock())
132 {
133 auto const output_id = so.second;
134
135 mir::geometry::Rectangle rect{surface->top_left(), surface->size()};
136
137 if (display_layout->place_in_output(output_id, rect))
138 {
139 surface->move_to(rect.top_left);
140 surface->resize(rect.size);
141 }
142 }
143 }
102}144}
103145
104void msh::SystemCompositorWindowManager::remove_display(mir::geometry::Rectangle const& /*area*/)146void msh::SystemCompositorWindowManager::remove_display(mir::geometry::Rectangle const& /*area*/)
105147
=== modified file 'tests/acceptance-tests/test_system_compositor_window_manager.cpp'
--- tests/acceptance-tests/test_system_compositor_window_manager.cpp 2015-10-02 04:27:35 +0000
+++ tests/acceptance-tests/test_system_compositor_window_manager.cpp 2015-11-12 18:48:46 +0000
@@ -167,7 +167,7 @@
167 auto surface = client.create_surface(0);167 auto surface = client.create_surface(0);
168168
169 EXPECT_FALSE(mir_surface_is_valid(surface));169 EXPECT_FALSE(mir_surface_is_valid(surface));
170 EXPECT_THAT(mir_surface_get_error_message(surface), HasSubstr("Failed to place surface"));170 EXPECT_THAT(mir_surface_get_error_message(surface), HasSubstr("An output ID must be specified"));
171}171}
172172
173TEST_F(SystemCompositorWindowManager, if_a_surface_posts_client_gets_focus)173TEST_F(SystemCompositorWindowManager, if_a_surface_posts_client_gets_focus)

Subscribers

People subscribed via source and target branches