From 892acd197630fb3f52ddfac5fc73196ba6759318 Mon Sep 17 00:00:00 2001 From: Milan Crha Date: Wed, 15 Jun 2022 17:47:11 +0200 Subject: [PATCH 01/10] plugin-loader: Actively track count of running jobs This helps to avoid mistakes when hinting a job finished. --- lib/gs-plugin-loader.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/lib/gs-plugin-loader.c b/lib/gs-plugin-loader.c index a7f86d5f2..e5c2a2c23 100644 --- a/lib/gs-plugin-loader.c +++ b/lib/gs-plugin-loader.c @@ -56,6 +56,7 @@ struct _GsPluginLoader GsAppList *pending_apps; /* (nullable) (owned) */ GThreadPool *queued_ops_pool; + gint active_jobs; GSettings *settings; @@ -3911,6 +3912,19 @@ cancellable_data_free (CancellableData *data) G_DEFINE_AUTOPTR_CLEANUP_FUNC (CancellableData, cancellable_data_free) +static void +plugin_loader_task_freed_cb (gpointer user_data, + GObject *freed_object) +{ + g_autoptr(GsPluginLoader) plugin_loader = user_data; + if (g_atomic_int_dec_and_test (&plugin_loader->active_jobs)) { + /* if the plugin used updates-changed during its job, actually schedule + * the signal emission now */ + if (plugin_loader->updates_changed_cnt > 0) + gs_plugin_loader_updates_changed (plugin_loader); + } +} + static gboolean job_process_setup_complete_cb (GCancellable *cancellable, gpointer user_data); static void job_process_cb (GTask *task); @@ -3977,6 +3991,10 @@ gs_plugin_loader_job_process_async (GsPluginLoader *plugin_loader, g_task_set_name (task, task_name); g_task_set_task_data (task, g_object_ref (plugin_job), (GDestroyNotify) g_object_unref); + g_atomic_int_inc (&plugin_loader->active_jobs); + g_object_weak_ref (G_OBJECT (task), + plugin_loader_task_freed_cb, g_object_ref (plugin_loader)); + /* Wait until the plugin has finished setting up. * * Do this using a #GCancellable. While we’re not using the #GCancellable -- GitLab From 89a3767bd0ad94d3f3d83f8c4ecbb2b79072f536 Mon Sep 17 00:00:00 2001 From: Milan Crha Date: Wed, 15 Jun 2022 17:51:13 +0200 Subject: [PATCH 02/10] plugin-loader: Remove obsolete gs_plugin_loader_hint_job_finished() The function is not needed, since the plugin loader tracks count of active jobs automatically now. --- lib/gs-plugin-job-refresh-metadata.c | 5 ----- lib/gs-plugin-loader.c | 28 ---------------------------- lib/gs-plugin-loader.h | 2 -- 3 files changed, 35 deletions(-) diff --git a/lib/gs-plugin-job-refresh-metadata.c b/lib/gs-plugin-job-refresh-metadata.c index a57b194bc..8d0b95b5d 100644 --- a/lib/gs-plugin-job-refresh-metadata.c +++ b/lib/gs-plugin-job-refresh-metadata.c @@ -387,7 +387,6 @@ finish_op (GTask *task, GError *error) { GsPluginJobRefreshMetadata *self = g_task_get_source_object (task); - GsPluginLoader *plugin_loader = g_task_get_task_data (task); g_autoptr(GError) error_owned = g_steal_pointer (&error); g_autofree gchar *job_debug = NULL; @@ -408,10 +407,6 @@ finish_op (GTask *task, progress_cb (self); g_source_destroy (self->progress_source); - /* If any plugin emitted updates-changed, actually schedule it to be - * emitted now (even if the overall operation failed). */ - gs_plugin_loader_hint_job_finished (plugin_loader); - /* Get the results of the parallel ops. */ if (self->saved_error != NULL) { g_task_return_error (task, g_steal_pointer (&self->saved_error)); diff --git a/lib/gs-plugin-loader.c b/lib/gs-plugin-loader.c index e5c2a2c23..fc374256d 100644 --- a/lib/gs-plugin-loader.c +++ b/lib/gs-plugin-loader.c @@ -3676,9 +3676,6 @@ gs_plugin_loader_process_thread_cb (GTask *task, /* sort these again as the refine may have added useful metadata */ gs_plugin_loader_job_sorted_truncation_again (helper->plugin_job, list); - /* Hint that the job has finished. */ - gs_plugin_loader_hint_job_finished (plugin_loader); - #ifdef HAVE_SYSPROF if (plugin_loader->sysprof_writer != NULL) { g_autofree gchar *sysprof_name = g_strconcat ("process-thread:", gs_plugin_action_to_string (action), NULL); @@ -3849,9 +3846,6 @@ run_job_cb (GObject *source_object, } #endif /* HAVE_SYSPROF */ - /* Hint that the job has finished. */ - gs_plugin_loader_hint_job_finished (plugin_loader); - /* FIXME: This will eventually go away when * gs_plugin_loader_job_process_finish() is removed. */ job_class = GS_PLUGIN_JOB_GET_CLASS (plugin_job); @@ -4459,25 +4453,3 @@ gs_plugin_loader_get_category_manager (GsPluginLoader *plugin_loader) return plugin_loader->category_manager; } - -/** - * gs_plugin_loader_hint_job_finished: - * @plugin_loader: a #GsPluginLoader - * - * Hint to the @plugin_loader that the set of changes caused by the current - * #GsPluginJob is likely to be finished. - * - * The @plugin_loader may emit queued-up signals as a result. - * - * Since: 42 - */ -void -gs_plugin_loader_hint_job_finished (GsPluginLoader *plugin_loader) -{ - g_return_if_fail (GS_IS_PLUGIN_LOADER (plugin_loader)); - - /* if the plugin used updates-changed during its job, actually schedule - * the signal emission now */ - if (plugin_loader->updates_changed_cnt > 0) - gs_plugin_loader_updates_changed (plugin_loader); -} diff --git a/lib/gs-plugin-loader.h b/lib/gs-plugin-loader.h index 0d2035bfd..bb2c7d8e6 100644 --- a/lib/gs-plugin-loader.h +++ b/lib/gs-plugin-loader.h @@ -126,6 +126,4 @@ gboolean gs_plugin_loader_app_is_compatible (GsPluginLoader *plugin_loader, void gs_plugin_loader_run_adopt (GsPluginLoader *plugin_loader, GsAppList *list); -void gs_plugin_loader_hint_job_finished (GsPluginLoader *plugin_loader); - G_END_DECLS -- GitLab From 680726fa2fa5b7f39aa86da05f161604a132085f Mon Sep 17 00:00:00 2001 From: Milan Crha Date: Wed, 15 Jun 2022 17:55:31 +0200 Subject: [PATCH 03/10] plugin-loader: Schedule 'updates-changed' signal when received with no job running This helps to avoid a race condition when a plugin calls updates-changed at the end of the job, but the job is finished before the callback gets called in the main thread. --- lib/gs-plugin-loader.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/lib/gs-plugin-loader.c b/lib/gs-plugin-loader.c index fc374256d..d728bfc5c 100644 --- a/lib/gs-plugin-loader.c +++ b/lib/gs-plugin-loader.c @@ -1774,12 +1774,6 @@ gs_plugin_loader_job_actions_changed_delay_cb (gpointer user_data) return FALSE; } -static void -gs_plugin_loader_job_actions_changed_cb (GsPlugin *plugin, GsPluginLoader *plugin_loader) -{ - plugin_loader->updates_changed_cnt++; -} - static void gs_plugin_loader_updates_changed (GsPluginLoader *plugin_loader) { @@ -1791,6 +1785,20 @@ gs_plugin_loader_updates_changed (GsPluginLoader *plugin_loader) g_object_ref (plugin_loader)); } +static void +gs_plugin_loader_job_updates_changed_cb (GsPlugin *plugin, + GsPluginLoader *plugin_loader) +{ + plugin_loader->updates_changed_cnt++; + + /* Schedule emit of updates changed when no job is active. + This helps to avoid a race condition when a plugin calls + updates-changed at the end of the job, but the job is + finished before the callback gets called in the main thread. */ + if (!g_atomic_int_get (&plugin_loader->active_jobs)) + gs_plugin_loader_updates_changed (plugin_loader); +} + static gboolean gs_plugin_loader_reload_delay_cb (gpointer user_data) { @@ -1849,7 +1857,7 @@ gs_plugin_loader_open_plugin (GsPluginLoader *plugin_loader, return; } g_signal_connect (plugin, "updates-changed", - G_CALLBACK (gs_plugin_loader_job_actions_changed_cb), + G_CALLBACK (gs_plugin_loader_job_updates_changed_cb), plugin_loader); g_signal_connect (plugin, "reload", G_CALLBACK (gs_plugin_loader_reload_cb), -- GitLab From 3c915f31197addf8becf9aa62a07dae35b440044 Mon Sep 17 00:00:00 2001 From: Milan Crha Date: Wed, 15 Jun 2022 17:58:32 +0200 Subject: [PATCH 04/10] plugin-loader: Change how updates-checked is scheduled for emission This way the GSource can be removed without leaking the plugin loader. --- lib/gs-plugin-loader.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/lib/gs-plugin-loader.c b/lib/gs-plugin-loader.c index d728bfc5c..9f64a3559 100644 --- a/lib/gs-plugin-loader.c +++ b/lib/gs-plugin-loader.c @@ -1760,7 +1760,7 @@ gs_plugin_loader_ask_untrusted_cb (GsPlugin *plugin, } static gboolean -gs_plugin_loader_job_actions_changed_delay_cb (gpointer user_data) +gs_plugin_loader_job_updates_changed_delay_cb (gpointer user_data) { GsPluginLoader *plugin_loader = GS_PLUGIN_LOADER (user_data); @@ -1770,7 +1770,6 @@ gs_plugin_loader_job_actions_changed_delay_cb (gpointer user_data) plugin_loader->updates_changed_id = 0; plugin_loader->updates_changed_cnt = 0; - g_object_unref (plugin_loader); return FALSE; } @@ -1780,9 +1779,11 @@ gs_plugin_loader_updates_changed (GsPluginLoader *plugin_loader) if (plugin_loader->updates_changed_id != 0) return; plugin_loader->updates_changed_id = - g_timeout_add_seconds (GS_PLUGIN_LOADER_UPDATES_CHANGED_DELAY, - gs_plugin_loader_job_actions_changed_delay_cb, - g_object_ref (plugin_loader)); + g_timeout_add_seconds_full (G_PRIORITY_DEFAULT, + GS_PLUGIN_LOADER_UPDATES_CHANGED_DELAY, + gs_plugin_loader_job_updates_changed_delay_cb, + g_object_ref (plugin_loader), + g_object_unref); } static void -- GitLab From 2930c01ee97a10577fa2860bd8df1124cfb7128d Mon Sep 17 00:00:00 2001 From: Milan Crha Date: Wed, 15 Jun 2022 18:02:11 +0200 Subject: [PATCH 05/10] plugin-loader: Add gs_plugin_loader_emit_updates_changed() It will help to avoid delays with the GUI after changes in the updates. --- lib/gs-plugin-loader.c | 23 +++++++++++++++++++++++ lib/gs-plugin-loader.h | 1 + 2 files changed, 24 insertions(+) diff --git a/lib/gs-plugin-loader.c b/lib/gs-plugin-loader.c index 9f64a3559..c0e711477 100644 --- a/lib/gs-plugin-loader.c +++ b/lib/gs-plugin-loader.c @@ -4462,3 +4462,26 @@ gs_plugin_loader_get_category_manager (GsPluginLoader *plugin_loader) return plugin_loader->category_manager; } + +/** + * gs_plugin_loader_emit_updates_changed: + * @self: a #GsPluginLoader + * + * Emits the #GsPluginLoader:updates-changed signal in the nearest + * idle in the main thread. + * + * Since: 43 + **/ +void +gs_plugin_loader_emit_updates_changed (GsPluginLoader *self) +{ + g_return_if_fail (GS_IS_PLUGIN_LOADER (self)); + + if (self->updates_changed_id != 0) + g_source_remove (self->updates_changed_id); + + self->updates_changed_id = + g_idle_add_full (G_PRIORITY_HIGH_IDLE, + gs_plugin_loader_job_updates_changed_delay_cb, + g_object_ref (self), g_object_unref); +} diff --git a/lib/gs-plugin-loader.h b/lib/gs-plugin-loader.h index bb2c7d8e6..91c5f643c 100644 --- a/lib/gs-plugin-loader.h +++ b/lib/gs-plugin-loader.h @@ -125,5 +125,6 @@ gboolean gs_plugin_loader_app_is_compatible (GsPluginLoader *plugin_loader, void gs_plugin_loader_run_adopt (GsPluginLoader *plugin_loader, GsAppList *list); +void gs_plugin_loader_emit_updates_changed (GsPluginLoader *self); G_END_DECLS -- GitLab From edfbbfb0be112b76becc1f38c920cfd45a95885c Mon Sep 17 00:00:00 2001 From: Milan Crha Date: Wed, 15 Jun 2022 18:05:38 +0200 Subject: [PATCH 06/10] gs-app: Add gs_app_is_downloaded() This avoid code duplication on two places. It also makes sure the places agree on the way the app is/is-not downloaded is determined. --- lib/gs-app.c | 32 ++++++++++++++++++++++++++++++++ lib/gs-app.h | 1 + 2 files changed, 33 insertions(+) diff --git a/lib/gs-app.c b/lib/gs-app.c index 29898765b..d7f8be279 100644 --- a/lib/gs-app.c +++ b/lib/gs-app.c @@ -6656,3 +6656,35 @@ gs_app_set_has_translations (GsApp *app, priv->has_translations = has_translations; gs_app_queue_notify (app, obj_props[PROP_HAS_TRANSLATIONS]); } + +/** + * gs_app_is_downloaded: + * @app: a #GsApp + * + * Returns whether the @app is downloaded for updates or not, + * considering also its dependencies. + * + * Returns: %TRUE, when the @app is downloaded, %FALSE otherwise + * + * Since: 43 + **/ +gboolean +gs_app_is_downloaded (GsApp *app) +{ + GsSizeType size_type; + guint64 size_bytes = 0; + + g_return_val_if_fail (GS_IS_APP (app), FALSE); + + if (!gs_app_has_quirk (app, GS_APP_QUIRK_IS_PROXY)) { + size_type = gs_app_get_size_download (app, &size_bytes); + if (size_type != GS_SIZE_TYPE_VALID || size_bytes != 0) + return FALSE; + } + + size_type = gs_app_get_size_download_dependencies (app, &size_bytes); + if (size_type != GS_SIZE_TYPE_VALID || size_bytes != 0) + return FALSE; + + return TRUE; +} diff --git a/lib/gs-app.h b/lib/gs-app.h index d78503c3e..a053299f2 100644 --- a/lib/gs-app.h +++ b/lib/gs-app.h @@ -516,5 +516,6 @@ void gs_app_set_relations (GsApp *app, gboolean gs_app_get_has_translations (GsApp *app); void gs_app_set_has_translations (GsApp *app, gboolean has_translations); +gboolean gs_app_is_downloaded (GsApp *app); G_END_DECLS -- GitLab From c6903fd6b74ddbde5db045ea67fc826824153210 Mon Sep 17 00:00:00 2001 From: Milan Crha Date: Wed, 15 Jun 2022 18:15:04 +0200 Subject: [PATCH 07/10] updates-section: Use gs_app_is_downloaded() --- src/gs-updates-section.c | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/src/gs-updates-section.c b/src/gs-updates-section.c index ca39e6fab..69e083984 100644 --- a/src/gs-updates-section.c +++ b/src/gs-updates-section.c @@ -288,15 +288,7 @@ _all_offline_updates_downloaded (GsUpdatesSection *self) /* use the download size to figure out what is downloaded and what not */ for (guint i = 0; i < gs_app_list_length (self->list); i++) { GsApp *app = gs_app_list_index (self->list, i); - GsSizeType size_type; - guint64 size_bytes; - - size_type = gs_app_get_size_download (app, &size_bytes); - if (size_type != GS_SIZE_TYPE_VALID || size_bytes != 0) - return FALSE; - - size_type = gs_app_get_size_download_dependencies (app, &size_bytes); - if (size_type != GS_SIZE_TYPE_VALID || size_bytes != 0) + if (!gs_app_is_downloaded (app)) return FALSE; } -- GitLab From 972def24d2bc54b09fe57879efb4c30962858dc0 Mon Sep 17 00:00:00 2001 From: Milan Crha Date: Wed, 15 Jun 2022 18:15:25 +0200 Subject: [PATCH 08/10] update-monitor: Use gs_app_is_downloaded() --- src/gs-update-monitor.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/gs-update-monitor.c b/src/gs-update-monitor.c index 324ff4d3e..a2dc9bb92 100644 --- a/src/gs-update-monitor.c +++ b/src/gs-update-monitor.c @@ -125,22 +125,18 @@ check_updates_kind (GsAppList *apps, for (ii = 0; ii < len && (!has_important || all_downloaded || !any_downloaded); ii++) { gboolean is_important; - guint64 size_download_bytes; app = gs_app_list_index (apps, ii); is_important = gs_app_get_update_urgency (app) == AS_URGENCY_KIND_CRITICAL; has_important = has_important || is_important; - /* took from gs-updates-section.c: _all_offline_updates_downloaded(); - the app is considered downloaded, when its download size is 0 */ - if (gs_app_get_size_download (app, &size_download_bytes) != GS_SIZE_TYPE_VALID || - size_download_bytes != 0) { - all_downloaded = FALSE; - } else { + if (gs_app_is_downloaded (app)) { any_downloaded = TRUE; if (is_important) any_important_downloaded = TRUE; + } else { + all_downloaded = FALSE; } } -- GitLab From 1aaa9617fc81651f17ed2f864bec35b958d67eed Mon Sep 17 00:00:00 2001 From: Milan Crha Date: Wed, 15 Jun 2022 18:17:18 +0200 Subject: [PATCH 09/10] update-monitor: Force reload of the Updates page before showing notification Thus it reflects what the update-monitor notifies about. --- src/gs-update-monitor.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/gs-update-monitor.c b/src/gs-update-monitor.c index a2dc9bb92..01e5869eb 100644 --- a/src/gs-update-monitor.c +++ b/src/gs-update-monitor.c @@ -288,6 +288,10 @@ notify_about_pending_updates (GsUpdateMonitor *monitor, return; } + /* To force reload of the Updates page, thus it reflects what + the update-monitor notifies about */ + gs_plugin_loader_emit_updates_changed (monitor->plugin_loader); + monitor->last_notification_time_usec = g_get_real_time (); g_debug ("Notify about update: '%s'", title); -- GitLab From 64585205437371c459d5de792d50af95e23ba346 Mon Sep 17 00:00:00 2001 From: Milan Crha Date: Wed, 15 Jun 2022 18:19:36 +0200 Subject: [PATCH 10/10] update-monitor: Change when notifications are shown This handles the fact that offline updates can be installed either all in once or none of them, which is reflected in the notification. --- src/gs-update-monitor.c | 34 ++++++++++++++-------------------- 1 file changed, 14 insertions(+), 20 deletions(-) diff --git a/src/gs-update-monitor.c b/src/gs-update-monitor.c index 01e5869eb..292d2bc4b 100644 --- a/src/gs-update-monitor.c +++ b/src/gs-update-monitor.c @@ -109,17 +109,15 @@ G_DEFINE_AUTOPTR_CLEANUP_FUNC(WithAppData, with_app_data_free); static void check_updates_kind (GsAppList *apps, gboolean *out_has_important, - gboolean *out_any_important_downloaded, gboolean *out_all_downloaded, gboolean *out_any_downloaded) { - gboolean has_important, any_important_downloaded, all_downloaded, any_downloaded; + gboolean has_important, all_downloaded, any_downloaded; guint ii, len; GsApp *app; len = gs_app_list_length (apps); has_important = FALSE; - any_important_downloaded = FALSE; all_downloaded = len > 0; any_downloaded = FALSE; @@ -131,17 +129,13 @@ check_updates_kind (GsAppList *apps, is_important = gs_app_get_update_urgency (app) == AS_URGENCY_KIND_CRITICAL; has_important = has_important || is_important; - if (gs_app_is_downloaded (app)) { + if (gs_app_is_downloaded (app)) any_downloaded = TRUE; - if (is_important) - any_important_downloaded = TRUE; - } else { + else all_downloaded = FALSE; - } } *out_has_important = has_important; - *out_any_important_downloaded = any_important_downloaded; *out_all_downloaded = all_downloaded; *out_any_downloaded = any_downloaded; } @@ -193,14 +187,15 @@ should_download_updates (GsUpdateMonitor *monitor) #endif } -/* The days below are discussed at https://gitlab.gnome.org/GNOME/gnome-software/-/issues/947 */ +/* The days below are discussed at https://gitlab.gnome.org/GNOME/gnome-software/-/issues/947 + and https://wiki.gnome.org/Design/Apps/Software/Updates#Tentative_Design */ static gboolean should_notify_about_pending_updates (GsUpdateMonitor *monitor, GsAppList *apps, const gchar **out_title, const gchar **out_body) { - gboolean has_important = FALSE, any_important_downloaded = FALSE, all_downloaded = FALSE, any_downloaded = FALSE; + gboolean has_important = FALSE, all_downloaded = FALSE, any_downloaded = FALSE; gboolean should_download, res = FALSE; gint64 timestamp_days; @@ -210,7 +205,7 @@ should_notify_about_pending_updates (GsUpdateMonitor *monitor, } should_download = should_download_updates (monitor); - check_updates_kind (apps, &has_important, &any_important_downloaded, &all_downloaded, &any_downloaded); + check_updates_kind (apps, &has_important, &all_downloaded, &any_downloaded); if (!gs_app_list_length (apps)) { /* Notify only when the download is disabled and it's the 4th day or it's more than 7 days */ @@ -221,7 +216,7 @@ should_notify_about_pending_updates (GsUpdateMonitor *monitor, } } else if (has_important) { if (timestamp_days >= 1) { - if (any_important_downloaded) { + if (all_downloaded) { *out_title = _("Critical Software Update Ready to Install"); *out_body = _("An important software update is ready to be installed."); res = TRUE; @@ -231,24 +226,23 @@ should_notify_about_pending_updates (GsUpdateMonitor *monitor, res = TRUE; } } - /* When automatic updates are on and there are things ready to be installed, then rather claim - * about things to be installed, than things to be downloaded. */ - } else if (all_downloaded || (any_downloaded && should_download)) { + } else if (all_downloaded) { if (timestamp_days >= 3) { *out_title = _("Software Updates Ready to Install"); *out_body = _("Software updates are waiting and ready to be installed."); res = TRUE; } - /* To not hide downloaded updates for 14 days when new updates were discovered meanwhile */ - } else if (timestamp_days >= (any_downloaded ? 3 : 14)) { + /* To not hide downloaded updates for 14 days when new updates were discovered meanwhile. + Never show "Available to Download" when it's supposed to download the updates. */ + } else if (!should_download && timestamp_days >= 14) { *out_title = _("Software Updates Available to Download"); *out_body = _("Please download waiting software updates."); res = TRUE; } - g_debug ("%s: last_test_days:%" G_GINT64_FORMAT " n-apps:%u should_download:%d has_important:%d any_important_downloaded:%d " + g_debug ("%s: last_test_days:%" G_GINT64_FORMAT " n-apps:%u should_download:%d has_important:%d " "all_downloaded:%d any_downloaded:%d res:%d%s%s%s%s", G_STRFUNC, - timestamp_days, gs_app_list_length (apps), should_download, has_important, any_important_downloaded, + timestamp_days, gs_app_list_length (apps), should_download, has_important, all_downloaded, any_downloaded, res, res ? " reason:" : "", res ? *out_title : "", -- GitLab