Merge lp:~aacid/qtmir/create_observer_sooner into lp:qtmir

[Albert Astals Cid]

Move the creation of the surface observer to SessionListener::surface_created

Otherwise we may lose some frame_posted calls if we delay it (it was run in a different thread), meaning that if the app doesn't paint anything else qtmir will think that nothing was yet drawn showing the splash screen forever.

* Are there any related MPs required for this MP to build/function as expected? No

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

 * If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?
N/A

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

[Gerry Boland (gerboland)]

You approach should work nicely, but I'd prefer SurfaceObserver to emit framesPosted signals and MirSurfaceItem to connect to them, instead of using QMetaObject::invokeMethod (to get compile time checks, have Qt decide if queued connection needed or not, and is slightly faster).

To be absolutely correct, we shouldn't be ignoring the count parameter passed to frame_posted, but that can be a later fix.

review: Needs Fixing

[Gerry Boland (gerboland)]

+ SurfaceObserver *so = dynamic_cast<SurfaceObserver *>(observer.get());
makes me think you'd be better just having MirSurfaceManager::onSessionCreatedSurface explicitly pass the QObject derived SurfaceObserver, instead of just referring to its interface and having to cast. i.e.

 void MirSurfaceManager::onSessionCreatedSurface(const mir::scene::Session *mirSession,
- const std::shared_ptr<mir::scene::Surface> &surface)
+ const std::shared_ptr<mir::scene::Surface> &surface,
+ const std::shared_ptr<SurfaceObserver> &observer)

Am also doubting if it needs to be a shared_ptr at all. Since SurfaceObserver is a QObject, and we use connect() to listen for its signals, if it is deleted everything should still be ok. I think raw pointer would do just fine. So

 void MirSurfaceManager::onSessionCreatedSurface(const mir::scene::Session *mirSession,
- const std::shared_ptr<mir::scene::Surface> &surface)
+ const std::shared_ptr<mir::scene::Surface> &surface,
+ const SurfaceObserver *observer)

review: Needs Fixing

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

[Gerry Boland (gerboland)]

Code wise, looks great. Need to test on all our devices

review: Approve (code)

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

[Gerry Boland (gerboland)]

Ok tested on N7, kryllin and my laptop. LGTM

 * Did you perform an exploratory manual test run of the code change and any related functionality?
Y
 * Did CI run pass? If not, please explain why.
Y

review: Approve

[Michał Sawicz (saviq)]

Text conflict in src/modules/Unity/Application/mirsurfaceitem.h
1 conflicts encountered.

review: Needs Fixing

[lp:~aacid/qtmir/create_observer_sooner]

300. By Albert Astals Cid on 2015-01-14

Merge

[Albert Astals Cid (aacid)]

Merged

[Michał Sawicz (saviq)]

review: Abstain

=== modified file 'src/modules/Unity/Application/mirsurfaceitem.cpp'
--- src/modules/Unity/Application/mirsurfaceitem.cpp	2014-11-05 14:57:19 +0000
+++ src/modules/Unity/Application/mirsurfaceitem.cpp	2015-01-14 08:24:48 +0000
@@ -26,6 +26,9 @@
 #include "logging.h"
 #include "ubuntukeyboardinfo.h"
 
+// mirserver
+#include "surfaceobserver.h"
+
 // common
 #include <debughelpers.h>
 
@@ -231,21 +234,9 @@
     }
 };
 
-MirSurfaceObserver::MirSurfaceObserver()
-    : m_listener(nullptr) {
-}
-
-void MirSurfaceObserver::setListener(QObject *listener) {
-    m_listener = listener;
-}
-
-void MirSurfaceObserver::frame_posted(int frames_available) {
-    Q_UNUSED(frames_available);
-    QMetaObject::invokeMethod(m_listener, "surfaceDamaged");
-}
-
 MirSurfaceItem::MirSurfaceItem(std::shared_ptr<mir::scene::Surface> surface,
                                SessionInterface* session,
+                               std::shared_ptr<SurfaceObserver> observer,
                                QQuickItem *parent)
     : QQuickItem(parent)
     , m_surface(surface)
