Code review comment for lp:~nick-dedekind/qtubuntu/shell_chrome

> +++ src/ubuntumirclient/input.cpp
> at the end of the anonymous namespace, could you please add "// namespace"
> I always find
> +}
> +
> +}
> hard to parse because you're missing the hint the indent usually gives - since
> we don't indent namespace contents.

Done

>
>
> +++ src/ubuntumirclient/window.cpp
> +const Qt::WindowType WindowHidesShellDecorations =
> (Qt::WindowType)0x00800000;
> being a pedant, but if mir chose the name "shell chrome" I guess we should use
> its vocabulary too.

Done

>
> + void updateSurface();
> While it was private, I didn't mind, but now it is public, I do. Function name
> is terrible considering what it does. Please either rename, or refactor a
> little to avoid making it public member of UbuntuSurface.

I've refactored, but not sure it's the best way. I'm wondering if we can mode the code to QPlatformWindow::setParent.
We should probably be documenting everything we need to refactor!

>
> There is a general problem in this file, it's unclear what UbuntuSurface is,
> and how it relates to UbuntuWindow, and what responsibilities it has. My
> understanding was that UbuntuSurface should wrap the mir surface api in a Qt
> way, but as you saw it also is saving state, and referring to state in
> UbuntuWindow/QWindow when working. So the separation of responsibilities here
> is totally unclear.
>
> So what you've done is natural, moving some states out of UbuntuSurface into
> UbuntuWindow. What is annoying is that UbuntuSurface imposes some sanity
> checks, referring to those states. I suspect we'll try to remove those, and
> trust that Mir will impose correct behaviour, instead of doing that here.
>
> But
> + MirShellChrome mShellChrome;
> does we need to save this state? Can we not just query the value from the
> mirclient api?

There doesnt seem to be a client query api for this. It's pushed through the surface spec.

>
> + const QRect& exposeRect = mWindowVisible ? QRect(QPoint(),
> geometry().size()) : QRect();
> & beside the variable name plz.

Not needed. removed.