QtMir

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

  1. qmirserver-hides-mirserver
  2. Merge into trunk
Proposed by Gerry Boland on 2015-05-01
Status: Merged
Approved by: Daniel d'Andrada on 2015-05-29
Approved revision: 340
Merged at revision: 344
Proposed branch: lp:~gerboland/qtmir/qmirserver-hides-mirserver
Merge into: lp:qtmir
Diff against target: 750 lines (+279/-192)
14 files modified
demos/qml-demo-shell/qml-demo-shell.qml (+15/-0)
src/platforms/mirserver/CMakeLists.txt (+1/-0)
src/platforms/mirserver/display.cpp (+1/-6)
src/platforms/mirserver/display.h (+7/-9)
src/platforms/mirserver/miropenglcontext.cpp (+2/-3)
src/platforms/mirserver/miropenglcontext.h (+0/-1)
src/platforms/mirserver/mirserverintegration.cpp (+24/-35)
src/platforms/mirserver/mirserverintegration.h (+3/-4)
src/platforms/mirserver/nativeinterface.cpp (+13/-10)
src/platforms/mirserver/nativeinterface.h (+3/-3)
src/platforms/mirserver/qmirserver.cpp (+69/-72)
src/platforms/mirserver/qmirserver.h (+21/-49)
src/platforms/mirserver/qmirserver_p.cpp (+53/-0)
src/platforms/mirserver/qmirserver_p.h (+67/-0)
To merge this branch: bzr merge lp:~gerboland/qtmir/qmirserver-hides-mirserver
Reviewer Review Type Date Requested Status
Daniel d'Andrada (community) 2015-05-01 Approve on 2015-05-29
PS Jenkins bot continuous-integration Needs Fixing on 2015-05-27
Review via email: mp+258038@code.launchpad.net

Commit Message

[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

To post a comment you must log in.
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Daniel d'Andrada (dandrader) wrote :

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) wrote :

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

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

review: Needs Information
Daniel d'Andrada (dandrader) wrote :

> 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) wrote :

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) wrote :

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) 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
Daniel d'Andrada (dandrader) wrote :

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) wrote :

> 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) wrote :

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

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) wrote :

> 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) 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.
>
> 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) wrote :

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 updated on 2015-05-18
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) wrote :
review: Needs Fixing (continuous-integration)
lp:~gerboland/qtmir/qmirserver-hides-mirserver updated on 2015-05-18
337. By Gerry Boland on 2015-05-18

Merge qtmir trunk

Gerry Boland (gerboland) wrote :

> > 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) wrote :
review: Approve (continuous-integration)
lp:~gerboland/qtmir/qmirserver-hides-mirserver updated on 2015-05-19
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) wrote :
review: Needs Fixing (continuous-integration)
lp:~gerboland/qtmir/qmirserver-hides-mirserver updated on 2015-05-27
340. By Gerry Boland on 2015-05-27

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

Gerry Boland (gerboland) wrote :

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) wrote :

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

Is this signal being used for anything?

review: Needs Information
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Gerry Boland (gerboland) wrote :

