Merge lp:~gerboland/qtmir/fix-prompt-session-crash into lp:qtmir

Proposed by Gerry Boland
Status: Merged
Approved by: Daniel d'Andrada
Approved revision: 621
Merged at revision: 621
Proposed branch: lp:~gerboland/qtmir/fix-prompt-session-crash
Merge into: lp:qtmir
Prerequisite: lp:~alan-griffiths/qtmir/tidy-mirserver-deps
Diff against target: 969 lines (+675/-84)
13 files modified
src/modules/Unity/Application/application_manager.h (+4/-2)
src/modules/Unity/Application/mirsurface.cpp (+0/-14)
src/modules/Unity/Application/mirsurface.h (+0/-1)
src/modules/Unity/Application/sessionmap_interface.h (+38/-0)
src/modules/Unity/Application/surfacemanager.cpp (+29/-4)
src/modules/Unity/Application/surfacemanager.h (+9/-2)
tests/framework/CMakeLists.txt (+3/-0)
tests/framework/mock_sessionmap.h (+32/-0)
tests/framework/mock_window_controller.h (+43/-0)
tests/modules/CMakeLists.txt (+2/-1)
tests/modules/SurfaceManager/CMakeLists.txt (+40/-0)
tests/modules/SurfaceManager/surface_manager_test.cpp (+475/-0)
tests/modules/WindowManager/mirsurface_test.cpp (+0/-60)
To merge this branch: bzr merge lp:~gerboland/qtmir/fix-prompt-session-crash
Reviewer Review Type Date Requested Status
Unity8 CI Bot (community) continuous-integration Approve
Daniel d'Andrada (community) Approve
Albert Astals Cid (community) Abstain
Review via email: mp+318759@code.launchpad.net

Commit message

Stop MirSurface deleting itself, ensure SurfaceManager alone manages MirSurface lifetimes. Add SurfaceManager test suite.

This fixes bugs where a MirSurface would call deleteLater on itself, but SurfaceManager would have no idea and keep a pointer to that MirSurface in its internal list.

Instead SurfaceManager listens for signals from both miral and the MirSurface itself to decide when to delete it.

Add a test suite to verify MirSurface lifetimes.

To post a comment you must log in.
Revision history for this message
Unity8 CI Bot (unity8-ci-bot) wrote :

PASSED: Continuous integration, rev:618
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/545/
Executed test runs:
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build/4283
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/4311
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4145
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4145/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4145
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4145/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4145
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4145/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4145
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4145/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4145
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4145/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4145
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4145/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/545/rebuild

review: Approve (continuous-integration)
Revision history for this message
Unity8 CI Bot (unity8-ci-bot) wrote :

PASSED: Continuous integration, rev:619
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/546/
Executed test runs:
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build/4287
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/4315
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4148
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4148/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4148
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4148/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4148
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4148/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4148
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4148/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4148
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4148/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4148
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4148/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/546/rebuild

review: Approve (continuous-integration)
Revision history for this message
Daniel d'Andrada (dandrader) wrote :

$ bzr branch lp:qtmir foo && cd foo
$ bzr merge lp:~alan-griffiths/qtmir/tidy-mirserver-deps && bzr commit -m"tidy-mirserver-deps"
$ bzr merge lp:~gerboland/qtmir/fix-prompt-session-crash

Text conflict in src/modules/Unity/Application/surfacemanager.cpp

Probably because your prerequisite branch has changed in the meantime

review: Needs Fixing
Revision history for this message
Daniel d'Andrada (dandrader) wrote :

In src/modules/Unity/Application/mirsurface.cpp:

"""
@@ -710,14 +706,11 @@
 {
     m_views.remove(viewId);
     DEBUG_MSG << "(" << viewId << ")" << " after=" << m_views.count() << " live=" << m_live;
+ updateExposure();
+ setViewActiveFocus(viewId, false);
     if (m_views.count() == 0) {
         Q_EMIT isBeingDisplayedChanged();
- if (m_session.isNull() || !m_live) {
- deleteLater();
- }
     }
- updateExposure();
- setViewActiveFocus(viewId, false);
 }

 void MirSurface::setViewExposure(qintptr viewId, bool exposed)
"""

Why did you move updateExposure() and setViewActiveFocus() up?

Revision history for this message
Daniel d'Andrada (dandrader) wrote :

You should rebase on top of https://code.launchpad.net/~dandrader/qtmir/byeSessionManager as you're changing SessionManager here

Revision history for this message
Daniel d'Andrada (dandrader) wrote :

But I like the core idea of this MP.

Revision history for this message
Gerry Boland (gerboland) wrote :

> In src/modules/Unity/Application/mirsurface.cpp:
>
> """
> @@ -710,14 +706,11 @@
> {
> m_views.remove(viewId);
> DEBUG_MSG << "(" << viewId << ")" << " after=" << m_views.count() << "
> live=" << m_live;
> + updateExposure();
> + setViewActiveFocus(viewId, false);
> if (m_views.count() == 0) {
> Q_EMIT isBeingDisplayedChanged();
> - if (m_session.isNull() || !m_live) {
> - deleteLater();
> - }
> }
> - updateExposure();
> - setViewActiveFocus(viewId, false);
> }
>
> void MirSurface::setViewExposure(qintptr viewId, bool exposed)
> """
>
> Why did you move updateExposure() and setViewActiveFocus() up?

Exactly something I was a little worried about. The slot connected to isBeingDisplayedChanged() can cause SurfaceManager to delete the MirSurface. So it might delete it in the middle of the method. I moved the signal to the end to avoid a crash, but yes, that is fragile.

I suspect a deleteLater is safer. But would you have any better idea?

Revision history for this message
Daniel d'Andrada (dandrader) wrote :

On 03/03/2017 08:40, Gerry Boland wrote:
>> In src/modules/Unity/Application/mirsurface.cpp:
>>
>> """
>> @@ -710,14 +706,11 @@
>> {
>> m_views.remove(viewId);
>> DEBUG_MSG << "(" << viewId << ")" << " after=" << m_views.count() << "
>> live=" << m_live;
>> + updateExposure();
>> + setViewActiveFocus(viewId, false);
>> if (m_views.count() == 0) {
>> Q_EMIT isBeingDisplayedChanged();
>> - if (m_session.isNull() || !m_live) {
>> - deleteLater();
>> - }
>> }
>> - updateExposure();
>> - setViewActiveFocus(viewId, false);
>> }
>>
>> void MirSurface::setViewExposure(qintptr viewId, bool exposed)
>> """
>>
>> Why did you move updateExposure() and setViewActiveFocus() up?
> Exactly something I was a little worried about. The slot connected to isBeingDisplayedChanged() can cause SurfaceManager to delete the MirSurface. So it might delete it in the middle of the method. I moved the signal to the end to avoid a crash, but yes, that is fragile.
>
> I suspect a deleteLater is safer. But would you have any better idea?

