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

Revision history for this message
Daniel d'Andrada (dandrader) wrote :

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.

"""
    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();
}
"""

review: Needs Information

« Back to merge proposal