Merge lp:~agateau/unity-2d/reason-fv into lp:unity-2d/3.0

Proposed by Aurélien Gâteau
Status: Work in progress
Proposed branch: lp:~agateau/unity-2d/reason-fv
Merge into: lp:unity-2d/3.0
Diff against target: 396 lines (+91/-57)
12 files modified
launcher/Launcher.qml (+2/-2)
launcher/LauncherList.qml (+6/-6)
launcher/app/launcher.xml (+11/-0)
launcher/app/launcherdbus.cpp (+4/-4)
launcher/app/launcherdbus.h (+2/-2)
launcher/app/visibilitycontroller.cpp (+40/-24)
launcher/app/visibilitycontroller.h (+12/-6)
libunity-2d-private/src/launcherclient.cpp (+6/-5)
libunity-2d-private/src/launcherclient.h (+2/-2)
panel/applets/homebutton/homebuttonapplet.cpp (+2/-2)
places/app/dashdeclarativeview.cpp (+2/-2)
spread/app/spreadcontrol.cpp (+2/-2)
To merge this branch: bzr merge lp:~agateau/unity-2d/reason-fv
Reviewer Review Type Date Requested Status
Ugo Riboni (community) Needs Fixing
Review via email: mp+54371@code.launchpad.net

Commit message

[launcher] replaces the refcount method to track forced visibility requests with a "reason-based" method

Every call to beginForceVisible() must be passed a reason, and the same reason must be passed back to endForceVisible().
Also introduces an environment variable to help debugging visibility issues: UNITY2DLAUNCHER_DEBUG_VISIBILITY.

Description of the change

This branch replaces the refcount method to track forced visibility requests with a "reason-based" method: every call to beginForceVisible() must be passed a reason, and the same reason must be passed back to endForceVisible().

It also introduces an environment variable to help debugging visibility issues: UNITY2DLAUNCHER_DEBUG_VISIBILITY.

To post a comment you must log in.
Ugo Riboni (uriboni) wrote :

61 + <arg type="s" name="reason">
62 + <dox:d>Reason for ending the visibility forcing. Must match
63 + with a previous call to BeginForceVisible</dox:d>
64 + </arg>

Please make sure to mention that you just need one call to terminate all outstanding requests for FV for that specific reason for that process.
Would be nice to say that in the commit message as well.

145 + m_forceVisibleReasonHash[service].insert(reason);
146 + if (!service.isEmpty()) {
147 + m_dbusWatcher->addWatchedService(service);
148 + }

What would be the reason for service to be emtpy ? If it's indeed possible, please move the hash insert into the if, otherwise if it's not expected to happen I suggest you put an assert or a critical error message.

