QtMir

Merge lp:~nick-dedekind/qtmir/lp1475678.surface-occlude into lp:qtmir

  1. lp1475678.surface-occlude
  2. Merge into trunk
Proposed by Nick Dedekind on 2015-10-05
Status: Merged
Approved by: Daniel d'Andrada on 2015-10-20
Approved revision: 390
Merged at revision: 401
Proposed branch: lp:~nick-dedekind/qtmir/lp1475678.surface-occlude
Merge into: lp:qtmir
Diff against target: 638 lines (+294/-49)
12 files modified
debian/control (+1/-1)
src/modules/Unity/Application/mirsurface.cpp (+48/-14)
src/modules/Unity/Application/mirsurface.h (+11/-3)
src/modules/Unity/Application/mirsurfaceinterface.h (+4/-2)
src/modules/Unity/Application/mirsurfaceitem.cpp (+17/-4)
src/modules/Unity/Application/mirsurfaceitem.h (+1/-0)
tests/modules/CMakeLists.txt (+2/-2)
tests/modules/SurfaceManager/CMakeLists.txt (+6/-4)
tests/modules/SurfaceManager/mirsurface_test.cpp (+107/-0)
tests/modules/SurfaceManager/mirsurfaceitem_test.cpp (+54/-5)
tests/modules/common/fake_mirsurface.h (+42/-14)
tests/modules/common/fake_session.h (+1/-0)
To merge this branch: bzr merge lp:~nick-dedekind/qtmir/lp1475678.surface-occlude
Reviewer Review Type Date Requested Status
Daniel d'Andrada (community) 2015-10-05 Approve on 2015-10-20
PS Jenkins bot continuous-integration Needs Fixing on 2015-10-20
Review via email: mp+273426@code.launchpad.net

Commit Message

