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

Proposed by Nick Dedekind
Status: Merged
Approved by: Daniel d'Andrada
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) Approve
PS Jenkins bot (community) continuous-integration Needs Fixing
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.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
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
380. By Nick Dedekind

Aggregate the surface visibility

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

381. By Nick Dedekind

only log on change

382. By Nick Dedekind

use isEmpty

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
383. By Nick Dedekind

use isEmpty

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

Please remove the cyclic dependency between MirSurfaceItem and MirSurface.

review: Needs Fixing
384. By Nick Dedekind

remove circ dep

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
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
Revision history for this message
Daniel d'Andrada (dandrader) wrote :

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

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

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

385. By Nick Dedekind

removed debug

Revision history for this message
Nick Dedekind (nick-dedekind) wrote :

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

Done.

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

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
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.

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

386. By Nick Dedekind

use qintptr for viewId

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

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
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
Revision history for this message
Daniel d'Andrada (dandrader) wrote :

Kept in MirSurface::unregisterView() I mean.

387. By Nick Dedekind

Re-added deleteLater

388. By Nick Dedekind

Added test for Surface deleteLater on deregister

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

389. By Nick Dedekind

added missing file

390. By Nick Dedekind

removed line

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

Code looks good. Didn't test yet.

review: Approve (code)
Revision history for this message
Daniel d'Andrada (dandrader) :
review: Approve
391. By Nick Dedekind

merged with trunk

392. By Nick Dedekind

merged with trunk

393. By Nick Dedekind

