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

Proposed by Nick Dedekind on 2015-10-05
Status: Merged
Approved by: Daniel d'Andrada on 2015-10-20
Approved revision: 284
Merged at revision: 284
Proposed branch: lp:~nick-dedekind/qtubuntu/lp1475678.surface-occlude
Merge into: lp:qtubuntu
Diff against target: 151 lines (+48/-6)
3 files modified
src/ubuntumirclient/input.cpp (+7/-1)
src/ubuntumirclient/window.cpp (+39/-5)
src/ubuntumirclient/window.h (+2/-0)
To merge this branch: bzr merge lp:~nick-dedekind/qtubuntu/lp1475678.surface-occlude
Reviewer Review Type Date Requested Status
Nick Dedekind (community) Abstain on 2015-11-06
Daniel d'Andrada (community) 2015-10-05 Needs Fixing on 2015-11-05
PS Jenkins bot continuous-integration Approve on 2015-10-12
Review via email: mp+273424@code.launchpad.net

Commit message

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

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

In src/ubuntumirclient/window.cpp:

"""
+ window()->setVisible(exposed);
"""

You sure this is necessary?

It doesn't look right that a backend is calling the client-side API. Since, QWindow::visible property is under control of application code, it would be weird that it suddenly turn false without app code having explicitly set it (either via setVisible(false) or hide()).

review: Needs Information
284. By Nick Dedekind on 2015-10-12

better exposure handling

Nick Dedekind (nick-dedekind) wrote :

> In src/ubuntumirclient/window.cpp:
>
> """
> + window()->setVisible(exposed);
> """
>
> You sure this is necessary?
>
> It doesn't look right that a backend is calling the client-side API. Since,
> QWindow::visible property is under control of application code, it would be
> weird that it suddenly turn false without app code having explicitly set it
> (either via setVisible(false) or hide()).

Apparently not. Looks like it's the isExposed that stops the redraw.
I've fixed it up now.

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

Code looks ok. Didn't test yet.

review: Approve (code)
Daniel d'Andrada (dandrader) wrote :

And works fine.

review: Approve
review: Approve
285. By Nick Dedekind on 2015-10-26

merge with trunk

286. By Nick Dedekind on 2015-11-05

Exposure initialization

Daniel d'Andrada (dandrader) wrote :

something like pendingOcclusion or occlusionCatchup would be clearer than exposeCatchUp as this is meant specifically to occlude the window. Corner cases like this should be well explained.

review: Needs Fixing
Nick Dedekind (nick-dedekind) wrote :

> something like pendingOcclusion or occlusionCatchup would be clearer than
> exposeCatchUp as this is meant specifically to occlude the window. Corner
> cases like this should be well explained.

Done and added a comment explaining.

review: Disapprove
review: Abstain
Daniel d'Andrada (dandrader) wrote :

