[Merge] ~aleasto/update-notifier:update-notification into update-notifier:master
Alessandro Astone
mp+487372 at code.launchpad.net
Tue Sep 16 18:27:05 UTC 2025
Thanks, addressed.
Diff comments:
> diff --git a/src/update-notifier.c b/src/update-notifier.c
> index 50a048e..c5fd6b6 100644
> --- a/src/update-notifier.c
> +++ b/src/update-notifier.c
> @@ -157,7 +158,7 @@ void invoke(const gchar *cmd, const gchar *desktop, gboolean with_pkexec)
> static GtkWidget *w = NULL;
>
> // pkexec
> - if(with_pkexec || FORCE_PKEXEC) {
> + if(FORCE_PKEXEC) {
Done (deleted invoke() function)
> invoke_with_pkexec(cmd);
> return;
> }
> @@ -633,6 +635,10 @@ main (int argc, char **argv)
> }
>
> notify_init("update-notifier");
> + software_updater_app = g_desktop_app_info_new ("update-manager.desktop");
Added assert
> + notify_set_app_name (g_app_info_get_name (G_APP_INFO (software_updater_app)));
> + notify_set_app_icon ("system-software-update");
Went for the second options
> +
> 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..72a971a 100644
> --- a/src/update.c
> +++ b/src/update.c
> @@ -22,24 +22,23 @@
> #define UPGRADE_CHECKER PACKAGE_LIB_DIR"/update-notifier/apt-check"
> #define PACKAGE_SYSTEM_LOCKED PACKAGE_LIB_DIR"/update-notifier/package-system-locked"
>
> -// command, description, desktopfile, needs_pkexec
> -const char* actions[][4] = {
> - { "/usr/lib/update-notifier/backend_helper.py show_updates",
> - N_("Show updates"),
> - "/usr/share/applications/update-manager.desktop",
> - GINT_TO_POINTER(FALSE) },
> - { "/usr/lib/update-notifier/backend_helper.py install_all_updates",
> - N_("Install all updates"), "/usr/share/applications/synaptic.desktop",
> - GINT_TO_POINTER(FALSE)
> +typedef struct {
> + const char *command; // the command to execude
> + const char *desc; // the user-visible description
> +} action_t;
> +
> +action_t actions[] = {
> + {
> + "/usr/bin/update-manager --no-update --install-all",
> + N_("Install Now"),
This string was given by the Design team: https://www.figma.com/file/BoqGPVANc2zFhvGI6oJUr2?node-id=1129%3A167&mode=dev
It is always clear by the surrounding context where this string is shown: both the notification and the appindicator mention "There are %d updates available".
> },
> - { "/usr/lib/update-notifier/backend_helper.py check_updates",
> - N_("Check for updates"), "/usr/share/applications/synaptic.desktop",
> - GINT_TO_POINTER(FALSE) },
> - { "/usr/lib/update-notifier/backend_helper.py start_packagemanager",
> - N_("Start package manager"), "/usr/share/applications/synaptic.desktop",
> - GINT_TO_POINTER(FALSE)},
> - { NULL, NULL, NULL }
> + {
> + "/usr/bin/update-manager --no-update",
Unfortunately update-manager does not properly implement desktop actions. They can't be hidden with NoDisplay so having them visible but not working would worsen the experience.
> + N_("Show Updates"),
> + },
> + { NULL, NULL },
> };
> +action_t *default_action = &actions[1];
Great idea, done
>
> enum {
> NOTIFICATION_DEFAULT,
> @@ -130,10 +127,8 @@ update_trayicon_create_menu(TrayApplet *ta)
>
> ta->menu = gtk_menu_new ();
>
> - for(i=0;actions[i][0]!=NULL;i++) {
> - if (!g_file_test(actions[i][2], G_FILE_TEST_EXISTS))
> - continue;
> - menuitem = gtk_menu_item_new_with_label (_(actions[i][1]));
> + for(i=0;actions[i].command!=NULL;i++) {
Done
> + menuitem = gtk_menu_item_new_with_label (_(actions[i].desc));
>
> gtk_menu_shell_append (GTK_MENU_SHELL (ta->menu), menuitem);
> g_object_set_data(G_OBJECT(menuitem), "action", GINT_TO_POINTER(i));
> @@ -214,53 +206,61 @@ update_apt_is_running(TrayApplet *ta, gboolean is_running)
> }
> }
>
> +static void
> +on_notification_action(NotifyNotification *notification, char* id, gpointer user_data)
> +{
> + const action_t *action = user_data;
> +
> + g_return_if_fail (action != NULL);
> + invoke (action->command);
Done
> +}
> +
> // 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_autofree char *title;
> + g_autofree char *desc;
>
> // 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;
>
> // 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);
> - g_object_unref(pix);
> + title = g_strdup_printf(ngettext("%i Update Available",
> + "%i Updates Available",
> + priv->num_upgrades),
> + priv->num_upgrades);
> + desc = g_strdup_printf(_("You can review them in the %s."),
I'm using the translated string from the desktop file because that's what the user will be seeing in the application list. If update-notifier gets translated and the update-manager desktop file doesn't, or viceversa, we would be showing two different names for the same app.
I've used the string as given by the design team: https://www.figma.com/file/BoqGPVANc2zFhvGI6oJUr2?node-id=1129%3A167&mode=dev
I've added TRANSLATORS: though.
> + notify_get_app_name());
Done
> + NotifyNotification *n = notify_notification_new(title, desc, NULL);
Done (+ g_steal_pointer)
> + notify_notification_set_hint (n, "desktop-entry", g_variant_new_string ("update-manager"));
Done
> notify_notification_set_timeout (n, 60*1000);
> + notify_notification_add_action (n, "default", _(default_action->desc),
> + on_notification_action, default_action, NULL);
> + for (int i = 0; actions[i].command != NULL; i++) {
> + notify_notification_add_action (n, actions[i].command, _(actions[i].desc),
> + on_notification_action, &actions[i], NULL);
> + }
> notify_notification_show(n, NULL);
> // save the notification handle
> if (priv->active_notification)
> g_object_unref(priv->active_notification);
> priv->active_notification = 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
> @@ -649,45 +629,21 @@ update_check (TrayApplet *ta)
> }
>
> if (priv->num_security == 0)
> - ta->name = "software-update-available";
> + ta->name = "software-update-available-symbolic";
> else
> - ta->name = "software-update-urgent";
> + ta->name = "software-update-urgent-symbolic";
> tray_applet_ui_set_icon(ta, ta->name);
>
> // update the tooltip
> update_trayicon_update_tooltip(ta, priv->num_upgrades);
>
> - tray_applet_ui_set_visible(ta, FALSE);
> - if (auto_launch_now(ta))
> + if (auto_launch_now(ta) &&
> + !g_settings_get_boolean(ta->un->settings, SETTINGS_KEY_NO_UPDATE_NOTIFICATIONS))
> {
> - g_autoptr(GError) error = NULL;
> - g_autofree gchar *list_oem_metapackages = NULL;
> - GPid pid;
> - g_auto(GStrv) argv = g_new0(char *, 2);
> -
> - argv[0] = g_build_filename (PACKAGE_LIB_DIR,
> - "update-notifier",
> - "list-oem-metapackages",
> - NULL);
> -
> - g_spawn_async (NULL, /* working_directory */
> - argv,
> - NULL, /* envp */
> - G_SPAWN_DO_NOT_REAP_CHILD,
> - NULL, /* child_setup */
> - NULL, /* user_data */
> - &pid,
> - &error);
> -
> - if (error != NULL)
> - {
> - g_printerr("Failed to spawn ‘list-oem-metapackages’: %s\n", error->message);
> - return FALSE;
> - }
> -
> - g_child_watch_add (pid, launch_update_manager, NULL);
> + // show the notification with some delay. otherwise on a login
> + // the origin of the window is 0,0 and that looks ugly
> + g_timeout_add_seconds(5, show_notification, ta);
Which systemd user service? :)
This is /etc/xdg/autostart unfortunately. We should move it to native systemd, but that's for a separate patch.
> }
> - return TRUE;
>
> // if we are already visible, skip the rest
> if(tray_applet_ui_get_visible (ta))
--
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