Merge into development-branch : cursor-spike-phase-2-resubmit : Code : Mir

Status:	Merged
Approved by:	Robert Carr on 2014-05-12
Approved revision:	no longer in the source branch.
Merged at revision:	1621
Proposed branch:	lp:~mir-team/mir/cursor-spike-phase-2-resubmit
Merge into:	lp:mir
Prerequisite:	lp:~mir-team/mir/cursor-spike-phase-1-resubmit-resubmit-resubmit-hello-out-there
Diff against target:	926 lines (+685/-10) 18 files modified include/client/mir_toolkit/mir_cursor_configuration.h (+68/-0) include/client/mir_toolkit/mir_surface.h (+13/-0) include/platform/mir/graphics/cursor.h (+3/-1) src/client/CMakeLists.txt (+1/-0) src/client/cursor_configuration.h (+31/-0) src/client/mir_cursor_api.cpp (+44/-0) src/client/mir_surface.cpp (+19/-0) src/client/mir_surface.h (+4/-0) src/client/mir_surface_api.cpp (+16/-0) src/platform/graphics/mesa/cursor.cpp (+2/-2) src/platform/graphics/mesa/cursor.h (+2/-2) src/server/frontend/protobuf_message_processor.cpp (+4/-0) src/server/frontend/session_mediator.cpp (+10/-0) src/server/frontend/session_mediator.h (+6/-1) src/shared/protobuf/mir_protobuf.proto (+8/-0) tests/acceptance-tests/CMakeLists.txt (+1/-0) tests/acceptance-tests/test_client_cursor_api.cpp (+449/-0) tests/unit-tests/graphics/mesa/test_cursor.cpp (+4/-4)
To merge this branch:	bzr merge lp:~mir-team/mir/cursor-spike-phase-2-resubmit
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration		Approve on 2014-05-12
Daniel van Vugt			Abstain on 2014-05-12
Alberto Aguirre (community)			Approve on 2014-05-09
Chris Halse Rogers			Approve on 2014-05-08
Andreas Pokorny (community)			Approve on 2014-05-07
Alan Griffiths			Approve on 2014-05-07
Kevin DuBois (community)		2014-04-29	Needs Information on 2014-05-01
Review via email: mp+217685@code.launchpad.net

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-29:

#

FAILED: Continuous integration, rev:1498
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1437/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1755/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1753/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/1325/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1169/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1174/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1328/console

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

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-30:

#

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Robert Carr (robertcarr) wrote on 2014-04-30:

#

Failure looks unrelated.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-05-01:

#

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

review: Approve (continuous-integration)

Revision history for this message

Kevin DuBois (kdub) wrote on 2014-05-01:

#

> Failure looks unrelated.

There's been a few of those floating around, haven't been able to figure out why

Revision history for this message

Kevin DuBois (kdub) wrote on 2014-05-01:

#

mostly "needs info":

1) +MirWaitHandle* mir_surface_configure_cursor(MirSurface *surface, MirCursor const* parameters);
We submit the cursor and the surface, but don't tell what we want (I assume its like "make an hourglass over this surface", or "hide cursor over this surface"). Is a 3rd parameter coming in the future?
2) is it difficult to add a mir_available_cursor_names() sort of function, so we know what strings will get a cursor? this could condense the 'default' and 'disabled' cursor acquisition too.
3) MirCursor is c++ code, why not std::string?
4) include guards on cursor_representation.h

review: Needs Information

Revision history for this message

Robert Carr (robertcarr) wrote on 2014-05-01:

#

Thanks!

>> We submit the cursor and the surface, but don't tell what we want (I assume its like "make an hourglass
>> over this surface", or "hide cursor over this surface"). Is a 3rd parameter coming in the future?

MirCursor was in fact this struct. I've renamed it to MirCursorConfiguration to be more explicit.

>> 2) is it difficult to add a mir_available_cursor_names() sort of function, so we know what strings will get >> a cursor? this could condense the 'default' and 'disabled' cursor acquisition too.

I'll add this with the loader.

>> 3) MirCursor is c++ code, why not std::string?
>> 4) include guards on cursor_representation.h

Fixed!

Revision history for this message

Robert Carr (robertcarr) wrote on 2014-05-01:

#

>> 2) is it difficult to add a mir_available_cursor_names() sort of function, so we know what strings will get >> a cursor? this could condense the 'default' and 'disabled' cursor acquisition too.

>> I'll add this with the loader.

Or something of the sort at least.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-05-01:

#

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

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-05-02:

#

43 +void mir_cursor_configuration_destroy(MirCursorConfiguration *parameters);
44 +
45 +/**
46 + * Returns a new MirCursorConfiguration representing a disabled cursor
47 + * \return A cursor parameters object which must be passed
48 + * to mir_cursor_configuration_destroy
49 + */
50 +MirCursorConfiguration* mir_cursor_configuration_disabled();
51 +
52 +/**
53 + * Returns a new MirCursorConfiguration representing the default cursor
54 + * request.
55 + * \return A cursor parameters object which must be passed
56 + * to_mir_cursor_configuration_destroy
57 + */
58 +MirCursorConfiguration *mir_cursor_configuration_default();
59 +
60 +/**
61 + * Returns a new MirCursorConfiguration representing a named cursor
62 + * from the system cursor theme. At present all implementations
63 + * follow XCursor naming conventions.
64 + * \param [in] name The cursor name
65 + * \return A cursor parameters object which must be passed
66 + * to_mir_cursor_configuration_destroy
67 + */
68 +MirCursorConfiguration *mir_cursor_configuration_from_name(char const* name);

Is it really worth having 3 functions?

Couldn't we have one function and some default names? Vis:

extern char const *const mir_default_cursor_name;
extern char const *const mir_disabled_cursor_name;

MirCursorConfiguration *mir_cursor_configuration_from_name(char const* name);

...

mir_cursor_configuration_from_name(mir_disabled_cursor_name);

~~~~

200 +MirCursorConfiguration* mir_cursor_configuration_from_name(char const* name)
201 +{
202 + auto c = new MirCursorConfiguration;
203 + c->name = std::string(name);
204 +
205 + return c;
206 +}

This is a C API - but C++ memory allocations can throw.

review: Needs Fixing

Revision history for this message

Robert Carr (robertcarr) wrote on 2014-05-02:

#

Alan: Addressed concerns.

Also made some changes to mg::Cursor API, since set_image was changed to accept references, had to stop using nullptr to disable cursor, api is now show(image)/hide()

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-05-02:

#

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

