GNOME Bugzilla – Bug 609654
Support for application-indicators/StatusNotifierIcon
Last modified: 2020-04-24 15:19:46 UTC
We would like to support application-indicators for gnome-power-manager 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!
Created attachment 153535 [details] [review] Patch which implements support for application indicators in GNOME Power manager
Review of attachment 153535 [details] [review]: This patch causes crashes, so should not be accepted as is (https://launchpad.net/bugs/523041) This should be written more robustly. It initializes icon->priv->app_indicator = app_indicator_new() without ever checking for success, or NULL, and all the new code which accesses this does not do anything like g_assert() or g_return_if_fail() if the indicator is NULL. It should just fall back to the current upstream behaviour (showing its own tray icon) if initializing the indicator fails. As for a general "yay" or "nay" to this, I can't comment on this from an upstream POV.
(In reply to comment #2) > Review of attachment 153535 [details] [review]: > > This patch causes crashes, so should not be accepted as is > (https://launchpad.net/bugs/523041) The crashes were cause by a bug in application-indicators which is fixed now. > This should be written more robustly. It initializes icon->priv->app_indicator > = app_indicator_new() without ever checking for success, or NULL, and all the > new code which accesses this does not do anything like g_assert() or > g_return_if_fail() if the indicator is NULL. It should just fall back to the > current upstream behaviour (showing its own tray icon) if initializing the > indicator fails. app_indicator_new() never fails and the fall back is done internally in application-indicators. It just crashed because of a bug in application-indicators, so the patch should be fine.
Created attachment 155315 [details] [review] Updated patch for application indciators support Updated patch.
(Not potentially blocking the upstream release, hence removing GNOME Target milestone)
There are quite a few changes to strings there: +#ifdef HAVE_APP_INDICATOR + /* TRANSLATORS: the device is fully charged */ + description = g_strdup_printf (_("%s is charged"), type_desc); +#else /* TRANSLATORS: the device is fully charged */ description = g_strdup_printf (_("%s is fully charged"), type_desc); +#endif I'm really not keen having two sets of translations for this kind of thing. I don't mind changing the strings where it makes sense, but app-indicator was meant to be about how the data is shown, not changing the data that's shown. Richard.
Application indicators do not support tooltips, so the relevant information needs to be displayed in the menu. The strings where changed to make them fit into menu items. See https://wiki.ubuntu.com/PowerStatusMenu#Items Of course it is not so great to have two sets of translations and it would be nice if one could unify the strings.
I will attach the patch split in two parts. The first patch is adding support for the application indicator, the second patch changes the strings.
Created attachment 157487 [details] [review] Add support for application indicators.
Created attachment 157488 [details] [review] Display more information in the application indicator menu items and updated strings.
I've merged some of the hunks of both patches into master, and another string patch which should make your life a little easier when patching. I'm still keen on seeing this accepted as an external dep by GNOME before we add the other main hunks.
Created attachment 169426 [details] [review] Updated ubuntu patch This patch updates the strings and conditions to meet the specifications outlined by mpt on; https://wiki.ubuntu.com/BatteryStatusMenu#Items
I believe this bug can be closed as system tray support has been removed from g-p-m. http://git.gnome.org/browse/gnome-power-manager/commit/?id=baa2317cd326304ac785514f84b505df5a6f80ba Also, Ubuntu has indicator-power now which I believe is Unity's frontend to g-p-m. But where's the power menu for GNOME Fallback?
Closing per last comment