Merge lp:~gerboland/miral/qt-add-window-model-mir-side into lp:miral

[Gerry Boland]

Qt: implement basic Mir-side window "model" which emits signals describing the model changes to a consumer (to come later)

Primary problem to be solved: have a model of windows that Qt can implement a view for.

Qt unfortunately requires that the model and the view have matching thread affinity - meaning I cannot manage the model on Mir's threads and use locking to keep consistent state for the view.

Instead I need to create and manage the model on Qt's GUI thread (where the view will live). I will do this by firing signals from this Mir-side WindowModel which will describe how the model changes, signals which a GUI thread model will listen for and use to synchronize its state with.

[Alan Griffiths (alan-griffiths)]

Nits:

+// miral::WindowInfo has a copy constructor, but implicitly shares a Self object between each
+// copy. If a miral::WindowInfo copy is sent over signal/slot connection across thread boundaries,
+// it could be changed in a Mir thread before the slot processes it.
+//
+// This is undesirable as we need to update the GUI thread model in a consistent way.
+//
+// Instead we duplicate the miral::WindowInfo data, in a way that can be sent over signal/slot
+// connections safely.

Obsolete/misleading comment?

1. miral::WindowInfo doesn't share self, it copies it.
2. I don't see the duplicate.

~~~~

+// We assign each Window with a unique ID that both Mir-side and Qt-side WindowModels can share

Can't we just use a weak_ptr<Surface> as an ID?

review: Needs Fixing

[Gerry Boland (gerboland)]

> Nits:
>
> +// miral::WindowInfo has a copy constructor, but implicitly shares a Self
> object between each
> +// copy. If a miral::WindowInfo copy is sent over signal/slot connection
> across thread boundaries,
> +// it could be changed in a Mir thread before the slot processes it.
> +//
> +// This is undesirable as we need to update the GUI thread model in a
> consistent way.
> +//
> +// Instead we duplicate the miral::WindowInfo data, in a way that can be sent
> over signal/slot
> +// connections safely.
>
> Obsolete/misleading comment?

Yep, sorry. Fixed.

> +// We assign each Window with a unique ID that both Mir-side and Qt-side
> WindowModels can share
> Can't we just use a weak_ptr<Surface> as an ID?
Yes that's a much better idea. I was worried about lifetimes, but the view (qtmir::MirSurface) holds a copy of the shared_ptr<mir::scene::Surface> until it is deleted - deletion which only happens after its corresponding entry is removed from the model.

[Alan Griffiths (alan-griffiths)]

> > Can't we just use a weak_ptr<Surface> as an ID?
> Yes that's a much better idea. I was worried about lifetimes, but the view
> (qtmir::MirSurface) holds a copy of the shared_ptr<mir::scene::Surface> until
> it is deleted - deletion which only happens after its corresponding entry is
> removed from the model.

You can use a weak_ptr<Surface> - which avoids accidentally affecting the lifetime. (I do this in BasicWindowManager.)

[Gerry Boland (gerboland)]

> > Nits:
> >
> > +// miral::WindowInfo has a copy constructor, but implicitly shares a Self
> > object between each
> > +// copy. If a miral::WindowInfo copy is sent over signal/slot connection
> > across thread boundaries,
> > +// it could be changed in a Mir thread before the slot processes it.
> > +//
> > +// This is undesirable as we need to update the GUI thread model in a
> > consistent way.
> > +//
> > +// Instead we duplicate the miral::WindowInfo data, in a way that can be
> sent
> > over signal/slot
> > +// connections safely.
> >
> > Obsolete/misleading comment?
>
> Yep, sorry. Fixed.
>
>
> > +// We assign each Window with a unique ID that both Mir-side and Qt-side
> > WindowModels can share
> > Can't we just use a weak_ptr<Surface> as an ID?
> Yes that's a much better idea. I was worried about lifetimes, but the view
> (qtmir::MirSurface) holds a copy of the shared_ptr<mir::scene::Surface> until
> it is deleted - deletion which only happens after its corresponding entry is
> removed from the model.

Ah, not quite, there is more going on. I want the Mir-side window model (mirserver/windowmodel) to tell the Qt-side at what position in the stack a window is. I was (ab)using this id to indicate the position in the window stack.

So the comment is not quite accurate, I've updated it to be more correct.

[lp:~gerboland/miral/qt-add-window-model-mir-side]

240. By Gerry Boland on 2016-07-27

Old comment not relevent, corrected

241. By Gerry Boland on 2016-07-27

Another badly written comment updated

[Alan Griffiths (alan-griffiths)]

OK

review: Approve

