Merge into development-branch : cursor-spike-phase-1-resubmit-resubmit-resubmit-hello-out-there : Code : Mir

Status:	Merged
Approved by:	Robert Carr on 2014-04-29
Approved revision:	no longer in the source branch.
Merged at revision:	1584
Proposed branch:	lp:~mir-team/mir/cursor-spike-phase-1-resubmit-resubmit-resubmit-hello-out-there
Merge into:	lp:mir
Diff against target:	944 lines (+348/-130) 24 files modified examples/render_surfaces.cpp (+2/-78) include/platform/mir/graphics/cursor.h (+4/-1) include/platform/mir/graphics/cursor_image.h (+46/-0) include/platform/mir/graphics/display.h (+3/-2) include/server/mir/default_server_configuration.h (+10/-0) include/server/mir/graphics/cursor_images.h (+53/-0) include/test/mir_test_doubles/mock_display.h (+1/-1) include/test/mir_test_doubles/null_display.h (+4/-1) src/platform/graphics/android/android_display.cpp (+2/-2) src/platform/graphics/android/android_display.h (+1/-1) src/platform/graphics/mesa/cursor.cpp (+12/-8) src/platform/graphics/mesa/cursor.h (+5/-2) src/platform/graphics/mesa/display.cpp (+13/-8) src/platform/graphics/mesa/display.h (+3/-2) src/server/default_server_configuration.cpp (+4/-7) src/server/graphics/CMakeLists.txt (+1/-0) src/server/graphics/builtin_cursor_images.cpp (+52/-0) src/server/graphics/builtin_cursor_images.h (+51/-0) src/server/graphics/default_configuration.cpp (+35/-0) src/server/graphics/nested/nested_display.cpp (+2/-2) src/server/graphics/nested/nested_display.h (+1/-1) src/server/graphics/offscreen/display.cpp (+1/-1) src/server/graphics/offscreen/display.h (+1/-1) tests/unit-tests/graphics/mesa/test_cursor.cpp (+41/-12)
To merge this branch:	bzr merge lp:~mir-team/mir/cursor-spike-phase-1-resubmit-resubmit-resubmit-hello-out-there
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration	2014-04-23	Approve on 2014-04-29
Alan Griffiths		2014-04-23	Abstain on 2014-04-28
Daniel van Vugt		2014-04-23	Abstain on 2014-04-28
Kevin DuBois (community)		2014-04-23	Abstain on 2014-04-24
Alberto Aguirre (community)		2014-04-23	Approve on 2014-04-24
Chris Halse Rogers		2014-04-23	Approve on 2014-04-23
Review via email: mp+216961@code.launchpad.net

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-03-13: Posted in a previous version of this proposal

#

FAILED: Continuous integration, rev:1472
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1055/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1217
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1215/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/799/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/787/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/792/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/800/console

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-03-13: Posted in a previous version of this proposal

#

Please resubmit and move black_arrow.c using "bzr mv" so it's not bloating the diff.

review: Needs Resubmitting

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-03-13: Posted in a previous version of this proposal

#

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-03-14: Posted in a previous version of this proposal

#

black_arrow.c isn't an important file to track the history of, I admit. But in general we should "bzr mv" to ensure files don't lose history when they move. And doing a move would omit its contents from the diff too, halving the size of the merge proposal.

Revision history for this message

Kevin DuBois (kdub) wrote on 2014-03-15: Posted in a previous version of this proposal

#

> black_arrow.c isn't an important file to track the history of, I admit. But in
> general we should "bzr mv" to ensure files don't lose history when they move.
> And doing a move would omit its contents from the diff too, halving the size
> of the merge proposal.

+1

Revision history for this message

Kevin DuBois (kdub) wrote on 2014-03-15: Posted in a previous version of this proposal

#

16 +#include <assert.h>
17 +
1551 +#include <assert.h>
1552 +
needed?

CursorImage might better be a mir::scene::PixelBuffer

1568 + const int cursor_width = 64;
1569 + const int cursor_height = 64;
const ordering and magic numbers, maybe black_arrow.width/height

1665 + static const geometry::Size default_cursor_size = {geometry::Width{64},
1666 + geometry::Height{64}};
const ordering and why static

mg::BuiltinCursorRepository::lookup_cursor()
I'd expect a repository to be something more like a std::map, this seems to allocate a new object during every lookup.

1652 +mir::DefaultServerConfiguration::the_cursor()
can we assume we only have one cursor? The nexus 4 could potentially have 4 cursors that are hardware accelerated.

+ return the_display()->create_hardware_cursor(the_default_cursor_image());
This is a pre-existing issue, but it strikes me that its very strange that the display interface allocates the hardware cursor, when the display interface does not even allocate the framebuffers. A hardware cursor is a special buffer, it should be allocated from mg::GraphicBufferAllocator

review: Needs Fixing

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-03-17: Posted in a previous version of this proposal

#

Also remember to move black_arrow.xcf to wherever black_arrow.c goes.

Revision history for this message

Robert Carr (robertcarr) wrote on 2014-03-17: Posted in a previous version of this proposal

#

Fixed assert and constant stuff.

>> mg::BuiltinCursorRepository::lookup_cursor()
>> I'd expect a repository to be something more like a std::map, this seems to allocate a new object during
>> every lookup.

Probably for the themed cursors, it didnt seem worth defining the ctor in this case since its only called once, but I decided there is no harm and changed it to define a ctor so we can just return the same instance.

Strictly this code is probably less performant ;) as there is no runtime difference (only called once) and the non defaulted ctor increases binary size. Still I dont think there is any harm anyway.

>> CursorImage might better be a mir::scene::PixelBuffer

Ive changed raw_argb to use as_argb_8888 as in the style of PixelBuffer.

For now the PixelBuffer interface is obviously a bit specific:
/*
* Interface for extracting the pixels from a graphics::Buffer.
*/

Conceptually they both implement some sort of mir::graphics::Pixels/mir::graphics::Image interface, I dont think we need to spend time on it until there is a consumer though.

>> can we assume we only have one cursor? The nexus 4 could potentially have 4 cursors that are hardware
>> accelerated.

No we cant assume this forever. For now though I have a pretty simple target, mostly for the desktop preview/xmir:

A single themeable cursor controllable by client API (controllable, as in: theme, enable/disable).

I dont think we have any requirements for any multi cursor stuff now.

>> + return the_display()->create_hardware_cursor(the_default_cursor_image());
>> This is a pre-existing issue, but it strikes me that its very strange that the display interface
>> allocates the hardware cursor, when the display interface does not even allocate the framebuffers. A
>> hardware cursor is a special buffer, it should be allocated from mg::GraphicBufferAllocator

I agree its weird. Ill fix up this in one of the next branches as I clean up the GBM cursor a little more. This branch is mostly about encapsulating the cursor image and cursor in the server configuration so that work on cursor implementation independent code can continue (cursor theme loader, client API for cursor image control) in parallel to the cursor backend work.

Thanks!

Fixed assert and constant stuff.

>> mg::BuiltinCursorRepository::lookup_cursor()
>> I'd expect a repository to be something more like a std::map, this seems to allocate a new object during 
>> every lookup.

Probably for the themed cursors, it didnt seem worth defining the ctor in this case since its only called once, but I decided there is no harm and changed it to define a ctor so we can just return the same instance.

Strictly this code is probably less performant ;) as there is no runtime difference (only called once) and the non defaulted ctor increases binary size. Still I dont think there is any harm anyway.

>> CursorImage might better be a mir::scene::PixelBuffer

Ive changed raw_argb to use as_argb_8888 as in the style of PixelBuffer.

For now the PixelBuffer interface is obviously a bit specific:
/*
 * Interface for extracting the pixels from a graphics::Buffer.      
 */

Conceptually they both implement some sort of mir::graphics::Pixels/mir::graphics::Image interface, I dont think we need to spend time on it until there is a consumer though.

>> can we assume we only have one cursor? The nexus 4 could potentially have 4 cursors that are hardware 
>> accelerated.

No we cant assume this forever. For now though I have a pretty simple target, mostly for the desktop preview/xmir:

A single themeable cursor controllable by client API (controllable, as in: theme, enable/disable).

I dont think we have any requirements for any multi cursor stuff now.

>> + return the_display()->create_hardware_cursor(the_default_cursor_image());
>> This is a pre-existing issue, but it strikes me that its very strange that the display interface 
>> allocates the hardware cursor, when the display interface does not even allocate the framebuffers. A 
>> hardware cursor is a special buffer, it should be allocated from mg::GraphicBufferAllocator

I agree its weird. Ill fix up this in one of the next branches as I clean up the GBM cursor a little more. This branch is mostly about encapsulating the cursor image and cursor in the server configuration so that work on cursor implementation independent code can continue (cursor theme loader, client API for cursor image control) in parallel to the cursor backend work.

Thanks!

Revision history for this message

Robert Carr (robertcarr) wrote on 2014-03-17: Posted in a previous version of this proposal

#

Tried to revert black_arrow.c and then move it but bzr doesnt like that...ust a sec

Revision history for this message

Robert Carr (robertcarr) wrote on 2014-03-17: Posted in a previous version of this proposal

#

Fixed black_arrow.c...was there a better way to do that than recreate the entire branch?

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-03-17: Posted in a previous version of this proposal

#

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-03-18: Posted in a previous version of this proposal

#

No diff, no code?!

review: Needs Resubmitting

Revision history for this message

Chris Halse Rogers (raof) wrote on 2014-03-18: Posted in a previous version of this proposal

#

Apart from the lack of code… assuming the diff is roughly equivalent to the previous submission:

I'd prefer create_hardware_cursor to return a unique_ptr; it's easy to go unique→shared, but not possible the other way. It also makes it clear that the factory doesn't have a reference. (This applies to basically all our factory methods, to be fair)

----
2. Enable client API for setting cursor (using a similar style to display mediator, focused client can set the cursor, including disabling it)

This is the wrong way approach, I think. The focused client should not be able to set the cursor; a client should be able to set how the cursor appears over their surface.

Yes, that requires work down the input mines...

While I'm on the subject of cursor handling, I think in the post-HWC-enablement world we should be leaving it to the compositor. I'll catch you on IRC perhaps.

Revision history for this message

