GNOME Bugzilla – Bug 796258
AppIndicator mode: VPN connections need extra icons due to missing pixbufs support in AppIndicator
Last modified: 2018-06-01 13:05:13 UTC
Created attachment 372220 [details] [review] Fix VPN icons when displayed in AppIndicator mode Patch 1: src/applet.c: Fix VPN icons when displayed in AppIndicator mode. We must use premade icons for the indicator VPN case. The icons are copies of the original with the padlock pre-composited, because indicators can't use pixbufs; we use the suffix -secure. This change got introduced with Ubuntu 16.04. Several icon themes have upstreamed those -secure icons meanwhile (Numix, MATE Faenza, elementaryXubuntu-dark from murrine-themes). This patch requires additional icons in nm-applet: nm-signal-<xx>.png -> nm-signal-<xx>-secure.png nm-device-wired.png -> nm-device-wired-secure.png Patch author: Mathieu Trudel-Lapierre <mathieu@canonical.com> Patch 2: Add -secure icons required for correct VPN icon display in AppIndicator mode. Icons were created for Ubuntu and have been obtained from the network-manager-applet src:package in Ubuntu. The package maintainer applied no specific copyrights to those files. In debian/copyright of the Ubuntu package they are covered by this copyright info block: Files: * Copyright: 2004 - 2015 Red Hat, Inc. 2005 - 2008 Novell, Inc. 2001, 2002 Free Software Foundation License: GPL-2+
Created attachment 372221 [details] [review] Additional icons for VPN connections when used in AppIndicator mode
The code g_strdup_printf ("%s-secure", app_indicator_get_icon (applet->app_indicator)); uses the icon that was set above, during foo_set_icon (applet, ICON_LAYER_LINK, pixbuf, icon_name); But @icon_name could also be something like "nm-no-connection". In that case, no icon "nm-no-connection-secure" exists. The patch seems wrong in this regard.
Hi Thomas, sorry for the late reply. I reconsidered the code and in theory, "nm-no-connection" could be set, but there are more ifs and thens in that code passage, that need to be considered: ``` switch (state) { case NM_STATE_UNKNOWN: case NM_STATE_ASLEEP: icon_name = "nm-no-connection"; dev_tip = _("Networking disabled"); break; case NM_STATE_DISCONNECTED: icon_name = "nm-no-connection"; dev_tip = _("No network connection"); break; default: applet_get_device_icon_for_state (applet, &pixbuf, &icon_name_free, &dev_tip_free); icon_name = icon_name_free; dev_tip = dev_tip_free; break; } foo_set_icon (applet, ICON_LAYER_LINK, pixbuf, icon_name); icon_name = NULL; g_clear_pointer (&icon_name_free, g_free); /* VPN state next */ active_vpn = applet_get_active_vpn_connection (applet, &vpn_state); if (active_vpn) { switch (vpn_state) { case NM_VPN_CONNECTION_STATE_ACTIVATED: icon_name = "nm-vpn-active-lock"; #ifdef WITH_APPINDICATOR if (with_appindicator) icon_name = icon_name_free = g_strdup_printf ("%s-secure", app_indicator_get_icon (applet->app_indicator)); #endif /* WITH_APPINDICATOR */ break; [...] ``` So the question, libnm wise is: Can vpn_state be NM_VPN_CONNECTION_STATE_ACTIVATED, while the NM state at the same time is either of NM_STATE_UNKNOWN, NM_STATE_ASLEEP, NM_STATE_DISCONNECTED). My guess is: no. So, the icon_name, if vpn_state is NM_VPN_CONNECTION_STATE_ACTIVATED, will be derived from applet_get_device_icon_for_state(). And then my / Canonical's patch should be ok. Greets, Mike
> So the question, libnm wise is: Can vpn_state be > NM_VPN_CONNECTION_STATE_ACTIVATED, while the NM state at the same time is > either of NM_STATE_UNKNOWN, NM_STATE_ASLEEP, NM_STATE_DISCONNECTED). My guess > is: no. Probably that case should not happen. However, the code should be robust against inconsistent/unexpected states. Anyway, I merged your two patches as-is to master [1]. I only re-ordered them, because otherwise, the first commit would have no icons and be broken on its own. Thanks Mike!! [1] https://gitlab.gnome.org/GNOME/network-manager-applet/commit/387b55a5c313dfc0f103b76f5cf869b0f0f0dbdb