Merge lp:~gerboland/qtmir/qmirserver-hides-mirserver into lp:qtmir

[Gerry Boland]

[qpa] refactor QMirServer to clean up its API, and fix strange thread design.

This does the following:
- QMirServer has a much cleaner API - using d-pointer to encourage ABI stability
- move quit decision point to MirServerIntegration
- the Mir server is run from a class inheriting a QThread, simplifies thread control significantly

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:331
http://jenkins.qa.ubuntu.com/job/qtmir-ci/258/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-amd64-ci/109
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/109
        deb: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/109/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtmir-ci/258/rebuild

review: Approve (continuous-integration)

[Daniel d'Andrada (dandrader)]

If you're thinking about making QMirServer public API, could you please add some documentation to src/platforms/mirserver/qmirserver.h. Explain what it does, what's its main purpose.

[Daniel d'Andrada (dandrader)]

So MirServer::stop() is thread safe or is it a hack?

"""
void MirServerWorker::stop()
{
    server->stop();
}
"""

review: Needs Information

[Daniel d'Andrada (dandrader)]

> So MirServer::stop() is thread safe or is it a hack?
>
>
> """
> void MirServerWorker::stop()
> {
> server->stop();
> }
> """

Please ignore me. Now I see that MirServerWorker::run() and stop() are never called directly from the main thread but only from the within worker thread's event loop.

[Daniel d'Andrada (dandrader)]

In src/platforms/mirserver/qmirserver.h:

"""
    Q_SLOT void serverStopped();
"""

Are you sure you want this slot in the public API?

And, by the way, it's named like a signal. It should be onServerStopped() or, even better, quit().

[Daniel d'Andrada (dandrader)]

in src/platforms/mirserver/qmirserver.cpp:

"""
    connect(d->serverWorker, &MirServerWorker::stopped, this, &QMirServer::serverStopped, Qt::DirectConnection);
"""

I think this line deserves a comment explaining why this has to be a direct connection, which will cause QMirServer::serverStopped to be called from within MirServerWorker's thread.

trunk has a terse comment about it.

Which makes me think maybe you should say from which thread QMirServer::serverStopped() will be called in the documentation of QMirServer::serverStopped() itself. Helps keeping tabs on this tricky cross-thread communication.

review: Needs Information

[Daniel d'Andrada (dandrader)]

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

[Daniel d'Andrada (dandrader)]

In src/platforms/mirserver/display.cpp:

"""
-Display::Display(const QSharedPointer<MirServer> &server, QObject *parent)
- : QObject(parent)
- , m_mirServer(server)
+Display::Display(const QSharedPointer<MirServer> &server)
 {
- std::shared_ptr<mir::graphics::DisplayConfiguration> displayConfig = m_mirServer->the_display()->configuration();
+ std::shared_ptr<mir::graphics::DisplayConfiguration> displayConfig = server->the_display()->configuration();
"""

I think you could go further and pass the display config already in the constructor instead of the whole server since that's the only thing Display is interested in after all.

-------------------------

In src/platforms/mirserver/miropenglcontext.cpp:

"""
 MirOpenGLContext::MirOpenGLContext(const QSharedPointer<MirServer> &server, const QSurfaceFormat &format)
- : m_mirServer(server)
 #if GL_DEBUG
- , m_logger(new QOpenGLDebugLogger(this))
+ : m_logger(new QOpenGLDebugLogger(this))
 #endif
 {
- std::shared_ptr<mir::graphics::Display> display = m_mirServer->the_display();
+ std::shared_ptr<mir::graphics::Display> display = server->the_display();
"""

Likewise here.

[Gerry Boland (gerboland)]

> If you're thinking about making QMirServer public API, could you please add
> some documentation to src/platforms/mirserver/qmirserver.h. Explain what it
> does, what's its main purpose.

It's far from ready yet, but that is the plan. I'll start writing documentation when we've the API in a good condition, which will be another while yet.

[Gerry Boland (gerboland)]

> in src/platforms/mirserver/qmirserver.cpp:
>
> """
> connect(d->serverWorker, &MirServerWorker::stopped, this,
> &QMirServer::serverStopped, Qt::DirectConnection);
> """
>
> I think this line deserves a comment explaining why this has to be a direct
> connection, which will cause QMirServer::serverStopped to be called from
> within MirServerWorker's thread.
>
> trunk has a terse comment about it.

Fair point. The line above it also creates a direct connection, and that line has a comment pointing to the explanation. But yeah, that connection is easily missed, will fix

[Gerry Boland (gerboland)]

> 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?

[Gerry Boland (gerboland)]

> In src/platforms/mirserver/display.cpp:
>
> """
> -Display::Display(const QSharedPointer<MirServer> &server, QObject *parent)
> - : QObject(parent)
> - , m_mirServer(server)
> +Display::Display(const QSharedPointer<MirServer> &server)
> {
> - std::shared_ptr<mir::graphics::DisplayConfiguration> displayConfig =
> m_mirServer->the_display()->configuration();
> + std::shared_ptr<mir::graphics::DisplayConfiguration> displayConfig =
> server->the_display()->configuration();
> """
>
> I think you could go further and pass the display config already in the
> constructor instead of the whole server since that's the only thing Display is
> interested in after all.
>
> -------------------------
>
> In src/platforms/mirserver/miropenglcontext.cpp:
>
> """
> MirOpenGLContext::MirOpenGLContext(const QSharedPointer<MirServer> &server,
> const QSurfaceFormat &format)
> - : m_mirServer(server)
> #if GL_DEBUG
> - , m_logger(new QOpenGLDebugLogger(this))
> + : m_logger(new QOpenGLDebugLogger(this))
> #endif
> {
> - std::shared_ptr<mir::graphics::Display> display =
> m_mirServer->the_display();
> + std::shared_ptr<mir::graphics::Display> display = server->the_display();
> """
>
> Likewise here.

I had a reason for not doing that at the time, but I can't recall it now. Will do

[Daniel d'Andrada (dandrader)]

> > 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.

[Daniel d'Andrada (dandrader)]

The commit message line is a bit too long. That first line should shorter, followed by a longer description below if needed

[lp:~gerboland/qtmir/qmirserver-hides-mirserver]

332. By Gerry Boland on 2015-05-18

Trying to fix shutdown, incomplete

333. By Gerry Boland on 2015-05-18

Fix shutdown issues by inheriting QThread directly - also get to avoid having qt event loop for mir

334. By Gerry Boland on 2015-05-18

Clean up shutdown codepath further. run/stop return void

335. By Gerry Boland on 2015-05-18

Unneeded return statements

336. By Gerry Boland on 2015-05-18

Remove old comment, it not the case any more

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:336
http://jenkins.qa.ubuntu.com/job/qtmir-ci/268/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/1/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/1/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtmir-ci/268/rebuild

review: Needs Fixing (continuous-integration)

[lp:~gerboland/qtmir/qmirserver-hides-mirserver]

337. By Gerry Boland on 2015-05-18

Merge qtmir trunk

[Gerry Boland (gerboland)]

> > In src/platforms/mirserver/display.cpp:
> >
> > """
> > -Display::Display(const QSharedPointer<MirServer> &server, QObject *parent)
> > - : QObject(parent)
> > - , m_mirServer(server)
> > +Display::Display(const QSharedPointer<MirServer> &server)
> > {
> > - std::shared_ptr<mir::graphics::DisplayConfiguration> displayConfig =
> > m_mirServer->the_display()->configuration();
> > + std::shared_ptr<mir::graphics::DisplayConfiguration> displayConfig =
> > server->the_display()->configuration();
> > """
> >
> > I think you could go further and pass the display config already in the
> > constructor instead of the whole server since that's the only thing Display
> is
> > interested in after all.
> >
> > -------------------------
> >
> > In src/platforms/mirserver/miropenglcontext.cpp:
> >
> > """
> > MirOpenGLContext::MirOpenGLContext(const QSharedPointer<MirServer> &server,
> > const QSurfaceFormat &format)
> > - : m_mirServer(server)
> > #if GL_DEBUG
> > - , m_logger(new QOpenGLDebugLogger(this))
> > + : m_logger(new QOpenGLDebugLogger(this))
> > #endif
> > {
> > - std::shared_ptr<mir::graphics::Display> display =
> > m_mirServer->the_display();
> > + std::shared_ptr<mir::graphics::Display> display =
> server->the_display();
> > """
> >
> > Likewise here.
>
>
> I had a reason for not doing that at the time, but I can't recall it now. Will
> do

I thought of the reason. It would require me to include loads of Mir internal headers in MirServerIntegration. Since qtmir::Display wants mir::graphics::DisplayConfiguration, I would need to include headers for mg::Display & mg::DisplayConfiguration.

As the multimonitor work will refactor those classes quite a bit, I decided to just keep this approach knowing that I'll improve it later.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:337
http://jenkins.qa.ubuntu.com/job/qtmir-ci/269/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/2
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/2
        deb: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/2/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtmir-ci/269/rebuild

review: Approve (continuous-integration)

[lp:~gerboland/qtmir/qmirserver-hides-mirserver]

338. By Gerry Boland on 2015-05-19

Merge trunk

339. By Gerry Boland on 2015-05-19

Remove unneeded header

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:339
http://jenkins.qa.ubuntu.com/job/qtmir-ci/270/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/3/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/3/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtmir-ci/270/rebuild

review: Needs Fixing (continuous-integration)

[lp:~gerboland/qtmir/qmirserver-hides-mirserver]

340. By Gerry Boland on 2015-05-27

Only pass mg::DisplayConfiguration to qtmir::Display, as that is all it needs

[Gerry Boland (gerboland)]

Ready for a second review pass. Main change is in the thread design which simplifies event passing greatly. API improved too, QMirServer no longer quits the process, owner does instead

[Daniel d'Andrada (dandrader)]

"""
bool QMirServer::start()
{
    [...]
    Q_EMIT started();
    [...]
}
"""

Is this signal being used for anything?

review: Needs Information

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:340
http://jenkins.qa.ubuntu.com/job/qtmir-ci/271/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/4/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/4/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtmir-ci/271/rebuild

review: Needs Fixing (continuous-integration)

[Gerry Boland (gerboland)]

> """
> bool QMirServer::start()
> {
> [...]
> Q_EMIT started();
> [...]
> }
> """
>
> Is this signal being used for anything?

Nope, added to API for symmetry (the "stopped" signal is necessary)

[Daniel d'Andrada (dandrader)]

Code looks ok, but didn't test on the device to see if broke anything.

review: Approve (code)

[Daniel d'Andrada (dandrader)]

* Did you perform an exploratory manual test run of the code change and any related functionality?
Yes.

* Did CI run pass? If not, please explain why.
No, because Jenkins runs on wily and wily doesn't have mir 0.13 yet.

review: Approve

=== modified file 'demos/qml-demo-shell/qml-demo-shell.qml'
--- demos/qml-demo-shell/qml-demo-shell.qml	2015-01-23 10:23:07 +0000
+++ demos/qml-demo-shell/qml-demo-shell.qml	2015-05-27 12:12:52 +0000
@@ -110,6 +110,21 @@
         y: point.y
     }
 
+    Rectangle {
+        width: 60
+        height: 40
+        color: "red"
+        anchors { right: parent.right; bottom: parent.bottom }
+        Text {
+            anchors.centerIn: parent
+            text: "Quit"
+        }
+        MouseArea {
+            anchors.fill: parent
+            onClicked: Qt.quit()
+        }
+    }
+
     Connections {
         target: SurfaceManager
         onSurfaceCreated: {

=== modified file 'src/platforms/mirserver/CMakeLists.txt'
--- src/platforms/mirserver/CMakeLists.txt	2015-05-13 09:40:03 +0000
+++ src/platforms/mirserver/CMakeLists.txt	2015-05-27 12:12:52 +0000
@@ -44,6 +44,7 @@
     qteventfeeder.cpp
     plugin.cpp
     qmirserver.cpp
+    qmirserver_p.cpp
     sessionauthorizer.cpp
     sessionlistener.cpp
     surfaceobserver.cpp

=== modified file 'src/platforms/mirserver/display.cpp'
--- src/platforms/mirserver/display.cpp	2014-12-01 11:10:54 +0000
+++ src/platforms/mirserver/display.cpp	2015-05-27 12:12:52 +0000
@@ -23,18 +23,13 @@
 
 #include <mir/graphics/display.h>
 #include <mir/graphics/display_configuration.h>
-#include <QDebug>
 
 namespace mg = mir::graphics;
 
 // TODO: Listen for display changes and update the list accordingly
 
-Display::Display(const QSharedPointer<MirServer> &server, QObject *parent)
-  : QObject(parent)
-  , m_mirServer(server)
+Display::Display(const std::shared_ptr<mir::graphics::DisplayConfiguration> &displayConfig)
 {
-    std::shared_ptr<mir::graphics::DisplayConfiguration> displayConfig = m_mirServer->the_display()->configuration();
-
     displayConfig->for_each_output([this](mg::DisplayConfigurationOutput const& output) {
         if (output.used) {
             auto screen = new Screen(output);

=== modified file 'src/platforms/mirserver/display.h'
--- src/platforms/mirserver/display.h	2014-12-01 11:10:54 +0000
+++ src/platforms/mirserver/display.h	2015-05-27 12:12:52 +0000
@@ -19,23 +19,21 @@
 #ifndef DISPLAY_H
 #define DISPLAY_H
 
-#include <QObject>
 #include <qpa/qplatformscreen.h>
-
-class MirServer;
-
-class Display : public QObject
+#include <memory>
+
+namespace mir { namespace graphics { class DisplayConfiguration; }}
+
+class Display
 {
-    Q_OBJECT
 public:
-    Display(const QSharedPointer<MirServer> &server, QObject *parent = 0);
-    ~Display();
+    Display(const std::shared_ptr<mir::graphics::DisplayConfiguration> &displayConfig);
+    virtual ~Display();
 
     QList<QPlatformScreen *> screens() const { return m_screens; }
 
 private:
     QList<QPlatformScreen *> m_screens;
-    const QSharedPointer<MirServer> m_mirServer;
 };
 
 #endif // DISPLAY_H

=== modified file 'src/platforms/mirserver/miropenglcontext.cpp'
--- src/platforms/mirserver/miropenglcontext.cpp	2015-01-12 17:54:55 +0000
+++ src/platforms/mirserver/miropenglcontext.cpp	2015-05-27 12:12:52 +0000
@@ -36,12 +36,11 @@
 // (i.e. individual display output buffers) to use as a common base context.
 
 MirOpenGLContext::MirOpenGLContext(const QSharedPointer<MirServer> &server, const QSurfaceFormat &format)
-    : m_mirServer(server)
 #if GL_DEBUG
-    , m_logger(new QOpenGLDebugLogger(this))
+    : m_logger(new QOpenGLDebugLogger(this))
 #endif
 {
-    std::shared_ptr<mir::graphics::Display> display = m_mirServer->the_display();
+    std::shared_ptr<mir::graphics::Display> display = server->the_display();
 
     // create a temporary GL context to fetch the EGL display and config, so Qt can determine the surface format
     std::unique_ptr<mir::graphics::GLContext> mirContext = display->create_gl_context();

=== modified file 'src/platforms/mirserver/miropenglcontext.h'
--- src/platforms/mirserver/miropenglcontext.h	2014-12-01 11:10:54 +0000
+++ src/platforms/mirserver/miropenglcontext.h	2015-05-27 12:12:52 +0000
@@ -52,7 +52,6 @@
 #endif
 
 private:
-    const QSharedPointer<MirServer> m_mirServer;
     QSurfaceFormat m_format;
 #if GL_DEBUG
     QOpenGLDebugLogger *m_logger;

=== modified file 'src/platforms/mirserver/mirserverintegration.cpp'
--- src/platforms/mirserver/mirserverintegration.cpp	2015-04-01 10:48:28 +0000
+++ src/platforms/mirserver/mirserverintegration.cpp	2015-05-27 12:12:52 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2013-2014 Canonical, Ltd.
+ * Copyright (C) 2013-2015 Canonical, Ltd.
  *
  * This program is free software: you can redistribute it and/or modify it under
  * the terms of the GNU Lesser General Public License version 3, as published by
@@ -30,7 +30,6 @@
 #include <qpa/qwindowsysteminterface.h>
 
 #include <QCoreApplication>
-#include <QStringList>
 #include <QOpenGLContext>
 
 #if QT_VERSION < QT_VERSION_CHECK(5, 2, 0)
@@ -41,10 +40,7 @@
 
 // Mir
 #include <mir/graphics/display.h>
-#include <mir/graphics/display_buffer.h>
-
-// std
-#include <csignal>
+#include <mir/graphics/display_configuration.h>
 
 // local
 #include "clipboard.h"
@@ -66,36 +62,25 @@
 #if QT_VERSION < QT_VERSION_CHECK(5, 2, 0)
     , m_eventDispatcher(createUnixEventDispatcher())
 #endif
+    , m_mirServer(new QMirServer(QCoreApplication::arguments()))
     , m_display(nullptr)
-    , m_qmirServer(nullptr)
     , m_nativeInterface(nullptr)
     , m_clipboard(new Clipboard)
 {
-    // Start Mir server only once Qt has initialized its event dispatcher, see initialize()
-
-    QStringList args = QCoreApplication::arguments();
-    // convert arguments back into argc-argv form that Mir wants
-    char **argv;
-    argv = new char*[args.size() + 1];
-    for (int i = 0; i < args.size(); i++) {
-        argv[i] = new char[strlen(args.at(i).toStdString().c_str())+1];
-        memcpy(argv[i], args.at(i).toStdString().c_str(), strlen(args.at(i).toStdString().c_str())+1);
-    }
-    argv[args.size()] = '\0';
-
     // For access to sensors, qtmir uses qtubuntu-sensors. qtubuntu-sensors reads the
     // UBUNTU_PLATFORM_API_BACKEND variable to decide if to load a valid sensor backend or not.
     // For it to function we need to ensure a valid backend has been specified
     if (qEnvironmentVariableIsEmpty("UBUNTU_PLATFORM_API_BACKEND")) {
-        if (qgetenv("DESKTOP_SESSION").contains("mir")) {
+        if (qgetenv("DESKTOP_SESSION").contains("mir") || !qEnvironmentVariableIsSet("ANDROID_DATA")) {
             qputenv("UBUNTU_PLATFORM_API_BACKEND", "desktop_mirclient");
         } else {
             qputenv("UBUNTU_PLATFORM_API_BACKEND", "touch_mirclient");
         }
     }
 
-    m_mirServer = QSharedPointer<MirServer>(
-                      new MirServer(args.length(), const_cast<const char**>(argv)));
+    // If Mir shuts down, quit.
+    QObject::connect(m_mirServer.data(), &QMirServer::stopped,
+                     QCoreApplication::instance(), &QCoreApplication::quit);
 
 #if QT_VERSION < QT_VERSION_CHECK(5, 2, 0)
     QGuiApplicationPrivate::instance()->setEventDispatcher(eventDispatcher_);
@@ -109,7 +94,6 @@
 {
     delete m_nativeInterface;
     delete m_display;
-    delete m_qmirServer;
 }
 
 bool MirServerIntegration::hasCapability(QPlatformIntegration::Capability cap) const
@@ -135,18 +119,21 @@
 
     DisplayWindow* displayWindow = nullptr;
 
+    auto const mirServer = m_mirServer->mirServer().lock();
     mg::DisplayBuffer* first_buffer{nullptr};
     mg::DisplaySyncGroup* first_group{nullptr};
-    m_mirServer->the_display()->for_each_display_sync_group([&](mg::DisplaySyncGroup &group) {
-        if (!first_group) {
-            first_group = &group;
-        }
-        group.for_each_display_buffer([&](mg::DisplayBuffer &buffer) {
-            if (!first_buffer) {
-                first_buffer = &buffer;
+    if (mirServer) {
+        mirServer->the_display()->for_each_display_sync_group([&](mg::DisplaySyncGroup &group) {
+            if (!first_group) {
+                first_group = &group;
             }
+            group.for_each_display_buffer([&](mg::DisplayBuffer &buffer) {
+                if (!first_buffer) {
+                    first_buffer = &buffer;
+                }
+            });
         });
-    });
+    }
 
     // FIXME(gerry) this will go very bad for >1 display buffer
     if (first_group && first_buffer)
@@ -168,7 +155,7 @@
 QPlatformOpenGLContext *MirServerIntegration::createPlatformOpenGLContext(QOpenGLContext *context) const
 {
     qDebug() << "createPlatformOpenGLContext" << context;
-    return new MirOpenGLContext(m_mirServer, context->format());
+    return new MirOpenGLContext(m_mirServer->mirServer(), context->format());
 }
 
 #if QT_VERSION >= QT_VERSION_CHECK(5, 2, 0)
@@ -181,10 +168,12 @@
 void MirServerIntegration::initialize()
 {
     // Creates instance of and start the Mir server in a separate thread
-    m_qmirServer = new QMirServer(m_mirServer);
+    if (!m_mirServer->start()) {
+        exit(2);
+    }
 
-    m_display = new Display(m_mirServer);
-    m_nativeInterface = new NativeInterface(m_mirServer);
+    m_display = new Display(m_mirServer->mirServer().data()->the_display()->configuration());
+    m_nativeInterface = new NativeInterface(m_mirServer->mirServer());
 
     for (QPlatformScreen *screen : m_display->screens())
         screenAdded(screen);

=== modified file 'src/platforms/mirserver/mirserverintegration.h'
--- src/platforms/mirserver/mirserverintegration.h	2014-12-11 15:27:02 +0000
+++ src/platforms/mirserver/mirserverintegration.h	2015-05-27 12:12:52 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2013-2014 Canonical, Ltd.
+ * Copyright (C) 2013-2015 Canonical, Ltd.
  *
  * This program is free software: you can redistribute it and/or modify it under
  * the terms of the GNU Lesser General Public License version 3, as published by
@@ -69,8 +69,6 @@
     QPlatformNativeInterface *nativeInterface() const override;
 
 private:
-    QSharedPointer<MirServer> m_mirServer;
-
     QScopedPointer<QPlatformAccessibility> m_accessibility;
     QScopedPointer<QPlatformFontDatabase> m_fontDb;
     QScopedPointer<QPlatformServices> m_services;
@@ -78,8 +76,9 @@
     QScopedPointer<QAbstractEventDispatcher> m_eventDispatcher;
 #endif
 
+    QScopedPointer<QMirServer> m_mirServer;
+
     Display *m_display;
-    QMirServer *m_qmirServer;
     NativeInterface *m_nativeInterface;
     QPlatformInputContext* m_inputContext;
     QScopedPointer<qtmir::Clipboard> m_clipboard;

=== modified file 'src/platforms/mirserver/nativeinterface.cpp'
--- src/platforms/mirserver/nativeinterface.cpp	2015-01-28 14:25:36 +0000
+++ src/platforms/mirserver/nativeinterface.cpp	2015-05-27 12:12:52 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2013 Canonical, Ltd.
+ * Copyright (C) 2013-2015 Canonical, Ltd.
  *
  * This program is free software: you can redistribute it and/or modify it under
  * the terms of the GNU Lesser General Public License version 3, as published by
@@ -18,7 +18,7 @@
 
 #include "nativeinterface.h"
 
-NativeInterface::NativeInterface(const QSharedPointer<MirServer> &server)
+NativeInterface::NativeInterface(const QWeakPointer<MirServer> &server)
     : m_mirServer(server)
 {
 }
@@ -27,14 +27,17 @@
 {
     void *result = nullptr;
 
-    if (resource == "SessionAuthorizer")
-        result = m_mirServer->sessionAuthorizer();
-    else if (resource == "Shell")
-        result = m_mirServer->shell();
-    else if (resource == "SessionListener")
-        result = m_mirServer->sessionListener();
-    else if (resource == "PromptSessionListener")
-        result = m_mirServer->promptSessionListener();
+    auto const server = m_mirServer.lock();
 
+    if (server) {
+        if (resource == "SessionAuthorizer")
+            result = server->sessionAuthorizer();
+        else if (resource == "Shell")
+            result = server->shell();
+        else if (resource == "SessionListener")
+            result = server->sessionListener();
+        else if (resource == "PromptSessionListener")
+            result = server->promptSessionListener();
+    }
     return result;
 }

=== modified file 'src/platforms/mirserver/nativeinterface.h'
--- src/platforms/mirserver/nativeinterface.h	2014-12-01 11:05:01 +0000
+++ src/platforms/mirserver/nativeinterface.h	2015-05-27 12:12:52 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2013 Canonical, Ltd.
+ * Copyright (C) 2013-2015 Canonical, Ltd.
  *
  * This program is free software: you can redistribute it and/or modify it under
  * the terms of the GNU Lesser General Public License version 3, as published by
@@ -29,11 +29,11 @@
 class NativeInterface : public QPlatformNativeInterface
 {
 public:
-    NativeInterface(const QSharedPointer<MirServer> &);
+    NativeInterface(const QWeakPointer<MirServer> &);
 
     virtual void *nativeResourceForIntegration(const QByteArray &resource);
 
-    QSharedPointer<MirServer> m_mirServer;
+    QWeakPointer<MirServer> m_mirServer;
 };
 
 #endif // NATIVEINTEGRATION_H

=== modified file 'src/platforms/mirserver/qmirserver.cpp'
--- src/platforms/mirserver/qmirserver.cpp	2015-04-27 15:59:42 +0000
+++ src/platforms/mirserver/qmirserver.cpp	2015-05-27 12:12:52 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2013 Canonical, Ltd.
+ * Copyright (C) 2013-2015 Canonical, Ltd.
  *
  * This program is free software: you can redistribute it and/or modify it under
  * the terms of the GNU Lesser General Public License version 3, as published by
@@ -19,80 +19,77 @@
 #include <QCoreApplication>
 #include <QDebug>
 
-#include <mir/main_loop.h>
-
 // local
+#include "mirserver.h"
 #include "qmirserver.h"
-
-
-void MirServerWorker::run()
-{
-    auto const main_loop = server->the_main_loop();
-    // By enqueuing the notification code in the main loop, we are
-    // ensuring that the server has really and fully started before
-    // leaving wait_for_startup().
-    main_loop->enqueue(
-        this,
-        [&]
-        {
-            std::lock_guard<std::mutex> lock(mutex);
-            mir_running = true;
-            started_cv.notify_one();
-        });
-
-    server->run();
-    Q_EMIT stopped();
-}
-
-bool MirServerWorker::wait_for_mir_startup()
-{
-    std::unique_lock<decltype(mutex)> lock(mutex);
-    started_cv.wait_for(lock, std::chrono::seconds{10}, [&]{ return mir_running; });
-    return mir_running;
-}
-
-QMirServer::QMirServer(const QSharedPointer<MirServer> &server, QObject *parent)
+#include "qmirserver_p.h"
+
+
+QMirServer::QMirServer(const QStringList &arguments, QObject *parent)
     : QObject(parent)
-    , m_mirServer(new MirServerWorker(server))
-{
-    m_mirServer->moveToThread(&m_mirThread);
-
-    connect(this, &QMirServer::run, m_mirServer, &MirServerWorker::run);
-    connect(this, &QMirServer::stop, m_mirServer, &MirServerWorker::stop);
-
-    connect(QCoreApplication::instance(), &QCoreApplication::aboutToQuit, this, &QMirServer::shutDownMirServer);
-    connect(m_mirServer, &MirServerWorker::stopped, this, &QMirServer::shutDownQApplication, Qt::DirectConnection); // since no event loop
-
-    m_mirThread.start(QThread::TimeCriticalPriority);
-    Q_EMIT run();
-
-    if (!m_mirServer->wait_for_mir_startup())
+    , d_ptr(new QMirServerPrivate())
+{
+    Q_D(QMirServer);
+
+    // convert arguments back into argc-argv form that Mir wants
+    int argc = arguments.size();
+    char **argv = new char*[argc + 1];
+    for (int i = 0; i < argc; i++) {
+        argv[i] = new char[strlen(arguments.at(i).toStdString().c_str())+1];
+        memcpy(argv[i], arguments.at(i).toStdString().c_str(), strlen(arguments.at(i).toStdString().c_str())+1);
+    }
+    argv[argc] = '\0';
+
+    d->server = QSharedPointer<MirServer>(new MirServer(argc, const_cast<const char**>(argv)));
+
+    d->serverThread = new MirServerThread(d->server);
+
+    connect(d->serverThread, &MirServerThread::stopped, this, &QMirServer::stopped);
+}
+
+QMirServer::~QMirServer()
+{
+    stop();
+}
+
+bool QMirServer::start()
+{
+    Q_D(QMirServer);
+
+    d->serverThread->start(QThread::TimeCriticalPriority);
+
+    if (!d->serverThread->waitForMirStartup())
     {
         qCritical() << "ERROR: QMirServer - Mir failed to start";
-        exit(2);
-    }
-}
-
-QMirServer::~QMirServer()
-{
-    shutDownMirServer();
-}
-
-void QMirServer::shutDownMirServer()
-{
-    if (m_mirThread.isRunning()) {
-        m_mirServer->stop();
-        m_mirThread.wait();
-    }
-}
-
-void QMirServer::shutDownQApplication()
-{
-    if (m_mirThread.isRunning())
-        m_mirThread.quit();
-
-    // if unexpected mir server stop, better quit the QApplication
-    if (!QCoreApplication::closingDown()) {
-        QCoreApplication::quit();
-    }
+        return false;
+    }
+
+    Q_EMIT started();
+    return true;
+}
+
+void QMirServer::stop()
+{
+    Q_D(QMirServer);
+
+    if (d->serverThread->isRunning()) {
+        d->serverThread->stop();
+        if (!d->serverThread->wait(10000)) {
+            // do something to indicate fail during shutdown
+            qCritical() << "ERROR: QMirServer - Mir failed to shut down correctly, terminating it";
+            d->serverThread->terminate();
+        }
+    }
+}
+
+bool QMirServer::isRunning() const
+{
+    Q_D(const QMirServer);
+    return d->serverThread->isRunning();
+}
+
+QWeakPointer<MirServer> QMirServer::mirServer() const
+{
+    Q_D(const QMirServer);
+    return d->server.toWeakRef();
 }

=== modified file 'src/platforms/mirserver/qmirserver.h'
--- src/platforms/mirserver/qmirserver.h	2014-12-02 14:55:17 +0000
+++ src/platforms/mirserver/qmirserver.h	2015-05-27 12:12:52 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2013 Canonical, Ltd.
+ * Copyright (C) 2013-2015 Canonical, Ltd.
  *
  * This program is free software: you can redistribute it and/or modify it under
  * the terms of the GNU Lesser General Public License version 3, as published by
@@ -19,63 +19,35 @@
 
 // Qt
 #include <QObject>
-#include <QThread>
-#include <QSharedPointer>
-
-// local
-#include "mirserver.h"
-
-#include <condition_variable>
-#include <mutex>
-
-// Wrap mir::Server with QObject, so it can be controlled via QThread
-class MirServerWorker : public QObject
+#include <QWeakPointer>
+
+class QMirServerPrivate;
+class MirServer;
+
+class QMirServer: public QObject
 {
     Q_OBJECT
 
 public:
-    MirServerWorker(const QSharedPointer<MirServer> &server)
-        : server(server)
-    {}
-
-    bool wait_for_mir_startup();
+    QMirServer(const QStringList &arguments, QObject* parent=0);
+    virtual ~QMirServer();
+
+    bool start();
+    Q_SLOT void stop();
+    bool isRunning() const;
+
+    QWeakPointer<MirServer> mirServer() const;
 
 Q_SIGNALS:
+    void started();
     void stopped();
 
-public Q_SLOTS:
-    void run();
-    void stop() { server->stop(); }
-
-private:
-    std::mutex mutex;
-    std::condition_variable started_cv;
-    bool mir_running{false};
-
-    const QSharedPointer<MirServer> server;
-};
-
-
-class QMirServer: public QObject
-{
-    Q_OBJECT
-
-public:
-    QMirServer(const QSharedPointer<MirServer> &config, QObject* parent=0);
-    ~QMirServer();
-
-Q_SIGNALS:
-    void run();
-    void stop();
-
-protected Q_SLOTS:
-    void shutDownMirServer();
-    void shutDownQApplication();
-
-private:
-    QThread m_mirThread;
-    MirServerWorker *m_mirServer;
+protected:
+    QMirServerPrivate * const d_ptr;
+
+private:
     Q_DISABLE_COPY(QMirServer)
+    Q_DECLARE_PRIVATE(QMirServer)
 };
 
 #endif // QMIRSERVER_H

=== added file 'src/platforms/mirserver/qmirserver_p.cpp'
--- src/platforms/mirserver/qmirserver_p.cpp	1970-01-01 00:00:00 +0000
+++ src/platforms/mirserver/qmirserver_p.cpp	2015-05-27 12:12:52 +0000
@@ -0,0 +1,53 @@
+/*
+ * Copyright (C) 2015 Canonical, Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it under
+ * the terms of the GNU Lesser General Public License version 3, as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranties of MERCHANTABILITY,
+ * SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+// Mir
+#include <mir/main_loop.h>
+
+// local
+#include "qmirserver_p.h"
+
+
+void MirServerThread::run()
+{
+    auto const main_loop = server->the_main_loop();
+    // By enqueuing the notification code in the main loop, we are
+    // ensuring that the server has really and fully started before
+    // leaving wait_for_startup().
+    main_loop->enqueue(
+        this,
+        [&]
+        {
+            std::lock_guard<std::mutex> lock(mutex);
+            mir_running = true;
+            started_cv.notify_one();
+        });
+
+    server->run(); // blocks until Mir server stopped
+    Q_EMIT stopped();
+}
+
+void MirServerThread::stop()
+{
+    server->stop();
+}
+
+bool MirServerThread::waitForMirStartup()
+{
+    std::unique_lock<decltype(mutex)> lock(mutex);
+    started_cv.wait_for(lock, std::chrono::seconds{10}, [&]{ return mir_running; });
+    return mir_running;
+}

=== added file 'src/platforms/mirserver/qmirserver_p.h'
--- src/platforms/mirserver/qmirserver_p.h	1970-01-01 00:00:00 +0000
+++ src/platforms/mirserver/qmirserver_p.h	2015-05-27 12:12:52 +0000
@@ -0,0 +1,67 @@
+/*
+ * Copyright (C) 2015 Canonical, Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it under
+ * the terms of the GNU Lesser General Public License version 3, as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranties of MERCHANTABILITY,
+ * SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef QMIRSERVER_P_H
+#define QMIRSERVER_P_H
+
+// Qt
+#include <QThread>
+#include <QSharedPointer>
+
+// std
+#include <condition_variable>
+#include <mutex>
+
+// local
+#include "mirserver.h"
+
+class QMirServer;
+class MirServerThread;
+
+struct QMirServerPrivate
+{
+    QSharedPointer<MirServer> server;
+    MirServerThread *serverThread;
+};
+
+
+class MirServerThread : public QThread
+{
+    Q_OBJECT
+
+public:
+    MirServerThread(const QSharedPointer<MirServer> &server)
+        : server(server)
+    {}
+
+    bool waitForMirStartup();
+
+Q_SIGNALS:
+    void stopped();
+
+public Q_SLOTS:
+    void run() override;
+    void stop();
+
+private:
+    std::mutex mutex;
+    std::condition_variable started_cv;
+    bool mir_running{false};
+
+    const QSharedPointer<MirServer> server;
+};
+
+#endif // QMIRSERVER_P_H