Robert Carr (robertcarr) wrote on 2014-03-18: Posted in a previous version of this proposal

#

Not sure what the missing diff was about...

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-03-18: Posted in a previous version of this proposal

#

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Kevin DuBois (kdub) wrote on 2014-03-18: Posted in a previous version of this proposal

#

#1

> Probably for the themed cursors, it didnt seem worth defining the ctor in this case since its only called
> once, but I decided there is no harm and changed it to define a ctor so we can just return the same instance.
>
> Strictly this code is probably less performant ;) as there is no runtime difference (only called once) and the
> non defaulted ctor increases binary size. Still I dont think there is any harm anyway.

I'm not so concerned about the performance either here. It seems in the superseded review the make_shared() was in lookup_cursor, and in this one its in the constructor of BuiltinCursorRepository, so this is okay by me.

Revision history for this message

Kevin DuBois (kdub) wrote on 2014-03-18: Posted in a previous version of this proposal

#

#2
I guess my main point I have to make is that a hardware cursor, should be modelled just like a buffer in the parts of the system that don't care about hardware bits. (much like mg::Buffer could have any sort of buffer backing it, with different flags and use cases)

On mesa, mg::BufferAllocator::allocate_buffer(mg::BufferUsage::hardware_cursor) could give the little specialised region reserved for the hardware buffer. On android, it could return any old a hardware-capable buffer. At the time of posting, if the mesa stuff finds the flags on the topmost layer is the 'hardware cursor' buffer, then it can use the hardware cursor.

With the ms::PixelBuffer point, it seems funny to have:
class CursorImage
{
virtual void const* as_argb_8888() = 0;
virtual geometry::Size size() = 0;
};

and

class PixelBuffer
{
  virtual void fill_from(graphics::Buffer& buffer) = 0;
  virtual void const* as_argb_8888() = 0;
  virtual geometry::Size size() const = 0;
  virtual geometry::Stride stride() const = 0;
}

which both model software access to a buffer.

Maybe this means:
mg::SoftwareBuffer
{
    virtual void const* as_argb_8888() = 0;
    virtual geometry::Size size() const = 0;
    virtual geometry::Stride stride() const = 0;
};
ms::PixelBuffer : public mg::SoftwareBuffer
{
    virtual void fill_from(graphics::Buffer& buffer) = 0;
}

Revision history for this message

Chris Halse Rogers (raof) wrote on 2014-03-19: Posted in a previous version of this proposal

#

+1 on Kevin's point #2, with the additional point that there isn't any special region reserved for the hardware cursor; AFAIK the only restriction is that the buffer must be 64x64 (on most cards, 128x128 is an option on newer radeons, for example).

The cursor image really is just a buffer.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-03-20: Posted in a previous version of this proposal

#

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

review: Approve (continuous-integration)

Revision history for this message

Robert Carr (robertcarr) wrote on 2014-03-20: Posted in a previous version of this proposal

#

Download full text (5.0 KiB)

I am not sure sure about the cursor image/pixel buffer stuff, the cursor image is something that you will upload to a buffer (a buffer which while not being a special buffer in the buffer sense is given special status in the server), pixel buffer instead models something which you read OUT of a buffer. Of course it seems there is some sort of mg::Pixels base.

RAOF and I had a discussion on the Cursor is just a buffer thread:
22:42 < RAOF> racarr_: You're thinking cursors?
22:43 -!- FunnyLookinHat [~funnylook@ubuntu/member/funnylookinhat] has quit [Quit: Leaving]
22:44 < racarr_> RAOF: Yes. and wondering wwhat you were thinking
22:45 < RAOF> Cool.
22:45 < racarr_> as to the small bits that were on launchpad...uh
22:45 < racarr_> it was a mild hallucination that doing it per connection was going to be easier and also a useful step towards per surface so I ust imagined doing that first
22:45 < RAOF> Hah!
22:45 < racarr_> but have now decided it was silly and am on to mir_surface_set_cursor
22:45 < racarr_> and um...
22:45 < RAOF> It probably _would_ be easier to do per-connection :)
22:46 < racarr_> well but you have to write code that ends up not being useful
22:46 < racarr_> so its pretty pointless
22:46 < RAOF> Yeah.
22:46 < RAOF> It's not a step towards per-surface cursors, which is what you need.
22:46 < racarr_> Also re:create_hardware_cursor
22:46 < racarr_> and unique/shared here it actually is shared
22:46 < racarr_> with the GBM::Display which manipulates it in case of pause/stop, etc.
22:47 < racarr_> which is mostly why it hasnt been moved to some more appropriate place of allocation yet
22:47 < RAOF> Urgh.
22:47 < racarr_> err pause/resume
22:47 < racarr_> to restore the position iirc
22:48 < RAOF> Yeah, because it's all wrong! :)
22:48 < racarr_> lol
22:48 < racarr_> what are your thoughts :D
22:49 < RAOF> So, what I think should be the final state is: The cursor is a renderable in the scene, and the compositor is responsible for getting it on the screen.
22:50 < RAOF> Under GBM the compositor will check if the topmost renderable in the scene is (a) the existing hardware cursor or (b) usable as a HW cursor, and do the API
twiddling necessary.
22:50 < racarr_> ok
22:51 < RAOF> Under Android the HWC machinery will translate into basically a HW cursor anyway
22:51 < racarr_> Yes
22:51 < RAOF> So I think all the Cursor API wants to be is basically a pointer at a specific renderable.
22:52 < racarr_> hmm well I mean
22:52 < RAOF> If we want a server cursor API at all, I guess?
22:52 < racarr_> it seems like it still needs to be promoted to the server configuration and
22:52 < racarr_> from the perspective of mg::Cursor
22:52 < racarr_> RAOF: Well we are baking window management in to mir
22:53 < racarr_> so the surface cursor selection has to work by default
22:53 < racarr_> and input should be hooked up to the cursor, etc
22:53 < RAOF> Oh, yeah.
22:53 < RAOF> That was just rambling as to what extent we need a Mir-private Cursor API.
22:53 < RAOF> Or whether the various bits could just prod the relevant Renderable.
22:55 < RAOF> It's probably cleaner to have an actual interface to prod, though.
22:55 < racarr_> hmm
22:55...

I am not sure sure about the cursor image/pixel buffer stuff, the cursor image is something that you will upload to a buffer (a buffer which while not being a special buffer in the buffer sense is given special status in the server), pixel buffer instead models something which you read OUT of a buffer. Of course it seems there is some sort of mg::Pixels base.

RAOF and I had a discussion on the Cursor is just a buffer thread:
22:42 < RAOF> racarr_: You're thinking cursors?
22:43 -!- FunnyLookinHat [~funnylook@ubuntu/member/funnylookinhat] has quit [Quit: Leaving]
22:44 < racarr_> RAOF: Yes. and wondering wwhat you were thinking
22:45 < RAOF> Cool.
22:45 < racarr_> as to the small bits that were on launchpad...uh
22:45 < racarr_> it was a mild hallucination that doing it per connection was going to be easier and also a useful step towards per surface so I ust imagined doing that first
22:45 < RAOF> Hah!
22:45 < racarr_> but have now decided it was silly and am on to mir_surface_set_cursor
22:45 < racarr_> and um...
22:45 < RAOF> It probably _would_ be easier to do per-connection :)
22:46 < racarr_> well but you have to write code that ends up not being useful
22:46 < racarr_> so its pretty pointless
22:46 < RAOF> Yeah.
22:46 < RAOF> It's not a step towards per-surface cursors, which is what you need.
22:46 < racarr_> Also re:create_hardware_cursor
22:46 < racarr_> and unique/shared here it actually is shared
22:46 < racarr_> with the GBM::Display which manipulates it in case of pause/stop, etc.
22:47 < racarr_> which is mostly why it hasnt been moved to some more appropriate place of allocation yet
22:47 < RAOF> Urgh.
22:47 < racarr_> err pause/resume
22:47 < racarr_> to restore the position iirc
22:48 < RAOF> Yeah, because it's all wrong! :)
22:48 < racarr_> lol
22:48 < racarr_> what are your thoughts :D
22:49 < RAOF> So, what I think should be the final state is: The cursor is a renderable in the scene, and the compositor is responsible for getting it on the screen.
22:50 < RAOF> Under GBM the compositor will check if the topmost renderable in the scene is (a) the existing hardware cursor or (b) usable as a HW cursor, and do the API 
              twiddling necessary.
22:50 < racarr_> ok
22:51 < RAOF> Under Android the HWC machinery will translate into basically a HW cursor anyway
22:51 < racarr_> Yes
22:51 < RAOF> So I think all the Cursor API wants to be is basically a pointer at a specific renderable.
22:52 < racarr_> hmm well I mean
22:52 < RAOF> If we want a server cursor API at all, I guess?
22:52 < racarr_> it seems like it still needs to be promoted to the server configuration and
22:52 < racarr_> from the perspective of mg::Cursor
22:52 < racarr_> RAOF: Well we are baking window management in to mir
22:53 < racarr_> so the surface cursor selection has to work by default
22:53 < racarr_> and input should be hooked up to the cursor, etc
22:53 < RAOF> Oh, yeah.
22:53 < RAOF> That was just rambling as to what extent we need a Mir-private Cursor API.
22:53 < RAOF> Or whether the various bits could just prod the relevant Renderable.
22:55 < RAOF> It's probably cleaner to have an actual interface to prod, though.
22:55 < racarr_> hmm
22:55 < racarr_> yes it is for tests
22:55 < racarr_> at least
22:55 < RAOF> Right.
22:55 < racarr_> its nice for example in the tests im doing to ust mock the_cursor on the server side
22:55 < racarr_> and expect that set_image is called various ways as
22:55 < racarr_> I generate motion events
22:55 < racarr_> etc
22:55 < RAOF> Yeah.
22:56 < racarr_> so, it should also be a renderable though
22:56 < racarr_> and rather than this whole weird bit where
22:56 < racarr_> the GBMDisplay holds on to it and 
22:56 < racarr_> such
22:56 < racarr_> the default shell becomes responsible for putting it in the scene I guess?
22:56 < RAOF> Right.
22:56 < RAOF> I think so, yes.
22:57 < RAOF> Unless we want to bake in ‘the cursor is always on top of everything’.
22:57 < RAOF> Which I'm ambivalent about.
22:57 < racarr_> mm I mean maybe its the deault shell, deault server configuration
22:57 < racarr_> somewhere there is a line that is like hey take the_cursor and put it on the_top_of_everything
22:58 < racarr_> lol Mir::DefaultServerConfiguration::the_top_of_everything
22:58 < racarr_> that seems like a good interface
22:58 < RAOF> :)
22:59 < RAOF> I think having it default to on top but overridable in the same way we let everything be overridable is correct.
22:59 < racarr_> ok  I think that makes sense. I think I can do that when I implement a software cursor 
22:59 < racarr_> I want to finish this client API/theme loader stuff first
22:59 < racarr_> its gotten kind of large diffish lol
23:00 < RAOF> Yeah, theme loader stuff is a reasonable first step.
23:01 < RAOF> That way we can actually load different cursors :)
23:03 < racarr_> Im hoping to finish the client API bits and the cursor changer response to input thingamawidget tomorroww so thatw
23:03 < racarr_> we can at least enable/disable cursor and use that for xmir/desktop preview
23:03 < racarr_> then will finish the theme loader branch by friday

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-03-21: Posted in a previous version of this proposal

