[Merge] ~aleasto/update-notifier:update-notification into update-notifier:master
Marco Trevisan (Treviño)
mp+487372 at code.launchpad.net
Wed Sep 17 23:53:05 UTC 2025
I think we're there, I've added further fixes and cleanups to https://git.launchpad.net/~3v1n0/update-notifier/log/?h=update-notification
Please, add them here (after you're check them of course).
Then of course the room for improvements for the rest of the codebase is huge, but we can do it at later point
Diff comments:
> diff --git a/debian/control b/debian/control
> index 9bb1458..7038edf 100644
> --- a/debian/control
> +++ b/debian/control
Also bump build-dependendcy to libnotify 0.8.4 to both configure.ac and debian/control (as per notify_notification_set_app_icon)
> @@ -30,9 +30,8 @@ Depends: gnome-shell <!s390x> | notification-daemon <!s390x>,
> pkexec,
> polkitd,
> python3-dbus,
> - ubuntu-drivers-common,
> ubuntu-release-upgrader-gtk,
> - update-manager-gnome | update-manager (>= 1:17.04.3),
> + update-manager (>> 1:25.10~),
> update-notifier-common (= ${source:Version}),
> ${misc:Depends},
> ${shlibs:Depends}
> diff --git a/src/update-notifier.c b/src/update-notifier.c
> index 50a048e..15b0a03 100644
> --- a/src/update-notifier.c
> +++ b/src/update-notifier.c
> @@ -632,7 +598,16 @@ main (int argc, char **argv)
> exit(1);
> }
>
> + software_updater_app = g_desktop_app_info_new ("update-manager.desktop");
> + g_return_val_if_fail (software_updater_app != NULL, EXIT_FAILURE);
Better not to exit here, otherwise if the software center is not installed we are not even showing the live patch indicator
> +
> notify_init("update-notifier");
> + notify_set_app_name (g_app_info_get_name (G_APP_INFO (software_updater_app)));
This is basically overriding the previous call, but it's also wrong IMHO, since we're just a notifier, and there are other appindicators shown (livepatch) that could show other notifications too.
You already properly set the desktop hint, and this will be already enough for gnome-shell to associate it to the right app (see _getApp in notificationDaemon.js, we prioritize the appId coming from 'desktop-entry' entry)
> +
> + icon_name = g_desktop_app_info_get_string (software_updater_app, "Icon");
> + if (icon_name)
> + notify_set_app_icon (icon_name);
I think it's better not to set the global (and ideally to be deprecated icon name per app here), but rather use the icon name when creating the notification, so that we are not bound to it if we need other notifications in future
> +
> bindtextdomain(GETTEXT_PACKAGE, PACKAGE_LOCALE_DIR);
> bind_textdomain_codeset(GETTEXT_PACKAGE, "UTF-8");
> textdomain(GETTEXT_PACKAGE);
> diff --git a/src/update.c b/src/update.c
> index 588fa66..664aab3 100644
> --- a/src/update.c
> +++ b/src/update.c
> @@ -90,18 +121,27 @@ update_trayicon_update_tooltip (TrayApplet *ta, int num_upgrades)
>
>
> static void
> -cb_action(GObject *self, void *user_data __attribute__ ((__unused__)))
> +cb_action(GObject *self, void *user_data)
> {
> + TrayApplet *ta = (TrayApplet*)user_data;
> + UpdateTrayAppletPrivate *priv = (UpdateTrayAppletPrivate*)ta->user_data;
> + g_autoptr (GAppLaunchContext) context = NULL;
> +
> int i = GPOINTER_TO_INT(g_object_get_data(G_OBJECT(self), "action"));
> - invoke (actions[i][0], _(actions[i][2]), (long)actions[i][3]);
> + context = g_app_launch_context_new ();
> + launch_update_action (&actions[i], ta, context);
> }
>
> static void
> cb_preferences(GObject *self, void *user_data)
> {
> - invoke("/usr/bin/software-properties-gtk",
> - "/usr/share/applications/software-properties.desktop",
> - FALSE);
> + TrayApplet *ta = (TrayApplet*)user_data;
> + UpdateTrayAppletPrivate *priv = (UpdateTrayAppletPrivate*)ta->user_data;
> + g_autoptr (GAppLaunchContext) context = g_app_launch_context_new ();
> + g_autoptr (GError) error = NULL;
> +
> + if (!g_app_info_launch (G_APP_INFO (priv->software_properties_app), NULL, context, &error))
> + g_warning ("failed to launch preferences: %s", error->message);
IMHO we can even exit here..
> }
>
> static void
> @@ -214,53 +250,88 @@ update_apt_is_running(TrayApplet *ta, gboolean is_running)
> }
> }
>
> +static void
> +on_notification_action(NotifyNotification *notification, char* id, gpointer user_data)
> +{
> + TrayApplet *ta = (TrayApplet *)user_data;
> + UpdateTrayAppletPrivate *priv = (UpdateTrayAppletPrivate*)ta->user_data;
> + g_autoptr (GAppLaunchContext) context = NULL;
> + const char* activation_token;
> + int i;
> +
> + g_return_if_fail (ta != NULL);
> +
> + for (i = 0; i < G_N_ELEMENTS (actions); i++)
> + if (g_str_equal (id, actions[i].id))
> + break;
> +
> + if (g_str_equal (id, "default"))
> + i = DEFAULT_ACTION_IDX;
> +
> + g_return_if_fail (i < G_N_ELEMENTS (actions));
> +
> + activation_token = notify_notification_get_activation_token (notification);
> + context = update_app_launch_context_new (activation_token);
> + launch_update_action (&actions[i], ta, context);
> +}
> +
> // actually show the notification
> static gint
> show_notification(gpointer user_data)
> {
> TrayApplet *ta = (TrayApplet *)user_data;
> UpdateTrayAppletPrivate *priv = (UpdateTrayAppletPrivate*)ta->user_data;
> + g_autoptr (GSettings) settings = NULL;
> + g_autoptr (NotifyNotification) n = NULL;
> + g_autofree char *title;
ditto
> + g_autofree char *desc;
ditto
> + const char *app_name;
> + const char *app_id;
> + g_autofree char *app_id_no_suffix;
Oh, this must be initialized to NULL, otherwise we crash on early return!
>
> // apt is running, no point in showing a notification
> if(priv->apt_is_running)
> - return TRUE;
> + return G_SOURCE_CONTINUE;
>
> // check if the update-icon is still visible (in the delay time a
> // update may already have been performed)
> if(!tray_applet_ui_get_visible(ta))
> - return FALSE;
> + return G_SOURCE_REMOVE;
> +
> + app_name = g_app_info_get_name (G_APP_INFO (priv->software_updater_app));
> + app_id = g_app_info_get_id (G_APP_INFO (priv->software_updater_app));
> +
> + g_return_val_if_fail (app_name && app_id, G_SOURCE_REMOVE);
This is even a g_assert IMHO... We should techincally check if app_id is larger than .desktop (as per the check later), but I think we're good enough
>
> // now show a notification handle
> - gchar *updates;
> - updates = g_strdup_printf(ngettext("There is %i update available. "
> - "Click on the notification "
> - "icon to show the "
> - "available update.",
> - "There are %i updates available. "
> - "Click on the notification "
> - "icon to show the "
> - "available updates.",
> - priv->num_upgrades),
> - priv->num_upgrades);
> - NotifyNotification *n = notify_notification_new(
> - _("Software updates available"),
> - updates,
> - NULL);
> -
> - GdkPixbuf* pix= gtk_icon_theme_load_icon(gtk_icon_theme_get_default(),
> - GTK_STOCK_DIALOG_INFO, 48,0,NULL);
> - notify_notification_set_icon_from_pixbuf (n, pix);
I noticed there was a commit that was initially changing this code, since you're correctly dropping it, I think we can just avoid changing code and then refactor it
> - g_object_unref(pix);
> + title = g_strdup_printf(ngettext("%i Update Available",
> + "%i Updates Available",
> + priv->num_upgrades),
> + priv->num_upgrades);
> + /* TRANSLATORS: %s is the already translated name for the update-manager.desktop application */
> + desc = g_strdup_printf(_("You can review them in the %s."), app_name);
> +
> + app_id_no_suffix = g_strndup (app_id, strlen (app_id) - strlen (".desktop"));
> + n = notify_notification_new (title, desc, NULL);
> + notify_notification_set_hint (n, "desktop-entry", g_variant_new_string (app_id_no_suffix));
> notify_notification_set_timeout (n, 60*1000);
> + notify_notification_add_action (n, "default", _(actions[DEFAULT_ACTION_IDX].desc),
> + on_notification_action, ta, NULL);
> + for (int i = 0; i < G_N_ELEMENTS (actions); i++) {
> + notify_notification_add_action (n, actions[i].id, _(actions[i].desc),
> + on_notification_action, ta, NULL);
> + }
> notify_notification_show(n, NULL);
> // save the notification handle
> if (priv->active_notification)
> g_object_unref(priv->active_notification);
> - priv->active_notification = n;
> + priv->active_notification = g_steal_pointer (&n);
> +
> + settings = g_settings_new (SETTINGS_UM_SCHEMA);
> + g_settings_set_int64 (settings, SETTINGS_UM_KEY_LAST_LAUNCH, time(NULL));
>
> // remove this from the timeout now
> - g_free(updates);
> - return FALSE;
> + return G_SOURCE_REMOVE;
> }
>
> static void
--
https://code.launchpad.net/~aleasto/update-notifier/+git/update-notifier/+merge/487372
Your team Ubuntu Core Development Team is requested to review the proposed merge of ~aleasto/update-notifier:update-notification into update-notifier:master.
More information about the Ubuntu-reviews
mailing list