Mir

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

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 :
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
=== modified file 'include/server/mir/frontend/surface.h'
--- include/server/mir/frontend/surface.h 2014-04-15 05:31:19 +0000
+++ include/server/mir/frontend/surface.h 2014-05-07 09:58:29 +0000
@@ -51,7 +51,8 @@
5151
52 virtual ~Surface() {}52 virtual ~Surface() {}
5353
54 virtual geometry::Size size() const = 0;54 /// Size of the client area of the surface (excluding any decorations)
55 virtual geometry::Size client_size() const = 0;
55 virtual MirPixelFormat pixel_format() const = 0;56 virtual MirPixelFormat pixel_format() const = 0;
5657
57 virtual void swap_buffers(graphics::Buffer* old_buffer, std::function<void(graphics::Buffer* new_buffer)> complete) = 0;58 virtual void swap_buffers(graphics::Buffer* old_buffer, std::function<void(graphics::Buffer* new_buffer)> complete) = 0;
5859
=== modified file 'include/server/mir/input/surface.h'
--- include/server/mir/input/surface.h 2014-04-24 08:42:12 +0000
+++ include/server/mir/input/surface.h 2014-05-07 09:58:29 +0000
@@ -20,7 +20,7 @@
20#define MIR_INPUT_SURFACE_H_20#define MIR_INPUT_SURFACE_H_
2121
22#include "mir/geometry/size.h"22#include "mir/geometry/size.h"
23#include "mir/geometry/point.h"23#include "mir/geometry/rectangle.h"
2424
25#include <string>25#include <string>
26#include <memory>26#include <memory>
@@ -35,10 +35,8 @@
35{35{
36public:36public:
37 virtual std::string name() const = 0;37 virtual std::string name() const = 0;
38 virtual geometry::Point top_left() const = 0;38 virtual geometry::Rectangle input_bounds() const = 0;
39 virtual geometry::Size size() const = 0;39 virtual bool input_area_contains(geometry::Point const& point) const = 0;
40 virtual bool contains(geometry::Point const& point) const = 0;
41
42 virtual std::shared_ptr<input::InputChannel> input_channel() const = 0;40 virtual std::shared_ptr<input::InputChannel> input_channel() const = 0;
4341
44protected:42protected:
4543
=== modified file 'include/server/mir/scene/surface.h'
--- include/server/mir/scene/surface.h 2014-05-05 03:36:45 +0000
+++ include/server/mir/scene/surface.h 2014-05-07 09:58:29 +0000
@@ -43,11 +43,18 @@
43{43{
44public:44public:
45 // resolve ambiguous member function names45 // resolve ambiguous member function names
46
46 std::string name() const override = 0;47 std::string name() const override = 0;
47 geometry::Size size() const override = 0;48 geometry::Size client_size() const override = 0;
48 geometry::Point top_left() const override = 0;49 geometry::Rectangle input_bounds() const override = 0;
4950
50 // member functions that don't exist in base classes51 // member functions that don't exist in base classes
52
53 /// Top-left corner (of the window frame if present)
54 virtual geometry::Point top_left() const = 0;
55 /// Size of the surface including window frame (if any)
56 virtual geometry::Size size() const = 0;
57
51 virtual std::unique_ptr<graphics::Renderable> compositor_snapshot(void const* compositor_id) const = 0;58 virtual std::unique_ptr<graphics::Renderable> compositor_snapshot(void const* compositor_id) const = 0;
5259
53 virtual float alpha() const = 0; //only used in examples/60 virtual float alpha() const = 0; //only used in examples/
5461
=== modified file 'include/test/mir_test_doubles/mock_frontend_surface.h'
--- include/test/mir_test_doubles/mock_frontend_surface.h 2014-03-06 06:05:17 +0000
+++ include/test/mir_test_doubles/mock_frontend_surface.h 2014-05-07 09:58:29 +0000
@@ -41,7 +41,7 @@
41 MOCK_METHOD0(force_requests_to_complete, void());41 MOCK_METHOD0(force_requests_to_complete, void());
42 MOCK_METHOD2(swap_buffers, void(graphics::Buffer*, std::function<void(graphics::Buffer*)> complete));42 MOCK_METHOD2(swap_buffers, void(graphics::Buffer*, std::function<void(graphics::Buffer*)> complete));
4343
44 MOCK_CONST_METHOD0(size, geometry::Size());44 MOCK_CONST_METHOD0(client_size, geometry::Size());
45 MOCK_CONST_METHOD0(pixel_format, MirPixelFormat());45 MOCK_CONST_METHOD0(pixel_format, MirPixelFormat());
4646
47 MOCK_CONST_METHOD0(supports_input, bool());47 MOCK_CONST_METHOD0(supports_input, bool());
4848
=== modified file 'include/test/mir_test_doubles/mock_input_surface.h'
--- include/test/mir_test_doubles/mock_input_surface.h 2014-04-24 08:42:12 +0000
+++ include/test/mir_test_doubles/mock_input_surface.h 2014-05-07 09:58:29 +0000
@@ -35,12 +35,8 @@
35 MockInputSurface()35 MockInputSurface()
36 {36 {
37 using namespace ::testing;37 using namespace ::testing;
38 ON_CALL(*this, top_left())38 ON_CALL(*this, input_bounds())
39 .WillByDefault(39 .WillByDefault(Return(geometry::Rectangle()));
40 Return(geometry::Point{}));
41 ON_CALL(*this, size())
42 .WillByDefault(
43 Return(geometry::Size{}));
44 static std::string n;40 static std::string n;
45 ON_CALL(*this, name())41 ON_CALL(*this, name())
46 .WillByDefault(Return(n));42 .WillByDefault(Return(n));
@@ -49,10 +45,9 @@
49 .WillByDefault(Return(c));45 .WillByDefault(Return(c));
50 }46 }
51 ~MockInputSurface() noexcept {}47 ~MockInputSurface() noexcept {}
52 MOCK_CONST_METHOD0(top_left, geometry::Point());
53 MOCK_CONST_METHOD0(size, geometry::Size());
54 MOCK_CONST_METHOD0(name, std::string());48 MOCK_CONST_METHOD0(name, std::string());
55 MOCK_CONST_METHOD1(contains, bool(geometry::Point const&));49 MOCK_CONST_METHOD0(input_bounds, geometry::Rectangle());
50 MOCK_CONST_METHOD1(input_area_contains, bool(geometry::Point const&));
56 MOCK_CONST_METHOD0(input_channel, std::shared_ptr<input::InputChannel>());51 MOCK_CONST_METHOD0(input_channel, std::shared_ptr<input::InputChannel>());
57};52};
5853
5954
=== modified file 'src/server/frontend/session_mediator.cpp'
--- src/server/frontend/session_mediator.cpp 2014-04-15 05:31:19 +0000
+++ src/server/frontend/session_mediator.cpp 2014-05-07 09:58:29 +0000
@@ -169,9 +169,10 @@
169 .with_output_id(graphics::DisplayConfigurationOutputId(request->output_id())));169 .with_output_id(graphics::DisplayConfigurationOutputId(request->output_id())));
170170
171 auto surface = session->get_surface(surf_id);171 auto surface = session->get_surface(surf_id);
172 auto const& client_size = surface->client_size();
172 response->mutable_id()->set_value(surf_id.as_value());173 response->mutable_id()->set_value(surf_id.as_value());
173 response->set_width(surface->size().width.as_uint32_t());174 response->set_width(client_size.width.as_uint32_t());
174 response->set_height(surface->size().height.as_uint32_t());175 response->set_height(client_size.height.as_uint32_t());
175 response->set_pixel_format((int)surface->pixel_format());176 response->set_pixel_format((int)surface->pixel_format());
176 response->set_buffer_usage(request->buffer_usage());177 response->set_buffer_usage(request->buffer_usage());
177178
178179
=== modified file 'src/server/frontend/surface.cpp'
--- src/server/frontend/surface.cpp 2014-03-06 06:05:17 +0000
+++ src/server/frontend/surface.cpp 2014-05-07 09:58:29 +0000
@@ -45,7 +45,7 @@
45 {45 {
46 surface->swap_buffers_blocking(buffer);46 surface->swap_buffers_blocking(buffer);
47 }47 }
48 virtual mir::geometry::Size size() const { return surface->size(); }48 virtual mir::geometry::Size size() const { return surface->client_size(); }
49 virtual MirPixelFormat pixel_format() const { return surface->pixel_format(); }49 virtual MirPixelFormat pixel_format() const { return surface->pixel_format(); }
5050
51 std::shared_ptr<Surface> const surface;51 std::shared_ptr<Surface> const surface;
5252
=== modified file 'src/server/input/android/android_input_window_handle.cpp'
--- src/server/input/android/android_input_window_handle.cpp 2014-03-06 06:05:17 +0000
+++ src/server/input/android/android_input_window_handle.cpp 2014-05-07 09:58:29 +0000
@@ -41,7 +41,7 @@
4141
42 bool touchableRegionContainsPoint(int32_t x, int32_t y) const override42 bool touchableRegionContainsPoint(int32_t x, int32_t y) const override
43 {43 {
44 return surface->contains(geom::Point{x, y});44 return surface->input_area_contains({x, y});
45 }45 }
4646
47 std::shared_ptr<mi::Surface> const surface;47 std::shared_ptr<mi::Surface> const surface;
@@ -69,13 +69,12 @@
69 input_channel->server_fd());69 input_channel->server_fd());
70 }70 }
7171
72 auto surface_size = surface->size();72 auto const& bounds = surface->input_bounds();
73 auto surface_position = surface->top_left();
7473
75 mInfo->frameLeft = surface_position.x.as_uint32_t();74 mInfo->frameLeft = bounds.top_left.x.as_uint32_t();
76 mInfo->frameTop = surface_position.y.as_uint32_t();75 mInfo->frameTop = bounds.top_left.y.as_uint32_t();
77 mInfo->frameRight = mInfo->frameLeft + surface_size.width.as_uint32_t();76 mInfo->frameRight = mInfo->frameLeft + bounds.size.width.as_uint32_t();
78 mInfo->frameBottom = mInfo->frameTop + surface_size.height.as_uint32_t();77 mInfo->frameBottom = mInfo->frameTop + bounds.size.height.as_uint32_t();
7978
80 mInfo->touchableRegionLeft = mInfo->frameLeft;79 mInfo->touchableRegionLeft = mInfo->frameLeft;
81 mInfo->touchableRegionTop = mInfo->frameTop;80 mInfo->touchableRegionTop = mInfo->frameTop;
8281
=== modified file 'src/server/scene/basic_surface.cpp'
--- src/server/scene/basic_surface.cpp 2014-05-05 03:36:45 +0000
+++ src/server/scene/basic_surface.cpp 2014-05-07 09:58:29 +0000
@@ -186,6 +186,12 @@
186 return surface_rect.size;186 return surface_rect.size;
187}187}
188188
189mir::geometry::Size ms::BasicSurface::client_size() const
190{
191 // TODO: In future when decorated, client_size() would be smaller than size
192 return size();
193}
194
189MirPixelFormat ms::BasicSurface::pixel_format() const195MirPixelFormat ms::BasicSurface::pixel_format() const
190{196{
191 return surface_buffer_stream->get_stream_pixel_format();197 return surface_buffer_stream->get_stream_pixel_format();
@@ -273,7 +279,16 @@
273 return surface_rect.top_left;279 return surface_rect.top_left;
274}280}
275281
276bool ms::BasicSurface::contains(geom::Point const& point) const282geom::Rectangle ms::BasicSurface::input_bounds() const
283{
284 std::unique_lock<std::mutex> lk(guard);
285
286 // This is historically unchanged, but if you look at input_area_contains()
287 // it seems a bit inconsistent...
288 return surface_rect;
289}
290
291bool ms::BasicSurface::input_area_contains(geom::Point const& point) const
277{292{
278 std::unique_lock<std::mutex> lock(guard);293 std::unique_lock<std::mutex> lock(guard);
279 if (hidden)294 if (hidden)
280295
=== modified file 'src/server/scene/basic_surface.h'
--- src/server/scene/basic_surface.h 2014-05-05 03:36:45 +0000
+++ src/server/scene/basic_surface.h 2014-05-07 09:58:29 +0000
@@ -94,6 +94,7 @@
94 void set_hidden(bool is_hidden);94 void set_hidden(bool is_hidden);
9595
96 geometry::Size size() const override;96 geometry::Size size() const override;
97 geometry::Size client_size() const override;
9798
98 MirPixelFormat pixel_format() const;99 MirPixelFormat pixel_format() const;
99100
@@ -112,7 +113,8 @@
112113
113 void resize(geometry::Size const& size) override;114 void resize(geometry::Size const& size) override;
114 geometry::Point top_left() const override;115 geometry::Point top_left() const override;
115 bool contains(geometry::Point const& point) const override;116 geometry::Rectangle input_bounds() const override;
117 bool input_area_contains(geometry::Point const& point) const override;
116 void set_alpha(float alpha) override;118 void set_alpha(float alpha) override;
117 void set_transformation(glm::mat4 const&) override;119 void set_transformation(glm::mat4 const&) override;
118120
119121
=== modified file 'tests/unit-tests/frontend/test_session_mediator.cpp'
--- tests/unit-tests/frontend/test_session_mediator.cpp 2014-05-01 07:13:02 +0000
+++ tests/unit-tests/frontend/test_session_mediator.cpp 2014-05-07 09:58:29 +0000
@@ -101,7 +101,7 @@
101 mock_surfaces[mf::SurfaceId{1}] = mock_surface;101 mock_surfaces[mf::SurfaceId{1}] = mock_surface;
102 mock_buffer = std::make_shared<NiceMock<mtd::MockBuffer>>(geom::Size(), geom::Stride(), MirPixelFormat());102 mock_buffer = std::make_shared<NiceMock<mtd::MockBuffer>>(geom::Size(), geom::Stride(), MirPixelFormat());
103103
104 EXPECT_CALL(*mock_surface, size()).Times(AnyNumber()).WillRepeatedly(Return(geom::Size()));104 EXPECT_CALL(*mock_surface, client_size()).Times(AnyNumber()).WillRepeatedly(Return(geom::Size()));
105 EXPECT_CALL(*mock_surface, pixel_format()).Times(AnyNumber()).WillRepeatedly(Return(MirPixelFormat()));105 EXPECT_CALL(*mock_surface, pixel_format()).Times(AnyNumber()).WillRepeatedly(Return(MirPixelFormat()));
106 EXPECT_CALL(*mock_surface, swap_buffers(_, _)).Times(AnyNumber())106 EXPECT_CALL(*mock_surface, swap_buffers(_, _)).Times(AnyNumber())
107 .WillRepeatedly(InvokeArgument<1>(mock_buffer.get()));107 .WillRepeatedly(InvokeArgument<1>(mock_buffer.get()));
@@ -122,7 +122,7 @@
122 if (last_surface_id != 1) {122 if (last_surface_id != 1) {
123 mock_surfaces[id] = std::make_shared<mtd::MockFrontendSurface>();123 mock_surfaces[id] = std::make_shared<mtd::MockFrontendSurface>();
124124
125 EXPECT_CALL(*mock_surfaces[id], size()).Times(AnyNumber()).WillRepeatedly(Return(geom::Size()));125 EXPECT_CALL(*mock_surfaces[id], client_size()).Times(AnyNumber()).WillRepeatedly(Return(geom::Size()));
126 EXPECT_CALL(*mock_surfaces[id], pixel_format()).Times(AnyNumber()).WillRepeatedly(Return(MirPixelFormat()));126 EXPECT_CALL(*mock_surfaces[id], pixel_format()).Times(AnyNumber()).WillRepeatedly(Return(MirPixelFormat()));
127 EXPECT_CALL(*mock_surfaces[id], swap_buffers(_, _)).Times(AnyNumber())127 EXPECT_CALL(*mock_surfaces[id], swap_buffers(_, _)).Times(AnyNumber())
128 .WillRepeatedly(InvokeArgument<1>(mock_buffer.get()));128 .WillRepeatedly(InvokeArgument<1>(mock_buffer.get()));
129129
=== modified file 'tests/unit-tests/input/android/test_android_input_target_enumerator.cpp'
--- tests/unit-tests/input/android/test_android_input_target_enumerator.cpp 2014-04-24 08:42:12 +0000
+++ tests/unit-tests/input/android/test_android_input_target_enumerator.cpp 2014-05-07 09:58:29 +0000
@@ -74,9 +74,8 @@
74 }74 }
7575
76 std::string name() const { return {}; }76 std::string name() const { return {}; }
77 geom::Point top_left() const { return {}; }77 geom::Rectangle input_bounds() const override { return {{},{}}; }
78 geom::Size size() const { return {}; }78 bool input_area_contains(geom::Point const&) const override { return false; }
79 bool contains(geom::Point const&) const { return false; }
80 void cursor_parameters(bool&, std::string&, std::string&) const { }79 void cursor_parameters(bool&, std::string&, std::string&) const { }
81 80
82 std::shared_ptr<mi::InputChannel> const channel;81 std::shared_ptr<mi::InputChannel> const channel;
8382
=== modified file 'tests/unit-tests/input/android/test_android_input_window_handle.cpp'
--- tests/unit-tests/input/android/test_android_input_window_handle.cpp 2014-03-26 14:20:14 +0000
+++ tests/unit-tests/input/android/test_android_input_window_handle.cpp 2014-05-07 09:58:29 +0000
@@ -89,12 +89,10 @@
8989
90 // For now since we are just doing keyboard input we only need surface size,90 // For now since we are just doing keyboard input we only need surface size,
91 // for touch/pointer events we will need a position91 // for touch/pointer events we will need a position
92 EXPECT_CALL(mock_surface, size())92 EXPECT_CALL(mock_surface, input_bounds())
93 .Times(1)93 .Times(1)
94 .WillOnce(Return(default_surface_size));94 .WillOnce(Return(geom::Rectangle{default_surface_top_left,
95 EXPECT_CALL(mock_surface, top_left())95 default_surface_size}));
96 .Times(1)
97 .WillOnce(Return(default_surface_top_left));
98 EXPECT_CALL(mock_surface, name())96 EXPECT_CALL(mock_surface, name())
99 .Times(1)97 .Times(1)
100 .WillOnce(Return(testing_surface_name));98 .WillOnce(Return(testing_surface_name));
10199
=== modified file 'tests/unit-tests/scene/test_basic_surface.cpp'
--- tests/unit-tests/scene/test_basic_surface.cpp 2014-05-05 03:36:45 +0000
+++ tests/unit-tests/scene/test_basic_surface.cpp 2014-05-07 09:58:29 +0000
@@ -200,6 +200,33 @@
200 EXPECT_EQ(old_transformation, surface.compositor_snapshot(compositor_id)->transformation());200 EXPECT_EQ(old_transformation, surface.compositor_snapshot(compositor_id)->transformation());
201}201}
202202
203/*
204 * Until logic is implemented to separate size() from client_size(), verify
205 * they do return the same thing for backward compatibility.
206 */
207TEST_F(BasicSurfaceTest, size_equals_client_size)
208{
209 geom::Size const new_size{34, 56};
210
211 ms::BasicSurface surface{
212 name,
213 rect,
214 false,
215 mock_buffer_stream,
216 std::shared_ptr<mi::InputChannel>(),
217 stub_configurator,
218 report};
219
220 EXPECT_EQ(rect.size, surface.size());
221 EXPECT_EQ(rect.size, surface.client_size());
222 EXPECT_NE(new_size, surface.size());
223 EXPECT_NE(new_size, surface.client_size());
224
225 surface.resize(new_size);
226 EXPECT_EQ(new_size, surface.size());
227 EXPECT_EQ(new_size, surface.client_size());
228}
229
203TEST_F(BasicSurfaceTest, test_surface_set_transformation_updates_transform)230TEST_F(BasicSurfaceTest, test_surface_set_transformation_updates_transform)
204{231{
205 EXPECT_CALL(mock_callback, call())232 EXPECT_CALL(mock_callback, call())
@@ -383,7 +410,7 @@
383 for(auto y = 0; y <= 3; y++)410 for(auto y = 0; y <= 3; y++)
384 {411 {
385 auto test_pt = geom::Point{x, y};412 auto test_pt = geom::Point{x, y};
386 auto contains = surface.contains(test_pt);413 auto contains = surface.input_area_contains(test_pt);
387 if (std::find(contained_pt.begin(), contained_pt.end(), test_pt) != contained_pt.end())414 if (std::find(contained_pt.begin(), contained_pt.end(), test_pt) != contained_pt.end())
388 {415 {
389 EXPECT_TRUE(contains);416 EXPECT_TRUE(contains);
@@ -430,7 +457,7 @@
430 for(auto y = 0; y <= 3; y++)457 for(auto y = 0; y <= 3; y++)
431 {458 {
432 auto test_pt = geom::Point{x, y};459 auto test_pt = geom::Point{x, y};
433 auto contains = surface.contains(test_pt);460 auto contains = surface.input_area_contains(test_pt);
434 if (std::find(contained_pt.begin(), contained_pt.end(), test_pt) != contained_pt.end())461 if (std::find(contained_pt.begin(), contained_pt.end(), test_pt) != contained_pt.end())
435 {462 {
436 EXPECT_TRUE(contains);463 EXPECT_TRUE(contains);

Subscribers

People subscribed via source and target branches