#

(3) render_surfaces now segfaults on the first movement of the cursor:
(gdb) bt
#0 0x00007f0a4d0703de in mir::DefaultServerConfiguration::DefaultCursorListener::cursor_moved_to (this=0x96f9f8, abs_x=6, abs_y=0)
    at /home/dan/bzr/mir/tmp.cursor/src/server/default_server_configuration.cpp:101
#1 0x00007f0a4d117292 in mir::input::android::PointerController::notify_listener (this=0x11a3150)
    at /home/dan/bzr/mir/tmp.cursor/src/server/input/android/android_pointer_controller.cpp:56
#2 0x00007f0a4d117597 in mir::input::android::PointerController::setPosition (
    this=0x11a3150, new_x=6, new_y=-5)
    at /home/dan/bzr/mir/tmp.cursor/src/server/input/android/android_pointer_controller.cpp:112

(4) I agree mir::graphics::Buffer half fits what we need for CursorImage. Perhaps we need to keep the commonality in Buffer and move the surface-only stuff into a "SurfaceBuffer" or "ClientBuffer". Then CursorImage becomes a simple implementation of Buffer. Although such a change could come any time later...

(5) CursorRepository is a bad abstraction and the confusing word "Repository" is the reason. A repository of cursors is not a familiar concept even to a graphics programmer. I think the cursor theme should be the main object there. So instead of:
img = repository->lookup_cursor(theme_name, cursor_name, size);
a better design would be:
img = theme->get_image(cursor_name, size);

Remember to link the relevant bugs when ready:
https://bugs.launchpad.net/mir/+bugs?field.tag=cursor

review: Needs Fixing

Revision history for this message

Kevin DuBois (kdub) wrote on 2014-03-21: Posted in a previous version of this proposal

#

+1 for RAOF's thoughts at 22:49-22:51 and for Daniel's points.
As for the cursor always being on top of everything, it seems a bit more flexible not to bake that into mir and have that be a decision of the shell stuff. (I could see someone wanting a cursor to go under a pop-up notification maybe)

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-03-24: Posted in a previous version of this proposal

#

Another issue:

(6) I think it would scale/port better to more toolkits for us to choose the cursor type using an enum. Otherwise we will find ourselves having to translate toolkit requirements into strings on the client side, which is less efficient and not strongly typed like an enum is.
251 + std::string const& cursor_name,
Something like this would be good:
img = cursor_theme->get_image(mir_cursor_crosshair, size);

Revision history for this message

Robert Carr (robertcarr) wrote on 2014-03-24: Posted in a previous version of this proposal

#

3. Fixed. Whoops! Lost track during refactoring and was returning a nullptr rather than a null cursor instance.
4. Im still not sure what we mean here. mir::graphics::Buffer is a GPU side buffer (which could be mapped). mir::graphics::CursorImage is just a way to access (CPU side, for example for upload to a buffer), the pixels of a loaded cursor image (loaded from the on disk theme). Maybe you mean mir::graphics::PixelBuffer. In this case I agree there is a common mg::Image/mg::Pixels interface they implement (raw_argb, size). Of course we shouldnt get overexcited about making classes implement interfaces until we have someone who wishes to consume them through that interface.
5. What loads the Cursor theme then?
6. I chose string/string on the suggestion of RAOF...and that afaik it is what toolkits will be best prepared to deal with (working like other cursor APIs).

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-03-24: Posted in a previous version of this proposal

#

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

review: Approve (continuous-integration)

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-03-25: Posted in a previous version of this proposal

#

(3) render_surfaces: Confirmed it no longer crashes. I think this is a bad example though. Setting a NullCursor is not how anyone will hide the cursor in reality. That should be a separate show/hide function. Maybe as an example you could just change NullCursor to some kind of SimpleCursor (but visible).

(4) Yes, Buffer is presently a GPU-side buffer. My suggestion describes how it might be generalized to represent any type of buffer though. Not yet useful, but plausibly in future. Not a blocker.

(5) Good point. But "Repository" still feels very counter-intuitive. Perhaps it's the graphics platform which determines and constructs available themes. Or maybe it's the Display. Or to just be generic, let it be a CursorThemeFactory which produces CursorTheme's, from which you can load a known cursor image.

(6) Converting between strings and enums is not a big deal. However I think there is sufficient evidence that enums are a better fit for plenty of toolkits:
   https://developer.gnome.org/gdk3/stable/gdk3-Cursors.html#GdkCursorType
   http://qt-project.org/doc/qt-5/qt.html#CursorShape-enum
   http://www.opengl.org/resources/libraries/glut/spec3/node28.html#SECTION000513000000000000000
And I think enums are a better fit for apps too, which simply want to say "give me an arrow/crosshair/bottom-right-corner".

review: Needs Fixing

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-03-25: Posted in a previous version of this proposal

#

P.S. Cursor themes don't have to be complicated or even depend on X libraries. We could for example say that a cursor theme is a single image (dimensions WxH) loaded any way you like. And cursor type N can be found at square x=N*H,y=0,w=H,h=H. In that case all you need to produce a cursor theme is an image editor. And no dependencies on legacy X code.

Revision history for this message

Robert Carr (robertcarr) wrote on 2014-03-27: Posted in a previous version of this proposal

#

>> That should be a separate show/hide function. Maybe as an example you could just change NullCursor to some >> kind of SimpleCursor (but visible).

What do you mean by SimpleCursor. Not sure what render_surfaces cursor stuff is intended to be an example of really. A later branch in this spike introduces cursor show/hide as part of the general control API. Maybe change it then?

>> (5) Good point. But "Repository" still feels very counter-intuitive. Perhaps it's the graphics platform
>> which determines and constructs available themes. Or maybe it's the Display. Or to just be generic, let it >> be a CursorThemeFactory which produces CursorTheme's, from which you can load a known cursor image.

Its certainly not the display or graphics platform. In an upcoming branch there is a component which changes the cursor image based on entering/leaving surfaces (and what these surfaces have requested for their cursor). Certainly this component should not depend on the entire Display/Graphics platform interface. Plus the cursor theme loader doesnt vary with graphics platform.

In this case Repository is a little more honest than Factory because ownership of the images is shared with the creator which acts as a cache (or generically: repository) of the loaded images.

>> (6) Converting between strings and enums is not a big deal. However I think there is sufficient evidence >> that enums are a better fit for plenty of toolkits:

Maybe RAOF will have some insight?

Revision history for this message

Chris Halse Rogers (raof) wrote on 2014-03-28: Posted in a previous version of this proposal

#

1) I don't think there's any value in not using existing XCursor themes. There's nothing wrong with them, and it's one less point of friction.

2) I think strings for the cursor-lookup API are going to be a better idea than enums. While toolkits generally provide a set of enums for common cursors, they *also* provide the ability to look up arbitrary cursors. This also lets clients load their own themes with semantically useful identifiers.

Overall, I think this is a step in the right direction, and will approve as such.

Review nits:
185 + virtual std::shared_ptr<Cursor> create_hardware_cursor(std::shared_ptr<CursorImage> const& initial_image) = 0;
Should take a std::shared_ptr<CursorImage const> const& (or a std::unique_ptr, of course ☺), to make it obvious that create_hardware_cursor isn't going to mutate the image.

Relatedly, all the public members of CursorImage and CursorRepository want to be const.

I'd really like it if create_hardware_cursor returned a std::unique_ptr; I think we would gain clarity of access and mutation - and thread-obviousness - if we weren't sharing all our state, all the time :)

review: Needs Fixing

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-03-28: Posted in a previous version of this proposal

#

(3) NullCursor/SimpleCursor: As NullCursor is not a realistic way to hide the cursor (eventually), we should instead demo the functionality being introduced. That is put a few pixels in there and make it a simple visible cursor.

(5) "Repository" is still a very ambiguous word. As such, people (including myself) can't figure out what it is or where such a thing would or should be instantiated. If your class design necessitates ambiguous words then that's a sign you need a different class design.

(6) XCursor themes are a stopgap. They shouldn't drive future architecture. The above links all show toolkits and apps fit better with enums. And we can still translate them so XCursors can be used in the mean time.

review: Needs Fixing

Revision history for this message

Chris Halse Rogers (raof) wrote on 2014-03-28: Posted in a previous version of this proposal

#

I think we disagree about (6) - the toolkits all _support_ the use of enums for common cursors, but they also support the use of strings (and, indeed, arbitrary images). I'm perfectly happy for the client API for this to have two entry points - mir_surface_set_standard_cursor(MirCursorType im_an_enum) and mir_surface_set_cursor_from_theme(MirCursorTheme theme, char const* name).

Going further, I'd like clients to be able to load cursor themes and index into them; in this case, string indices are mandatory (or, at least, really useful).

Revision history for this message

Robert Carr (robertcarr) wrote on 2014-03-31: Posted in a previous version of this proposal

#

>>Review nits:
>>185 + virtual std::shared_ptr<Cursor> create_hardware_cursor(std::shared_ptr<CursorImage> const& initial_image) = 0;
>>Should take a std::shared_ptr<CursorImage const> const& (or a std::unique_ptr, of course ☺), to make it obvious that >>create_hardware_cursor isn't going to mutate the image.

