Mir

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

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

(3) ExampleCursorImage doesn't seem to do anything(?). I can just get none (default) or the standard black arrow with --display-cursor=1. Looks like the bug pre-dates this proposal, but it makes the proposed changes untestable.

(5) Yes I am serious about "Repository". Names are important because a well chosen one leads to universal understanding, high productivity and avoids refactoring. Poorly chosen names lead to confusion and thus slow development or even bugs. The word "Repository" I know means where you put or get something. So is the cursor coming or going from a "repository"? It's not clear from the name. A realistic mental model for cursors is that you put it in on the screen, but you get it from a theme. Those are more concrete nouns that will lead to wider understanding and better code reuse/utilization. "CursorLoader" is mildly better, but it makes the mistake of using a verb in the noun, which tends to limit how well multiple people can agree on what it represents. If there is a more concrete noun available like Theme or Factory then I'd prefer we used it.

(6) Per these links:
   https://developer.gnome.org/gdk3/stable/gdk3-Cursors.html#GdkCursorType
   http://qt-project.org/doc/qt-5/qt.html#CursorShape-enum
   http://www.opengl.org/resources/libraries/glut/spec3/node28.html#SECTION000513000000000000000
you can see there's going to be a fair amount of mapping between enums and/or strings. While strings are completely flexible, it's more efficient and simpler to map between enum values. That said, I'm not sure that's a performance/convenience penalty worth blocking on, so won't.

review: Needs Fixing

« Back to merge proposal