bump libunity-api version

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'debian/control'
--- debian/control 2015-10-21 11:46:54 +0000
+++ debian/control 2015-10-27 16:23:25 +0000
@@ -23,7 +23,7 @@
23 libubuntu-app-launch2-dev,23 libubuntu-app-launch2-dev,
24 libubuntu-application-api-dev (>= 2.1.0),24 libubuntu-application-api-dev (>= 2.1.0),
25 libudev-dev,25 libudev-dev,
26 libunity-api-dev (>= 7.101),26 libunity-api-dev (>= 7.102),
27 liburl-dispatcher1-dev,27 liburl-dispatcher1-dev,
28 libxkbcommon-dev,28 libxkbcommon-dev,
29 libxrender-dev,29 libxrender-dev,
3030
=== modified file 'src/modules/Unity/Application/mirsurface.cpp'
--- src/modules/Unity/Application/mirsurface.cpp 2015-10-21 11:47:03 +0000
+++ src/modules/Unity/Application/mirsurface.cpp 2015-10-27 16:23:25 +0000
@@ -176,7 +176,6 @@
176 , m_textureUpdated(false)176 , m_textureUpdated(false)
177 , m_currentFrameNumber(0)177 , m_currentFrameNumber(0)
178 , m_live(true)178 , m_live(true)
179 , m_viewCount(0)
180{179{
181 m_surfaceObserver = observer;180 m_surfaceObserver = observer;
182 if (observer) {181 if (observer) {
@@ -208,9 +207,9 @@
208207
209MirSurface::~MirSurface()208MirSurface::~MirSurface()
210{209{
211 qCDebug(QTMIR_SURFACES).nospace() << "MirSurface::~MirSurface this=" << this << " viewCount=" << m_viewCount;210 qCDebug(QTMIR_SURFACES).nospace() << "MirSurface::~MirSurface this=" << this << " viewCount=" << m_views.count();
212211
213 Q_ASSERT(m_viewCount == 0);212 Q_ASSERT(m_views.isEmpty());
214213
215 if (m_session) {214 if (m_session) {
216 m_session->setSurface(nullptr);215 m_session->setSurface(nullptr);
@@ -242,6 +241,9 @@
242 case mir_surface_attrib_state:241 case mir_surface_attrib_state:
243 Q_EMIT stateChanged(state());242 Q_EMIT stateChanged(state());
244 break;243 break;
244 case mir_surface_attrib_visibility:
245 Q_EMIT visibleChanged(visible());
246 break;
245 default:247 default:
246 break;248 break;
247 }249 }
@@ -555,6 +557,11 @@
555 return m_live;557 return m_live;
556}558}
557559
560bool MirSurface::visible() const
561{
562 return m_surface->query(mir_surface_attrib_visibility) == mir_surface_visibility_exposed;
563}
564
558void MirSurface::mousePressEvent(QMouseEvent *event)565void MirSurface::mousePressEvent(QMouseEvent *event)
559{566{
560 auto ev = makeMirEvent(event, mir_pointer_action_button_down);567 auto ev = makeMirEvent(event, mir_pointer_action_button_down);
@@ -638,29 +645,56 @@
638645
639bool MirSurface::isBeingDisplayed() const646bool MirSurface::isBeingDisplayed() const
640{647{
641 return m_viewCount > 0;648 return !m_views.isEmpty();
642}649}
643650
644void MirSurface::incrementViewCount()651void MirSurface::registerView(qintptr viewId)
645{652{
646 ++m_viewCount;653 m_views.insert(viewId, MirSurface::View{false});
647 qCDebug(QTMIR_SURFACES).nospace() << "MirSurface::incrementViewCount after=" << m_viewCount;654 qCDebug(QTMIR_SURFACES).nospace() << "MirSurface[" << appId() << "]::registerView(" << viewId << ")"
648 if (m_viewCount == 1) {655 << " after=" << m_views.count();
656 if (m_views.count() == 1) {
649 Q_EMIT isBeingDisplayedChanged();657 Q_EMIT isBeingDisplayedChanged();
650 }658 }
651}659}
652660
653void MirSurface::decrementViewCount()661void MirSurface::unregisterView(qintptr viewId)
654{662{
655 Q_ASSERT(m_viewCount > 0);663 qCDebug(QTMIR_SURFACES).nospace() << "MirSurface[" << appId() << "]::unregisterView(" << viewId << ")"
656 --m_viewCount;664 << " after=" << m_views.count() << " live=" << m_live;
657 qCDebug(QTMIR_SURFACES).nospace() << "MirSurface::decrementViewCount after=" << m_viewCount;665 m_views.remove(viewId);
658 if (m_viewCount == 0) {666 if (m_views.count() == 0) {
659 Q_EMIT isBeingDisplayedChanged();667 Q_EMIT isBeingDisplayedChanged();
660 if (m_session.isNull() || !m_live) {668 if (m_session.isNull() || !m_live) {
661 deleteLater();669 deleteLater();
662 }670 }
663 }671 }
672 updateVisibility();
673}
674
675void MirSurface::setViewVisibility(qintptr viewId, bool visible)
676{
677 if (!m_views.contains(viewId)) return;
678
679 m_views[viewId].visible = visible;
680 updateVisibility();
681}
682
683void MirSurface::updateVisibility()
684{
685 bool newVisible = false;
686 QHashIterator<qintptr, View> i(m_views);
687 while (i.hasNext()) {
688 i.next();
689 newVisible |= i.value().visible;
690 }
691
692 if (newVisible != visible()) {
693 qCDebug(QTMIR_SURFACES).nospace() << "MirSurface[" << appId() << "]::updateVisibility(" << newVisible << ")";
694
695 m_surface->configure(mir_surface_attrib_visibility,
696 newVisible ? mir_surface_visibility_exposed : mir_surface_visibility_occluded);
697 }
664}698}
665699
666unsigned int MirSurface::currentFrameNumber() const700unsigned int MirSurface::currentFrameNumber() const
@@ -670,7 +704,7 @@
670704
671void MirSurface::onSessionDestroyed()705void MirSurface::onSessionDestroyed()
672{706{
673 if (m_viewCount == 0) {707 if (m_views.isEmpty()) {
674 deleteLater();708 deleteLater();
675 }709 }
676}710}
677711
=== modified file 'src/modules/Unity/Application/mirsurface.h'
--- src/modules/Unity/Application/mirsurface.h 2015-10-21 11:47:03 +0000
+++ src/modules/Unity/Application/mirsurface.h 2015-10-27 16:23:25 +0000
@@ -67,6 +67,8 @@
6767
68 bool live() const override;68 bool live() const override;
6969
70 bool visible() const override;
71
70 Mir::OrientationAngle orientationAngle() const override;72 Mir::OrientationAngle orientationAngle() const override;
71 void setOrientationAngle(Mir::OrientationAngle angle) override;73 void setOrientationAngle(Mir::OrientationAngle angle) override;
7274
@@ -81,8 +83,10 @@
81 void startFrameDropper() override;83 void startFrameDropper() override;
8284
83 bool isBeingDisplayed() const override;85 bool isBeingDisplayed() const override;
84 void incrementViewCount() override;86
85 void decrementViewCount() override;87 void registerView(qintptr viewId) override;
88 void unregisterView(qintptr viewId) override;
89 void setViewVisibility(qintptr viewId, bool visible) override;
8690
87 // methods called from the rendering (scene graph) thread:91 // methods called from the rendering (scene graph) thread:
88 QSharedPointer<QSGTexture> texture() override;92 QSharedPointer<QSGTexture> texture() override;
@@ -125,6 +129,7 @@
125private:129private:
126 void syncSurfaceSizeWithItemSize();130 void syncSurfaceSizeWithItemSize();
127 bool clientIsRunning() const;131 bool clientIsRunning() const;
132 void updateVisibility();
128133
129 std::shared_ptr<mir::scene::Surface> m_surface;134 std::shared_ptr<mir::scene::Surface> m_surface;
130 QPointer<SessionInterface> m_session;135 QPointer<SessionInterface> m_session;
@@ -144,7 +149,10 @@
144 unsigned int m_currentFrameNumber;149 unsigned int m_currentFrameNumber;
145150
146 bool m_live;151 bool m_live;
147 int m_viewCount;152 struct View {
153 bool visible;
154 };
155 QHash<qintptr, View> m_views;
148156
149 std::shared_ptr<SurfaceObserver> m_surfaceObserver;157 std::shared_ptr<SurfaceObserver> m_surfaceObserver;
150158
151159
=== modified file 'src/modules/Unity/Application/mirsurfaceinterface.h'
--- src/modules/Unity/Application/mirsurfaceinterface.h 2015-10-13 18:29:13 +0000
+++ src/modules/Unity/Application/mirsurfaceinterface.h 2015-10-27 16:23:25 +0000
@@ -48,8 +48,10 @@
48 virtual void startFrameDropper() = 0;48 virtual void startFrameDropper() = 0;
4949
50 virtual bool isBeingDisplayed() const = 0;50 virtual bool isBeingDisplayed() const = 0;
51 virtual void incrementViewCount() = 0;51
52 virtual void decrementViewCount() = 0;52 virtual void registerView(qintptr viewId) = 0;
53 virtual void unregisterView(qintptr viewId) = 0;
54 virtual void setViewVisibility(qintptr viewId, bool visible) = 0;
5355
54 // methods called from the rendering (scene graph) thread:56 // methods called from the rendering (scene graph) thread:
55 virtual QSharedPointer<QSGTexture> texture() = 0;57 virtual QSharedPointer<QSGTexture> texture() = 0;
5658
=== modified file 'src/modules/Unity/Application/mirsurfaceitem.cpp'
--- src/modules/Unity/Application/mirsurfaceitem.cpp 2015-10-13 18:29:13 +0000
+++ src/modules/Unity/Application/mirsurfaceitem.cpp 2015-10-27 16:23:25 +0000
@@ -106,6 +106,7 @@
106 connect(&m_updateMirSurfaceSizeTimer, &QTimer::timeout, this, &MirSurfaceItem::updateMirSurfaceSize);106 connect(&m_updateMirSurfaceSizeTimer, &QTimer::timeout, this, &MirSurfaceItem::updateMirSurfaceSize);
107107
108 connect(this, &QQuickItem::activeFocusChanged, this, &MirSurfaceItem::updateMirSurfaceFocus);108 connect(this, &QQuickItem::activeFocusChanged, this, &MirSurfaceItem::updateMirSurfaceFocus);
109 connect(this, &QQuickItem::visibleChanged, this, &MirSurfaceItem::updateMirSurfaceVisibility);
109}110}
110111
111MirSurfaceItem::~MirSurfaceItem()112MirSurfaceItem::~MirSurfaceItem()
@@ -524,6 +525,15 @@
524 m_surface->resize(width, height);525 m_surface->resize(width, height);
525}526}
526527
528void MirSurfaceItem::updateMirSurfaceVisibility()
529{
530 if (!m_surface || !m_surface->live()) {
531 return;
532 }
533
534 m_surface->setViewVisibility((qintptr)this, isVisible());
535}
536
527void MirSurfaceItem::updateMirSurfaceFocus(bool focused)537void MirSurfaceItem::updateMirSurfaceFocus(bool focused)
528{538{
529 if (m_surface && m_consumesInput && m_surface->live()) {539 if (m_surface && m_consumesInput && m_surface->live()) {
@@ -603,7 +613,7 @@
603 m_surface->setFocus(false);613 m_surface->setFocus(false);
604 }614 }
605615
606 m_surface->decrementViewCount();616 m_surface->unregisterView((qintptr)this);
607617
608 if (!m_surface->isBeingDisplayed() && window()) {618 if (!m_surface->isBeingDisplayed() && window()) {
609 disconnect(window(), nullptr, m_surface, nullptr);619 disconnect(window(), nullptr, m_surface, nullptr);
@@ -613,7 +623,7 @@
613 m_surface = surface;623 m_surface = surface;
614624
615 if (m_surface) {625 if (m_surface) {
616 m_surface->incrementViewCount();626 m_surface->registerView((qintptr)this);
617627
618 // When a new mir frame gets posted we notify the QML engine that this item needs redrawing,628 // When a new mir frame gets posted we notify the QML engine that this item needs redrawing,
619 // schedules call to updatePaintNode() from the rendering thread629 // schedules call to updatePaintNode() from the rendering thread
@@ -623,8 +633,10 @@
623 connect(m_surface, &MirSurfaceInterface::liveChanged, this, &MirSurfaceItem::liveChanged);633 connect(m_surface, &MirSurfaceInterface::liveChanged, this, &MirSurfaceItem::liveChanged);
624 connect(m_surface, &MirSurfaceInterface::sizeChanged, this, &MirSurfaceItem::onActualSurfaceSizeChanged);634 connect(m_surface, &MirSurfaceInterface::sizeChanged, this, &MirSurfaceItem::onActualSurfaceSizeChanged);
625635
626 connect(window(), &QQuickWindow::frameSwapped, m_surface, &MirSurfaceInterface::onCompositorSwappedBuffers,636 if (window()) {
627 (Qt::ConnectionType) (Qt::DirectConnection | Qt::UniqueConnection));637 connect(window(), &QQuickWindow::frameSwapped, m_surface, &MirSurfaceInterface::onCompositorSwappedBuffers,
638 (Qt::ConnectionType) (Qt::DirectConnection | Qt::UniqueConnection));
639 }
628640
629 Q_EMIT typeChanged(m_surface->type());641 Q_EMIT typeChanged(m_surface->type());
630 Q_EMIT liveChanged(true);642 Q_EMIT liveChanged(true);
@@ -632,6 +644,7 @@
632644
633 updateMirSurfaceSize();645 updateMirSurfaceSize();
634 setImplicitSize(m_surface->size().width(), m_surface->size().height());646 setImplicitSize(m_surface->size().width(), m_surface->size().height());
647 updateMirSurfaceVisibility();
635648
636 if (m_orientationAngle) {649 if (m_orientationAngle) {
637 m_surface->setOrientationAngle(*m_orientationAngle);650 m_surface->setOrientationAngle(*m_orientationAngle);
638651
=== modified file 'src/modules/Unity/Application/mirsurfaceitem.h'
--- src/modules/Unity/Application/mirsurfaceitem.h 2015-09-18 20:11:50 +0000
+++ src/modules/Unity/Application/mirsurfaceitem.h 2015-10-27 16:23:25 +0000
@@ -112,6 +112,7 @@
112 void updateMirSurfaceSize();112 void updateMirSurfaceSize();
113113
114 void updateMirSurfaceFocus(bool focused);114 void updateMirSurfaceFocus(bool focused);
115 void updateMirSurfaceVisibility();
115116
116 void onActualSurfaceSizeChanged(const QSize &size);117 void onActualSurfaceSizeChanged(const QSize &size);
117118
118119
=== modified file 'tests/modules/CMakeLists.txt'
--- tests/modules/CMakeLists.txt 2015-01-09 11:25:13 +0000
+++ tests/modules/CMakeLists.txt 2015-10-27 16:23:25 +0000
@@ -2,7 +2,7 @@
2add_subdirectory(ApplicationManager)2add_subdirectory(ApplicationManager)
3add_subdirectory(DesktopFileReader)3add_subdirectory(DesktopFileReader)
4add_subdirectory(General)4add_subdirectory(General)
5add_subdirectory(MirSurfaceItem)5add_subdirectory(SharedWakelock)
6add_subdirectory(SessionManager)6add_subdirectory(SessionManager)
7add_subdirectory(SharedWakelock)7add_subdirectory(SurfaceManager)
8add_subdirectory(TaskController)8add_subdirectory(TaskController)
99
=== renamed directory 'tests/modules/MirSurfaceItem' => 'tests/modules/SurfaceManager'
=== modified file 'tests/modules/SurfaceManager/CMakeLists.txt'
--- tests/modules/MirSurfaceItem/CMakeLists.txt 2015-08-31 09:51:28 +0000
+++ tests/modules/SurfaceManager/CMakeLists.txt 2015-10-27 16:23:25 +0000
@@ -1,8 +1,10 @@
1set(1set(
2 MIR_SURFACE_ITEM_TEST_SOURCES2 MIR_SURFACE_MANAGER_TEST_SOURCES
3 mirsurfaceitem_test.cpp3 mirsurfaceitem_test.cpp
4 mirsurface_test.cpp
4 ${CMAKE_SOURCE_DIR}/src/common/debughelpers.cpp5 ${CMAKE_SOURCE_DIR}/src/common/debughelpers.cpp
5 ${CMAKE_SOURCE_DIR}/tests/modules/common/fake_mirsurface.h6 ${CMAKE_SOURCE_DIR}/tests/modules/common/fake_mirsurface.h
7 ${CMAKE_SOURCE_DIR}/tests/modules/common/fake_session.h
6)8)
79
8include_directories(10include_directories(
@@ -11,10 +13,10 @@
11 ${MIRSERVER_INCLUDE_DIRS}13 ${MIRSERVER_INCLUDE_DIRS}
12)14)
1315
14add_executable(mirsurfaceitem_test ${MIR_SURFACE_ITEM_TEST_SOURCES})16add_executable(surfacemanager_test ${MIR_SURFACE_MANAGER_TEST_SOURCES})
1517
16target_link_libraries(18target_link_libraries(
17 mirsurfaceitem_test19 surfacemanager_test
1820
19 qpa-mirserver21 qpa-mirserver
20 unityapplicationplugin22 unityapplicationplugin
@@ -25,4 +27,4 @@
25 ${GMOCK_LIBRARIES}27 ${GMOCK_LIBRARIES}
26)28)
2729
28add_test(MirSurfaceItem mirsurfaceitem_test)30add_test(SurfaceManager surfacemanager_test)
2931
=== added file 'tests/modules/SurfaceManager/mirsurface_test.cpp'
--- tests/modules/SurfaceManager/mirsurface_test.cpp 1970-01-01 00:00:00 +0000
+++ tests/modules/SurfaceManager/mirsurface_test.cpp 2015-10-27 16:23:25 +0000
@@ -0,0 +1,107 @@
1/*
2 * Copyright (C) 2014-2015 Canonical, Ltd.
3 *
4 * This program is free software: you can redistribute it and/or modify it under
5 * the terms of the GNU Lesser General Public License version 3, as published by
6 * the Free Software Foundation.
7 *
8 * This program is distributed in the hope that it will be useful, but WITHOUT
9 * ANY WARRANTY; without even the implied warranties of MERCHANTABILITY,
10 * SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
11 * Lesser General Public License for more details.
12 *
13 * You should have received a copy of the GNU Lesser General Public License
14 * along with this program. If not, see <http://www.gnu.org/licenses/>.
15 */
16
17#define MIR_INCLUDE_DEPRECATED_EVENT_HEADER
18
19#include <gtest/gtest.h>
20
21#include <QLoggingCategory>
22#include <QTest>
23
24// the test subject
25#include <Unity/Application/mirsurface.h>
26
27// tests/modules/common
28#include <fake_session.h>
29#include <mock_surface.h>
30
31using namespace qtmir;
32
33namespace ms = mir::scene;
34
35class MirSurfaceTest : public ::testing::Test
36{
37public:
38 MirSurfaceTest()
39 {
40 // We don't want the logging spam cluttering the test results
41 QLoggingCategory::setFilterRules(QStringLiteral("qtmir.surfaces=false"));
42 }
43};
44
45TEST_F(MirSurfaceTest, DeleteMirSurfaceOnLastNonLiveUnregisterView)
46{
47 using namespace testing;
48
49 int argc = 0;
50 char* argv[0];
51 QCoreApplication qtApp(argc, argv); // app for deleteLater event
52
53 auto fakeSession = new FakeSession();
54 auto mockSurface = std::make_shared<NiceMock<ms::MockSurface>>();
55
56 MirSurface *surface = new MirSurface(mockSurface, fakeSession, nullptr, nullptr);
57 bool surfaceDeleted = false;
58 QObject::connect(surface, &QObject::destroyed, surface, [&surfaceDeleted](){ surfaceDeleted = true; });
59
60 qintptr view1 = (qintptr)1;
61 qintptr view2 = (qintptr)2;
62
63 surface->registerView(view1);
64 surface->registerView(view2);
65 surface->setLive(false);
66 EXPECT_FALSE(surfaceDeleted);
67
68 surface->unregisterView(view1);
69 surface->unregisterView(view2);
70
71 QCoreApplication::sendPostedEvents(0, QEvent::DeferredDelete);
72 EXPECT_TRUE(surfaceDeleted);
73
74 delete fakeSession;
75}
76
77
78TEST_F(MirSurfaceTest, DoNotDeleteMirSurfaceOnLastLiveUnregisterView)
79{
80 using namespace testing;
81
82 int argc = 0;
83 char* argv[0];
84 QCoreApplication qtApp(argc, argv); // app for deleteLater event
85
86 auto fakeSession = new FakeSession();
87 auto mockSurface = std::make_shared<NiceMock<ms::MockSurface>>();
88
89 MirSurface *surface = new MirSurface(mockSurface, fakeSession, nullptr, nullptr);
90 bool surfaceDeleted = false;
91 QObject::connect(surface, &QObject::destroyed, surface, [&surfaceDeleted](){ surfaceDeleted = true; });
92
93 qintptr view1 = (qintptr)1;
94 qintptr view2 = (qintptr)2;
95
96 surface->registerView(view1);
97 surface->registerView(view2);
98
99 surface->unregisterView(view1);
100 surface->unregisterView(view2);
101
102 QCoreApplication::sendPostedEvents(0, QEvent::DeferredDelete);
103 EXPECT_FALSE(surfaceDeleted);
104
105 delete fakeSession;
106 delete surface;
107}
0108
=== modified file 'tests/modules/SurfaceManager/mirsurfaceitem_test.cpp'
--- tests/modules/MirSurfaceItem/mirsurfaceitem_test.cpp 2015-08-31 09:51:28 +0000
+++ tests/modules/SurfaceManager/mirsurfaceitem_test.cpp 2015-10-27 16:23:25 +0000
@@ -29,22 +29,28 @@
2929
30using namespace qtmir;30using namespace qtmir;
3131
32class MirSurfaceItemTest : public ::testing::Test
33{
34public:
35 MirSurfaceItemTest()
36 {
37 // We don't want the logging spam cluttering the test results
38 QLoggingCategory::setFilterRules(QStringLiteral("qtmir.surfaces=false"));
39 }
40};
41
32/*42/*
33 Tests that even if Qt fails to finish a touch sequence, MirSurfaceItem will43 Tests that even if Qt fails to finish a touch sequence, MirSurfaceItem will
34 properly finish it when forwarding it to its mir::input::surface. So44 properly finish it when forwarding it to its mir::input::surface. So
35 mir::input::surface will still consume a proper sequence of touch events45 mir::input::surface will still consume a proper sequence of touch events
36 (comprised of a begin, zero or more updates and an end).46 (comprised of a begin, zero or more updates and an end).
37 */47 */
38TEST(MirSurfaceItemTest, MissingTouchEnd)48TEST_F(MirSurfaceItemTest, MissingTouchEnd)
39{49{
40 // We don't want the logging spam cluttering the test results
41 QLoggingCategory::setFilterRules(QStringLiteral("qtmir*=false"));
42
43 MirSurfaceItem *surfaceItem = new MirSurfaceItem;50 MirSurfaceItem *surfaceItem = new MirSurfaceItem;
4451
45 FakeMirSurface *fakeSurface = new FakeMirSurface;52 FakeMirSurface *fakeSurface = new FakeMirSurface;
4653
47
48 surfaceItem->setSurface(fakeSurface);54 surfaceItem->setSurface(fakeSurface);
49 surfaceItem->setConsumesInput(true);55 surfaceItem->setConsumesInput(true);
5056
@@ -93,3 +99,46 @@
93 delete surfaceItem;99 delete surfaceItem;
94 delete fakeSurface;100 delete fakeSurface;
95}101}
102
103TEST_F(MirSurfaceItemTest, SetSurfaceInitializesVisiblity)
104{
105 MirSurfaceItem *surfaceItem = new MirSurfaceItem;
106 surfaceItem->setVisible(false);
107
108 FakeMirSurface *fakeSurface = new FakeMirSurface;
109 surfaceItem->setSurface(fakeSurface);
110
111 EXPECT_FALSE(fakeSurface->visible());
112
113 delete surfaceItem;
114 delete fakeSurface;
115}
116
117TEST_F(MirSurfaceItemTest, AggregateSurfaceVisibility)
118{
119 MirSurfaceItem *surfaceItem1 = new MirSurfaceItem;
120 surfaceItem1->setVisible(true);
121 MirSurfaceItem *surfaceItem2 = new MirSurfaceItem;
122 surfaceItem1->setVisible(true);
123
124 FakeMirSurface *fakeSurface = new FakeMirSurface;
125 surfaceItem1->setSurface(fakeSurface);
126 surfaceItem2->setSurface(fakeSurface);
127
128 EXPECT_TRUE(fakeSurface->visible());
129
130 surfaceItem1->setVisible(false);
131 EXPECT_TRUE(fakeSurface->visible());
132
133 surfaceItem2->setVisible(false);
134 EXPECT_FALSE(fakeSurface->visible());
135
136 surfaceItem1->setVisible(true);
137 EXPECT_TRUE(fakeSurface->visible());
138
139 delete surfaceItem1;
140 EXPECT_FALSE(fakeSurface->visible());
141
142 delete surfaceItem2;
143 delete fakeSurface;
144}
96145
=== modified file 'tests/modules/common/fake_mirsurface.h'
--- tests/modules/common/fake_mirsurface.h 2015-10-13 18:29:13 +0000
+++ tests/modules/common/fake_mirsurface.h 2015-10-27 16:23:25 +0000
@@ -21,6 +21,7 @@
2121
22#include <QSharedPointer>22#include <QSharedPointer>
23#include <QSGTexture>23#include <QSGTexture>
24#include <QPointer>
2425
25namespace qtmir {26namespace qtmir {
2627
@@ -56,7 +57,7 @@
56 , m_live(true)57 , m_live(true)
57 , m_state(Mir::RestoredState)58 , m_state(Mir::RestoredState)
58 , m_orientationAngle(Mir::Angle0)59 , m_orientationAngle(Mir::Angle0)
59 , m_viewCount(0)60 , m_visible(true)
60 , m_focused(false)61 , m_focused(false)
61 {}62 {}
6263
@@ -87,6 +88,8 @@
8788
88 bool live() const override { return m_live; }89 bool live() const override { return m_live; }
8990
91 bool visible() const override { return m_visible; }
92
90 Mir::OrientationAngle orientationAngle() const override { return m_orientationAngle; }93 Mir::OrientationAngle orientationAngle() const override { return m_orientationAngle; }
91 void setOrientationAngle(Mir::OrientationAngle angle) override {94 void setOrientationAngle(Mir::OrientationAngle angle) override {
92 if (m_orientationAngle != angle) {95 if (m_orientationAngle != angle) {
@@ -116,18 +119,28 @@
116 }119 }
117 }120 }
118121
119 bool isBeingDisplayed() const override { return m_viewCount > 0; }122 void setViewVisibility(qintptr viewId, bool visible) override {
120 void incrementViewCount() override {123 if (!m_views.contains(viewId)) return;
121 ++m_viewCount;124
122 if (m_viewCount == 1) {125 m_views[viewId] = visible;
123 Q_EMIT isBeingDisplayedChanged();126 updateVisibility();
124 }127 }
125 }128
126 void decrementViewCount() override {129 bool isBeingDisplayed() const override { return !m_views.isEmpty(); }
127 --m_viewCount;130
128 if (m_viewCount == 0) {131 void registerView(qintptr viewId) override {
129 Q_EMIT isBeingDisplayedChanged();132 m_views.insert(viewId, false);
130 }133 if (m_views.count() == 1) {
134 Q_EMIT isBeingDisplayedChanged();
135 }
136 }
137
138 void unregisterView(qintptr viewId) override {
139 m_views.remove(viewId);
140 if (m_views.count() == 0) {
141 Q_EMIT isBeingDisplayedChanged();
142 }
143 updateVisibility();
131 }144 }
132145
133 // methods called from the rendering (scene graph) thread:146 // methods called from the rendering (scene graph) thread:
@@ -182,6 +195,20 @@
182 QList<TouchEvent> &touchesReceived() { return m_touchesReceived; }195 QList<TouchEvent> &touchesReceived() { return m_touchesReceived; }
183196
184private:197private:
198 void updateVisibility() {
199 bool newVisible = false;
200 QHashIterator<int, bool> i(m_views);
201 while (i.hasNext()) {
202 i.next();
203 newVisible |= i.value();
204 }
205
206 if (m_visible != newVisible) {
207 m_visible = newVisible;
208 Q_EMIT visibleChanged(newVisible);
209 }
210 }
211
185212
186 bool m_isFirstFrameDrawn;213 bool m_isFirstFrameDrawn;
187 SessionInterface *m_session;214 SessionInterface *m_session;
@@ -189,8 +216,9 @@
189 bool m_live;216 bool m_live;
190 Mir::State m_state;217 Mir::State m_state;
191 Mir::OrientationAngle m_orientationAngle;218 Mir::OrientationAngle m_orientationAngle;
219 bool m_visible;
192 QSize m_size;220 QSize m_size;
193 int m_viewCount;221 QHash<int, bool> m_views;
194 bool m_focused;222 bool m_focused;
195223
196 QList<TouchEvent> m_touchesReceived;224 QList<TouchEvent> m_touchesReceived;
197225
=== modified file 'tests/modules/common/fake_session.h'
--- tests/modules/common/fake_session.h 2015-08-31 09:51:28 +0000
+++ tests/modules/common/fake_session.h 2015-10-27 16:23:25 +0000
@@ -28,6 +28,7 @@
28public:28public:
29 FakeSession()29 FakeSession()
30 : SessionInterface(0)30 : SessionInterface(0)
31 , m_application(nullptr)
31 , m_state(Starting)32 , m_state(Starting)
32 {33 {
33 }34 }

Subscribers

People subscribed via source and target branches