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

Alberto/Andreas: Addressed comments.

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

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

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

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

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

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

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

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

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

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

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

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

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

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