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

[Aurélien Gâteau]

[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.

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.

[Ugo Riboni (uriboni)]

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]

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)]

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]

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.

=== modified file 'launcher/Launcher.qml'
--- launcher/Launcher.qml	2011-05-11 10:29:54 +0000
+++ launcher/Launcher.qml	2011-05-18 16:23:29 +0000
@@ -122,8 +122,8 @@
             else visibilityController.endForceVisible()
         }
         onFocusChanged: {
-            if (focus) visibilityController.beginForceVisible()
-            else visibilityController.endForceVisible()
+            if (focus) visibilityController.beginForceVisible("launcher-focus")
+            else visibilityController.endForceVisible("launcher-focus")
         }
     }
 

=== modified file 'launcher/LauncherList.qml'
--- launcher/LauncherList.qml	2011-05-02 16:25:49 +0000
+++ launcher/LauncherList.qml	2011-05-18 16:23:29 +0000
@@ -222,9 +222,9 @@
             target: item.menu
             onFoldedChanged: {
                 if (item.menu.folded) {
-                    visibilityController.endForceVisible();
+                    visibilityController.endForceVisible("launcher-menu");
                 } else {
-                    visibilityController.beginForceVisible();
+                    visibilityController.beginForceVisible("launcher-menu");
                 }
             }
         }
@@ -277,19 +277,19 @@
             target: urgentAnimation
             onRunningChanged: {
                 if (urgentAnimation.running) {
-                    visibilityController.beginForceVisible()
+                    visibilityController.beginForceVisible("launcher-urgent");
                 } else {
-                    visibilityController.endForceVisible()
+                    visibilityController.endForceVisible("launcher-urgent");
                 }
             }
             Component.onCompleted: {
                 if (urgentAnimation.running) {
-                    visibilityController.beginForceVisible()
+                    visibilityController.beginForceVisible("launcher-urgent")
                 }
             }
             Component.onDestruction: {
                 if (urgentAnimation.running) {
-                    visibilityController.endForceVisible()
+                    visibilityController.endForceVisible("launcher-urgent")
                 }
             }
         }

=== modified file 'launcher/app/launcher.xml'
--- launcher/app/launcher.xml	2011-03-22 11:30:53 +0000
+++ launcher/app/launcher.xml	2011-05-18 16:23:29 +0000
@@ -23,13 +23,24 @@
                EndForceVisible() when you don't need the launcher to stay
                visible anymore.
             ]]></dox:d>
+            <arg type="s" name="reason">
+                <dox:d>Reason for forcing visibility</dox:d>
+            </arg>
         </method>
         <method name="EndForceVisible">
             <dox:d><![CDATA[
                Tells the launcher you don't need it to stay visible anymore.
                Please note this does not mean the launcher will necessarily
                hide.
+
+               One call to EndForceVisible() will stop all previous calls to
+               BeginForceVisible() coming from the same application with the
+               same reason.
             ]]></dox:d>
+            <arg type="s" name="reason">
+                <dox:d>Reason for ending the visibility forcing. Must match
+                with a previous call to BeginForceVisible</dox:d>
+            </arg>
         </method>
     </interface>
 </node>

=== modified file 'launcher/app/launcherdbus.cpp'
--- launcher/app/launcherdbus.cpp	2011-03-22 11:30:53 +0000
+++ launcher/app/launcherdbus.cpp	2011-05-18 16:23:29 +0000
@@ -65,15 +65,15 @@
 }
 
 void
-LauncherDBus::BeginForceVisible()
+LauncherDBus::BeginForceVisible(const QString& reason)
 {
     UQ_RETURN_IF_FAIL(calledFromDBus());
-    m_visibilityController->beginForceVisible(message().service());
+    m_visibilityController->beginForceVisible(reason, message().service());
 }
 
 void
-LauncherDBus::EndForceVisible()
+LauncherDBus::EndForceVisible(const QString& reason)
 {
     UQ_RETURN_IF_FAIL(calledFromDBus());
-    m_visibilityController->endForceVisible(message().service());
+    m_visibilityController->endForceVisible(reason, message().service());
 }