>> Relatedly, all the public members of CursorImage and CursorRepository want to be const.

Fixed

>>1) I don't think there's any value in not using existing XCursor themes. There's nothing wrong with them, and it's one less >>point of friction.

I agree.

----------------------

>> (3) NullCursor/SimpleCursor: As NullCursor is not a realistic way to hide the cursor (eventually), we should instead demo >> the functionality being introduced. That is put a few pixels in there and make it a simple visible cursor.

There is also a simple visible cursor, however the example previously came with an option to totally disable the cursor so I preserve that.

>> (5) "Repository" is still a very ambiguous word. As such, people (including myself) can't figure out what it is or where >> >> such a thing would or should be instantiated. If your class design necessitates ambiguous words then that's a sign you need >> a different class design.

Are you being serious about the word repository or just trying to prove a point re: your opinion on object architecture? I am only aware of two main definitions of repository: 1. A location where things are stored and offered. 2. A burial place. Mulled it over the weekend and do not see the confusion.

Still I am not particular attached to the word. How about CursorLoader as in the most recent revision. This fails to capture the aspect that you share ownership with the loader, but that information is also in the shared_ptr signature so I do not think it is a big loss.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-03-31: Posted in a previous version of this proposal

#

FAILED: Continuous integration, rev:1485
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1207/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1427/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1425/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/1006/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/939/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/944/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1007/console

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-04-01: Posted in a previous version of this proposal

#

(3) ExampleCursorImage doesn't seem to do anything(?). I can just get none (default) or the standard black arrow with --display-cursor=1. Looks like the bug pre-dates this proposal, but it makes the proposed changes untestable.

(5) Yes I am serious about "Repository". Names are important because a well chosen one leads to universal understanding, high productivity and avoids refactoring. Poorly chosen names lead to confusion and thus slow development or even bugs. The word "Repository" I know means where you put or get something. So is the cursor coming or going from a "repository"? It's not clear from the name. A realistic mental model for cursors is that you put it in on the screen, but you get it from a theme. Those are more concrete nouns that will lead to wider understanding and better code reuse/utilization. "CursorLoader" is mildly better, but it makes the mistake of using a verb in the noun, which tends to limit how well multiple people can agree on what it represents. If there is a more concrete noun available like Theme or Factory then I'd prefer we used it.

(6) Per these links:
   https://developer.gnome.org/gdk3/stable/gdk3-Cursors.html#GdkCursorType
   http://qt-project.org/doc/qt-5/qt.html#CursorShape-enum
   http://www.opengl.org/resources/libraries/glut/spec3/node28.html#SECTION000513000000000000000
you can see there's going to be a fair amount of mapping between enums and/or strings. While strings are completely flexible, it's more efficient and simpler to map between enum values. That said, I'm not sure that's a performance/convenience penalty worth blocking on, so won't.

review: Needs Fixing

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-01: Posted in a previous version of this proposal

#

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

review: Approve (continuous-integration)

Revision history for this message

Robert Carr (robertcarr) wrote on 2014-04-01: Posted in a previous version of this proposal

#

CursorLoader->CursorImages

The code in render surfaces was only triggered when input was disabled (to provide...some sort of cursor animation when running examples as non root?!), it seems bad to start mir with input disabled (you cant quit), and this didnt seem to be a particularly important example so Ive removed it.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-01: Posted in a previous version of this proposal

#

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

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-01: Posted in a previous version of this proposal

#

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

review: Approve (continuous-integration)

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-04-02: Posted in a previous version of this proposal

#

(3) NullCursor is not a realistic way to hide the cursor. We should either display a simple custom cursor or not modify render_surfaces at all in this proposal. NullCursor is not a useful example or manual test at all. Because showing nothing means you can't really tell it's working properly. And in reality when you do want "no cursor" you will probably toggle a flag, but not set a NullCursor.

(7) "CursorImages" vs "theme": In my mind cursor images come from a theme, but you're selecting a theme from CursorImages. That sounds backwards and will be hard to understand/extend/use:

304 +class CursorImages
305 +{
306 +public:
307 + /// Looks up the image for a named cursor in a given cursor theme. Cursor names
308 + /// follow the XCursor naming conventions.
309 + virtual std::shared_ptr<CursorImage> lookup_cursor(std::string const& theme_name,
310 + std::string const& cursor_name,
311 + geometry::Size const& size) = 0;
312 +

The most realistic (IMHO) interpretation of "cursor images" is a set of cursor images provided by a theme. In that way the cursor images concept is a subset of the theme, not the superset. Maybe there does exist a class name where this design makes sense, but not "CursorImages".

review: Needs Fixing

Revision history for this message

Chris Halse Rogers (raof) wrote on 2014-04-02: Posted in a previous version of this proposal

#

This suggests:

class CursorTheme
{
public:

virtual std::unique_ptr<CursorImage> lookup_cursor(std::string const& name, geometry::Size const& size) const = 0;
}

Plus

class XCursorTheme : public CursorTheme
{
public:
XCursorTheme(std::string const& name);
}
?

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-04-03: Posted in a previous version of this proposal

#

Chris: Yeah I suggested CursorTheme above on 2014-03-21. And Robert rightfully pointed out you still need a factory to abstract the construction of a CursorTheme. But I think that's probably OK. And it has the added benefit of not implying that all your themes are loaded and available simultaneously, because they probably shouldn't be.

On the other hand, I'm not 100% against them being a single class as it is designed right now. But the proposed names of "CursorRepository" and "CursorImages" don't describe the design clearly and unambiguously enough.

Revision history for this message

Chris Halse Rogers (raof) wrote on 2014-04-03: Posted in a previous version of this proposal

#

CursorImageLoader?

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-04-14: Posted in a previous version of this proposal

#

875 +void const* StubCursorImage::image_data = reinterpret_cast<void*>(0x1729);

It is non-portable to reinterpret 0x1729 as a pointer? Can't we use a sensible value. E.g.

void const* StubCursorImage::image_data = &image_data;

review: Needs Fixing

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-04-14: Posted in a previous version of this proposal

#

reading through the comments there seem to be three issues in play:

1. How to model not having a cursor.
2. The interface name for sourcing images ("CursorImages" in the current revision)
3. Whether cursor themes are textual labels or objects in their own right.

1. How to model not having a cursor.
===================================

I like NullObject pattern for a lot of things but in this case something is wrong about it.

Looking at the mg::Cursor interface it isn't a cursor, it is a sink for cursor updates (motion and image changes). Having a Null version of an event sink makes more sense than a null cursor.

Maybe mg::Cursor has the wrong name and should be mg::CursorModifier. NullCursorModifier doesn't seem strange.

2. The interface name for sourcing images
=========================================

I'm not entirely convinced by "Loader" as that loading is an implementation detail. In fact, the "Loader" implementations in the MP don't load anything.

The current name "CursorImages" doesn't seem too bad in itself, but this doesn't read well:

"the_cursor_images()->lookup_cursor("default", "arrow", default_cursor_size);"

How about something like:

"the_cursor_store()->set_cursor(the_cursor_modifier(), "default", "arrow", default_cursor_size);"

3. Whether cursor themes are textual labels or objects
======================================================

I don't know enough about cursors to have an opinion.

Revision history for this message

Robert Carr (robertcarr) wrote on 2014-04-14: Posted in a previous version of this proposal

#

But 1729 was a perfectly reasonable choice, its the smallest number expressible as the sum of 2 cubes in 2 ways...ok but it's gone.

Renamed lookup cursor:
the_cursor_images->image("default", "arrow", default_cursor_size) -> CursorImage

im pretty happy with this one.

DO you think cursor store is better? Is it better than the original repository? (Repository sounds slightly better to me than store...not sure why...use of store like that isnt so common in american english I guess).

I am trying not to deal with the mg::Cursor (I think you are on to something with a different name, but modifier doesnt strike me) until I establish the cursor API and can think about it a little more clearly.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-14: Posted in a previous version of this proposal

#

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

review: Approve (continuous-integration)

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2014-04-15: Posted in a previous version of this proposal

#

I know you are dying to hear more naming suggestions so here's mine :) - given the current CursorImages interface it leads itself to CursorImageProvider.

which leads to
the_cursor_image_provider->image_from(...)

Repository for me implies having size/add/remove methods, store - yeah not a fan, I suppose it's the US context - why am I buying cursors? :)

Cursor and NullCursor make sense to me - I think the definition of cursor here makes sense - a movable image. A NullCursor then makes sense - an empty image that doesn't move.

CursorModifier would not make sense to me, as then I would have the following questions - what "cursor" is it modifying? Who implements cursor?

I'm not hung up on the names though functionally it looks fine to me.

review: Approve

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-04-15: Posted in a previous version of this proposal

#

+1 that the existing mir::graphics::Cursor makes sense. It's a nice simple interface for implementing a real working cursor. That makes it rather clear we are only dealing with cursor "themes" and/or "images" in this proposal. Although where "hotspot" will live is not year clear.

At the highest level I think the shell will have a current active (perhaps fixed) cursor theme. So that's something you should be able to query to get an image:
    shell->current_cursor_theme()->get_image("arrow");
or
    config->the_cursor_theme()->get_image("crosshairs");
And an app/surface could have its own too:
    img = surface->cursor_theme()->get_image("custom_cursor_thingy");

Who or what changes the theme is an exercise for later. But first we need to define (like in the above examples) where/what the theme exists. If you change the theme, who do you change it for? I think you change it for:
(a) the whole shell; or
(b) a specific surface/app.

Also, once you set a theme, how do you refer to it? In the current design that would be a string. I think that's inefficient. It's a set of images with metadata like hotspot coordinates. The theme sounds like an object to me.

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-04-15: Posted in a previous version of this proposal

#

Well if themes are meaningful as objects why don't we want code like:

auto const new_theme = ???.cursor_theme("default");

    cursor->change_theme_to(new_theme);
    ...
    cursor->change_size_to(new_size);
    ...
    cursor->change_image_to("arrow");

or

cursor->change_to(new_theme, new_size, "arrow");

I.e. I don't get why the client code is messing with getting and setting CursorImages instead of asking the cursor to talk to its current theme.

review: Needs Information

Revision history for this message

Robert Carr (robertcarr) wrote on 2014-04-15: Posted in a previous version of this proposal

#

I don't think a cursor theme is particularly useful as an object. I dont see cursor theme changes being handled by the shell swapping out cursor theme objects, rather I would say when the system is reconfigured the default cursor theme changes.

As far as I can see the main point for selecting alternative cursor themes, is for example a game may install an alternate cursor theme, and refer to it by name, as a more convenient form of uploading custom cursors.

Revision history for this message

Chris Halse Rogers (raof) wrote on 2014-04-16: Posted in a previous version of this proposal

#

This looks to me like a step in the right direction.

Random nit:
309 + virtual std::shared_ptr<CursorImage> image(std::string const& theme_name,
→ virtual std::shared_ptr<CursorImage const> image(...)
☺

Re: cursor themes as objects. The bare minimum required is the ability to associate a CursorImage with a surface, and this CursorImage is going to come from the client - either by reference “give me MIR_CURSOR_CROSSHAIR”, “give me "fancy arrow" from "MySuperGameTheme"”, or by value “set my cursor to <blob of ARGB data>”.

Whether or not we want to *have* a global default cursor theme - so that clients which have specified MIR_CURSOR_CROSSHAIR get their cursor image updated automatically should that theme change - is, I think, something for a later MP.

I would be quite happy with Mir not providing any means to create CursorImages *at all*. It's something that all desktop shells are going to need to do, so I don't think it's out of scope for Mir, but equally it's not something that needs to interact with the rest of Mir at all. We could perfectly happily have a purely client-side library that handles the cursor loading/themes/whatever and hands CursorImages to the server.

review: Approve

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-04-17: Posted in a previous version of this proposal

#

Alan's concern concerned me too when I read it:
"I.e. I don't get why the client code is messing with getting and setting CursorImages instead of asking the cursor to talk to its current theme."
But I don't see where client code is messing with CursorImages. Unless you mistake the render_surfaces demo for a client? It's not a client... or a server :)

(7) I'm trying to not be the blocker here and responding to all new comments every day. But I still can't get past the repository/CursorImages stuff. Perhaps as raof alludes to, we shouldn't use the word "theme" at all. Mir doesn't (yet) need themes. So maybe just remove the parameter:
std::string const& theme_name,
and worry about the "theme" question later. Then it matters much less what "CursorImages" is called.

(3) NullCursor still needs fixing IMHO per comments on 2014-04-02.

review: Needs Fixing

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-04-17: Posted in a previous version of this proposal

#

> Alan's concern concerned me too when I read it:
> "I.e. I don't get why the client code is messing with getting and setting
> CursorImages instead of asking the cursor to talk to its current theme."
> But I don't see where client code is messing with CursorImages. Unless you
> mistake the render_surfaces demo for a client? It's not a client... or a
> server :)

