Merge lp:~dandrader/qtmir/fixSurfaceFocus-lp1491034 into lp:qtmir

[Daniel d'Andrada]

Update surface focus when a surface enters or leaves a MirSurfaceItem

+ Refactor MirSurfaceItem so that it talks to a MirSurfaceInterface
  instead of the concrete MirSurface class
+ Make MirSurface::setFocus() more robust

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

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?

Not applicable

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

[Gerry Boland (gerboland)]

You know I dislike refactoring in a bug fix commit. I'll accept this time, but please resist the temptation in future.

Code change makes sense, and I can't reproduce the bug with it.

+ qCDebug(QTMIR_SURFACES).nospace() << "MirSurface[" << appId() << "]::setFocus(" << (focus ? "true" : "false") << ")";
Do you really need to interpret a bool into true/false for qDebug?

review: Approve

[lp:~dandrader/qtmir/fixSurfaceFocus-lp1491034]

373. By Daniel d'Andrada on 2015-09-15

qDebug is smarter than printf

[Daniel d'Andrada (dandrader)]

On 15/09/15 08:16, Gerry Boland wrote:
> + qCDebug(QTMIR_SURFACES).nospace() << "MirSurface[" << appId() << "]::setFocus(" << (focus ? "true" : "false") << ")";
> Do you really need to interpret a bool into true/false for qDebug?
>

Right, qDebug() is smarter than printf(). Fixed.

=== modified file 'src/modules/Unity/Application/mirsurface.cpp'
--- src/modules/Unity/Application/mirsurface.cpp	2015-08-26 11:38:50 +0000
+++ src/modules/Unity/Application/mirsurface.cpp	2015-09-15 19:10:17 +0000
@@ -222,11 +222,6 @@
     }
 }
 
