Code review comment for lp:~mir-team/mir/touchspot-renderable

needed?:
639 + auto mga_buffer = std::dynamic_pointer_cast<mga::Buffer>(buffer);
640 + if (!mga_buffer)
641 + BOOST_THROW_EXCEPTION(std::logic_error("Invalid buffer (did not originate from platform graphics driver)"));

Yeah...kind of strange I guess, just using static cast.

>> could there be stronger typing than void*?
>> void mga::AndroidBufferWriter::write(std::shared_ptr<mg::Buffer> const& buffer, void const* data, >> size_t size)

Using unsigned char now or did you mean something that encodes the pixel format?

>> overlay is a bit overloaded already, maybe just add_cursor_renderable()?
>> 2824 + void add_overlay(std::shared_ptr<graphics::Renderable> const& overlay);
>> 2825 + void remove_overlay(std::weak_ptr<graphics::Renderable> const& overlay);

How about "input_visualization"

>> const&
>> 643 + auto handle = buffer->native_buffer_handle();

Fixed

>> we shouldn't be adding this to Renderable without rejecting all 'decorated' renderables down in >> the hwc code. This function makes the RenderableList indeterminate in terms of what would appear >> on the screen. It says it should be decorated but doesn't say what this entails.
>> 165 + virtual bool should_be_decorated() const = 0;

I'm not sure the renderables should be / need to be rejected. If a shell wants to draw decorations then it is responsible for recognizing that passing the renderlist to the display buffer isn't appropriate right? So this method is really just for the shell compositor tor ecognize if the renderable is of a type which should be decorated. I agree it's strange...but I don't know how to do anything better without decorations expressed in the scene.