Code review comment for lp:~gerboland/qtmir/multimonitor-spike

I think I've followed all the recommendations you've made. I've only a couple of replies:

> """
> // If Screen was not specified, just grab an unused one, if available
> """
>
> I don't get the first "if". This method doesn't seem to handle the case where
> a screen *is* specified. It doesn't even seem possible.

You're right, it was terribly phrased. Have updated it to make sense now

> qDebug() << "Screens are not initialized, unable to create a new
> QWindow/ScreenWindow";
<snip>
> qDebug() << "No available Screens to create a new QWindow/ScreenWindow
>
> I think these should be turned into criticals (and also use the logging
> category API).

I did make them critical, but I always want those errors to print so didn't make them part of a category.

> In src/platforms/mirserver/qmirserver.h:
>
> """
> QWeakPointer<ScreenController> screenController() const;
> """
>
> Why return a weak pointer if all users of it do "screenController().lock()"?.
> So why not make it return a shared pointer already (making for cleaner code)?

Because the QMirServer has ownership of the ScreenController, and manages its lifetime. A ScreenController will only exist after the mir server is started, and must be deleted before mir shuts down. I can't have API users keeping it around longer than it should.

This may be exposing an API problem which can be resolved, but I didn't see any obvious solution. So this will have to do.

> src/platforms/mirserver/screencontroller.cpp
> """
> auto displayConfig = display->configuration();
> """
>
> I think you're abusing the use of "auto" here. I would prefer to know the
> class of this object.

You really want to know is it std::unique_ptr<mg::DisplayConfiguration> ? I don't, the variable name is enough for me.