Code review comment for lp:~larsu/libindicator/add-indicator-ng

> > Right, this isn't true for all indicators though. How about a
> > "auto-reload" property that is set for all built-in indicators (or
> > even settable from the .indicator file)?
>
> Uhm, which ones don't? That would be bad if there were some that don't.
> Remember that we only want to use this for system indicators, not
> applications. Because we need to wait to display the panel until all of
> the services that have .indicator files are started and giving us
> default state. We can't assume that for appindicators.

Matthew is thinking about having the sync indicator only show up when it is syncing. The (now deprecated) print indicator only shows when printing. Similar for keyboard / bluetooth, alhough they would probably only hide their indicators instead of terminating the service.

I definitely want to use the same mechanism for appindicators.

> > I want to enable applications to reuse their app. actions in an
> > indicator. Without this distinction, apps would have to add the same
> > actions twice.
>
> Are you thinking appindicators here? I don't think we can drop KSNI
> there, so we won't be able to use this for that. Otherwise all
> indicators don't have any application actions.

An application having an indicator is pretty common. I think it would be much easier for them if the could just hook up existing actions to the indicator menu, in the same way that they hook up those actions to the application menu.

I'm not saying we should drop support for KSNI. Though I think that spec needs to be updated (it depends on dbusmenu, doesn't it?)

> Really application actions will probably only be used long term to
> support GNOME applications on the desktop. They've shown no desire to
> work with us (and KDE) on indicators in the past, I doubt they will in
> the future. So never the two shall meet :-)

Hm? Why would a $TOOLKIT application not have application actions? I thought application menus were part of our platform?

What do you mean by GNOME applications? Apps using GNOME tech? Lots of those use indicators and go out of their way to support the Unity desktop (tomboy, transmission, shutter, sparkleshare). Also note that even though the indicator concept wasn't picked up by GNOME's default applications, there are features in GTK+ and GMenuModel that are *only* used in Unity's indicators.

I'm really just trying to make it as easy as possible for application developers to add an indicator to their app and hook it up to existing actions (see also my libunity merge request).

> [...]
> > On second look, this is indeed a bit convoluted. But I'm not sure
> > what you mean: move the g_bus_watch_name call out of the condition, or
> > out of the function? It doesn't have any error conditions (I admit
> > the "return self->name_watch_id > 0" is a bit misleading).
>
> I was more thinking of breaking up this if statement into to:
>
> 515 + if ((self->name = g_key_file_get_string (keyfile, "Indicator
> Service", "Name", error)) &&
> 516 + (bus_name = g_key_file_get_string (keyfile, "Indicator Service",
> "BusName", error)) &&
> 517 + (self->object_path = g_key_file_get_string (keyfile, "Indicator
> Service", "ObjectPath", error)))
>
> It seems like the Name property is independent of BusName and
> ObjectPath.

Fair point. Will do.

> > The name from the key file the name-hint really, which isn't
> > user-visible, is it? Would it be clearer if we named the key
> > "NameHint" instead?
>
> There needs to be two, though I'm not sure the old protocol has done
> this properly. We need one to show when doing the large text on the
> phone and for prefacing the data in the HUD, and we need the ID as well.

Agreed. But I'm not sure if the visible name shouldn't be part of the action state instead, or is that name always static?

By the way: we're using the indicator's label for the big phone text right now. That probably wouldn't be the right choince for the HUD prefix, though.

> [...]
> > You mean AppIndicator.label-guide, right? Neat feature, I didn't know
> > about it. I suggest adding this as a separate attribute on the root
> > menu item (since it won't change). Does "x-canonical-label-guide"
> > sound right?
>
> Well, it can change. Everything can, assuming that they won't is just a
> way to avoid hard synchronization problems :-) Where the label guide
> changes is based on settings. For instance someone turns on the "show
> seconds" setting of the datetime indicator. In libappindicator it can
> be change every time someone sets the label, but I don't know of anyone
> that does do that.

Fair point, it should be part of the action state then.

> > Hm, I thought the .indicator files will be in the same package as the
> > indicator services, so the panel would never connect to "old"
> > indicator services.
>
> Sure, but the unity-panel-service can be running and the indicator gets
> upgraded. We don't force the user to login�out on upgrades.

Right, but when the indicator gets upgraded, u-p-s would notice the indicator service on the bus (given that the service restarted) and show the new menu. What would it do differently when it knew that this menu is older than the one it showed previously?

> [...]
> Okay, I can see not having this merge blocked on that, but we should
> file bugs to track regressions.

Agreed. I will file those bugs as soon as this is merged.