Mir

Merge lp:~brandontschaefer/mir/update-confined-region into lp:mir

Proposed by Brandon Schaefer on 2016-07-20
Status: Merged
Approved by: Alan Griffiths on 2016-07-25
Approved revision: 3604
Merged at revision: 3611
Proposed branch: lp:~brandontschaefer/mir/update-confined-region
Merge into: lp:mir
Diff against target: 330 lines (+112/-11)
11 files modified
include/server/mir/scene/null_surface_observer.h (+1/-0)
include/server/mir/scene/surface_observer.h (+2/-0)
include/server/mir/shell/abstract_shell.h (+2/-0)
src/include/server/mir/scene/surface_observers.h (+1/-0)
src/server/scene/basic_surface.cpp (+21/-0)
src/server/scene/legacy_surface_change_notification.cpp (+4/-0)
src/server/scene/legacy_surface_change_notification.h (+1/-0)
src/server/scene/null_surface_observer.cpp (+1/-0)
src/server/shell/abstract_shell.cpp (+27/-1)
src/server/symbols.map (+1/-0)
tests/acceptance-tests/test_confined_pointer.cpp (+51/-10)
To merge this branch: bzr merge lp:~brandontschaefer/mir/update-confined-region
Reviewer Review Type Date Requested Status
Alan Griffiths 2016-07-20 Approve on 2016-07-25
Mir CI Bot continuous-integration Approve on 2016-07-23
Review via email: mp+300557@code.launchpad.net

Commit message

Update the confine region if we are modifying the state/width/height

Description of the change

Update the confine region if we are modifying the state/width/height

To post a comment you must log in.
Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:3603
https://mir-jenkins.ubuntu.com/job/mir-ci/1326/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/1564
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/1617
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/1608
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/1608
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/1608
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/1579
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/1579/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/1579
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/1579/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/1579
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/1579/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/1579
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/1579/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/1579
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/1579/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/1326/rebuild

review: Approve (continuous-integration)
Alan Griffiths (alan-griffiths) wrote :

This isn't a sufficient fix. For example:

  o in mir_demo_server fullscreen surfaces are resized when display resolutions are changed.
  o in mir_demo_server --window-manager tiling surfaces are resized when other sessions start/stop

I suggest adding an observer to the surface that updates the seat when resized_to() is called.

review: Needs Fixing
Alan Griffiths (alan-griffiths) wrote :

> This isn't a sufficient fix. For example:
>

Or the surface is resized by user interaction. Drag Alt+Middle, Ctrl+F11 etc.

Brandon Schaefer (brandontschaefer) wrote :

I agree, attempting to do a surface observer in the abstract shell has proven to need a bit more thought. Its strange having to store a unoredered mapping of the observers for a surface id. As well as passing the session/surface id/seat to the observer so it can check if the surface is confined before updating that seat.

My other thought is to possibly add a new callback to the observer update_confinement_region(geom::Rectangle const&) ... or Rectangles (as the input bounds).

More thinking to come here (would like it a bit cleaner then the current branch im messing with WIP until i figure a better way)

Alan Griffiths (alan-griffiths) wrote :

> I agree, attempting to do a surface observer in the abstract shell has proven
> to need a bit more thought. Its strange having to store a unoredered mapping
> of the observers for a surface id. As well as passing the session/surface
> id/seat to the observer so it can check if the surface is confined before
> updating that seat.

That sounds like hard work.

Can't you just have have one observer that is pointed at whichever surface is confining the cursor?

Brandon Schaefer (brandontschaefer) wrote :

Like usual your way makes more sense and requires less work. Best of both worlds!

Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:3604
https://mir-jenkins.ubuntu.com/job/mir-ci/1337/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/1590
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/1643
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/1634
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/1634
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/1634
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/1605
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/1605/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/1605
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/1605/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/1605
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/1605/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/1605
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/1605/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/1605
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/1605/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/1337/rebuild

review: Approve (continuous-integration)
Alan Griffiths (alan-griffiths) wrote :

Does what it claims.

But there still seems to be an issue with pointer confinement: if the cursor isn't over the active surface when confined it remains visible until it moves.

review: Approve
Brandon Schaefer (brandontschaefer) wrote :

The issue with that ... is it doesnt contain anything until a move happens. (ie. warp) I suppose I could attempt to make sure the demo hides the cursor when we enter confinement. Though it didnt seem to like doing that? ie. Swapping the surface after setting the cursor to Null. I can dig into that some more.

Alan Griffiths (alan-griffiths) wrote :

