Code review comment for lp:~alan-griffiths/mir/even-NullWindowManager-configures-surface

Revision history for this message
Alexandros Frantzis (afrantzis) wrote :

> Why? It is entirely possible for a responsible user to specialize the class successfully

It is entirely possible, and more straightforward (IMO, read below), for a responsible user to directly use the WindowManager interface directly, too :)

> I don't think that is true. If overriding from a concrete class one should (with rare exceptions)
> call the overridden function from the override. For the user to assume that it is null omitting the
> call is a mistake, but not one we should be protecting them from.

I am more concerned about the order of the call to the overriden function in the new code, rather than whether the function is called or not. In order to write correct code the user needs to know what the overriden function does (e.g. in the set_surface_attribute() case that it calls surface->configure()), to avoid performing the same action again (e.g. call surface->configure() again), and ensure that it's called at the correct point in the new code.

In addition, we are making the implementation details of SkeletonWindowManager part of our effective ABI.

All of the above are general issues with inheriting implementation, and present a trade-off that sometimes is worth it. I am leaning towards "not worth it" in this particular case.

Needs discussion

review: Needs Information

« Back to merge proposal