Ah, ok.

Revision history for this message
Unity8 CI Bot (unity8-ci-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Unity8 CI Bot (unity8-ci-bot) wrote :

PASSED: Continuous integration, rev:623
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/556/
Executed test runs:
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build/4317
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/4345
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4179
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4179/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4179
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4179/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4179
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4179/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4179
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4179/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4179
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4179/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4179
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4179/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/556/rebuild

review: Approve (continuous-integration)
Revision history for this message
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.
Yes

review: Approve
Revision history for this message
Albert Astals Cid (aacid) wrote :

Text conflict in src/modules/Unity/Application/mirbuffersgtexture.cpp
Contents conflict in src/modules/Unity/Application/sessionmanager.cpp
Contents conflict in src/modules/Unity/Application/sessionmanager.h
Text conflict in src/modules/Unity/Application/surfacemanager.cpp
Text conflict in tests/modules/CMakeLists.txt
5 conflicts encountered.

Was already top approved.

review: Needs Fixing
616. By Alan Griffiths

merge :parent

Revision history for this message
Unity8 CI Bot (unity8-ci-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Unity8 CI Bot (unity8-ci-bot) wrote :

FAILED: Continuous integration, rev:625
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/570/
Executed test runs:
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build/4421/console
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/4449
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4282/console
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4282
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4282/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4282
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4282/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4282
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4282/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4282
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4282/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4282
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4282/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/570/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
Unity8 CI Bot (unity8-ci-bot) wrote :

PASSED: Continuous integration, rev:625
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/573/
Executed test runs:
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build/4429
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/4457
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4290
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4290/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4290
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4290/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4290
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4290/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4290
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4290/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4290
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4290/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4290
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4290/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/573/rebuild

review: Approve (continuous-integration)
Revision history for this message
Daniel d'Andrada (dandrader) wrote :

There's still a tests/framework/mock_session_manager.h

review: Needs Fixing
Revision history for this message
Albert Astals Cid (aacid) :
review: Abstain
617. By Alan Griffiths

Move MirBuffer to miral namespace

618. By Alan Griffiths

Better naming

619. By Alan Griffiths

A move towards MirAL style

Revision history for this message
Gerry Boland (gerboland) wrote :

MockSessionManager removed, good catch

Revision history for this message
Unity8 CI Bot (unity8-ci-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Daniel d'Andrada (dandrader) wrote :

In the test code:

"""
struct TestableSurfaceManager : public SurfaceManager
{
    // just exports the test-only protected constructor
    TestableSurfaceManager(WindowControllerInterface *windowController,
                           WindowModelNotifier *windowModel,
                           SessionManager *sessionManager)
        : SurfaceManager(windowController, windowModel, sessionManager) {}

    MirSurface* find(const miral::WindowInfo &needle) const
    {
        return SurfaceManager::find(needle);
    }
};
"""

Why don't you just make this constructor and method public in surface manager?

review: Needs Information
620. By Gerry Boland

Stop MirSurface deleting itself, ensure SurfaceManager alone manages MirSurface lifetimes. Add SurfaceManager test suite.

This fixes bugs where a MirSurface would call deleteLater on itself, but SurfaceManager would have no idea and keep a pointer to that MirSurface in its internal list.

Instead SurfaceManager listens for signals from both miral and the MirSurface itself to decide when to delete it.

Add a test suite to verify MirSurface lifetimes.

621. By Gerry Boland

Actually include SurfaceManager tests. SM relies on AppMan singleton, which I had to abstract out to a SessionMap in order to make it testable.

Revision history for this message
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.
Passed locally

review: Approve
Revision history for this message
Unity8 CI Bot (unity8-ci-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Unity8 CI Bot (unity8-ci-bot) wrote :

PASSED: Continuous integration, rev:621
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/601/
Executed test runs:
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build/4573
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/4601
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4428
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4428/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4428
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4428/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4428
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4428/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4428
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4428/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4428
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4428/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4428
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4428/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/601/rebuild

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/modules/Unity/Application/application_manager.h'
2--- src/modules/Unity/Application/application_manager.h 2017-02-21 18:46:30 +0000
3+++ src/modules/Unity/Application/application_manager.h 2017-03-20 15:56:36 +0000
4@@ -26,6 +26,7 @@
5
6 // local
7 #include "application.h"
8+#include "sessionmap_interface.h"
9 #include "taskcontroller.h"
10
11 // Unity API
12@@ -55,7 +56,8 @@
13 class SharedWakelock;
14 class SettingsInterface;
15
16-class ApplicationManager : public unity::shell::application::ApplicationManagerInterface
17+class ApplicationManager : public unity::shell::application::ApplicationManagerInterface,
18+ public SessionMapInterface
19 {
20 Q_OBJECT
21
22@@ -94,7 +96,7 @@
23 const QList<Application*> &list() const { return m_applications; }
24 qtmir::Application* findApplicationWithPid(const pid_t pid) const;
25
26- SessionInterface *findSession(const mir::scene::Session* session) const;
27+ SessionInterface *findSession(const mir::scene::Session* session) const override;
28
29 public Q_SLOTS:
30 void authorizeSession(const pid_t pid, bool &authorized);
31
32=== modified file 'src/modules/Unity/Application/mirsurface.cpp'
33--- src/modules/Unity/Application/mirsurface.cpp 2017-03-07 23:42:05 +0000
34+++ src/modules/Unity/Application/mirsurface.cpp 2017-03-20 15:56:36 +0000
35@@ -168,7 +168,6 @@
36 connect(m_surfaceObserver.get(), &SurfaceObserver::confinesMousePointerChanged, this, &MirSurface::confinesMousePointerChanged);
37 m_surfaceObserver->setListener(this);
38
39- //connect(session, &QObject::destroyed, this, &MirSurface::onSessionDestroyed); // TODO try using Shared pointer for lifecycle
40 connect(session, &SessionInterface::stateChanged, this, [this]() {
41 if (clientIsRunning() && m_pendingResize.isValid()) {
42 resize(m_pendingResize.width(), m_pendingResize.height());
43@@ -584,9 +583,6 @@
44 INFO_MSG << "(" << value << ")";
45 m_live = value;
46 Q_EMIT liveChanged(value);
47- if (m_views.isEmpty() && !m_live) {
48- deleteLater();
49- }
50 }
51 }
52
53@@ -711,9 +707,6 @@
54 INFO_MSG << "(" << viewId << ")" << " after=" << m_views.count() << " live=" << m_live;
55 if (m_views.count() == 0) {
56 Q_EMIT isBeingDisplayedChanged();
57- if (m_session.isNull() || !m_live) {
58- deleteLater();
59- }
60 }
61 updateExposure();
62 setViewActiveFocus(viewId, false);
63@@ -758,13 +751,6 @@
64 return m_currentFrameNumber;
65 }
66
67-void MirSurface::onSessionDestroyed()
68-{
69- if (m_views.isEmpty()) {
70- deleteLater();
71- }
72-}
73-
74 void MirSurface::emitSizeChanged()
75 {
76 Q_EMIT sizeChanged(m_size);
77
78=== modified file 'src/modules/Unity/Application/mirsurface.h'
79--- src/modules/Unity/Application/mirsurface.h 2017-02-02 09:17:48 +0000
80+++ src/modules/Unity/Application/mirsurface.h 2017-03-20 15:56:36 +0000
81@@ -188,7 +188,6 @@
82 void dropPendingBuffer();
83 void onAttributeChanged(const MirWindowAttrib, const int);
84 void onFramesPostedObserved();
85- void onSessionDestroyed();
86 void emitSizeChanged();
87 void setCursor(const QCursor &cursor);
88 void onCloseTimedOut();
89
90=== added file 'src/modules/Unity/Application/sessionmap_interface.h'
91--- src/modules/Unity/Application/sessionmap_interface.h 1970-01-01 00:00:00 +0000
92+++ src/modules/Unity/Application/sessionmap_interface.h 2017-03-20 15:56:36 +0000
93@@ -0,0 +1,38 @@
94+/*
95+ * Copyright (C) 2017 Canonical, Ltd.
96+ *
97+ * This program is free software: you can redistribute it and/or modify it under
98+ * the terms of the GNU Lesser General Public License version 3, as published by
99+ * the Free Software Foundation.
100+ *
101+ * This program is distributed in the hope that it will be useful, but WITHOUT
102+ * ANY WARRANTY; without even the implied warranties of MERCHANTABILITY,
103+ * SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
104+ * Lesser General Public License for more details.
105+ *
106+ * You should have received a copy of the GNU Lesser General Public License
107+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
108+ */
109+
110+#ifndef SESSIONMAP_INTERFACE_H
111+#define SESSIONMAP_INTERFACE_H
112+
113+namespace mir { namespace scene { class Session; }}
114+
115+namespace qtmir {
116+
117+class SessionInterface;
118+
119+
120+class SessionMapInterface
121+{
122+public:
123+ SessionMapInterface() = default;
124+ virtual ~SessionMapInterface() = default;
125+
126+ virtual SessionInterface *findSession(const mir::scene::Session* session) const = 0;
127+};
128+
129+} // namespace qtmir
130+
131+#endif // SESSIONMAP_INTERFACE_H
132
133=== modified file 'src/modules/Unity/Application/surfacemanager.cpp'
134--- src/modules/Unity/Application/surfacemanager.cpp 2017-03-20 15:56:36 +0000
135+++ src/modules/Unity/Application/surfacemanager.cpp 2017-03-20 15:56:36 +0000
136@@ -38,7 +38,8 @@
137 using namespace qtmir;
138 namespace unityapi = unity::shell::application;
139
140-SurfaceManager::SurfaceManager(QObject *)
141+
142+SurfaceManager::SurfaceManager()
143 {
144 DEBUG_MSG << "()";
145
146@@ -48,12 +49,23 @@
147 qFatal("ERROR: Unity.Application QML plugin requires use of the 'mirserver' QPA plugin");
148 }
149
150+ m_sessionMap = ApplicationManager::singleton();
151 m_windowController = static_cast<WindowControllerInterface*>(nativeInterface->nativeResourceForIntegration("WindowController"));
152
153 auto windowModel = static_cast<WindowModelNotifier*>(nativeInterface->nativeResourceForIntegration("WindowModelNotifier"));
154 connectToWindowModelNotifier(windowModel);
155 }
156
157+SurfaceManager::SurfaceManager(WindowControllerInterface *windowController,
158+ WindowModelNotifier *windowModel,
159+ SessionMapInterface *sessionMap)
160+ : m_windowController(windowController)
161+ , m_sessionMap(sessionMap)
162+{
163+ DEBUG_MSG << "()";
164+ connectToWindowModelNotifier(windowModel);
165+}
166+
167 void SurfaceManager::connectToWindowModelNotifier(WindowModelNotifier *notifier)
168 {
169 connect(notifier, &WindowModelNotifier::windowAdded, this, &SurfaceManager::onWindowAdded, Qt::QueuedConnection);
170@@ -95,12 +107,21 @@
171 }
172
173 auto mirSession = windowInfo.window().application();
174- SessionInterface* session = ApplicationManager::singleton()->findSession(mirSession.get());
175+ SessionInterface* session = m_sessionMap->findSession(mirSession.get());
176
177 const auto parentSurface = find(windowInfo.parent());
178 const auto surface = new MirSurface(window, m_windowController, session, parentSurface);
179 rememberMirSurface(surface);
180
181+ connect(surface, &MirSurface::isBeingDisplayedChanged, this, [this, surface]() {
182+ if ((!surface->live() || !surface->session())
183+ && !surface->isBeingDisplayed()) {
184+ forgetMirSurface(static_cast<MirSurface*>(surface)->window());
185+ surface->deleteLater(); // don't delete immediately, slot may be directly connected
186+ tracepoint(qtmir, surfaceDestroyed);
187+ }
188+ });
189+
190 if (parentSurface) {
191 static_cast<MirSurfaceListModel*>(parentSurface->childSurfaceList())->prependSurface(surface);
192 }
193@@ -117,8 +138,12 @@
194 DEBUG_MSG << "()";
195 MirSurface *surface = find(windowInfo);
196 forgetMirSurface(windowInfo.window());
197- surface->setLive(false);
198- tracepoint(qtmir, surfaceDestroyed);
199+ if (surface->isBeingDisplayed()) {
200+ surface->setLive(false);
201+ } else {
202+ delete surface;
203+ tracepoint(qtmir, surfaceDestroyed);
204+ }
205 }
206
207 MirSurface *SurfaceManager::find(const miral::WindowInfo &needle) const
208
209=== modified file 'src/modules/Unity/Application/surfacemanager.h'
210--- src/modules/Unity/Application/surfacemanager.h 2017-03-20 15:56:36 +0000
211+++ src/modules/Unity/Application/surfacemanager.h 2017-03-20 15:56:36 +0000
212@@ -31,6 +31,7 @@
213 namespace qtmir {
214
215 class MirSurface;
216+class SessionMapInterface;
217 class WindowControllerInterface;
218
219 class SurfaceManager : public unity::shell::application::SurfaceManagerInterface
220@@ -38,12 +39,18 @@
221 Q_OBJECT
222
223 public:
224- explicit SurfaceManager(QObject *parent = 0);
225+ explicit SurfaceManager();
226+ SurfaceManager(WindowControllerInterface *windowController,
227+ WindowModelNotifier *windowModel,
228+ SessionMapInterface *sessionMap);
229 virtual ~SurfaceManager() {}
230
231 void raise(unity::shell::application::MirSurfaceInterface *surface) override;
232 void activate(unity::shell::application::MirSurfaceInterface *surface) override;
233
234+ // mainly for test usage
235+ MirSurface* find(const miral::WindowInfo &needle) const;
236+
237 private Q_SLOTS:
238 void onWindowAdded(const qtmir::NewWindow &windowInfo);
239 void onWindowRemoved(const miral::WindowInfo &windowInfo);
240@@ -58,12 +65,12 @@
241 void connectToWindowModelNotifier(WindowModelNotifier *notifier);
242 void rememberMirSurface(MirSurface *surface);
243 void forgetMirSurface(const miral::Window &window);
244- MirSurface* find(const miral::WindowInfo &needle) const;
245 MirSurface* find(const miral::Window &needle) const;
246
247 QVector<MirSurface*> m_allSurfaces;
248
249 WindowControllerInterface *m_windowController;
250+ SessionMapInterface *m_sessionMap;
251 };
252
253 } // namespace qtmir
254
255=== modified file 'tests/framework/CMakeLists.txt'
256--- tests/framework/CMakeLists.txt 2016-12-02 16:22:45 +0000
257+++ tests/framework/CMakeLists.txt 2017-03-20 15:56:36 +0000
258@@ -28,6 +28,7 @@
259 mock_prompt_session_manager.cpp
260 mock_renderable.cpp
261 mock_session.cpp
262+ mock_sessionmap.h
263 mock_settings.cpp
264 mock_shared_wakelock.cpp
265 mock_surface.cpp
266@@ -46,6 +47,8 @@
267 target_link_libraries(
268 qtmir-test-framework-static
269
270+ unityapplicationplugin
271+
272 ${GTEST_BOTH_LIBRARIES}
273 ${GMOCK_LIBRARIES}
274 )
275
276=== added file 'tests/framework/mock_sessionmap.h'
277--- tests/framework/mock_sessionmap.h 1970-01-01 00:00:00 +0000
278+++ tests/framework/mock_sessionmap.h 2017-03-20 15:56:36 +0000
279@@ -0,0 +1,32 @@
280+/*
281+ * Copyright (C) 2017 Canonical, Ltd.
282+ *
283+ * This program is free software: you can redistribute it and/or modify it under
284+ * the terms of the GNU Lesser General Public License version 3, as published by
285+ * the Free Software Foundation.
286+ *
287+ * This program is distributed in the hope that it will be useful, but WITHOUT
288+ * ANY WARRANTY; without even the implied warranties of MERCHANTABILITY,
289+ * SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
290+ * Lesser General Public License for more details.
291+ *
292+ * You should have received a copy of the GNU Lesser General Public License
293+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
294+ */
295+
296+#ifndef MOCK_SESSIONMAP_H
297+#define MOCK_SESSIONMAP_H
298+
299+#include <gmock/gmock.h>
300+
301+#include <Unity/Application/sessionmap_interface.h>
302+
303+struct MockSessionMap : public qtmir::SessionMapInterface
304+{
305+ MockSessionMap() {}
306+ virtual ~MockSessionMap() {}
307+
308+ MOCK_CONST_METHOD1(findSession, SessionInterface*(const mir::scene::Session*));
309+};
310+
311+#endif // MOCK_SESSIONMAP_H
312
313=== added file 'tests/framework/mock_window_controller.h'
314--- tests/framework/mock_window_controller.h 1970-01-01 00:00:00 +0000
315+++ tests/framework/mock_window_controller.h 2017-03-20 15:56:36 +0000
316@@ -0,0 +1,43 @@
317+/*
318+ * Copyright (C) 2017 Canonical, Ltd.
319+ *
320+ * This program is free software: you can redistribute it and/or modify it under
321+ * the terms of the GNU Lesser General Public License version 3, as published by
322+ * the Free Software Foundation.
323+ *
324+ * This program is distributed in the hope that it will be useful, but WITHOUT
325+ * ANY WARRANTY; without even the implied warranties of MERCHANTABILITY,
326+ * SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
327+ * Lesser General Public License for more details.
328+ *
329+ * You should have received a copy of the GNU Lesser General Public License
330+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
331+ */
332+
333+#ifndef MOCK_WINDOW_CONTROLLER_H
334+#define MOCK_WINDOW_CONTROLLER_H
335+
336+#include "windowcontrollerinterface.h"
337+
338+#include <gmock/gmock.h>
339+
340+class MockWindowController : public qtmir::WindowControllerInterface
341+{
342+public:
343+ MOCK_METHOD1(activate, void(const miral::Window &));
344+ MOCK_METHOD1(raise, void(const miral::Window &));
345+
346+ MOCK_METHOD2(resize, void(const miral::Window &, const QSize &));
347+ MOCK_METHOD2(move, void(const miral::Window &, const QPoint &));
348+
349+ MOCK_METHOD1(requestClose, void(const miral::Window &));
350+ MOCK_METHOD1(forceClose, void(const miral::Window &));
351+
352+ MOCK_METHOD2(requestState, void(const miral::Window &, const Mir::State));
353+
354+ MOCK_METHOD2(deliverKeyboardEvent, void(const miral::Window &, const MirKeyboardEvent *));
355+ MOCK_METHOD2(deliverTouchEvent, void(const miral::Window &, const MirTouchEvent *));
356+ MOCK_METHOD2(deliverPointerEvent, void(const miral::Window &, const MirPointerEvent *));
357+};
358+
359+#endif // MOCK_WINDOW_CONTROLLER_H
360
361=== modified file 'tests/modules/CMakeLists.txt'
362--- tests/modules/CMakeLists.txt 2017-02-21 18:46:30 +0000
363+++ tests/modules/CMakeLists.txt 2017-03-20 15:56:36 +0000
364@@ -1,6 +1,7 @@
365 add_subdirectory(Application)
366 add_subdirectory(ApplicationManager)
367 add_subdirectory(General)
368+add_subdirectory(Session)
369 add_subdirectory(SharedWakelock)
370-add_subdirectory(Session)
371+add_subdirectory(SurfaceManager)
372 add_subdirectory(WindowManager)
373
374=== added directory 'tests/modules/SurfaceManager'
375=== added file 'tests/modules/SurfaceManager/CMakeLists.txt'
376--- tests/modules/SurfaceManager/CMakeLists.txt 1970-01-01 00:00:00 +0000
377+++ tests/modules/SurfaceManager/CMakeLists.txt 2017-03-20 15:56:36 +0000
378@@ -0,0 +1,40 @@
379+set(
380+ SURFACE_MANAGER_TEST_SOURCES
381+ surface_manager_test.cpp
382+ ${CMAKE_SOURCE_DIR}/src/common/debughelpers.cpp
383+)
384+
385+include_directories(
386+ ${CMAKE_SOURCE_DIR}/src/common
387+ ${CMAKE_SOURCE_DIR}/src/platforms/mirserver
388+ ${CMAKE_SOURCE_DIR}/src/modules
389+ ${CMAKE_SOURCE_DIR}/tests/framework
390+)
391+
392+include_directories(
393+ SYSTEM
394+ ${APPLICATION_API_INCLUDE_DIRS}
395+ ${MIRAL_INCLUDE_DIRS}
396+ ${MIRTEST_INCLUDE_DIRS}
397+)
398+
399+add_executable(surfacemanager_test ${SURFACE_MANAGER_TEST_SOURCES})
400+
401+add_dependencies(surfacemanager_test qtmir-test-framework-static)
402+
403+target_link_libraries(
404+ surfacemanager_test
405+
406+ unityapplicationplugin
407+
408+ Qt5::Test
409+
410+ -L${CMAKE_BINARY_DIR}/tests/framework
411+ qtmir-test-framework-static
412+
413+ ${MIRTEST_LDFLAGS}
414+ ${GTEST_BOTH_LIBRARIES}
415+ ${GMOCK_LIBRARIES}
416+)
417+
418+add_test(SurfaceManager, surfacemanager_test)
419
420=== added file 'tests/modules/SurfaceManager/surface_manager_test.cpp'
421--- tests/modules/SurfaceManager/surface_manager_test.cpp 1970-01-01 00:00:00 +0000
422+++ tests/modules/SurfaceManager/surface_manager_test.cpp 2017-03-20 15:56:36 +0000
423@@ -0,0 +1,475 @@
424+/*
425+ * Copyright (C) 2017 Canonical, Ltd.
426+ *
427+ * This program is free software: you can redistribute it and/or modify it under
428+ * the terms of the GNU Lesser General Public License version 3, as published by
429+ * the Free Software Foundation.
430+ *
431+ * This program is distributed in the hope that it will be useful, but WITHOUT
432+ * ANY WARRANTY; without even the implied warranties of MERCHANTABILITY,
433+ * SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
434+ * Lesser General Public License for more details.
435+ *
436+ * You should have received a copy of the GNU Lesser General Public License
437+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
438+ */
439+
440+#include <QLoggingCategory>
441+#include <QSignalSpy>
442+
443+// the test subject
444+#include <Unity/Application/surfacemanager.h>
445+#include <Unity/Application/mirsurface.h>
446+
447+// miral
448+#include <miral/window.h>
449+#include <miral/window_info.h>
450+
451+// mirtest
452+#include <mir/test/doubles/stub_session.h>
453+#include <mir/test/doubles/stub_surface.h>
454+
455+// local
456+#include "qtmir_test.h"
457+#include "fake_session.h"
458+#include "mock_sessionmap.h"
459+#include "mock_window_controller.h"
460+
461+using namespace qtmir;
462+using StubSurface = mir::test::doubles::StubSurface;
463+using StubSession = mir::test::doubles::StubSession;
464+
465+
466+class SurfaceManagerTests : public ::testing::Test
467+{
468+public:
469+ SurfaceManagerTests()
470+ {
471+ // We don't want the logging spam cluttering the test results
472+ QLoggingCategory::setFilterRules(QStringLiteral("qtmir.*=false"));
473+
474+ qRegisterMetaType<unity::shell::application::MirSurfaceInterface*>();
475+ qRegisterMetaType<QVector<unity::shell::application::MirSurfaceInterface*>>();
476+ }
477+
478+ testing::NiceMock<MockWindowController> wmController;
479+ WindowModelNotifier wmNotifier;
480+ MockSessionMap sessionMap;
481+ QScopedPointer<SurfaceManager> surfaceManager;
482+ QScopedPointer<QCoreApplication> qtApp; // need to spin event loop for queued connections
483+
484+ // Needed to create miral::WindowInfo
485+ const std::shared_ptr<StubSession> stubSession{std::make_shared<StubSession>()};
486+ const std::shared_ptr<StubSurface> stubSurface{std::make_shared<StubSurface>()};
487+ const miral::Window window{stubSession, stubSurface};
488+ const ms::SurfaceCreationParameters spec;
489+ const miral::WindowInfo windowInfo{window, spec};
490+ FakeSession fakeSession;
491+
492+protected:
493+ void SetUp() override
494+ {
495+ int argc = 0;
496+ char* argv[0];
497+ qtApp.reset(new QCoreApplication(argc, argv));
498+
499+ using namespace ::testing;
500+ ON_CALL(sessionMap,findSession(_))
501+ .WillByDefault(Return(&fakeSession));
502+
503+ surfaceManager.reset(new SurfaceManager(&wmController, &wmNotifier, &sessionMap));
504+ }
505+};
506+
507+/*
508+ * Test if MirAL notifies that a window was created, SurfaceManager emits the surfaceCreated
509+ * signal
510+ */
511+TEST_F(SurfaceManagerTests, miralWindowCreationCausesSignalEmission)
512+{
513+ QSignalSpy newMirSurfaceSpy(surfaceManager.data(), &SurfaceManager::surfaceCreated);
514+
515+ // Test
516+ Q_EMIT wmNotifier.windowAdded(windowInfo);
517+ qtApp->sendPostedEvents();
518+
519+ // Check result
520+ EXPECT_EQ(1, newMirSurfaceSpy.count());
521+}
522+
523+
524+/*
525+ * Test if MirAL notifies that a window was created, SurfaceManager emits the surfaceCreated
526+ * signal with a corresponding MirSurface
527+ */
528+TEST_F(SurfaceManagerTests, miralWindowCreationCausesMirSurfaceCreation)
529+{
530+ QSignalSpy newMirSurfaceSpy(surfaceManager.data(), &SurfaceManager::surfaceCreated);
531+
532+ // Test
533+ Q_EMIT wmNotifier.windowAdded(windowInfo);
534+ qtApp->sendPostedEvents();
535+
536+ // Check result
537+ auto mirSurface = qvariant_cast<MirSurface*>(newMirSurfaceSpy.takeFirst().at(0));
538+ ASSERT_TRUE(mirSurface);
539+ EXPECT_EQ(window, mirSurface->window());
540+}
541+
542+/*
543+ * Test if MirAL notifies that a window was created, SurfaceManager adds the corresponding
544+ * MirSurface to its internal list
545+ */
546+TEST_F(SurfaceManagerTests, miralWindowCreationAddsMirSurfaceToItsInternalList)
547+{
548+ // Test
549+ Q_EMIT wmNotifier.windowAdded(windowInfo);
550+ qtApp->sendPostedEvents();
551+
552+ // Check result
553+ auto mirSurface = surfaceManager->find(windowInfo);
554+ ASSERT_TRUE(mirSurface);
555+ EXPECT_EQ(window, mirSurface->window());
556+}
557+
558+/*
559+ * Test SurfaceManager creates a MirSurface with a Session associated
560+ */
561+TEST_F(SurfaceManagerTests, createdMirSurfaceHasSessionSet)
562+{
563+ // Setup
564+ using namespace ::testing;
565+ EXPECT_CALL(sessionMap,findSession(_))
566+ .WillOnce(Return(&fakeSession));
567+
568+ // Test
569+ Q_EMIT wmNotifier.windowAdded(windowInfo);
570+ qtApp->sendPostedEvents();
571+
572+ // Check result
573+ auto mirSurface = surfaceManager->find(windowInfo);
574+ ASSERT_TRUE(mirSurface);
575+ EXPECT_EQ(&fakeSession, mirSurface->session());
576+}
577+
578+/*
579+ * Test when MirAL creates a surface with a parent, SurfaceManager correctly associates
580+ * the parent MirSurface to the MirSurface
581+ */
582+TEST_F(SurfaceManagerTests, parentedMiralWindowGeneratesMirSurfaceWithCorrectParent)
583+{
584+ // Setup
585+ miral::Window parentWindow(stubSession, stubSurface);
586+ miral::Window childWindow(stubSession, stubSurface);
587+ miral::WindowInfo parentWindowInfo(parentWindow, spec);
588+ miral::WindowInfo childWindowInfo(childWindow, spec);
589+ childWindowInfo.parent(parentWindow);
590+
591+ // Test
592+ Q_EMIT wmNotifier.windowAdded(parentWindowInfo);
593+ Q_EMIT wmNotifier.windowAdded(childWindowInfo);
594+ qtApp->sendPostedEvents();
595+
596+ // Check result
597+ auto childMirSurface = surfaceManager->find(childWindowInfo);
598+ auto parentMirSurface = surfaceManager->find(parentWindowInfo);
599+ ASSERT_TRUE(childMirSurface);
600+ ASSERT_TRUE(parentMirSurface);
601+
602+ EXPECT_EQ(parentMirSurface, childMirSurface->parentSurface());
603+}
604+
605+/*
606+ * Test when MirAL creates a surface with a parent, SurfaceManager correctly associates
607+ * the child MirSurface to the parent MirSurface
608+ */
609+TEST_F(SurfaceManagerTests, miralWindowWithChildHasMirSurfaceWithCorrectChild)
610+{
611+ // Setup
612+ miral::Window parentWindow(stubSession, stubSurface);
613+ miral::Window childWindow(stubSession, stubSurface);
614+ miral::WindowInfo parentWindowInfo(parentWindow, spec);
615+ miral::WindowInfo childWindowInfo(childWindow, spec);
616+ childWindowInfo.parent(parentWindow);
617+
618+ // Test
619+ Q_EMIT wmNotifier.windowAdded(parentWindowInfo);
620+ Q_EMIT wmNotifier.windowAdded(childWindowInfo);
621+ qtApp->sendPostedEvents();
622+
623+ // Check result
624+ auto childMirSurface = surfaceManager->find(childWindowInfo);
625+ auto parentMirSurface = surfaceManager->find(parentWindowInfo);
626+ ASSERT_TRUE(childMirSurface);
627+ ASSERT_TRUE(parentMirSurface);
628+
629+ ASSERT_EQ(1, parentMirSurface->childSurfaceList()->count());
630+ EXPECT_EQ(childMirSurface, parentMirSurface->childSurfaceList()->first());
631+}
632+
633+/*
634+ * Test if MirAL notifies that a window is ready, SurfaceManager updates the corresponding
635+ * MirSurface causing it to emit a ready() signal
636+ */
637+TEST_F(SurfaceManagerTests, miralWindowReadyUpdatesMirSurfaceState)
638+{
639+ // Setup: add window and get corresponding MirSurface
640+ Q_EMIT wmNotifier.windowAdded(windowInfo);
641+ qtApp->sendPostedEvents();
642+ auto mirSurface = surfaceManager->find(windowInfo);
643+ ASSERT_TRUE(mirSurface);
644+
645+ QSignalSpy mirSurfaceReadySpy(mirSurface, &MirSurface::ready);
646+
647+ // Test
648+ Q_EMIT wmNotifier.windowReady(windowInfo);
649+ qtApp->sendPostedEvents();
650+
651+ // Check result
652+ EXPECT_EQ(1, mirSurfaceReadySpy.count());
653+}
654+
655+/*
656+ * Test if MirAL notifies that a window is moved, SurfaceManager updates the corresponding
657+ * MirSurface position
658+ */
659+TEST_F(SurfaceManagerTests, miralWindowMoveUpdatesMirSurfacePosition)
660+{
661+ QPoint newPosition(222,333);
662+
663+ // Setup: add window and get corresponding MirSurface
664+ Q_EMIT wmNotifier.windowAdded(windowInfo);
665+ qtApp->sendPostedEvents();
666+ auto mirSurface = surfaceManager->find(windowInfo);
667+ ASSERT_TRUE(mirSurface);
668+
669+ QSignalSpy mirSurfacePositionSpy(mirSurface, &MirSurface::positionChanged);
670+
671+ // Test
672+ Q_EMIT wmNotifier.windowMoved(windowInfo, newPosition);
673+ qtApp->sendPostedEvents();
674+
675+ // Check result
676+ EXPECT_EQ(1, mirSurfacePositionSpy.count());
677+ EXPECT_EQ(mirSurface->position(), newPosition);
678+}
679+
680+/*
681+ * Test if MirAL notifies that a window's focus state changes, SurfaceManager updates the corresponding
682+ * MirSurface focus state
683+ */
684+TEST_F(SurfaceManagerTests, miralWindowFocusChangeUpdatesMirSurfaceFocus)
685+{
686+ // Setup: add window and get corresponding MirSurface
687+ Q_EMIT wmNotifier.windowAdded(windowInfo);
688+ qtApp->sendPostedEvents();
689+ auto mirSurface = surfaceManager->find(windowInfo);
690+ ASSERT_TRUE(mirSurface);
691+ ASSERT_FALSE(mirSurface->focused()); // false must be the initial state
692+
693+ QSignalSpy mirSurfaceFocusSpy(mirSurface, &MirSurface::focusedChanged);
694+
695+ // Test
696+ Q_EMIT wmNotifier.windowFocusChanged(windowInfo, true);
697+ qtApp->sendPostedEvents();
698+
699+ // Check result
700+ EXPECT_EQ(1, mirSurfaceFocusSpy.count());
701+ EXPECT_EQ(mirSurface->focused(), true);
702+}
703+
704+/*
705+ * Test if MirAL notifies that a window's state changes, SurfaceManager updates the corresponding
706+ * MirSurface state (just testing a single state, see no value in testing all possible states here)
707+ */
708+TEST_F(SurfaceManagerTests, miralWindowStateChangeUpdatesMirSurfaceState)
709+{
710+ auto newState = Mir::FullscreenState;
711+
712+ // Setup: add window and get corresponding MirSurface
713+ Q_EMIT wmNotifier.windowAdded(windowInfo);
714+ qtApp->sendPostedEvents();
715+ auto mirSurface = surfaceManager->find(windowInfo);
716+ ASSERT_TRUE(mirSurface);
717+
718+ QSignalSpy mirSurfaceStateSpy(mirSurface, &MirSurface::stateChanged);
719+
720+ // Test
721+ Q_EMIT wmNotifier.windowStateChanged(windowInfo, newState);
722+ qtApp->sendPostedEvents();
723+
724+ // Check result
725+ EXPECT_EQ(1, mirSurfaceStateSpy.count());
726+ EXPECT_EQ(mirSurface->state(), newState);
727+}
728+
729+/*
730+ * Test when miral raises a list of surfaces, the raise signal is fired by SurfaceManager with
731+ * a list of MirSurfaces in the matching order
732+ */
733+TEST_F(SurfaceManagerTests, miralsRaiseWindowListTransformedToVectorOfMirSurfaces)
734+{
735+ // Setup
736+ miral::Window window1(stubSession, stubSurface);
737+ miral::Window window2(stubSession, stubSurface);
738+ miral::WindowInfo windowInfo1(window1, spec);
739+ miral::WindowInfo windowInfo2(window2, spec);
740+
741+ // Setup: add 2 windows and get their MirSurfaces
742+ Q_EMIT wmNotifier.windowAdded(windowInfo1);
743+ Q_EMIT wmNotifier.windowAdded(windowInfo2);
744+ qtApp->sendPostedEvents();
745+ auto mirSurface1 = surfaceManager->find(windowInfo1);
746+ auto mirSurface2 = surfaceManager->find(windowInfo2);
747+ ASSERT_TRUE(mirSurface1);
748+ ASSERT_TRUE(mirSurface2);
749+
750+ QSignalSpy mirSurfacesRaisedSpy(surfaceManager.data(), &SurfaceManager::surfacesRaised);
751+
752+ // Test
753+ std::vector<miral::Window> raiseWindowList{window2, window1};
754+ Q_EMIT wmNotifier.windowsRaised(raiseWindowList);
755+ qtApp->sendPostedEvents();
756+
757+ // Check results
758+ ASSERT_EQ(1, mirSurfacesRaisedSpy.count());
759+ auto raiseMirSurfaceList = qvariant_cast<QVector<unity::shell::application::MirSurfaceInterface*>>(
760+ mirSurfacesRaisedSpy.takeFirst().at(0)); // first argument of signal
761+ ASSERT_EQ(2, raiseMirSurfaceList.count());
762+ EXPECT_EQ(mirSurface1, raiseMirSurfaceList.at(1));
763+ EXPECT_EQ(mirSurface2, raiseMirSurfaceList.at(0));
764+}
765+
766+/*
767+ * Test focus requests fire focusRequested signal of the MirSurface
768+ */
769+TEST_F(SurfaceManagerTests, focusRequestCausesMirSurfaceToFireFocusRequestedSignal)
770+{
771+ // Setup: add window and get corresponding MirSurface
772+ Q_EMIT wmNotifier.windowAdded(windowInfo);
773+ qtApp->sendPostedEvents();
774+ auto mirSurface = surfaceManager->find(windowInfo);
775+ ASSERT_TRUE(mirSurface);
776+
777+ QSignalSpy mirSurfaceFocusRequestedSpy(mirSurface, &MirSurface::focusRequested);
778+
779+ // Test
780+ Q_EMIT wmNotifier.windowRequestedRaise(windowInfo);
781+ qtApp->sendPostedEvents();
782+
783+ // Check result
784+ EXPECT_EQ(1, mirSurfaceFocusRequestedSpy.count());
785+}
786+
787+/*
788+ * If MirAL notifies that a window was removed, and its corresponding MirSurface is not
789+ * being displayed, test that SurfaceManager removes the corresponding MirSurface from
790+ * its internal list and deletes the MirSurface
791+ */
792+TEST_F(SurfaceManagerTests, miralWindowRemovedDeletesSurfaceManagerInternalEntryAndMirSurface)
793+{
794+ // Setup: add window and get corresponding MirSurface
795+ Q_EMIT wmNotifier.windowAdded(windowInfo);
796+ qtApp->sendPostedEvents();
797+ auto mirSurface = surfaceManager->find(windowInfo);
798+ ASSERT_TRUE(mirSurface);
799+
800+ QSignalSpy mirSurfaceDestroyedSpy(mirSurface, &QObject::destroyed);
801+
802+ // Test
803+ Q_EMIT wmNotifier.windowRemoved(windowInfo);
804+ qtApp->sendPostedEvents();
805+
806+ // Check result
807+ ASSERT_EQ(2, mirSurfaceDestroyedSpy.count()); //FIXME - should be 1
808+ EXPECT_FALSE(surfaceManager->find(windowInfo));
809+}
810+
811+/*
812+ * If MirAL notifies that a window was removed, and its corresponding MirSurface *is*
813+ * being displayed, test that SurfaceManager removes the corresponding MirSurface from
814+ * its internal list but does *not* delete the MirSurface, but sets it as not "live"
815+ */
816+TEST_F(SurfaceManagerTests, miralWindowRemovedDeletesSurfaceManagerInternalEntryButNotMirSurface)
817+{
818+ // Setup: add window and get corresponding MirSurface
819+ Q_EMIT wmNotifier.windowAdded(windowInfo);
820+ qtApp->sendPostedEvents();
821+ auto mirSurface = surfaceManager->find(windowInfo);
822+ ASSERT_TRUE(mirSurface);
823+
824+ QSignalSpy mirSurfaceDestroyedSpy(mirSurface, &QObject::destroyed);
825+
826+ mirSurface->registerView(1);
827+
828+ // Test
829+ Q_EMIT wmNotifier.windowRemoved(windowInfo);
830+ qtApp->sendPostedEvents();
831+
832+ // Check result
833+ ASSERT_EQ(0, mirSurfaceDestroyedSpy.count());
834+ EXPECT_FALSE(surfaceManager->find(windowInfo));
835+ EXPECT_FALSE(mirSurface->live());
836+}
837+
838+/*
839+ * If MirAL notifies that a window was removed, and its corresponding MirSurface *is*
840+ * being displayed, SurfaceManager does *not* delete the MirSurface. Later when that
841+ * MirSurface is not being displayed any more, SurfaceManager should delete it.
842+ */
843+TEST_F(SurfaceManagerTests, miralWindowRemovedSurfaceManagerDeletesMirSurfaceWhenItDoneWith)
844+{
845+ int viewId = 99;
846+
847+ // Setup: add window and get corresponding MirSurface
848+ Q_EMIT wmNotifier.windowAdded(windowInfo);
849+ qtApp->sendPostedEvents();
850+ auto mirSurface = surfaceManager->find(windowInfo);
851+ ASSERT_TRUE(mirSurface);
852+
853+ QSignalSpy mirSurfaceDestroyedSpy(mirSurface, &QObject::destroyed);
854+
855+ // Setup: indicate MirSurface is being displayed
856+ mirSurface->registerView(viewId);
857+
858+ // Setup: notify that window removed
859+ Q_EMIT wmNotifier.windowRemoved(windowInfo);
860+ qtApp->sendPostedEvents();
861+
862+ // Test
863+ mirSurface->unregisterView(viewId);
864+ // MirSurface is deleteLater()ed by SurfaceManager, so need to spin event loop.
865+ // But DeferredDelete is special: likes to be called out specifically or it won't come out
866+ qtApp->sendPostedEvents(mirSurface, QEvent::DeferredDelete);
867+ qtApp->sendPostedEvents();
868+
869+ // Check result
870+ ASSERT_EQ(2, mirSurfaceDestroyedSpy.count()); //FIXME - should be 1
871+}
872+
873+/*
874+ * Test that if a MirSurface is live, and stops being displayed, SurfaceManager does *not*
875+ * delete the MirSurface.
876+ */
877+TEST_F(SurfaceManagerTests, surfaceManagerDoesNotDeleteLiveMirSurfaceWhenStopsBeingDisplayed)
878+{
879+ int viewId = 99;
880+
881+ // Setup: add window and get corresponding MirSurface
882+ Q_EMIT wmNotifier.windowAdded(windowInfo);
883+ qtApp->sendPostedEvents();
884+ auto mirSurface = surfaceManager->find(windowInfo);
885+ ASSERT_TRUE(mirSurface);
886+ ASSERT_TRUE(mirSurface->live());
887+
888+ QSignalSpy mirSurfaceDestroyedSpy(mirSurface, &QObject::destroyed);
889+
890+ // Setup: indicate MirSurface is being displayed
891+ mirSurface->registerView(viewId);
892+
893+ // Test
894+ mirSurface->unregisterView(viewId);
895+
896+ // Check result
897+ ASSERT_EQ(0, mirSurfaceDestroyedSpy.count());
898+}
899
900=== modified file 'tests/modules/WindowManager/mirsurface_test.cpp'
901--- tests/modules/WindowManager/mirsurface_test.cpp 2017-02-02 09:29:46 +0000
902+++ tests/modules/WindowManager/mirsurface_test.cpp 2017-03-20 15:56:36 +0000
903@@ -110,66 +110,6 @@
904 ASSERT_TRUE(spyFrameDropped.count() > 0);
905 }
906
907-
908-TEST_F(MirSurfaceTest, DeleteMirSurfaceOnLastNonLiveUnregisterView)
909-{
910- int argc = 0;
911- char* argv[0];
912- QCoreApplication qtApp(argc, argv); // app for deleteLater event
913-
914- miral::Window mockWindow(stubSession, stubSurface);
915- ms::SurfaceCreationParameters spec;
916- miral::WindowInfo mockWindowInfo(mockWindow, spec);
917-
918- auto surface = new MirSurface(mockWindowInfo, nullptr); // lives on the heap, as it will delete itself
919-
920- bool surfaceDeleted = false;
921- QObject::connect(surface, &QObject::destroyed, [&surfaceDeleted](){ surfaceDeleted = true; });
922-
923- qintptr view1 = (qintptr)1;
924- qintptr view2 = (qintptr)2;
925-
926- surface->registerView(view1);
927- surface->registerView(view2);
928- surface->setLive(false);
929- EXPECT_FALSE(surfaceDeleted);
930-
931- surface->unregisterView(view1);
932- surface->unregisterView(view2);
933-
934- QCoreApplication::sendPostedEvents(0, QEvent::DeferredDelete);
935- EXPECT_TRUE(surfaceDeleted);
936-}
937-
938-
939-TEST_F(MirSurfaceTest, DISABLED_DoNotDeleteMirSurfaceOnLastLiveUnregisterView)
940-{
941- int argc = 0;
942- char* argv[0];
943- QCoreApplication qtApp(argc, argv); // app for deleteLater event
944-
945- miral::Window mockWindow(stubSession, stubSurface);
946- ms::SurfaceCreationParameters spec;
947- miral::WindowInfo mockWindowInfo(mockWindow, spec);
948-
949- auto surface = new MirSurface(mockWindowInfo, nullptr); // lives on the heap, as it may delete itself
950-
951- bool surfaceDeleted = false;
952- QObject::connect(surface, &QObject::destroyed, [&surfaceDeleted](){ surfaceDeleted = true; });
953-
954- qintptr view1 = (qintptr)1;
955- qintptr view2 = (qintptr)2;
956-
957- surface->registerView(view1);
958- surface->registerView(view2);
959-
960- surface->unregisterView(view1);
961- surface->unregisterView(view2);
962-
963- QCoreApplication::sendPostedEvents(0, QEvent::DeferredDelete);
964- EXPECT_FALSE(surfaceDeleted);
965-}
966-
967 /*
968 * Test that MirSurface.visible is recalculated after the client swaps the first frame.
969 * A surface is not considered visible unless it has a non-hidden & non-minimized state, and

Subscribers

People subscribed via source and target branches