Mir

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

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

(1) Reading again, I still believe there's no justification for MirCursorConfiguration. All the relevant info would be contained in a MirCursor (image + hotspot... what else is there?).

(3) You should refer to them by identifier name in the docs. Not all readers will be looking at the header file and immediately see which identifiers to use (if any):
58 + * from the system cursor theme. Currently only the default cursor
59 + * and a disabled cursor are supported.

(4) OK, assuming the verb "configure" describes something more complex than "setting" a cursor, what's the extra complexity? What is it doing other than setting a cursor?

(5) Your justification for having invisible cursors is not valid, I believe. Games with no visible cursor require relative movement data, which is unrelated to any "cursor" location. See: https://bugs.launchpad.net/mir/+bug/1276322
In particular, in your "racing game" or FPS you don't want mouse movement to suddenly hit an invisible wall like a cursor would at the edge of the screen. Thus, you need to turn the cursor "off" (including events) and not just make it invisible. Per bug 1276322.

(6) I don't really mind letting it be a toolkit/app problem. I was particularly curious since I noticed we already have BasicSurface::input_rectangles in the server. It would be nice if we avoided libmirserver having to maintain any knowledge of the sub-regions of the client surface.

review: Needs Fixing

« Back to merge proposal