Mir

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

Revision history for this message
Kevin DuBois (kdub) wrote :

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

« Back to merge proposal