Code review comment for lp:~kdub/mir/produce-renderlist-for-particular-compositor

> I think more thought needs to go into breaking the Surface-is-a-Renderable
> relationship, before it gets broken:
>
> 423 - // Renderable interface
> 424 - std::shared_ptr<graphics::Buffer> buffer(void const*) const
> override;
> 425 - bool alpha_enabled() const override;
> 426 - geometry::Rectangle screen_position() const override;
> 427 - int buffers_ready_for_compositor() const override;
> 428 + std::shared_ptr<graphics::Renderable> renderable_for(void const*
> compositor_id) const;
>
> It messes with my mental model to say a surface "contains" a renderable.
> Changes like this which cannot be easily visualized typically result in
> confusion and slow down later development exponentially, so I want to make
> sure we have a clearer shared plan before it lands.

A BasicSurface contains a bunch of information pertaining to the surface. This information has to be comprehensive enough to provide for multi-compositor access, input, surface states, etc. It can be accessed by different parts of the system, it also has to do more to ensure thread-safety. Just by being the central place for surface information, it has a fair amount going on.

From this information we can produce a Renderable. This is a simple object that gives all the information a compositor needs for rendering. We can give the compositors a simple representation of the surface, which in turn makes writing compositors simpler.

>
> The primary problem we're dealing with is that Renderable has now been over-
> generalized to the point where it does not represent something that is easily
> conceivable (it's not even a surface any more). Somehow we need to move the
> parts around so they make good "obvious" sense to anyone who sees them.
>
> Problems like this are sometimes overcome by saying "land it now and fix it
> later". However in this case I feel that would entangle the logic too much for
> it to be easily fixable, resulting in exponentially more work than fixing the
> proposal itself.

I agree that the interface is overgeneralized and is incoherent. I'll also assert that this is due to the ms::BasicSurface : public mg::Renderable, as this inheritance makes it tempting to move functions out of BasicSurface to Renderable without thinking about the incoherence introduced into Renderable (and the extra burdens on the compositor).

I guess I'll just go function by function and tell how I'd like the interface to improve:

mg::Renderable::id()
This is only used for texture caching at the moment, but I can see how this would be useful for tying a particular idea to a renderable that is produced from the scene. I'd rather any of those ideas appear as an interface function, as opposed to using id's to associate the renderable with a particular 'thing' in the SurfaceStack. Function is pretty tolerable though.

mg::Renderable::buffer(void const*) / mg::Renderable::buffer()
Obviously, this is what this MP intends to address, favoring the parameter-less function so the user of the function doesn't worry about the buffer stream backing up the buffer access.

mg::Renderable::shaped(), mg::Renderable::alpha(); mg::Renderable::alpha_enabled()
These seem like they could be condensed into returning just one object representative of the alpha properties of the surface.

mg::Renderable::screen_position(), mg::Renderable::transformation()
These need unification, they both represent some sort of positioning on the screen. We will probably have to discuss what's the best way to represent position; I'll leave that for a future debate.

mg::Renderable::visible()
This really doesn't make any sense, because you can have a std::list<std::shared_ptr<Renderable>> with some that aren't visible. The scene should just not insert those renderables into the list, then we don't need this function.

mg::Renderable::buffers_ready_for_compositor()
This, like buffer() leaks details of the buffer stream to the compositors and forces the strange render loop the compositors must use: 'wait for message, then run in a loop until all the renderables return 0 on this function'. This should be removed once the Compositor can observe the SurfaceStack and be smarter about scheduling compositions.