Mir

Merge lp:~vanvugt/mir/clarify-surface-size-pos into lp:mir

  1. clarify-surface-size-pos
  2. Merge into development-branch
Proposed by Daniel van Vugt
Status: Merged
Approved by: Robert Carr
Approved revision: no longer in the source branch.
Merged at revision: 1613
Proposed branch: lp:~vanvugt/mir/clarify-surface-size-pos
Merge into: lp:mir
Diff against target: 343 lines (+85/-43)
14 files modified
include/server/mir/frontend/surface.h (+2/-1)
include/server/mir/input/surface.h (+3/-5)
include/server/mir/scene/surface.h (+9/-2)
include/test/mir_test_doubles/mock_frontend_surface.h (+1/-1)
include/test/mir_test_doubles/mock_input_surface.h (+4/-9)
src/server/frontend/session_mediator.cpp (+3/-2)
src/server/frontend/surface.cpp (+1/-1)
src/server/input/android/android_input_window_handle.cpp (+6/-7)
src/server/scene/basic_surface.cpp (+16/-1)
src/server/scene/basic_surface.h (+3/-1)
tests/unit-tests/frontend/test_session_mediator.cpp (+2/-2)
tests/unit-tests/input/android/test_android_input_target_enumerator.cpp (+2/-3)
tests/unit-tests/input/android/test_android_input_window_handle.cpp (+4/-6)
tests/unit-tests/scene/test_basic_surface.cpp (+29/-2)
To merge this branch: bzr merge lp:~vanvugt/mir/clarify-surface-size-pos
Reviewer Review Type Date Requested Status
Robert Carr (community) Approve
Alexandros Frantzis (community) Approve
Alberto Aguirre (community) Approve
PS Jenkins bot (community) continuous-integration Approve
Alan Griffiths Approve
Review via email: mp+218578@code.launchpad.net

Commit message

Clarify size/position interfaces in the Surface classes to be less ambiguous
when it comes to more advanced (future) behaviour such as window frames
and decoration.

This initial work just clarifies the interfaces by renaming functions. The
related behavioural changes will come later.

The new naming scheme is:
   size(), top_left() - Describes the whole surface (window) including
                        frame/titlebar (if any).
   client_size() - Describes the size of the client rectangle within the
                        window (note position is not implemented yet).
   input_*() - Input coordinate testing.

To post a comment you must log in.
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

LGTM

(Have you stopped setting the description for a reason?)

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:1611
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1540/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/77
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/78
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/78
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/59
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/59/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/57
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/57/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/150
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/150/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1347
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/6874

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1540/rebuild

review: Approve (continuous-integration)
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Alan - I haven't used the Description field for most of the proposals I've done over the past 2.5 years. You only just noticed :)

My reasoning is most useful information should appear in the commit message (which it does) for permanent record. Only extended information which should explicitly not appear in the commit message need go in the Description.

Revision history for this message
Alberto Aguirre (albaguirre) wrote :

Looks good.

review: Approve
Revision history for this message
Alexandros Frantzis (afrantzis) wrote :

Nit:

196 + std::unique_lock<std::mutex> lk(guard);

std::lock_guard<>

review: Approve
Revision history for this message
Robert Carr (robertcarr) wrote :

