Merge lp:~uriboni/unity-2d/clean-up-dash-dbus into lp:unity-2d/3.0

[Ugo Riboni]

[dash] Avoid exposing the entire QObject to DBUS, expose instead only what we really use

[Olivier Tilloy (osomon)]

You need to update the other components to activate the service on the new, normalized interface name:

$ grep -rn "\"com.canonical.Unity2d\"" *
launcher/UnityApplications/placeentry.cpp:146:static const char* DASH_DBUS_SERVICE = "com.canonical.Unity2d";
launcher/app/gesturehandler.cpp:120: QDBusInterface dashInterface("com.canonical.Unity2d", "/Dash", "com.canonical.Unity2d.Dash");
panel/applets/homebutton/homebuttonapplet.cpp:34:static const char* DBUS_SERVICE = "com.canonical.Unity2d";

review: Needs Fixing (code)

[Olivier Tilloy (osomon)]

Also, I compared the interfaces before and after your changes, and it seems a lot of methods are gone, some (or all?) of which are actually used in the other components:

List of methods before:
 - activateHome
 - activatePlaceEntry
 - setActive
 - setActivePlaceEntry
 - setExpanded

List of methods after:
 - activateHome

review: Needs Fixing

[Ugo Riboni (uriboni)]

> Also, I compared the interfaces before and after your changes, and it seems a
> lot of methods are gone, some (or all?) of which are actually used in the
> other components:

Actually the only one that was really removed in error is activatePlaceEntry. The others are not actually used via DBUS.

I also fixed the interface name used by clients, as pointed out in the previous comment.

[Olivier Tilloy (osomon)]

In places/app/dash.xml:

+ <method name="activatePlaceEntry">
+ <dox:d>Activates the Dash and shows the specified Place</dox:d>

Should be "[…] shows the specified place entry".

+ <signal name="activePlaceEntryChanged">
+ <dox:d>Signals when the currently active place in the Dash changes</dox:d>

Same here, and everywhere else really: "place" ≠ "place entry". Please make sure that the terminology is consistent.

review: Needs Fixing

[Olivier Tilloy (osomon)]

The value of the 'Name' key in places/app/unity-2d-places.service.in needs to be updated.

review: Needs Fixing

[Olivier Tilloy (osomon)]

Packages fail to build from source with the following error:

Building CXX object places/app/CMakeFiles/unity-2d-places.dir/dashdeclarativeview.cpp.o
cd /home/osomon/dev/unity-2d/build-area/unity-2d-3.6.1/obj-x86_64-linux-gnu/places/app && /usr/bin/g++ -g -O2 -g -Wall -O2 -Woverloaded-virtual -Wall -std=c++0x -I/home/osomon/dev/unity-2d/build-area/unity-2d-3.6.1/obj-x86_64-linux-gnu -I/usr/include/qt4 -I/usr/include/qt4/QtCore -I/usr/include/qt4/QtDBus -I/usr/include/qt4/QtDeclarative -I/usr/include/qt4/QtXml -I/usr/include/qt4/QtGui -I/home/osomon/dev/unity-2d/build-area/unity-2d-3.6.1/obj-x86_64-linux-gnu/places/app -I/usr/include/QtGConf -I/usr/include/gtk-2.0 -I/usr/lib/gtk-2.0/include -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/pango-1.0 -I/usr/include/gio-unix-2.0 -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/freetype2 -I/usr/include/libpng12 -I/home/osomon/dev/unity-2d/build-area/unity-2d-3.6.1/libunity-2d-private/src -o CMakeFiles/unity-2d-places.dir/dashdeclarativeview.cpp.o -c /home/osomon/dev/unity-2d/build-area/unity-2d-3.6.1/places/app/dashdeclarativeview.cpp

In file included from /home/osomon/dev/unity-2d/build-area/unity-2d-3.6.1/places/app/dashdeclarativeview.cpp:18:
/home/osomon/dev/unity-2d/build-area/unity-2d-3.6.1/obj-x86_64-linux-gnu/places/app/dashadaptor.h:17: fatal error: dashdeclarativeview.h: No such file or directory

