Merge lp:~nick-dedekind/qtmir/lp1352977 into lp:qtmir

[Nick Dedekind]

Added application prompt surfaces to allow prompting application which have not yet created a surface

Added application prompt surfaces to allow prompting application which have not yet created a surface

 * Are there any related MPs required for this MP to build/function as expected? Please list.
https://code.launchpad.net/~nick-dedekind/unity8/lp1352977/+merge/229945

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

[lp:~nick-dedekind/qtmir/lp1352977]

226. By Nick Dedekind on 2014-08-07

rebased

[Gerry Boland (gerboland)]

No tests?

[Gerry Boland (gerboland)]

I can see that this approach works, and is reasonably elegant on the QML side for our phone/tablet purposes. But I will ask you to keep in mind the future desktop situation where, unlike phone/tablet, surfaces will have child surfaces, so an application will build up a tree of surfaces. When that situation happens, this separate list approach won't gel at all - I think we'll need to add prompt surfaces just as more nodes in that tree (with a special "I am a prompt session surface" property attached) and work from there.

In fact, I think if we did not have this separation between app surface and trust surfaces, then the problem you're addressing here would not exist.

But for now, this approach will do.

+ for(MirSurfaceItem* promptSurface : promptSurfaces) {
nit-pick, space after "for"

+ if (surface == m_surface || m_promptSurfaces.contains(surface)) return;
please move return to the next line

+ qDebug()
use the categorised logging please.

+void Application::removeSurface(MirSurfaceItem* surface)
add log output here please, is significant enough IMO
Combining management of app surface and trust helper surfaces bit odd here. Think it would be clearer to have 2 methods.

+++ src/modules/Unity/Application/application_manager.cpp
+ m_roleNames.insert(RoleApplication, "application");
why expose the entire Application object? I don't think it's really necessary.

- case RoleFullscreen:
- return QVariant::fromValue(application->fullscreen());
Eh? We need this.

void MirSurfaceItem::setApplication(Application *app)
{
- m_application = app;
+ m_application = app;
Bad whitespace change.

+ qDebug()
qCDebug please

-void MirSurfaceManager::refreshPromptSessionSurfaces(const Application* application)
+void MirSurfaceManager::refreshPromptSessionSurfaces(Application* application)
No more const pointer? I don't see where we are changing it.

+ qCDebug(QTMIR_SURFACES) << "MirSurfaceManager::rehostPromptSessionSurfaces - remove: " << item->name();
rehost?

Checklist for the MR please. Please use the unity-mir one.

review: Needs Fixing

[lp:~nick-dedekind/qtmir/lp1352977]

227. By Nick Dedekind on 2014-08-08

fixed changelog

228. By Nick Dedekind on 2014-08-08

fixed up some logging

[Nick Dedekind (nick-dedekind)]

> I can see that this approach works, and is reasonably elegant on the QML side
> for our phone/tablet purposes. But I will ask you to keep in mind the future
> desktop situation where, unlike phone/tablet, surfaces will have child
> surfaces, so an application will build up a tree of surfaces. When that
> situation happens, this separate list approach won't gel at all - I think
> we'll need to add prompt surfaces just as more nodes in that tree (with a
> special "I am a prompt session surface" property attached) and work from
> there.
>
> In fact, I think if we did not have this separation between app surface and
> trust surfaces, then the problem you're addressing here would not exist.

Right, so this is a bit of a hotfix which we can live with for awhile. But it's definately what I wanted to end up with.
Firstly, I think that the trust surfaces should just be siblings of each other, not children with the first being a top level surface. So I've already started to look at putting them linearly (unfortunately it makes messing around with the previous surfaces position in the animations a bit more difficult, but that's unity8 problem.
I'm not sure about having the app surface in the same sector as the prompt surfaces, since it will make the management a bit shifty.

>
> But for now, this approach will do.
>
> + for(MirSurfaceItem* promptSurface : promptSurfaces) {
> nit-pick, space after "for"
>
> + if (surface == m_surface || m_promptSurfaces.contains(surface)) return;
> please move return to the next line
>
> + qDebug()
> use the categorised logging please.
>
> +void Application::removeSurface(MirSurfaceItem* surface)
> add log output here please, is significant enough IMO
> Combining management of app surface and trust helper surfaces bit odd here.
> Think it would be clearer to have 2 methods.

Done.

>
>
> +++ src/modules/Unity/Application/application_manager.cpp
> + m_roleNames.insert(RoleApplication, "application");
> why expose the entire Application object? I don't think it's really necessary.

QML can't work out using out a QVariant(QmlListProperty) if we use the app->promptSurfaces as the role. I'm looking into switching the list into a fully fledged MirSurfaceItemModel (list model which can be used for prompt surfaces, surface children and the surface manager) which will hopefully remove the need to expose the application as a role.

>
> - case RoleFullscreen:
> - return QVariant::fromValue(application->fullscreen());
> Eh? We need this.

doh. fixed.

>
> void MirSurfaceItem::setApplication(Application *app)
> {
> - m_application = app;
> + m_application = app;
> Bad whitespace change.
>
> + qDebug()
> qCDebug please

Done & Done

>
> -void MirSurfaceManager::refreshPromptSessionSurfaces(const Application*
> application)
> +void MirSurfaceManager::refreshPromptSessionSurfaces(Application*
> application)
> No more const pointer? I don't see where we are changing it.

application->addPromptSurface(surface);

>
> + qCDebug(QTMIR_SURFACES) << "MirSurfaceManager::rehostPromptSessionSurfaces -
> remove: " << item->name();
> rehost?

Done.

>
> Checklist for the MR please. Please use the unity-mir one.

[lp:~nick-dedekind/qtmir/lp1352977]

229. By Nick Dedekind on 2014-08-08

review comments

[Nick Dedekind (nick-dedekind)]

> +void Application::removeSurface(MirSurfaceItem* surface)
> add log output here please, is significant enough IMO
> Combining management of app surface and trust helper surfaces bit odd here.
> Think it would be clearer to have 2 methods.
>

Hm. both calling setSurface & addPromptSruface set the application. would have to do this:

if (m_application) {
 m_application->setSurface(nullptr);
}
if (m_application) {
 m_application->removePromptSurface(this);
}

Which i just find a bit weird. I though having removeSurface handling removal or all types of surfaces was lesser of evils.

[lp:~nick-dedekind/qtmir/lp1352977]

230. By Nick Dedekind on 2014-08-08

more review comments

[Gerry Boland (gerboland)]

Code looks ok, but not had chance to test

review: Approve (code)

[Gerry Boland (gerboland)]

Oh except Checklist please!!

[Daniel d'Andrada (dandrader)]

109 + Q_PROPERTY(QQmlListProperty<qtmir::MirSurfaceItem> promptSurfaces READ promptSurfaces NOTIFY promptSurfacesChanged DESIGNABLE false)

You should update unity-api accordingly.

review: Needs Fixing

[Nick Dedekind (nick-dedekind)]

> 109 + Q_PROPERTY(QQmlListProperty<qtmir::MirSurfaceItem> promptSurfaces
> READ promptSurfaces NOTIFY promptSurfacesChanged DESIGNABLE false)
>
> You should update unity-api accordingly.

This can be added later. It's not a requirement for this MP and shouldn't be considered a blocker.

[Daniel d'Andrada (dandrader)]

> > 109 + Q_PROPERTY(QQmlListProperty<qtmir::MirSurfaceItem> promptSurfaces
> > READ promptSurfaces NOTIFY promptSurfacesChanged DESIGNABLE false)
> >
> > You should update unity-api accordingly.
>
> This can be added later. It's not a requirement for this MP and shouldn't be
> considered a blocker.

That's the right time to add it. Otherwise "later" can very well mean "never".

=== modified file 'debian/changelog'
--- debian/changelog	2014-08-05 19:23:28 +0000
+++ debian/changelog	2014-08-08 10:47:06 +0000
@@ -1,3 +1,10 @@
+qtmir (0.4.1-0ubuntu1) UNRELEASED; urgency=medium
+
+  [ Nick Dedekind ]
+  * Added prompt surfaces to application
+
+ -- Nicholas Dedekind <[e-mail]>  Thu, 07 Aug 2014 14:56:41 +0100
+
 qtmir (0.4.0+14.10.20140805.1-0ubuntu1) utopic; urgency=low
 
   [ Nick Dedekind ]

=== modified file 'src/modules/Unity/Application/application.cpp'
--- src/modules/Unity/Application/application.cpp	2014-07-25 15:29:20 +0000
+++ src/modules/Unity/Application/application.cpp	2014-08-08 10:47:06 +0000
@@ -75,6 +75,11 @@
 
 Application::~Application()
 {
+    QList<MirSurfaceItem*> promptSurfaces(m_promptSurfaces);
+    for (MirSurfaceItem* promptSurface : promptSurfaces) {
+        delete promptSurface;
+    }
+
     qCDebug(QTMIR_APPLICATIONS) << "Application::~Application";
     {
         // In case we get a threaded screenshot callback once the application is deleted.
@@ -82,8 +87,8 @@
         m_screenShotGuard.clear();
     }
     stopPromptSessions();
+    delete m_surface;
     delete m_desktopData;
-    delete m_surface;
 }
 
 bool Application::isValid() const
@@ -480,4 +485,59 @@
     return false;
 }
 
+void Application::addPromptSurface(MirSurfaceItem* surface)
+{
+    qCDebug(QTMIR_APPLICATIONS) << "Application::addPromptSurface " << surface->name() << " to " << name();
+    if (surface == m_surface || m_promptSurfaces.contains(surface))
+        return;
+
+    surface->setApplication(this);
+    m_promptSurfaces.append(surface);
+    Q_EMIT promptSurfacesChanged();
+}
+
+void Application::removeSurface(MirSurfaceItem* surface)
+{
+    qCDebug(QTMIR_APPLICATIONS) << "Application::removeSurface " << surface->name() << " from " << name();
+
+    if (m_surface == surface) {
+        setSurface(nullptr);
+    } else if (m_promptSurfaces.contains(surface)) {
+        m_promptSurfaces.removeAll(surface);
+        surface->setApplication(nullptr);
+
+        Q_EMIT promptSurfacesChanged();
+    }
+}
+
+void Application::foreachPromptSurface(std::function<void(MirSurfaceItem*)> f) const
+{
+    for (MirSurfaceItem* promptSurface : m_promptSurfaces) {
+        f(promptSurface);
+    }
+}
+
+QQmlListProperty<MirSurfaceItem> Application::promptSurfaces()
+{
+    return QQmlListProperty<MirSurfaceItem>(this,
+                                            0,
+                                            Application::promptSurfaceCount,
+                                            Application::promptSurfaceAt);
+}
+
+int Application::promptSurfaceCount(QQmlListProperty<MirSurfaceItem> *prop)
+{
+    Application *p = qobject_cast<Application*>(prop->object);
+    return p->m_promptSurfaces.count();
+}
+
+MirSurfaceItem* Application::promptSurfaceAt(QQmlListProperty<MirSurfaceItem> *prop, int index)
+{
+    Application *p = qobject_cast<Application*>(prop->object);
+
+    if (index < 0 || index >= p->m_promptSurfaces.count())
+        return nullptr;
+    return p->m_promptSurfaces[index];
+}
+
 } // namespace qtmir

=== modified file 'src/modules/Unity/Application/application.h'
--- src/modules/Unity/Application/application.h	2014-07-18 16:29:01 +0000
+++ src/modules/Unity/Application/application.h	2014-08-08 10:47:06 +0000
@@ -58,6 +58,7 @@
     Q_PROPERTY(Stage stage READ stage WRITE setStage NOTIFY stageChanged)
     Q_PROPERTY(SupportedOrientations supportedOrientations READ supportedOrientations CONSTANT)
     Q_PROPERTY(MirSurfaceItem* surface READ surface NOTIFY surfaceChanged)
+    Q_PROPERTY(QQmlListProperty<qtmir::MirSurfaceItem> promptSurfaces READ promptSurfaces NOTIFY promptSurfacesChanged DESIGNABLE false)
 
 public:
     Q_DECLARE_FLAGS(Stages, Stage)
@@ -112,6 +113,10 @@
 
     bool containsProcess(pid_t pid) const;
 
+    void foreachPromptSurface(std::function<void(MirSurfaceItem*)> f) const;
+    void addPromptSurface(MirSurfaceItem* surface);
+    void removeSurface(MirSurfaceItem* surface);
+
 public Q_SLOTS:
     void suspend();
     void resume();
@@ -121,6 +126,7 @@
     void fullscreenChanged();
     void stageChanged(Stage stage);
     void surfaceChanged();
+    void promptSurfacesChanged();
 
     void surfaceDestroyed(MirSurfaceItem *surface);
 
@@ -144,6 +150,10 @@
 
     void updateFullscreenProperty();
 
+    QQmlListProperty<MirSurfaceItem> promptSurfaces();
+    static int promptSurfaceCount(QQmlListProperty<MirSurfaceItem> *prop);
+    static MirSurfaceItem* promptSurfaceAt(QQmlListProperty<MirSurfaceItem> *prop, int index);
+
     ApplicationManager* m_appMgr;
     QSharedPointer<TaskController> m_taskController;
     DesktopFileReader* m_desktopData;
@@ -162,6 +172,7 @@
     QTimer* m_suspendTimer;
     SupportedOrientations m_supportedOrientations;
     MirSurfaceItem *m_surface;
+    QList<MirSurfaceItem*> m_promptSurfaces;
     QList<std::shared_ptr<mir::scene::PromptSession>> m_promptSessions;
     std::shared_ptr<mir::scene::PromptSessionManager> const m_promptSessionManager;
 

=== modified file 'src/modules/Unity/Application/application_manager.cpp'
--- src/modules/Unity/Application/application_manager.cpp	2014-08-05 10:53:04 +0000
+++ src/modules/Unity/Application/application_manager.cpp	2014-08-08 10:47:06 +0000
@@ -213,6 +213,7 @@
 
     m_roleNames.insert(RoleSurface, "surface");
     m_roleNames.insert(RoleFullscreen, "fullscreen");
+    m_roleNames.insert(RoleApplication, "application");
 }
 
 ApplicationManager::~ApplicationManager()
@@ -250,6 +251,8 @@
                 return QVariant::fromValue(application->surface());
             case RoleFullscreen:
                 return QVariant::fromValue(application->fullscreen());
+            case RoleApplication:
+                return QVariant::fromValue(application);
             default:
                 return QVariant();
         }

=== modified file 'src/modules/Unity/Application/application_manager.h'
--- src/modules/Unity/Application/application_manager.h	2014-08-05 10:53:04 +0000
+++ src/modules/Unity/Application/application_manager.h	2014-08-08 10:47:06 +0000
@@ -68,6 +68,7 @@
     enum MoreRoles {
         RoleSurface = RoleScreenshot+1,
         RoleFullscreen,
+        RoleApplication,
     };
 
     // Mapping enums to Ubuntu Platform API enums.

=== modified file 'src/modules/Unity/Application/mirsurfaceitem.cpp'
--- src/modules/Unity/Application/mirsurfaceitem.cpp	2014-07-29 15:01:34 +0000
+++ src/modules/Unity/Application/mirsurfaceitem.cpp	2014-08-08 10:47:06 +0000
@@ -312,6 +312,9 @@
     if (m_parentSurface) {
         m_parentSurface->removeChildSurface(this);
     }
+    if (m_application) {
+        m_application->removeSurface(this);
+    }
 
     qCDebug(QTMIR_SURFACES) << "MirSurfaceItem::~MirSurfaceItem - this=" << this;
     QMutexLocker locker(&m_mutex);
@@ -334,7 +337,7 @@
     }
 
     if (m_application) {
-        m_application->setSurface(nullptr);
+        m_application->removeSurface(this);
     }
     if (!parent()) {
         deleteLater();
@@ -706,7 +709,7 @@
 
 void MirSurfaceItem::addChildSurface(MirSurfaceItem* surface)
 {
-    qDebug() << "MirSurfaceItem::addChildSurface " << surface->name() << " to " << name();
+    qCDebug(QTMIR_SURFACES) << "MirSurfaceItem::addChildSurface " << surface->name() << " to " << name();
 
     m_children.append(surface);
     Q_EMIT childSurfacesChanged();
@@ -714,6 +717,8 @@
 
 void MirSurfaceItem::removeChildSurface(MirSurfaceItem* surface)
 {
+    qCDebug(QTMIR_SURFACES) << "MirSurfaceItem::removeChildSurface " << surface->name() << " from " << name();
+
     if (m_children.contains(surface)) {
         m_children.removeOne(surface);
 

=== modified file 'src/modules/Unity/Application/mirsurfacemanager.cpp'
--- src/modules/Unity/Application/mirsurfacemanager.cpp	2014-08-01 11:46:30 +0000
+++ src/modules/Unity/Application/mirsurfacemanager.cpp	2014-08-08 10:47:06 +0000
@@ -196,8 +196,11 @@
 }
 
 void MirSurfaceManager::onPromptProviderAdded(const mir::scene::PromptSession *promptSession,
-                                              const std::shared_ptr<mir::scene::Session> &)
+                                              const std::shared_ptr<mir::scene::Session> &session)
 {
+    qCDebug(QTMIR_SURFACES) << "MirSurfaceManager::onPromptProviderAdded - promptSession=" << promptSession
+                            << "session=" << session.get();
+
     ApplicationManager* appMgr = static_cast<ApplicationManager*>(ApplicationManager::singleton());
     Application* application = appMgr->findApplicationWithPromptSession(promptSession);
 
@@ -205,8 +208,11 @@
 }
 
 void MirSurfaceManager::onPromptProviderRemoved(const mir::scene::PromptSession *promptSession,
-                                                const std::shared_ptr<mir::scene::Session> &)
+                                                const std::shared_ptr<mir::scene::Session> &session)
 {
+    qCDebug(QTMIR_SURFACES) << "MirSurfaceManager::onPromptProviderRemoved - promptSession=" << promptSession
+                            << "session=" << session.get();
+
     ApplicationManager* appMgr = static_cast<ApplicationManager*>(ApplicationManager::singleton());
     Application* application = appMgr->findApplicationWithPromptSession(promptSession);
 
@@ -252,12 +258,12 @@
     return m_surfaceItems[index];
 }
 
-void getSurfaceDecendents(MirSurfaceItem* item, QList<MirSurfaceItem*>& surfaceChildren)
+void appendSurfaceDecendents(MirSurfaceItem* item, QList<MirSurfaceItem*>& surfaceChildren)
 {
     // recursive function. fetch all decendent surface items as a list.
     item->foreachChildSurface([&surfaceChildren](MirSurfaceItem* child) {
         surfaceChildren.append(child);
-        getSurfaceDecendents(child, surfaceChildren);
+        appendSurfaceDecendents(child, surfaceChildren);
     });
 }
 
@@ -271,29 +277,32 @@
     refreshPromptSessionSurfaces(application);
 }
 