The phrase "client code" shouldn't be misinterpreted like this. We're discussing interface design and in that context it means code that is a client of that interface.

Revision history for this message

Chris Halse Rogers (raof) wrote on 2014-04-22: Posted in a previous version of this proposal

#

So, given that ‘cursor->change_image_to(<blob of ARGB data>)’ needs to be supported, which roughly implies that CursorImage needs to exist, what blockers, if any, are there for this to land?

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-04-22: Posted in a previous version of this proposal

#

The blockers are:

(7) Remove the "theme_name" parameter to eliminate all confusion and debate over theming. If no one mentions the word "theme" for now then there's less to argue about.

(3) NullCursor still needs fixing per comments on 2014-04-02.

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-04-22: Posted in a previous version of this proposal

#

If we're not messing with the abstraction that mg::Cursor represents then NullCursor makes sense. (I'm unconvinced by the naming as discussed above, but that isn't a blocker for this MP.)

I tend to agree with Daniel about "theme_name" - we don't need any representation of themes (yet).

Is there anything cursor specific about CursorImage? Or is it just a holder for any ARGB image?

review: Needs Fixing

Revision history for this message

Robert Carr (robertcarr) wrote on 2014-04-22: Posted in a previous version of this proposal

#

As it stands, CursorImage is just a holder for any ARGB image, in the future it could grow a hotspot, or come to understand animated cursors, etc....

>> (7) Remove the "theme_name" parameter to eliminate all confusion and debate over theming.
>> If no one mentions the word "theme" for now then there's less to argue about.
----
>>I tend to agree with Daniel about "theme_name" - we don't need any representation of themes (yet).

I think the game cursor theme example is a good use case for themes, we can support it easily. If the only reason is so there "is less" to argue about why waste the time to remove it now and then add it later?

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-04-23: Posted in a previous version of this proposal

#

The use of arbitrary cursor_name strings eliminates the need for any theme right now, even in the game case. Because your game can just:
load_cursor("my_special_menu_cursor");

I'd rather not take the approach of land it now and fix it later. It's not clear any theme specification will be required at all in the foreseeable future. And it just creates design disagreements over what a theme represents.

Revision history for this message

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

#

Le cursor themes are gone.

Revision history for this message

Chris Halse Rogers (raof) wrote on 2014-04-23:

#

I think that's the prudent course.

We probably _will_ want some sort of cursor theme object, but let's defer that until the rest is sorted out; we don't have a consensus for how it should look, and it'll probably be clearer when the rest of the infrastructure is in place.

Still a couple of places where std::shared_ptr<CursorImage> could be std::shared_ptr<CursorImage const>, but I'll certainly not block on that.

review: Approve

Revision history for this message

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

#

FAILED: Continuous integration, rev:1492
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~mir-team/mir/cursor-spike-phase-1-resubmit-resubmit-resubmit-hello-out-there/+merge/216961/+edit-commit-message

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

review: Needs Fixing (continuous-integration)

Revision history for this message

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

#

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

review: Approve (continuous-integration)

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-04-24:

#

(3) render_surfaces.cpp's use of "NullCursor" is not a useful demonstration of anything and should either be removed or turned into a real cursor, per comments on 2014-03-25.

Other than that I'm not going to look too hard or else run the risk of spotting new issues.

review: Needs Fixing

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2014-04-24:

#

> (3) render_surfaces.cpp's use of "NullCursor" is not a useful demonstration of
> anything and should either be removed or turned into a real cursor, per
> comments on 2014-03-25.
>

+1 but not a blocker for me.

> Other than that I'm not going to look too hard or else run the risk of
> spotting new issues.

Uh, isn't that contrary to the purpose of code review?

Anyway, the code looks simple enough, LGTM.

review: Approve

Revision history for this message

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

#

In death may null cursor find the peace it never found in life.

Revision history for this message

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

#

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

review: Approve (continuous-integration)

Revision history for this message

Kevin DuBois (kdub) wrote on 2014-04-24:

#

I'll abstain, until I know more of the future plan in the area of cursors.

Some of the things are pre-existing.

I still think CursorImage is close to:
http://bazaar.launchpad.net/~mir-team/mir/development-branch/view/head:/src/server/scene/pixel_buffer.h

and that there's the pre-existing issue that android has no good way to implement
auto mga::AndroidDisplay::create_hardware_cursor

It makes more sense to me for a cursor buffer to be generated from the_buffer_allocator(). Android would just return any old buffer, mesa could return a sensibly-sized hardware cursor buffer.

shouldn't the destructor for CursorImage{,s} be public?

review: Abstain

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-04-28:

#

I'm not sure you need to change render_surfaces.cpp at all in this proposal any more.

/me shuts eyes

review: Abstain

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-04-28:

#

466 + void set_image(std::shared_ptr<CursorImage const> const& cursor_image);

Are we really sharing ownership of CursorImage? Wouldn't CursorImage const& be enough?

review: Abstain

Revision history for this message

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

#

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

review: Approve (continuous-integration)

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-04-29:

#

Alan makes a good point about set_image. That shared_ptr parameter isn't shared at all. It should just be a reference parameter. But that's a minor non-blocker. I'm sure we make the same mistake elsewhere and haven't fixed it yet.

Revision history for this message

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

#

review: Approve (continuous-integration)

Mir

Merge lp:~mir-team/mir/cursor-spike-phase-1-resubmit-resubmit-resubmit-hello-out-there into lp:mir

Commit message

Description of the change

Preview Diff

Subscribers

 === modified file 'examples/render_surfaces.cpp'
 --- examples/render_surfaces.cpp	2014-04-15 10:19:19 +0000
 +++ examples/render_surfaces.cpp	2014-04-28 23:16:18 +0000
@@ -26,7 +26,6 @@
  #include "mir/geometry/size.h"
  #include "mir/geometry/rectangles.h"
  #include "mir/graphics/buffer_initializer.h"
--#include "mir/graphics/cursor.h"
  #include "mir/graphics/display.h"
  #include "mir/graphics/display_buffer.h"
  #include "mir/graphics/gl_context.h"
@@ -89,62 +88,10 @@
  namespace
+ {
  std::atomic<bool> created{false};
--bool input_is_on = false;
--std::weak_ptr<mg::Cursor> cursor;
--static const uint32_t bg_color = 0x00000000;
--static const uint32_t fg_color = 0xffdd4814;
++
  static const float min_alpha = 0.3f;
--void update_cursor(uint32_t bg_color, uint32_t fg_color)
--{
--    if (auto cursor = ::cursor.lock())
--    {
--        static const int width = 64;
--        static const int height = 64;
--        std::vector<uint32_t> image(height * width, bg_color);
--        for (int i = 0; i != width-1; ++i)
--        {
--            if (i < 16)
--            {
--                image[0 * height + i] = fg_color;
--                image[1 * height + i] = fg_color;
--                image[i * height + 0] = fg_color;
--                image[i * height + 1] = fg_color;
--            }
--            image[i * height + i] = fg_color;
--            image[(i+1) * height + i] = fg_color;
--            image[i * height + i + 1] = fg_color;
--        }
--        cursor->set_image(image.data(), geom::Size{width, height});
--    }
--}
--
--void animate_cursor()
--{
--    if (!input_is_on)
--    {
--        if (auto cursor = ::cursor.lock())
--        {
--            static int cursor_pos = 0;
--            if (++cursor_pos == 300)
--            {
--                cursor_pos = 0;
--
--                static const uint32_t fg_colors[3] = { fg_color, 0xffffffff, 0x3f000000 };
--                static int fg_color = 0;
--
--                if (++fg_color == 3) fg_color = 0;
--
--                update_cursor(bg_color, fg_colors[fg_color]);
--            }
--
--            cursor->move_to(geom::Point{cursor_pos, cursor_pos});
--        }
--    }
--}
--
  char const* const surfaces_to_render = "surfaces-to-render";
--char const* const display_cursor     = "display-cursor";
  ///\internal [StopWatch_tag]
  // tracks elapsed time - for animation.
@@ -278,9 +225,7 @@
              result->add_options()
                  (surfaces_to_render, po::value<int>()->default_value(5),
--                    "Number of surfaces to render")
--                (display_cursor, po::value<bool>()->default_value(false),
--                    "Display test cursor. (If input is disabled it gets animated.)");
++                    "Number of surfaces to render");
              return result;
          }())