163 + if (m_forceVisibleReasonHash.contains(service)) {
164 + m_forceVisibleReasonHash[service].remove(reason);
165 + if (m_forceVisibleReasonHash[service].isEmpty()) {
166 + m_forceVisibleReasonHash.remove(service);

Why do you check if m_forceVisibleReasonHash[service] is emtpy instead of checking if service is empty like you do in the beginFV method ?
This way you're actually checking if the reason is empty, which i don't think is what you want.

review: Needs Fixing
lp:~agateau/unity-2d/reason-fv updated
468. By Aurélien Gâteau on 2011-03-23

- Document tricky parts of the code
- Avoid asking QDBusServiceWatcher to monitor an empty service (harmless but
  stupid)

Aurélien Gâteau (agateau) wrote :

Addressed all your questions with comments explaining the tricky parts of the code as well as more doc in the DBus API.

lp:~agateau/unity-2d/reason-fv updated
469. By Aurélien Gâteau on 2011-03-23

Merged from trunk

470. By Aurélien Gâteau on 2011-05-18

Merged with trunk

Unmerged revisions

470. By Aurélien Gâteau on 2011-05-18

Merged with trunk

469. By Aurélien Gâteau on 2011-03-23

Merged from trunk

468. By Aurélien Gâteau on 2011-03-23

- Document tricky parts of the code
- Avoid asking QDBusServiceWatcher to monitor an empty service (harmless but
  stupid)

467. By Aurélien Gâteau on 2011-03-22

Log behavior class when switching behaviors

466. By Aurélien Gâteau on 2011-03-22

Use explicit reasons instead of refcounts to implement forced visibility

This is more robust and makes it easier to debug as one only needs to set the
UNITY2DLAUNCHER_DEBUG_VISIBILITY environment variable to get a log of the
active reasons for forced visibility.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'launcher/Launcher.qml'
2--- launcher/Launcher.qml 2011-05-11 10:29:54 +0000
3+++ launcher/Launcher.qml 2011-05-18 16:23:29 +0000
4@@ -122,8 +122,8 @@
5 else visibilityController.endForceVisible()
6 }
7 onFocusChanged: {
8- if (focus) visibilityController.beginForceVisible()
9- else visibilityController.endForceVisible()
10+ if (focus) visibilityController.beginForceVisible("launcher-focus")
11+ else visibilityController.endForceVisible("launcher-focus")
12 }
13 }
14
15
16=== modified file 'launcher/LauncherList.qml'
17--- launcher/LauncherList.qml 2011-05-02 16:25:49 +0000
18+++ launcher/LauncherList.qml 2011-05-18 16:23:29 +0000
19@@ -222,9 +222,9 @@
20 target: item.menu
21 onFoldedChanged: {
22 if (item.menu.folded) {
23- visibilityController.endForceVisible();
24+ visibilityController.endForceVisible("launcher-menu");
25 } else {
26- visibilityController.beginForceVisible();
27+ visibilityController.beginForceVisible("launcher-menu");
28 }
29 }
30 }
31@@ -277,19 +277,19 @@
32 target: urgentAnimation
33 onRunningChanged: {
34 if (urgentAnimation.running) {
35- visibilityController.beginForceVisible()
36+ visibilityController.beginForceVisible("launcher-urgent");
37 } else {
38- visibilityController.endForceVisible()
39+ visibilityController.endForceVisible("launcher-urgent");
40 }
41 }
42 Component.onCompleted: {
43 if (urgentAnimation.running) {
44- visibilityController.beginForceVisible()
45+ visibilityController.beginForceVisible("launcher-urgent")
46 }
47 }
48 Component.onDestruction: {
49 if (urgentAnimation.running) {
50- visibilityController.endForceVisible()
51+ visibilityController.endForceVisible("launcher-urgent")
52 }
53 }
54 }
55
56=== modified file 'launcher/app/launcher.xml'
57--- launcher/app/launcher.xml 2011-03-22 11:30:53 +0000
58+++ launcher/app/launcher.xml 2011-05-18 16:23:29 +0000
59@@ -23,13 +23,24 @@
60 EndForceVisible() when you don't need the launcher to stay
61 visible anymore.
62 ]]></dox:d>
63+ <arg type="s" name="reason">
64+ <dox:d>Reason for forcing visibility</dox:d>
65+ </arg>
66 </method>
67 <method name="EndForceVisible">
68 <dox:d><![CDATA[
69 Tells the launcher you don't need it to stay visible anymore.
70 Please note this does not mean the launcher will necessarily
71 hide.
72+
73+ One call to EndForceVisible() will stop all previous calls to
74+ BeginForceVisible() coming from the same application with the
75+ same reason.
76 ]]></dox:d>
77+ <arg type="s" name="reason">
78+ <dox:d>Reason for ending the visibility forcing. Must match
79+ with a previous call to BeginForceVisible</dox:d>
80+ </arg>
81 </method>
82 </interface>
83 </node>
84
85=== modified file 'launcher/app/launcherdbus.cpp'
86--- launcher/app/launcherdbus.cpp 2011-03-22 11:30:53 +0000
87+++ launcher/app/launcherdbus.cpp 2011-05-18 16:23:29 +0000
88@@ -65,15 +65,15 @@
89 }
90
91 void
92-LauncherDBus::BeginForceVisible()
93+LauncherDBus::BeginForceVisible(const QString& reason)
94 {
95 UQ_RETURN_IF_FAIL(calledFromDBus());
96- m_visibilityController->beginForceVisible(message().service());
97+ m_visibilityController->beginForceVisible(reason, message().service());
98 }
99
100 void
101-LauncherDBus::EndForceVisible()
102+LauncherDBus::EndForceVisible(const QString& reason)
103 {
104 UQ_RETURN_IF_FAIL(calledFromDBus());
105- m_visibilityController->endForceVisible(message().service());
106+ m_visibilityController->endForceVisible(reason, message().service());
107 }
108
109=== modified file 'launcher/app/launcherdbus.h'
110--- launcher/app/launcherdbus.h 2011-03-22 11:30:53 +0000
111+++ launcher/app/launcherdbus.h 2011-05-18 16:23:29 +0000
112@@ -43,8 +43,8 @@
113 bool connectToBus();
114
115 public Q_SLOTS:
116- Q_NOREPLY void BeginForceVisible();
117- Q_NOREPLY void EndForceVisible();
118+ Q_NOREPLY void BeginForceVisible(const QString& reason);
119+ Q_NOREPLY void EndForceVisible(const QString& reason);
120 Q_NOREPLY void AddWebFavorite(const QString& url);
121
122 private:
123
124=== modified file 'launcher/app/visibilitycontroller.cpp'
125--- launcher/app/visibilitycontroller.cpp 2011-03-22 11:30:53 +0000
126+++ launcher/app/visibilitycontroller.cpp 2011-05-18 16:23:29 +0000
127@@ -39,6 +39,8 @@
128
129 static const char* GCONF_LAUNCHER_HIDEMODE_KEY = "/desktop/unity-2d/launcher/hide_mode";
130
131+static const bool DEBUG_VISIBILITY = getenv("UNITY2DLAUNCHER_DEBUG_VISIBILITY") != 0;
132+
133 VisibilityController::VisibilityController(Unity2dPanel* panel)
134 : QObject(panel)
135 , m_panel(panel)
136@@ -63,7 +65,7 @@
137
138 void VisibilityController::update()
139 {
140- if (!m_forceVisibleCountHash.isEmpty()) {
141+ if (!m_forceVisibleReasonHash.isEmpty()) {
142 return;
143 }
144 AutoHideMode mode = AutoHideMode(m_hideModeKey->getValue().toInt());
145@@ -88,35 +90,46 @@
146 }
147 }
148
149-void VisibilityController::beginForceVisible(const QString& service)
150+void VisibilityController::beginForceVisible(const QString& reason, const QString& service)
151 {
152- bool wasEmpty = m_forceVisibleCountHash.isEmpty();
153- if (m_forceVisibleCountHash.contains(service)) {
154- ++m_forceVisibleCountHash[service];
155- } else {
156- m_forceVisibleCountHash[service] = 1;
157- if (!service.isEmpty()) {
158- m_dbusWatcher->addWatchedService(service);
159- }
160+ bool wasEmpty = m_forceVisibleReasonHash.isEmpty();
161+ m_forceVisibleReasonHash[service].insert(reason);
162+ if (!service.isEmpty()) {
163+ // This call for beginForceVisible() comes from another application,
164+ // start watching it to be able to remove its request if the
165+ // application quits without calling endForceVisible()
166+ m_dbusWatcher->addWatchedService(service);
167+ }
168+ if (DEBUG_VISIBILITY) {
169+ UQ_VAR(m_forceVisibleReasonHash);
170 }
171 if (wasEmpty) {
172 setBehavior(new ForceVisibleBehavior(m_panel));
173 }
174 }
175
176-void VisibilityController::endForceVisible(const QString& service)
177+void VisibilityController::endForceVisible(const QString& reason, const QString& service)
178 {
179- if (m_forceVisibleCountHash.contains(service)) {
180- if (m_forceVisibleCountHash[service] == 1) {
181- m_forceVisibleCountHash.remove(service);
182- m_dbusWatcher->removeWatchedService(service);
183- } else {
184- --m_forceVisibleCountHash[service];
185+ if (m_forceVisibleReasonHash.contains(service)) {
186+ m_forceVisibleReasonHash[service].remove(reason);
187+ if (m_forceVisibleReasonHash[service].isEmpty()) {
188+ // No more reason for forced visibility from this application, remove
189+ // its hash entry
190+ m_forceVisibleReasonHash.remove(service);
191+ if (!service.isEmpty()) {
192+ // This call to endForceVisible() comes from another
193+ // application, since it does not force visibility anymore,
194+ // stop watching it
195+ m_dbusWatcher->removeWatchedService(service);
196+ }
197 }
198 } else {
199- UQ_WARNING << "Application" << service << "called endForceVisible() more than beginForceVisible().";
200- }
201- if (m_forceVisibleCountHash.isEmpty()) {
202+ UQ_WARNING << "Application" << service << "called endForceVisible(" << reason << ") without a matching call to beginForceVisible(" << reason << ").";
203+ }
204+ if (DEBUG_VISIBILITY) {
205+ UQ_VAR(m_forceVisibleReasonHash);
206+ }
207+ if (m_forceVisibleReasonHash.isEmpty()) {
208 update();
209 }
210 }
211@@ -125,20 +138,23 @@
212 {
213 // This method could be replaced by code calling reset() directly but
214 // having only one point where the behavior is changed makes it easy to log
215- // behavior changes using something like: UQ_VAR(behavior);
216+ // behavior changes
217+ if (DEBUG_VISIBILITY) {
218+ UQ_VAR(behavior);
219+ }
220 m_behavior.reset(behavior);
221 }
222
223 void VisibilityController::slotServiceUnregistered(const QString& service)
224 {
225- if (!m_forceVisibleCountHash.contains(service)) {
226+ if (!m_forceVisibleReasonHash.contains(service)) {
227 return;
228 }
229
230 UQ_WARNING << "Application" << service << "quit without calling endForceVisible().";
231- m_forceVisibleCountHash.remove(service);
232+ m_forceVisibleReasonHash.remove(service);
233 m_dbusWatcher->removeWatchedService(service);
234- if (m_forceVisibleCountHash.isEmpty()) {
235+ if (m_forceVisibleReasonHash.isEmpty()) {
236 update();
237 }
238 }
239
240=== modified file 'launcher/app/visibilitycontroller.h'
241--- launcher/app/visibilitycontroller.h 2011-03-22 11:30:53 +0000
242+++ launcher/app/visibilitycontroller.h 2011-05-18 16:23:29 +0000
243@@ -44,7 +44,7 @@
244 * or because the dash is visible. This is handled by the beginForceVisible()
245 * and endForceVisible() methods.
246 *
247- * Internally it maintains a refcount-per-app of forced visibility requests so
248+ * Internally it maintains a reason-per-app of forced visibility requests so
249 * that it can restore the default mode if an application quits without calling
250 * endForceVisible().
251 */
252@@ -57,12 +57,17 @@
253
254 /**
255 * Force visibility of the launcher.
256- * service is the dbus service (@see QDBusConnection::baseService()) of the
257+ *
258+ * @param reason the reason for forcing visibility. VisibilityController
259+ * will keep the launcher visible until one calls endForceVisible with the
260+ * same reason or the application which requested it quits.
261+ *
262+ * @param service the dbus service (@see QDBusConnection::baseService()) of the
263 * application which requested forced visibility. It is set to an empty
264 * string for internal requests.
265 */
266- Q_INVOKABLE void beginForceVisible(const QString& service = QString());
267- Q_INVOKABLE void endForceVisible(const QString& service = QString());
268+ Q_INVOKABLE void beginForceVisible(const QString& reason, const QString& service = QString());
269+ Q_INVOKABLE void endForceVisible(const QString& reason, const QString& service = QString());
270
271 private Q_SLOTS:
272 void update();
273@@ -80,8 +85,9 @@
274 QDBusServiceWatcher* m_dbusWatcher;
275 QScopedPointer<AbstractVisibilityBehavior> m_behavior;
276
277- typedef QHash<QString, int> ForceVisibleCountHash;
278- ForceVisibleCountHash m_forceVisibleCountHash;
279+ typedef QSet<QString> ReasonSet;
280+ typedef QHash<QString, ReasonSet> ForceVisibleReasonHash;
281+ ForceVisibleReasonHash m_forceVisibleReasonHash;
282
283 void setBehavior(AbstractVisibilityBehavior*);
284 };
285
286=== modified file 'libunity-2d-private/src/launcherclient.cpp'
287--- libunity-2d-private/src/launcherclient.cpp 2011-03-22 16:30:31 +0000
288+++ libunity-2d-private/src/launcherclient.cpp 2011-05-18 16:23:29 +0000
289@@ -36,7 +36,7 @@
290 {
291 LauncherClient* q;
292
293- void asyncDBusCall(const QString& methodName)
294+ void asyncDBusCall(const QString& methodName, const QString& arg)
295 {
296 /* The constructor for QDBusInterface potentially does synchronous
297 introspection calls. In contrast, this is really asynchronous.
298@@ -45,6 +45,7 @@
299 LAUNCHER_DBUS_OBJECT_PATH,
300 LAUNCHER_DBUS_INTERFACE,
301 methodName);
302+ call.setArguments(QVariantList() << arg);
303 QDBusConnection::sessionBus().asyncCall(call);
304 }
305 };
306@@ -61,14 +62,14 @@
307 delete d;
308 }
309
310-void LauncherClient::beginForceVisible()
311+void LauncherClient::beginForceVisible(const QString& reason)
312 {
313- d->asyncDBusCall("BeginForceVisible");
314+ d->asyncDBusCall("BeginForceVisible", reason);
315 }
316
317-void LauncherClient::endForceVisible()
318+void LauncherClient::endForceVisible(const QString& reason)
319 {
320- d->asyncDBusCall("EndForceVisible");
321+ d->asyncDBusCall("EndForceVisible", reason);
322 }
323
324 #include "launcherclient.moc"
325
326=== modified file 'libunity-2d-private/src/launcherclient.h'
327--- libunity-2d-private/src/launcherclient.h 2011-03-22 11:30:53 +0000
328+++ libunity-2d-private/src/launcherclient.h 2011-05-18 16:23:29 +0000
329@@ -41,8 +41,8 @@
330 LauncherClient(QObject* parent = 0);
331 ~LauncherClient();
332
333- void beginForceVisible();
334- void endForceVisible();
335+ void beginForceVisible(const QString& reason);
336+ void endForceVisible(const QString& reason);
337
338 private:
339 LauncherClientPrivate* const d;
340
341=== modified file 'panel/applets/homebutton/homebuttonapplet.cpp'
342--- panel/applets/homebutton/homebuttonapplet.cpp 2011-04-06 11:01:48 +0000
343+++ panel/applets/homebutton/homebuttonapplet.cpp 2011-05-18 16:23:29 +0000
344@@ -112,13 +112,13 @@
345 void HomeButtonApplet::enterEvent(QEvent* event)
346 {
347 Unity2d::Applet::enterEvent(event);
348- m_launcherClient->beginForceVisible();
349+ m_launcherClient->beginForceVisible("panel-homebutton");
350 }
351
352 void HomeButtonApplet::leaveEvent(QEvent* event)
353 {
354 Unity2d::Applet::leaveEvent(event);
355- m_launcherClient->endForceVisible();
356+ m_launcherClient->endForceVisible("panel-homebutton");
357 }
358
359 #include "homebuttonapplet.moc"
360
361=== modified file 'places/app/dashdeclarativeview.cpp'
362--- places/app/dashdeclarativeview.cpp 2011-04-29 16:26:25 +0000
363+++ places/app/dashdeclarativeview.cpp 2011-05-18 16:23:29 +0000
364@@ -178,7 +178,7 @@
365 m_mode = mode;
366 if (m_mode == HiddenMode) {
367 hide();
368- m_launcherClient->endForceVisible();
369+ m_launcherClient->endForceVisible("dash-active");
370 activeChanged(false);
371 } else {
372 show();
373@@ -193,7 +193,7 @@
374 if (oldMode == HiddenMode) {
375 // Check old mode to ensure we do not call BeginForceVisible twice
376 // if we go from desktop to fullscreen mode
377- m_launcherClient->beginForceVisible();
378+ m_launcherClient->beginForceVisible("dash-active");
379 }
380 activeChanged(true);
381 }
382
383=== modified file 'spread/app/spreadcontrol.cpp'
384--- spread/app/spreadcontrol.cpp 2011-03-17 17:28:31 +0000
385+++ spread/app/spreadcontrol.cpp 2011-05-18 16:23:29 +0000
386@@ -38,9 +38,9 @@
387 {
388 m_isShown = isShown;
389 if (m_isShown) {
390- m_launcherClient->beginForceVisible();
391+ m_launcherClient->beginForceVisible("spread-shown");
392 } else {
393- m_launcherClient->endForceVisible();
394+ m_launcherClient->endForceVisible("spread-shown");
395 }
396 }
397

Subscribers

People subscribed via source and target branches