-void MirSurfaceManager::refreshPromptSessionSurfaces(const Application* application)
+void MirSurfaceManager::refreshPromptSessionSurfaces(Application* application)
 {
     // Re-contruct the child surface heirachy from prompt session providers.
-    // Each provider becomes the parent surface of the next surface,
-    // with the application surface at the top of the heirachy.
+    // First provider becomes prompt surface of Application, subsequent providers
+    // becoming the children of the previous provider.
 
-    if (!application)
+    if (!application) {
+        qCDebug(QTMIR_SURFACES) << "MirSurfaceManager::refreshPromptSessionSurfaces - No Application for prompt session";
         return;
+    }
     qCDebug(QTMIR_SURFACES) << "MirSurfaceManager::refreshPromptSessionSurfaces - appId=" << application->name();
 
     std::shared_ptr<ms::PromptSessionManager> manager = m_mirConfig->the_prompt_session_manager();
 
-    MirSurfaceItem* parentItem = application->surface();
-    if (!parentItem) {
-        return;
-    }
-
-    QList<MirSurfaceItem*> surfaceChildren;
-    getSurfaceDecendents(parentItem, surfaceChildren);
+    QList<MirSurfaceItem*> surfaces;
+    application->foreachPromptSurface([&surfaces](MirSurfaceItem* promptSurface) {
+        surfaces.append(promptSurface);
+        appendSurfaceDecendents(promptSurface, surfaces);
+    });
+    // stop adding providers to the child stack if we come across one which has not yet added a surface.
     bool continueProviders = true;
 
     auto refreshFn = [&](const std::shared_ptr<mir::scene::PromptSession>& promptSession) {
 
+        MirSurfaceItem* parentSurface = nullptr;
+
         manager->for_each_provider_in(promptSession,
             [&](const std::shared_ptr<ms::Session> &session) {
                 QMutexLocker lock(&m_mutex);
@@ -302,31 +311,39 @@
                     return;
 
                 auto it = m_mirSessionToItemHash.find(session.get());
-                MirSurfaceItem *nextParent = nullptr;
+                MirSurfaceItem *nextParentSurface = nullptr;
 
                 while (it != m_mirSessionToItemHash.end() && it.key() == session.get()) { // all surfaces for session
                     MirSurfaceItem *surface = it.value();
-                    surfaceChildren.removeOne(surface);
+                    surfaces.removeOne(surface);
 
                     if (!surface->isFirstFrameDrawn()) { // only add if the surface is ready.
                         continueProviders = false;
                         break;
                     }
 
-                    qCDebug(QTMIR_SURFACES) << "MirSurfaceManager::refreshPromptSessionSurfaces - " << surface->name() << "->setParent(" << parentItem->name() << ")";
-                    surface->setParentSurface(parentItem);
-                    if (!nextParent)
-                        nextParent = surface;
+                    // FIXME - all providers should be "prompt surfaces" of application
+
+                    // first privder is a application "prompt surface"
+                    if (parentSurface == nullptr) {
+                        surface->setParentSurface(nullptr);
+                        application->addPromptSurface(surface);
+                    // susquent provider is child of previous provider
+                    } else {
+                        surface->setParentSurface(parentSurface);
+                    }
+                    if (!nextParentSurface)
+                        nextParentSurface = surface;
                     ++it;
                 }
                 // first valid session surface is parent of next provider session surface.
-                if (nextParent) {
-                    parentItem = nextParent;
+                if (nextParentSurface) {
+                    parentSurface = nextParentSurface;
                 }
             });
 
-        for (MirSurfaceItem* item : surfaceChildren) {
-            qCDebug(QTMIR_SURFACES) << "MirSurfaceManager::rehostPromptSessionSurfaces - remove: " << item->name() << " from " << item->parentSurface()->name();
+        for (MirSurfaceItem* item : surfaces) {
+            qCDebug(QTMIR_SURFACES) << "MirSurfaceManager::refreshPromptSessionSurfaces - remove: " << item->name();
             Q_EMIT item->removed();
         }
     };

=== modified file 'src/modules/Unity/Application/mirsurfacemanager.h'
--- src/modules/Unity/Application/mirsurfacemanager.h	2014-07-18 09:44:54 +0000
+++ src/modules/Unity/Application/mirsurfacemanager.h	2014-08-08 10:47:06 +0000
@@ -93,7 +93,7 @@
     );
 
     void refreshPromptSessionSurfaces(const mir::scene::Session *session);
-    void refreshPromptSessionSurfaces(const Application* application);
+    void refreshPromptSessionSurfaces(Application* application);
 
     void removePromptSessionSurface(MirSurfaceItem* surface);