Merge lp:~alan-griffiths/miral/rework-handling-of-surface-state-changes into lp:miral
- rework-handling-of-surface-state-changes
- Merge into trunk
Status: | Merged |
---|---|
Approved by: | Gerry Boland |
Approved revision: | 384 |
Merged at revision: | 375 |
Proposed branch: | lp:~alan-griffiths/miral/rework-handling-of-surface-state-changes |
Merge into: | lp:miral |
Diff against target: |
425 lines (+137/-81) 11 files modified
debian/libmiral1.symbols (+1/-0) include/miral/window_manager_tools.h (+2/-0) miral-shell/titlebar_window_manager.cpp (+1/-1) miral/basic_window_manager.cpp (+114/-78) miral/basic_window_manager.h (+2/-1) miral/set_window_managment_policy.cpp (+1/-1) miral/symbols.map (+1/-0) miral/window_management_trace.cpp (+8/-0) miral/window_management_trace.h (+1/-0) miral/window_manager_tools.cpp (+4/-0) miral/window_manager_tools_implementation.h (+2/-0) |
To merge this branch: | bzr merge lp:~alan-griffiths/miral/rework-handling-of-surface-state-changes |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Gerry Boland (community) | Approve | ||
Daniel d'Andrada (community) | test | Approve | |
Review via email:
|
Commit message
Rework the handling of surface state changes:
1. shells now have the option of ignoring (or adjusting) the default placement.
2. fixes issues arising when the state of the active window changes.
Description of the change
Rework the handling of surface state changes:
1. shells now have the option of ignoring (or adjusting) the default placement.
2. fixes issues arising when the state of the active window changes.
It may seem odd to provide position_
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Gerry Boland (gerboland) wrote : | # |
+ void position_
name doesn't quite match implementation, as it also impacts size, perhaps "geometry_
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Gerry Boland (gerboland) wrote : | # |
+void miral::
+ void position_
Order of the arguments now do not match. Consistency would be nice. I'm unsure why you changed the first one here.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Gerry Boland (gerboland) wrote : | # |
- policy-
- window_
- mir_surface-
- mir_surface-
- if (window_
- {
- mru_active_
-
- // Try to activate to recently active window of any application
- mru_active_
- {
- auto const w = window;
- return !(select_
- });
- }
- return;
I wasn't expecting this to vanish in this MP. Is it dead code?
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Gerry Boland (gerboland) wrote : | # |
> +void
> miral::
> const& modifications, WindowInfo const& window_info) const
>
> + void position_
> const& window_info) const override;
>
> Order of the arguments now do not match. Consistency would be nice. I'm unsure
> why you changed the first one here.
Or the consistency you're developing is an (out, const in) design?
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Alan Griffiths (alan-griffiths) wrote : | # |
> - policy-
> - window_
> - mir_surface-
> - mir_surface-
> - if (window_
> - {
> - mru_active_
> -
> - // Try to activate to recently active window of any application
> - mru_active_
> - {
> - auto const w = window;
> - return !(select_
> - });
> - }
> - return;
>
> I wasn't expecting this to vanish in this MP. Is it dead code?
Neither.
+ if (window == active_window())
+ {
+ // Try to activate to recently active window of any application
+ mru_active_
+ {
+ if (candidate == window)
+ return true;
+ auto const w = candidate;
+ return !(select_
+ });
+ }
Plus some duplicated code consolidated.
- 384. By Alan Griffiths
-
position_
for_state( ) => place_and_ size_for_ state()
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Alan Griffiths (alan-griffiths) wrote : | # |
> > +void
> > miral::
> > const& modifications, WindowInfo const& window_info) const
> >
> > + void position_
> > const& window_info) const override;
> >
> > Order of the arguments now do not match. Consistency would be nice. I'm
> unsure
> > why you changed the first one here.
>
> Or the consistency you're developing is an (out, const in) design?
Partly that, but also both functions are /about/ the first parameter, with the second parameter being secondary.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Alan Griffiths (alan-griffiths) wrote : | # |
> + void position_
> const& window_info) const;
> name doesn't quite match implementation, as it also impacts size, perhaps
> "geometry_
place_and_
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Gerry Boland (gerboland) wrote : | # |
> > + void position_
> > const& window_info) const;
> > name doesn't quite match implementation, as it also impacts size, perhaps
> > "geometry_
>
> place_and_
You use similar elsewhere, so yeah.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Gerry Boland (gerboland) : | # |
Preview Diff
1 | === modified file 'debian/libmiral1.symbols' |
2 | --- debian/libmiral1.symbols 2016-09-28 10:38:36 +0000 |
3 | +++ debian/libmiral1.symbols 2016-09-29 14:41:47 +0000 |
4 | @@ -180,6 +180,7 @@ |
5 | (c++)"miral::WindowManagerTools::invoke_under_lock(std::function<void ()> const&)@MIRAL_0.1" 0.1.0 |
6 | (c++)"miral::WindowManagerTools::modify_window(miral::WindowInfo&, miral::WindowSpecification const&)@MIRAL_0.1" 0.1.0 |
7 | (c++)"miral::WindowManagerTools::operator=(miral::WindowManagerTools const&)@MIRAL_0.1" 0.1.0 |
8 | + (c++)"miral::WindowManagerTools::place_and_size_for_state(miral::WindowSpecification&, miral::WindowInfo const&) const@MIRAL_0.2" 0.2.0 |
9 | (c++)"miral::WindowManagerTools::raise_tree(miral::Window const&)@MIRAL_0.1" 0.1.0 |
10 | (c++)"miral::WindowManagerTools::select_active_window(miral::Window const&)@MIRAL_0.1" 0.1.0 |
11 | (c++)"miral::WindowManagerTools::window_at(mir::geometry::Point) const@MIRAL_0.1" 0.1.0 |
12 | |
13 | === modified file 'include/miral/window_manager_tools.h' |
14 | --- include/miral/window_manager_tools.h 2016-09-07 20:23:20 +0000 |
15 | +++ include/miral/window_manager_tools.h 2016-09-29 14:41:47 +0000 |
16 | @@ -146,6 +146,8 @@ |
17 | /// Apply modifications to a window |
18 | void modify_window(WindowInfo& window_info, WindowSpecification const& modifications); |
19 | |
20 | + /// Set a default size and position to reflect state change |
21 | + void place_and_size_for_state(WindowSpecification& modifications, WindowInfo const& window_info) const; |
22 | /** @} */ |
23 | |
24 | /** Multi-thread support |
25 | |
26 | === modified file 'miral-shell/titlebar_window_manager.cpp' |
27 | --- miral-shell/titlebar_window_manager.cpp 2016-09-28 11:42:35 +0000 |
28 | +++ miral-shell/titlebar_window_manager.cpp 2016-09-29 14:41:47 +0000 |
29 | @@ -455,7 +455,7 @@ |
30 | WindowSpecification modifications; |
31 | |
32 | modifications.state() = (info.state() == state) ? mir_surface_state_restored : state; |
33 | - |
34 | + tools.place_and_size_for_state(modifications, info); |
35 | tools.modify_window(info, modifications); |
36 | } |
37 | } |
38 | |
39 | === modified file 'miral/basic_window_manager.cpp' |
40 | --- miral/basic_window_manager.cpp 2016-09-23 16:55:46 +0000 |
41 | +++ miral/basic_window_manager.cpp 2016-09-29 14:41:47 +0000 |
42 | @@ -143,8 +143,10 @@ |
43 | { |
44 | Locker lock{mutex, policy}; |
45 | auto& info = info_for(surface); |
46 | - validate_modification_request(info, modifications); |
47 | - policy->handle_modify_window(info, modifications); |
48 | + WindowSpecification mods{modifications}; |
49 | + validate_modification_request(mods, info); |
50 | + place_and_size_for_state(mods, info); |
51 | + policy->handle_modify_window(info, mods); |
52 | } |
53 | |
54 | void miral::BasicWindowManager::remove_surface( |
55 | @@ -321,7 +323,8 @@ |
56 | Locker lock{mutex, policy}; |
57 | auto& info = info_for(surface); |
58 | |
59 | - validate_modification_request(info, modification); |
60 | + validate_modification_request(modification, info); |
61 | + place_and_size_for_state(modification, info); |
62 | policy->handle_modify_window(info, modification); |
63 | |
64 | switch (attrib) |
65 | @@ -601,6 +604,38 @@ |
66 | if (modifications.input_shape().is_set()) |
67 | std::shared_ptr<scene::Surface>(window)->set_input_region(modifications.input_shape().value()); |
68 | |
69 | + if (modifications.state().is_set() && window_info.state() != modifications.state().value()) |
70 | + { |
71 | + switch (window_info.state()) |
72 | + { |
73 | + case mir_surface_state_restored: |
74 | + case mir_surface_state_hidden: |
75 | + window_info.restore_rect({window.top_left(), window.size()}); |
76 | + break; |
77 | + |
78 | + case mir_surface_state_vertmaximized: |
79 | + { |
80 | + auto restore_rect = window_info.restore_rect(); |
81 | + restore_rect.top_left.x = window.top_left().x; |
82 | + restore_rect.size.width = window.size().width; |
83 | + window_info.restore_rect(restore_rect); |
84 | + break; |
85 | + } |
86 | + |
87 | + case mir_surface_state_horizmaximized: |
88 | + { |
89 | + auto restore_rect = window_info.restore_rect(); |
90 | + restore_rect.top_left.y = window.top_left().y; |
91 | + restore_rect.size.height= window.size().height; |
92 | + window_info.restore_rect(restore_rect); |
93 | + break; |
94 | + } |
95 | + |
96 | + default: |
97 | + break; |
98 | + } |
99 | + } |
100 | + |
101 | Point new_pos = modifications.top_left().is_set() ? modifications.top_left().value() : window.top_left(); |
102 | |
103 | if (modifications.size().is_set()) |
104 | @@ -687,48 +722,36 @@ |
105 | move_tree(root, new_pos - root.window().top_left()); |
106 | } |
107 | |
108 | -void miral::BasicWindowManager::set_state(miral::WindowInfo& window_info, MirSurfaceState value) |
109 | +void miral::BasicWindowManager::place_and_size_for_state( |
110 | + WindowSpecification& modifications, WindowInfo const& window_info) const |
111 | { |
112 | - auto const mir_surface = std::shared_ptr<scene::Surface>(window_info.window()); |
113 | - |
114 | - switch (value) |
115 | - { |
116 | - case mir_surface_state_restored: |
117 | - case mir_surface_state_maximized: |
118 | - case mir_surface_state_vertmaximized: |
119 | - case mir_surface_state_horizmaximized: |
120 | - case mir_surface_state_fullscreen: |
121 | - case mir_surface_state_hidden: |
122 | - case mir_surface_state_minimized: |
123 | - break; |
124 | - |
125 | - default: |
126 | - mir_surface->configure(mir_surface_attrib_state, window_info.state()); |
127 | + if (!modifications.state().is_set() || modifications.state().value() == window_info.state()) |
128 | return; |
129 | - } |
130 | - |
131 | + |
132 | + auto const display_area = displays.bounding_rectangle(); |
133 | + auto const window = window_info.window(); |
134 | + |
135 | + auto restore_rect = window_info.restore_rect(); |
136 | + |
137 | + // window_info.restore_rect() was cached on last state change, update to reflect current window position |
138 | switch (window_info.state()) |
139 | { |
140 | case mir_surface_state_restored: |
141 | case mir_surface_state_hidden: |
142 | - window_info.restore_rect({window_info.window().top_left(), window_info.window().size()}); |
143 | + restore_rect = {window.top_left(), window.size()}; |
144 | break; |
145 | |
146 | case mir_surface_state_vertmaximized: |
147 | { |
148 | - auto restore_rect = window_info.restore_rect(); |
149 | - restore_rect.top_left.x = window_info.window().top_left().x; |
150 | - restore_rect.size.width = window_info.window().size().width; |
151 | - window_info.restore_rect(restore_rect); |
152 | + restore_rect.top_left.x = window.top_left().x; |
153 | + restore_rect.size.width = window.size().width; |
154 | break; |
155 | } |
156 | |
157 | case mir_surface_state_horizmaximized: |
158 | { |
159 | - auto restore_rect = window_info.restore_rect(); |
160 | - restore_rect.top_left.y = window_info.window().top_left().y; |
161 | - restore_rect.size.height= window_info.window().size().height; |
162 | - window_info.restore_rect(restore_rect); |
163 | + restore_rect.top_left.y = window.top_left().y; |
164 | + restore_rect.size.height= window.size().height; |
165 | break; |
166 | } |
167 | |
168 | @@ -736,29 +759,20 @@ |
169 | break; |
170 | } |
171 | |
172 | - if (value != mir_surface_state_fullscreen) |
173 | - { |
174 | - window_info.output_id({}); |
175 | - fullscreen_surfaces.erase(window_info.window()); |
176 | - } |
177 | - else |
178 | - { |
179 | - fullscreen_surfaces.insert(window_info.window()); |
180 | - } |
181 | + // If the shell has also set position, that overrides restore_rect default |
182 | + if (modifications.top_left().is_set()) |
183 | + restore_rect.top_left = modifications.top_left().value(); |
184 | |
185 | - if (window_info.state() == value) |
186 | - { |
187 | - return; |
188 | - } |
189 | + // If the client or shell has also set size, that overrides restore_rect default |
190 | + if (modifications.size().is_set()) |
191 | + restore_rect.size = modifications.size().value(); |
192 | |
193 | Rectangle rect; |
194 | |
195 | - auto const display_area = displays.bounding_rectangle(); |
196 | - |
197 | - switch (value) |
198 | + switch (modifications.state().value()) |
199 | { |
200 | case mir_surface_state_restored: |
201 | - rect = window_info.restore_rect(); |
202 | + rect = restore_rect; |
203 | break; |
204 | |
205 | case mir_surface_state_maximized: |
206 | @@ -766,18 +780,18 @@ |
207 | break; |
208 | |
209 | case mir_surface_state_horizmaximized: |
210 | - rect.top_left = {display_area.top_left.x, window_info.restore_rect().top_left.y}; |
211 | - rect.size = {display_area.size.width, window_info.restore_rect().size.height}; |
212 | + rect.top_left = {display_area.top_left.x, restore_rect.top_left.y}; |
213 | + rect.size = {display_area.size.width, restore_rect.size.height}; |
214 | break; |
215 | |
216 | case mir_surface_state_vertmaximized: |
217 | - rect.top_left = {window_info.restore_rect().top_left.x, display_area.top_left.y}; |
218 | - rect.size = {window_info.restore_rect().size.width, display_area.size.height}; |
219 | + rect.top_left = {restore_rect.top_left.x, display_area.top_left.y}; |
220 | + rect.size = {restore_rect.size.width, display_area.size.height}; |
221 | break; |
222 | |
223 | case mir_surface_state_fullscreen: |
224 | { |
225 | - rect = {(window_info.window().top_left()), window_info.window().size()}; |
226 | + rect = {(window.top_left()), window.size()}; |
227 | |
228 | if (window_info.has_output_id()) |
229 | { |
230 | @@ -794,35 +808,59 @@ |
231 | |
232 | case mir_surface_state_hidden: |
233 | case mir_surface_state_minimized: |
234 | - policy->advise_state_change(window_info, value); |
235 | - window_info.state(value); |
236 | - mir_surface->hide(); |
237 | - mir_surface->configure(mir_surface_attrib_state, value); |
238 | - if (window_info.window() == active_window()) |
239 | - { |
240 | - mru_active_windows.erase(window_info.window()); |
241 | - |
242 | - // Try to activate to recently active window of any application |
243 | - mru_active_windows.enumerate([&](Window& window) |
244 | - { |
245 | - auto const w = window; |
246 | - return !(select_active_window(w)); |
247 | - }); |
248 | - } |
249 | - return; |
250 | - |
251 | default: |
252 | - break; |
253 | - } |
254 | - |
255 | - place_and_size(window_info, rect.top_left, rect.size); |
256 | + return; |
257 | + } |
258 | + |
259 | + modifications.top_left() = rect.top_left; |
260 | + modifications.size() = rect.size; |
261 | +} |
262 | + |
263 | +void miral::BasicWindowManager::set_state(miral::WindowInfo& window_info, MirSurfaceState value) |
264 | +{ |
265 | + auto const window = window_info.window(); |
266 | + auto const mir_surface = std::shared_ptr<scene::Surface>(window); |
267 | + |
268 | + if (value != mir_surface_state_fullscreen) |
269 | + { |
270 | + window_info.output_id({}); |
271 | + fullscreen_surfaces.erase(window); |
272 | + } |
273 | + else |
274 | + { |
275 | + fullscreen_surfaces.insert(window); |
276 | + } |
277 | + |
278 | + if (window_info.state() == value) |
279 | + { |
280 | + return; |
281 | + } |
282 | + |
283 | policy->advise_state_change(window_info, value); |
284 | window_info.state(value); |
285 | |
286 | if (window_info.is_visible()) |
287 | + { |
288 | mir_surface->show(); |
289 | - |
290 | - mir_surface->configure(mir_surface_attrib_state, window_info.state()); |
291 | + } |
292 | + else |
293 | + { |
294 | + mir_surface->hide(); |
295 | + |
296 | + if (window == active_window()) |
297 | + { |
298 | + // Try to activate to recently active window of any application |
299 | + mru_active_windows.enumerate([&](Window& candidate) |
300 | + { |
301 | + if (candidate == window) |
302 | + return true; |
303 | + auto const w = candidate; |
304 | + return !(select_active_window(w)); |
305 | + }); |
306 | + } |
307 | + } |
308 | + |
309 | + mir_surface->configure(mir_surface_attrib_state, value); |
310 | } |
311 | |
312 | |
313 | @@ -1434,9 +1472,7 @@ |
314 | return default_result; |
315 | } |
316 | |
317 | -void miral::BasicWindowManager::validate_modification_request( |
318 | - WindowInfo const& window_info, |
319 | - WindowSpecification const& modifications) const |
320 | +void miral::BasicWindowManager::validate_modification_request(WindowSpecification const& modifications, WindowInfo const& window_info) const |
321 | { |
322 | auto target_type = window_info.type(); |
323 | |
324 | |
325 | === modified file 'miral/basic_window_manager.h' |
326 | --- miral/basic_window_manager.h 2016-09-21 13:37:35 +0000 |
327 | +++ miral/basic_window_manager.h 2016-09-29 14:41:47 +0000 |
328 | @@ -133,6 +133,7 @@ |
329 | auto info_for_window_id(std::string const& id) const -> WindowInfo& override; |
330 | |
331 | auto id_for_window(Window const& window) const -> std::string override; |
332 | + void place_and_size_for_state(WindowSpecification& modifications, WindowInfo const& window_info) const override; |
333 | |
334 | void invoke_under_lock(std::function<void()> const& callback) override; |
335 | |
336 | @@ -167,7 +168,7 @@ |
337 | |
338 | void move_tree(miral::WindowInfo& root, mir::geometry::Displacement movement); |
339 | void erase(miral::WindowInfo const& info); |
340 | - void validate_modification_request(WindowInfo const& window_info, WindowSpecification const& modifications) const; |
341 | + void validate_modification_request(WindowSpecification const& modifications, WindowInfo const& window_info) const; |
342 | void place_and_size(WindowInfo& root, Point const& new_pos, Size const& new_size); |
343 | void set_state(miral::WindowInfo& window_info, MirSurfaceState value); |
344 | }; |
345 | |
346 | === modified file 'miral/set_window_managment_policy.cpp' |
347 | --- miral/set_window_managment_policy.cpp 2016-09-21 14:43:46 +0000 |
348 | +++ miral/set_window_managment_policy.cpp 2016-09-29 14:41:47 +0000 |
349 | @@ -57,7 +57,7 @@ |
350 | { |
351 | auto trace_builder = [this](WindowManagerTools const& tools) -> std::unique_ptr<miral::WindowManagementPolicy> |
352 | { |
353 | - return std::make_unique<WindowManagementTrace>(tools, builder); |
354 | + return std::make_unique<WindowManagementTrace>(tools, builder); |
355 | }; |
356 | |
357 | return std::make_shared<BasicWindowManager>(focus_controller, display_layout, persistent_surface_store, trace_builder); |
358 | |
359 | === modified file 'miral/symbols.map' |
360 | --- miral/symbols.map 2016-09-28 10:38:36 +0000 |
361 | +++ miral/symbols.map 2016-09-29 14:41:47 +0000 |
362 | @@ -289,6 +289,7 @@ |
363 | miral::CursorTheme::CursorTheme*; |
364 | miral::CursorTheme::operator*; |
365 | miral::WindowInfo::confine_pointer*; |
366 | + miral::WindowManagerTools::place_and_size_for_state*; |
367 | miral::WindowSpecification::confine_pointer*; |
368 | typeinfo?for?miral::CursorTheme; |
369 | vtable?for?miral::CursorTheme; |
370 | |
371 | === modified file 'miral/window_management_trace.cpp' |
372 | --- miral/window_management_trace.cpp 2016-09-23 16:54:30 +0000 |
373 | +++ miral/window_management_trace.cpp 2016-09-29 14:41:47 +0000 |
374 | @@ -401,6 +401,14 @@ |
375 | return result; |
376 | } |
377 | |
378 | +void miral::WindowManagementTrace::place_and_size_for_state( |
379 | + WindowSpecification& modifications, WindowInfo const& window_info) const |
380 | +{ |
381 | + log_input(); |
382 | + mir::log_info("%s modifications=%s window_info=%s", __func__, dump_of(modifications).c_str(), dump_of(window_info).c_str()); |
383 | + wrapped.place_and_size_for_state(modifications, window_info); |
384 | +} |
385 | + |
386 | void miral::WindowManagementTrace::drag_active_window(mir::geometry::Displacement movement) |
387 | { |
388 | log_input(); |
389 | |
390 | === modified file 'miral/window_management_trace.h' |
391 | --- miral/window_management_trace.h 2016-09-23 10:28:25 +0000 |
392 | +++ miral/window_management_trace.h 2016-09-29 14:41:47 +0000 |
393 | @@ -56,6 +56,7 @@ |
394 | virtual auto active_display() -> mir::geometry::Rectangle const override; |
395 | virtual auto info_for_window_id(std::string const& id) const -> WindowInfo& override; |
396 | virtual auto id_for_window(Window const& window) const -> std::string override; |
397 | + virtual void place_and_size_for_state(WindowSpecification& modifications, WindowInfo const& window_info) const override; |
398 | |
399 | virtual void drag_active_window(mir::geometry::Displacement movement) override; |
400 | |
401 | |
402 | === modified file 'miral/window_manager_tools.cpp' |
403 | --- miral/window_manager_tools.cpp 2016-09-07 20:23:20 +0000 |
404 | +++ miral/window_manager_tools.cpp 2016-09-29 14:41:47 +0000 |
405 | @@ -85,3 +85,7 @@ |
406 | |
407 | void miral::WindowManagerTools::invoke_under_lock(std::function<void()> const& callback) |
408 | { tools->invoke_under_lock(callback); } |
409 | + |
410 | +void miral::WindowManagerTools::place_and_size_for_state( |
411 | + WindowSpecification& modifications, WindowInfo const& window_info) const |
412 | +{ tools->place_and_size_for_state(modifications, window_info); } |
413 | |
414 | === modified file 'miral/window_manager_tools_implementation.h' |
415 | --- miral/window_manager_tools_implementation.h 2016-09-21 13:37:35 +0000 |
416 | +++ miral/window_manager_tools_implementation.h 2016-09-29 14:41:47 +0000 |
417 | @@ -65,6 +65,8 @@ |
418 | virtual void modify_window(WindowInfo& window_info, WindowSpecification const& modifications) = 0; |
419 | virtual auto info_for_window_id(std::string const& id) const -> WindowInfo& = 0; |
420 | virtual auto id_for_window(Window const& window) const -> std::string = 0; |
421 | + virtual void place_and_size_for_state(WindowSpecification& modifications, WindowInfo const& window_info) const= 0; |
422 | + |
423 | /** @} */ |
424 | |
425 | /** @name Multi-thread support |
Fixes those two bugs I reported, so I'm happy with it.
No opinion on the code itself, just stating that it fixed those bugs.