Thanks! But looks like it was too late. This last change didn't show up in the merged branch.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/ubuntumirclient/input.cpp'
2--- src/ubuntumirclient/input.cpp 2015-10-21 11:47:48 +0000
3+++ src/ubuntumirclient/input.cpp 2015-11-06 09:06:33 +0000
4@@ -232,9 +232,15 @@
5 case mir_event_type_surface:
6 {
7 auto surfaceEvent = mir_event_get_surface_event(nativeEvent);
8- if (mir_surface_event_get_attribute(surfaceEvent) == mir_surface_attrib_focus) {
9+ auto surfaceEventAttribute = mir_surface_event_get_attribute(surfaceEvent);
10+
11+ if (surfaceEventAttribute == mir_surface_attrib_focus) {
12 ubuntuEvent->window->handleSurfaceFocusChange(mir_surface_event_get_attribute_value(surfaceEvent) ==
13 mir_surface_focused);
14+
15+ } else if (surfaceEventAttribute == mir_surface_attrib_visibility) {
16+ ubuntuEvent->window->handleSurfaceExposeChange(mir_surface_event_get_attribute_value(surfaceEvent) ==
17+ mir_surface_visibility_exposed);
18 }
19 break;
20 }
21
22=== modified file 'src/ubuntumirclient/window.cpp'
23--- src/ubuntumirclient/window.cpp 2015-09-29 18:10:51 +0000
24+++ src/ubuntumirclient/window.cpp 2015-11-06 09:06:33 +0000
25@@ -99,7 +99,9 @@
26 QSize bufferSize;
27 QMutex mutex;
28 QSharedPointer<UbuntuClipboard> clipboard;
29+ bool exposed;
30 int resizeCatchUpAttempts;
31+ bool exposeCatchUp;
32 #if !defined(QT_NO_DEBUG)
33 int frameNumber;
34 #endif
35@@ -136,6 +138,8 @@
36 d->connection = connection;
37 d->clipboard = clipboard;
38 d->resizeCatchUpAttempts = 0;
39+ d->exposed = true;
40+ d->exposeCatchUp = false;
41
42 static int id = 1;
43 d->id = id++;
44@@ -283,6 +287,7 @@
45 // Create platform window
46 mir_wait_for(mir_surface_create(spec, surfaceCreateCallback, this));
47 mir_surface_spec_release(spec);
48+ d->exposeCatchUp = mir_surface_get_visibility(d->surface) == mir_surface_visibility_occluded;
49
50 DASSERT(d->surface != NULL);
51 d->createEGLSurface((EGLNativeWindowType)mir_buffer_stream_get_egl_native_window(mir_surface_get_buffer_stream(d->surface)));
52@@ -339,7 +344,7 @@
53 // swap more than once to get a buffer with the new size!
54 d->resizeCatchUpAttempts = 2;
55
56- QWindowSystemInterface::handleExposeEvent(window(), geometry());
57+ QWindowSystemInterface::handleExposeEvent(window(), d->exposed ? QRect(QPoint(), geometry().size()) : QRect());
58 QWindowSystemInterface::flushWindowSystemEvents();
59 }
60 }
61@@ -362,6 +367,19 @@
62 QWindowSystemInterface::handleWindowActivated(activatedWindow, Qt::ActiveWindowFocusReason);
63 }
64
65+void UbuntuWindow::handleSurfaceExposeChange(bool exposed)
66+{
67+ QMutexLocker(&d->mutex);
68+ DLOG("UbuntuWindow::handleSurfaceExposeChange(exposed=%s)", exposed ? "true" : "false");
69+
70+ if (d->exposed != exposed) {
71+ d->exposed = exposed;
72+
73+ QWindowSystemInterface::handleExposeEvent(window(), d->exposed ? QRect(QPoint(), geometry().size()) : QRect());
74+ QWindowSystemInterface::flushWindowSystemEvents();
75+ }
76+}
77+
78 void UbuntuWindow::setWindowState(Qt::WindowState state)
79 {
80 QMutexLocker(&d->mutex);
81@@ -399,14 +417,19 @@
82
83 if (visible) {
84 mir_wait_for(mir_surface_set_state(d->surface, qtWindowStateToMirSurfaceState(d->state)));
85-
86- QWindowSystemInterface::handleExposeEvent(window(), QRect());
87- QWindowSystemInterface::flushWindowSystemEvents();
88 } else {
89 // TODO: Use the new mir_surface_state_hidden state instead of mir_surface_state_minimized.
90 // Will have to change qtmir and unity8 for that.
91 mir_wait_for(mir_surface_set_state(d->surface, mir_surface_state_minimized));
92 }
93+
94+ QWindowSystemInterface::handleExposeEvent(window(), d->exposed ? QRect(QPoint(), geometry().size()) : QRect());
95+ QWindowSystemInterface::flushWindowSystemEvents();
96+}
97+
98+bool UbuntuWindow::isExposed() const
99+{
100+ return d->exposed && window()->isVisible();
101 }
102
103 void UbuntuWindow::setWindowTitle(const QString &title)
104@@ -437,6 +460,13 @@
105 ++d->frameNumber;
106 #endif
107
108+ bool exposureChanged = false;
109+ if (d->exposeCatchUp) {
110+ d->exposeCatchUp = false;
111+ d->exposed = false;
112+ exposureChanged = true;
113+ }
114+
115 if (sizeKnown && (d->bufferSize.width() != newBufferWidth ||
116 d->bufferSize.height() != newBufferHeight)) {
117 d->resizeCatchUpAttempts = 0;
118@@ -462,9 +492,13 @@
119 DLOG("UbuntuWindow::onBuffersSwapped_threadSafe [%d] - buffer size (%d,%d). Redrawing to catch up a resized buffer."
120 " resizeCatchUpAttempts=%d",
121 d->frameNumber, d->bufferSize.width(), d->bufferSize.height(), d->resizeCatchUpAttempts);
122- QWindowSystemInterface::handleExposeEvent(window(), geometry());
123+ QWindowSystemInterface::handleExposeEvent(window(), d->exposed ? QRect(QPoint(), geometry().size()) : QRect());
124 } else {
125 DLOG("UbuntuWindow::onBuffersSwapped_threadSafe [%d] - buffer size (%d,%d). resizeCatchUpAttempts=%d",
126 d->frameNumber, d->bufferSize.width(), d->bufferSize.height(), d->resizeCatchUpAttempts);
127+
128+ if (exposureChanged) {
129+ QWindowSystemInterface::handleExposeEvent(window(), d->exposed ? QRect(QPoint(), geometry().size()) : QRect());
130+ }
131 }
132 }
133
134=== modified file 'src/ubuntumirclient/window.h'
135--- src/ubuntumirclient/window.h 2015-09-29 18:10:51 +0000
136+++ src/ubuntumirclient/window.h 2015-11-06 09:06:33 +0000
137@@ -40,12 +40,14 @@
138 void setGeometry(const QRect&) override;
139 void setWindowState(Qt::WindowState state) override;
140 void setVisible(bool visible) override;
141+ bool isExposed() const override;
142 void setWindowTitle(const QString &title) override;
143
144 // New methods.
145 void* eglSurface() const;
146 void handleSurfaceResize(int width, int height);
147 void handleSurfaceFocusChange(bool focused);
148+ void handleSurfaceExposeChange(bool visible);
149 void onBuffersSwapped_threadSafe(int newBufferWidth, int newBufferHeight);
150
151 UbuntuWindowPrivate* priv() { return d; }

Subscribers

People subscribed via source and target branches