Merge lp:~muktupavels/libappindicator/create-as-service into lp:libappindicator/15.10
- create-as-service
- Merge into trunk.15.10
Status: | Needs review |
---|---|
Proposed branch: | lp:~muktupavels/libappindicator/create-as-service |
Merge into: | lp:libappindicator/15.10 |
Diff against target: |
346 lines (+82/-76) 2 files modified
src/app-indicator.c (+80/-75) src/dbus-shared.h (+2/-1) |
To merge this branch: | bzr merge lp:~muktupavels/libappindicator/create-as-service |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Ted Gould (community) | Needs Information | ||
Dmitry Shachnev | Approve | ||
Review via email: mp+263108@code.launchpad.net |
Commit message
Follow spec more closely. "RegisterStatus
Description of the change
Follow spec more closely. "RegisterStatus
Alberts Muktupāvels (muktupavels) wrote : | # |
> Unfortunately it fails to build because NOTIFICATION_
> defined:
> http://
> .build
Previous commit was missing one file... I hope that now it builds.
Dmitry Shachnev (mitya57) wrote : | # |
Thanks! It builds now. I tested with remmina and transmission, everything seems to work as before.
Ted Gould (ted) wrote : | # |
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.
Alberts Muktupāvels (muktupavels) 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://
"Register a StatusNotifierItem into the StatusNotifierW
> 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:/
Did not understand part about not allowing name registrations... :(
Ted Gould (ted) wrote : | # |
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://
> "Register a StatusNotifierItem into the StatusNotifierW
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:/
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) wrote : | # |
> 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://
> fierWatcher/
> > "Register a StatusNotifierItem into the StatusNotifierW
> of its full name on the session bus, for instance
> org.freedesktop
>
>
> 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:/
> /libstatus-
>
>
>
> 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) wrote : | # |
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://
> > 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) wrote : | # |
> 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://
> 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 StatusNotifierW
Is not unique name meant to be org.freedesktop
RegisterStatusI
"Register a StatusNotifierItem into the StatusNotifierW
> > > 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:/
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) wrote : | # |
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 StatusNotifierW
>
> Is not unique name meant to be org.freedesktop
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://
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:/
> 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
-
Fix build...
- 274. By Alberts Muktupāvels
-
Create StatusNotifierItem as service and use its name to register with watcher.
Preview Diff
1 | === modified file 'src/app-indicator.c' |
2 | --- src/app-indicator.c 2014-11-10 09:14:07 +0000 |
3 | +++ src/app-indicator.c 2015-06-27 17:40:16 +0000 |
4 | @@ -67,7 +67,6 @@ |
5 | /*< Private >*/ |
6 | /* Properties */ |
7 | gchar *id; |
8 | - gchar *clean_id; |
9 | AppIndicatorCategory category; |
10 | AppIndicatorStatus status; |
11 | gchar *icon_name; |
12 | @@ -91,8 +90,9 @@ |
13 | /* Fun stuff */ |
14 | GDBusProxy *watcher_proxy; |
15 | GDBusConnection *connection; |
16 | + guint name_id; |
17 | guint dbus_registration; |
18 | - gchar * path; |
19 | + gchar * service; |
20 | |
21 | /* Might be used */ |
22 | IndicatorDesktopShortcuts * shorties; |
23 | @@ -152,9 +152,6 @@ |
24 | #define APP_INDICATOR_GET_PRIVATE(o) \ |
25 | (G_TYPE_INSTANCE_GET_PRIVATE ((o), APP_INDICATOR_TYPE, AppIndicatorPrivate)) |
26 | |
27 | -/* Default Path */ |
28 | -#define DEFAULT_ITEM_PATH "/org/ayatana/NotificationItem" |
29 | - |
30 | /* More constants */ |
31 | #define DEFAULT_FALLBACK_TIMER 100 /* in milliseconds */ |
32 | |
33 | @@ -584,7 +581,6 @@ |
34 | AppIndicatorPrivate * priv = APP_INDICATOR_GET_PRIVATE(self); |
35 | |
36 | priv->id = NULL; |
37 | - priv->clean_id = NULL; |
38 | priv->category = APP_INDICATOR_CATEGORY_OTHER; |
39 | priv->status = APP_INDICATOR_STATUS_PASSIVE; |
40 | priv->icon_name = NULL; |
41 | @@ -600,8 +596,9 @@ |
42 | |
43 | priv->watcher_proxy = NULL; |
44 | priv->connection = NULL; |
45 | + priv->name_id = 0; |
46 | priv->dbus_registration = 0; |
47 | - priv->path = NULL; |
48 | + priv->service = NULL; |
49 | |
50 | priv->status_icon = NULL; |
51 | priv->fallback_timer = 0; |
52 | @@ -681,6 +678,11 @@ |
53 | priv->dbus_registration = 0; |
54 | } |
55 | |
56 | + if (priv->name_id != 0) { |
57 | + g_bus_unown_name(priv->name_id); |
58 | + priv->name_id = 0; |
59 | + } |
60 | + |
61 | if (priv->connection != NULL) { |
62 | g_object_unref(G_OBJECT(priv->connection)); |
63 | priv->connection = NULL; |
64 | @@ -715,11 +717,6 @@ |
65 | priv->id = NULL; |
66 | } |
67 | |
68 | - if (priv->clean_id != NULL) { |
69 | - g_free(priv->clean_id); |
70 | - priv->clean_id = NULL; |
71 | - } |
72 | - |
73 | if (priv->icon_name != NULL) { |
74 | g_free(priv->icon_name); |
75 | priv->icon_name = NULL; |
76 | @@ -760,9 +757,9 @@ |
77 | priv->att_accessible_desc = NULL; |
78 | } |
79 | |
80 | - if (priv->path != NULL) { |
81 | - g_free(priv->path); |
82 | - priv->path = NULL; |
83 | + if (priv->service != NULL) { |
84 | + g_free(priv->service); |
85 | + priv->service = NULL; |
86 | } |
87 | |
88 | G_OBJECT_CLASS (app_indicator_parent_class)->finalize (object); |
89 | @@ -788,14 +785,6 @@ |
90 | |
91 | priv->id = g_strdup (g_value_get_string (value)); |
92 | |
93 | - priv->clean_id = g_strdup(priv->id); |
94 | - gchar * cleaner; |
95 | - for (cleaner = priv->clean_id; *cleaner != '\0'; cleaner++) { |
96 | - if (!g_ascii_isalnum(*cleaner)) { |
97 | - *cleaner = '_'; |
98 | - } |
99 | - } |
100 | - |
101 | check_connect (self); |
102 | break; |
103 | |
104 | @@ -881,8 +870,8 @@ |
105 | GError * error = NULL; |
106 | |
107 | g_dbus_connection_emit_signal(self->priv->connection, |
108 | - NULL, |
109 | - self->priv->path, |
110 | + self->priv->service, |
111 | + NOTIFICATION_ITEM_DBUS_OBJ, |
112 | NOTIFICATION_ITEM_DBUS_IFACE, |
113 | "NewTitle", |
114 | NULL, |
115 | @@ -1180,8 +1169,8 @@ |
116 | GError * error = NULL; |
117 | |
118 | g_dbus_connection_emit_signal(priv->connection, |
119 | - NULL, |
120 | - priv->path, |
121 | + priv->service, |
122 | + NOTIFICATION_ITEM_DBUS_OBJ, |
123 | NOTIFICATION_ITEM_DBUS_IFACE, |
124 | "XAyatanaNewLabel", |
125 | g_variant_new("(ss)", label, guide), |
126 | @@ -1214,48 +1203,25 @@ |
127 | return; |
128 | } |
129 | |
130 | -/* This function is used to see if we have enough information to |
131 | - connect to things. If we do, and we're not connected, it |
132 | - connects for us. */ |
133 | static void |
134 | -check_connect (AppIndicator *self) |
135 | +bus_acquired_handler (GDBusConnection *connection, |
136 | + const gchar *name, |
137 | + gpointer user_data) |
138 | { |
139 | + AppIndicator *self = (AppIndicator *) user_data; |
140 | AppIndicatorPrivate *priv = self->priv; |
141 | |
142 | - /* Do we have a connection? */ |
143 | - if (priv->connection == NULL) return; |
144 | - |
145 | - /* If we already have a proxy, let's see if it has someone |
146 | - implementing it. If not, we can't do much more than to |
147 | - do nothing. */ |
148 | - if (priv->watcher_proxy != NULL) { |
149 | - gchar * name = g_dbus_proxy_get_name_owner(priv->watcher_proxy); |
150 | - if (name == NULL) { |
151 | - return; |
152 | - } |
153 | - g_free(name); |
154 | - } |
155 | - |
156 | - /* Do we have enough information? */ |
157 | - if (priv->menu == NULL) return; |
158 | - if (priv->icon_name == NULL) return; |
159 | - if (priv->id == NULL) return; |
160 | - |
161 | - if (priv->path == NULL) { |
162 | - priv->path = g_strdup_printf(DEFAULT_ITEM_PATH "/%s", priv->clean_id); |
163 | - } |
164 | - |
165 | if (priv->dbus_registration == 0) { |
166 | GError * error = NULL; |
167 | priv->dbus_registration = g_dbus_connection_register_object(priv->connection, |
168 | - priv->path, |
169 | + NOTIFICATION_ITEM_DBUS_OBJ, |
170 | item_interface_info, |
171 | &item_interface_table, |
172 | self, |
173 | NULL, |
174 | &error); |
175 | if (error != NULL) { |
176 | - g_warning("Unable to register object on path '%s': %s", priv->path, error->message); |
177 | + g_warning("Unable to register object on path '%s': %s", NOTIFICATION_ITEM_DBUS_OBJ, error->message); |
178 | g_error_free(error); |
179 | return; |
180 | } |
181 | @@ -1282,6 +1248,50 @@ |
182 | } else { |
183 | bus_watcher_ready(NULL, NULL, self); |
184 | } |
185 | +} |
186 | + |
187 | +/* This function is used to see if we have enough information to |
188 | + connect to things. If we do, and we're not connected, it |
189 | + connects for us. */ |
190 | +static void |
191 | +check_connect (AppIndicator *self) |
192 | +{ |
193 | + AppIndicatorPrivate *priv = self->priv; |
194 | + static guint id = 0; |
195 | + |
196 | + /* Do we have a connection? */ |
197 | + if (priv->connection == NULL) return; |
198 | + |
199 | + /* If we already have a proxy, let's see if it has someone |
200 | + implementing it. If not, we can't do much more than to |
201 | + do nothing. */ |
202 | + if (priv->watcher_proxy != NULL) { |
203 | + gchar * name = g_dbus_proxy_get_name_owner(priv->watcher_proxy); |
204 | + if (name == NULL) { |
205 | + return; |
206 | + } |
207 | + g_free(name); |
208 | + } |
209 | + |
210 | + /* Do we have enough information? */ |
211 | + if (priv->menu == NULL) return; |
212 | + if (priv->icon_name == NULL) return; |
213 | + if (priv->id == NULL) return; |
214 | + |
215 | + if (priv->service == NULL) { |
216 | + priv->service = g_strdup_printf(NOTIFICATION_ITEM_DBUS_ADDR "-%d-%d", getpid(), id++); |
217 | + } |
218 | + |
219 | + if (priv->name_id == 0) { |
220 | + priv->name_id = g_bus_own_name (G_BUS_TYPE_SESSION, |
221 | + priv->service, |
222 | + G_BUS_NAME_OWNER_FLAGS_NONE, |
223 | + (GBusAcquiredCallback) bus_acquired_handler, |
224 | + NULL, |
225 | + NULL, |
226 | + self, |
227 | + NULL); |
228 | + } |
229 | |
230 | return; |
231 | } |
232 | @@ -1338,7 +1348,7 @@ |
233 | |
234 | g_dbus_proxy_call(app->priv->watcher_proxy, |
235 | "RegisterStatusNotifierItem", |
236 | - g_variant_new("(s)", app->priv->path), |
237 | + g_variant_new("(s)", app->priv->service), |
238 | G_DBUS_CALL_FLAGS_NONE, |
239 | -1, |
240 | NULL, /* cancelable */ |
241 | @@ -1499,8 +1509,8 @@ |
242 | GError * error = NULL; |
243 | |
244 | g_dbus_connection_emit_signal(priv->connection, |
245 | - NULL, |
246 | - priv->path, |
247 | + priv->service, |
248 | + NOTIFICATION_ITEM_DBUS_OBJ, |
249 | NOTIFICATION_ITEM_DBUS_IFACE, |
250 | "NewIcon", |
251 | NULL, |
252 | @@ -1837,8 +1847,8 @@ |
253 | GError * error = NULL; |
254 | |
255 | g_dbus_connection_emit_signal(self->priv->connection, |
256 | - NULL, |
257 | - self->priv->path, |
258 | + self->priv->service, |
259 | + NOTIFICATION_ITEM_DBUS_OBJ, |
260 | NOTIFICATION_ITEM_DBUS_IFACE, |
261 | "NewStatus", |
262 | g_variant_new("(s)", value->value_nick), |
263 | @@ -1904,8 +1914,8 @@ |
264 | GError * error = NULL; |
265 | |
266 | g_dbus_connection_emit_signal(self->priv->connection, |
267 | - NULL, |
268 | - self->priv->path, |
269 | + self->priv->service, |
270 | + NOTIFICATION_ITEM_DBUS_OBJ, |
271 | NOTIFICATION_ITEM_DBUS_IFACE, |
272 | "NewAttentionIcon", |
273 | NULL, |
274 | @@ -1982,8 +1992,8 @@ |
275 | GError * error = NULL; |
276 | |
277 | g_dbus_connection_emit_signal(self->priv->connection, |
278 | - NULL, |
279 | - self->priv->path, |
280 | + self->priv->service, |
281 | + NOTIFICATION_ITEM_DBUS_OBJ, |
282 | NOTIFICATION_ITEM_DBUS_IFACE, |
283 | "NewIcon", |
284 | NULL, |
285 | @@ -2048,8 +2058,8 @@ |
286 | GError * error = NULL; |
287 | |
288 | g_dbus_connection_emit_signal(self->priv->connection, |
289 | - NULL, |
290 | - self->priv->path, |
291 | + self->priv->service, |
292 | + NOTIFICATION_ITEM_DBUS_OBJ, |
293 | NOTIFICATION_ITEM_DBUS_IFACE, |
294 | "NewIconThemePath", |
295 | g_variant_new("(s)", self->priv->icon_theme_path), |
296 | @@ -2081,9 +2091,7 @@ |
297 | } |
298 | |
299 | if (priv->menuservice == NULL) { |
300 | - gchar * path = g_strdup_printf(DEFAULT_ITEM_PATH "/%s/Menu", priv->clean_id); |
301 | - priv->menuservice = dbusmenu_server_new (path); |
302 | - g_free(path); |
303 | + priv->menuservice = dbusmenu_server_new (NOTIFICATION_ITEM_DBUS_OBJ "/Menu"); |
304 | } |
305 | |
306 | dbusmenu_server_set_root (priv->menuservice, root); |
307 | @@ -2114,7 +2122,6 @@ |
308 | |
309 | g_return_if_fail (IS_APP_INDICATOR (self)); |
310 | g_return_if_fail (GTK_IS_MENU (menu)); |
311 | - g_return_if_fail (self->priv->clean_id != NULL); |
312 | |
313 | priv = self->priv; |
314 | |
315 | @@ -2528,9 +2535,7 @@ |
316 | |
317 | /* Swap it if needed */ |
318 | if (priv->menuservice == NULL) { |
319 | - gchar * path = g_strdup_printf(DEFAULT_ITEM_PATH "/%s/Menu", priv->clean_id); |
320 | - priv->menuservice = dbusmenu_server_new (path); |
321 | - g_free(path); |
322 | + priv->menuservice = dbusmenu_server_new (NOTIFICATION_ITEM_DBUS_OBJ "/Menu"); |
323 | } |
324 | |
325 | dbusmenu_server_set_root (priv->menuservice, root); |
326 | @@ -2541,4 +2546,4 @@ |
327 | } |
328 | |
329 | return; |
330 | -} |
331 | +} |
332 | \ No newline at end of file |
333 | |
334 | === modified file 'src/dbus-shared.h' |
335 | --- src/dbus-shared.h 2011-01-14 02:05:12 +0000 |
336 | +++ src/dbus-shared.h 2015-06-27 17:40:16 +0000 |
337 | @@ -28,8 +28,9 @@ |
338 | #define NOTIFICATION_WATCHER_DBUS_OBJ "/StatusNotifierWatcher" |
339 | #define NOTIFICATION_WATCHER_DBUS_IFACE "org.kde.StatusNotifierWatcher" |
340 | |
341 | +#define NOTIFICATION_ITEM_DBUS_ADDR "org.kde.StatusNotifierItem" |
342 | +#define NOTIFICATION_ITEM_DBUS_OBJ "/StatusNotifierItem" |
343 | #define NOTIFICATION_ITEM_DBUS_IFACE "org.kde.StatusNotifierItem" |
344 | -#define NOTIFICATION_ITEM_DEFAULT_OBJ "/StatusNotifierItem" |
345 | |
346 | #define NOTIFICATION_APPROVER_DBUS_IFACE "org.ayatana.StatusNotifierApprover" |
347 |
Unfortunately it fails to build because NOTIFICATION_ ITEM_DBUS_ OBJ is not defined: mitya57. me/builds/ libappindicator _12.10. 1+15.04. 20141110- 0ubuntu2_ amd64.build
http://