Code review comment for lp:~nick-dedekind/qtmir/lp1475678.surface-occlude

Revision history for this message
Daniel d'Andrada (dandrader) wrote :

On 14/10/2015 11:57, Nick Dedekind wrote:
>> """
>> +int MirSurface::registerView()
>> """
>>
>> It would be better if it took the view id as a parameter, like this:
>> void MirSurface::registerView(qintptr viewId)
>>
>>
>> That way MirSurfaceItem could simply do:
>> m_surface->registerView((qintptr)this);
>>
>> and not have to bother storing any id at all. Afterall, MirSurface::m_views is
>> already storing them. So no need to store on both ends.
>>
>> Sorry, I should have clarified that when you sent me
>> http://pastebin.ubuntu.com/12773655/. I thought had you simply overlooked the
>> missing parameter in that draft (and I didn't notice it was returning an int).
> I don't think that's very good coding practice. It requires the caller to be responsible for choosing a value unique to another class (even if it's its mem address, it's still the assumption). Most registration mechanisms do it my way, for good reason.

mir::graphics::RenderableList
generate_renderables(compositor::CompositorID id) uses the same approach
(CompositorID being "void const*") as well, if you want an exmaple.

« Back to merge proposal