@@ -258,9 +249,11 @@
 {
     qCDebug(QTMIR_SURFACES) << "MirSurfaceItem::MirSurfaceItem";
 
-    m_surfaceObserver = std::make_shared<MirSurfaceObserver>();
-    m_surfaceObserver->setListener(this);
-    m_surface->add_observer(m_surfaceObserver);
+    m_surfaceObserver = observer;
+    if (observer) {
+        connect(observer.get(), &SurfaceObserver::framesPosted, this, &MirSurfaceItem::surfaceDamaged);
+        observer->setListener(this);
+    }
 
     setSmooth(true);
     setFlag(QQuickItem::ItemHasContents, true); //so scene graph will render this item

=== modified file 'src/modules/Unity/Application/mirsurfaceitem.h'
--- src/modules/Unity/Application/mirsurfaceitem.h	2015-01-06 17:28:52 +0000
+++ src/modules/Unity/Application/mirsurfaceitem.h	2015-01-14 08:24:48 +0000
@@ -29,11 +29,12 @@
 
 // mir
 #include <mir/scene/surface.h>
-#include <mir/scene/surface_observer.h>
 #include <mir_toolkit/common.h>
 
 #include "session_interface.h"
 
+class SurfaceObserver;
+
 namespace qtmir {
 
 class MirSurfaceManager;
@@ -41,30 +42,6 @@
 class QMirSurfaceTextureProvider;
 class Application;
 
-class MirSurfaceObserver : public mir::scene::SurfaceObserver {
-public:
-    MirSurfaceObserver();
-
-    void setListener(QObject *listener);
-
-    void attrib_changed(MirSurfaceAttrib, int) override {}
-    void resized_to(mir::geometry::Size const&) override {}
-    void moved_to(mir::geometry::Point const&) override {}
-    void hidden_set_to(bool) override {}
-
-    // Get new frame notifications from Mir, called from a Mir thread.
-    void frame_posted(int frames_available) override;
-
-    void alpha_set_to(float) override {}
-    void transformation_set_to(glm::mat4 const&) override {}
-    void reception_mode_set_to(mir::input::InputReceptionMode) override {}
-    void cursor_image_set_to(mir::graphics::CursorImage const&) override {}
-    void orientation_set_to(MirOrientation) override {}
-    void client_surface_close_requested() override {}
-private:
-    QObject *m_listener;
-};
-
 class MirSurfaceItem : public QQuickItem
 {
     Q_OBJECT
@@ -80,6 +57,7 @@
 public:
     explicit MirSurfaceItem(std::shared_ptr<mir::scene::Surface> surface,
                             SessionInterface* session,
+                            std::shared_ptr<SurfaceObserver> observer,
                             QQuickItem *parent = 0);
     ~MirSurfaceItem();
 
@@ -199,7 +177,7 @@
 
     QMirSurfaceTextureProvider *m_textureProvider;
 
-    std::shared_ptr<MirSurfaceObserver> m_surfaceObserver;
+    std::shared_ptr<SurfaceObserver> m_surfaceObserver;
 
     QTimer m_frameDropperTimer;
 

=== modified file 'src/modules/Unity/Application/mirsurfacemanager.cpp'
--- src/modules/Unity/Application/mirsurfacemanager.cpp	2014-12-01 11:05:01 +0000
+++ src/modules/Unity/Application/mirsurfacemanager.cpp	2015-01-14 08:24:48 +0000
@@ -100,13 +100,14 @@
 }
 
 void MirSurfaceManager::onSessionCreatedSurface(const mir::scene::Session *mirSession,
-                                                const std::shared_ptr<mir::scene::Surface> &surface)
+                                                const std::shared_ptr<mir::scene::Surface> &surface,
+                                                const std::shared_ptr<SurfaceObserver> &observer)
 {
     qCDebug(QTMIR_SURFACES) << "MirSurfaceManager::onSessionCreatedSurface - mirSession=" << mirSession
                             << "surface=" << surface.get() << "surface.name=" << surface->name().c_str();
 
     SessionInterface* session = m_sessionManager->findSession(mirSession);
-    auto qmlSurface = new MirSurfaceItem(surface, session);
+    auto qmlSurface = new MirSurfaceItem(surface, session, observer);
     {
         QMutexLocker lock(&m_mutex);
         m_mirSurfaceToItemHash.insert(surface.get(), qmlSurface);

=== modified file 'src/modules/Unity/Application/mirsurfacemanager.h'
--- src/modules/Unity/Application/mirsurfacemanager.h	2014-12-01 11:05:01 +0000
+++ src/modules/Unity/Application/mirsurfacemanager.h	2015-01-14 08:24:48 +0000
@@ -68,7 +68,7 @@
 //    void fullscreenSurfaceChanged();
 
 public Q_SLOTS:
-    void onSessionCreatedSurface(const mir::scene::Session *, const std::shared_ptr<mir::scene::Surface> &);
+    void onSessionCreatedSurface(const mir::scene::Session *, const std::shared_ptr<mir::scene::Surface> &, std::shared_ptr<SurfaceObserver> const&);
     void onSessionDestroyingSurface(const mir::scene::Session *, const std::shared_ptr<mir::scene::Surface> &);
 
     void onSurfaceAttributeChanged(const mir::scene::Surface *, MirSurfaceAttrib, int);

=== modified file 'src/platforms/mirserver/CMakeLists.txt'
--- src/platforms/mirserver/CMakeLists.txt	2014-12-11 15:27:02 +0000
+++ src/platforms/mirserver/CMakeLists.txt	2015-01-14 08:24:48 +0000
@@ -45,6 +45,7 @@
     sessionauthorizer.cpp
     sessionlistener.cpp
     surfaceconfigurator.cpp
+    surfaceobserver.cpp
     promptsessionlistener.cpp
     mirplacementstrategy.cpp
     mirserver.cpp

=== modified file 'src/platforms/mirserver/sessionlistener.cpp'
--- src/platforms/mirserver/sessionlistener.cpp	2014-09-26 17:38:05 +0000
+++ src/platforms/mirserver/sessionlistener.cpp	2015-01-14 08:24:48 +0000
@@ -15,9 +15,12 @@
  */
 
 #include "sessionlistener.h"
+#include "surfaceobserver.h"
 #include "logging.h"
 #include "tracepoints.h" // generated from tracepoints.tp
 
+#include <mir/scene/surface.h>
+
 namespace ms = mir::scene;
 
 Q_DECLARE_METATYPE(std::shared_ptr<ms::Session>)
@@ -30,6 +33,7 @@
     // need to register type to send over threads with signal/slot
     qRegisterMetaType<std::shared_ptr<ms::Session>>("std::shared_ptr<mir::scene::Session>");
     qRegisterMetaType<std::shared_ptr<ms::Surface>>("std::shared_ptr<mir::scene::Surface>");
+    qRegisterMetaType<std::shared_ptr<SurfaceObserver>>("std::shared_ptr<SurfaceObserver>");
 }
 
 SessionListener::~SessionListener()
@@ -68,7 +72,9 @@
     tracepoint(qtmirserver, surfaceCreated);
     qCDebug(QTMIR_MIR_MESSAGES) << "SessionListener::surface_created - this=" << this << "session=" << &session
                                    << "surface=" << surface.get();
-    Q_EMIT sessionCreatedSurface(&session, surface);
+    std::shared_ptr<SurfaceObserver> surfaceObserver = std::make_shared<SurfaceObserver>();
+    surface->add_observer(surfaceObserver);
+    Q_EMIT sessionCreatedSurface(&session, surface, surfaceObserver);
 }
 
 void SessionListener::destroying_surface(ms::Session& session, std::shared_ptr<ms::Surface> const& surface)

=== modified file 'src/platforms/mirserver/sessionlistener.h'
--- src/platforms/mirserver/sessionlistener.h	2014-04-17 22:25:39 +0000
+++ src/platforms/mirserver/sessionlistener.h	2015-01-14 08:24:48 +0000
@@ -22,6 +22,8 @@
 #include "mir/scene/session_listener.h"
 #include "mir/scene/session.h"
 
+class SurfaceObserver;
+
 class SessionListener : public QObject, public mir::scene::SessionListener
 {
     Q_OBJECT
@@ -43,7 +45,7 @@
     void sessionFocused(std::shared_ptr<mir::scene::Session> const& session);
     void sessionUnfocused();
 
-    void sessionCreatedSurface(mir::scene::Session const*, std::shared_ptr<mir::scene::Surface> const&);
+    void sessionCreatedSurface(mir::scene::Session const*, std::shared_ptr<mir::scene::Surface> const&, std::shared_ptr<SurfaceObserver> const&);
     void sessionDestroyingSurface(mir::scene::Session const*, std::shared_ptr<mir::scene::Surface> const&);
 };
 

=== added file 'src/platforms/mirserver/surfaceobserver.cpp'
--- src/platforms/mirserver/surfaceobserver.cpp	1970-01-01 00:00:00 +0000
+++ src/platforms/mirserver/surfaceobserver.cpp	2015-01-14 08:24:48 +0000
@@ -0,0 +1,39 @@
+/*
+ * Copyright (C) 2014 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 "surfaceobserver.h"
+
+#include <QMetaObject>
+
+SurfaceObserver::SurfaceObserver()
+    : m_listener(nullptr)
+    , m_framesPosted(false)
+{
+}
+
+void SurfaceObserver::setListener(QObject *listener) {
+    m_listener = listener;
+    if (m_framesPosted) {
+        Q_EMIT framesPosted();
+    }
+}
+
+void SurfaceObserver::frame_posted(int /*frames_available*/) {
+    m_framesPosted = true;
+    if (m_listener) {
+        Q_EMIT framesPosted();
+    }
+}