=== modified file 'launcher/app/launcherdbus.h'
--- launcher/app/launcherdbus.h	2011-03-22 11:30:53 +0000
+++ launcher/app/launcherdbus.h	2011-05-18 16:23:29 +0000
@@ -43,8 +43,8 @@
     bool connectToBus();
 
 public Q_SLOTS:
-    Q_NOREPLY void BeginForceVisible();
-    Q_NOREPLY void EndForceVisible();
+    Q_NOREPLY void BeginForceVisible(const QString& reason);
+    Q_NOREPLY void EndForceVisible(const QString& reason);
     Q_NOREPLY void AddWebFavorite(const QString& url);
 
 private:

=== modified file 'launcher/app/visibilitycontroller.cpp'
--- launcher/app/visibilitycontroller.cpp	2011-03-22 11:30:53 +0000
+++ launcher/app/visibilitycontroller.cpp	2011-05-18 16:23:29 +0000
@@ -39,6 +39,8 @@
 
 static const char* GCONF_LAUNCHER_HIDEMODE_KEY = "/desktop/unity-2d/launcher/hide_mode";
 
+static const bool DEBUG_VISIBILITY = getenv("UNITY2DLAUNCHER_DEBUG_VISIBILITY") != 0;
+
 VisibilityController::VisibilityController(Unity2dPanel* panel)
 : QObject(panel)
 , m_panel(panel)
@@ -63,7 +65,7 @@
 
 void VisibilityController::update()
 {
-    if (!m_forceVisibleCountHash.isEmpty()) {
+    if (!m_forceVisibleReasonHash.isEmpty()) {
         return;
     }
     AutoHideMode mode = AutoHideMode(m_hideModeKey->getValue().toInt());
@@ -88,35 +90,46 @@
     }
 }
 