@@ -382,7 +327,6 @@
              bool composite()
+             {
                  while (!created) std::this_thread::yield();
--                animate_cursor();
                  stop_watch.stop();
                  if (stop_watch.elapsed_seconds_since_last_restart() >= 1)
+                 {
@@ -501,23 +445,6 @@
          created = true;
+     }
--    bool input_is_on()
--    {
--        return the_options()->get<bool>(mo::enable_input_opt);
--    }
--
--    std::weak_ptr<mg::Cursor> the_cursor()
--    {
--        if (the_options()->get<bool>(display_cursor))
--        {
--            return the_display()->the_cursor();
--        }
--        else
--        {
--            return {};
--        }
--    }
--
  private:
      std::vector<Moveable> moveables;
  };
@@ -532,9 +459,6 @@
      mir::run_mir(conf, [&](mir::DisplayServer&)
+     {
--        cursor = conf.the_cursor();
--
--        input_is_on = conf.input_is_on();
      });
      ///\internal [main_tag]
 === modified file 'include/platform/mir/graphics/cursor.h'
 --- include/platform/mir/graphics/cursor.h	2013-08-28 03:41:48 +0000
 +++ include/platform/mir/graphics/cursor.h	2014-04-28 23:16:18 +0000
@@ -23,14 +23,17 @@
  #include "mir/geometry/size.h"
  #include "mir/geometry/point.h"
++#include <memory>
++
  namespace mir
+ {
  namespace graphics
+ {
++class CursorImage;
  class Cursor
+ {
  public:
--    virtual void set_image(void const* raw_argb, geometry::Size size) = 0;
++    virtual void set_image(std::shared_ptr<CursorImage const> const& cursor_image) = 0;
      virtual void move_to(geometry::Point position) = 0;
  protected:
 === added file 'include/platform/mir/graphics/cursor_image.h'
 --- include/platform/mir/graphics/cursor_image.h	1970-01-01 00:00:00 +0000
 +++ include/platform/mir/graphics/cursor_image.h	2014-04-28 23:16:18 +0000
@@ -0,0 +1,46 @@
++/*
++ * 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_GRAPHICS_CURSOR_IMAGE_H_
++#define MIR_GRAPHICS_CURSOR_IMAGE_H_
++
++#include "mir/geometry/size.h"
++
++namespace mir
++{
++namespace graphics
++{
++class CursorImage
++{
++public:
++    virtual ~CursorImage() = default;
++
++    virtual void const* as_argb_8888() const = 0;
++    virtual geometry::Size size() const = 0;
++
++protected:
++    CursorImage() = default;
++    CursorImage(CursorImage const&) = delete;
++    CursorImage& operator=(CursorImage const&) = delete;
++};
++}
++}
++
++
++#endif /* MIR_GRAPHICS_CURSOR_IMAGE_H_ */
 === modified file 'include/platform/mir/graphics/display.h'
 --- include/platform/mir/graphics/display.h	2014-01-31 07:48:24 +0000
 +++ include/platform/mir/graphics/display.h	2014-04-28 23:16:18 +0000
@@ -31,6 +31,7 @@
  class DisplayBuffer;
  class DisplayConfiguration;
  class Cursor;
++class CursorImage;
  class GLContext;
  class EventHandlerRegister;
@@ -97,9 +98,9 @@
      virtual void resume() = 0;
      /**
--     * Gets the hardware cursor object.
++     * Create a hardware cursor object.
       */
--    virtual std::weak_ptr<Cursor> the_cursor() = 0;
++    virtual std::shared_ptr<Cursor> create_hardware_cursor(std::shared_ptr<CursorImage> const& initial_image) = 0;
      /**
       * Creates a GLContext object that shares resources with the Display's GL context.
 === modified file 'include/server/mir/default_server_configuration.h'
 --- include/server/mir/default_server_configuration.h	2014-04-25 14:38:31 +0000
 +++ include/server/mir/default_server_configuration.h	2014-04-28 23:16:18 +0000
@@ -91,6 +91,9 @@
  class BufferInitializer;
  class DisplayReport;
  class GraphicBufferAllocator;
++class Cursor;
++class CursorImage;
++class CursorImages;
  class GLConfig;
  class GLProgramFactory;
  namespace nested { class HostConnection; }
@@ -159,6 +162,10 @@
       * dependencies of graphics on the rest of the Mir
       *  @{ */
      virtual std::shared_ptr<graphics::DisplayReport> the_display_report();
++    virtual std::shared_ptr<graphics::Cursor> the_cursor();
++    virtual std::shared_ptr<graphics::CursorImage> the_default_cursor_image();
++    virtual std::shared_ptr<graphics::CursorImages> the_cursor_images();
++
      /** @} */
      /** @name compositor configuration - customization
@@ -270,6 +277,9 @@
      CachedPtr<graphics::BufferInitializer> buffer_initializer;
      CachedPtr<graphics::GraphicBufferAllocator> buffer_allocator;
      CachedPtr<graphics::Display>      display;
++    CachedPtr<graphics::Cursor>       cursor;
++    CachedPtr<graphics::CursorImage>  default_cursor_image;
++    CachedPtr<graphics::CursorImages> cursor_images;
      CachedPtr<frontend::ConnectorReport>   connector_report;
      CachedPtr<frontend::ProtobufIpcFactory>  ipc_factory;
 === added file 'include/server/mir/graphics/cursor_images.h'
 --- include/server/mir/graphics/cursor_images.h	1970-01-01 00:00:00 +0000
 +++ include/server/mir/graphics/cursor_images.h	2014-04-28 23:16:18 +0000
@@ -0,0 +1,53 @@
++/*
++ * Copyright © 2014 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>
++ */
++
++
++#ifndef MIR_GRAPHICS_CURSOR_LOADER_H_
++#define MIR_GRAPHICS_CURSOR_LOADER_H_
++
++#include "mir/geometry/size.h"
++
++#include <memory>
++#include <string>
++
++namespace mir
++{
++namespace graphics
++{
++class CursorImage;
++
++/// CursorImages is used to lookup cursor images.
++class CursorImages
++{
++public:
++    virtual ~CursorImages() = default;
++
++    /// Looks up the image for a named cursor. Cursor names
++    /// follow the XCursor naming conventions.
++    virtual std::shared_ptr<CursorImage> image(std::string const& cursor_name,
++        geometry::Size const& size) = 0;
++
++protected:
++    CursorImages() = default;
++    CursorImages(CursorImages const&) = delete;
++    CursorImages& operator=(CursorImages const&) = delete;
++};
++}
++}
++
++#endif /* MIR_GRAPHICS_CURSOR_LOADER_H_ */
 === modified file 'include/test/mir_test_doubles/mock_display.h'
 --- include/test/mir_test_doubles/mock_display.h	2014-01-31 07:48:24 +0000
 +++ include/test/mir_test_doubles/mock_display.h	2014-04-28 23:16:18 +0000
@@ -46,7 +46,7 @@
                                                        graphics::DisplayResumeHandler const&));
      MOCK_METHOD0(pause, void());
      MOCK_METHOD0(resume, void());
--    MOCK_METHOD0(the_cursor, std::weak_ptr<graphics::Cursor>());
++    MOCK_METHOD1(create_hardware_cursor, std::shared_ptr<graphics::Cursor>(std::shared_ptr<graphics::CursorImage> const&));
      MOCK_METHOD0(create_gl_context, std::unique_ptr<graphics::GLContext>());
  };
 === modified file 'include/test/mir_test_doubles/null_display.h'
 --- include/test/mir_test_doubles/null_display.h	2014-01-31 07:48:24 +0000
 +++ include/test/mir_test_doubles/null_display.h	2014-04-28 23:16:18 +0000
@@ -58,7 +58,10 @@
+     }
      void pause() {}
      void resume() {}
--    std::weak_ptr<graphics::Cursor> the_cursor() { return {}; }
++    std::shared_ptr<graphics::Cursor> create_hardware_cursor(std::shared_ptr<graphics::CursorImage> const& /* initial_image */)
++    {
++         return {};
++    }
      std::unique_ptr<graphics::GLContext> create_gl_context()
