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

On 14/10/2015 13:41, Gerry Boland 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.
> You're right in that this way is strictly better, especially if you don't have control over the clients registering. For now, we do have control over the clients, so we can ensure they behave correctly.
>
> Re-using "this" memory address as unique identifier means there's one less variable to worry about.
>
> Since we control the clients, I don't think we need the complexity of the correct approach just now. If third parties start using qtmir MirSurface directly, then we will need to rethink it.

qtmir::MirSurfaceInterface (where registerView() belongs to) is an
internal qtmir API.