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

233 + {
234 + std::unique_lock<decltype(mutex)> lock(mutex);
235 + setting.mutable_surfaceid()->CopyFrom(surface.id());
236 + if (cursor->name != "")
237 + setting.set_name(cursor->name.c_str());
238 + }

indentation

~~~~

374 + BOOST_THROW_EXCEPTION(std::runtime_error("Cursor API not yet implemented"));

We have a tradition of having a "TODO" comment to mark pending work. (Also, getting very picky, I think this is more of a logic_error than a runtime_error.)

~~~~

406 + // No name is interpreted as disabled cursor.
407 + optional string name = 2;

Is a comment really clearer than a boolean field?

~~~~

528 + if (image->cursor_name != "default")
529 + return false;
530 + return true;

Overly verbose: return image->cursor_name == "default";

~~~~

662 +// In this set we create a 1x1 client surface at the point (1,0). The client requests to disable the cursor
663 +// over this surface. Since the cursor starts at (0,0) we when we move the cursor by (1,0) thus causing it
664 +// to enter the bounds of the first surface, we should observe it being disabled.
665 +TEST_F(TestClientCursorAPI, DISABLED_client_may_disable_cursor_over_surface)

We have a tradition of having a "TODO" comment to mark pending work.

Also with this and other tests it looks like a lot of the setup could be moved into the test fixture - which would make toe behavior under test a lot clearer.