Support server->client visibility change to stop rendering (lp:#1475678)

Description of the Change

Support server->client visibility change to stop rendering (lp:#1475678)

 * Are there any related MPs required for this MP to build/function as expected? Please list.
https://code.launchpad.net/~nick-dedekind/unity8/lp1475678.surface-occlude/+merge/273427
https://code.launchpad.net/~nick-dedekind/unity-api/lp1475678.surface-occlude/+merge/273425
https://code.launchpad.net/~nick-dedekind/qtubuntu/lp1475678.surface-occlude/+merge/273424

 * Did you perform an exploratory manual test run of your code change and any related functionality?
 * If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?

To post a comment you must log in.
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Daniel d'Andrada (dandrader) wrote :

One thing to look at is to what happens when MirSurfaceItem::setSurface() is called and the MirSurfaceItem in question is already hidden. You have to synce the visibility of MirSurface with the one of its MirSurfaceItem inside setSurface().

Another thing to consider is what happens when there are multiple MirSurfaceItems displaying the same MirSurface? It will only get occluded if *all* items showing it are hidden. In other words: the MirSurface will be exposed as long as there's some visible MirSurfaceItem in the scene displaying it.

review: Needs Fixing
lp:~nick-dedekind/qtmir/lp1475678.surface-occlude updated on 2015-10-12
380. By Nick Dedekind on 2015-10-12

Aggregate the surface visibility

Nick Dedekind (nick-dedekind) wrote :

> One thing to look at is to what happens when MirSurfaceItem::setSurface() is
> called and the MirSurfaceItem in question is already hidden. You have to synce
> the visibility of MirSurface with the one of its MirSurfaceItem inside
> setSurface().

It did already, but I've updated since (view registration).

>
> Another thing to consider is what happens when there are multiple
> MirSurfaceItems displaying the same MirSurface? It will only get occluded if
> *all* items showing it are hidden. In other words: the MirSurface will be
> exposed as long as there's some visible MirSurfaceItem in the scene displaying
> it.

I've added surface visibility aggregation through view registration.

lp:~nick-dedekind/qtmir/lp1475678.surface-occlude updated on 2015-10-12
381. By Nick Dedekind on 2015-10-12

only log on change

382. By Nick Dedekind on 2015-10-12

use isEmpty

PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
lp:~nick-dedekind/qtmir/lp1475678.surface-occlude updated on 2015-10-12
383. By Nick Dedekind on 2015-10-12

use isEmpty

PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Daniel d'Andrada (dandrader) wrote :

Please remove the cyclic dependency between MirSurfaceItem and MirSurface.

review: Needs Fixing
lp:~nick-dedekind/qtmir/lp1475678.surface-occlude updated on 2015-10-13
384. By Nick Dedekind on 2015-10-13

remove circ dep

PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Daniel d'Andrada (dandrader) wrote :

"""
+int MirSurface::registerView()
"""

It would be better if it took the view id as a parameter, like this:
void MirSurface::registerView(qintptr viewId)

That way MirSurfaceItem could simply do:
m_surface->registerView((qintptr)this);

and not have to bother storing any id at all. Afterall, MirSurface::m_views is already storing them. So no need to store on both ends.

Sorry, I should have clarified that when you sent me http://pastebin.ubuntu.com/12773655/. I thought had you simply overlooked the missing parameter in that draft (and I didn't notice it was returning an int).

review: Needs Fixing
Daniel d'Andrada (dandrader) wrote :

I think you can remove that from tests/modules/common/fake_mirsurface.h

"""
+#include <QDebug>
"""

Nick Dedekind (nick-dedekind) wrote :

> """
> +int MirSurface::registerView()
> """
>
> It would be better if it took the view id as a parameter, like this:
> void MirSurface::registerView(qintptr viewId)
>
>
> That way MirSurfaceItem could simply do:
> m_surface->registerView((qintptr)this);
>
> and not have to bother storing any id at all. Afterall, MirSurface::m_views is
> already storing them. So no need to store on both ends.
>
> Sorry, I should have clarified that when you sent me
> http://pastebin.ubuntu.com/12773655/. I thought had you simply overlooked the
> missing parameter in that draft (and I didn't notice it was returning an int).

I don't think that's very good coding practice. It requires the caller to be responsible for choosing a value unique to another class (even if it's its mem address, it's still the assumption). Most registration mechanisms do it my way, for good reason.

lp:~nick-dedekind/qtmir/lp1475678.surface-occlude updated on 2015-10-14
385. By Nick Dedekind on 2015-10-14

removed debug

Nick Dedekind (nick-dedekind) wrote :

> I think you can remove that from tests/modules/common/fake_mirsurface.h
>
> """
> +#include <QDebug>
> """

Done.

Daniel d'Andrada (dandrader) wrote :

On 14/10/2015 11:57, Nick Dedekind wrote:
>> """
>> +int MirSurface::registerView()
>> """
>>
>> It would be better if it took the view id as a parameter, like this:
>> void MirSurface::registerView(qintptr viewId)
>>
>>
>> That way MirSurfaceItem could simply do:
>> m_surface->registerView((qintptr)this);
>>
>> and not have to bother storing any id at all. Afterall, MirSurface::m_views is
>> already storing them. So no need to store on both ends.
>>
>> Sorry, I should have clarified that when you sent me
>> http://pastebin.ubuntu.com/12773655/. I thought had you simply overlooked the
>> missing parameter in that draft (and I didn't notice it was returning an int).
> I don't think that's very good coding practice. It requires the caller to be responsible for choosing a value unique to another class (even if it's its mem address, it's still the assumption). Most registration mechanisms do it my way, for good reason.

mir::graphics::RenderableList
generate_renderables(compositor::CompositorID id) uses the same approach
(CompositorID being "void const*") as well, if you want an exmaple.

PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Gerry Boland (gerboland) wrote :

> > """
> > +int MirSurface::registerView()
> > """
> >
> > It would be better if it took the view id as a parameter, like this:
> > void MirSurface::registerView(qintptr viewId)
> >
> >
> > That way MirSurfaceItem could simply do:
> > m_surface->registerView((qintptr)this);
> >
> > and not have to bother storing any id at all. Afterall, MirSurface::m_views
> is
> > already storing them. So no need to store on both ends.
> >
> > Sorry, I should have clarified that when you sent me
> > http://pastebin.ubuntu.com/12773655/. I thought had you simply overlooked
> the
> > missing parameter in that draft (and I didn't notice it was returning an
> int).
>
> I don't think that's very good coding practice. It requires the caller to be
> responsible for choosing a value unique to another class (even if it's its mem
> address, it's still the assumption). Most registration mechanisms do it my
> way, for good reason.

You're right in that this way is strictly better, especially if you don't have control over the clients registering. For now, we do have control over the clients, so we can ensure they behave correctly.

Re-using "this" memory address as unique identifier means there's one less variable to worry about.

Since we control the clients, I don't think we need the complexity of the correct approach just now. If third parties start using qtmir MirSurface directly, then we will need to rethink it.

Daniel d'Andrada (dandrader) wrote :

On 14/10/2015 13:41, Gerry Boland wrote:
>>> """
>>> +int MirSurface::registerView()
>>> """
>>>
>>> It would be better if it took the view id as a parameter, like this:
>>> void MirSurface::registerView(qintptr viewId)
>>>
>>>
>>> That way MirSurfaceItem could simply do:
>>> m_surface->registerView((qintptr)this);
>>>
>>> and not have to bother storing any id at all. Afterall, MirSurface::m_views
>> is
>>> already storing them. So no need to store on both ends.
>>>
>>> Sorry, I should have clarified that when you sent me
>>> http://pastebin.ubuntu.com/12773655/. I thought had you simply overlooked
>> the
>>> missing parameter in that draft (and I didn't notice it was returning an
>> int).
>>
>> I don't think that's very good coding practice. It requires the caller to be
>> responsible for choosing a value unique to another class (even if it's its mem
>> address, it's still the assumption). Most registration mechanisms do it my
>> way, for good reason.
> You're right in that this way is strictly better, especially if you don't have control over the clients registering. For now, we do have control over the clients, so we can ensure they behave correctly.
>
> Re-using "this" memory address as unique identifier means there's one less variable to worry about.
>
> Since we control the clients, I don't think we need the complexity of the correct approach just now. If third parties start using qtmir MirSurface directly, then we will need to rethink it.

qtmir::MirSurfaceInterface (where registerView() belongs to) is an
internal qtmir API.

lp:~nick-dedekind/qtmir/lp1475678.surface-occlude updated on 2015-10-19
386. By Nick Dedekind on 2015-10-19

use qintptr for viewId

Nick Dedekind (nick-dedekind) wrote :

> On 14/10/2015 13:41, Gerry Boland wrote:
> >>> """
> >>> +int MirSurface::registerView()
> >>> """
> >>>
> >>> It would be better if it took the view id as a parameter, like this:
> >>> void MirSurface::registerView(qintptr viewId)
> >>>
> >>>
> >>> That way MirSurfaceItem could simply do:
> >>> m_surface->registerView((qintptr)this);
> >>>
> >>> and not have to bother storing any id at all. Afterall,
> MirSurface::m_views
> >> is
> >>> already storing them. So no need to store on both ends.
> >>>
> >>> Sorry, I should have clarified that when you sent me
> >>> http://pastebin.ubuntu.com/12773655/. I thought had you simply overlooked
> >> the
> >>> missing parameter in that draft (and I didn't notice it was returning an
> >> int).
> >>
> >> I don't think that's very good coding practice. It requires the caller to
> be
> >> responsible for choosing a value unique to another class (even if it's its
> mem
> >> address, it's still the assumption). Most registration mechanisms do it my
> >> way, for good reason.
> > You're right in that this way is strictly better, especially if you don't
> have control over the clients registering. For now, we do have control over
> the clients, so we can ensure they behave correctly.
> >
> > Re-using "this" memory address as unique identifier means there's one less
> variable to worry about.
> >
> > Since we control the clients, I don't think we need the complexity of the
> correct approach just now. If third parties start using qtmir MirSurface
> directly, then we will need to rethink it.
>
> qtmir::MirSurfaceInterface (where registerView() belongs to) is an
> internal qtmir API.

Done.

PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Daniel d'Andrada (dandrader) wrote :

MirSurface should call deleteLater() if the last MirSurfaceItem unregisters from it and it's no longer live() or pointed to by a session.

Ie, this code should be kept:

"""
if (m_viewCount == 0) {
    [..]
    if (m_session.isNull() || !m_live) {
        deleteLater();
    }
}
"""

review: Needs Fixing
Daniel d'Andrada (dandrader) wrote :

Kept in MirSurface::unregisterView() I mean.

lp:~nick-dedekind/qtmir/lp1475678.surface-occlude updated on 2015-10-20
387. By Nick Dedekind on 2015-10-20

Re-added deleteLater

388. By Nick Dedekind on 2015-10-20

Added test for Surface deleteLater on deregister

Nick Dedekind (nick-dedekind) wrote :

> MirSurface should call deleteLater() if the last MirSurfaceItem unregisters
> from it and it's no longer live() or pointed to by a session.
>
> Ie, this code should be kept:
>
> """
> if (m_viewCount == 0) {
> [..]
> if (m_session.isNull() || !m_live) {
> deleteLater();
> }
> }
> """

Yikes, my bad. don't know how that happened.
re-added.

I also added a MirSurface test (to test the delete on unregister) and moved the surface related tests to a "SurfaceManager" test folder. Most other things are hosted under their Management related folders (App & Session tests). We should be adding SurfaceManager tests anyway. Guess this will come with multi-window support.

lp:~nick-dedekind/qtmir/lp1475678.surface-occlude updated on 2015-10-20
389. By Nick Dedekind on 2015-10-20

added missing file

390. By Nick Dedekind on 2015-10-20

removed line

PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Daniel d'Andrada (dandrader) wrote :

Code looks good. Didn't test yet.

review: Approve (code)
Daniel d'Andrada (dandrader) :
review: Approve
lp:~nick-dedekind/qtmir/lp1475678.surface-occlude updated on 2015-10-27
391. By Nick Dedekind on 2015-10-21

merged with trunk

392. By Nick Dedekind on 2015-10-26

merged with trunk

393. By Nick Dedekind on 2015-10-26

bump libunity-api version

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'debian/control'
2--- debian/control 2015-10-21 11:46:54 +0000
3+++ debian/control 2015-10-27 16:23:25 +0000
4@@ -23,7 +23,7 @@
5 libubuntu-app-launch2-dev,
6 libubuntu-application-api-dev (>= 2.1.0),
7 libudev-dev,
8- libunity-api-dev (>= 7.101),
9+ libunity-api-dev (>= 7.102),
10 liburl-dispatcher1-dev,
11 libxkbcommon-dev,
12 libxrender-dev,
13
14=== modified file 'src/modules/Unity/Application/mirsurface.cpp'
15--- src/modules/Unity/Application/mirsurface.cpp 2015-10-21 11:47:03 +0000
16+++ src/modules/Unity/Application/mirsurface.cpp 2015-10-27 16:23:25 +0000
17@@ -176,7 +176,6 @@
18 , m_textureUpdated(false)
19 , m_currentFrameNumber(0)
20 , m_live(true)
21- , m_viewCount(0)
22 {
23 m_surfaceObserver = observer;
24 if (observer) {
25@@ -208,9 +207,9 @@
26
27 MirSurface::~MirSurface()
28 {
29- qCDebug(QTMIR_SURFACES).nospace() << "MirSurface::~MirSurface this=" << this << " viewCount=" << m_viewCount;
30+ qCDebug(QTMIR_SURFACES).nospace() << "MirSurface::~MirSurface this=" << this << " viewCount=" << m_views.count();
31
32- Q_ASSERT(m_viewCount == 0);
33+ Q_ASSERT(m_views.isEmpty());
34
35 if (m_session) {
36 m_session->setSurface(nullptr);
37@@ -242,6 +241,9 @@
38 case mir_surface_attrib_state:
39 Q_EMIT stateChanged(state());
40 break;
41+ case mir_surface_attrib_visibility:
42+ Q_EMIT visibleChanged(visible());
43+ break;
44 default:
45 break;
46 }
47@@ -555,6 +557,11 @@
48 return m_live;
49 }
50
51+bool MirSurface::visible() const
52+{
53+ return m_surface->query(mir_surface_attrib_visibility) == mir_surface_visibility_exposed;
54+}
55+
56 void MirSurface::mousePressEvent(QMouseEvent *event)
57 {
58 auto ev = makeMirEvent(event, mir_pointer_action_button_down);
59@@ -638,29 +645,56 @@
60
61 bool MirSurface::isBeingDisplayed() const
62 {
63- return m_viewCount > 0;
64+ return !m_views.isEmpty();
65 }
66
67-void MirSurface::incrementViewCount()
68+void MirSurface::registerView(qintptr viewId)
69 {
70- ++m_viewCount;
71- qCDebug(QTMIR_SURFACES).nospace() << "MirSurface::incrementViewCount after=" << m_viewCount;
72- if (m_viewCount == 1) {
73+ m_views.insert(viewId, MirSurface::View{false});
74+ qCDebug(QTMIR_SURFACES).nospace() << "MirSurface[" << appId() << "]::registerView(" << viewId << ")"
75+ << " after=" << m_views.count();
76+ if (m_views.count() == 1) {
77 Q_EMIT isBeingDisplayedChanged();
78 }
79 }
80
81-void MirSurface::decrementViewCount()
82+void MirSurface::unregisterView(qintptr viewId)
83 {
84- Q_ASSERT(m_viewCount > 0);
85- --m_viewCount;
86- qCDebug(QTMIR_SURFACES).nospace() << "MirSurface::decrementViewCount after=" << m_viewCount;
87- if (m_viewCount == 0) {
88+ qCDebug(QTMIR_SURFACES).nospace() << "MirSurface[" << appId() << "]::unregisterView(" << viewId << ")"
89+ << " after=" << m_views.count() << " live=" << m_live;
90+ m_views.remove(viewId);
91+ if (m_views.count() == 0) {
92 Q_EMIT isBeingDisplayedChanged();
93 if (m_session.isNull() || !m_live) {
94 deleteLater();
95 }
96 }
97+ updateVisibility();
98+}
99+
100+void MirSurface::setViewVisibility(qintptr viewId, bool visible)
101+{
102+ if (!m_views.contains(viewId)) return;
103+
104+ m_views[viewId].visible = visible;
105+ updateVisibility();
106+}
107+
108+void MirSurface::updateVisibility()
109+{
110+ bool newVisible = false;
111+ QHashIterator<qintptr, View> i(m_views);
112+ while (i.hasNext()) {
113+ i.next();
114+ newVisible |= i.value().visible;
115+ }
116+
117+ if (newVisible != visible()) {
118+ qCDebug(QTMIR_SURFACES).nospace() << "MirSurface[" << appId() << "]::updateVisibility(" << newVisible << ")";
119+
120+ m_surface->configure(mir_surface_attrib_visibility,
121+ newVisible ? mir_surface_visibility_exposed : mir_surface_visibility_occluded);
122+ }
123 }
124
125 unsigned int MirSurface::currentFrameNumber() const
126@@ -670,7 +704,7 @@
127
128 void MirSurface::onSessionDestroyed()
129 {
130- if (m_viewCount == 0) {
131+ if (m_views.isEmpty()) {
132 deleteLater();
133 }
134 }
135
136=== modified file 'src/modules/Unity/Application/mirsurface.h'
137--- src/modules/Unity/Application/mirsurface.h 2015-10-21 11:47:03 +0000
138+++ src/modules/Unity/Application/mirsurface.h 2015-10-27 16:23:25 +0000
139@@ -67,6 +67,8 @@
140
141 bool live() const override;
142
143+ bool visible() const override;
144+
145 Mir::OrientationAngle orientationAngle() const override;
146 void setOrientationAngle(Mir::OrientationAngle angle) override;
147
148@@ -81,8 +83,10 @@
149 void startFrameDropper() override;
150
151 bool isBeingDisplayed() const override;
152- void incrementViewCount() override;
153- void decrementViewCount() override;
154+
155+ void registerView(qintptr viewId) override;
156+ void unregisterView(qintptr viewId) override;
157+ void setViewVisibility(qintptr viewId, bool visible) override;
158
159 // methods called from the rendering (scene graph) thread:
160 QSharedPointer<QSGTexture> texture() override;
161@@ -125,6 +129,7 @@
162 private:
163 void syncSurfaceSizeWithItemSize();
164 bool clientIsRunning() const;
165+ void updateVisibility();
166
167 std::shared_ptr<mir::scene::Surface> m_surface;
168 QPointer<SessionInterface> m_session;
169@@ -144,7 +149,10 @@
170 unsigned int m_currentFrameNumber;
171
172 bool m_live;
173- int m_viewCount;
174+ struct View {
175+ bool visible;
176+ };
177+ QHash<qintptr, View> m_views;
178
179 std::shared_ptr<SurfaceObserver> m_surfaceObserver;
180
181
182=== modified file 'src/modules/Unity/Application/mirsurfaceinterface.h'
183--- src/modules/Unity/Application/mirsurfaceinterface.h 2015-10-13 18:29:13 +0000
184+++ src/modules/Unity/Application/mirsurfaceinterface.h 2015-10-27 16:23:25 +0000
185@@ -48,8 +48,10 @@
186 virtual void startFrameDropper() = 0;
187
188 virtual bool isBeingDisplayed() const = 0;
189- virtual void incrementViewCount() = 0;
190- virtual void decrementViewCount() = 0;
191+
192+ virtual void registerView(qintptr viewId) = 0;
193+ virtual void unregisterView(qintptr viewId) = 0;
194+ virtual void setViewVisibility(qintptr viewId, bool visible) = 0;
195
196 // methods called from the rendering (scene graph) thread:
197 virtual QSharedPointer<QSGTexture> texture() = 0;
198
199=== modified file 'src/modules/Unity/Application/mirsurfaceitem.cpp'
200--- src/modules/Unity/Application/mirsurfaceitem.cpp 2015-10-13 18:29:13 +0000
201+++ src/modules/Unity/Application/mirsurfaceitem.cpp 2015-10-27 16:23:25 +0000
202@@ -106,6 +106,7 @@
203 connect(&m_updateMirSurfaceSizeTimer, &QTimer::timeout, this, &MirSurfaceItem::updateMirSurfaceSize);
204
205 connect(this, &QQuickItem::activeFocusChanged, this, &MirSurfaceItem::updateMirSurfaceFocus);
206+ connect(this, &QQuickItem::visibleChanged, this, &MirSurfaceItem::updateMirSurfaceVisibility);
207 }
208
209 MirSurfaceItem::~MirSurfaceItem()
210@@ -524,6 +525,15 @@
211 m_surface->resize(width, height);
212 }
213
214+void MirSurfaceItem::updateMirSurfaceVisibility()
215+{
216+ if (!m_surface || !m_surface->live()) {
217+ return;
218+ }
219+
220+ m_surface->setViewVisibility((qintptr)this, isVisible());
221+}
222+
223 void MirSurfaceItem::updateMirSurfaceFocus(bool focused)
224 {
225 if (m_surface && m_consumesInput && m_surface->live()) {
226@@ -603,7 +613,7 @@
227 m_surface->setFocus(false);
228 }
229
230- m_surface->decrementViewCount();
231+ m_surface->unregisterView((qintptr)this);
232
233 if (!m_surface->isBeingDisplayed() && window()) {
234 disconnect(window(), nullptr, m_surface, nullptr);
235@@ -613,7 +623,7 @@
236 m_surface = surface;
237
238 if (m_surface) {
239- m_surface->incrementViewCount();
240+ m_surface->registerView((qintptr)this);
241
242 // When a new mir frame gets posted we notify the QML engine that this item needs redrawing,
243 // schedules call to updatePaintNode() from the rendering thread
244@@ -623,8 +633,10 @@
245 connect(m_surface, &MirSurfaceInterface::liveChanged, this, &MirSurfaceItem::liveChanged);
246 connect(m_surface, &MirSurfaceInterface::sizeChanged, this, &MirSurfaceItem::onActualSurfaceSizeChanged);
247
248- connect(window(), &QQuickWindow::frameSwapped, m_surface, &MirSurfaceInterface::onCompositorSwappedBuffers,
249- (Qt::ConnectionType) (Qt::DirectConnection | Qt::UniqueConnection));
250+ if (window()) {
251+ connect(window(), &QQuickWindow::frameSwapped, m_surface, &MirSurfaceInterface::onCompositorSwappedBuffers,
252+ (Qt::ConnectionType) (Qt::DirectConnection | Qt::UniqueConnection));
253+ }
254
255 Q_EMIT typeChanged(m_surface->type());
256 Q_EMIT liveChanged(true);
257@@ -632,6 +644,7 @@
258
259 updateMirSurfaceSize();
260 setImplicitSize(m_surface->size().width(), m_surface->size().height());
261+ updateMirSurfaceVisibility();
262
263 if (m_orientationAngle) {
264 m_surface->setOrientationAngle(*m_orientationAngle);
265
266=== modified file 'src/modules/Unity/Application/mirsurfaceitem.h'
267--- src/modules/Unity/Application/mirsurfaceitem.h 2015-09-18 20:11:50 +0000
268+++ src/modules/Unity/Application/mirsurfaceitem.h 2015-10-27 16:23:25 +0000
269@@ -112,6 +112,7 @@
270 void updateMirSurfaceSize();
271
272 void updateMirSurfaceFocus(bool focused);
273+ void updateMirSurfaceVisibility();
274
275 void onActualSurfaceSizeChanged(const QSize &size);
276
277
278=== modified file 'tests/modules/CMakeLists.txt'
279--- tests/modules/CMakeLists.txt 2015-01-09 11:25:13 +0000
280+++ tests/modules/CMakeLists.txt 2015-10-27 16:23:25 +0000
281@@ -2,7 +2,7 @@
282 add_subdirectory(ApplicationManager)
283 add_subdirectory(DesktopFileReader)
284 add_subdirectory(General)
285-add_subdirectory(MirSurfaceItem)
286+add_subdirectory(SharedWakelock)
287 add_subdirectory(SessionManager)
288-add_subdirectory(SharedWakelock)
289+add_subdirectory(SurfaceManager)
290 add_subdirectory(TaskController)
291
292=== renamed directory 'tests/modules/MirSurfaceItem' => 'tests/modules/SurfaceManager'
293=== modified file 'tests/modules/SurfaceManager/CMakeLists.txt'
294--- tests/modules/MirSurfaceItem/CMakeLists.txt 2015-08-31 09:51:28 +0000
295+++ tests/modules/SurfaceManager/CMakeLists.txt 2015-10-27 16:23:25 +0000
296@@ -1,8 +1,10 @@
297 set(
298- MIR_SURFACE_ITEM_TEST_SOURCES
299+ MIR_SURFACE_MANAGER_TEST_SOURCES
300 mirsurfaceitem_test.cpp
301+ mirsurface_test.cpp
302 ${CMAKE_SOURCE_DIR}/src/common/debughelpers.cpp
303 ${CMAKE_SOURCE_DIR}/tests/modules/common/fake_mirsurface.h
304+ ${CMAKE_SOURCE_DIR}/tests/modules/common/fake_session.h
305 )
306
307 include_directories(
308@@ -11,10 +13,10 @@
309 ${MIRSERVER_INCLUDE_DIRS}
310 )
311
312-add_executable(mirsurfaceitem_test ${MIR_SURFACE_ITEM_TEST_SOURCES})
313+add_executable(surfacemanager_test ${MIR_SURFACE_MANAGER_TEST_SOURCES})
314
315 target_link_libraries(
316- mirsurfaceitem_test
317+ surfacemanager_test
318
319 qpa-mirserver
320 unityapplicationplugin
321@@ -25,4 +27,4 @@
322 ${GMOCK_LIBRARIES}
323 )
324
325-add_test(MirSurfaceItem mirsurfaceitem_test)
326+add_test(SurfaceManager surfacemanager_test)
327
328=== added file 'tests/modules/SurfaceManager/mirsurface_test.cpp'
329--- tests/modules/SurfaceManager/mirsurface_test.cpp 1970-01-01 00:00:00 +0000
330+++ tests/modules/SurfaceManager/mirsurface_test.cpp 2015-10-27 16:23:25 +0000
331@@ -0,0 +1,107 @@
332+/*
333+ * Copyright (C) 2014-2015 Canonical, Ltd.
334+ *
335+ * This program is free software: you can redistribute it and/or modify it under
336+ * the terms of the GNU Lesser General Public License version 3, as published by
337+ * the Free Software Foundation.
338+ *
339+ * This program is distributed in the hope that it will be useful, but WITHOUT
340+ * ANY WARRANTY; without even the implied warranties of MERCHANTABILITY,
341+ * SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
342+ * Lesser General Public License for more details.
343+ *
344+ * You should have received a copy of the GNU Lesser General Public License
345+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
346+ */
347+
348+#define MIR_INCLUDE_DEPRECATED_EVENT_HEADER
349+
350+#include <gtest/gtest.h>
351+
352+#include <QLoggingCategory>
353+#include <QTest>
354+
355+// the test subject
356+#include <Unity/Application/mirsurface.h>
357+
358+// tests/modules/common
359+#include <fake_session.h>
360+#include <mock_surface.h>
361+
362+using namespace qtmir;
363+
364+namespace ms = mir::scene;
365+
366+class MirSurfaceTest : public ::testing::Test
367+{
368+public:
369+ MirSurfaceTest()
370+ {
371+ // We don't want the logging spam cluttering the test results
372+ QLoggingCategory::setFilterRules(QStringLiteral("qtmir.surfaces=false"));
373+ }
374+};
375+
376+TEST_F(MirSurfaceTest, DeleteMirSurfaceOnLastNonLiveUnregisterView)
377+{
378+ using namespace testing;
379+
380+ int argc = 0;
381+ char* argv[0];
382+ QCoreApplication qtApp(argc, argv); // app for deleteLater event
383+
384+ auto fakeSession = new FakeSession();
385+ auto mockSurface = std::make_shared<NiceMock<ms::MockSurface>>();
386+
387+ MirSurface *surface = new MirSurface(mockSurface, fakeSession, nullptr, nullptr);
388+ bool surfaceDeleted = false;
389+ QObject::connect(surface, &QObject::destroyed, surface, [&surfaceDeleted](){ surfaceDeleted = true; });
390+
391+ qintptr view1 = (qintptr)1;
392+ qintptr view2 = (qintptr)2;
393+
394+ surface->registerView(view1);
395+ surface->registerView(view2);
396+ surface->setLive(false);
397+ EXPECT_FALSE(surfaceDeleted);
398+
399+ surface->unregisterView(view1);
400+ surface->unregisterView(view2);
401+
402+ QCoreApplication::sendPostedEvents(0, QEvent::DeferredDelete);
403+ EXPECT_TRUE(surfaceDeleted);
404+
405+ delete fakeSession;
406+}
407+
408+
409+TEST_F(MirSurfaceTest, DoNotDeleteMirSurfaceOnLastLiveUnregisterView)
410+{
411+ using namespace testing;
412+
413+ int argc = 0;
414+ char* argv[0];
415+ QCoreApplication qtApp(argc, argv); // app for deleteLater event
416+
417+ auto fakeSession = new FakeSession();
418+ auto mockSurface = std::make_shared<NiceMock<ms::MockSurface>>();
419+
420+ MirSurface *surface = new MirSurface(mockSurface, fakeSession, nullptr, nullptr);
421+ bool surfaceDeleted = false;
422+ QObject::connect(surface, &QObject::destroyed, surface, [&surfaceDeleted](){ surfaceDeleted = true; });
423+
424+ qintptr view1 = (qintptr)1;
425+ qintptr view2 = (qintptr)2;
426+
427+ surface->registerView(view1);
428+ surface->registerView(view2);
429+
430+ surface->unregisterView(view1);
431+ surface->unregisterView(view2);
432+
433+ QCoreApplication::sendPostedEvents(0, QEvent::DeferredDelete);
434+ EXPECT_FALSE(surfaceDeleted);
435+
436+ delete fakeSession;
437+ delete surface;
438+}
439
440=== modified file 'tests/modules/SurfaceManager/mirsurfaceitem_test.cpp'
441--- tests/modules/MirSurfaceItem/mirsurfaceitem_test.cpp 2015-08-31 09:51:28 +0000
442+++ tests/modules/SurfaceManager/mirsurfaceitem_test.cpp 2015-10-27 16:23:25 +0000
443@@ -29,22 +29,28 @@
444
445 using namespace qtmir;
446
447+class MirSurfaceItemTest : public ::testing::Test
448+{
449+public:
450+ MirSurfaceItemTest()
451+ {
452+ // We don't want the logging spam cluttering the test results
453+ QLoggingCategory::setFilterRules(QStringLiteral("qtmir.surfaces=false"));
454+ }
455+};
456+
457 /*
458 Tests that even if Qt fails to finish a touch sequence, MirSurfaceItem will
459 properly finish it when forwarding it to its mir::input::surface. So
460 mir::input::surface will still consume a proper sequence of touch events
461 (comprised of a begin, zero or more updates and an end).
462 */
463-TEST(MirSurfaceItemTest, MissingTouchEnd)
464+TEST_F(MirSurfaceItemTest, MissingTouchEnd)
465 {
466- // We don't want the logging spam cluttering the test results
467- QLoggingCategory::setFilterRules(QStringLiteral("qtmir*=false"));
468-
469 MirSurfaceItem *surfaceItem = new MirSurfaceItem;
470
471 FakeMirSurface *fakeSurface = new FakeMirSurface;
472
473-
474 surfaceItem->setSurface(fakeSurface);
475 surfaceItem->setConsumesInput(true);
476
477@@ -93,3 +99,46 @@
478 delete surfaceItem;
479 delete fakeSurface;
480 }
481+
482+TEST_F(MirSurfaceItemTest, SetSurfaceInitializesVisiblity)
483+{
484+ MirSurfaceItem *surfaceItem = new MirSurfaceItem;
485+ surfaceItem->setVisible(false);
486+
487+ FakeMirSurface *fakeSurface = new FakeMirSurface;
488+ surfaceItem->setSurface(fakeSurface);
489+
490+ EXPECT_FALSE(fakeSurface->visible());
491+
492+ delete surfaceItem;
493+ delete fakeSurface;
494+}
495+
496+TEST_F(MirSurfaceItemTest, AggregateSurfaceVisibility)
497+{
498+ MirSurfaceItem *surfaceItem1 = new MirSurfaceItem;
499+ surfaceItem1->setVisible(true);
500+ MirSurfaceItem *surfaceItem2 = new MirSurfaceItem;
501+ surfaceItem1->setVisible(true);
502+
503+ FakeMirSurface *fakeSurface = new FakeMirSurface;
504+ surfaceItem1->setSurface(fakeSurface);
505+ surfaceItem2->setSurface(fakeSurface);
506+
507+ EXPECT_TRUE(fakeSurface->visible());
508+
509+ surfaceItem1->setVisible(false);
510+ EXPECT_TRUE(fakeSurface->visible());
511+
512+ surfaceItem2->setVisible(false);
513+ EXPECT_FALSE(fakeSurface->visible());
514+
515+ surfaceItem1->setVisible(true);
516+ EXPECT_TRUE(fakeSurface->visible());
517+
518+ delete surfaceItem1;
519+ EXPECT_FALSE(fakeSurface->visible());
520+
521+ delete surfaceItem2;
522+ delete fakeSurface;
523+}
524
525=== modified file 'tests/modules/common/fake_mirsurface.h'
526--- tests/modules/common/fake_mirsurface.h 2015-10-13 18:29:13 +0000
527+++ tests/modules/common/fake_mirsurface.h 2015-10-27 16:23:25 +0000
528@@ -21,6 +21,7 @@
529
530 #include <QSharedPointer>
531 #include <QSGTexture>
532+#include <QPointer>
533
534 namespace qtmir {
535
536@@ -56,7 +57,7 @@
537 , m_live(true)
538 , m_state(Mir::RestoredState)
539 , m_orientationAngle(Mir::Angle0)
540- , m_viewCount(0)
541+ , m_visible(true)
542 , m_focused(false)
543 {}
544
545@@ -87,6 +88,8 @@
546
547 bool live() const override { return m_live; }
548
549+ bool visible() const override { return m_visible; }
550+
551 Mir::OrientationAngle orientationAngle() const override { return m_orientationAngle; }
552 void setOrientationAngle(Mir::OrientationAngle angle) override {
553 if (m_orientationAngle != angle) {
554@@ -116,18 +119,28 @@
555 }
556 }
557
558- bool isBeingDisplayed() const override { return m_viewCount > 0; }
559- void incrementViewCount() override {
560- ++m_viewCount;
561- if (m_viewCount == 1) {
562- Q_EMIT isBeingDisplayedChanged();
563- }
564- }
565- void decrementViewCount() override {
566- --m_viewCount;
567- if (m_viewCount == 0) {
568- Q_EMIT isBeingDisplayedChanged();
569- }
570+ void setViewVisibility(qintptr viewId, bool visible) override {
571+ if (!m_views.contains(viewId)) return;
572+
573+ m_views[viewId] = visible;
574+ updateVisibility();
575+ }
576+
577+ bool isBeingDisplayed() const override { return !m_views.isEmpty(); }
578+
579+ void registerView(qintptr viewId) override {
580+ m_views.insert(viewId, false);
581+ if (m_views.count() == 1) {
582+ Q_EMIT isBeingDisplayedChanged();
583+ }
584+ }
585+
586+ void unregisterView(qintptr viewId) override {
587+ m_views.remove(viewId);
588+ if (m_views.count() == 0) {
589+ Q_EMIT isBeingDisplayedChanged();
590+ }
591+ updateVisibility();
592 }
593
594 // methods called from the rendering (scene graph) thread:
595@@ -182,6 +195,20 @@
596 QList<TouchEvent> &touchesReceived() { return m_touchesReceived; }
597
598 private:
599+ void updateVisibility() {
600+ bool newVisible = false;
601+ QHashIterator<int, bool> i(m_views);
602+ while (i.hasNext()) {
603+ i.next();
604+ newVisible |= i.value();
605+ }
606+
607+ if (m_visible != newVisible) {
608+ m_visible = newVisible;
609+ Q_EMIT visibleChanged(newVisible);
610+ }
611+ }
612+
613
614 bool m_isFirstFrameDrawn;
615 SessionInterface *m_session;
616@@ -189,8 +216,9 @@
617 bool m_live;
618 Mir::State m_state;
619 Mir::OrientationAngle m_orientationAngle;
620+ bool m_visible;
621 QSize m_size;
622- int m_viewCount;
623+ QHash<int, bool> m_views;
624 bool m_focused;
625
626 QList<TouchEvent> m_touchesReceived;
627
628=== modified file 'tests/modules/common/fake_session.h'
629--- tests/modules/common/fake_session.h 2015-08-31 09:51:28 +0000
630+++ tests/modules/common/fake_session.h 2015-10-27 16:23:25 +0000
631@@ -28,6 +28,7 @@
632 public:
633 FakeSession()
634 : SessionInterface(0)
635+ , m_application(nullptr)
636 , m_state(Starting)
637 {
638 }

Subscribers

People subscribed via source and target branches