=== added file 'miral-qt/src/common/mirqtconversion.h'
--- miral-qt/src/common/mirqtconversion.h	1970-01-01 00:00:00 +0000
+++ miral-qt/src/common/mirqtconversion.h	2016-07-27 16:33:34 +0000
@@ -0,0 +1,28 @@
+#ifndef MIRQTCONVERSION_H
+#define MIRQTCONVERSION_H
+
+#include <QSize>
+#include <QPoint>
+
+#include <mir/geometry/size.h>
+#include <mir/geometry/point.h>
+
+namespace qtmir {
+
+/*
+ * Some handy conversions from Mir types to Qt types
+ */
+
+inline QSize toQSize(mir::geometry::Size size)
+{
+    return QSize(size.width.as_int(), size.height.as_int());
+}
+
+inline QPoint toQPoint(mir::geometry::Point point)
+{
+    return QPoint(point.x.as_int(), point.y.as_int());
+}
+
+} // namespace qtmir
+
+#endif // MIRQTCONVERSION_H

=== added file 'miral-qt/src/common/windowmodelinterface.h'
--- miral-qt/src/common/windowmodelinterface.h	1970-01-01 00:00:00 +0000
+++ miral-qt/src/common/windowmodelinterface.h	2016-07-27 16:33:34 +0000
@@ -0,0 +1,78 @@
+/*
+ * Copyright (C) 2016 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 WINDOWMODELINTERFACE_H
+#define WINDOWMODELINTERFACE_H
+
+#include <QObject>
+#include <QSize>
+#include <QPoint>
+#include <QVector>
+
+#include <mir/scene/surface.h>
+
+namespace qtmir {
+
+// miral::WindowInfo contains all the metadata the WindowManager{,Policy} needs. However the
+// WindowModel only needs a read-only subset of this data, which is what the struct is for.
+struct WindowInfo {
+    QSize size;
+    QPoint position;
+    bool focused;
+    const std::shared_ptr<mir::scene::Surface> surface;
+
+    enum class DirtyStates {
+        Size = 1,
+        Position = 2,
+        Focus = 4
+    };
+};
+
+// We assign each Window with an index which corresponds to the position it has in the window stack.
+struct NumberedWindow {
+    unsigned int index;
+    WindowInfo windowInfo;
+};
+
+struct DirtiedWindow {
+    unsigned int index;
+    WindowInfo windowInfo;
+    WindowInfo::DirtyStates dirtyWindowInfo;
+};
+
+
+class WindowModelInterface : public QObject
+{
+    Q_OBJECT
+public:
+    WindowModelInterface() = default;
+    virtual ~WindowModelInterface() = default;
+
+Q_SIGNALS:
+    void windowAdded(const NumberedWindow);
+    void windowRemoved(const unsigned int index);
+    void windowChanged(const DirtiedWindow);
+
+private:
+    Q_DISABLE_COPY(WindowModelInterface)
+};
+
+} // namespace qtmir
+
+Q_DECLARE_METATYPE(qtmir::NumberedWindow)
+Q_DECLARE_METATYPE(qtmir::DirtiedWindow)
+
+#endif // WINDOWMODELINTERFACE_H

=== modified file 'miral-qt/src/platforms/mirserver/CMakeLists.txt'
--- miral-qt/src/platforms/mirserver/CMakeLists.txt	2016-06-21 13:38:53 +0000
+++ miral-qt/src/platforms/mirserver/CMakeLists.txt	2016-07-27 16:33:34 +0000
@@ -81,10 +81,12 @@
     clipboard.cpp
     creationhints.cpp
     windowmanagementpolicy.cpp
+    windowmodel.cpp
     tracepoints.c
 # We need to run moc on these headers
     ${APPLICATION_API_INCLUDEDIR}/unity/shell/application/Mir.h
     ${APPLICATION_API_INCLUDEDIR}/unity/shell/application/MirMousePointerInterface.h
+    ${MIRAL_QT_SOURCE_DIR}/src/common/windowmodelinterface.h
     )
 
 add_library(qpa-mirserver SHARED

=== modified file 'miral-qt/src/platforms/mirserver/windowmanagementpolicy.cpp'
--- miral-qt/src/platforms/mirserver/windowmanagementpolicy.cpp	2016-07-11 16:14:37 +0000
+++ miral-qt/src/platforms/mirserver/windowmanagementpolicy.cpp	2016-07-27 16:33:34 +0000
@@ -20,12 +20,13 @@
 
 #include "miral/window_specification.h"
 
-WindowManagementPolicy::WindowManagementPolicy(const miral::WindowManagerTools *tools,
+WindowManagementPolicy::WindowManagementPolicy(miral::WindowManagerTools * const tools,
                                                const QSharedPointer<ScreensModel> screensModel)
-    : m_tools(tools)
+    : CanonicalWindowManagerPolicy(tools)
+    , m_tools(tools)
     , m_eventFeeder(new QtEventFeeder(screensModel))
 {
-    Q_UNUSED(m_tools); // REMOVEME once m_tools is used (keep clang happy)
+
 }
 
 void WindowManagementPolicy::advise_displays_updated(const Rectangles&/*displays*/)