review: Approve (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-05-04:

#

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

review: Approve (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-05-05:

#

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

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-05-06:

#

233 + {
234 + std::unique_lock<decltype(mutex)> lock(mutex);
235 + setting.mutable_surfaceid()->CopyFrom(surface.id());
236 + if (cursor->name != "")
237 + setting.set_name(cursor->name.c_str());
238 + }

indentation

~~~~

374 + BOOST_THROW_EXCEPTION(std::runtime_error("Cursor API not yet implemented"));

We have a tradition of having a "TODO" comment to mark pending work. (Also, getting very picky, I think this is more of a logic_error than a runtime_error.)

~~~~

406 + // No name is interpreted as disabled cursor.
407 + optional string name = 2;

Is a comment really clearer than a boolean field?

~~~~

528 + if (image->cursor_name != "default")
529 + return false;
530 + return true;

Overly verbose: return image->cursor_name == "default";

~~~~

662 +// In this set we create a 1x1 client surface at the point (1,0). The client requests to disable the cursor
663 +// over this surface. Since the cursor starts at (0,0) we when we move the cursor by (1,0) thus causing it
664 +// to enter the bounds of the first surface, we should observe it being disabled.
665 +TEST_F(TestClientCursorAPI, DISABLED_client_may_disable_cursor_over_surface)

We have a tradition of having a "TODO" comment to mark pending work.

Also with this and other tests it looks like a lot of the setup could be moved into the test fixture - which would make toe behavior under test a lot clearer.

review: Needs Fixing

Revision history for this message

Robert Carr (robertcarr) wrote on 2014-05-06:

#

Fixed indentation, verbose and TODOs. Though, the branch removing the TODOs (which now has to be updated to remove the TODOs) is ready to propose when this one lands ;)

>> Is a comment really clearer than a boolean field?

In this case I think it is. If you use a field there are two more questions:
What if the enabled field is true but there is no cursor image?
What if the enabled field is false but there is a cursor image?

Not only are there multiple possible behaviors but it requires documentation as well.

>> Also with this and other tests it looks like a lot of the setup could be moved into the test fixture -
>> which would make toe behavior under test a lot clearer.

I'll do another round of abstraction on the tests once I have a little more time to deal with the input tests as well and arrive at the grand unified input test fixture.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-05-06:

#

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

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-05-07:

#

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

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-05-07:

#

OK for now

review: Approve

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2014-05-07:

#

~~~
208 + catch (std::exception const&)
294 + catch (std::exception const&)
~~~
Shouldn't you be catching all exceptions with catch(...) ?

~~~
191 +char const *const mir_disabled_cursor_name = NULL;
287 + MirWaitHandle *result = NULL;
~~~

Use nullptr

~~~
487 +namespace
488 +{
489 + char const* const mir_test_socket = mtf::test_socket_file().c_str();
490 +}
~~~

Perhaps define that as a static member variable of CursorSettingClient?

review: Needs Fixing

Revision history for this message

Andreas Pokorny (andreas-pokorny) wrote on 2014-05-07:

#

while you are at it and fix the stuff above, please have a look at:
159 + Brace Style

apart from that looks good so..

review: Approve

Revision history for this message

Chris Halse Rogers (raof) wrote on 2014-05-08:

#

Looks broadly fine to me.

I suspect that the tests would be simpler to write and easier to debug if they used InProcessServer.

review: Approve

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-05-08:

#

Note that in Alberto's comment:

287 + MirWaitHandle *result = NULL;
~~~
Use nullptr

The result is being returned from a C API, not C++. So NULL is more correct than nullptr, because the caller will only understand NULL, not nullptr.

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-05-08:

#

(1) I think "Configuration" is a vague word with respect to MirCursorConfiguration, and find it confusing. Looking to the source for an explanation, I find a comment which is also confusing:

158 +// Cursor parameterization. May grow to include RGBA cursors, etc...
159 +struct MirCursorConfiguration {
160 + std::string name;
161 +};

We already support colour cursors(!?), so can you explain what the struct might grow to represent? Then it might be easier to come up with a more appropriate name/structure.

(2) Very minor issue: This function name is inconsistent with the existing convention of:
mir_OBJECT_OPERATION(OBJECT x, ...)
64 +MirCursorConfiguration *mir_cursor_configuration_from_name(char const* name);
Although maintaining complete consistency can yield even more awkward names. So I'm not sure if it should be changed at all.

(3) This inline documentation needs elaboration (like a URL of X docs). There's presently not enough information provided in the header/docs to really use the function...
59 + * follow XCursor naming conventions.

(4) Inappropriate function name:
98 +MirWaitHandle* mir_surface_configure_cursor(MirSurface *surface, MirCursorConfiguration const* parameters);
The verb "configure" implies you're changing the cursor object, which is not true. More accurate would be "mir_surface_set_cursor".

(5) Per previous discussions I don't think this is correct for common toolkit usage:
405 +message CursorSetting {
406 + required SurfaceId surfaceid = 1;
407 + // No name is interpreted as disabled cursor.
408 + optional string name = 2;
409 +}
Hiding the cursor should probably not change its appearance for when it becomes visible again. And simply making the cursor invisible will lead to bugs where it sounds like the cursor still exists and is clickable on things while invisible.

The big issue is (1). We either need some explanation of what a "MirCursorConfiguration" is, or a clearer name like "MirCursor" which would already be understood.

(1) I think "Configuration" is a vague word with respect to MirCursorConfiguration, and find it confusing. Looking to the source for an explanation, I find a comment which is also confusing:

158	+// Cursor parameterization. May grow to include RGBA cursors, etc...
159	+struct MirCursorConfiguration {
160	+    std::string name;
161	+};

We already support colour cursors(!?), so can you explain what the struct might grow to represent? Then it might be easier to come up with a more appropriate name/structure.

(2) Very minor issue: This function name is inconsistent with the existing convention of:
     mir_OBJECT_OPERATION(OBJECT x, ...)
64	+MirCursorConfiguration *mir_cursor_configuration_from_name(char const* name);
Although maintaining complete consistency can yield even more awkward names. So I'm not sure if it should be changed at all.

(3) This inline documentation needs elaboration (like a URL of X docs). There's presently not enough information provided in the header/docs to really use the function...
59	+ * follow XCursor naming conventions.

(4) Inappropriate function name:
98	+MirWaitHandle* mir_surface_configure_cursor(MirSurface *surface, MirCursorConfiguration const* parameters);
The verb "configure" implies you're changing the cursor object, which is not true. More accurate would be "mir_surface_set_cursor".

(5) Per previous discussions I don't think this is correct for common toolkit usage:
405	+message CursorSetting {
406	+  required SurfaceId surfaceid = 1;
407	+  // No name is interpreted as disabled cursor.
408	+  optional string name = 2;
409	+}
Hiding the cursor should probably not change its appearance for when it becomes visible again. And simply making the cursor invisible will lead to bugs where it sounds like the cursor still exists and is clickable on things while invisible.

The big issue is (1). We either need some explanation of what a "MirCursorConfiguration" is, or a clearer name like "MirCursor" which would already be understood.

review: Needs Fixing

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-05-08:

#

(6) The API implication that a surface has a single cursor setting is misleading. Obviously the cursor will vary over the surface depending on the widget. So although the one-cursor-set-per-surface sounds wrong, I now realize it does work so long as you make the app/toolkit do the hard work of changing the cursor as it moves.

On the other hand, Mir does already have the concept internally of input regions (still needs work). But since we already have multiple input rectangle support per-surface in libmirserver (BasicSurface::input_rectangles), wouldn't it be better to expose that to the client API and simply attach a MirCursor to each input rectangle? Then the server could do the cursor changes for you. I think that's what X and Win32 does... And it certainly eliminates the redundancy of asking every toolkit to re-implement cursor tracking.

Revision history for this message

Chris Halse Rogers (raof) wrote on 2014-05-08:

#

On Thu, May 8, 2014 at 2:02 PM, Daniel van Vugt
<email address hidden> wrote:
> (6) The API implication that a surface has a single cursor setting is
> misleading. Obviously the cursor will vary over the surface depending
> on the widget. So although the one-cursor-set-per-surface sounds
> wrong, I now realize it does work so long as you make the app/toolkit
> do the hard work of changing the cursor as it moves.
>
> On the other hand, Mir does already have the concept internally of
> input regions (still needs work). But since we already have multiple
> input rectangle support per-surface in libmirserver
> (BasicSurface::input_rectangles), wouldn't it be better to expose
> that to the client API and simply attach a MirCursor to each input
> rectangle? Then the server could do the cursor changes for you. I
> think that's what X and Win32 does... And it certainly eliminates the
> redundancy of asking every toolkit to re-implement cursor tracking.

While that used to be true, I don't think it is still the case - GTK at
least moved away from multiple X11-protocol-windows per window some
time ago. Sometime late in the 2.x series, IIRC.

While exposing subrectangles is appealing for other reasons, I think
the toolkit boat sailed some time ago :)