review: Needs Fixing

[Olivier Tilloy (osomon)]

Quoting Aurélien on IRC:

bzr bd uses a separate build dir (src dir != build dir),
dashdeclarativeview.h is in src dir, so it needs to be added to the list of include directories,
adding ${CMAKE_CURRENT_SOURCE_DIR} to the existing include_directories() call should be enough

[Ugo Riboni (uriboni)]

Build error when building package is fixed now. Aureliene suggestion was good.

[Olivier Tilloy (osomon)]

It now builds and works as expected.

review: Approve (code functional)

[Florian Boucault (fboucault)]

Attempt to merge into lp:unity-2d failed due to conflicts:

text conflict in places/app/dashdeclarativeview.cpp

[Ugo Riboni (uriboni)]

Conflict is now fixed

=== modified file '.bzrignore'
--- .bzrignore	2011-02-10 13:44:13 +0000
+++ .bzrignore	2011-03-08 11:26:49 +0000
@@ -11,6 +11,7 @@
 places/UnityPlaces/libUnityPlaces.so*
 places/app/unity-2d-places
 places/app/unity-2d-places.service
+places/app/dashadaptor.*
 
 spread/app/unity-2d-spread
 spread/app/unity-2d-spread.service
 
=== modified file 'launcher/UnityApplications/placeentry.cpp'
--- launcher/UnityApplications/placeentry.cpp	2011-02-10 01:00:41 +0000
+++ launcher/UnityApplications/placeentry.cpp	2011-03-08 11:26:49 +0000
@@ -143,7 +143,7 @@
 static const char* UNITY_PLACE_ENTRY_INTERFACE = "com.canonical.Unity.PlaceEntry";
 static const char* SECTION_PROPERTY = "section";
 
-static const char* DASH_DBUS_SERVICE = "com.canonical.Unity2d";
+static const char* DASH_DBUS_SERVICE = "com.canonical.Unity2d.Dash";
 static const char* DASH_DBUS_PATH = "/Dash";
 static const char* DASH_DBUS_INTERFACE = "com.canonical.Unity2d.Dash";
 
 
=== modified file 'launcher/app/gesturehandler.cpp'
--- launcher/app/gesturehandler.cpp	2011-02-21 00:25:44 +0000
+++ launcher/app/gesturehandler.cpp	2011-03-08 11:26:49 +0000
@@ -117,7 +117,7 @@
         - show the home page of the dash if the dash is closed
         - close the dash, if the dash is opened
     */
