On Fri, 2013-01-25 at 10:58 +0000, Lars Uebernickel wrote: > > * When the service vanishes the program is immediately hiding the indicator > > and destroying everything. It doesn't make much sense to show this error to > > the user unless it can't be restarted. It should try to restart, and if > > either a timeout happens or an error on restart happens it hides the > > indicator. Much better user experience if we can handle the error in a > > reasonable way. > > 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. > > * I don't understand that, if we only have one action group, the action names > > need the "indicator" prefix. It seems they could just be unprefixed and make > > it easier on everyone. > > 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. 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 :-) > > * In indicator_ng_initable_init() you should really check for the group before > > looking at individual values in the group. It makes the errors much better > > for people trying to figure out WTH is going on when they mess up the file. > > Good point. I started to implement it when I realised that > g_key_file_get_string() already returns an error when the group > doesn't exist: > > could not load indicator from : Key file does not have group 'Indicator Service' > > I don't know what else a special check could add. Ah, okay. That should be fine then. Didn't realize it gave that error. > > * In indicator_ng_initable_init() it seems like name and the bus name check > > should be separate, they're pretty independent and again it'll help error > > reporting. > > 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. > > * It looks like the name that is gotten from the Key file is not going through > > translations, which it needs to as it is a user visible string. This also > > means that the indicator file will need to give the translation domain. > > 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. > > * I don't understand why the profile is a string and not an enum. It seems > > like there'll always be a limited set, and they can't overlap, so enum is the > > right type to use. Seems like this should be passed up to the "environments" > > of indicator-object. > > Oh! I meant to look into "environments", but forgot. Maybe we don't > even need a separate "profile" property at all. > > How exactly do these work? Does unity-panrl-service already create an > indicator-object for each "environment" (greeter, desktop)? I don't think that u-p-s uses that today, but that was the idea. So, I think there's room to change it. We just need to talk about how. > > * We probably need a way for a .indicator file to support multiple profiles in > > the same file. It doesn't really make sense to have a whole set of files > > since all of them will have to be loaded for the convergence story. > > That's already the way it works: each indicator-service can expose as > many profiles as it wants to on �, where > is from the .indicator file. Ah, I see. I missed that. > > * The indicator needs a way to provide a string that can be used for measuring > > the space needed. We provide that in the libappindicator API and we should > > honor it further up the stack. > > 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. > > * In the new interfacing there is no way to check the version of the service. > > The scenario would be that an upgrade happens, and the service no longer > > matches the version that the panel-service thinks it's looking for. It needs > > to handle that error instead of displaying erroneous results or an empty menu. > > I'd recommend adding this to the indicator file format and as an attribute on > > the base menu. > > 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. > > * It seems like the about-to-show needs to be passed down to the indicator > > somehow. This isn't currently handled and it's required for things like the > > network indicator and datetime indicator. > > Yes, this is something on my TODO list. It will probably be the same > protocol that indicator-appmenu is already using. > > > * I don't see any way that there is to pass things like middle click and > > scroll wheel from the indicator down to the service. This is used in the > > sound indicator and messaging indicator. > > Also on my TODO list :) > > I'm not entirely sure yet, but I think the best way to handle this > would be to activate the action of the root item with a parameter that > signifies which button was pressed. Okay, I can see not having this merge blocked on that, but we should file bugs to track regressions.