Revision history for this message

Robert Carr (robertcarr) wrote on 2014-05-08:

#

Alberto/Andreas: Addressed comments.

>> (1) I think "Configuration" is a vague word with respect to MirCursorConfiguration.

I disagree. I've updated the comments though. If you look up in the review you can see it was MirCursor and why it was changed.

>> (2) Very minor issue: This function name is inconsistent with the existing convention of:

It's because there are no existing named constructors which do not require an existing mir object. A possible name is mir_cursor_name_create_configuration(const char*) but this is the more familiar pattern.

>> (3) (3) This inline documentation needs elaboration (like a URL of X docs). There's presently not enough
>> information provided in the header/docs to really use the function...

I've removed the suggestion that you can set arbitrary cursor and am starting with just default/disabled for the XMir use case. Will elaborate on additional cursors in cursor spike phase 5 (XCursor theme loader...)

>> (4) The verb "configure" implies you're changing the cursor object, which is not true. More accurate would be "mir_surface_set_cursor".

I disagree. Perhaps more accurate would be mir_surface_configure_"cursor_request" but everything is a request so I dropped the suffix.

>> (5) Hiding the cursor should probably not change its appearance for when it becomes visible again

The cursor can only be made visable again by requesting a specific issue.

>> (5) And simply making the cursor invisible will lead to bugs where it sounds like the cursor still exists and is clickable on things while invisible.

Yes this is the intended use of the API, for apps which have no cursor (i.e. perhaps a racing game) but still respond to mouse events or apps which really want to draw their own cursor (virtualization software, rooted XMir).

>> (6)
>> (6) The API implication that a surface has a single cursor setting is misleading. Obviously the cursor
>> will vary over the surface depending on the widget. So although the one-cursor-set-per-surface sounds
>> wrong, I now realize it does work so long as you make the app/toolkit do the hard work of changing the
>> cursor as it moves.

In my experience with Qt, Chromium, and GTK it would be more effort (Qt, Chromium) or about the same (GTK) to require the toolkit to assemble these rectangles. In the case of QML and Chromium I am not even sure you can accurately assemble these rectangles and they would likely use a full surface rectangle that they changed the cursor name on.

Alberto/Andreas: Addressed comments.

>> (1) I think "Configuration" is a vague word with respect to MirCursorConfiguration.

I disagree. I've updated the comments though. If you look up in the review you can see it was MirCursor and why it was changed.

>> (2) Very minor issue: This function name is inconsistent with the existing convention of:

It's because there are no existing named constructors which do not require an existing mir object. A possible name is mir_cursor_name_create_configuration(const char*) but this is the more familiar pattern.

>> (3) (3) This inline documentation needs elaboration (like a URL of X docs). There's presently not enough 
>> information provided in the header/docs to really use the function...

I've removed the suggestion that you can set arbitrary cursor and am starting with just default/disabled for the XMir use case. Will elaborate on additional cursors in cursor spike phase 5 (XCursor theme loader...)

>> (4) The verb "configure" implies you're changing the cursor object, which is not true. More accurate would be "mir_surface_set_cursor".

I disagree. Perhaps more accurate would be mir_surface_configure_"cursor_request" but everything is a request so I dropped the suffix.

>> (5) Hiding the cursor should probably not change its appearance for when it becomes visible again

The cursor can only be made visable again by requesting a specific issue.

>> (5) And simply making the cursor invisible will lead to bugs where it sounds like the cursor still exists and is clickable on things while invisible.

Yes this is the intended use of the API, for apps which have no cursor (i.e. perhaps a racing game) but still respond to mouse events or apps which really want to draw their own cursor (virtualization software, rooted XMir).

>> (6) 
>> (6) The API implication that a surface has a single cursor setting is misleading. Obviously the cursor 
>> will vary over the surface depending on the widget. So although the one-cursor-set-per-surface sounds 
>> wrong, I now realize it does work so long as you make the app/toolkit do the hard work of changing the 
>> cursor as it moves.

In my experience with Qt, Chromium, and GTK it would be more effort (Qt, Chromium) or about the same (GTK) to require the toolkit to assemble these rectangles. In the case of QML and Chromium I am not even sure you can accurately assemble these rectangles and they would likely use a full surface rectangle that they changed the cursor name on.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-05-09:

#

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

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-05-09:

#

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

review: Approve (continuous-integration)

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-05-09:

#

(1) Reading again, I still believe there's no justification for MirCursorConfiguration. All the relevant info would be contained in a MirCursor (image + hotspot... what else is there?).

(3) You should refer to them by identifier name in the docs. Not all readers will be looking at the header file and immediately see which identifiers to use (if any):
58 + * from the system cursor theme. Currently only the default cursor
59 + * and a disabled cursor are supported.

(4) OK, assuming the verb "configure" describes something more complex than "setting" a cursor, what's the extra complexity? What is it doing other than setting a cursor?

(5) Your justification for having invisible cursors is not valid, I believe. Games with no visible cursor require relative movement data, which is unrelated to any "cursor" location. See: https://bugs.launchpad.net/mir/+bug/1276322
In particular, in your "racing game" or FPS you don't want mouse movement to suddenly hit an invisible wall like a cursor would at the edge of the screen. Thus, you need to turn the cursor "off" (including events) and not just make it invisible. Per bug 1276322.

(6) I don't really mind letting it be a toolkit/app problem. I was particularly curious since I noticed we already have BasicSurface::input_rectangles in the server. It would be nice if we avoided libmirserver having to maintain any knowledge of the sub-regions of the client surface.

review: Needs Fixing

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2014-05-09:

#

ok.

review: Approve

Revision history for this message

Robert Carr (robertcarr) wrote on 2014-05-09:

#

>> (1) Reading again, I still believe there's no justification for MirCursorConfiguration. All the relevant '
>> info would be contained in a MirCursor (image + hotspot... what else is there?).

The idea in changing it was that MirCursor could be meant to refer to a particular on-screen cursor. RAther what this struct contains is the parameters used to configure an on screen cursor.

>> (3) You should refer to them by identifier name in the docs.

Ok.

>> (4) OK, assuming the verb "configure" describes something more complex than "setting" a cursor, what's the extra complexity? What is it doing other than setting a cursor?

I am thinking of the cursor as the onscreen buffer with a particular location. In this sense you can't set a cursor on a surface, rather you set the cursor configuration which a surface requests.

>> (5) Your justification for having invisible cursors is not valid, I believe. Games with no visible cursor require relative movement data, which is unrelated to any "cursor" location

I guess that's true for racing games which would need some sort of mouse grab...I think there are still use cases though.

1. Virtual Machine/VNC type apps operating without mouse grab
2. Apps which may wish to use crazy cursors (i.e. maybe a fullscreen painting app has some crazy paint brush cursor which interacts with the surface in animations and its easiest to just do it on the client side).
3. Xmir (arguably, but its quite convenient!)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-05-09:

#

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-05-12:

#