@@ -103,6 +104,7 @@
 
 void WindowManagementPolicy::advise_move_to(const miral::WindowInfo &/*window_info*/, Point /*top_left*/)
 {
+
 }
 
 void WindowManagementPolicy::advise_resize(const miral::WindowInfo &/*info*/, const Size &/*newSize*/)

=== modified file 'miral-qt/src/platforms/mirserver/windowmanagementpolicy.h'
--- miral-qt/src/platforms/mirserver/windowmanagementpolicy.h	2016-07-11 16:14:37 +0000
+++ miral-qt/src/platforms/mirserver/windowmanagementpolicy.h	2016-07-27 16:33:34 +0000
@@ -17,7 +17,7 @@
 #ifndef WINDOWMANAGEMENTPOLICY_H
 #define WINDOWMANAGEMENTPOLICY_H
 
-#include "miral/window_management_policy.h"
+#include "miral/canonical_window_manager.h"
 
 #include "qteventfeeder.h"
 
@@ -29,10 +29,10 @@
 
 class ScreensModel;
 
-class WindowManagementPolicy : public QObject, public miral::WindowManagementPolicy
+class WindowManagementPolicy : public QObject, public miral::CanonicalWindowManagerPolicy
 {
 public:
-    WindowManagementPolicy(const miral::WindowManagerTools *tools,
+    WindowManagementPolicy(miral::WindowManagerTools * const tools,
                            const QSharedPointer<ScreensModel> screensModel);
 
 
@@ -68,18 +68,9 @@
     void advise_displays_updated(const Rectangles &displays) override;
 
 Q_SIGNALS:
-//    void sessionCreatedSurface(mir::scene::Session const*,
-//                               std::shared_ptr<mir::scene::Surface> const&,
-//                               std::shared_ptr<SurfaceObserver> const&,
-//                               qtmir::CreationHints);
-//    void sessionDestroyingSurface(mir::scene::Session const*, std::shared_ptr<mir::scene::Surface> const&);
-
-//    // requires Qt::BlockingQueuedConnection!!
-//    void sessionAboutToCreateSurface(const miral::ApplicationInfo &app_info,
-//                                     const miral::WindowSpecification &request_parameters);
 
 private:
-    const miral::WindowManagerTools *m_tools;
+    const miral::WindowManagerTools * const m_tools;
     const QScopedPointer<QtEventFeeder> m_eventFeeder;
 };
 

=== added file 'miral-qt/src/platforms/mirserver/windowmodel.cpp'
--- miral-qt/src/platforms/mirserver/windowmodel.cpp	1970-01-01 00:00:00 +0000
+++ miral-qt/src/platforms/mirserver/windowmodel.cpp	2016-07-27 16:33:34 +0000
@@ -0,0 +1,125 @@
+/*
+ * Copyright (C) 2016 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/>.
+ */
+
+#include "windowmodel.h"
+
+#include "mirqtconversion.h"
+#include <QDebug>
+
+/*
+ * WindowModel - tracks Mir Window Manager operations and duplicates the window stack
+ * that Mir has created internally. Any changes to this model are emitted as change
+ * signals to the Qt GUI thread which will effectively duplicate this model again.
+ *
+ * Use a window ID as a shared identifier between this Mir-side model and the Qt-side model
+ */
+
+using namespace qtmir;
+
+WindowModel::WindowModel()
+{
+    qDebug("WindowModel::WindowModel");
+    qRegisterMetaType<NumberedWindow>();
+    qRegisterMetaType<DirtiedWindow>();
+}
+
+WindowModel::~WindowModel()
+{
+
+}
+
+void WindowModel::addWindow(const miral::WindowInfo &windowInfo)
+{
+    qDebug("WindowModel::addWindow");
+    auto stackPosition = static_cast<unsigned int>(m_windowIdStack.count());
+    m_windowIdStack.push_back(windowInfo.window().surface_id()); // ASSUMPTION: Mir should tell us where in stack
+
+    QSize size = toQSize(windowInfo.window().size());
+    QPoint position = toQPoint(windowInfo.window().top_left());
+
+    WindowInfo info{ size, position, false, windowInfo.window() };
+    NumberedWindow window{ stackPosition, info };
+    Q_EMIT windowAdded(window);
+}
+
+void WindowModel::removeWindow(const miral::WindowInfo &windowInfo)
+{
+    qDebug("WindowModel::removeWindow");
+    const int pos = m_windowIdStack.indexOf(windowInfo.window().surface_id());
+    if (pos < 0) {
+        qDebug("Unknown window removed");
+        return;
+    }
+    m_windowIdStack.removeAt(pos);
+    auto upos = static_cast<unsigned int>(pos);
+    Q_EMIT windowRemoved(upos);
+}
+
+void WindowModel::focusWindow(const miral::WindowInfo &windowInfo, const bool focus)
+{
+    const int pos = m_windowIdStack.indexOf(windowInfo.window().surface_id());
+    if (pos < 0) {
+        qDebug("Unknown window focused");
+        return;
+    }
+    auto upos = static_cast<unsigned int>(pos);
+    m_focusedWindowIndex = upos;
+    QSize size = toQSize(windowInfo.window().size());
+    QPoint position = toQPoint(windowInfo.window().top_left());
+
+    WindowInfo info{ size, position, focus, windowInfo.window() };
+    DirtiedWindow window{ upos, info, WindowInfo::DirtyStates::Focus};
+    Q_EMIT windowChanged(window);
+}
+
+void WindowModel::moveWindow(const miral::WindowInfo &windowInfo, mir::geometry::Point topLeft)
+{
+    const int pos = m_windowIdStack.indexOf(windowInfo.window().surface_id());
+    if (pos < 0) {
+        qDebug("Unknown window moved");
+        return;
+    }
+    auto upos = static_cast<unsigned int>(pos);
+    const bool focused = (m_focusedWindowIndex == upos);
+    QSize size = toQSize(windowInfo.window().size());
+    QPoint position = toQPoint(topLeft);
+
+    WindowInfo info{ size, position, focused, windowInfo.window() };
+    DirtiedWindow window{ upos, info, WindowInfo::DirtyStates::Position};
+    Q_EMIT windowChanged(window);
+}
+
+void WindowModel::resizeWindow(const miral::WindowInfo &windowInfo, mir::geometry::Size newSize)
+{
+    const int pos = m_windowIdStack.indexOf(windowInfo.window().surface_id());
+    if (pos < 0) {
+        qDebug("Unknown window resized");
+        return;
+    }
+    auto upos = static_cast<unsigned int>(pos);
+    const bool focused = (m_focusedWindowIndex == upos);
+    QSize size = toQSize(newSize);
+    QPoint position = toQPoint(windowInfo.window().top_left());
+
+    WindowInfo info{ size, position, focused, windowInfo.window() };
+    DirtiedWindow window{ upos, info, WindowInfo::DirtyStates::Size};
+    Q_EMIT windowChanged(window);
+}
+
+void WindowModel::raiseWindows(const std::vector<miral::Window> &/*windows*/)
+{
+
+}

=== added file 'miral-qt/src/platforms/mirserver/windowmodel.h'
--- miral-qt/src/platforms/mirserver/windowmodel.h	1970-01-01 00:00:00 +0000
+++ miral-qt/src/platforms/mirserver/windowmodel.h	2016-07-27 16:33:34 +0000
@@ -0,0 +1,52 @@
+/*
+ * Copyright (C) 2016 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 WINDOWMODEL_H
+#define WINDOWMODEL_H
+
+#include "windowmodelinterface.h"
+
+#include "miral/window_info.h"
+#include <mir/frontend/surface_id.h>
+
+#include <QPair>
+
+namespace qtmir {
+
+class WindowModel : public WindowModelInterface
+{
+    Q_OBJECT
+public:
+    WindowModel();
+    virtual ~WindowModel();
+
+    void addWindow(const miral::WindowInfo &windowInfo);
+    void removeWindow(const miral::WindowInfo &windowInfo);
+
+    void moveWindow(const miral::WindowInfo &windowInfo, mir::geometry::Point topLeft);
+    void resizeWindow(const miral::WindowInfo &windowInfo, mir::geometry::Size newSize);
+
+    void focusWindow(const miral::WindowInfo &windowInfo, const bool focus);
+    void raiseWindows(const std::vector<miral::Window> &windows);  //window?? Not WindowInfo??
+
+private:
+    QVector<mir::frontend::SurfaceId> m_windowIdStack;
+    unsigned int m_focusedWindowIndex;
+};
+
+} // namespace qtmir
+
+#endif // WINDOWMODEL_H