-SessionInterface* MirSurface::session() const
-{
-    return m_session.data();
-}
-
 Mir::Type MirSurface::type() const
 {
     switch (m_surface->type()) {
@@ -358,12 +353,20 @@
 
 void MirSurface::setFocus(bool focus)
 {
+    if (!m_session) {
+        return;
+    }
+
     // Temporary hotfix for http://pad.lv/1483752
-    if (session() && session()->childSessions()->rowCount() > 0) {
+    if (m_session->childSessions()->rowCount() > 0) {
         // has child trusted session, ignore any focus change attempts
+        qCDebug(QTMIR_SURFACES).nospace() << "MirSurface[" << appId() << "]::setFocus(" << focus
+            << ") - has child trusted session, ignore any focus change attempts";
         return;
     }
 
+    qCDebug(QTMIR_SURFACES).nospace() << "MirSurface[" << appId() << "]::setFocus(" << focus << ")";
+
     if (focus) {
         m_shell->set_surface_attribute(m_session->session(), m_surface, mir_surface_attrib_focus, mir_surface_focused);
     } else {
@@ -635,3 +638,15 @@
 {
     Q_EMIT sizeChanged(m_size);
 }
+
+QString MirSurface::appId() const
+{
+    QString appId;
+
+    if (m_session && m_session->application()) {
+        appId = m_session->application()->appId();
+    } else {
+        appId.append("-");
+    }
+    return appId;
+}

=== modified file 'src/modules/Unity/Application/mirsurface.h'
--- src/modules/Unity/Application/mirsurface.h	2015-08-24 12:43:01 +0000
+++ src/modules/Unity/Application/mirsurface.h	2015-09-15 19:10:17 +0000
@@ -77,8 +77,6 @@
 
     bool isFirstFrameDrawn() const override { return m_firstFrameDrawn; }
 
-    SessionInterface *session() const override;
-
     void stopFrameDropper() override;
     void startFrameDropper() override;
 
@@ -110,6 +108,8 @@
             Qt::TouchPointStates qtTouchPointStates,
             ulong qtTimestamp) override;
 
+    QString appId() const override;
+
 public Q_SLOTS:
     void onCompositorSwappedBuffers() override;
 

=== modified file 'src/modules/Unity/Application/mirsurfaceinterface.h'
--- src/modules/Unity/Application/mirsurfaceinterface.h	2015-08-24 12:43:01 +0000
+++ src/modules/Unity/Application/mirsurfaceinterface.h	2015-09-15 19:10:17 +0000
@@ -43,8 +43,6 @@
 
     virtual bool isFirstFrameDrawn() const = 0;
 
-    virtual SessionInterface *session() const = 0;
-
     virtual void stopFrameDropper() = 0;
     virtual void startFrameDropper() = 0;
 
@@ -76,6 +74,8 @@
             Qt::TouchPointStates qtTouchPointStates,
             ulong qtTimestamp) = 0;
 
+    virtual QString appId() const = 0;
+
 public Q_SLOTS:
     virtual void onCompositorSwappedBuffers() = 0;
 

=== modified file 'src/modules/Unity/Application/mirsurfaceitem.cpp'
--- src/modules/Unity/Application/mirsurfaceitem.cpp	2015-08-24 12:43:01 +0000
+++ src/modules/Unity/Application/mirsurfaceitem.cpp	2015-09-15 19:10:17 +0000
@@ -33,6 +33,7 @@
 #include <QScreen>
 #include <private/qsgdefaultimagenode_p.h>
 #include <QTimer>
+#include <QSGTextureProvider>
 
 #include <QRunnable>
 
@@ -333,16 +334,11 @@
 
 QString MirSurfaceItem::appId() const
 {
-    QString appId;
-
-    SessionInterface *session = m_surface ? m_surface->session() : nullptr;
-
-    if (session && session->application()) {
-        appId = session->application()->appId();
+    if (m_surface) {
+        return m_surface->appId();
     } else {
-        appId.append("-");
+        return QString("-");
     }
-    return appId;
 }
 
 void MirSurfaceItem::endCurrentTouchSequence(ulong timestamp)
@@ -571,7 +567,7 @@
 {
     QMutexLocker mutexLocker(&m_mutex);
 
-    qtmir::MirSurface *surface = static_cast<qtmir::MirSurface*>(unitySurface);
+    auto surface = static_cast<qtmir::MirSurfaceInterface*>(unitySurface);
     qCDebug(QTMIR_SURFACES).nospace() << "MirSurfaceItem::setSurface surface=" << surface;
 
     if (surface == m_surface) {
@@ -580,7 +576,13 @@
 
     if (m_surface) {
         disconnect(m_surface, nullptr, this, nullptr);
+
+        if (hasActiveFocus() && m_consumesInput && m_surface->live()) {
+            m_surface->setFocus(false);
+        }
+
         m_surface->decrementViewCount();
+
         if (!m_surface->isBeingDisplayed() && window()) {
             disconnect(window(), nullptr, m_surface, nullptr);
         }
@@ -596,13 +598,13 @@
 
         // When a new mir frame gets posted we notify the QML engine that this item needs redrawing,
         // schedules call to updatePaintNode() from the rendering thread
-        connect(m_surface, &MirSurface::framesPosted, this, &QQuickItem::update);
-
-        connect(m_surface, &MirSurface::stateChanged, this, &MirSurfaceItem::surfaceStateChanged);
-        connect(m_surface, &MirSurface::liveChanged, this, &MirSurfaceItem::liveChanged);
-        connect(m_surface, &MirSurface::sizeChanged, this, &MirSurfaceItem::onActualSurfaceSizeChanged);
-
-        connect(window(), &QQuickWindow::frameSwapped, m_surface, &MirSurface::onCompositorSwappedBuffers,
+        connect(m_surface, &MirSurfaceInterface::framesPosted, this, &QQuickItem::update);
+
+        connect(m_surface, &MirSurfaceInterface::stateChanged, this, &MirSurfaceItem::surfaceStateChanged);
+        connect(m_surface, &MirSurfaceInterface::liveChanged, this, &MirSurfaceItem::liveChanged);
+        connect(m_surface, &MirSurfaceInterface::sizeChanged, this, &MirSurfaceItem::onActualSurfaceSizeChanged);
+
+        connect(window(), &QQuickWindow::frameSwapped, m_surface, &MirSurfaceInterface::onCompositorSwappedBuffers,
             (Qt::ConnectionType) (Qt::DirectConnection | Qt::UniqueConnection));
 
         Q_EMIT typeChanged(m_surface->type());
@@ -614,13 +616,17 @@
 
         if (m_orientationAngle) {
             m_surface->setOrientationAngle(*m_orientationAngle);
-            connect(m_surface, &MirSurface::orientationAngleChanged, this, &MirSurfaceItem::orientationAngleChanged);
+            connect(m_surface, &MirSurfaceInterface::orientationAngleChanged, this, &MirSurfaceItem::orientationAngleChanged);
             delete m_orientationAngle;
             m_orientationAngle = nullptr;
         } else {
-            connect(m_surface, &MirSurface::orientationAngleChanged, this, &MirSurfaceItem::orientationAngleChanged);
+            connect(m_surface, &MirSurfaceInterface::orientationAngleChanged, this, &MirSurfaceItem::orientationAngleChanged);
             Q_EMIT orientationAngleChanged(m_surface->orientationAngle());
         }
+
+        if (m_consumesInput) {
+            m_surface->setFocus(hasActiveFocus());
+        }
     }
 
     Q_EMIT surfaceChanged(m_surface);

=== modified file 'src/modules/Unity/Application/mirsurfaceitem.h'
--- src/modules/Unity/Application/mirsurfaceitem.h	2015-08-24 12:43:01 +0000
+++ src/modules/Unity/Application/mirsurfaceitem.h	2015-09-15 19:10:17 +0000
@@ -26,7 +26,7 @@
 // Unity API
 #include <unity/shell/application/MirSurfaceItemInterface.h>
 
-#include "mirsurface.h"
+#include "mirsurfaceinterface.h"
 #include "session_interface.h"
 
 namespace qtmir {
@@ -129,7 +129,7 @@
             const QList<QTouchEvent::TouchPoint> &touchPoints,
             Qt::TouchPointStates touchPointStates);
 
-    MirSurface* m_surface;
+    MirSurfaceInterface* m_surface;
 
     QMutex m_mutex;
     MirTextureProvider *m_textureProvider;

=== modified file 'tests/modules/common/fake_mirsurface.h'
--- tests/modules/common/fake_mirsurface.h	2015-08-24 12:43:01 +0000
+++ tests/modules/common/fake_mirsurface.h	2015-09-15 19:10:17 +0000
@@ -102,8 +102,6 @@
         return m_isFirstFrameDrawn;
     }
 
-    SessionInterface *session() const override { return m_session; }
-
     void stopFrameDropper() override {
         m_isFrameDropperRunning = false;
     }
@@ -158,6 +156,8 @@
         m_touchesReceived.append(TouchEvent(mods, points, states, timestamp));
     }
 
+    QString appId() const override { return "foo-app"; }
+
 public Q_SLOTS:
     void onCompositorSwappedBuffers() override {}