(1) Alright, MirCursorConfiguration is a separate object to a specific MirCursor instance (which moves). I get that now, but suggest we try to come up with a noun less clumsy than the (slightly too long) "Configuration".

(5) OK, so there are indeed invisible cursor use cases, but I think they're less common than needing to hide the cursor (and not get any absolute location events). Games need grabs, certainly.

review: Abstain

Revision history for this message

Robert Carr (robertcarr) wrote on 2014-05-12:

#

Jenkins failure is intermittent and unrelated. Landing.

Revision history for this message

PS Jenkins bot (ps-jenkins) on 2014-05-12:

#

review: Approve (continuous-integration)

Mir

Merge lp:~mir-team/mir/cursor-spike-phase-2-resubmit into lp:mir

Commit message

Description of the change

Preview Diff

Subscribers

 === added file 'include/client/mir_toolkit/mir_cursor_configuration.h'
 --- include/client/mir_toolkit/mir_cursor_configuration.h	1970-01-01 00:00:00 +0000
 +++ include/client/mir_toolkit/mir_cursor_configuration.h	2014-05-09 20:09:14 +0000
@@ -0,0 +1,68 @@
++/*
++ * Copyright © 2014 Canonical Ltd.
++ *
++ * This program is free software: you can redistribute it and/or modify it
++ * under the terms of the GNU Lesser General Public License version 3,
++ * as published by the Free Software Foundation.
++ *
++ * This program is distributed in the hope that it will be useful,
++ * but WITHOUT ANY WARRANTY; without even the implied warranty of
++ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
++ * GNU Lesser General Public License for more details.
++ *
++ * You should have received a copy of the GNU Lesser General Public License
++ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
++ *
++ */
++
++#ifndef MIR_TOOLKIT_MIR_CURSOR_H_
++#define MIR_TOOLKIT_MIR_CURSOR_H_
++
++/**
++ * Opaque structure containing cursor parameterization. Create with mir_cursor* family.
++ * Used with mir_surface_configure_cursor.
++ */
++typedef struct MirCursorConfiguration MirCursorConfiguration;
++
++#ifdef __cplusplus
++/**
++ * \addtogroup mir_toolkit
++ * @{
++ */
++extern "C" {
++#endif
++
++/**
++ * A special cursor name for use with mir_cursor_configuration_from_name
++ * representing the system default cursor.
++ */
++extern char const *const mir_default_cursor_name;
++/**
++ * A special cursor name for use with mir_cursor_configuration_from_name
++ * representing a disabled cursor image.
++ */
++extern char const *const mir_disabled_cursor_name;
++
++/**
++ * Release resources assosciated with cursor parameters
++ *     \param [in] parameters The operand
++ */
++void mir_cursor_configuration_destroy(MirCursorConfiguration *parameters);
++
++/**
++ * Returns a new MirCursorConfiguration representing a named cursor
++ * from the system cursor theme. Currently only the symbolic values,
++ * mir_default_cursor_name and mir_disabled_cursor_name are supported
++ * as input.
++ *    \param [in] name The cursor name
++ *    \return A cursor parameters object which must be passed
++ *            to_mir_cursor_configuration_destroy
++ */
++MirCursorConfiguration *mir_cursor_configuration_from_name(char const* name);
++
++#ifdef __cplusplus
++}
++/**@}*/
++#endif
++
++#endif /* MIR_TOOLKIT_MIR_CURSOR_H_ */
 === modified file 'include/client/mir_toolkit/mir_surface.h'
 --- include/client/mir_toolkit/mir_surface.h	2014-03-31 14:36:08 +0000
 +++ include/client/mir_toolkit/mir_surface.h	2014-05-09 20:09:14 +0000
@@ -21,6 +21,7 @@
  #include <mir_toolkit/mir_native_buffer.h>
  #include <mir_toolkit/client_types.h>
  #include <mir_toolkit/common.h>
++#include <mir_toolkit/mir_cursor_configuration.h>
  #ifdef __cplusplus
  /**
@@ -246,6 +247,18 @@
   */
  int mir_surface_get_swapinterval(MirSurface* surface);
++/**
++ * Choose the cursor state for a surface: whether a cursor is shown,
++ * and which cursor if so.
++ *    \param [in] surface    The surface to operate on
++ *    \param [in] parameters The configuration parameters obtained
++ *                           from mir_cursor* family of functions.
++ *    \return                A wait handle that can be passed to mir_wait_for,
++ *                           or NULL if parameters is invalid.
++ *
++ */
++MirWaitHandle* mir_surface_configure_cursor(MirSurface *surface, MirCursorConfiguration const* parameters);
++
  #ifdef __cplusplus
+ }
  /**@}*/
 === modified file 'include/platform/mir/graphics/cursor.h'
 --- include/platform/mir/graphics/cursor.h	2014-05-06 03:33:05 +0000
 +++ include/platform/mir/graphics/cursor.h	2014-05-09 20:09:14 +0000
@@ -33,7 +33,9 @@
  class Cursor
+ {
  public:
--    virtual void set_image(CursorImage const& cursor_image) = 0;
++    virtual void show(CursorImage const& cursor_image) = 0;
++    virtual void hide() = 0;
++
      virtual void move_to(geometry::Point position) = 0;
  protected:
 === modified file 'src/client/CMakeLists.txt'
 --- src/client/CMakeLists.txt	2014-03-31 14:36:08 +0000
 +++ src/client/CMakeLists.txt	2014-05-09 20:09:14 +0000
@@ -47,6 +47,7 @@
    private.cpp
    mir_screencast.cpp
    mir_screencast_api.cpp
++  mir_cursor_api.cpp
+ )
  add_library(
 === added file 'src/client/cursor_configuration.h'
 --- src/client/cursor_configuration.h	1970-01-01 00:00:00 +0000
 +++ src/client/cursor_configuration.h	2014-05-09 20:09:14 +0000
@@ -0,0 +1,31 @@
++/*
++ * Copyright © 2014 Canonical Ltd.
++ *
++ * This program is free software: you can redistribute it and/or modify
++ * it under the terms of the GNU Lesser General Public License version 3 as
++ * published by the Free Software Foundation.
++ *
++ * This program is distributed in the hope that it will be useful,
++ * but WITHOUT ANY WARRANTY; without even the implied warranty of
++ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
++ * GNU Lesser General Public License for more details.
++ *
++ * You should have received a copy of the GNU Lesser General Public License
++ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
++ *
++ * Authored by: Robert Carr <robert.carr@canonical.com>
++ */
++
++#ifndef MIR_CLIENT_CURSOR_CONFIGURATION_H_
++#define MIR_CLIENT_CURSOR_CONFIGURATION_H_
++
++#include <string>
++
++// Parameters for configuring the apperance and behavior of the system cursor.
++// Will grow to include cursors specified by raw RGBA data, hotspots, etc...
++struct MirCursorConfiguration
++{
++    std::string name;
++};
++
++#endif // MIR_CLIENT_CURSOR_CONFIGURATION_H_
 === added file 'src/client/mir_cursor_api.cpp'
 --- src/client/mir_cursor_api.cpp	1970-01-01 00:00:00 +0000
 +++ src/client/mir_cursor_api.cpp	2014-05-09 20:09:14 +0000
@@ -0,0 +1,44 @@
++/*
++ * Copyright © 2014 Canonical Ltd.
++ *
++ * This program is free software: you can redistribute it and/or modify it
++ * under the terms of the GNU Lesser General Public License version 3,
++ * as published by the Free Software Foundation.
++ *
++ * This program is distributed in the hope that it will be useful,
++ * but WITHOUT ANY WARRANTY; without even the implied warranty of
++ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
++ * GNU Lesser General Public License for more details.
++ *
++ * You should have received a copy of the GNU Lesser General Public License
++ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
++ *
++ * Authored by: Robert Carr <robert.carr@canonical.com>
++ */
++
++#include "mir_toolkit/mir_cursor_configuration.h"
++#include "cursor_configuration.h"
++
++char const *const mir_default_cursor_name = "default";
++char const *const mir_disabled_cursor_name = nullptr;
++
++
++void mir_cursor_configuration_destroy(MirCursorConfiguration *cursor)
++{
++    delete cursor;
++}
++
++MirCursorConfiguration* mir_cursor_configuration_from_name(char const* name)
++{
++    try
++    {
++        auto c = new MirCursorConfiguration;
++        c->name = std::string(name);
++
++        return c;
++    }
++    catch (...)
++    {
++        return nullptr;
++    }
++}
 === modified file 'src/client/mir_surface.cpp'
 --- src/client/mir_surface.cpp	2014-03-06 06:05:17 +0000
 +++ src/client/mir_surface.cpp	2014-05-09 20:09:14 +0000
@@ -20,6 +20,7 @@
  #include "mir/frontend/client_constants.h"
  #include "client_buffer.h"
  #include "mir_surface.h"
++#include "cursor_configuration.h"
  #include "mir_connection.h"
  #include "mir/input/input_receiver_thread.h"
  #include "mir/input/input_platform.h"
@@ -297,6 +298,24 @@
      return *accelerated_window;
+ }
++MirWaitHandle* MirSurface::configure_cursor(MirCursorConfiguration const* cursor)
++{
++    mp::CursorSetting setting;
++
++    {
++        std::unique_lock<decltype(mutex)> lock(mutex);
++        setting.mutable_surfaceid()->CopyFrom(surface.id());
++        if (cursor->name != "")
++            setting.set_name(cursor->name.c_str());
++    }
++
++    configure_cursor_wait_handle.expect_result();
++    server.configure_cursor(0, &setting, &void_response,
++        google::protobuf::NewCallback(this, &MirSurface::on_configured));
++
++    return &configure_cursor_wait_handle;
++}
++
  MirWaitHandle* MirSurface::configure(MirSurfaceAttrib at, int value)
