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

>>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.