> The issue with that ... is it doesnt contain anything until a move happens.
> (ie. warp) I suppose I could attempt to make sure the demo hides the cursor
> when we enter confinement. Though it didnt seem to like doing that? ie.
> Swapping the surface after setting the cursor to Null. I can dig into that
> some more.

The client only has "control" of the cursor when it is over the surface. This is something that would need to be addressed server-side.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'include/server/mir/scene/null_surface_observer.h'
2--- include/server/mir/scene/null_surface_observer.h 2016-01-29 08:18:22 +0000
3+++ include/server/mir/scene/null_surface_observer.h 2016-07-23 06:00:22 +0000
4@@ -45,6 +45,7 @@
5 void keymap_changed(MirInputDeviceId id, std::string const& model, std::string const& layout, std::string const& variant, std::string const& options) override;
6 void renamed(char const* name) override;
7 void cursor_image_removed() override;
8+ void confinement_region_updated(geometry::Rectangle const& rect) override;
9
10 protected:
11 NullSurfaceObserver(NullSurfaceObserver const&) = delete;
12
13=== modified file 'include/server/mir/scene/surface_observer.h'
14--- include/server/mir/scene/surface_observer.h 2016-01-29 08:18:22 +0000
15+++ include/server/mir/scene/surface_observer.h 2016-07-23 06:00:22 +0000
16@@ -23,6 +23,7 @@
17 #include "mir_toolkit/events/event.h"
18
19 #include "mir/input/input_reception_mode.h"
20+#include "mir/geometry/rectangle.h"
21
22 #include <glm/glm.hpp>
23 #include <string>
24@@ -59,6 +60,7 @@
25 std::string const& variant, std::string const& options) = 0;
26 virtual void renamed(char const* name) = 0;
27 virtual void cursor_image_removed() = 0;
28+ virtual void confinement_region_updated(geometry::Rectangle const& rect) = 0;
29
30 protected:
31 SurfaceObserver() = default;
32
33=== modified file 'include/server/mir/shell/abstract_shell.h'
34--- include/server/mir/shell/abstract_shell.h 2016-07-01 20:32:34 +0000
35+++ include/server/mir/shell/abstract_shell.h 2016-07-23 06:00:22 +0000
36@@ -21,6 +21,7 @@
37
38 #include "mir/shell/shell.h"
39 #include "mir/shell/window_manager_builder.h"
40+#include "mir/scene/surface_observer.h"
41
42 #include <mutex>
43
44@@ -135,6 +136,7 @@
45 std::mutex mutable focus_mutex;
46 std::weak_ptr<scene::Surface> focus_surface;
47 std::weak_ptr<scene::Session> focus_session;
48+ std::shared_ptr<scene::SurfaceObserver> const focus_surface_observer;
49
50 void set_focus_to_locked(
51 std::unique_lock<std::mutex> const& lock,
52
53=== modified file 'src/include/server/mir/scene/surface_observers.h'
54--- src/include/server/mir/scene/surface_observers.h 2016-05-03 06:55:25 +0000
55+++ src/include/server/mir/scene/surface_observers.h 2016-07-23 06:00:22 +0000
56@@ -49,6 +49,7 @@
57 std::string const& variant, std::string const& options) override;
58 void renamed(char const*) override;
59 void cursor_image_removed() override;
60+ void confinement_region_updated(geometry::Rectangle const& rect) override;
61 };
62
63 }
64
65=== modified file 'src/server/scene/basic_surface.cpp'
66--- src/server/scene/basic_surface.cpp 2016-06-27 20:44:49 +0000
67+++ src/server/scene/basic_surface.cpp 2016-07-23 06:00:22 +0000
68@@ -131,6 +131,12 @@
69 { observer->cursor_image_removed(); });
70 }
71
72+void ms::SurfaceObservers::confinement_region_updated(geom::Rectangle const& rect)
73+{
74+ for_each([rect](std::shared_ptr<SurfaceObserver> const& observer)
75+ { observer->confinement_region_updated(rect); });
76+}
77+
78 struct ms::CursorStreamImageAdapter
79 {
80 CursorStreamImageAdapter(ms::BasicSurface &surface)
81@@ -282,6 +288,11 @@
82 surface_rect.top_left = top_left;
83 }
84 observers.moved_to(top_left);
85+
86+ if (confine_pointer_state_ == mir_pointer_confined_to_surface)
87+ {
88+ observers.confinement_region_updated(surface_rect);
89+ }
90 }
91
92 float ms::BasicSurface::alpha() const
93@@ -364,6 +375,11 @@
94 surface_rect.size = new_size;
95 }
96 observers.resized_to(new_size);
97+
98+ if (confine_pointer_state_ == mir_pointer_confined_to_surface)
99+ {
100+ observers.confinement_region_updated(surface_rect);
101+ }
102 }
103
104 geom::Point ms::BasicSurface::top_left() const
105@@ -895,6 +911,11 @@
106 });
107 }
108 observers.moved_to(surface_rect.top_left);
109+
110+ if (confine_pointer_state_ == mir_pointer_confined_to_surface)
111+ {
112+ observers.confinement_region_updated(surface_rect);
113+ }
114 }
115
116 mg::RenderableList ms::BasicSurface::generate_renderables(mc::CompositorID id) const
117
118=== modified file 'src/server/scene/legacy_surface_change_notification.cpp'
119--- src/server/scene/legacy_surface_change_notification.cpp 2016-01-29 08:18:22 +0000
120+++ src/server/scene/legacy_surface_change_notification.cpp 2016-07-23 06:00:22 +0000
121@@ -100,3 +100,7 @@
122 {
123 notify_scene_change();
124 }
125+
126+void ms::LegacySurfaceChangeNotification::confinement_region_updated(geometry::Rectangle const&)
127+{
128+}
129
130=== modified file 'src/server/scene/legacy_surface_change_notification.h'
131--- src/server/scene/legacy_surface_change_notification.h 2016-01-29 08:18:22 +0000
132+++ src/server/scene/legacy_surface_change_notification.h 2016-07-23 06:00:22 +0000
133@@ -49,6 +49,7 @@
134 std::string const& variant, std::string const& options) override;
135 void renamed(char const*) override;
136 void cursor_image_removed() override;
137+ void confinement_region_updated(geometry::Rectangle const& rect) override;
138
139 private:
140 std::function<void()> const notify_scene_change;
141
142=== modified file 'src/server/scene/null_surface_observer.cpp'
143--- src/server/scene/null_surface_observer.cpp 2016-01-29 08:18:22 +0000
144+++ src/server/scene/null_surface_observer.cpp 2016-07-23 06:00:22 +0000
145@@ -39,3 +39,4 @@
146 }
147 void ms::NullSurfaceObserver::renamed(char const*) {}
148 void ms::NullSurfaceObserver::cursor_image_removed() {}
149+void ms::NullSurfaceObserver::confinement_region_updated(geometry::Rectangle const& /*rect*/) {}
150
151=== modified file 'src/server/shell/abstract_shell.cpp'
152--- src/server/shell/abstract_shell.cpp 2016-07-01 20:32:34 +0000
153+++ src/server/shell/abstract_shell.cpp 2016-07-23 06:00:22 +0000
154@@ -24,6 +24,7 @@
155 #include "mir/shell/window_manager.h"
156 #include "mir/scene/prompt_session.h"
157 #include "mir/scene/prompt_session_manager.h"
158+#include "mir/scene/null_surface_observer.h"
159 #include "mir/scene/session_coordinator.h"
160 #include "mir/scene/session.h"
161 #include "mir/scene/surface.h"
162@@ -33,6 +34,26 @@
163 namespace ms = mir::scene;
164 namespace mi = mir::input;
165 namespace msh = mir::shell;
166+namespace geom = mir::geometry;
167+
168+namespace
169+{
170+
171+struct UpdateConfinementOnSurfaceChanges : ms::NullSurfaceObserver
172+{
173+ UpdateConfinementOnSurfaceChanges(std::shared_ptr<mi::Seat> seat) :
174+ seat(seat)
175+ {
176+ }
177+
178+ void confinement_region_updated(geom::Rectangle const& rect)
179+ {
180+ seat->set_confinement_regions({rect});
181+ }
182+
183+ std::shared_ptr<mi::Seat> seat;
184+};
185+}
186
187 msh::AbstractShell::AbstractShell(
188 std::shared_ptr<InputTargeter> const& input_targeter,
189@@ -48,7 +69,8 @@
190 prompt_session_manager(prompt_session_manager),
191 window_manager(wm_builder(this)),
192 seat(seat),
193- report(report)
194+ report(report),
195+ focus_surface_observer(std::make_shared<UpdateConfinementOnSurfaceChanges>(seat))
196 {
197 }
198
199@@ -243,7 +265,10 @@
200 seat->reset_confinement_regions();
201
202 if (current_focus)
203+ {
204 current_focus->configure(mir_surface_attrib_focus, mir_surface_unfocused);
205+ current_focus->remove_observer(focus_surface_observer);
206+ }
207
208 if (surface)
209 {
210@@ -256,6 +281,7 @@
211 input_targeter->set_focus(surface);
212 surface->consume(seat->create_device_state().get());
213 surface->configure(mir_surface_attrib_focus, mir_surface_focused);
214+ surface->add_observer(focus_surface_observer);
215 }
216 else
217 {
218
219=== modified file 'src/server/symbols.map'
220--- src/server/symbols.map 2016-07-20 15:58:59 +0000
221+++ src/server/symbols.map 2016-07-23 06:00:22 +0000
222@@ -116,6 +116,7 @@
223 mir::scene::NullSurfaceObserver::renamed*;
224 mir::scene::NullSurfaceObserver::resized_to*;
225 mir::scene::NullSurfaceObserver::transformation_set_to*;
226+ mir::scene::NullSurfaceObserver::confinement_region_updated*;
227 mir::scene::Observer::?Observer*;
228 mir::scene::Observer::Observer*;
229 mir::scene::Observer::operator*;
230
231=== modified file 'tests/acceptance-tests/test_confined_pointer.cpp'
232--- tests/acceptance-tests/test_confined_pointer.cpp 2016-06-21 21:31:05 +0000
233+++ tests/acceptance-tests/test_confined_pointer.cpp 2016-07-23 06:00:22 +0000
234@@ -53,8 +53,6 @@
235
236 struct Client
237 {
238- MirSurface* surface{nullptr};
239-
240 MOCK_METHOD1(handle_input, void(MirEvent const*));
241
242 Client(std::string const& con, std::string const& name)
243@@ -90,6 +88,22 @@
244 }
245 }
246
247+ void resize(int width, int height)
248+ {
249+ auto spec = mir_connection_create_spec_for_changes(connection);
250+ mir_surface_spec_set_width (spec, width);
251+ mir_surface_spec_set_height(spec, height);
252+
253+ mir_surface_apply_spec(surface, spec);
254+ mir_surface_spec_release(spec);
255+
256+ received_resize.wait_for(1s);
257+ if (!received_resize.raised())
258+ {
259+ BOOST_THROW_EXCEPTION(std::runtime_error("Timeout waiting for surface to become resized"));
260+ }
261+ }
262+
263 void handle_surface_event(MirSurfaceEvent const* event)
264 {
265 auto const attrib = mir_surface_event_get_attribute(event);
266@@ -111,15 +125,19 @@
267 {
268 auto const client = static_cast<Client*>(context);
269 auto type = mir_event_get_type(ev);
270- if (type == mir_event_type_surface)
271- {
272- auto surface_event = mir_event_get_surface_event(ev);
273- client->handle_surface_event(surface_event);
274-
275- }
276- if (type == mir_event_type_input)
277- {
278+ switch (type)
279+ {
280+ case mir_event_type_surface:
281+ client->handle_surface_event(mir_event_get_surface_event(ev));
282+ break;
283+ case mir_event_type_input:
284 client->handle_input(ev);
285+ break;
286+ case mir_event_type_resize:
287+ client->received_resize.raise();
288+ break;
289+ default:
290+ break;
291 }
292 }
293 ~Client()
294@@ -132,9 +150,11 @@
295 mir_connection_release(connection);
296 }
297
298+ MirSurface* surface{nullptr};
299 MirConnection* connection;
300 mir::test::Signal ready_to_accept_events;
301 mir::test::Signal all_events_received;
302+ mir::test::Signal received_resize;
303 bool exposed = false;
304 bool focused = false;
305 };
306@@ -212,3 +232,24 @@
307
308 client.all_events_received.wait_for(10s);
309 }
310+
311+TEST_F(PointerConfinement, test_we_update_our_confined_region_on_a_resize)
312+{
313+ using namespace ::testing;
314+
315+ positions[first] = geom::Rectangle{{0,0}, {surface_width, surface_height}};
316+ Client client(new_connection(), first);
317+
318+ client.resize(surface_width + 100, surface_height);
319+
320+ InSequence seq;
321+ EXPECT_CALL(client, handle_input(mt::PointerEnterEvent()));
322+ EXPECT_CALL(client, handle_input(mt::PointerEventWithPosition(surface_width + 100 - 1, 0)));
323+ EXPECT_CALL(client, handle_input(AllOf(mt::PointerEventWithPosition(surface_width + 100 - 1, 0), mt::PointerEventWithDiff(10, 0))))
324+ .WillOnce(mt::WakeUp(&client.all_events_received));
325+
326+ fake_mouse->emit_event(mis::a_pointer_event().with_movement(surface_width + 101, 0));
327+ fake_mouse->emit_event(mis::a_pointer_event().with_movement(10, 0));
328+
329+ client.all_events_received.wait_for(10s);
330+}

Subscribers

People subscribed via source and target branches