GNOME Bugzilla – Bug 607800
Support for application-indicators/StatusNotifierIcon
Last modified: 2010-03-28 11:43:38 UTC
We would like to support application-indicators for Banshee as proposed at this page: https://wiki.ubuntu.com/DesktopExperienceTeam/ApplicationIndicators I wanted to open this wishlist bug as a placeholder and to get feedback on the feature, thanks!
Created attachment 153966 [details] [review] Adding Indicator Application support to Banshee Here a patch. Is it agreeable?
Created attachment 154237 [details] [review] Banshee Indicator Application support, patch against upstream HEAD This patch contains some fixes compared to the previous patch. Please keep in mind that Indicator Applications don't support tooltips, so this patch doesn't make them available as well.
Review of attachment 154237 [details] [review]: Hey Sense, thanks for the patch! Please read the HACKING file and follow our code style guidelines (if { instead of if\n{ for example). I'd like people on systems where appindicator exists to be able to choose to use the old notification area.
Created attachment 155448 [details] [review] Adding Indicator Application support to Banshee An updated patch. This adds a checkbox to Preferences for disabling the Application Indicator. It should follow the code style guideline now. Custom fallback support isn't implemented, the lack of updates for GTK# made it impossible since without virtual member support of GAPI -- the Mono bindings generator -- it is not possible to use custom fallbacks in C#. Just like Rhythmbox two insensitive menu items are added to the top of the menu. They show the artist and title of the track that's currently being played. However, due to a bug in libappindicator the MenuItem labels aren't updated. Therefore both labels always read "Not playing". Someone is working on this.
Review of attachment 155448 [details] [review]: ::: configure.ac @@ +320,2 @@ Build Environment + Install Prefix: ${prefix} Why are all these white space changes part of this patch? ::: src/Extensions/Banshee.NotificationArea/Banshee.NotificationArea/IndicatorApplicationNotificationAreaBox.cs @@ +6,3 @@ +// Sense Hofstede <qense@ubuntu.com> +// +// Copyright (C) 2010 Novell, Inc. You keep your own copyright; if Aaron (Novell) contributed to this b/c you copy/pasted, keep it, just add yours, otherwise remove @@ +58,3 @@ + public void PositionMenu (Menu menu, out int x, out int y, out bool push_in) + { + /* Assigning meaningless value to comply to the interface */ Please use // for all comments ::: src/Extensions/Banshee.NotificationArea/Banshee.NotificationArea/NotificationAreaService.cs @@ +149,3 @@ + #if HAVE_APPINDICATOR + artist_menu_action = new Gtk.Action("ArtistMenuAction", Catalog.GetString("Not Playing")); Action( => Action ( add space between method name and argument list, here, below, and in .Add calls below @@ +280,3 @@ } + Hyena.Log.Debug (Catalog.GetString (String.Format("Now using the NotificationArea with the driver: {0}", notif_type))); Don't use Catalog.GetString for Log.Debug messages @@ +307,3 @@ private void DisposeNotificationArea () { + #if HAVE_APPINDICATOR notif_type exists even if !HAVE_APPINDICATOR - why #if this out? @@ +331,3 @@ menu.Show (); + #if HAVE_APPINDICATOR same question as above - this would just be false is !HAVE_APPINDICATOR - and the code is easier to read w/ fewer #if statements @@ +337,3 @@ + Widget song_menu_item = song_menu_action.CreateMenuItem (); + + menu.Prepend(new SeparatorMenuItem ()); again w/ the method/space/args @@ +685,3 @@ + + legacy_status_icon_preference = service["general"]["misc"].Add (new SchemaPreference<bool> (LegacyStatusIconSchema, + Catalog.GetString ("_Use the legacy status icon"), I'm not super happy w/ this wording, but I don't have an alternative to suggest atm. @@ +692,3 @@ + notification_service.ReloadNotificationArea(); + } catch { + Hyena.Log.Warning (Catalog.GetString ("You tried to change a setting of the Banshee.NotificationArea, but the extension isn't enabled.")); Log.Warning msgs aren't i18n'd either @@ +743,3 @@ + + #if HAVE_APPINDICATOR + public static readonly SchemaEntry<bool> LegacyStatusIconSchema = new SchemaEntry<bool> ( Since (AFAIK) AppIndicator isn't fully accepted upstream yet, let's rename this from LegacyStatusIconSchema to UseAppIndicatorSchema (default to true, I guess) ::: src/Extensions/Banshee.NotificationArea/Banshee.NotificationArea/X11NotificationAreaBox.cs @@ +59,3 @@ public event PopupMenuHandler PopupMenuEvent; + public Menu Menu { instead of adding this pointless Menu property to here and GtkNotificationAreaBox, why not just leave it only in IndicatorApplicationNotificationAreaBox (which should probably be named ApplicationIndicator?)? You can use it wherever by casting/checking type, ala var indicator = foo as ApplicationIndicator; if (indicator != null) { DoSomethingWith (indicator.Menu); } ::: src/Extensions/Banshee.NotificationArea/Resources/NotificationAreaMenu.xml @@ -6,3 @@ <menuitem name="Previous" action="PreviousAction"/> <separator/> - <placeholder name="NotificationPlaceholder" /> Why is this removed?
Created attachment 155664 [details] [review] Adding Indicator Application support to Banshee Here the latest version of the patch. I'm very sorry, but unfortunately I won't have time to work on this before the upcoming release at 11 March. Thank you for your review. I've tried to address all your points. I've changed to spacing to the end overview in ./configure.ac because the description whether Banshee is or is not built with AppIndicator support didn't fit within the space before the list of yes and no. I removed the placeholder from the pop-up menu because it seemed redundant and it added a few extra separators to the menu when using AppIndicator. I noticed no different when using the other tray icons. There is one issue I'm sorry not to have found a solution for: when you toggle the checkbox for enabling AppIndicator -- I've reversed the wording for the preference -- Banshee automatically switches to the other icon. However, I couldn't find a way to reliably regenerate the menu, so now the menu items added to the menu in code are added again each time you toggle the checkbox.
Created attachment 156020 [details] [review] Adding Indicator Application support to Banshee Two small fixes: Correctly setting the top menu entry to the artist that's currently being played, instead of the string "yes". Now depending on libappindicator 0.0.16 since that version supports the updating of labels in actions as well as in widgets, which is needed to correctly show the track info in the context menu.
Created attachment 156122 [details] [review] Adding Indicator Application support to Banshee Regenerated the patch against HEAD.
During a discussion at the #banshee IRC channel it was decided to not continue with adapting the Banshee.NotificationArea code, but instead go for a separate plugin in the Banshee Community Extensions. I've pushed a working plugin to <http://gitorious.org/~qense/banshee-community-extensions/banshee-community-extensions-appindicator>. The only issue I want to look at before requesting a merge is whether Mono Addins support conflicts just like they support dependencies. Banshee.AppIndicator has got an action group that conflicts with an identical action group in Banshee.NotificationArea and it provides almost the same functionality; no one needs two status icons. Any help would be appreciated.
The AppIndicator extension has been merged into the Banshee Community Extensions, so I'm marking this as fixed. If both are enabled, the NotificationArea and AppIndicator fight over the "close window" action, and in the end, when that action is activated, Banshee quits instead of just hiding the main window. I'm open to suggestions on how to avoid that, or on how to avoid having both extensions enabled at the same time. Feel free to open new bugs for any issues related to the AppIndicator extension.