Code review comment for lp:~alan-griffiths/mir/add-mir_surface_spec_attach

Revision history for this message
Kevin DuBois (kdub) wrote :

> > OK on the first issue. Although "helps with an immediate problem without
> > getting in the way of further enhancements" is a benefit that's also
> > contradictory to good engineering: Everything you write should ideally have
> > multiple uses so that functionality grows exponentially faster than the code
> > size. We're failing on that part.
> Having a function that (re)specifies the attachment without reference to the
> surface type does provide multiple uses. If we later (as one suggestion has
> it) enhance MirEdgeAttachment it automatically gains that functionality.

I suppose there's also the issue of how easy things are to understand (which naming and typing helps with). I'd rather see two needs fulfilled by two different easy-to-understand functions than two needs fulfilled by one harder-to-understand function. It seems that we don't really know the future enhancements, so I'd rather just see

mir_surface_spec_attach_relative(MirSurfaceSpec*, MirPlaceMode, MirRectangle*)

without reserving the upper bits of the mode as opcodes that alter what the function does

« Back to merge proposal