Mir

Code review comment for lp:~mir-team/mir/improve-attribute-protocol-and-normalize-getters

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

Just some quick comments:

(1) This should be 0..N:
+ /* Do not specify values...code relies on 1...N ordering. */

(2) The suffix "_state" is unnecessary and potentially confusing with the attribute called "state":
15 +MirSurfaceFocusState mir_surface_get_focus_state(MirSurface *surface);
23 +MirSurfaceVisibility mir_surface_get_visibility_state(MirSurface *surface);

(3) Returning an integer value outside the range of an enum is a violation of the implicit contract that a well designed API will only return values of those enums:
13 + * -1 if the surface is invalid.
21 + * -1 if the surface is invalid.
I think a more sensible fallback for invalid surfaces would be to return mir_surface_unfocused and mir_surface_visibility_occluded.

(4) src/shared/protobuf/mir_protobuf.proto: Protocol ABI change!
Please either avoid protocol changes, or record the change explicitly with a bump of libmirprotobuf0 to libmirprotobuf1. Not ideal.

review: Needs Fixing

« Back to merge proposal