+     {
          return std::unique_ptr<NullGLContext>{new NullGLContext()};
 === modified file 'src/platform/graphics/android/android_display.cpp'
 --- src/platform/graphics/android/android_display.cpp	2014-04-16 17:07:02 +0000
 +++ src/platform/graphics/android/android_display.cpp	2014-04-28 23:16:18 +0000
@@ -101,9 +101,9 @@
+ {
+ }
--auto mga::AndroidDisplay::the_cursor() -> std::weak_ptr<Cursor>
++auto mga::AndroidDisplay::create_hardware_cursor(std::shared_ptr<mg::CursorImage> const& /* initial_image */) -> std::shared_ptr<Cursor>
+ {
--    return std::weak_ptr<Cursor>();
++    return std::shared_ptr<Cursor>();
+ }
  std::unique_ptr<mg::GLContext> mga::AndroidDisplay::create_gl_context()
 === modified file 'src/platform/graphics/android/android_display.h'
 --- src/platform/graphics/android/android_display.h	2014-04-16 17:07:02 +0000
 +++ src/platform/graphics/android/android_display.h	2014-04-28 23:16:18 +0000
@@ -65,7 +65,7 @@
      void pause();
      void resume();
--    std::weak_ptr<Cursor> the_cursor();
++    std::shared_ptr<Cursor> create_hardware_cursor(std::shared_ptr<CursorImage> const& initial_image);
      std::unique_ptr<graphics::GLContext> create_gl_context();
  private:
 === modified file 'src/platform/graphics/mesa/cursor.cpp'
 --- src/platform/graphics/mesa/cursor.cpp	2014-03-06 06:05:17 +0000
 +++ src/platform/graphics/mesa/cursor.cpp	2014-04-28 23:16:18 +0000
@@ -22,20 +22,21 @@
  #include "kms_output_container.h"
  #include "kms_display_configuration.h"
  #include "mir/geometry/rectangle.h"
++#include "mir/graphics/cursor_image.h"
  #include <boost/exception/errinfo_errno.hpp>
  #include <stdexcept>
  #include <vector>
--namespace mgm = mir::graphics::mesa;
++namespace mg = mir::graphics;
++namespace mgm = mg::mesa;
  namespace geom = mir::geometry;
  namespace
+ {
--#include "black_arrow.c"
--int const width = black_arrow.width;
--int const height = black_arrow.height;
++int const width = 64;
++int const height = 64;
  // Transforms a relative position within the display bounds described by \a rect which is rotated with \a orientation
  geom::Displacement transform(geom::Rectangle const& rect, geom::Displacement const& vector, MirOrientation orientation)
@@ -72,13 +73,14 @@
  mgm::Cursor::Cursor(
      gbm_device* gbm,
      KMSOutputContainer& output_container,
--    std::shared_ptr<CurrentConfiguration> const& current_configuration) :
++    std::shared_ptr<CurrentConfiguration> const& current_configuration,
++    std::shared_ptr<mg::CursorImage> const& initial_image) :
          output_container(output_container),
          current_position(),
          buffer(gbm),
          current_configuration(current_configuration)
+ {
--    set_image(black_arrow.pixel_data, geometry::Size{width, height});
++    set_image(initial_image);
      show_at_last_known_position();
+ }
@@ -88,14 +90,16 @@
      hide();
+ }
--void mgm::Cursor::set_image(const void* raw_argb, geometry::Size size)
++void mgm::Cursor::set_image(std::shared_ptr<CursorImage const> const& cursor_image)
+ {
++    auto const& size = cursor_image->size();
++
      if (size != geometry::Size{width, height})
          BOOST_THROW_EXCEPTION(std::logic_error("No support for cursors that aren't 64x64"));
      auto const count = size.width.as_uint32_t() * size.height.as_uint32_t() * sizeof(uint32_t);
--    if (auto result = gbm_bo_write(buffer, raw_argb, count))
++    if (auto result = gbm_bo_write(buffer, cursor_image->as_argb_8888(), count))
+     {
          BOOST_THROW_EXCEPTION(
              ::boost::enable_error_info(std::runtime_error("failed to initialize gbm buffer"))
 === modified file 'src/platform/graphics/mesa/cursor.h'
 --- src/platform/graphics/mesa/cursor.h	2014-03-06 06:05:17 +0000
 +++ src/platform/graphics/mesa/cursor.h	2014-04-28 23:16:18 +0000
@@ -34,6 +34,8 @@
+ }
  namespace graphics
+ {
++class CursorImage;
++
  namespace mesa
+ {
  class KMSOutputContainer;
@@ -61,11 +63,12 @@
      Cursor(
          gbm_device* device,
          KMSOutputContainer& output_container,
--        std::shared_ptr<CurrentConfiguration> const& current_configuration);
++        std::shared_ptr<CurrentConfiguration> const& current_configuration,
++        std::shared_ptr<CursorImage> const& cursor_image);
      ~Cursor() noexcept;
--    void set_image(const void* raw_argb, geometry::Size size);
++    void set_image(std::shared_ptr<CursorImage const> const& cursor_image);
      void move_to(geometry::Point position);
 === modified file 'src/platform/graphics/mesa/display.cpp'
 --- src/platform/graphics/mesa/display.cpp	2014-04-15 05:31:19 +0000
 +++ src/platform/graphics/mesa/display.cpp	2014-04-28 23:16:18 +0000
@@ -224,7 +224,7 @@
          clear_connected_unused_outputs();
+     }
--    if (cursor) cursor->show_at_last_known_position();
++    if (auto c = cursor.lock()) c->show_at_last_known_position();
+ }
  void mgm::Display::register_configuration_change_handler(
@@ -255,7 +255,7 @@
+ {
      try
+     {
--        if (cursor) cursor->hide();
++        if (auto c = cursor.lock()) c->hide();
          platform->drm.drop_master();
+     }
      catch(std::runtime_error const& e)
@@ -291,12 +291,16 @@
          clear_connected_unused_outputs();
+     }
--    if (cursor) cursor->show_at_last_known_position();
++    if (auto c = cursor.lock()) c->show_at_last_known_position();
+ }
--auto mgm::Display::the_cursor() -> std::weak_ptr<graphics::Cursor>
++auto mgm::Display::create_hardware_cursor(std::shared_ptr<mg::CursorImage> const& initial_image) -> std::shared_ptr<graphics::Cursor>
+ {
--    if (!cursor)
++    // There is only one hardware cursor. We do not keep a strong reference to it in the display though,
++    // if no other component of Mir is interested (i.e. the input stack does not keep a reference to send
++    // position updates) we must be configured not to use a cursor and thusly let it deallocate.
++    std::shared_ptr<mgm::Cursor> locked_cursor = cursor.lock();
++    if (!locked_cursor)
+     {
          class KMSCurrentConfiguration : public CurrentConfiguration
+         {
@@ -317,11 +321,12 @@
              Display& display;
          };
--        cursor = std::make_shared<Cursor>(platform->gbm.device, output_container,
--                                          std::make_shared<KMSCurrentConfiguration>(*this));
++        cursor = locked_cursor = std::make_shared<Cursor>(platform->gbm.device, output_container,
++            std::make_shared<KMSCurrentConfiguration>(*this),
++            initial_image);
+     }
--    return cursor;
++    return locked_cursor;
+ }
  std::unique_ptr<mg::GLContext> mgm::Display::create_gl_context()
 === modified file 'src/platform/graphics/mesa/display.h'
 --- src/platform/graphics/mesa/display.h	2014-03-27 09:46:09 +0000
 +++ src/platform/graphics/mesa/display.h	2014-04-28 23:16:18 +0000
@@ -79,7 +79,7 @@
      void pause();
      void resume();
--    std::weak_ptr<graphics::Cursor> the_cursor();
++    std::shared_ptr<graphics::Cursor> create_hardware_cursor(std::shared_ptr<CursorImage> const& initial_image);
      std::unique_ptr<GLContext> create_gl_context();
  private:
@@ -93,7 +93,8 @@
      std::vector<std::unique_ptr<DisplayBuffer>> display_buffers;
      RealKMSOutputContainer output_container;
      mutable RealKMSDisplayConfiguration current_display_configuration;
--    std::shared_ptr<Cursor> cursor;
++
++    std::weak_ptr<Cursor> cursor;
      std::shared_ptr<GLConfig> const gl_config;
  };
 === modified file 'src/server/default_server_configuration.cpp'
 --- src/server/default_server_configuration.cpp	2014-04-28 12:55:28 +0000
 +++ src/server/default_server_configuration.cpp	2014-04-28 23:16:18 +0000
@@ -92,25 +92,22 @@
+ {
      struct DefaultCursorListener : mi::CursorListener
+     {
--        DefaultCursorListener(std::weak_ptr<mg::Cursor> const& cursor) :
++        DefaultCursorListener(std::shared_ptr<mg::Cursor> const& cursor) :
              cursor(cursor)
+         {
+         }
          void cursor_moved_to(float abs_x, float abs_y)
+         {
--            if (auto c = cursor.lock())
--            {
--                c->move_to(geom::Point{abs_x, abs_y});
--            }
++            cursor->move_to(geom::Point{abs_x, abs_y});
+         }
--        std::weak_ptr<mg::Cursor> const cursor;
++        std::shared_ptr<mg::Cursor> const cursor;
      };
      return cursor_listener(
          [this]() -> std::shared_ptr<mi::CursorListener>
+         {
--            return std::make_shared<DefaultCursorListener>(the_display()->the_cursor());
++            return std::make_shared<DefaultCursorListener>(the_cursor());
          });
+ }
 === modified file 'src/server/graphics/CMakeLists.txt'
 --- src/server/graphics/CMakeLists.txt	2014-04-24 08:42:12 +0000
 +++ src/server/graphics/CMakeLists.txt	2014-04-28 23:16:18 +0000
@@ -9,6 +9,7 @@
    gl_program.cpp
    program_factory.cpp
    surfaceless_egl_context.cpp
++  builtin_cursor_images.cpp
+ )
  add_subdirectory(nested/)
 === renamed file 'src/platform/graphics/mesa/black_arrow.c' => 'src/server/graphics/black_arrow.c'
 === renamed file 'src/platform/graphics/mesa/black_arrow.xcf' => 'src/server/graphics/black_arrow.xcf'
 === added file 'src/server/graphics/builtin_cursor_images.cpp'
 --- src/server/graphics/builtin_cursor_images.cpp	1970-01-01 00:00:00 +0000
 +++ src/server/graphics/builtin_cursor_images.cpp	2014-04-28 23:16:18 +0000
@@ -0,0 +1,52 @@
++/*
++ * Copyright © 2014 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 "black_arrow.c"
++#include "builtin_cursor_images.h"
++
++#include "mir/graphics/cursor_image.h"
++
++namespace mg = mir::graphics;
++namespace geom = mir::geometry;
++
++namespace
++{
++struct BlackArrowCursorImage : public mg::CursorImage
++{
++    void const* as_argb_8888() const
++    {
++        return black_arrow.pixel_data;
++    }
++    geom::Size size() const
++    {
++        return { black_arrow.width, black_arrow.height };
++    }
++};
++}
++
++mg::BuiltinCursorImages::BuiltinCursorImages()
++    : builtin_image(std::make_shared<BlackArrowCursorImage>())
++{
++}
++
++std::shared_ptr<mg::CursorImage> mg::BuiltinCursorImages::image(std::string const& /* cursor_name */,
++    geom::Size const& /* size */)
++{
++    // Builtin repository only has one cursor at a single size.
++    return builtin_image;
++}
 === added file 'src/server/graphics/builtin_cursor_images.h'
 --- src/server/graphics/builtin_cursor_images.h	1970-01-01 00:00:00 +0000
 +++ src/server/graphics/builtin_cursor_images.h	2014-04-28 23:16:18 +0000