+ {
      std::unique_lock<decltype(mutex)> lock(mutex);
 === modified file 'src/client/mir_surface.h'
 --- src/client/mir_surface.h	2014-03-11 15:46:03 +0000
 +++ src/client/mir_surface.h	2014-05-09 20:09:14 +0000
@@ -87,6 +87,8 @@
      MirWaitHandle* configure(MirSurfaceAttrib a, int value);
      int attrib(MirSurfaceAttrib a) const;
++
++    MirWaitHandle* configure_cursor(MirCursorConfiguration const* cursor);
      void set_event_handler(MirEventDelegate const* delegate);
      void handle_event(MirEvent const& e);
@@ -109,11 +111,13 @@
      mir::protobuf::DisplayServer::Stub & server;
      mir::protobuf::Surface surface;
      std::string error_message;
++    mir::protobuf::Void void_response;
      MirConnection* const connection;
      MirWaitHandle create_wait_handle;
      MirWaitHandle next_buffer_wait_handle;
      MirWaitHandle configure_wait_handle;
++    MirWaitHandle configure_cursor_wait_handle;
      std::shared_ptr<mir::client::MemoryRegion> secured_region;
      std::shared_ptr<mir::client::ClientBufferDepository> buffer_depository;
 === modified file 'src/client/mir_surface_api.cpp'
 --- src/client/mir_surface_api.cpp	2014-03-31 14:36:08 +0000
 +++ src/client/mir_surface_api.cpp	2014-05-09 20:09:14 +0000
@@ -245,6 +245,22 @@
      return surf ? surf->attrib(mir_surface_attrib_swapinterval) : -1;
+ }
++MirWaitHandle* mir_surface_configure_cursor(MirSurface* surface, MirCursorConfiguration const* cursor)
++{
++    MirWaitHandle *result = nullptr;
++
++    try
++    {
++        if (surface)
++            result = surface->configure_cursor(cursor);
++    }
++    catch (...)
++    {
++    }
++
++    return result;
++}
++
  /* Debug functions */
  uint32_t mir_debug_surface_current_buffer_id(MirSurface* surface)
 === modified file 'src/platform/graphics/mesa/cursor.cpp'
 --- src/platform/graphics/mesa/cursor.cpp	2014-05-06 03:33:05 +0000
 +++ src/platform/graphics/mesa/cursor.cpp	2014-05-09 20:09:14 +0000
@@ -80,7 +80,7 @@
          buffer(gbm),
          current_configuration(current_configuration)
+ {
--    set_image(*initial_image);
++    show(*initial_image);
      show_at_last_known_position();
+ }
@@ -90,7 +90,7 @@
      hide();
+ }
--void mgm::Cursor::set_image(CursorImage const& cursor_image)
++void mgm::Cursor::show(CursorImage const& cursor_image)
+ {
      auto const& size = cursor_image.size();
 === modified file 'src/platform/graphics/mesa/cursor.h'
 --- src/platform/graphics/mesa/cursor.h	2014-05-06 03:33:05 +0000
 +++ src/platform/graphics/mesa/cursor.h	2014-05-09 20:09:14 +0000
@@ -68,12 +68,12 @@
      ~Cursor() noexcept;
--    void set_image(CursorImage const& cursor_image) override;
++    void show(CursorImage const& cursor_image) override;
++    void hide() override;
      void move_to(geometry::Point position);
      void show_at_last_known_position();
--    void hide();
  private:
      enum ForceCursorState { UpdateState, ForceState };
 === modified file 'src/server/frontend/protobuf_message_processor.cpp'
 --- src/server/frontend/protobuf_message_processor.cpp	2014-05-06 16:54:43 +0000
 +++ src/server/frontend/protobuf_message_processor.cpp	2014-05-09 20:09:14 +0000
@@ -174,6 +174,10 @@
+         {
              invoke(this, display_server.get(), &protobuf::DisplayServer::release_screencast, invocation);
+         }
++        else if ("configure_cursor" == invocation.method_name())
++        {
++            invoke(this, display_server.get(), &protobuf::DisplayServer::configure_cursor, invocation);
++        }
          else if ("new_fds_for_trusted_clients" == invocation.method_name())
+         {
              invoke(this, display_server.get(), &protobuf::DisplayServer::new_fds_for_trusted_clients, invocation);
 === modified file 'src/server/frontend/session_mediator.cpp'
 --- src/server/frontend/session_mediator.cpp	2014-05-09 11:01:53 +0000
 +++ src/server/frontend/session_mediator.cpp	2014-05-09 20:09:14 +0000
@@ -416,6 +416,16 @@
      done->Run();
+ }
++void mf::SessionMediator::configure_cursor(
++    google::protobuf::RpcController*,
++    mir::protobuf::CursorSetting const* /* cursor_request */,
++    mir::protobuf::Void* /* void_response */,
++    google::protobuf::Closure* /* done */)
++{
++    // TODO: Pass cursor settings down to surface.
++    BOOST_THROW_EXCEPTION(std::logic_error("Cursor API not yet implemented"));
++}
++
  void mf::SessionMediator::new_fds_for_trusted_clients(
      ::google::protobuf::RpcController* ,
      ::mir::protobuf::SocketFDRequest const* parameters,
 === modified file 'src/server/frontend/session_mediator.h'
 --- src/server/frontend/session_mediator.h	2014-05-06 16:54:43 +0000
 +++ src/server/frontend/session_mediator.h	2014-05-09 20:09:14 +0000
@@ -123,7 +123,12 @@
      void screencast_buffer(google::protobuf::RpcController*,
                             const mir::protobuf::ScreencastId*,
                             mir::protobuf::Buffer*,
--                           google::protobuf::Closure* done) override;
++                           google::protobuf::Closure* done);
++
++    void configure_cursor(google::protobuf::RpcController*,
++                          mir::protobuf::CursorSetting const*,
++                          mir::protobuf::Void*,
++                          google::protobuf::Closure* done);
      /* Platform specific requests */
      void drm_auth_magic(google::protobuf::RpcController* controller,
 === modified file 'src/shared/protobuf/mir_protobuf.proto'
 --- src/shared/protobuf/mir_protobuf.proto	2014-05-06 16:54:43 +0000
 +++ src/shared/protobuf/mir_protobuf.proto	2014-05-09 20:09:14 +0000
@@ -175,6 +175,12 @@
    optional string error = 127;
+ }
++message CursorSetting {
++  required SurfaceId surfaceid = 1;
++  // No name is interpreted as disabled cursor.
++  optional string name = 2;
++}
++
  message SocketFDRequest {
    required int32 number = 1;
+ }
@@ -206,5 +212,7 @@
    rpc create_screencast(ScreencastParameters) returns (Screencast);
    rpc screencast_buffer(ScreencastId) returns (Buffer);
    rpc release_screencast(ScreencastId) returns (Void);
++
++  rpc configure_cursor(CursorSetting) returns (Void);
    rpc new_fds_for_trusted_clients(SocketFDRequest) returns (SocketFD);
+ }
 === modified file 'tests/acceptance-tests/CMakeLists.txt'
 --- tests/acceptance-tests/CMakeLists.txt	2014-05-07 14:00:57 +0000
 +++ tests/acceptance-tests/CMakeLists.txt	2014-05-09 20:09:14 +0000