=== added file 'src/platforms/mirserver/surfaceobserver.h'
--- src/platforms/mirserver/surfaceobserver.h	1970-01-01 00:00:00 +0000
+++ src/platforms/mirserver/surfaceobserver.h	2015-01-14 08:24:48 +0000
@@ -0,0 +1,54 @@
+/*
+ * Copyright (C) 2014 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 SESSIONOBSERVER_H
+#define SESSIONOBSERVER_H
+
+#include <QObject>
+#include <mir/scene/surface_observer.h>
+
+class SurfaceObserver : public QObject, public mir::scene::SurfaceObserver {
+    Q_OBJECT
+
+public:
+    SurfaceObserver();
+
+    void setListener(QObject *listener);
+
+    void attrib_changed(MirSurfaceAttrib, int) override {}
+    void resized_to(mir::geometry::Size const&) override {}
+    void moved_to(mir::geometry::Point const&) override {}
+    void hidden_set_to(bool) override {}
+
+    // Get new frame notifications from Mir, called from a Mir thread.
+    void frame_posted(int frames_available) override;
+
+    void alpha_set_to(float) override {}
+    void transformation_set_to(glm::mat4 const&) override {}
+    void reception_mode_set_to(mir::input::InputReceptionMode) override {}
+    void cursor_image_set_to(mir::graphics::CursorImage const&) override {}
+    void orientation_set_to(MirOrientation) override {}
+    void client_surface_close_requested() override {}
+
+Q_SIGNALS:
+    void framesPosted();
+
+private:
+    QObject *m_listener;
+    bool m_framesPosted;
+};
+
+#endif

=== modified file 'tests/modules/MirSurfaceItem/mirsurfaceitem_test.cpp'
--- tests/modules/MirSurfaceItem/mirsurfaceitem_test.cpp	2014-09-11 20:21:47 +0000
+++ tests/modules/MirSurfaceItem/mirsurfaceitem_test.cpp	2015-01-14 08:24:48 +0000
@@ -86,7 +86,7 @@
         }));
 
 
-    MirSurfaceItem *surfaceItem = new MirSurfaceItem(mockSurface, mockSession);
+    MirSurfaceItem *surfaceItem = new MirSurfaceItem(mockSurface, mockSession, nullptr);
 
     ulong timestamp = 1234;
     QList<QTouchEvent::TouchPoint> touchPoints;