@@ -0,0 +1,51 @@
++/*
++ * Copyright © 2014 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>
++ */
++
++
++#ifndef MIR_GRAPHICS_BUILTIN_CURSOR_LOADER_H_
++#define MIR_GRAPHICS_BUILTIN_CURSOR_LOADER_H_
++
++#include "mir/graphics/cursor_images.h"
++
++namespace mir
++{
++namespace graphics
++{
++class CursorImage;
++
++class BuiltinCursorImages : public CursorImages
++{
++public:
++    BuiltinCursorImages();
++    virtual ~BuiltinCursorImages() = default;
++
++    std::shared_ptr<CursorImage> image(std::string const& cursor_name,
++        geometry::Size const& size);
++
++protected:
++    BuiltinCursorImages(BuiltinCursorImages const&) = delete;
++    BuiltinCursorImages& operator=(BuiltinCursorImages const&) = delete;
++
++private:
++    std::shared_ptr<CursorImage> const builtin_image;
++};
++}
++}
++
++
++#endif /* MIR_GRAPHICS_BUILTIN_CURSOR_LOADER_H_ */
 === modified file 'src/server/graphics/default_configuration.cpp'
 --- src/server/graphics/default_configuration.cpp	2014-04-25 12:35:50 +0000
 +++ src/server/graphics/default_configuration.cpp	2014-04-28 23:16:18 +0000
@@ -35,6 +35,8 @@
  #include <boost/throw_exception.hpp>
++#include "builtin_cursor_images.h"
++
  #include <map>
  namespace mg = mir::graphics;
@@ -116,6 +118,39 @@
          });
+ }
++std::shared_ptr<mg::Cursor>
++mir::DefaultServerConfiguration::the_cursor()
++{
++    return cursor(
++        [this]() -> std::shared_ptr<mg::Cursor>
++        {
++            // For now we only support a hardware cursor.
++            return the_display()->create_hardware_cursor(the_default_cursor_image());
++        });
++}
++
++std::shared_ptr<mg::CursorImage>
++mir::DefaultServerConfiguration::the_default_cursor_image()
++{
++    static geometry::Size const default_cursor_size = {geometry::Width{64},
++                                                       geometry::Height{64}};
++    return default_cursor_image(
++        [this]()
++        {
++            return the_cursor_images()->image("arrow", default_cursor_size);
++        });
++}
++
++std::shared_ptr<mg::CursorImages>
++mir::DefaultServerConfiguration::the_cursor_images()
++{
++    return cursor_images(
++        [this]()
++        {
++            return std::make_shared<mg::BuiltinCursorImages>();
++        });
++}
++
  auto mir::DefaultServerConfiguration::the_host_connection()
  -> std::shared_ptr<graphics::nested::HostConnection>
+ {
 === modified file 'src/server/graphics/nested/nested_display.cpp'
 --- src/server/graphics/nested/nested_display.cpp	2014-04-24 08:42:12 +0000
 +++ src/server/graphics/nested/nested_display.cpp	2014-04-28 23:16:18 +0000
@@ -259,10 +259,10 @@
      // TODO If we "own" the cursor then we need to restore it
+ }
--auto mgn::NestedDisplay::the_cursor()->std::weak_ptr<Cursor>
++auto mgn::NestedDisplay::create_hardware_cursor(std::shared_ptr<mg::CursorImage> const& /* initial image */)->std::shared_ptr<Cursor>
+ {
      // TODO Do we "own" the cursor or does the host mir?
--    return std::weak_ptr<Cursor>();
++    return std::shared_ptr<Cursor>();
+ }
  std::unique_ptr<mg::GLContext> mgn::NestedDisplay::create_gl_context()
 === modified file 'src/server/graphics/nested/nested_display.h'
 --- src/server/graphics/nested/nested_display.h	2014-04-24 08:42:12 +0000
 +++ src/server/graphics/nested/nested_display.h	2014-04-28 23:16:18 +0000
@@ -120,7 +120,7 @@
      void pause() override;
      void resume() override;
--    std::weak_ptr<Cursor> the_cursor() override;
++    std::shared_ptr<Cursor> create_hardware_cursor(std::shared_ptr<CursorImage> const& initial_image) override;
      std::unique_ptr<graphics::GLContext> create_gl_context() override;
  private:
 === modified file 'src/server/graphics/offscreen/display.cpp'
 --- src/server/graphics/offscreen/display.cpp	2014-03-26 05:48:59 +0000
 +++ src/server/graphics/offscreen/display.cpp	2014-04-28 23:16:18 +0000
@@ -163,7 +163,7 @@
+ {
+ }
--std::weak_ptr<mg::Cursor> mgo::Display::the_cursor()
++std::shared_ptr<mg::Cursor> mgo::Display::create_hardware_cursor(std::shared_ptr<mg::CursorImage> const& /* initial_image */)
+ {
      return {};
+ }
 === modified file 'src/server/graphics/offscreen/display.h'
 --- src/server/graphics/offscreen/display.h	2014-01-31 07:48:24 +0000
 +++ src/server/graphics/offscreen/display.h	2014-04-28 23:16:18 +0000
@@ -86,7 +86,7 @@
      void pause();
      void resume();
--    std::weak_ptr<Cursor> the_cursor();
++    std::shared_ptr<Cursor> create_hardware_cursor(std::shared_ptr<CursorImage> const& initial_image);
      std::unique_ptr<GLContext> create_gl_context();
  private:
 === modified file 'tests/unit-tests/graphics/mesa/test_cursor.cpp'
 --- tests/unit-tests/graphics/mesa/test_cursor.cpp	2014-03-26 05:48:59 +0000
 +++ tests/unit-tests/graphics/mesa/test_cursor.cpp	2014-04-28 23:16:18 +0000
@@ -21,6 +21,8 @@
  #include "src/platform/graphics/mesa/kms_output_container.h"
  #include "src/platform/graphics/mesa/kms_display_configuration.h"
++#include "mir/graphics/cursor_image.h"
++
  #include "mir_test_doubles/mock_gbm.h"
  #include "mir_test/fake_shared.h"
  #include "mock_kms_output.h"
@@ -31,11 +33,14 @@
  #include <unordered_map>
  #include <algorithm>
++#include <string.h>
++
  namespace mg = mir::graphics;
  namespace mgm = mir::graphics::mesa;
  namespace geom = mir::geometry;
--namespace mtd = mir::test::doubles;
--using mir::test::MockKMSOutput;
++namespace mt = mir::test;
++namespace mtd = mt::doubles;
++using mt::MockKMSOutput;
  namespace
+ {
@@ -196,15 +201,31 @@
      StubKMSDisplayConfiguration conf;
  };
++struct StubCursorImage : public mg::CursorImage
++{
++    void const* as_argb_8888() const
++    {
++        return image_data;
++    }
++    geom::Size size() const
++    {
++        return geom::Size{geom::Width{64}, geom::Height{64}};
++    }
++    static void const* image_data;
++};
++void const* StubCursorImage::image_data = reinterpret_cast<void*>(&StubCursorImage::image_data);
++
  struct MesaCursorTest : public ::testing::Test
+ {
      MesaCursorTest()
          : cursor{mock_gbm.fake_gbm.device, output_container,
--            mir::test::fake_shared(current_configuration)}
++            mt::fake_shared(current_configuration),
++            mt::fake_shared(stub_image)}
+     {
+     }
      StubCurrentConfiguration current_configuration;
++    StubCursorImage stub_image;
      testing::NiceMock<mtd::MockGBM> mock_gbm;
      StubKMSOutputContainer output_container;
      mgm::Cursor cursor;
@@ -221,33 +242,40 @@
                                          GBM_BO_USE_CURSOR_64X64 | GBM_BO_USE_WRITE));
      mgm::Cursor cursor_tmp{mock_gbm.fake_gbm.device, output_container,
--                              std::make_shared<StubCurrentConfiguration>()};
++        std::make_shared<StubCurrentConfiguration>(),
++        std::make_shared<StubCursorImage>()};
+ }
  TEST_F(MesaCursorTest, set_cursor_writes_to_bo)
+ {
      using namespace testing;
--    void* const image{reinterpret_cast<void*>(0x5678)};
++    StubCursorImage image;
      size_t const cursor_side{64};
      geom::Size const cursor_size{cursor_side, cursor_side};
      size_t const cursor_size_bytes{cursor_side * cursor_side * sizeof(uint32_t)};
--    EXPECT_CALL(mock_gbm, gbm_bo_write(mock_gbm.fake_gbm.bo, image, cursor_size_bytes));
++    EXPECT_CALL(mock_gbm, gbm_bo_write(mock_gbm.fake_gbm.bo, StubCursorImage::image_data, cursor_size_bytes));
--    cursor.set_image(image, cursor_size);
++    cursor.set_image(mt::fake_shared(image));
+ }
  TEST_F(MesaCursorTest, set_cursor_throws_on_incorrect_size)
+ {
      using namespace testing;
--    void* const image{reinterpret_cast<void*>(0x5678)};
--    size_t const cursor_side{48};
--    geom::Size const cursor_size{cursor_side, cursor_side};
++    struct InvalidlySizedCursorImage : public StubCursorImage
++    {
++        geom::Size size() const
++        {
++            return invalid_cursor_size;
++        }
++        size_t const cursor_side{48};
++        geom::Size const invalid_cursor_size{cursor_side, cursor_side};
++    };
      EXPECT_THROW(
--        cursor.set_image(image, cursor_size);
++        cursor.set_image(std::make_shared<InvalidlySizedCursorImage>());
      , std::logic_error);
+ }
@@ -266,7 +294,8 @@
      EXPECT_CALL(*output_container.outputs[12], has_cursor()).Times(0);
      mgm::Cursor cursor_tmp{mock_gbm.fake_gbm.device, output_container,
--                              std::make_shared<StubCurrentConfiguration>()};
++       std::make_shared<StubCurrentConfiguration>(),
++       std::make_shared<StubCursorImage>()};
      output_container.verify_and_clear_expectations();
+ }