Code review comment for lp:~gerboland/qtmir/qmirserver-hides-mirserver

> > In src/platforms/mirserver/qmirserver.h
> >
> > """
> > bool run();
> > Q_SLOT bool stop();
> > """
> >
> > Again, thinking about having it as public API: you should probably document
> > these methods, answering questions like: Is it a blocking call or does it
> > return immediately?
> >
> > What's the user expected to do with those return values? I don't see you
> using
> > them for anything.
>
> I was planning to document things later. But I can start now if you'd prefer.
>
>
>
> > """
> > connect(QCoreApplication::instance(), &QCoreApplication::aboutToQuit,
> > d->serverWorker, &MirServerWorker::stop, Qt::DirectConnection);
> //
> > see FIXME in MirServerWorker
> > """
> >
> > """
> > if (!d->serverWorker->waitForMirStartup())
> > {
> > qCritical() << "ERROR: QMirServer - Mir failed to start";
> > QCoreApplication::quit();
> > """
> >
> > Isn't QMirServer going out of its territory when it modifies
> QCoreApplication
> > directly like that?
> > I would expect the user of QMirServer to do this instead, like:
> >
> > """
> > if (!qmirServer->run()) {
> > QCoreApplication::quit();
> > }
> > """
>
>
> You're totally right, this is also the direction I wanted to go. I was
> hesitant to do so, as I feared it along with all the other changes would make
> quite a large MR. So your suggestion is to be the next step: have the
> QMirServer user listen for mir or qt shutdown, and react accordingly. Should I
> just do that now?

I can live with this being an intermediate step but it should leave the code in a consistent state. Eg: If you don't use the return values for anything then don't return them in the first place (have a void return type). Also it would be good to add TODOs for the next step, like over those QCoreApplication calls inside QMirServer.