From 5ac06ac8eaf43826c70195b4ae97cf8e7c2d7eb0 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 15 Nov 2022 17:44:59 +0000 Subject: [PATCH 01/13] gaction: Add missing annotations to g_action_parse_detailed_name() Signed-off-by: Philip Withnall --- gio/gaction.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/gio/gaction.c b/gio/gaction.c index 65d2f5aa8..e0ba84eec 100644 --- a/gio/gaction.c +++ b/gio/gaction.c @@ -437,8 +437,9 @@ g_action_name_is_valid (const gchar *action_name) /** * g_action_parse_detailed_name: * @detailed_name: a detailed action name - * @action_name: (out): the action name - * @target_value: (out): the target value, or %NULL for no target + * @action_name: (out) (optional) (not nullable) (transfer full): the action name + * @target_value: (out) (optional) (nullable) (transfer full): the target value, + * or %NULL for no target * @error: a pointer to a %NULL #GError, or %NULL * * Parses a detailed action name into its separate name and target From 4748db5fd3e3fb789182cebe6c1894f9177d426a Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 15 Nov 2022 17:45:40 +0000 Subject: [PATCH 02/13] gaction: Improve docs formatting for g_action_parse_detailed_name() Signed-off-by: Philip Withnall --- gio/gaction.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/gio/gaction.c b/gio/gaction.c index e0ba84eec..5394a1ae7 100644 --- a/gio/gaction.c +++ b/gio/gaction.c @@ -449,23 +449,23 @@ g_action_name_is_valid (const gchar *action_name) * * The first format is used to represent an action name with no target * value and consists of just an action name containing no whitespace - * nor the characters ':', '(' or ')'. For example: "app.action". + * nor the characters `:`, `(` or `)`. For example: `app.action`. * * The second format is used to represent an action with a target value - * that is a non-empty string consisting only of alphanumerics, plus '-' - * and '.'. In that case, the action name and target value are - * separated by a double colon ("::"). For example: - * "app.action::target". + * that is a non-empty string consisting only of alphanumerics, plus `-` + * and `.`. In that case, the action name and target value are + * separated by a double colon (`::`). For example: + * `app.action::target`. * * The third format is used to represent an action with any type of * target value, including strings. The target value follows the action - * name, surrounded in parens. For example: "app.action(42)". The + * name, surrounded in parens. For example: `app.action(42)`. The * target value is parsed using g_variant_parse(). If a tuple-typed * value is desired, it must be specified in the same way, resulting in - * two sets of parens, for example: "app.action((1,2,3))". A string - * target can be specified this way as well: "app.action('target')". - * For strings, this third format must be used if * target value is - * empty or contains characters other than alphanumerics, '-' and '.'. + * two sets of parens, for example: `app.action((1,2,3))`. A string + * target can be specified this way as well: `app.action('target')`. + * For strings, this third format must be used if target value is + * empty or contains characters other than alphanumerics, `-` and `.`. * * Returns: %TRUE if successful, else %FALSE with @error set * From d470e7227a4405262d5f0f4945dbe855d3adf60d Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 15 Nov 2022 17:45:59 +0000 Subject: [PATCH 03/13] gaction: Improve documentation around floating GVariants Signed-off-by: Philip Withnall --- gio/gaction.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/gio/gaction.c b/gio/gaction.c index 5394a1ae7..5599137fd 100644 --- a/gio/gaction.c +++ b/gio/gaction.c @@ -467,6 +467,12 @@ g_action_name_is_valid (const gchar *action_name) * For strings, this third format must be used if target value is * empty or contains characters other than alphanumerics, `-` and `.`. * + * If this function returns %TRUE, a non-%NULL value is guaranteed to be returned + * in @action_name (if a pointer is passed in). A %NULL value may still be + * returned in @target_value, as the @detailed_name may not contain a target. + * + * If returned, the #GVariant in @target_value is guaranteed to not be floating. + * * Returns: %TRUE if successful, else %FALSE with @error set * * Since: 2.38 From 716189c4c7e46c16b7c4903cc25bbfe506d214db Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 15 Nov 2022 17:46:42 +0000 Subject: [PATCH 04/13] gfdonotificationbackend: Improve internal docs around floating GVariants MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The code is correct, but from a quick read-through it wasn’t entirely clear to me how it handled floating `GVariant`s in object state or the `parameter` argument. Add an assertion and some comments to hopefully clarify things a little. Signed-off-by: Philip Withnall --- gio/gfdonotificationbackend.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/gio/gfdonotificationbackend.c b/gio/gfdonotificationbackend.c index e0bfea535..dd93a5dd8 100644 --- a/gio/gfdonotificationbackend.c +++ b/gio/gfdonotificationbackend.c @@ -63,8 +63,8 @@ typedef struct GFdoNotificationBackend *backend; gchar *id; guint32 notify_id; - gchar *default_action; - GVariant *default_action_target; + gchar *default_action; /* (nullable) (owned) */ + GVariant *default_action_target; /* (nullable) (owned), not floating */ } FreedesktopNotification; static void @@ -138,6 +138,9 @@ activate_action (GFdoNotificationBackend *backend, { GNotificationBackend *g_backend = G_NOTIFICATION_BACKEND (backend); + /* Callers should not provide a floating variant here */ + g_assert (parameter == NULL || !g_variant_is_floating (parameter)); + if (name) { if (g_str_has_prefix (name, "app.")) From af6bf2dc027f2308ceafbdd275f5c80c4265b633 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 15 Nov 2022 17:48:11 +0000 Subject: [PATCH 05/13] ggtknotificationbackend: Fix a minor typo in a comment Signed-off-by: Philip Withnall --- gio/ggtknotificationbackend.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gio/ggtknotificationbackend.c b/gio/ggtknotificationbackend.c index 8d6eab2fb..b749eae41 100644 --- a/gio/ggtknotificationbackend.c +++ b/gio/ggtknotificationbackend.c @@ -50,7 +50,7 @@ g_gtk_notification_backend_is_supported (void) GVariant *reply; /* Find out if the notification server is running. This is a - * synchronous call because gio extension points don't support asnyc + * synchronous call because gio extension points don't support async * backend verification. This is only run once and only contacts the * dbus daemon. */ From 18574d1ba9dae51bc49942201b3f6401b5051e2a Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 15 Nov 2022 17:48:47 +0000 Subject: [PATCH 06/13] gnotification: Improve docs around GVariant ownership MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a few missing introspection annotations too. This doesn’t change any of the ownership handling behaviour, just documents what’s there. What’s there seems to be correct, to the extent that I can see. Signed-off-by: Philip Withnall --- gio/gnotification.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/gio/gnotification.c b/gio/gnotification.c index 19bfca372..6848895b2 100644 --- a/gio/gnotification.c +++ b/gio/gnotification.c @@ -102,7 +102,7 @@ struct _GNotification gchar *category; GPtrArray *buttons; gchar *default_action; - GVariant *default_action_target; + GVariant *default_action_target; /* (nullable) (owned), not floating */ }; typedef struct @@ -615,11 +615,17 @@ g_notification_get_button_with_action (GNotification *notification, /*< private > * g_notification_get_default_action: * @notification: a #GNotification - * @action: (nullable): return location for the default action - * @target: (nullable): return location for the target of the default action + * @action: (out) (optional) (nullable) (transfer full): return location for the + * default action, or %NULL if unset + * @target: (out) (optional) (nullable) (transfer full): return location for the + * target of the default action, or %NULL if unset * * Gets the action and target for the default action of @notification. * + * If this function returns %TRUE, @action is guaranteed to be set to a non-%NULL + * value (if a pointer is passed to @action). @target may still return a %NULL + * value, as the default action may have no target. + * * Returns: %TRUE if @notification has a default action */ gboolean @@ -736,7 +742,7 @@ g_notification_set_default_action_and_target (GNotification *notification, * application-wide action (start with "app."). * * If @target is non-%NULL, @action will be activated with @target as - * its parameter. + * its parameter. If @target is floating, it will be consumed. * * When no default action is set, the application that the notification * was sent on is activated. From 683c7d05a35054d027f4fa8faa7ee673ae8b8a5d Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 15 Nov 2022 17:49:45 +0000 Subject: [PATCH 07/13] gnotification: Fix a couple of minor typos in a documentation comment Signed-off-by: Philip Withnall --- gio/gnotification.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gio/gnotification.c b/gio/gnotification.c index 6848895b2..abe68a215 100644 --- a/gio/gnotification.c +++ b/gio/gnotification.c @@ -355,11 +355,11 @@ g_notification_set_urgent (GNotification *notification, * g_notification_get_category: * @notification: a #GNotification * - * Gets the cateogry of @notification. + * Gets the category of @notification. * * This will be %NULL if no category is set. * - * Returns: (nullable): the cateogry of @notification + * Returns: (nullable): the category of @notification * * Since: 2.70 */ From e8c068db50a2e061c935367e59c492c445ab5f1f Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 15 Nov 2022 17:50:03 +0000 Subject: [PATCH 08/13] gnotificationbackend: Fix a GDBusConnection leak MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `g_notification_backend_new_default()` adds a reference on `backend->dbus_connection` (if non-`NULL`), but nothing ever unreffed that. Fix that by adding a dispose method. In practice this is not really a problem, because the notification backend is held alive by a `GApplication`, which lives as long as the process. It’ll be a problem if someone is to ever add unit tests for `GNotificationBackend`s though. So let’s fix it. Signed-off-by: Philip Withnall --- gio/gnotificationbackend.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/gio/gnotificationbackend.c b/gio/gnotificationbackend.c index e5f404705..acbd10820 100644 --- a/gio/gnotificationbackend.c +++ b/gio/gnotificationbackend.c @@ -28,9 +28,23 @@ G_DEFINE_TYPE (GNotificationBackend, g_notification_backend, G_TYPE_OBJECT) +static void +g_notification_backend_dispose (GObject *obj) +{ + GNotificationBackend *backend = G_NOTIFICATION_BACKEND (obj); + + backend->application = NULL; /* no reference held, but clear the pointer anyway to avoid it dangling */ + g_clear_object (&backend->dbus_connection); + + G_OBJECT_CLASS (g_notification_backend_parent_class)->dispose (obj); +} + static void g_notification_backend_class_init (GNotificationBackendClass *class) { + GObjectClass *object_class = G_OBJECT_CLASS (class); + + object_class->dispose = g_notification_backend_dispose; } static void From 19eee4bc412fa6a15a3b65711408bc54978474fe Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 15 Nov 2022 17:51:41 +0000 Subject: [PATCH 09/13] gtestdbus: Use g_timeout_add_seconds() rather than g_timeout_add() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This makes the code a little easier to understand and allows the kernel a little bit more leeway in scheduling the callback, which is fine because we don’t need high accuracy here. Signed-off-by: Philip Withnall --- gio/gtestdbus.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gio/gtestdbus.c b/gio/gtestdbus.c index cf7d1a4b8..6aedb3eae 100644 --- a/gio/gtestdbus.c +++ b/gio/gtestdbus.c @@ -94,7 +94,7 @@ _g_object_unref_and_wait_weak_notify (gpointer object) g_idle_add (unref_on_idle, object); /* Make sure we don't block forever */ - timeout_id = g_timeout_add (30 * 1000, on_weak_notify_timeout, &data); + timeout_id = g_timeout_add_seconds (30, on_weak_notify_timeout, &data); g_main_loop_run (data.loop); From b836ed5c13afbf931455167b117036789946713e Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 10 Nov 2022 12:47:27 +0000 Subject: [PATCH 10/13] tests: Stop using GMainLoop in actions test Instead, iterate the `GMainContext` directly. This allows tests on asynchronously returned values to be done in the actual test function, rather than a callback, which should make the tests a little clearer. This introduces no functional changes. Signed-off-by: Philip Withnall --- gio/tests/actions.c | 175 ++++++++++++++++++-------------------------- 1 file changed, 71 insertions(+), 104 deletions(-) diff --git a/gio/tests/actions.c b/gio/tests/actions.c index f27841b4b..f804f381a 100644 --- a/gio/tests/actions.c +++ b/gio/tests/actions.c @@ -2,6 +2,7 @@ * Copyright © 2010, 2011, 2013, 2014 Codethink Limited * Copyright © 2010, 2011, 2012, 2013, 2015 Red Hat, Inc. * Copyright © 2012 Pavel Vasin + * Copyright © 2022 Endless OS Foundation, LLC * * SPDX-License-Identifier: LGPL-2.1-or-later * @@ -644,11 +645,13 @@ compare_action_groups (GActionGroup *a, GActionGroup *b) } static gboolean -stop_loop (gpointer data) +timeout_cb (gpointer user_data) { - GMainLoop *loop = data; + gboolean *timed_out = user_data; - g_main_loop_quit (loop); + g_assert_false (*timed_out); + *timed_out = TRUE; + g_main_context_wakeup (NULL); return G_SOURCE_REMOVE; } @@ -664,96 +667,16 @@ static GActionEntry exported_entries[] = { }; static void -list_cb (GObject *source, - GAsyncResult *res, - gpointer user_data) +async_result_cb (GObject *source, + GAsyncResult *res, + gpointer user_data) { - GDBusConnection *bus = G_DBUS_CONNECTION (source); - GMainLoop *loop = user_data; - GError *error = NULL; - GVariant *v; - gchar **actions; + GAsyncResult **result_out = user_data; - v = g_dbus_connection_call_finish (bus, res, &error); - g_assert_nonnull (v); - g_variant_get (v, "(^a&s)", &actions); - g_assert_cmpint (g_strv_length (actions), ==, G_N_ELEMENTS (exported_entries)); - g_free (actions); - g_variant_unref (v); - g_main_loop_quit (loop); -} + g_assert_null (*result_out); + *result_out = g_object_ref (res); -static gboolean -call_list (gpointer user_data) -{ - GDBusConnection *bus; - - bus = g_bus_get_sync (G_BUS_TYPE_SESSION, NULL, NULL); - g_dbus_connection_call (bus, - g_dbus_connection_get_unique_name (bus), - "/", - "org.gtk.Actions", - "List", - NULL, - NULL, - 0, - G_MAXINT, - NULL, - list_cb, - user_data); - g_object_unref (bus); - - return G_SOURCE_REMOVE; -} - -static void -describe_cb (GObject *source, - GAsyncResult *res, - gpointer user_data) -{ - GDBusConnection *bus = G_DBUS_CONNECTION (source); - GMainLoop *loop = user_data; - GError *error = NULL; - GVariant *v; - gboolean enabled; - gchar *param; - GVariantIter *iter; - - v = g_dbus_connection_call_finish (bus, res, &error); - g_assert_nonnull (v); - /* FIXME: there's an extra level of tuplelization in here */ - g_variant_get (v, "((bgav))", &enabled, ¶m, &iter); - g_assert_true (enabled); - g_assert_cmpstr (param, ==, ""); - g_assert_cmpint (g_variant_iter_n_children (iter), ==, 0); - g_free (param); - g_variant_iter_free (iter); - g_variant_unref (v); - - g_main_loop_quit (loop); -} - -static gboolean -call_describe (gpointer user_data) -{ - GDBusConnection *bus; - - bus = g_bus_get_sync (G_BUS_TYPE_SESSION, NULL, NULL); - g_dbus_connection_call (bus, - g_dbus_connection_get_unique_name (bus), - "/", - "org.gtk.Actions", - "Describe", - g_variant_new ("(s)", "copy"), - NULL, - 0, - G_MAXINT, - NULL, - describe_cb, - user_data); - g_object_unref (bus); - - return G_SOURCE_REMOVE; + g_main_context_wakeup (NULL); } G_GNUC_BEGIN_IGNORE_DEPRECATIONS @@ -800,15 +723,16 @@ test_dbus_export (void) GSimpleActionGroup *group; GDBusActionGroup *proxy; GSimpleAction *action; - GMainLoop *loop; GError *error = NULL; GVariant *v; guint id; gchar **actions; guint n_actions_added = 0, n_actions_enabled_changed = 0, n_actions_removed = 0, n_actions_state_changed = 0; gulong added_signal_id, enabled_changed_signal_id, removed_signal_id, state_changed_signal_id; - - loop = g_main_loop_new (NULL, FALSE); + gboolean enabled; + gchar *param; + GVariantIter *iter; + GAsyncResult *async_result = NULL; session_bus_up (); bus = g_bus_get_sync (G_BUS_TYPE_SESSION, NULL, NULL); @@ -842,12 +766,58 @@ test_dbus_export (void) g_strfreev (actions); /* check that calling "List" works too */ - g_idle_add (call_list, loop); - g_main_loop_run (loop); + g_dbus_connection_call (bus, + g_dbus_connection_get_unique_name (bus), + "/", + "org.gtk.Actions", + "List", + NULL, + NULL, + 0, + G_MAXINT, + NULL, + async_result_cb, + &async_result); + + while (async_result == NULL) + g_main_context_iteration (NULL, TRUE); + + v = g_dbus_connection_call_finish (bus, async_result, &error); + g_assert_nonnull (v); + g_variant_get (v, "(^a&s)", &actions); + g_assert_cmpuint (g_strv_length (actions), ==, G_N_ELEMENTS (exported_entries)); + g_free (actions); + g_variant_unref (v); + g_clear_object (&async_result); /* check that calling "Describe" works */ - g_idle_add (call_describe, loop); - g_main_loop_run (loop); + g_dbus_connection_call (bus, + g_dbus_connection_get_unique_name (bus), + "/", + "org.gtk.Actions", + "Describe", + g_variant_new ("(s)", "copy"), + NULL, + 0, + G_MAXINT, + NULL, + async_result_cb, + &async_result); + + while (async_result == NULL) + g_main_context_iteration (NULL, TRUE); + + v = g_dbus_connection_call_finish (bus, async_result, &error); + g_assert_nonnull (v); + /* FIXME: there's an extra level of tuplelization in here */ + g_variant_get (v, "((bgav))", &enabled, ¶m, &iter); + g_assert_true (enabled); + g_assert_cmpstr (param, ==, ""); + g_assert_cmpint (g_variant_iter_n_children (iter), ==, 0); + g_free (param); + g_variant_iter_free (iter); + g_variant_unref (v); + g_clear_object (&async_result); /* test that the initial transfer works */ g_assert_true (G_IS_DBUS_ACTION_GROUP (proxy)); @@ -931,7 +901,6 @@ test_dbus_export (void) g_signal_handler_disconnect (proxy, state_changed_signal_id); g_object_unref (proxy); g_object_unref (group); - g_main_loop_unref (loop); g_object_unref (bus); session_bus_down (); @@ -1016,9 +985,7 @@ test_bug679509 (void) { GDBusConnection *bus; GDBusActionGroup *proxy; - GMainLoop *loop; - - loop = g_main_loop_new (NULL, FALSE); + gboolean timed_out = FALSE; session_bus_up (); bus = g_bus_get_sync (G_BUS_TYPE_SESSION, NULL, NULL); @@ -1027,10 +994,10 @@ test_bug679509 (void) g_strfreev (g_action_group_list_actions (G_ACTION_GROUP (proxy))); g_object_unref (proxy); - g_timeout_add (100, stop_loop, loop); - g_main_loop_run (loop); + g_timeout_add (100, timeout_cb, &timed_out); + while (!timed_out) + g_main_context_iteration (NULL, TRUE); - g_main_loop_unref (loop); g_object_unref (bus); session_bus_down (); From 73f445b099f6949a56d1ff7259cc445a6c7156b7 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 10 Nov 2022 13:25:07 +0000 Subject: [PATCH 11/13] tests: Add some missing error checks to actions test Signed-off-by: Philip Withnall --- gio/tests/actions.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/gio/tests/actions.c b/gio/tests/actions.c index f804f381a..749a782f1 100644 --- a/gio/tests/actions.c +++ b/gio/tests/actions.c @@ -783,6 +783,7 @@ test_dbus_export (void) g_main_context_iteration (NULL, TRUE); v = g_dbus_connection_call_finish (bus, async_result, &error); + g_assert_no_error (error); g_assert_nonnull (v); g_variant_get (v, "(^a&s)", &actions); g_assert_cmpuint (g_strv_length (actions), ==, G_N_ELEMENTS (exported_entries)); @@ -808,6 +809,7 @@ test_dbus_export (void) g_main_context_iteration (NULL, TRUE); v = g_dbus_connection_call_finish (bus, async_result, &error); + g_assert_no_error (error); g_assert_nonnull (v); /* FIXME: there's an extra level of tuplelization in here */ g_variant_get (v, "((bgav))", &enabled, ¶m, &iter); From fec865cb0c658a0f79af13189fc1408304861974 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 11 Nov 2022 00:49:35 +0000 Subject: [PATCH 12/13] gapplicationcommandline: Add a missing transfer annotation Signed-off-by: Philip Withnall --- gio/gapplicationcommandline.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gio/gapplicationcommandline.c b/gio/gapplicationcommandline.c index 9fe52da5d..80065c163 100644 --- a/gio/gapplicationcommandline.c +++ b/gio/gapplicationcommandline.c @@ -795,7 +795,7 @@ g_application_command_line_get_exit_status (GApplicationCommandLine *cmdline) * * For local invocation, it will be %NULL. * - * Returns: (nullable): the platform data, or %NULL + * Returns: (nullable) (transfer full): the platform data, or %NULL * * Since: 2.28 **/ From 5f51cc844cbcb06e6b863482f39c9a612d017c10 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 15 Nov 2022 19:35:34 +0000 Subject: [PATCH 13/13] gapplicationcommandline: Fix a minor typo in the documentation Signed-off-by: Philip Withnall --- gio/gapplicationcommandline.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gio/gapplicationcommandline.c b/gio/gapplicationcommandline.c index 80065c163..b8dc460c9 100644 --- a/gio/gapplicationcommandline.c +++ b/gio/gapplicationcommandline.c @@ -497,7 +497,7 @@ g_application_command_line_get_arguments (GApplicationCommandLine *cmdline, * g_application_command_line_get_options_dict: * @cmdline: a #GApplicationCommandLine * - * Gets the options there were passed to g_application_command_line(). + * Gets the options that were passed to g_application_command_line(). * * If you did not override local_command_line() then these are the same * options that were parsed according to the #GOptionEntrys added to the