-    QDBusInterface dashInterface("com.canonical.Unity2d", "/Dash", "com.canonical.Unity2d.Dash");
+    QDBusInterface dashInterface("com.canonical.Unity2d.Dash", "/Dash", "com.canonical.Unity2d.Dash");
     bool dashActive = dashInterface.property("active").toBool();
 
     if (dashActive) {
 
=== modified file 'panel/applets/homebutton/homebuttonapplet.cpp'
--- panel/applets/homebutton/homebuttonapplet.cpp	2011-02-15 11:47:42 +0000
+++ panel/applets/homebutton/homebuttonapplet.cpp	2011-03-08 11:26:49 +0000
@@ -31,7 +31,7 @@
 #include <QDBusServiceWatcher>
 #include <QDBusConnectionInterface>
 
-static const char* DBUS_SERVICE = "com.canonical.Unity2d";
+static const char* DBUS_SERVICE = "com.canonical.Unity2d.Dash";
 static const char* DBUS_PATH = "/Dash";
 static const char* DBUS_IFACE = "com.canonical.Unity2d.Dash";
 
 
=== modified file 'places/app/CMakeLists.txt'
--- places/app/CMakeLists.txt	2011-02-24 01:13:42 +0000
+++ places/app/CMakeLists.txt	2011-03-08 11:26:49 +0000
@@ -18,9 +18,14 @@
 # Build
 configure_file(unity-2d-places.service.in unity-2d-places.service)
 
+qt4_add_dbus_adaptor(places_SRCS dash.xml
+    dashdeclarativeview.h DashDeclarativeView
+    )
+
 add_executable(unity-2d-places ${places_SRCS} ${places_MOC_SRCS})
 
 include_directories(
+    ${CMAKE_CURRENT_SOURCE_DIR}
     ${CMAKE_CURRENT_BINARY_DIR}
     ${QTGCONF_INCLUDE_DIRS}
     ${GTK_INCLUDE_DIRS}
 
=== added file 'places/app/dash.xml'
--- places/app/dash.xml	1970-01-01 00:00:00 +0000
+++ places/app/dash.xml	2011-03-08 11:26:49 +0000
@@ -0,0 +1,47 @@
+<!DOCTYPE node PUBLIC "-//freedesktop//DTD D-BUS Object Introspection 1.0//EN" "http://www.freedesktop.org/standards/dbus/1.0/introspect.dtd">
+<node xmlns:dox="http://www.ayatana.org/dbus/dox.dtd">
+    <dox:d><![CDATA[
+      @mainpage
+
+      An interface to control the dash in Unity-2d.
+    ]]></dox:d>
+    <interface name="com.canonical.Unity2d.Dash" xmlns:dox="http://www.ayatana.org/dbus/dox.dtd">
+        <dox:d>
+          An interface to control the dash in Unity-2d.
+        </dox:d>
+        <method name="activateHome">
+            <dox:d>Activates the Dash and shows the home section</dox:d>
+        </method>
+        <method name="activatePlaceEntry">
+            <dox:d>Activates the Dash and shows the specified place entry</dox:d>
+            <arg name="file" type="s" direction="in">
+                    <dox:d>The .place file for the place containing the entry that should be activated</dox:d>
+            </arg>
+            <arg name="entry" type="s" direction="in">
+                    <dox:d>The place entry that should be activated</dox:d>
+            </arg>
+            <arg name="section" type="i" direction="in">
+                    <dox:d>The section of the place entry that should be activated</dox:d>
+            </arg>
+        </method>
+        <property name="active" type="b" access="readwrite">
+            <dox:d>True if the Dash is active</dox:d>
+        </property>
+        <property name="activePlaceEntry" type="s" access="readwrite">
+            <dox:d>The currently active place entry</dox:d>
+        </property>
+        <signal name="activeChanged">
+                <dox:d>Signals when the active status of the Dash changes</dox:d>
+                <arg name="active" type="b" direction="out">
+                        <dox:d>True if the Dash is active</dox:d>
+                </arg>
+        </signal>
+        <signal name="activePlaceEntryChanged">
+                <dox:d>Signals when the currently active place entry in the Dash changes</dox:d>
+                <arg name="activePlaceEntry" type="s" direction="out">
+                        <dox:d>The currently active place entry</dox:d>
+                </arg>
+        </signal>
+    </interface>
+</node>
+
 
=== modified file 'places/app/dashdeclarativeview.cpp'
--- places/app/dashdeclarativeview.cpp	2011-03-08 09:57:38 +0000
+++ places/app/dashdeclarativeview.cpp	2011-03-08 11:26:49 +0000
@@ -15,6 +15,8 @@
  */
 
 #include "dashdeclarativeview.h"
+#include "dashadaptor.h"
+
 #include <QDesktopWidget>
 #include <QApplication>
 #include <QBitmap>
@@ -22,8 +24,8 @@
 #include <QDeclarativeContext>
 #include <QX11Info>
 #include <QGraphicsObject>
+#include <QtDBus/QDBusConnection>
 #include <QTimer>
-
 #include <QDebug>
 
 #include <X11/Xlib.h>
@@ -38,6 +40,9 @@
 static const int DASH_DESKTOP_COLLAPSED_HEIGHT = 115;
 static const int DASH_DESKTOP_EXPANDED_HEIGHT = 606;
 
+static const char* DASH_DBUS_SERVICE = "com.canonical.Unity2d.Dash";
+static const char* DASH_DBUS_OBJECT_PATH = "/Dash";
+
 DashDeclarativeView::DashDeclarativeView()
 : QDeclarativeView()
 , m_mode(HiddenMode)
@@ -320,3 +325,14 @@
 {
     return QX11Info::isCompositingManagerRunning();
 }
+
+bool
+DashDeclarativeView::connectToBus()
+{
+    bool ok = QDBusConnection::sessionBus().registerService(DASH_DBUS_SERVICE);
+    if (!ok) {
+        return false;
+    }
+    new DashAdaptor(this);
+    return QDBusConnection::sessionBus().registerObject(DASH_DBUS_OBJECT_PATH, this);
+}
 
=== modified file 'places/app/dashdeclarativeview.h'
--- places/app/dashdeclarativeview.h	2011-03-08 09:57:38 +0000
+++ places/app/dashdeclarativeview.h	2011-03-08 11:26:49 +0000
@@ -24,7 +24,6 @@
     Q_OBJECT
     Q_ENUMS(DashMode)
 
-    Q_CLASSINFO("D-Bus Interface", "com.canonical.Unity2d.Dash")
     Q_PROPERTY(bool active READ active WRITE setActive NOTIFY activeChanged)
     Q_PROPERTY(bool expanded READ expanded WRITE setExpanded NOTIFY expandedChanged)
     Q_PROPERTY(DashMode dashMode READ dashMode WRITE setDashMode NOTIFY dashModeChanged)
@@ -58,7 +57,8 @@
 
     /* methods */
     Q_INVOKABLE void activatePlaceEntry(const QString& file, const QString& entry, const int section = 0);
-    Q_INVOKABLE void activateHome();
+    Q_SLOT void activateHome();
+    bool connectToBus();
 
 Q_SIGNALS:
     void activeChanged(bool);
 
=== modified file 'places/app/places.cpp'
--- places/app/places.cpp	2011-02-24 01:13:42 +0000
+++ places/app/places.cpp	2011-03-08 11:26:49 +0000
@@ -39,33 +39,6 @@
 #include "dashdeclarativeview.h"
 #include "config.h"
 
-/* Register a D-Bus service for activation and deactivation of the dash */
-static bool registerDBusService(DashDeclarativeView* view)
-{
-    QDBusConnection bus = QDBusConnection::sessionBus();
-    if (!bus.registerService("com.canonical.Unity2d")) {
-        qCritical() << "Failed to register DBus service, is there another instance already running?";
-        return false;
-    }
-    /* FIXME: use an adaptor class in order not to expose all of the view's
-       properties and methods. */\
-    if (!bus.registerObject("/Dash", view, QDBusConnection::ExportAllContents)) {
-        qCritical() << "Failed to register /Dash, this should not happen!";
-        return false;
-    }
-    /* It would be nice to support the newly introduced (D-Bus 0.14 07/09/2010)
-       property change notification that Qt 4.7 does not implement.
-
-        org.freedesktop.DBus.Properties.PropertiesChanged (
-            STRING interface_name,
-            DICT<STRING,VARIANT> changed_properties,
-            ARRAY<STRING> invalidated_properties);
-
-       ref.: http://randomguy3.wordpress.com/2010/09/07/the-magic-of-qtdbus-and-the-propertychanged-signal/
-    */
-    return true;
-}
-
 int main(int argc, char *argv[])
 {
     /* gtk needs to be inited, otherwise we get an assert failure in gdk */
@@ -86,8 +59,8 @@
 
     qmlRegisterType<DashDeclarativeView>("Places", 1, 0, "DashDeclarativeView");
     DashDeclarativeView view;
-
-    if (!registerDBusService(&view)) {
+    if (!view.connectToBus()) {
+        qCritical() << "Another instance of the Dash already exists. Quitting.";
         return -1;
     }
 
 
=== modified file 'places/app/unity-2d-places.service.in'
--- places/app/unity-2d-places.service.in	2011-01-14 20:47:33 +0000
+++ places/app/unity-2d-places.service.in	2011-03-08 11:26:49 +0000
@@ -1,3 +1,3 @@
 [D-BUS Service]
-Name=com.canonical.Unity2d
+Name=com.canonical.Unity2d.Dash
 Exec=@CMAKE_INSTALL_PREFIX@/bin/unity-2d-places