Mir

Code review comment for lp:~mir-team/mir/cursor-spike-phase-1

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

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!

« Back to merge proposal