> """
> 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) wrote :

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

review: Approve (code)
Daniel d'Andrada (dandrader) wrote :

* 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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'demos/qml-demo-shell/qml-demo-shell.qml'
2--- demos/qml-demo-shell/qml-demo-shell.qml 2015-01-23 10:23:07 +0000
3+++ demos/qml-demo-shell/qml-demo-shell.qml 2015-05-27 12:12:52 +0000
4@@ -110,6 +110,21 @@
5 y: point.y
6 }
7
8+ Rectangle {
9+ width: 60
10+ height: 40
11+ color: "red"
12+ anchors { right: parent.right; bottom: parent.bottom }
13+ Text {
14+ anchors.centerIn: parent
15+ text: "Quit"
16+ }
17+ MouseArea {
18+ anchors.fill: parent
19+ onClicked: Qt.quit()
20+ }
21+ }
22+
23 Connections {
24 target: SurfaceManager
25 onSurfaceCreated: {
26
27=== modified file 'src/platforms/mirserver/CMakeLists.txt'
28--- src/platforms/mirserver/CMakeLists.txt 2015-05-13 09:40:03 +0000
29+++ src/platforms/mirserver/CMakeLists.txt 2015-05-27 12:12:52 +0000
30@@ -44,6 +44,7 @@
31 qteventfeeder.cpp
32 plugin.cpp
33 qmirserver.cpp
34+ qmirserver_p.cpp
35 sessionauthorizer.cpp
36 sessionlistener.cpp
37 surfaceobserver.cpp
38
39=== modified file 'src/platforms/mirserver/display.cpp'
40--- src/platforms/mirserver/display.cpp 2014-12-01 11:10:54 +0000
41+++ src/platforms/mirserver/display.cpp 2015-05-27 12:12:52 +0000
42@@ -23,18 +23,13 @@
43
44 #include <mir/graphics/display.h>
45 #include <mir/graphics/display_configuration.h>
46-#include <QDebug>
47
48 namespace mg = mir::graphics;
49
50 // TODO: Listen for display changes and update the list accordingly
51
52-Display::Display(const QSharedPointer<MirServer> &server, QObject *parent)
53- : QObject(parent)
54- , m_mirServer(server)
55+Display::Display(const std::shared_ptr<mir::graphics::DisplayConfiguration> &displayConfig)
56 {
57- std::shared_ptr<mir::graphics::DisplayConfiguration> displayConfig = m_mirServer->the_display()->configuration();
58-
59 displayConfig->for_each_output([this](mg::DisplayConfigurationOutput const& output) {
60 if (output.used) {
61 auto screen = new Screen(output);
62
63=== modified file 'src/platforms/mirserver/display.h'
64--- src/platforms/mirserver/display.h 2014-12-01 11:10:54 +0000
65+++ src/platforms/mirserver/display.h 2015-05-27 12:12:52 +0000
66@@ -19,23 +19,21 @@
67 #ifndef DISPLAY_H
68 #define DISPLAY_H
69
70-#include <QObject>
71 #include <qpa/qplatformscreen.h>
72-
73-class MirServer;
74-
75-class Display : public QObject
76+#include <memory>
77+
78+namespace mir { namespace graphics { class DisplayConfiguration; }}
79+
80+class Display
81 {
82- Q_OBJECT
83 public:
84- Display(const QSharedPointer<MirServer> &server, QObject *parent = 0);
85- ~Display();
86+ Display(const std::shared_ptr<mir::graphics::DisplayConfiguration> &displayConfig);
87+ virtual ~Display();
88
89 QList<QPlatformScreen *> screens() const { return m_screens; }
90
91 private:
92 QList<QPlatformScreen *> m_screens;
93- const QSharedPointer<MirServer> m_mirServer;
94 };
95
96 #endif // DISPLAY_H
97
98=== modified file 'src/platforms/mirserver/miropenglcontext.cpp'
99--- src/platforms/mirserver/miropenglcontext.cpp 2015-01-12 17:54:55 +0000
100+++ src/platforms/mirserver/miropenglcontext.cpp 2015-05-27 12:12:52 +0000
101@@ -36,12 +36,11 @@
102 // (i.e. individual display output buffers) to use as a common base context.
103
104 MirOpenGLContext::MirOpenGLContext(const QSharedPointer<MirServer> &server, const QSurfaceFormat &format)
105- : m_mirServer(server)
106 #if GL_DEBUG
107- , m_logger(new QOpenGLDebugLogger(this))
108+ : m_logger(new QOpenGLDebugLogger(this))
109 #endif
110 {
111- std::shared_ptr<mir::graphics::Display> display = m_mirServer->the_display();
112+ std::shared_ptr<mir::graphics::Display> display = server->the_display();
113
114 // create a temporary GL context to fetch the EGL display and config, so Qt can determine the surface format
115 std::unique_ptr<mir::graphics::GLContext> mirContext = display->create_gl_context();
116
117=== modified file 'src/platforms/mirserver/miropenglcontext.h'
118--- src/platforms/mirserver/miropenglcontext.h 2014-12-01 11:10:54 +0000
119+++ src/platforms/mirserver/miropenglcontext.h 2015-05-27 12:12:52 +0000
120@@ -52,7 +52,6 @@
121 #endif
122
123 private:
124- const QSharedPointer<MirServer> m_mirServer;
125 QSurfaceFormat m_format;
126 #if GL_DEBUG
127 QOpenGLDebugLogger *m_logger;
128
129=== modified file 'src/platforms/mirserver/mirserverintegration.cpp'
130--- src/platforms/mirserver/mirserverintegration.cpp 2015-04-01 10:48:28 +0000
131+++ src/platforms/mirserver/mirserverintegration.cpp 2015-05-27 12:12:52 +0000
132@@ -1,5 +1,5 @@
133 /*
134- * Copyright (C) 2013-2014 Canonical, Ltd.
135+ * Copyright (C) 2013-2015 Canonical, Ltd.
136 *
137 * This program is free software: you can redistribute it and/or modify it under
138 * the terms of the GNU Lesser General Public License version 3, as published by
139@@ -30,7 +30,6 @@
140 #include <qpa/qwindowsysteminterface.h>
141
142 #include <QCoreApplication>
143-#include <QStringList>
144 #include <QOpenGLContext>
145
146 #if QT_VERSION < QT_VERSION_CHECK(5, 2, 0)
147@@ -41,10 +40,7 @@
148
149 // Mir
150 #include <mir/graphics/display.h>
151-#include <mir/graphics/display_buffer.h>
152-
153-// std
154-#include <csignal>
155+#include <mir/graphics/display_configuration.h>
156
157 // local
158 #include "clipboard.h"
159@@ -66,36 +62,25 @@
160 #if QT_VERSION < QT_VERSION_CHECK(5, 2, 0)
161 , m_eventDispatcher(createUnixEventDispatcher())
162 #endif
163+ , m_mirServer(new QMirServer(QCoreApplication::arguments()))
164 , m_display(nullptr)
165- , m_qmirServer(nullptr)
166 , m_nativeInterface(nullptr)
167 , m_clipboard(new Clipboard)
168 {
169- // Start Mir server only once Qt has initialized its event dispatcher, see initialize()
170-
171- QStringList args = QCoreApplication::arguments();
172- // convert arguments back into argc-argv form that Mir wants
173- char **argv;
174- argv = new char*[args.size() + 1];
175- for (int i = 0; i < args.size(); i++) {
176- argv[i] = new char[strlen(args.at(i).toStdString().c_str())+1];
177- memcpy(argv[i], args.at(i).toStdString().c_str(), strlen(args.at(i).toStdString().c_str())+1);
178- }
179- argv[args.size()] = '\0';
180-
181 // For access to sensors, qtmir uses qtubuntu-sensors. qtubuntu-sensors reads the
182 // UBUNTU_PLATFORM_API_BACKEND variable to decide if to load a valid sensor backend or not.
183 // For it to function we need to ensure a valid backend has been specified
184 if (qEnvironmentVariableIsEmpty("UBUNTU_PLATFORM_API_BACKEND")) {
185- if (qgetenv("DESKTOP_SESSION").contains("mir")) {
186+ if (qgetenv("DESKTOP_SESSION").contains("mir") || !qEnvironmentVariableIsSet("ANDROID_DATA")) {
187 qputenv("UBUNTU_PLATFORM_API_BACKEND", "desktop_mirclient");
188 } else {
189 qputenv("UBUNTU_PLATFORM_API_BACKEND", "touch_mirclient");
190 }
191 }
192
193- m_mirServer = QSharedPointer<MirServer>(
194- new MirServer(args.length(), const_cast<const char**>(argv)));
195+ // If Mir shuts down, quit.
196+ QObject::connect(m_mirServer.data(), &QMirServer::stopped,
197+ QCoreApplication::instance(), &QCoreApplication::quit);
198
199 #if QT_VERSION < QT_VERSION_CHECK(5, 2, 0)
200 QGuiApplicationPrivate::instance()->setEventDispatcher(eventDispatcher_);
201@@ -109,7 +94,6 @@
202 {
203 delete m_nativeInterface;
204 delete m_display;
205- delete m_qmirServer;
206 }
207
208 bool MirServerIntegration::hasCapability(QPlatformIntegration::Capability cap) const
209@@ -135,18 +119,21 @@
210
211 DisplayWindow* displayWindow = nullptr;
212
213+ auto const mirServer = m_mirServer->mirServer().lock();
214 mg::DisplayBuffer* first_buffer{nullptr};
215 mg::DisplaySyncGroup* first_group{nullptr};
216- m_mirServer->the_display()->for_each_display_sync_group([&](mg::DisplaySyncGroup &group) {
217- if (!first_group) {
218- first_group = &group;
219- }
220- group.for_each_display_buffer([&](mg::DisplayBuffer &buffer) {
221- if (!first_buffer) {
222- first_buffer = &buffer;
223+ if (mirServer) {
224+ mirServer->the_display()->for_each_display_sync_group([&](mg::DisplaySyncGroup &group) {
225+ if (!first_group) {
226+ first_group = &group;
227 }
228+ group.for_each_display_buffer([&](mg::DisplayBuffer &buffer) {
229+ if (!first_buffer) {
230+ first_buffer = &buffer;
231+ }
232+ });
233 });
234- });
235+ }
236
237 // FIXME(gerry) this will go very bad for >1 display buffer
238 if (first_group && first_buffer)
239@@ -168,7 +155,7 @@
240 QPlatformOpenGLContext *MirServerIntegration::createPlatformOpenGLContext(QOpenGLContext *context) const
241 {
242 qDebug() << "createPlatformOpenGLContext" << context;
243- return new MirOpenGLContext(m_mirServer, context->format());
244+ return new MirOpenGLContext(m_mirServer->mirServer(), context->format());
245 }
246
247 #if QT_VERSION >= QT_VERSION_CHECK(5, 2, 0)
248@@ -181,10 +168,12 @@
249 void MirServerIntegration::initialize()
250 {
251 // Creates instance of and start the Mir server in a separate thread
252- m_qmirServer = new QMirServer(m_mirServer);
253+ if (!m_mirServer->start()) {
254+ exit(2);
255+ }
256
257- m_display = new Display(m_mirServer);
258- m_nativeInterface = new NativeInterface(m_mirServer);
259+ m_display = new Display(m_mirServer->mirServer().data()->the_display()->configuration());
260+ m_nativeInterface = new NativeInterface(m_mirServer->mirServer());
261
262 for (QPlatformScreen *screen : m_display->screens())
263 screenAdded(screen);
264
265=== modified file 'src/platforms/mirserver/mirserverintegration.h'
266--- src/platforms/mirserver/mirserverintegration.h 2014-12-11 15:27:02 +0000
267+++ src/platforms/mirserver/mirserverintegration.h 2015-05-27 12:12:52 +0000
268@@ -1,5 +1,5 @@
269 /*
270- * Copyright (C) 2013-2014 Canonical, Ltd.
271+ * Copyright (C) 2013-2015 Canonical, Ltd.
272 *
273 * This program is free software: you can redistribute it and/or modify it under
274 * the terms of the GNU Lesser General Public License version 3, as published by
275@@ -69,8 +69,6 @@
276 QPlatformNativeInterface *nativeInterface() const override;
277
278 private:
279- QSharedPointer<MirServer> m_mirServer;
280-
281 QScopedPointer<QPlatformAccessibility> m_accessibility;
282 QScopedPointer<QPlatformFontDatabase> m_fontDb;
283 QScopedPointer<QPlatformServices> m_services;
284@@ -78,8 +76,9 @@
285 QScopedPointer<QAbstractEventDispatcher> m_eventDispatcher;
286 #endif
287
288+ QScopedPointer<QMirServer> m_mirServer;
289+
290 Display *m_display;
291- QMirServer *m_qmirServer;
292 NativeInterface *m_nativeInterface;
293 QPlatformInputContext* m_inputContext;
294 QScopedPointer<qtmir::Clipboard> m_clipboard;
295
296=== modified file 'src/platforms/mirserver/nativeinterface.cpp'
297--- src/platforms/mirserver/nativeinterface.cpp 2015-01-28 14:25:36 +0000
298+++ src/platforms/mirserver/nativeinterface.cpp 2015-05-27 12:12:52 +0000
299@@ -1,5 +1,5 @@
300 /*
301- * Copyright (C) 2013 Canonical, Ltd.
302+ * Copyright (C) 2013-2015 Canonical, Ltd.
303 *
304 * This program is free software: you can redistribute it and/or modify it under
305 * the terms of the GNU Lesser General Public License version 3, as published by
306@@ -18,7 +18,7 @@
307
308 #include "nativeinterface.h"
309
310-NativeInterface::NativeInterface(const QSharedPointer<MirServer> &server)
311+NativeInterface::NativeInterface(const QWeakPointer<MirServer> &server)
312 : m_mirServer(server)
313 {
314 }
315@@ -27,14 +27,17 @@
316 {
317 void *result = nullptr;
318
319- if (resource == "SessionAuthorizer")
320- result = m_mirServer->sessionAuthorizer();
321- else if (resource == "Shell")
322- result = m_mirServer->shell();
323- else if (resource == "SessionListener")
324- result = m_mirServer->sessionListener();
325- else if (resource == "PromptSessionListener")
326- result = m_mirServer->promptSessionListener();
327+ auto const server = m_mirServer.lock();
328
329+ if (server) {
330+ if (resource == "SessionAuthorizer")
331+ result = server->sessionAuthorizer();
332+ else if (resource == "Shell")
333+ result = server->shell();
334+ else if (resource == "SessionListener")
335+ result = server->sessionListener();
336+ else if (resource == "PromptSessionListener")
337+ result = server->promptSessionListener();
338+ }
339 return result;
340 }
341
342=== modified file 'src/platforms/mirserver/nativeinterface.h'
343--- src/platforms/mirserver/nativeinterface.h 2014-12-01 11:05:01 +0000
344+++ src/platforms/mirserver/nativeinterface.h 2015-05-27 12:12:52 +0000
345@@ -1,5 +1,5 @@
346 /*
347- * Copyright (C) 2013 Canonical, Ltd.
348+ * Copyright (C) 2013-2015 Canonical, Ltd.
349 *
350 * This program is free software: you can redistribute it and/or modify it under
351 * the terms of the GNU Lesser General Public License version 3, as published by
352@@ -29,11 +29,11 @@
353 class NativeInterface : public QPlatformNativeInterface
354 {
355 public:
356- NativeInterface(const QSharedPointer<MirServer> &);
357+ NativeInterface(const QWeakPointer<MirServer> &);
358
359 virtual void *nativeResourceForIntegration(const QByteArray &resource);
360
361- QSharedPointer<MirServer> m_mirServer;
362+ QWeakPointer<MirServer> m_mirServer;
363 };
364
365 #endif // NATIVEINTEGRATION_H
366
367=== modified file 'src/platforms/mirserver/qmirserver.cpp'
368--- src/platforms/mirserver/qmirserver.cpp 2015-04-27 15:59:42 +0000
369+++ src/platforms/mirserver/qmirserver.cpp 2015-05-27 12:12:52 +0000
370@@ -1,5 +1,5 @@
371 /*
372- * Copyright (C) 2013 Canonical, Ltd.
373+ * Copyright (C) 2013-2015 Canonical, Ltd.
374 *
375 * This program is free software: you can redistribute it and/or modify it under
376 * the terms of the GNU Lesser General Public License version 3, as published by
377@@ -19,80 +19,77 @@
378 #include <QCoreApplication>
379 #include <QDebug>
380
381-#include <mir/main_loop.h>
382-
383 // local
384+#include "mirserver.h"
385 #include "qmirserver.h"
386-
387-
388-void MirServerWorker::run()
389-{
390- auto const main_loop = server->the_main_loop();
391- // By enqueuing the notification code in the main loop, we are
392- // ensuring that the server has really and fully started before
393- // leaving wait_for_startup().
394- main_loop->enqueue(
395- this,
396- [&]
397- {
398- std::lock_guard<std::mutex> lock(mutex);
399- mir_running = true;
400- started_cv.notify_one();
401- });
402-
403- server->run();
404- Q_EMIT stopped();
405-}
406-
407-bool MirServerWorker::wait_for_mir_startup()
408-{
409- std::unique_lock<decltype(mutex)> lock(mutex);
410- started_cv.wait_for(lock, std::chrono::seconds{10}, [&]{ return mir_running; });
411- return mir_running;
412-}
413-
414-QMirServer::QMirServer(const QSharedPointer<MirServer> &server, QObject *parent)
415+#include "qmirserver_p.h"
416+
417+
418+QMirServer::QMirServer(const QStringList &arguments, QObject *parent)
419 : QObject(parent)
420- , m_mirServer(new MirServerWorker(server))
421-{
422- m_mirServer->moveToThread(&m_mirThread);
423-
424- connect(this, &QMirServer::run, m_mirServer, &MirServerWorker::run);
425- connect(this, &QMirServer::stop, m_mirServer, &MirServerWorker::stop);
426-
427- connect(QCoreApplication::instance(), &QCoreApplication::aboutToQuit, this, &QMirServer::shutDownMirServer);
428- connect(m_mirServer, &MirServerWorker::stopped, this, &QMirServer::shutDownQApplication, Qt::DirectConnection); // since no event loop
429-
430- m_mirThread.start(QThread::TimeCriticalPriority);
431- Q_EMIT run();
432-
433- if (!m_mirServer->wait_for_mir_startup())
434+ , d_ptr(new QMirServerPrivate())
435+{
436+ Q_D(QMirServer);
437+
438+ // convert arguments back into argc-argv form that Mir wants
439+ int argc = arguments.size();
440+ char **argv = new char*[argc + 1];
441+ for (int i = 0; i < argc; i++) {
442+ argv[i] = new char[strlen(arguments.at(i).toStdString().c_str())+1];
443+ memcpy(argv[i], arguments.at(i).toStdString().c_str(), strlen(arguments.at(i).toStdString().c_str())+1);
444+ }
445+ argv[argc] = '\0';
446+
447+ d->server = QSharedPointer<MirServer>(new MirServer(argc, const_cast<const char**>(argv)));
448+
449+ d->serverThread = new MirServerThread(d->server);
450+
451+ connect(d->serverThread, &MirServerThread::stopped, this, &QMirServer::stopped);
452+}
453+
454+QMirServer::~QMirServer()
455+{
456+ stop();
457+}
458+
459+bool QMirServer::start()
460+{
461+ Q_D(QMirServer);
462+
463+ d->serverThread->start(QThread::TimeCriticalPriority);
464+
465+ if (!d->serverThread->waitForMirStartup())
466 {
467 qCritical() << "ERROR: QMirServer - Mir failed to start";
468- exit(2);
469- }
470-}
471-
472-QMirServer::~QMirServer()
473-{
474- shutDownMirServer();
475-}
476-
477-void QMirServer::shutDownMirServer()
478-{
479- if (m_mirThread.isRunning()) {
480- m_mirServer->stop();
481- m_mirThread.wait();
482- }
483-}
484-
485-void QMirServer::shutDownQApplication()
486-{
487- if (m_mirThread.isRunning())
488- m_mirThread.quit();
489-
490- // if unexpected mir server stop, better quit the QApplication
491- if (!QCoreApplication::closingDown()) {
492- QCoreApplication::quit();
493- }
494+ return false;
495+ }
496+
497+ Q_EMIT started();
498+ return true;
499+}
500+
501+void QMirServer::stop()
502+{
503+ Q_D(QMirServer);
504+
505+ if (d->serverThread->isRunning()) {
506+ d->serverThread->stop();
507+ if (!d->serverThread->wait(10000)) {
508+ // do something to indicate fail during shutdown
509+ qCritical() << "ERROR: QMirServer - Mir failed to shut down correctly, terminating it";
510+ d->serverThread->terminate();
511+ }
512+ }
513+}
514+
515+bool QMirServer::isRunning() const
516+{
517+ Q_D(const QMirServer);
518+ return d->serverThread->isRunning();
519+}
520+
521+QWeakPointer<MirServer> QMirServer::mirServer() const
522+{
523+ Q_D(const QMirServer);
524+ return d->server.toWeakRef();
525 }
526
527=== modified file 'src/platforms/mirserver/qmirserver.h'
528--- src/platforms/mirserver/qmirserver.h 2014-12-02 14:55:17 +0000
529+++ src/platforms/mirserver/qmirserver.h 2015-05-27 12:12:52 +0000
530@@ -1,5 +1,5 @@
531 /*
532- * Copyright (C) 2013 Canonical, Ltd.
533+ * Copyright (C) 2013-2015 Canonical, Ltd.
534 *
535 * This program is free software: you can redistribute it and/or modify it under
536 * the terms of the GNU Lesser General Public License version 3, as published by
537@@ -19,63 +19,35 @@
538
539 // Qt
540 #include <QObject>
541-#include <QThread>
542-#include <QSharedPointer>
543-
544-// local
545-#include "mirserver.h"
546-
547-#include <condition_variable>
548-#include <mutex>
549-
550-// Wrap mir::Server with QObject, so it can be controlled via QThread
551-class MirServerWorker : public QObject
552+#include <QWeakPointer>
553+
554+class QMirServerPrivate;
555+class MirServer;
556+
557+class QMirServer: public QObject
558 {
559 Q_OBJECT
560
561 public:
562- MirServerWorker(const QSharedPointer<MirServer> &server)
563- : server(server)
564- {}
565-
566- bool wait_for_mir_startup();
567+ QMirServer(const QStringList &arguments, QObject* parent=0);
568+ virtual ~QMirServer();
569+
570+ bool start();
571+ Q_SLOT void stop();
572+ bool isRunning() const;
573+
574+ QWeakPointer<MirServer> mirServer() const;
575
576 Q_SIGNALS:
577+ void started();
578 void stopped();
579
580-public Q_SLOTS:
581- void run();
582- void stop() { server->stop(); }
583-
584-private:
585- std::mutex mutex;
586- std::condition_variable started_cv;
587- bool mir_running{false};
588-
589- const QSharedPointer<MirServer> server;
590-};
591-
592-
593-class QMirServer: public QObject
594-{
595- Q_OBJECT
596-
597-public:
598- QMirServer(const QSharedPointer<MirServer> &config, QObject* parent=0);
599- ~QMirServer();
600-
601-Q_SIGNALS:
602- void run();
603- void stop();
604-
605-protected Q_SLOTS:
606- void shutDownMirServer();
607- void shutDownQApplication();
608-
609-private:
610- QThread m_mirThread;
611- MirServerWorker *m_mirServer;
612+protected:
613+ QMirServerPrivate * const d_ptr;
614+
615+private:
616 Q_DISABLE_COPY(QMirServer)
617+ Q_DECLARE_PRIVATE(QMirServer)
618 };
619
620 #endif // QMIRSERVER_H
621
622=== added file 'src/platforms/mirserver/qmirserver_p.cpp'
623--- src/platforms/mirserver/qmirserver_p.cpp 1970-01-01 00:00:00 +0000
624+++ src/platforms/mirserver/qmirserver_p.cpp 2015-05-27 12:12:52 +0000
625@@ -0,0 +1,53 @@
626+/*
627+ * Copyright (C) 2015 Canonical, Ltd.
628+ *
629+ * This program is free software: you can redistribute it and/or modify it under
630+ * the terms of the GNU Lesser General Public License version 3, as published by
631+ * the Free Software Foundation.
632+ *
633+ * This program is distributed in the hope that it will be useful, but WITHOUT
634+ * ANY WARRANTY; without even the implied warranties of MERCHANTABILITY,
635+ * SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
636+ * Lesser General Public License for more details.
637+ *
638+ * You should have received a copy of the GNU Lesser General Public License
639+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
640+ */
641+
642+// Mir
643+#include <mir/main_loop.h>
644+
645+// local
646+#include "qmirserver_p.h"
647+
648+
649+void MirServerThread::run()
650+{
651+ auto const main_loop = server->the_main_loop();
652+ // By enqueuing the notification code in the main loop, we are
653+ // ensuring that the server has really and fully started before
654+ // leaving wait_for_startup().
655+ main_loop->enqueue(
656+ this,
657+ [&]
658+ {
659+ std::lock_guard<std::mutex> lock(mutex);
660+ mir_running = true;
661+ started_cv.notify_one();
662+ });
663+
664+ server->run(); // blocks until Mir server stopped
665+ Q_EMIT stopped();
666+}
667+
668+void MirServerThread::stop()
669+{
670+ server->stop();
671+}
672+
673+bool MirServerThread::waitForMirStartup()
674+{
675+ std::unique_lock<decltype(mutex)> lock(mutex);
676+ started_cv.wait_for(lock, std::chrono::seconds{10}, [&]{ return mir_running; });
677+ return mir_running;
678+}
679
680=== added file 'src/platforms/mirserver/qmirserver_p.h'
681--- src/platforms/mirserver/qmirserver_p.h 1970-01-01 00:00:00 +0000
682+++ src/platforms/mirserver/qmirserver_p.h 2015-05-27 12:12:52 +0000
683@@ -0,0 +1,67 @@
684+/*
685+ * Copyright (C) 2015 Canonical, Ltd.
686+ *
687+ * This program is free software: you can redistribute it and/or modify it under
688+ * the terms of the GNU Lesser General Public License version 3, as published by
689+ * the Free Software Foundation.
690+ *
691+ * This program is distributed in the hope that it will be useful, but WITHOUT
692+ * ANY WARRANTY; without even the implied warranties of MERCHANTABILITY,
693+ * SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
694+ * Lesser General Public License for more details.
695+ *
696+ * You should have received a copy of the GNU Lesser General Public License
697+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
698+ */
699+
700+#ifndef QMIRSERVER_P_H
701+#define QMIRSERVER_P_H
702+
703+// Qt
704+#include <QThread>
705+#include <QSharedPointer>
706+
707+// std
708+#include <condition_variable>
709+#include <mutex>
710+
711+// local
712+#include "mirserver.h"
713+
714+class QMirServer;
715+class MirServerThread;
716+
717+struct QMirServerPrivate
718+{
719+ QSharedPointer<MirServer> server;
720+ MirServerThread *serverThread;
721+};
722+
723+
724+class MirServerThread : public QThread
725+{
726+ Q_OBJECT
727+
728+public:
729+ MirServerThread(const QSharedPointer<MirServer> &server)
730+ : server(server)
731+ {}
732+
733+ bool waitForMirStartup();
734+
735+Q_SIGNALS:
736+ void stopped();
737+
738+public Q_SLOTS:
739+ void run() override;
740+ void stop();
741+
742+private:
743+ std::mutex mutex;
744+ std::condition_variable started_cv;
745+ bool mir_running{false};
746+
747+ const QSharedPointer<MirServer> server;
748+};
749+
750+#endif // QMIRSERVER_P_H

Subscribers

People subscribed via source and target branches