@@ -31,6 +31,7 @@
    test_protobuf.cpp
    test_client_screencast.cpp
    test_client_surface_swap_buffers.cpp
++  test_client_cursor_api.cpp
    test_trust_session_helper.cpp
    ${GENERATED_PROTOBUF_SRCS}
    ${GENERATED_PROTOBUF_HDRS}
 === added file 'tests/acceptance-tests/test_client_cursor_api.cpp'
 --- tests/acceptance-tests/test_client_cursor_api.cpp	1970-01-01 00:00:00 +0000
 +++ tests/acceptance-tests/test_client_cursor_api.cpp	2014-05-09 20:09:14 +0000
@@ -0,0 +1,449 @@
++/*
++ * Copyright © 2013 Canonical Ltd.
++ *
++ * This program is free software: you can redistribute it and/or modify
++ * it under the terms of the GNU General Public License version 3 as
++ * published by the Free Software Foundation.
++ *
++ * This program is distributed in the hope that it will be useful,
++ * but WITHOUT ANY WARRANTY; without even the implied warranty of
++ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
++ * GNU General Public License for more details.
++ *
++ * You should have received a copy of the GNU General Public License
++ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
++ *
++ * Authored by: Robert Carr <robert.carr@canonical.com>
++ */
++
++#include "mir/graphics/cursor.h"
++#include "mir/graphics/cursor_image.h"
++#include "mir/graphics/cursor_images.h"
++
++#include "mir_toolkit/mir_client_library.h"
++
++#include "mir_test/fake_event_hub.h"
++#include "mir_test/fake_shared.h"
++#include "mir_test/event_factory.h"
++#include "mir_test/wait_condition.h"
++#include "mir_test_framework/display_server_test_fixture.h"
++#include "mir_test_framework/input_testing_server_configuration.h"
++#include "mir_test_framework/input_testing_client_configuration.h"
++#include "mir_test_framework/declarative_placement_strategy.h"
++#include "mir_test_framework/cross_process_sync.h"
++
++#include <gtest/gtest.h>
++#include <gmock/gmock.h>
++
++#include <assert.h>
++
++namespace mg = mir::graphics;
++namespace ms = mir::scene;
++namespace msh = mir::shell;
++namespace mis = mir::input::synthesis;
++namespace geom = mir::geometry;
++
++namespace mt = mir::test;
++namespace mtf = mir_test_framework;
++
++namespace
++{
++
++struct MockCursor : public mg::Cursor
++{
++    MOCK_METHOD1(show, void(mg::CursorImage const&));
++    MOCK_METHOD0(hide, void());
++
++    MOCK_METHOD1(move_to, void(geom::Point));
++};
++
++struct NamedCursorImage : public mg::CursorImage
++{
++    NamedCursorImage(std::string const& name)
++        : cursor_name(name)
++    {
++    }
++
++    void const* as_argb_8888() const { return nullptr; }
++    geom::Size size() const { return geom::Size{}; }
++
++    std::string const cursor_name;
++};
++
++struct StubCursorImages : public mg::CursorImages
++{
++   std::shared_ptr<mg::CursorImage> image(std::string const& name, geom::Size const& /* size */)
++   {
++       return std::make_shared<NamedCursorImage>(name);
++   }
++};
++
++bool cursor_is_named(mg::CursorImage const& i, std::string const& name)
++{
++    auto image = dynamic_cast<NamedCursorImage const*>(&i);
++    assert(image);
++
++    return image->cursor_name == name;
++}
++
++MATCHER(DefaultCursorImage, "")
++{
++    return cursor_is_named(arg, "default");
++}
++
++MATCHER_P(CursorNamed, name, "")
++{
++    return cursor_is_named(arg, name);
++}
++
++struct CursorSettingClient : mtf::TestingClientConfiguration
++{
++    static char const* const mir_test_socket;
++
++    std::string const client_name;
++
++    mtf::CrossProcessSync set_cursor_complete;
++    mtf::CrossProcessSync client_may_exit;
++
++    std::function<void(MirSurface*)> const set_cursor;
++
++    CursorSettingClient(std::string const& client_name,
++                        mtf::CrossProcessSync const& cursor_ready_fence,
++                        mtf::CrossProcessSync const& client_may_exit_fence,
++                        std::function<void(MirSurface*)> const& set_cursor)
++        : client_name(client_name),
++          set_cursor_complete(cursor_ready_fence),
++          client_may_exit(client_may_exit_fence),
++          set_cursor(set_cursor)
++    {
++    }
++
++    void exec() override
++    {
++        auto connection = mir_connect_sync(mir_test_socket,
++                                           client_name.c_str());
++
++        ASSERT_TRUE(connection != NULL);
++        MirSurfaceParameters const request_params =
++            {
++                client_name.c_str(),
++                // For this fixture, we force geometry on server side
++                0, 0,
++                mir_pixel_format_abgr_8888,
++                mir_buffer_usage_hardware,
++                mir_display_output_id_invalid
++            };
++        auto surface = mir_connection_create_surface_sync(connection, &request_params);
++
++        set_cursor(surface);
++        set_cursor_complete.signal_ready();
++
++        client_may_exit.wait_for_signal_ready_for();
++
++        mir_surface_release_sync(surface);
++        mir_connection_release(connection);
++    }
++};
++
++char const* const CursorSettingClient::mir_test_socket = mtf::test_socket_file().c_str();
++
++
++typedef unsigned ClientCount;
++struct CursorTestServerConfiguration : mtf::InputTestingServerConfiguration
++{
++    std::shared_ptr<ms::PlacementStrategy> placement_strategy;
++    mtf::CrossProcessSync client_ready_fence;
++    mtf::CrossProcessSync client_may_exit_fence;
++    int const number_of_clients;
++
++    std::function<void(MockCursor&, mt::WaitCondition&)> const expect_cursor_states;
++    std::function<void(CursorTestServerConfiguration*)> const synthesize_cursor_motion;
++
++    MockCursor cursor;
++
++    CursorTestServerConfiguration(mtf::SurfaceGeometries surface_geometries_by_name,
++                                  mtf::SurfaceDepths surface_depths_by_name,
++                                  mtf::CrossProcessSync client_ready_fence,
++                                  mtf::CrossProcessSync client_may_exit_fence,
++                                  ClientCount const number_of_clients,
++                                  std::function<void(MockCursor&, mt::WaitCondition&)> const& expect_cursor_states,
++                                  std::function<void(CursorTestServerConfiguration*)> const& synthesize_cursor_motion)
++        : placement_strategy(
++              std::make_shared<mtf::DeclarativePlacementStrategy>(InputTestingServerConfiguration::the_placement_strategy(),
++                  surface_geometries_by_name, surface_depths_by_name)),
++          client_ready_fence(client_ready_fence),
++          client_may_exit_fence(client_may_exit_fence),
++          number_of_clients(number_of_clients),
++          expect_cursor_states(expect_cursor_states),
++          synthesize_cursor_motion(synthesize_cursor_motion)
++    {
++    }
++
++    std::shared_ptr<ms::PlacementStrategy> the_placement_strategy() override
++    {
++        return placement_strategy;
++    }
++
++    std::shared_ptr<mg::Cursor> the_cursor() override
++    {
++        return mt::fake_shared(cursor);
++    }
++
++    std::shared_ptr<mg::CursorImages> the_cursor_images() override
++    {
++        return std::make_shared<StubCursorImages>();
++    }
++
++    void inject_input()
++    {
++        using namespace ::testing;
++
++        for (int i = 1; i < number_of_clients + 1; i++)
++            EXPECT_EQ(i, client_ready_fence.wait_for_signal_ready_for());
++
++        mt::WaitCondition expectations_satisfied;
++
++        // Clear any states applied during server initialization.
++        Mock::VerifyAndClearExpectations(&cursor);
++        expect_cursor_states(cursor, expectations_satisfied);
++
++        synthesize_cursor_motion(this);
++        expectations_satisfied.wait_for_at_most_seconds(60);
++
++        EXPECT_CALL(cursor, show(_)).Times(AnyNumber()); // Client shutdown
++        for (int i = 0; i < number_of_clients; i++)
++            client_may_exit_fence.signal_ready();
++    }
++};
++
++}
++
++// TODO: A lot of common code setup in these tests could be moved to
++// a fixture.
++using TestClientCursorAPI = BespokeDisplayServerTestFixture;
++
++// In this set we create a 1x1 client surface at the point (1,0). The client requests to disable the cursor
++// over this surface. Since the cursor starts at (0,0) we when we move the cursor by (1,0) thus causing it
++// to enter the bounds of the first surface, we should observe it being disabled.
++// TODO: Enable
++TEST_F(TestClientCursorAPI, DISABLED_client_may_disable_cursor_over_surface)
++{
++    using namespace ::testing;
++
++    std::string const test_client_name = "1";
++    mtf::SurfaceGeometries client_geometries;
++    client_geometries[test_client_name] = geom::Rectangle{geom::Point{geom::X{1}, geom::Y{0}},
++                                                          geom::Size{geom::Width{1}, geom::Height{1}}};
++
++    mtf::CrossProcessSync client_ready_fence, client_may_exit_fence;
++
++    CursorTestServerConfiguration server_conf(
++        client_geometries, mtf::SurfaceDepths(),
++        client_ready_fence, client_may_exit_fence,
++        ClientCount{1},
++        [](MockCursor& cursor, mt::WaitCondition& expectations_satisfied)
++        {
++            EXPECT_CALL(cursor, hide()).Times(1)
++                .WillOnce(mt::WakeUp(&expectations_satisfied));
++        },
++        [](CursorTestServerConfiguration *server)
++        {
++            server->fake_event_hub->synthesize_event(mis::a_motion_event().with_movement(1, 0));
++        });
++    launch_server_process(server_conf);
++
++    CursorSettingClient client_conf(test_client_name, client_ready_fence, client_may_exit_fence,
++        [](MirSurface *surface)
++        {
++            // Disable cursor
++            mir_wait_for(mir_surface_configure_cursor(surface,
++                mir_cursor_configuration_from_name(mir_disabled_cursor_name)));
++        });
++    launch_client_process(client_conf);
++}
++
++// TODO: Enable
++TEST_F(TestClientCursorAPI, DISABLED_cursor_restored_when_leaving_surface)
++{
++    using namespace ::testing;
++
++    std::string const test_client_name = "1";
++    mtf::SurfaceGeometries client_geometries;
++    client_geometries[test_client_name] = geom::Rectangle{geom::Point{geom::X{1}, geom::Y{0}},
++                                                          geom::Size{geom::Width{1}, geom::Height{1}}};
++
++    mtf::CrossProcessSync client_ready_fence, client_may_exit_fence;
++
++    CursorTestServerConfiguration server_conf(
++        client_geometries, mtf::SurfaceDepths(),
++        client_ready_fence, client_may_exit_fence,
++        ClientCount{1},
++        [](MockCursor& cursor, mt::WaitCondition& expectations_satisfied)
++        {
++            InSequence seq;
++            EXPECT_CALL(cursor, hide()).Times(1);
++            EXPECT_CALL(cursor, show(DefaultCursorImage())).Times(1)
++                .WillOnce(mt::WakeUp(&expectations_satisfied));
++        },
++        [](CursorTestServerConfiguration *server)
++        {
++            server->fake_event_hub->synthesize_event(mis::a_motion_event().with_movement(1, 0));
++            server->fake_event_hub->synthesize_event(mis::a_motion_event().with_movement(2,0));
++        });
++    launch_server_process(server_conf);
++
++    CursorSettingClient client_conf(test_client_name, client_ready_fence, client_may_exit_fence,
++        [](MirSurface *surface)
++        {
++            // Disable cursor
++            mir_wait_for(mir_surface_configure_cursor(surface,
++                mir_cursor_configuration_from_name(mir_disabled_cursor_name)));
++        });
++    launch_client_process(client_conf);
++}
++
++// TODO: Enable
++TEST_F(TestClientCursorAPI, DISABLED_cursor_changed_when_crossing_surface_boundaries)
++{
++    using namespace ::testing;
++
++    static std::string const test_client_name_1 = "1";
++    static std::string const test_client_name_2 = "2";
++    static std::string const client_1_cursor = test_client_name_1;
++    static std::string const client_2_cursor = test_client_name_2;
++
++    mtf::SurfaceGeometries client_geometries;
++    client_geometries[test_client_name_1] = geom::Rectangle{geom::Point{geom::X{1}, geom::Y{0}},
++                                                          geom::Size{geom::Width{1}, geom::Height{1}}};
++    client_geometries[test_client_name_2] = geom::Rectangle{geom::Point{geom::X{2}, geom::Y{0}},
++                                                            geom::Size{geom::Width{1}, geom::Height{1}}};
++
++    mtf::CrossProcessSync client_ready_fence, client_may_exit_fence;
++
++    CursorTestServerConfiguration server_conf(
++        client_geometries, mtf::SurfaceDepths(),
++        client_ready_fence, client_may_exit_fence,
++        ClientCount{2},
++        [](MockCursor& cursor, mt::WaitCondition& expectations_satisfied)
++        {
++            InSequence seq;
++            EXPECT_CALL(cursor, show(CursorNamed(client_1_cursor))).Times(1);
++            EXPECT_CALL(cursor, show(CursorNamed(client_2_cursor))).Times(1)
++                .WillOnce(mt::WakeUp(&expectations_satisfied));
++        },
++        [](CursorTestServerConfiguration *server)
++        {
++            server->fake_event_hub->synthesize_event(mis::a_motion_event().with_movement(1, 0));
++            server->fake_event_hub->synthesize_event(mis::a_motion_event().with_movement(1, 0));
++        });
++    launch_server_process(server_conf);
++
++    CursorSettingClient client1_conf(test_client_name_1, client_ready_fence, client_may_exit_fence,
++        [](MirSurface *surface)
++        {
++            mir_wait_for(mir_surface_configure_cursor(surface, mir_cursor_configuration_from_name(client_1_cursor.c_str())));
++        });
++    launch_client_process(client1_conf);
++    CursorSettingClient client2_conf(test_client_name_2, client_ready_fence, client_may_exit_fence,
++        [](MirSurface *surface)
++        {
++            // Disable cursor
++            mir_wait_for(mir_surface_configure_cursor(surface, mir_cursor_configuration_from_name(client_2_cursor.c_str())));
++        });
++    launch_client_process(client2_conf);
++}
++
++// TODO: Enable
++TEST_F(TestClientCursorAPI, DISABLED_cursor_request_taken_from_top_surface)
++{
++    using namespace ::testing;
++
++    static std::string const test_client_name_1 = "1";
++    static std::string const test_client_name_2 = "2";
++    static std::string const client_1_cursor = test_client_name_1;
++    static std::string const client_2_cursor = test_client_name_2;
++
++    mtf::SurfaceGeometries client_geometries;
++    client_geometries[test_client_name_1] = geom::Rectangle{geom::Point{geom::X{1}, geom::Y{0}},
++                                                          geom::Size{geom::Width{1}, geom::Height{1}}};
++    client_geometries[test_client_name_1] = geom::Rectangle{geom::Point{geom::X{1}, geom::Y{0}},
++                                                          geom::Size{geom::Width{1}, geom::Height{1}}};
++    mtf::SurfaceDepths client_depths;
++    client_depths[test_client_name_1] = ms::DepthId{0};
++    client_depths[test_client_name_2] = ms::DepthId{1};
++
++    mtf::CrossProcessSync client_ready_fence, client_may_exit_fence;
++
++    CursorTestServerConfiguration server_conf(
++        client_geometries, client_depths,
++        client_ready_fence, client_may_exit_fence,
++        ClientCount{2},
++        [](MockCursor& cursor, mt::WaitCondition& expectations_satisfied)
++        {
++            InSequence seq;
++            EXPECT_CALL(cursor, show(CursorNamed(client_2_cursor))).Times(1)
++                .WillOnce(mt::WakeUp(&expectations_satisfied));
++        },
++        [](CursorTestServerConfiguration *server)
++        {
++            server->fake_event_hub->synthesize_event(mis::a_motion_event().with_movement(1, 0));
++        });
++    launch_server_process(server_conf);
++
++    CursorSettingClient client1_conf(test_client_name_1, client_ready_fence, client_may_exit_fence,
++        [](MirSurface *surface)
++        {
++            mir_wait_for(mir_surface_configure_cursor(surface, mir_cursor_configuration_from_name(client_1_cursor.c_str())));
++        });
++    launch_client_process(client1_conf);
++    CursorSettingClient client2_conf(test_client_name_2, client_ready_fence, client_may_exit_fence,
++        [](MirSurface *surface)
++        {
++            mir_wait_for(mir_surface_configure_cursor(surface, mir_cursor_configuration_from_name(client_1_cursor.c_str())));
++        });
++
++    launch_client_process(client2_conf);
++}
++
++// TODO: Enable
++TEST_F(TestClientCursorAPI, DISABLED_cursor_request_applied_without_cursor_motion)
++{
++    using namespace ::testing;
++    static std::string const test_client_name_1 = "1";
++    static std::string const client_1_cursor = test_client_name_1;
++
++    mtf::SurfaceGeometries client_geometries;
++    client_geometries[test_client_name_1] = geom::Rectangle{geom::Point{geom::X{0}, geom::Y{0}},
++                                                          geom::Size{geom::Width{1}, geom::Height{1}}};
++
++    mtf::CrossProcessSync client_ready_fence, client_may_exit_fence;
++    static mtf::CrossProcessSync client_may_change_cursor;
++
++    CursorTestServerConfiguration server_conf(
++        client_geometries, mtf::SurfaceDepths(),
++        client_ready_fence, client_may_exit_fence,
++        ClientCount{1},
++        [](MockCursor& cursor, mt::WaitCondition& expectations_satisfied)
++        {
++            InSequence seq;
++            EXPECT_CALL(cursor, show(CursorNamed(client_1_cursor))).Times(1);
++            EXPECT_CALL(cursor, hide()).Times(1)
++                .WillOnce(mt::WakeUp(&expectations_satisfied));
++        },
++        [](CursorTestServerConfiguration * /* server */)
++        {
++            client_may_change_cursor.signal_ready();
++        });
++    launch_server_process(server_conf);
++
++    CursorSettingClient client1_conf(test_client_name_1, client_ready_fence, client_may_exit_fence,
++        [&client_ready_fence](MirSurface *surface)
++        {
++            client_ready_fence.signal_ready();
++            client_may_change_cursor.wait_for_signal_ready_for();
++            mir_wait_for(mir_surface_configure_cursor(surface, mir_cursor_configuration_from_name(client_1_cursor.c_str())));
++            mir_wait_for(mir_surface_configure_cursor(surface,
++                mir_cursor_configuration_from_name(mir_disabled_cursor_name)));
++        });
++    launch_client_process(client1_conf);
++}
 === modified file 'tests/unit-tests/graphics/mesa/test_cursor.cpp'
 --- tests/unit-tests/graphics/mesa/test_cursor.cpp	2014-05-06 03:33:05 +0000
 +++ tests/unit-tests/graphics/mesa/test_cursor.cpp	2014-05-09 20:09:14 +0000
@@ -246,7 +246,7 @@
          std::make_shared<StubCursorImage>()};
+ }
--TEST_F(MesaCursorTest, set_cursor_writes_to_bo)
++TEST_F(MesaCursorTest, show_cursor_writes_to_bo)
+ {
      using namespace testing;
@@ -257,10 +257,10 @@
      EXPECT_CALL(mock_gbm, gbm_bo_write(mock_gbm.fake_gbm.bo, StubCursorImage::image_data, cursor_size_bytes));
--    cursor.set_image(image);
++    cursor.show(image);
+ }
--TEST_F(MesaCursorTest, set_cursor_throws_on_incorrect_size)
++TEST_F(MesaCursorTest, show_cursor_throws_on_incorrect_size)
+ {
      using namespace testing;
@@ -275,7 +275,7 @@
      };
      EXPECT_THROW(
--        cursor.set_image(InvalidlySizedCursorImage());
++        cursor.show(InvalidlySizedCursorImage());
      , std::logic_error);
+ }