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

Revision history for this message
Nick Dedekind (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.

« Back to merge proposal