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 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
1=== modified file 'include/server/mir/shell/display_layout.h'
2--- include/server/mir/shell/display_layout.h 2013-08-28 03:41:48 +0000
3+++ include/server/mir/shell/display_layout.h 2015-11-12 18:48:46 +0000
4@@ -53,12 +53,14 @@
5 virtual void size_to_output(geometry::Rectangle& rect) = 0;
6
7 /**
8- * Places a rectangle in an particular output.
9+ * Places a rectangle in an particular output if the display is known,
10+ * otherwise does nothing.
11 *
12 * @param [in] id the id of the output to place the rectangle in
13 * @param [in,out] rect the rectangle to place
14+ * @return true iff the display id is recognised
15 */
16- virtual void place_in_output(graphics::DisplayConfigurationOutputId id,
17+ virtual bool place_in_output(graphics::DisplayConfigurationOutputId id,
18 geometry::Rectangle& rect) = 0;
19
20 protected:
21
22=== modified file 'include/server/mir/shell/system_compositor_window_manager.h'
23--- include/server/mir/shell/system_compositor_window_manager.h 2015-10-29 22:07:47 +0000
24+++ include/server/mir/shell/system_compositor_window_manager.h 2015-11-12 18:48:46 +0000
25@@ -20,6 +20,10 @@
26 #define MIR_SHELL_SYSTEM_COMPOSITOR_WINDOW_MANAGER_H_
27
28 #include "mir/shell/window_manager.h"
29+#include "mir/graphics/display_configuration.h"
30+
31+#include <map>
32+#include <mutex>
33
34 namespace mir
35 {
36@@ -96,6 +100,11 @@
37 std::shared_ptr<scene::Surface> const& surface,
38 MirSurfaceAttrib attrib,
39 int value) override;
40+
41+ using OutputMap = std::map<std::weak_ptr<scene::Surface>, graphics::DisplayConfigurationOutputId, std::owner_less<std::weak_ptr<scene::Surface>>>;
42+
43+ std::mutex mutable mutex;
44+ OutputMap output_map;
45 };
46 }
47 }
48
49=== modified file 'src/server/shell/CMakeLists.txt'
50--- src/server/shell/CMakeLists.txt 2015-10-29 02:08:24 +0000
51+++ src/server/shell/CMakeLists.txt 2015-11-12 18:48:46 +0000
52@@ -5,7 +5,9 @@
53 canonical_window_manager.cpp
54 frontend_shell.cpp
55 graphics_display_layout.cpp
56+ graphics_display_layout.h
57 default_configuration.cpp
58+ null_host_lifecycle_event_listener.h
59 shell_wrapper.cpp
60 surface_ready_observer.cpp
61 system_compositor_window_manager.cpp
62
63=== modified file 'src/server/shell/graphics_display_layout.cpp'
64--- src/server/shell/graphics_display_layout.cpp 2015-04-28 07:54:10 +0000
65+++ src/server/shell/graphics_display_layout.cpp 2015-11-12 18:48:46 +0000
66@@ -24,9 +24,6 @@
67 #include "mir/geometry/rectangles.h"
68 #include "mir/geometry/displacement.h"
69
70-#include <boost/throw_exception.hpp>
71-#include <stdexcept>
72-
73 namespace msh = mir::shell;
74 namespace mg = mir::graphics;
75 namespace geom = mir::geometry;
76@@ -68,7 +65,7 @@
77 rect = output;
78 }
79
80-void msh::GraphicsDisplayLayout::place_in_output(
81+bool msh::GraphicsDisplayLayout::place_in_output(
82 graphics::DisplayConfigurationOutputId id,
83 geometry::Rectangle& rect)
84 {
85@@ -87,8 +84,7 @@
86 }
87 });
88
89- if (!placed)
90- BOOST_THROW_EXCEPTION(std::runtime_error("Failed to place surface in requested output"));
91+ return placed;
92 }
93
94 geom::Rectangle msh::GraphicsDisplayLayout::get_output_for(geometry::Rectangle& rect)
95
96=== modified file 'src/server/shell/graphics_display_layout.h'
97--- src/server/shell/graphics_display_layout.h 2014-03-06 06:05:17 +0000
98+++ src/server/shell/graphics_display_layout.h 2015-11-12 18:48:46 +0000
99@@ -37,10 +37,10 @@
100 public:
101 GraphicsDisplayLayout(std::shared_ptr<graphics::Display> const& display);
102
103- void clip_to_output(geometry::Rectangle& rect);
104- void size_to_output(geometry::Rectangle& rect);
105- void place_in_output(graphics::DisplayConfigurationOutputId output_id,
106- geometry::Rectangle& rect);
107+ void clip_to_output(geometry::Rectangle& rect) override;
108+ void size_to_output(geometry::Rectangle& rect) override;
109+ bool place_in_output(graphics::DisplayConfigurationOutputId output_id,
110+ geometry::Rectangle& rect) override;
111
112 private:
113 geometry::Rectangle get_output_for(geometry::Rectangle& rect);
114
115=== modified file 'src/server/shell/system_compositor_window_manager.cpp'
116--- src/server/shell/system_compositor_window_manager.cpp 2015-10-29 22:07:47 +0000
117+++ src/server/shell/system_compositor_window_manager.cpp 2015-11-12 18:48:46 +0000
118@@ -28,6 +28,8 @@
119 #include "mir/scene/surface.h"
120 #include "mir/scene/surface_creation_parameters.h"
121
122+#include <boost/throw_exception.hpp>
123+
124 namespace mf = mir::frontend;
125 namespace ms = mir::scene;
126 namespace msh = mir::shell;
127@@ -60,6 +62,9 @@
128 {
129 mir::geometry::Rectangle rect{params.top_left, params.size};
130
131+ if (!params.output_id.as_value())
132+ BOOST_THROW_EXCEPTION(std::runtime_error("An output ID must be specified"));
133+
134 display_layout->place_in_output(params.output_id, rect);
135
136 auto placed_parameters = params;
137@@ -78,6 +83,9 @@
138 surface);
139
140 surface->add_observer(session_ready_observer);
141+
142+ std::lock_guard<decltype(mutex)> lock{mutex};
143+ output_map[surface] = params.output_id;
144
145 return result;
146 }
147@@ -89,16 +97,50 @@
148 {
149 if (modifications.name.is_set())
150 surface->rename(modifications.name.value());
151+
152+ if (modifications.output_id.is_set())
153+ {
154+ auto const output_id = modifications.output_id.value();
155+
156+ mir::geometry::Rectangle rect{surface->top_left(), surface->size()};
157+
158+ if (display_layout->place_in_output(output_id, rect))
159+ {
160+ surface->move_to(rect.top_left);
161+ surface->resize(rect.size);
162+ }
163+
164+ std::lock_guard<decltype(mutex)> lock{mutex};
165+ output_map[surface] = output_id;
166+ }
167 }
168
169 void msh::SystemCompositorWindowManager::remove_surface(
170 std::shared_ptr<ms::Session> const& /*session*/,
171- std::weak_ptr<ms::Surface> const& /*surface*/)
172+ std::weak_ptr<ms::Surface> const& surface)
173 {
174+ output_map.erase(surface);
175 }
176
177 void msh::SystemCompositorWindowManager::add_display(mir::geometry::Rectangle const& /*area*/)
178 {
179+ std::lock_guard<decltype(mutex)> lock{mutex};
180+
181+ for (auto const& so : output_map)
182+ {
183+ if (auto surface = so.first.lock())
184+ {
185+ auto const output_id = so.second;
186+
187+ mir::geometry::Rectangle rect{surface->top_left(), surface->size()};
188+
189+ if (display_layout->place_in_output(output_id, rect))
190+ {
191+ surface->move_to(rect.top_left);
192+ surface->resize(rect.size);
193+ }
194+ }
195+ }
196 }
197
198 void msh::SystemCompositorWindowManager::remove_display(mir::geometry::Rectangle const& /*area*/)
199
200=== modified file 'tests/acceptance-tests/test_system_compositor_window_manager.cpp'
201--- tests/acceptance-tests/test_system_compositor_window_manager.cpp 2015-10-02 04:27:35 +0000
202+++ tests/acceptance-tests/test_system_compositor_window_manager.cpp 2015-11-12 18:48:46 +0000
203@@ -167,7 +167,7 @@
204 auto surface = client.create_surface(0);
205
206 EXPECT_FALSE(mir_surface_is_valid(surface));
207- EXPECT_THAT(mir_surface_get_error_message(surface), HasSubstr("Failed to place surface"));
208+ EXPECT_THAT(mir_surface_get_error_message(surface), HasSubstr("An output ID must be specified"));
209 }
210
211 TEST_F(SystemCompositorWindowManager, if_a_surface_posts_client_gets_focus)

Subscribers

People subscribed via source and target branches