I am unconvinced we will use this approach for decorations but this seems fine.

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/frontend/surface.h'
2--- include/server/mir/frontend/surface.h 2014-04-15 05:31:19 +0000
3+++ include/server/mir/frontend/surface.h 2014-05-07 09:58:29 +0000
4@@ -51,7 +51,8 @@
5
6 virtual ~Surface() {}
7
8- virtual geometry::Size size() const = 0;
9+ /// Size of the client area of the surface (excluding any decorations)
10+ virtual geometry::Size client_size() const = 0;
11 virtual MirPixelFormat pixel_format() const = 0;
12
13 virtual void swap_buffers(graphics::Buffer* old_buffer, std::function<void(graphics::Buffer* new_buffer)> complete) = 0;
14
15=== modified file 'include/server/mir/input/surface.h'
16--- include/server/mir/input/surface.h 2014-04-24 08:42:12 +0000
17+++ include/server/mir/input/surface.h 2014-05-07 09:58:29 +0000
18@@ -20,7 +20,7 @@
19 #define MIR_INPUT_SURFACE_H_
20
21 #include "mir/geometry/size.h"
22-#include "mir/geometry/point.h"
23+#include "mir/geometry/rectangle.h"
24
25 #include <string>
26 #include <memory>
27@@ -35,10 +35,8 @@
28 {
29 public:
30 virtual std::string name() const = 0;
31- virtual geometry::Point top_left() const = 0;
32- virtual geometry::Size size() const = 0;
33- virtual bool contains(geometry::Point const& point) const = 0;
34-
35+ virtual geometry::Rectangle input_bounds() const = 0;
36+ virtual bool input_area_contains(geometry::Point const& point) const = 0;
37 virtual std::shared_ptr<input::InputChannel> input_channel() const = 0;
38
39 protected:
40
41=== modified file 'include/server/mir/scene/surface.h'
42--- include/server/mir/scene/surface.h 2014-05-05 03:36:45 +0000
43+++ include/server/mir/scene/surface.h 2014-05-07 09:58:29 +0000
44@@ -43,11 +43,18 @@
45 {
46 public:
47 // resolve ambiguous member function names
48+
49 std::string name() const override = 0;
50- geometry::Size size() const override = 0;
51- geometry::Point top_left() const override = 0;
52+ geometry::Size client_size() const override = 0;
53+ geometry::Rectangle input_bounds() const override = 0;
54
55 // member functions that don't exist in base classes
56+
57+ /// Top-left corner (of the window frame if present)
58+ virtual geometry::Point top_left() const = 0;
59+ /// Size of the surface including window frame (if any)
60+ virtual geometry::Size size() const = 0;
61+
62 virtual std::unique_ptr<graphics::Renderable> compositor_snapshot(void const* compositor_id) const = 0;
63
64 virtual float alpha() const = 0; //only used in examples/
65
66=== modified file 'include/test/mir_test_doubles/mock_frontend_surface.h'
67--- include/test/mir_test_doubles/mock_frontend_surface.h 2014-03-06 06:05:17 +0000
68+++ include/test/mir_test_doubles/mock_frontend_surface.h 2014-05-07 09:58:29 +0000
69@@ -41,7 +41,7 @@
70 MOCK_METHOD0(force_requests_to_complete, void());
71 MOCK_METHOD2(swap_buffers, void(graphics::Buffer*, std::function<void(graphics::Buffer*)> complete));
72
73- MOCK_CONST_METHOD0(size, geometry::Size());
74+ MOCK_CONST_METHOD0(client_size, geometry::Size());
75 MOCK_CONST_METHOD0(pixel_format, MirPixelFormat());
76
77 MOCK_CONST_METHOD0(supports_input, bool());
78
79=== modified file 'include/test/mir_test_doubles/mock_input_surface.h'
80--- include/test/mir_test_doubles/mock_input_surface.h 2014-04-24 08:42:12 +0000
81+++ include/test/mir_test_doubles/mock_input_surface.h 2014-05-07 09:58:29 +0000
82@@ -35,12 +35,8 @@
83 MockInputSurface()
84 {
85 using namespace ::testing;
86- ON_CALL(*this, top_left())
87- .WillByDefault(
88- Return(geometry::Point{}));
89- ON_CALL(*this, size())
90- .WillByDefault(
91- Return(geometry::Size{}));
92+ ON_CALL(*this, input_bounds())
93+ .WillByDefault(Return(geometry::Rectangle()));
94 static std::string n;
95 ON_CALL(*this, name())
96 .WillByDefault(Return(n));
97@@ -49,10 +45,9 @@
98 .WillByDefault(Return(c));
99 }
100 ~MockInputSurface() noexcept {}
101- MOCK_CONST_METHOD0(top_left, geometry::Point());
102- MOCK_CONST_METHOD0(size, geometry::Size());
103 MOCK_CONST_METHOD0(name, std::string());
104- MOCK_CONST_METHOD1(contains, bool(geometry::Point const&));
105+ MOCK_CONST_METHOD0(input_bounds, geometry::Rectangle());
106+ MOCK_CONST_METHOD1(input_area_contains, bool(geometry::Point const&));
107 MOCK_CONST_METHOD0(input_channel, std::shared_ptr<input::InputChannel>());
108 };
109
110
111=== modified file 'src/server/frontend/session_mediator.cpp'
112--- src/server/frontend/session_mediator.cpp 2014-04-15 05:31:19 +0000
113+++ src/server/frontend/session_mediator.cpp 2014-05-07 09:58:29 +0000
114@@ -169,9 +169,10 @@
115 .with_output_id(graphics::DisplayConfigurationOutputId(request->output_id())));
116
117 auto surface = session->get_surface(surf_id);
118+ auto const& client_size = surface->client_size();
119 response->mutable_id()->set_value(surf_id.as_value());
120- response->set_width(surface->size().width.as_uint32_t());
121- response->set_height(surface->size().height.as_uint32_t());
122+ response->set_width(client_size.width.as_uint32_t());
123+ response->set_height(client_size.height.as_uint32_t());
124 response->set_pixel_format((int)surface->pixel_format());
125 response->set_buffer_usage(request->buffer_usage());
126
127
128=== modified file 'src/server/frontend/surface.cpp'
129--- src/server/frontend/surface.cpp 2014-03-06 06:05:17 +0000
130+++ src/server/frontend/surface.cpp 2014-05-07 09:58:29 +0000
131@@ -45,7 +45,7 @@
132 {
133 surface->swap_buffers_blocking(buffer);
134 }
135- virtual mir::geometry::Size size() const { return surface->size(); }
136+ virtual mir::geometry::Size size() const { return surface->client_size(); }
137 virtual MirPixelFormat pixel_format() const { return surface->pixel_format(); }
138
139 std::shared_ptr<Surface> const surface;
140
141=== modified file 'src/server/input/android/android_input_window_handle.cpp'
142--- src/server/input/android/android_input_window_handle.cpp 2014-03-06 06:05:17 +0000
143+++ src/server/input/android/android_input_window_handle.cpp 2014-05-07 09:58:29 +0000
144@@ -41,7 +41,7 @@
145
146 bool touchableRegionContainsPoint(int32_t x, int32_t y) const override
147 {
148- return surface->contains(geom::Point{x, y});
149+ return surface->input_area_contains({x, y});
150 }
151
152 std::shared_ptr<mi::Surface> const surface;
153@@ -69,13 +69,12 @@
154 input_channel->server_fd());
155 }
156
157- auto surface_size = surface->size();
158- auto surface_position = surface->top_left();
159+ auto const& bounds = surface->input_bounds();
160
161- mInfo->frameLeft = surface_position.x.as_uint32_t();
162- mInfo->frameTop = surface_position.y.as_uint32_t();
163- mInfo->frameRight = mInfo->frameLeft + surface_size.width.as_uint32_t();
164- mInfo->frameBottom = mInfo->frameTop + surface_size.height.as_uint32_t();
165+ mInfo->frameLeft = bounds.top_left.x.as_uint32_t();
166+ mInfo->frameTop = bounds.top_left.y.as_uint32_t();
167+ mInfo->frameRight = mInfo->frameLeft + bounds.size.width.as_uint32_t();
168+ mInfo->frameBottom = mInfo->frameTop + bounds.size.height.as_uint32_t();
169
170 mInfo->touchableRegionLeft = mInfo->frameLeft;
171 mInfo->touchableRegionTop = mInfo->frameTop;
172
173=== modified file 'src/server/scene/basic_surface.cpp'
174--- src/server/scene/basic_surface.cpp 2014-05-05 03:36:45 +0000
175+++ src/server/scene/basic_surface.cpp 2014-05-07 09:58:29 +0000
176@@ -186,6 +186,12 @@
177 return surface_rect.size;
178 }
179
180+mir::geometry::Size ms::BasicSurface::client_size() const
181+{
182+ // TODO: In future when decorated, client_size() would be smaller than size
183+ return size();
184+}
185+
186 MirPixelFormat ms::BasicSurface::pixel_format() const
187 {
188 return surface_buffer_stream->get_stream_pixel_format();
189@@ -273,7 +279,16 @@
190 return surface_rect.top_left;
191 }
192
193-bool ms::BasicSurface::contains(geom::Point const& point) const
194+geom::Rectangle ms::BasicSurface::input_bounds() const
195+{
196+ std::unique_lock<std::mutex> lk(guard);
197+
198+ // This is historically unchanged, but if you look at input_area_contains()
199+ // it seems a bit inconsistent...
200+ return surface_rect;
201+}
202+
203+bool ms::BasicSurface::input_area_contains(geom::Point const& point) const
204 {
205 std::unique_lock<std::mutex> lock(guard);
206 if (hidden)
207
208=== modified file 'src/server/scene/basic_surface.h'
209--- src/server/scene/basic_surface.h 2014-05-05 03:36:45 +0000
210+++ src/server/scene/basic_surface.h 2014-05-07 09:58:29 +0000
211@@ -94,6 +94,7 @@
212 void set_hidden(bool is_hidden);
213
214 geometry::Size size() const override;
215+ geometry::Size client_size() const override;
216
217 MirPixelFormat pixel_format() const;
218
219@@ -112,7 +113,8 @@
220
221 void resize(geometry::Size const& size) override;
222 geometry::Point top_left() const override;
223- bool contains(geometry::Point const& point) const override;
224+ geometry::Rectangle input_bounds() const override;
225+ bool input_area_contains(geometry::Point const& point) const override;
226 void set_alpha(float alpha) override;
227 void set_transformation(glm::mat4 const&) override;
228
229
230=== modified file 'tests/unit-tests/frontend/test_session_mediator.cpp'
231--- tests/unit-tests/frontend/test_session_mediator.cpp 2014-05-01 07:13:02 +0000
232+++ tests/unit-tests/frontend/test_session_mediator.cpp 2014-05-07 09:58:29 +0000
233@@ -101,7 +101,7 @@
234 mock_surfaces[mf::SurfaceId{1}] = mock_surface;
235 mock_buffer = std::make_shared<NiceMock<mtd::MockBuffer>>(geom::Size(), geom::Stride(), MirPixelFormat());
236
237- EXPECT_CALL(*mock_surface, size()).Times(AnyNumber()).WillRepeatedly(Return(geom::Size()));
238+ EXPECT_CALL(*mock_surface, client_size()).Times(AnyNumber()).WillRepeatedly(Return(geom::Size()));
239 EXPECT_CALL(*mock_surface, pixel_format()).Times(AnyNumber()).WillRepeatedly(Return(MirPixelFormat()));
240 EXPECT_CALL(*mock_surface, swap_buffers(_, _)).Times(AnyNumber())
241 .WillRepeatedly(InvokeArgument<1>(mock_buffer.get()));
242@@ -122,7 +122,7 @@
243 if (last_surface_id != 1) {
244 mock_surfaces[id] = std::make_shared<mtd::MockFrontendSurface>();
245
246- EXPECT_CALL(*mock_surfaces[id], size()).Times(AnyNumber()).WillRepeatedly(Return(geom::Size()));
247+ EXPECT_CALL(*mock_surfaces[id], client_size()).Times(AnyNumber()).WillRepeatedly(Return(geom::Size()));
248 EXPECT_CALL(*mock_surfaces[id], pixel_format()).Times(AnyNumber()).WillRepeatedly(Return(MirPixelFormat()));
249 EXPECT_CALL(*mock_surfaces[id], swap_buffers(_, _)).Times(AnyNumber())
250 .WillRepeatedly(InvokeArgument<1>(mock_buffer.get()));
251
252=== modified file 'tests/unit-tests/input/android/test_android_input_target_enumerator.cpp'
253--- tests/unit-tests/input/android/test_android_input_target_enumerator.cpp 2014-04-24 08:42:12 +0000
254+++ tests/unit-tests/input/android/test_android_input_target_enumerator.cpp 2014-05-07 09:58:29 +0000
255@@ -74,9 +74,8 @@
256 }
257
258 std::string name() const { return {}; }
259- geom::Point top_left() const { return {}; }
260- geom::Size size() const { return {}; }
261- bool contains(geom::Point const&) const { return false; }
262+ geom::Rectangle input_bounds() const override { return {{},{}}; }
263+ bool input_area_contains(geom::Point const&) const override { return false; }
264 void cursor_parameters(bool&, std::string&, std::string&) const { }
265
266 std::shared_ptr<mi::InputChannel> const channel;
267
268=== modified file 'tests/unit-tests/input/android/test_android_input_window_handle.cpp'
269--- tests/unit-tests/input/android/test_android_input_window_handle.cpp 2014-03-26 14:20:14 +0000
270+++ tests/unit-tests/input/android/test_android_input_window_handle.cpp 2014-05-07 09:58:29 +0000
271@@ -89,12 +89,10 @@
272
273 // For now since we are just doing keyboard input we only need surface size,
274 // for touch/pointer events we will need a position
275- EXPECT_CALL(mock_surface, size())
276- .Times(1)
277- .WillOnce(Return(default_surface_size));
278- EXPECT_CALL(mock_surface, top_left())
279- .Times(1)
280- .WillOnce(Return(default_surface_top_left));
281+ EXPECT_CALL(mock_surface, input_bounds())
282+ .Times(1)
283+ .WillOnce(Return(geom::Rectangle{default_surface_top_left,
284+ default_surface_size}));
285 EXPECT_CALL(mock_surface, name())
286 .Times(1)
287 .WillOnce(Return(testing_surface_name));
288
289=== modified file 'tests/unit-tests/scene/test_basic_surface.cpp'
290--- tests/unit-tests/scene/test_basic_surface.cpp 2014-05-05 03:36:45 +0000
291+++ tests/unit-tests/scene/test_basic_surface.cpp 2014-05-07 09:58:29 +0000
292@@ -200,6 +200,33 @@
293 EXPECT_EQ(old_transformation, surface.compositor_snapshot(compositor_id)->transformation());
294 }
295
296+/*
297+ * Until logic is implemented to separate size() from client_size(), verify
298+ * they do return the same thing for backward compatibility.
299+ */
300+TEST_F(BasicSurfaceTest, size_equals_client_size)
301+{
302+ geom::Size const new_size{34, 56};
303+
304+ ms::BasicSurface surface{
305+ name,
306+ rect,
307+ false,
308+ mock_buffer_stream,
309+ std::shared_ptr<mi::InputChannel>(),
310+ stub_configurator,
311+ report};
312+
313+ EXPECT_EQ(rect.size, surface.size());
314+ EXPECT_EQ(rect.size, surface.client_size());
315+ EXPECT_NE(new_size, surface.size());
316+ EXPECT_NE(new_size, surface.client_size());
317+
318+ surface.resize(new_size);
319+ EXPECT_EQ(new_size, surface.size());
320+ EXPECT_EQ(new_size, surface.client_size());
321+}
322+
323 TEST_F(BasicSurfaceTest, test_surface_set_transformation_updates_transform)
324 {
325 EXPECT_CALL(mock_callback, call())
326@@ -383,7 +410,7 @@
327 for(auto y = 0; y <= 3; y++)
328 {
329 auto test_pt = geom::Point{x, y};
330- auto contains = surface.contains(test_pt);
331+ auto contains = surface.input_area_contains(test_pt);
332 if (std::find(contained_pt.begin(), contained_pt.end(), test_pt) != contained_pt.end())
333 {
334 EXPECT_TRUE(contains);
335@@ -430,7 +457,7 @@
336 for(auto y = 0; y <= 3; y++)
337 {
338 auto test_pt = geom::Point{x, y};
339- auto contains = surface.contains(test_pt);
340+ auto contains = surface.input_area_contains(test_pt);
341 if (std::find(contained_pt.begin(), contained_pt.end(), test_pt) != contained_pt.end())
342 {
343 EXPECT_TRUE(contains);

Subscribers

People subscribed via source and target branches