GNOME Bugzilla – Bug 606667
Support for application-indicators/StatusNotifierIcon
Last modified: 2011-01-18 17:51:51 UTC
We would like to support application-indicators for gnome-bluetooth as proposed at this page: https://wiki.ubuntu.com/DesktopExperienceTeam/ApplicationIndicators The Canonical DX team will provide a patch, but the work has not started, so I wanted to open this bug as a placeholder and to get feedback on the feature, thanks!
Putting on NEEDINFO, as there's been no patches provided.
Created attachment 157025 [details] [review] Support for Application Indicators against git head (commit da7d9b847b0c66ecbb34ff8ba400caac9b4d445d)
Attached is a patch providing support for application indicators. The changes were discussed on the following Launchpad bug: https://bugs.launchpad.net/ubuntu/+source/gnome-bluetooth/+bug/497856 The attachment is a patch against gnome git head. The patch included in the latest version of Ubuntu Lucid is against Gnome Bluetooth version 2.29.91, which I then rebased onto git head to create this patch. Any and all feedback and suggestions to make this more accepted for upstream inclusion is welcome. Please don't hesitate to contact myself or Jorge with questions or concerns. Thanks!
Review of attachment 157025 [details] [review]: The whole of the GtkStatusIcon vs. AppIndicator changes should live in notify.c, there shouldn't be any ifdef leaking into other parts of the code. ::: applet/notify.c @@ +126,3 @@ + + /* TODO: Change this to an appropriate icon. */ + app_indicator_set_attention_icon (indicator, "bluetooth-manager"); Need to fix this obviously. @@ +170,3 @@ if (statusicon != NULL) gtk_status_icon_set_visible(statusicon, TRUE); +#endif /* HAVE_APP_INDICATOR */ What's the point of ifdef'ing inside the function for 2 lines? @@ +201,3 @@ +#ifdef HAVE_APP_INDICATOR + app_indicator_set_icon(indicator, name); + /* NOTE: I chose not to use the tooltip info because that information is already displayed in the menu. Who's "I"? @@ +206,3 @@ gtk_status_icon_set_from_icon_name (statusicon, name); gtk_status_icon_set_tooltip_markup(statusicon, _tooltip); +#endif /* HAVE_APP_INDICATOR */ The function is unreadable in the end. Make your own version and ifdef it. ::: configure.ac @@ +92,3 @@ +dnl Requires for application indicators +APPINDICATOR_REQUIRED=0.0.7 Put at the same level as the other versions.
I should also mention that this patch needs to be on top of the patch in bug 613717, as I'll commit it as soon as I branch.
Thanks for the feedback. Comments and questions below: (In reply to comment #4) > Review of attachment 157025 [details] [review]: > > The whole of the GtkStatusIcon vs. AppIndicator changes should live in > notify.c, there shouldn't be any ifdef leaking into other parts of the code. The only problem is that the current implementation has the call to the function that initializes the status icon in the main function main.c. This is done so the proper signals can be hooked up for showing the menu, which is defined in main.c. Since AppIndicator is meant to replace the GtkStatusIcon, and also needs the menu hooked up, I'm not sure how to avoid editing main.c. > ::: applet/notify.c > @@ +126,3 @@ > + > + /* TODO: Change this to an appropriate icon. */ > + app_indicator_set_attention_icon (indicator, "bluetooth-manager"); > > Need to fix this obviously. > > @@ +170,3 @@ > if (statusicon != NULL) > gtk_status_icon_set_visible(statusicon, TRUE); > +#endif /* HAVE_APP_INDICATOR */ > > What's the point of ifdef'ing inside the function for 2 lines? By this do you mean I should just ifdef the entire function and make my own copy? > @@ +201,3 @@ > +#ifdef HAVE_APP_INDICATOR > + app_indicator_set_icon(indicator, name); > + /* NOTE: I chose not to use the tooltip info because that information > is already displayed in the menu. > > Who's "I"? Sorry, used to using first person in comments in my own personal code. Probably better to not have the comment or reword it like so: "Not using the tooltip because that information is already displayed in the menu"? > @@ +206,3 @@ > gtk_status_icon_set_from_icon_name (statusicon, name); > gtk_status_icon_set_tooltip_markup(statusicon, _tooltip); > +#endif /* HAVE_APP_INDICATOR */ > > The function is unreadable in the end. Make your own version and ifdef it. I agree. I originally did it this way to avoid duplication of the code that was in common, but agree that the amount of duplication is not worth avoiding for the readability. > ::: configure.ac > @@ +92,3 @@ > > +dnl Requires for application indicators > +APPINDICATOR_REQUIRED=0.0.7 > > Put at the same level as the other versions. At the same level, do you mean move this constant up to where the earlier *_REQUIRED constants are defined?
(In reply to comment #6) > Thanks for the feedback. Comments and questions below: > > (In reply to comment #4) > > Review of attachment 157025 [details] [review] [details]: > > > > The whole of the GtkStatusIcon vs. AppIndicator changes should live in > > notify.c, there shouldn't be any ifdef leaking into other parts of the code. > > The only problem is that the current implementation has the call to > the function that initializes the status icon in the main function > main.c. This is done so the proper signals can be hooked up for > showing the menu, which is defined in main.c. Since AppIndicator is > meant to replace the GtkStatusIcon, and also needs the menu hooked up, > I'm not sure how to avoid editing main.c. You can use something like g_signal_lookup() to see whether those signals are available. > > ::: applet/notify.c > > @@ +126,3 @@ > > + > > + /* TODO: Change this to an appropriate icon. */ > > + app_indicator_set_attention_icon (indicator, "bluetooth-manager"); > > > > Need to fix this obviously. > > > > @@ +170,3 @@ > > if (statusicon != NULL) > > gtk_status_icon_set_visible(statusicon, TRUE); > > +#endif /* HAVE_APP_INDICATOR */ > > > > What's the point of ifdef'ing inside the function for 2 lines? > > By this do you mean I should just ifdef the entire function and make > my own copy? Yes please. > > @@ +201,3 @@ > > +#ifdef HAVE_APP_INDICATOR > > + app_indicator_set_icon(indicator, name); > > + /* NOTE: I chose not to use the tooltip info because that information > > is already displayed in the menu. > > > > Who's "I"? > > Sorry, used to using first person in comments in my own personal > code. Probably better to not have the comment or reword it like so: > "Not using the tooltip because that information is already displayed > in the menu"? Yep. Is the menu really never ellipsised? What happens for very very long device names? (up to 256 bytes, so could pack quite a few ASCII characters in there) > > @@ +206,3 @@ > > gtk_status_icon_set_from_icon_name (statusicon, name); > > gtk_status_icon_set_tooltip_markup(statusicon, _tooltip); > > +#endif /* HAVE_APP_INDICATOR */ > > > > The function is unreadable in the end. Make your own version and ifdef it. > > I agree. I originally did it this way to avoid duplication of the code > that was in common, but agree that the amount of duplication is not > worth avoiding for the readability. Cool. > > ::: configure.ac > > @@ +92,3 @@ > > > > +dnl Requires for application indicators > > +APPINDICATOR_REQUIRED=0.0.7 > > > > Put at the same level as the other versions. > > At the same level, do you mean move this constant up to where the > earlier *_REQUIRED constants are defined? Yes.
*** Bug 618207 has been marked as a duplicate of this bug. ***
Created attachment 169424 [details] [review] Updated ubuntu patch Attached is the updated ubuntu distro patch against the current release. This probably needs slight adjustment to fit in with the current git head. A review of the patch would also be appreciated.
One more note on the above patch, the only *real* change is that instead of the visible menu item being shown/hidden it is made sensitive/insensitive and is hidden/shown as with the rest of the group.
It's too late for 2.32, and it's too not-upstream for 3.0.
I'll close this as WONTFIX. This would be better implemented as a separate binary, akin to the way the gnome-shell Bluetooth menu works. The library lives in applet/ and should be quite straight forward to work with, and you don't even need to use C or C++ to code with, as it is introspected.
I'd take a patch to hide/disable the current bluetooth-applet when application indicators are used though. File it in a separate bug please.