Merge lp:~muktupavels/libappindicator/create-as-service into lp:libappindicator/15.10

[Alberts Muktupāvels]

Follow spec more closely. "RegisterStatusNotifierItem" method on watcher should be called with dbus name/address not path.

Follow spec more closely. "RegisterStatusNotifierItem" method on watcher should be called with dbus name/address not path.

[Dmitry Shachnev (mitya57)]

Unfortunately it fails to build because NOTIFICATION_ITEM_DBUS_OBJ is not defined:
http://mitya57.me/builds/libappindicator_12.10.1+15.04.20141110-0ubuntu2_amd64.build

review: Needs Fixing

[Alberts Muktupāvels (muktupavels)]

> Unfortunately it fails to build because NOTIFICATION_ITEM_DBUS_OBJ is not
> defined:
> http://mitya57.me/builds/libappindicator_12.10.1+15.04.20141110-0ubuntu2_amd64
> .build

Previous commit was missing one file... I hope that now it builds.

[Dmitry Shachnev (mitya57)]

Thanks! It builds now. I tested with remmina and transmission, everything seems to work as before.

review: Approve

[Ted Gould (ted)]

I'm a little confused, why do we want Items to register a name on the bus? I'm not sure what benefit that has, and it makes things more difficult when we start looking at application confinement where we're not allowing name registrations.

review: Needs Information

[Alberts Muktupāvels (muktupavels)]

> I'm a little confused, why do we want Items to register a name on the bus? I'm

Because it is what spec says, no?

http://www.freedesktop.org/wiki/Specifications/StatusNotifierItem/StatusNotifierWatcher/
"Register a StatusNotifierItem into the StatusNotifierWatcher, in the form of its full name on the session bus, for instance org.freedesktop.StatusNotifierItem-4077-1."

> not sure what benefit that has, and it makes things more difficult when we
> start looking at application confinement where we're not allowing name
> registrations.

For example it makes easier to watch when name (status notifier item) disappears from bus:
https://github.com/albertsmuktupavels/libstatus-notifier/blob/master/libstatus-notifier/sn-watcher.c#L117