-void VisibilityController::beginForceVisible(const QString& service)
+void VisibilityController::beginForceVisible(const QString& reason, const QString& service)
 {
-    bool wasEmpty = m_forceVisibleCountHash.isEmpty();
-    if (m_forceVisibleCountHash.contains(service)) {
-        ++m_forceVisibleCountHash[service];
-    } else {
-        m_forceVisibleCountHash[service] = 1;
-        if (!service.isEmpty()) {
-            m_dbusWatcher->addWatchedService(service);
-        }
+    bool wasEmpty = m_forceVisibleReasonHash.isEmpty();
+    m_forceVisibleReasonHash[service].insert(reason);
+    if (!service.isEmpty()) {
+        // This call for beginForceVisible() comes from another application,
+        // start watching it to be able to remove its request if the
+        // application quits without calling endForceVisible()
+        m_dbusWatcher->addWatchedService(service);
+    }
+    if (DEBUG_VISIBILITY) {
+        UQ_VAR(m_forceVisibleReasonHash);
     }
     if (wasEmpty) {
         setBehavior(new ForceVisibleBehavior(m_panel));
     }
 }
 
-void VisibilityController::endForceVisible(const QString& service)
+void VisibilityController::endForceVisible(const QString& reason, const QString& service)
 {
-    if (m_forceVisibleCountHash.contains(service)) {
-        if (m_forceVisibleCountHash[service] == 1) {
-            m_forceVisibleCountHash.remove(service);
-            m_dbusWatcher->removeWatchedService(service);
-        } else {
-            --m_forceVisibleCountHash[service];
+    if (m_forceVisibleReasonHash.contains(service)) {
+        m_forceVisibleReasonHash[service].remove(reason);
+        if (m_forceVisibleReasonHash[service].isEmpty()) {
+            // No more reason for forced visibility from this application, remove
+            // its hash entry
+            m_forceVisibleReasonHash.remove(service);
+            if (!service.isEmpty()) {
+                // This call to endForceVisible() comes from another
+                // application, since it does not force visibility anymore,
+                // stop watching it
+                m_dbusWatcher->removeWatchedService(service);
+            }
         }
     } else {
-        UQ_WARNING << "Application" << service << "called endForceVisible() more than beginForceVisible().";
-    }
-    if (m_forceVisibleCountHash.isEmpty()) {
+        UQ_WARNING << "Application" << service << "called endForceVisible(" << reason << ") without a matching call to beginForceVisible(" << reason << ").";
+    }
+    if (DEBUG_VISIBILITY) {
+        UQ_VAR(m_forceVisibleReasonHash);
+    }
+    if (m_forceVisibleReasonHash.isEmpty()) {
         update();
     }
 }
@@ -125,20 +138,23 @@
 {
     // This method could be replaced by code calling reset() directly but
     // having only one point where the behavior is changed makes it easy to log
-    // behavior changes using something like: UQ_VAR(behavior);
+    // behavior changes
+    if (DEBUG_VISIBILITY) {
+        UQ_VAR(behavior);
+    }
     m_behavior.reset(behavior);
 }
 
 void VisibilityController::slotServiceUnregistered(const QString& service)
 {
-    if (!m_forceVisibleCountHash.contains(service)) {
+    if (!m_forceVisibleReasonHash.contains(service)) {
         return;
     }
 
     UQ_WARNING << "Application" << service << "quit without calling endForceVisible().";
-    m_forceVisibleCountHash.remove(service);
+    m_forceVisibleReasonHash.remove(service);
     m_dbusWatcher->removeWatchedService(service);
-    if (m_forceVisibleCountHash.isEmpty()) {
+    if (m_forceVisibleReasonHash.isEmpty()) {
         update();
     }
 }

=== modified file 'launcher/app/visibilitycontroller.h'
--- launcher/app/visibilitycontroller.h	2011-03-22 11:30:53 +0000
+++ launcher/app/visibilitycontroller.h	2011-05-18 16:23:29 +0000
@@ -44,7 +44,7 @@
  * or because the dash is visible. This is handled by the beginForceVisible()
  * and endForceVisible() methods.
  *
- * Internally it maintains a refcount-per-app of forced visibility requests so
+ * Internally it maintains a reason-per-app of forced visibility requests so
  * that it can restore the default mode if an application quits without calling
  * endForceVisible().
  */
@@ -57,12 +57,17 @@
 
     /**
      * Force visibility of the launcher.
-     * service is the dbus service (@see QDBusConnection::baseService()) of the
+     *
+     * @param reason the reason for forcing visibility. VisibilityController
+     * will keep the launcher visible until one calls endForceVisible with the
+     * same reason or the application which requested it quits.
+     *
+     * @param service the dbus service (@see QDBusConnection::baseService()) of the
      * application which requested forced visibility. It is set to an empty
      * string for internal requests.
      */
-    Q_INVOKABLE void beginForceVisible(const QString& service = QString());
-    Q_INVOKABLE void endForceVisible(const QString& service = QString());
+    Q_INVOKABLE void beginForceVisible(const QString& reason, const QString& service = QString());
+    Q_INVOKABLE void endForceVisible(const QString& reason, const QString& service = QString());
 
 private Q_SLOTS:
     void update();
@@ -80,8 +85,9 @@
     QDBusServiceWatcher* m_dbusWatcher;
     QScopedPointer<AbstractVisibilityBehavior> m_behavior;
 
-    typedef QHash<QString, int> ForceVisibleCountHash;
-    ForceVisibleCountHash m_forceVisibleCountHash;
+    typedef QSet<QString> ReasonSet;
+    typedef QHash<QString, ReasonSet> ForceVisibleReasonHash;
+    ForceVisibleReasonHash m_forceVisibleReasonHash;
 
     void setBehavior(AbstractVisibilityBehavior*);
 };

=== modified file 'libunity-2d-private/src/launcherclient.cpp'
--- libunity-2d-private/src/launcherclient.cpp	2011-03-22 16:30:31 +0000
+++ libunity-2d-private/src/launcherclient.cpp	2011-05-18 16:23:29 +0000
@@ -36,7 +36,7 @@
 {
     LauncherClient* q;
 
-    void asyncDBusCall(const QString& methodName)
+    void asyncDBusCall(const QString& methodName, const QString& arg)
     {
         /* The constructor for QDBusInterface potentially does synchronous
            introspection calls. In contrast, this is really asynchronous.
@@ -45,6 +45,7 @@
                                                            LAUNCHER_DBUS_OBJECT_PATH,
                                                            LAUNCHER_DBUS_INTERFACE,
                                                            methodName);
+        call.setArguments(QVariantList() << arg);
         QDBusConnection::sessionBus().asyncCall(call);
     }
 };
@@ -61,14 +62,14 @@
     delete d;
 }
 
-void LauncherClient::beginForceVisible()
+void LauncherClient::beginForceVisible(const QString& reason)
 {
-    d->asyncDBusCall("BeginForceVisible");
+    d->asyncDBusCall("BeginForceVisible", reason);
 }
 
-void LauncherClient::endForceVisible()
+void LauncherClient::endForceVisible(const QString& reason)
 {
-    d->asyncDBusCall("EndForceVisible");
+    d->asyncDBusCall("EndForceVisible", reason);
 }
 
 #include "launcherclient.moc"

=== modified file 'libunity-2d-private/src/launcherclient.h'
--- libunity-2d-private/src/launcherclient.h	2011-03-22 11:30:53 +0000
+++ libunity-2d-private/src/launcherclient.h	2011-05-18 16:23:29 +0000
@@ -41,8 +41,8 @@
     LauncherClient(QObject* parent = 0);
     ~LauncherClient();
 
-    void beginForceVisible();
-    void endForceVisible();
+    void beginForceVisible(const QString& reason);
+    void endForceVisible(const QString& reason);
 
 private:
     LauncherClientPrivate* const d;

=== modified file 'panel/applets/homebutton/homebuttonapplet.cpp'
--- panel/applets/homebutton/homebuttonapplet.cpp	2011-04-06 11:01:48 +0000
+++ panel/applets/homebutton/homebuttonapplet.cpp	2011-05-18 16:23:29 +0000
@@ -112,13 +112,13 @@
 void HomeButtonApplet::enterEvent(QEvent* event)
 {
     Unity2d::Applet::enterEvent(event);
-    m_launcherClient->beginForceVisible();
+    m_launcherClient->beginForceVisible("panel-homebutton");
 }
 
 void HomeButtonApplet::leaveEvent(QEvent* event)
 {
     Unity2d::Applet::leaveEvent(event);
-    m_launcherClient->endForceVisible();
+    m_launcherClient->endForceVisible("panel-homebutton");
 }
 
 #include "homebuttonapplet.moc"

=== modified file 'places/app/dashdeclarativeview.cpp'
--- places/app/dashdeclarativeview.cpp	2011-04-29 16:26:25 +0000
+++ places/app/dashdeclarativeview.cpp	2011-05-18 16:23:29 +0000
@@ -178,7 +178,7 @@
     m_mode = mode;
     if (m_mode == HiddenMode) {
         hide();
-        m_launcherClient->endForceVisible();
+        m_launcherClient->endForceVisible("dash-active");
         activeChanged(false);
     } else {
         show();
@@ -193,7 +193,7 @@
         if (oldMode == HiddenMode) {
             // Check old mode to ensure we do not call BeginForceVisible twice
             // if we go from desktop to fullscreen mode
-            m_launcherClient->beginForceVisible();
+            m_launcherClient->beginForceVisible("dash-active");
         }
         activeChanged(true);
     }

=== modified file 'spread/app/spreadcontrol.cpp'
--- spread/app/spreadcontrol.cpp	2011-03-17 17:28:31 +0000
+++ spread/app/spreadcontrol.cpp	2011-05-18 16:23:29 +0000
@@ -38,9 +38,9 @@
 {
     m_isShown = isShown;
     if (m_isShown) {
-        m_launcherClient->beginForceVisible();
+        m_launcherClient->beginForceVisible("spread-shown");
     } else {
-        m_launcherClient->endForceVisible();
+        m_launcherClient->endForceVisible("spread-shown");
     }
 }