Did not understand part about not allowing name registrations... :(

[Ted Gould (ted)]

On Tue, 2015-06-30 at 14:18 +0000, Alberts Muktupāvels wrote:

> > I'm a little confused, why do we want Items to register a name on the bus? I'm
>
> Because it is what spec says, no?
>
> http://www.freedesktop.org/wiki/Specifications/StatusNotifierItem/StatusNotifierWatcher/
> "Register a StatusNotifierItem into the StatusNotifierWatcher, in the form of its full name on the session bus, for instance org.freedesktop.StatusNotifierItem-4077-1."

Ah, interesting. That's been added since we implemented the spec.

> > not sure what benefit that has, and it makes things more difficult when we
> > start looking at application confinement where we're not allowing name
> > registrations.
>
> For example it makes easier to watch when name (status notifier item) disappears from bus:
> https://github.com/albertsmuktupavels/libstatus-notifier/blob/master/libstatus-notifier/sn-watcher.c#L117

You can watch for when the connection drops off the bus as well. I don't
think there's a real difference between watching the name and the
connection.

> Did not understand part about not allowing name registrations... :(

For Ubuntu Personal and Ubuntu Phone we want to allow application
indicators in the convergence cases, but the confinement there doesn't
allow for name registrations on the dbus session bus.

[Alberts Muktupāvels (muktupavels)]

> On Tue, 2015-06-30 at 14:18 +0000, Alberts Muktupāvels wrote:
>
> > > I'm a little confused, why do we want Items to register a name on the bus?
> I'm
> >
> > Because it is what spec says, no?
> >
> > http://www.freedesktop.org/wiki/Specifications/StatusNotifierItem/StatusNoti
> fierWatcher/
> > "Register a StatusNotifierItem into the StatusNotifierWatcher, in the form
> of its full name on the session bus, for instance
> org.freedesktop.StatusNotifierItem-4077-1."
>
>
> Ah, interesting. That's been added since we implemented the spec.

I don't like this spec, but I guess there is no better choice... Does that mean that they have changed spec when there was existing implementations?

> > > not sure what benefit that has, and it makes things more difficult when we
> > > start looking at application confinement where we're not allowing name
> > > registrations.
> >
> > For example it makes easier to watch when name (status notifier item)
> disappears from bus:
> > https://github.com/albertsmuktupavels/libstatus-notifier/blob/master
> /libstatus-notifier/sn-watcher.c#L117
>
>
>
> You can watch for when the connection drops off the bus as well. I don't
> think there's a real difference between watching the name and the
> connection.

That was just small example. Still I think it is better to follow spec not trying to support each possible implementation that is different from spec.

> > Did not understand part about not allowing name registrations... :(
>
>
> For Ubuntu Personal and Ubuntu Phone we want to allow application
> indicators in the convergence cases, but the confinement there doesn't
> allow for name registrations on the dbus session bus.

Is there a way to detect that application is running on Ubuntu Personal and/or Ubuntu Phone? Then I could update branch to register dbus name only if it is not running on Personal or Phone.

[Ted Gould (ted)]

On Wed, 2015-07-01 at 16:32 +0000, Alberts Muktupāvels wrote:

> I don't like this spec, but I guess there is no better choice... Does
> that mean that they have changed spec when there was existing
> implementations?

Yes, and I imagine that is why it is written as a "can" instead of a
"must" or even "should."

"Each application can register"

http://www.freedesktop.org/wiki/Specifications/StatusNotifierItem/StatusNotifierItem/

> > You can watch for when the connection drops off the bus as well. I don't
> > think there's a real difference between watching the name and the
> > connection.
>
>
> That was just small example. Still I think it is better to follow spec
> not trying to support each possible implementation that is different
> from spec.

Since it is not a required part of the spec I think you should probably
support the more generic DBus mechanisms as well as parts of the spec
that you wish.

> > > Did not understand part about not allowing name registrations... :(
> >
> >
> > For Ubuntu Personal and Ubuntu Phone we want to allow application
> > indicators in the convergence cases, but the confinement there doesn't
> > allow for name registrations on the dbus session bus.
>
>
> Is there a way to detect that application is running on Ubuntu
> Personal and/or Ubuntu Phone? Then I could update branch to register
> dbus name only if it is not running on Personal or Phone.

I think what would perhaps be better is allow for the name registration
to fail, and handle that exception. There's no real reason to put the
name in the messages. Basically make it so that the name request is only
optional to have succeed.

[Alberts Muktupāvels (muktupavels)]

> On Wed, 2015-07-01 at 16:32 +0000, Alberts Muktupāvels wrote:
>
> > I don't like this spec, but I guess there is no better choice... Does
> > that mean that they have changed spec when there was existing
> > implementations?
>
>
>
> Yes, and I imagine that is why it is written as a "can" instead of a
> "must" or even "should."
>
> "Each application can register"
>
> http://www.freedesktop.org/wiki/Specifications/StatusNotifierItem/StatusNotifi
> erItem/

I understand it like this:
Each application can register multiple items...

Next paragraph:
"As soon as a new instance of a StatusNotifierItem is created, the application must register the unique instance name to the StatusNotifierWatcher as described in the Section called StatusNotifierWatcher"

Is not unique name meant to be org.freedesktop.StatusNotifierItem-PID-ID?

RegisterStatusINotifierItem method:
"Register a StatusNotifierItem into the StatusNotifierWatcher, in the form of its full name on the session bus, for instance org.freedesktop.StatusNotifierItem-4077-1."

> > > You can watch for when the connection drops off the bus as well. I don't
> > > think there's a real difference between watching the name and the
> > > connection.
> >
> >
> > That was just small example. Still I think it is better to follow spec
> > not trying to support each possible implementation that is different
> > from spec.
>
>
>
> Since it is not a required part of the spec I think you should probably
> support the more generic DBus mechanisms as well as parts of the spec
> that you wish.

Well spec says exactly how item should be registered with watcher... I don't know how that spec looked in past - that is what I am reading right now.

> > > > Did not understand part about not allowing name registrations... :(
> > >
> > >
> > > For Ubuntu Personal and Ubuntu Phone we want to allow application
> > > indicators in the convergence cases, but the confinement there doesn't
> > > allow for name registrations on the dbus session bus.
> >
> >
> > Is there a way to detect that application is running on Ubuntu
> > Personal and/or Ubuntu Phone? Then I could update branch to register
> > dbus name only if it is not running on Personal or Phone.
>
>
> I think what would perhaps be better is allow for the name registration
> to fail, and handle that exception. There's no real reason to put the
> name in the messages. Basically make it so that the name request is only
> optional to have succeed.

https://developer.gnome.org/gio/stable/gio-Owning-Bus-Names.html#g-bus-own-name
That would be name_lost_handler with connection == NULL, right?

So, are you ok with this change? with adding fallback to old behaviour if name registration fails?

[Ted Gould (ted)]

On Wed, 2015-07-01 at 20:58 +0000, Alberts Muktupāvels wrote:

> I understand it like this:
> Each application can register multiple items...
>
> Next paragraph:
> "As soon as a new instance of a StatusNotifierItem is created, the application must register the unique instance name to the StatusNotifierWatcher as described in the Section called StatusNotifierWatcher"
>
> Is not unique name meant to be org.freedesktop.StatusNotifierItem-PID-ID?

No, it is not. A unique bus name is the name that DBus gives the
connection, for example :3.0. You can find a description of the various
bus names in the DBus spec:

http://dbus.freedesktop.org/doc/dbus-specification.html#message-protocol-names-bus

There can only be one item per connection because even if you registered
for another name SNI hardcodes the object path, and you only get one of
those per connection. (something I'm not fond of in the spec, but eh)

> https://developer.gnome.org/gio/stable/gio-Owning-Bus-Names.html#g-bus-own-name
> That would be name_lost_handler with connection == NULL, right?

It would be a call to name_lost_handler, but the connection would be
valid as there was the ability to connect to the bus.

> So, are you ok with this change? with adding fallback to old behaviour if name registration fails?

Yes, as long as we don't have the requirement to get the name I'm find
with asking for it.

Unmerged revisions

275. By Alberts Muktupāvels on 2015-06-27

Fix build...

274. By Alberts Muktupāvels on 2015-06-26

Create StatusNotifierItem as service and use its name to register with watcher.

=== modified file 'src/app-indicator.c'
--- src/app-indicator.c	2014-11-10 09:14:07 +0000
+++ src/app-indicator.c	2015-06-27 17:40:16 +0000
@@ -67,7 +67,6 @@
 	/*< Private >*/
 	/* Properties */
 	gchar                *id;
-	gchar                *clean_id;
 	AppIndicatorCategory  category;
 	AppIndicatorStatus    status;
 	gchar                *icon_name;
@@ -91,8 +90,9 @@
 	/* Fun stuff */
 	GDBusProxy           *watcher_proxy;
 	GDBusConnection      *connection;
+	guint                 name_id;
 	guint                 dbus_registration;
-	gchar *               path;
+	gchar *               service;
 
 	/* Might be used */
 	IndicatorDesktopShortcuts * shorties;
@@ -152,9 +152,6 @@
 #define APP_INDICATOR_GET_PRIVATE(o) \
                              (G_TYPE_INSTANCE_GET_PRIVATE ((o), APP_INDICATOR_TYPE, AppIndicatorPrivate))
 
-/* Default Path */
-#define DEFAULT_ITEM_PATH   "/org/ayatana/NotificationItem"
-
 /* More constants */
 #define DEFAULT_FALLBACK_TIMER  100 /* in milliseconds */
 
@@ -584,7 +581,6 @@
 	AppIndicatorPrivate * priv = APP_INDICATOR_GET_PRIVATE(self);
 
 	priv->id = NULL;
-	priv->clean_id = NULL;
 	priv->category = APP_INDICATOR_CATEGORY_OTHER;
 	priv->status = APP_INDICATOR_STATUS_PASSIVE;
 	priv->icon_name = NULL;
@@ -600,8 +596,9 @@
 
 	priv->watcher_proxy = NULL;
 	priv->connection = NULL;
+	priv->name_id = 0;
 	priv->dbus_registration = 0;
-	priv->path = NULL;
+	priv->service = NULL;
 
 	priv->status_icon = NULL;
 	priv->fallback_timer = 0;
@@ -681,6 +678,11 @@
 		priv->dbus_registration = 0;
 	}
 
+	if (priv->name_id != 0) {
+		g_bus_unown_name(priv->name_id);
+		priv->name_id = 0;
+	}
+
 	if (priv->connection != NULL) {
 		g_object_unref(G_OBJECT(priv->connection));
 		priv->connection = NULL;
@@ -715,11 +717,6 @@
 		priv->id = NULL;
 	}
 
-	if (priv->clean_id != NULL) {
-		g_free(priv->clean_id);
-		priv->clean_id = NULL;
-	}
-
 	if (priv->icon_name != NULL) {
 		g_free(priv->icon_name);
 		priv->icon_name = NULL;
@@ -760,9 +757,9 @@
 		priv->att_accessible_desc = NULL;
 	}
 
-	if (priv->path != NULL) {
-		g_free(priv->path);
-		priv->path = NULL;
+	if (priv->service != NULL) {
+		g_free(priv->service);
+		priv->service = NULL;
 	}
 
 	G_OBJECT_CLASS (app_indicator_parent_class)->finalize (object);
@@ -788,14 +785,6 @@
 
           priv->id = g_strdup (g_value_get_string (value));
 
-          priv->clean_id = g_strdup(priv->id);
-          gchar * cleaner;
-          for (cleaner = priv->clean_id; *cleaner != '\0'; cleaner++) {
-            if (!g_ascii_isalnum(*cleaner)) {
-              *cleaner = '_';
-            }
-          }
-
           check_connect (self);
           break;
 
@@ -881,8 +870,8 @@
 			GError * error = NULL;
 
 			g_dbus_connection_emit_signal(self->priv->connection,
-										  NULL,
-										  self->priv->path,
+										  self->priv->service,
+										  NOTIFICATION_ITEM_DBUS_OBJ,
 										  NOTIFICATION_ITEM_DBUS_IFACE,
 										  "NewTitle",
 										  NULL,
@@ -1180,8 +1169,8 @@
 		GError * error = NULL;
 
 		g_dbus_connection_emit_signal(priv->connection,
-		                              NULL,
-		                              priv->path,
+		                              priv->service,
+		                              NOTIFICATION_ITEM_DBUS_OBJ,
 		                              NOTIFICATION_ITEM_DBUS_IFACE,
 		                              "XAyatanaNewLabel",
 		                              g_variant_new("(ss)", label, guide),
@@ -1214,48 +1203,25 @@
 	return;
 }
 
-/* This function is used to see if we have enough information to
-   connect to things.  If we do, and we're not connected, it
-   connects for us. */
 static void
-check_connect (AppIndicator *self)
+bus_acquired_handler (GDBusConnection *connection,
+                      const gchar     *name,
+                      gpointer         user_data)
 {
+	AppIndicator *self = (AppIndicator *) user_data;
 	AppIndicatorPrivate *priv = self->priv;
 
-	/* Do we have a connection? */
-	if (priv->connection == NULL) return;
-
-	/* If we already have a proxy, let's see if it has someone
-	   implementing it.  If not, we can't do much more than to
-	   do nothing. */
-	if (priv->watcher_proxy != NULL) {
-		gchar * name = g_dbus_proxy_get_name_owner(priv->watcher_proxy);
-		if (name == NULL) {
-			return;
-		}
-		g_free(name);
-	}
-
-	/* Do we have enough information? */
-	if (priv->menu == NULL) return;
-	if (priv->icon_name == NULL) return;
-	if (priv->id == NULL) return;
-
-	if (priv->path == NULL) {
-		priv->path = g_strdup_printf(DEFAULT_ITEM_PATH "/%s", priv->clean_id);
-	}
-
 	if (priv->dbus_registration == 0) {
 		GError * error = NULL;
 		priv->dbus_registration = g_dbus_connection_register_object(priv->connection,
-		                                                            priv->path,
+		                                                            NOTIFICATION_ITEM_DBUS_OBJ,
 		                                                            item_interface_info,
 		                                                            &item_interface_table,
 		                                                            self,
 		                                                            NULL,
 		                                                            &error);
 		if (error != NULL) {
-			g_warning("Unable to register object on path '%s': %s", priv->path, error->message);
+			g_warning("Unable to register object on path '%s': %s", NOTIFICATION_ITEM_DBUS_OBJ, error->message);
 			g_error_free(error);
 			return;
 		}
@@ -1282,6 +1248,50 @@
 	} else {
 		bus_watcher_ready(NULL, NULL, self);
 	}
+}
+
+/* This function is used to see if we have enough information to
+   connect to things.  If we do, and we're not connected, it
+   connects for us. */
+static void
+check_connect (AppIndicator *self)
+{
+	AppIndicatorPrivate *priv = self->priv;
+	static guint id = 0;
+
+	/* Do we have a connection? */
+	if (priv->connection == NULL) return;
+
+	/* If we already have a proxy, let's see if it has someone
+	   implementing it.  If not, we can't do much more than to
+	   do nothing. */
+	if (priv->watcher_proxy != NULL) {
+		gchar * name = g_dbus_proxy_get_name_owner(priv->watcher_proxy);
+		if (name == NULL) {
+			return;
+		}
+		g_free(name);
+	}
+
+	/* Do we have enough information? */
+	if (priv->menu == NULL) return;
+	if (priv->icon_name == NULL) return;
+	if (priv->id == NULL) return;
+
+	if (priv->service == NULL) {
+		priv->service = g_strdup_printf(NOTIFICATION_ITEM_DBUS_ADDR "-%d-%d", getpid(), id++);
+	}
+
+	if (priv->name_id == 0) {
+		priv->name_id = g_bus_own_name (G_BUS_TYPE_SESSION,
+		                                priv->service,
+		                                G_BUS_NAME_OWNER_FLAGS_NONE,
+		                                (GBusAcquiredCallback) bus_acquired_handler,
+		                                NULL,
+		                                NULL,
+		                                self,
+		                                NULL);
+	}
 
 	return;
 }
@@ -1338,7 +1348,7 @@
 
 	g_dbus_proxy_call(app->priv->watcher_proxy,
 	                  "RegisterStatusNotifierItem",
-	                  g_variant_new("(s)", app->priv->path),
+	                  g_variant_new("(s)", app->priv->service),
 	                  G_DBUS_CALL_FLAGS_NONE,
 					  -1,
 	                  NULL, /* cancelable */
@@ -1499,8 +1509,8 @@
 		GError * error = NULL;
 
 		g_dbus_connection_emit_signal(priv->connection,
-		                              NULL,
-		                              priv->path,
+		                              priv->service,
+		                              NOTIFICATION_ITEM_DBUS_OBJ,
 		                              NOTIFICATION_ITEM_DBUS_IFACE,
 		                              "NewIcon",
 		                              NULL,
@@ -1837,8 +1847,8 @@
 			GError * error = NULL;
 
 			g_dbus_connection_emit_signal(self->priv->connection,
-										  NULL,
-										  self->priv->path,
+										  self->priv->service,
+										  NOTIFICATION_ITEM_DBUS_OBJ,
 										  NOTIFICATION_ITEM_DBUS_IFACE,
 										  "NewStatus",
 										  g_variant_new("(s)", value->value_nick),
@@ -1904,8 +1914,8 @@
 			GError * error = NULL;
 
 			g_dbus_connection_emit_signal(self->priv->connection,
-			                              NULL,
-			                              self->priv->path,
+			                              self->priv->service,
+			                              NOTIFICATION_ITEM_DBUS_OBJ,
 			                              NOTIFICATION_ITEM_DBUS_IFACE,
 			                              "NewAttentionIcon",
 			                              NULL,
@@ -1982,8 +1992,8 @@
 			GError * error = NULL;
 
 			g_dbus_connection_emit_signal(self->priv->connection,
-										  NULL,
-										  self->priv->path,
+										  self->priv->service,
+										  NOTIFICATION_ITEM_DBUS_OBJ,
 										  NOTIFICATION_ITEM_DBUS_IFACE,
 										  "NewIcon",
 										  NULL,
@@ -2048,8 +2058,8 @@
 			GError * error = NULL;
 
 			g_dbus_connection_emit_signal(self->priv->connection,
-										  NULL,
-										  self->priv->path,
+										  self->priv->service,
+										  NOTIFICATION_ITEM_DBUS_OBJ,
 										  NOTIFICATION_ITEM_DBUS_IFACE,
 										  "NewIconThemePath",
 										  g_variant_new("(s)", self->priv->icon_theme_path),
@@ -2081,9 +2091,7 @@
 	}
 
 	if (priv->menuservice == NULL) {
-		gchar * path = g_strdup_printf(DEFAULT_ITEM_PATH "/%s/Menu", priv->clean_id);
-		priv->menuservice = dbusmenu_server_new (path);
-		g_free(path);
+		priv->menuservice = dbusmenu_server_new (NOTIFICATION_ITEM_DBUS_OBJ "/Menu");
 	}
 
 	dbusmenu_server_set_root (priv->menuservice, root);
@@ -2114,7 +2122,6 @@
 
   g_return_if_fail (IS_APP_INDICATOR (self));
   g_return_if_fail (GTK_IS_MENU (menu));
-  g_return_if_fail (self->priv->clean_id != NULL);
 
   priv = self->priv;
 
@@ -2528,9 +2535,7 @@
 
 	/* Swap it if needed */
 	if (priv->menuservice == NULL) {
-		gchar * path = g_strdup_printf(DEFAULT_ITEM_PATH "/%s/Menu", priv->clean_id);
-		priv->menuservice = dbusmenu_server_new (path);
-		g_free(path);
+		priv->menuservice = dbusmenu_server_new (NOTIFICATION_ITEM_DBUS_OBJ "/Menu");
 	}
 
 	dbusmenu_server_set_root (priv->menuservice, root);
@@ -2541,4 +2546,4 @@
 	}
 
 	return;
-}
+}
 \ No newline at end of file
 
=== modified file 'src/dbus-shared.h'
--- src/dbus-shared.h	2011-01-14 02:05:12 +0000
+++ src/dbus-shared.h	2015-06-27 17:40:16 +0000
@@ -28,8 +28,9 @@
 #define NOTIFICATION_WATCHER_DBUS_OBJ     "/StatusNotifierWatcher"
 #define NOTIFICATION_WATCHER_DBUS_IFACE   "org.kde.StatusNotifierWatcher"
 
+#define NOTIFICATION_ITEM_DBUS_ADDR       "org.kde.StatusNotifierItem"
+#define NOTIFICATION_ITEM_DBUS_OBJ        "/StatusNotifierItem"
 #define NOTIFICATION_ITEM_DBUS_IFACE      "org.kde.StatusNotifierItem"
-#define NOTIFICATION_ITEM_DEFAULT_OBJ     "/StatusNotifierItem"
 
 #define NOTIFICATION_APPROVER_DBUS_IFACE  "org.ayatana.StatusNotifierApprover"