From 8b443c35561e1bd3f76696b1f1f4889871a957f2 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 16 Jun 2021 11:40:38 +0100 Subject: [PATCH 1/5] gdelayedsettingsbackend: Fix applying after calling g_settings_reset() `g_settings_reset()` changes the value of the setting to `NULL`; `add_to_tree()` was not handling that correctly. Add a unit test. Signed-off-by: Philip Withnall Fixes: #2426 --- gio/gdelayedsettingsbackend.c | 3 +- gio/tests/gsettings.c | 55 +++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 1 deletion(-) diff --git a/gio/gdelayedsettingsbackend.c b/gio/gdelayedsettingsbackend.c index 93ced25db..6fff6f71c 100644 --- a/gio/gdelayedsettingsbackend.c +++ b/gio/gdelayedsettingsbackend.c @@ -156,7 +156,8 @@ add_to_tree (gpointer key, gpointer value, gpointer user_data) { - g_tree_insert (user_data, g_strdup (key), g_variant_ref (value)); + /* A value may be %NULL if its key has been reset */ + g_tree_insert (user_data, g_strdup (key), (value != NULL) ? g_variant_ref (value) : NULL); return FALSE; } diff --git a/gio/tests/gsettings.c b/gio/tests/gsettings.c index 42eeccd7a..a53f5e4be 100644 --- a/gio/tests/gsettings.c +++ b/gio/tests/gsettings.c @@ -56,6 +56,15 @@ check_and_free (GVariant *value, g_variant_unref (value); } +/* Wrapper around g_assert_cmpstr() which gets a setting from a #GSettings + * using g_settings_get(). */ +#define settings_assert_cmpstr(settings, key, op, expected_value) G_STMT_START { \ + gchar *__str; \ + g_settings_get ((settings), (key), "s", &__str); \ + g_assert_cmpstr (__str, op, (expected_value)); \ + g_free (__str); \ +} G_STMT_END + /* Just to get warmed up: Read and set a string, and * verify that can read the changed string back @@ -670,6 +679,51 @@ test_delay_child (void) g_object_unref (base); } +static void +test_delay_reset_key (void) +{ + GSettings *direct_settings = NULL, *delayed_settings = NULL; + + g_test_summary ("Test that resetting a key on a delayed settings instance works"); + + delayed_settings = g_settings_new ("org.gtk.test"); + direct_settings = g_settings_new ("org.gtk.test"); + + g_settings_set (direct_settings, "greeting", "s", "ey up"); + + settings_assert_cmpstr (delayed_settings, "greeting", ==, "ey up"); + + /* Set up a delayed settings backend. */ + g_settings_delay (delayed_settings); + + g_settings_set (delayed_settings, "greeting", "s", "how do"); + + settings_assert_cmpstr (delayed_settings, "greeting", ==, "how do"); + settings_assert_cmpstr (direct_settings, "greeting", ==, "ey up"); + + g_assert_true (g_settings_get_has_unapplied (delayed_settings)); + + g_settings_reset (delayed_settings, "greeting"); + + /* There are still unapplied settings, because the reset is resetting to the + * value from the schema, not the value from @direct_settings. */ + g_assert_true (g_settings_get_has_unapplied (delayed_settings)); + + settings_assert_cmpstr (delayed_settings, "greeting", ==, "Hello, earthlings"); + settings_assert_cmpstr (direct_settings, "greeting", ==, "ey up"); + + /* Apply the settings changes (i.e. the reset). */ + g_settings_apply (delayed_settings); + + g_assert_false (g_settings_get_has_unapplied (delayed_settings)); + + settings_assert_cmpstr (delayed_settings, "greeting", ==, "Hello, earthlings"); + settings_assert_cmpstr (direct_settings, "greeting", ==, "Hello, earthlings"); + + g_object_unref (direct_settings); + g_object_unref (delayed_settings); +} + static void keys_changed_cb (GSettings *settings, const GQuark *keys, @@ -3114,6 +3168,7 @@ main (int argc, char *argv[]) g_test_add_func ("/gsettings/delay-apply", test_delay_apply); g_test_add_func ("/gsettings/delay-revert", test_delay_revert); g_test_add_func ("/gsettings/delay-child", test_delay_child); + g_test_add_func ("/gsettings/delay-reset-key", test_delay_reset_key); g_test_add_func ("/gsettings/atomic", test_atomic); g_test_add_func ("/gsettings/simple-binding", test_simple_binding); From 05523b75032bad92081a31285bb29ca773c9541a Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 16 Jun 2021 11:52:29 +0100 Subject: [PATCH 2/5] tests: Use a helper macro to drop redundant code in gsettings test This introduces no functional changes. Signed-off-by: Philip Withnall --- gio/tests/gsettings.c | 129 ++++++++---------------------------------- 1 file changed, 24 insertions(+), 105 deletions(-) diff --git a/gio/tests/gsettings.c b/gio/tests/gsettings.c index a53f5e4be..0b19f2a67 100644 --- a/gio/tests/gsettings.c +++ b/gio/tests/gsettings.c @@ -97,15 +97,10 @@ test_basic (void) g_object_unref (b); g_free (path); - g_settings_get (settings, "greeting", "s", &str); - g_assert_cmpstr (str, ==, "Hello, earthlings"); - g_free (str); + settings_assert_cmpstr (settings, "greeting", ==, "Hello, earthlings"); g_settings_set (settings, "greeting", "s", "goodbye world"); - g_settings_get (settings, "greeting", "s", &str); - g_assert_cmpstr (str, ==, "goodbye world"); - g_free (str); - str = NULL; + settings_assert_cmpstr (settings, "greeting", ==, "goodbye world"); if (!backend_set && g_test_undefined ()) { @@ -119,10 +114,7 @@ test_basic (void) g_object_unref (tmp_settings); } - g_settings_get (settings, "greeting", "s", &str); - g_assert_cmpstr (str, ==, "goodbye world"); - g_free (str); - str = NULL; + settings_assert_cmpstr (settings, "greeting", ==, "goodbye world"); g_settings_reset (settings, "greeting"); str = g_settings_get_string (settings, "greeting"); @@ -351,10 +343,7 @@ test_basic_types (void) g_settings_get (settings, "test-double", "d", &d); g_assert_cmpfloat (d, ==, G_MAXDOUBLE); - g_settings_get (settings, "test-string", "s", &str); - g_assert_cmpstr (str, ==, "a string, it seems"); - g_free (str); - str = NULL; + settings_assert_cmpstr (settings, "test-string", ==, "a string, it seems"); g_settings_get (settings, "test-objectpath", "o", &str); g_assert_cmpstr (str, ==, "/a/object/path"); @@ -493,7 +482,6 @@ test_delay_apply (void) { GSettings *settings; GSettings *settings2; - gchar *str; gboolean writable; GVariant *v; const gchar *s; @@ -539,20 +527,14 @@ test_delay_apply (void) writable = g_settings_is_writable (settings, "greeting"); g_assert_true (writable); - g_settings_get (settings, "greeting", "s", &str); - g_assert_cmpstr (str, ==, "greetings from test_delay_apply"); - g_free (str); - str = NULL; + settings_assert_cmpstr (settings, "greeting", ==, "greetings from test_delay_apply"); v = g_settings_get_user_value (settings, "greeting"); s = g_variant_get_string (v, NULL); g_assert_cmpstr (s, ==, "greetings from test_delay_apply"); g_variant_unref (v); - g_settings_get (settings2, "greeting", "s", &str); - g_assert_cmpstr (str, ==, "top o' the morning"); - g_free (str); - str = NULL; + settings_assert_cmpstr (settings2, "greeting", ==, "top o' the morning"); g_assert_true (g_settings_get_has_unapplied (settings)); g_assert_false (g_settings_get_has_unapplied (settings2)); @@ -565,15 +547,8 @@ test_delay_apply (void) g_assert_false (changed_cb_called); g_assert_true (changed_cb_called2); - g_settings_get (settings, "greeting", "s", &str); - g_assert_cmpstr (str, ==, "greetings from test_delay_apply"); - g_free (str); - str = NULL; - - g_settings_get (settings2, "greeting", "s", &str); - g_assert_cmpstr (str, ==, "greetings from test_delay_apply"); - g_free (str); - str = NULL; + settings_assert_cmpstr (settings, "greeting", ==, "greetings from test_delay_apply"); + settings_assert_cmpstr (settings2, "greeting", ==, "greetings from test_delay_apply"); g_assert_false (g_settings_get_has_unapplied (settings)); g_assert_false (g_settings_get_has_unapplied (settings2)); @@ -581,9 +556,7 @@ test_delay_apply (void) g_settings_reset (settings, "greeting"); g_settings_apply (settings); - g_settings_get (settings, "greeting", "s", &str); - g_assert_cmpstr (str, ==, "Hello, earthlings"); - g_free (str); + settings_assert_cmpstr (settings, "greeting", ==, "Hello, earthlings"); g_object_unref (settings2); g_object_unref (settings); @@ -597,30 +570,20 @@ test_delay_revert (void) { GSettings *settings; GSettings *settings2; - gchar *str; settings = g_settings_new ("org.gtk.test"); settings2 = g_settings_new ("org.gtk.test"); g_settings_set (settings2, "greeting", "s", "top o' the morning"); - g_settings_get (settings, "greeting", "s", &str); - g_assert_cmpstr (str, ==, "top o' the morning"); - g_free (str); + settings_assert_cmpstr (settings, "greeting", ==, "top o' the morning"); g_settings_delay (settings); g_settings_set (settings, "greeting", "s", "greetings from test_delay_revert"); - g_settings_get (settings, "greeting", "s", &str); - g_assert_cmpstr (str, ==, "greetings from test_delay_revert"); - g_free (str); - str = NULL; - - g_settings_get (settings2, "greeting", "s", &str); - g_assert_cmpstr (str, ==, "top o' the morning"); - g_free (str); - str = NULL; + settings_assert_cmpstr (settings, "greeting", ==, "greetings from test_delay_revert"); + settings_assert_cmpstr (settings2, "greeting", ==, "top o' the morning"); g_assert_true (g_settings_get_has_unapplied (settings)); @@ -628,15 +591,8 @@ test_delay_revert (void) g_assert_false (g_settings_get_has_unapplied (settings)); - g_settings_get (settings, "greeting", "s", &str); - g_assert_cmpstr (str, ==, "top o' the morning"); - g_free (str); - str = NULL; - - g_settings_get (settings2, "greeting", "s", &str); - g_assert_cmpstr (str, ==, "top o' the morning"); - g_free (str); - str = NULL; + settings_assert_cmpstr (settings, "greeting", ==, "top o' the morning"); + settings_assert_cmpstr (settings2, "greeting", ==, "top o' the morning"); g_object_unref (settings2); g_object_unref (settings); @@ -729,8 +685,6 @@ keys_changed_cb (GSettings *settings, const GQuark *keys, gint n_keys) { - gchar *str; - g_assert_cmpint (n_keys, ==, 2); g_assert_true ((keys[0] == g_quark_from_static_string ("greeting") && @@ -738,15 +692,8 @@ keys_changed_cb (GSettings *settings, (keys[1] == g_quark_from_static_string ("greeting") && keys[0] == g_quark_from_static_string ("farewell"))); - g_settings_get (settings, "greeting", "s", &str); - g_assert_cmpstr (str, ==, "greetings from test_atomic"); - g_free (str); - str = NULL; - - g_settings_get (settings, "farewell", "s", &str); - g_assert_cmpstr (str, ==, "atomic bye-bye"); - g_free (str); - str = NULL; + settings_assert_cmpstr (settings, "greeting", ==, "greetings from test_atomic"); + settings_assert_cmpstr (settings, "farewell", ==, "atomic bye-bye"); } /* Check that delay-applied changes appear atomically. @@ -758,7 +705,6 @@ test_atomic (void) { GSettings *settings; GSettings *settings2; - gchar *str; settings = g_settings_new ("org.gtk.test"); settings2 = g_settings_new ("org.gtk.test"); @@ -778,25 +724,10 @@ test_atomic (void) g_settings_apply (settings); - g_settings_get (settings, "greeting", "s", &str); - g_assert_cmpstr (str, ==, "greetings from test_atomic"); - g_free (str); - str = NULL; - - g_settings_get (settings, "farewell", "s", &str); - g_assert_cmpstr (str, ==, "atomic bye-bye"); - g_free (str); - str = NULL; - - g_settings_get (settings2, "greeting", "s", &str); - g_assert_cmpstr (str, ==, "greetings from test_atomic"); - g_free (str); - str = NULL; - - g_settings_get (settings2, "farewell", "s", &str); - g_assert_cmpstr (str, ==, "atomic bye-bye"); - g_free (str); - str = NULL; + settings_assert_cmpstr (settings, "greeting", ==, "greetings from test_atomic"); + settings_assert_cmpstr (settings, "farewell", ==, "atomic bye-bye"); + settings_assert_cmpstr (settings2, "greeting", ==, "greetings from test_atomic"); + settings_assert_cmpstr (settings2, "farewell", ==, "atomic bye-bye"); g_object_unref (settings2); g_object_unref (settings); @@ -905,13 +836,7 @@ test_l10n_context (void) setlocale (LC_MESSAGES, "de_DE.UTF-8"); /* Only do the test if translation is actually working... */ if (g_str_equal (dgettext ("test", "\"Unnamed\""), "\"Unbenannt\"")) - { - g_settings_get (settings, "backspace", "s", &str); - - g_assert_cmpstr (str, ==, "Löschen"); - g_free (str); - str = NULL; - } + settings_assert_cmpstr (settings, "backspace", ==, "Löschen"); else g_printerr ("warning: translation is not working... skipping test. "); @@ -2821,14 +2746,10 @@ test_null_backend (void) g_assert_cmpstr (str, ==, "org.gtk.test"); g_free (str); - g_settings_get (settings, "greeting", "s", &str); - g_assert_cmpstr (str, ==, "Hello, earthlings"); - g_free (str); + settings_assert_cmpstr (settings, "greeting", ==, "Hello, earthlings"); g_settings_set (settings, "greeting", "s", "goodbye world"); - g_settings_get (settings, "greeting", "s", &str); - g_assert_cmpstr (str, ==, "Hello, earthlings"); - g_free (str); + settings_assert_cmpstr (settings, "greeting", ==, "Hello, earthlings"); writable = g_settings_is_writable (settings, "greeting"); g_assert_false (writable); @@ -2838,9 +2759,7 @@ test_null_backend (void) g_settings_delay (settings); g_settings_set (settings, "greeting", "s", "goodbye world"); g_settings_apply (settings); - g_settings_get (settings, "greeting", "s", &str); - g_assert_cmpstr (str, ==, "Hello, earthlings"); - g_free (str); + settings_assert_cmpstr (settings, "greeting", ==, "Hello, earthlings"); g_object_unref (settings); g_object_unref (backend); From 3db22ab8db84bf64bd36d89c51ff573a7497565c Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 26 Oct 2021 14:02:54 +0100 Subject: [PATCH 3/5] gsettings: Improve documentation formatting slightly Signed-off-by: Philip Withnall --- gio/gsettings.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gio/gsettings.c b/gio/gsettings.c index c189a2e62..5ebac1170 100644 --- a/gio/gsettings.c +++ b/gio/gsettings.c @@ -2424,7 +2424,7 @@ g_settings_is_writable (GSettings *settings, * @settings. * * The schema for the child settings object must have been declared - * in the schema of @settings using a element. + * in the schema of @settings using a `` element. * * Returns: (not nullable) (transfer full): a 'child' settings object * From 0101ccba16419ab20ebfc2cbd5aa91924494f890 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 26 Oct 2021 14:10:19 +0100 Subject: [PATCH 4/5] gsettings: Clarify that g_settings_get_child() inherits delay-apply Previously, the delay-apply status of the parent `GSettings` object would be partially inherited: `settings->priv->backend` in the child `GSettings` object would point to a `GDelayedSettingsBackend`, but `settings->priv->delayed` would be `NULL`. The expectation from https://bugzilla.gnome.org/show_bug.cgi?id=720891 was that `get_child()` would fully inherit delay-apply status. So, ensure that `settings->priv->delayed` is correctly set to point to the delayed backend when constructing any `GSettings`. Update the tests to work again (presumably the inverted test was an oversight in the original changes). Signed-off-by: Philip Withnall Fixes: #2426 --- gio/gsettings.c | 16 +++++++++++++++- gio/tests/gsettings.c | 7 ++++++- 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/gio/gsettings.c b/gio/gsettings.c index 5ebac1170..711929cff 100644 --- a/gio/gsettings.c +++ b/gio/gsettings.c @@ -604,6 +604,11 @@ g_settings_set_property (GObject *object, case PROP_BACKEND: settings->priv->backend = g_value_dup_object (value); + if (G_IS_DELAYED_SETTINGS_BACKEND (settings->priv->backend)) + { + g_assert (settings->priv->delayed == NULL); + settings->priv->delayed = G_DELAYED_SETTINGS_BACKEND (settings->priv->backend); + } break; default: @@ -680,7 +685,13 @@ g_settings_constructed (GObject *object) } if (settings->priv->backend == NULL) - settings->priv->backend = g_settings_backend_get_default (); + { + settings->priv->backend = g_settings_backend_get_default (); + + /* The default had better not be delayed, otherwise we also need to set + * settings->priv->delayed. */ + g_assert (!G_IS_DELAYED_SETTINGS_BACKEND (settings->priv->backend)); + } g_settings_backend_watch (settings->priv->backend, &listener_vtable, G_OBJECT (settings), @@ -2426,6 +2437,9 @@ g_settings_is_writable (GSettings *settings, * The schema for the child settings object must have been declared * in the schema of @settings using a `` element. * + * The created child settings object will inherit the #GSettings:delay-apply + * mode from @settings. + * * Returns: (not nullable) (transfer full): a 'child' settings object * * Since: 2.26 diff --git a/gio/tests/gsettings.c b/gio/tests/gsettings.c index 0b19f2a67..dad1623b7 100644 --- a/gio/tests/gsettings.c +++ b/gio/tests/gsettings.c @@ -619,7 +619,7 @@ test_delay_child (void) g_assert_nonnull (child); g_object_get (child, "delay-apply", &delay, NULL); - g_assert_false (delay); + g_assert_true (delay); g_settings_get (child, "test-byte", "y", &byte); g_assert_cmpuint (byte, ==, 36); @@ -630,6 +630,11 @@ test_delay_child (void) g_settings_get (base, "test-byte", "y", &byte); g_assert_cmpuint (byte, ==, 36); + /* apply the child and the changes should be saved */ + g_settings_apply (child); + g_settings_get (base, "test-byte", "y", &byte); + g_assert_cmpuint (byte, ==, 42); + g_object_unref (child); g_object_unref (settings); g_object_unref (base); From 44cbba5d78f7cf166498436f513edb5b9e02e13d Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 26 Oct 2021 14:29:31 +0100 Subject: [PATCH 5/5] gsettings: Drop internal delayed member MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This introduces no functional changes; it only simplifies the code. Instead of maintaining a separate pointer to the backend iff it’s a `GDelayedSettingsBackend`, just test the `backend` pointer’s type. Signed-off-by: Philip Withnall Helps: #2426 --- gio/gsettings.c | 36 ++++++++++++------------------------ 1 file changed, 12 insertions(+), 24 deletions(-) diff --git a/gio/gsettings.c b/gio/gsettings.c index 711929cff..e96c94e11 100644 --- a/gio/gsettings.c +++ b/gio/gsettings.c @@ -343,8 +343,6 @@ struct _GSettingsPrivate GSettingsBackend *backend; GSettingsSchema *schema; gchar *path; - - GDelayedSettingsBackend *delayed; }; enum @@ -604,11 +602,6 @@ g_settings_set_property (GObject *object, case PROP_BACKEND: settings->priv->backend = g_value_dup_object (value); - if (G_IS_DELAYED_SETTINGS_BACKEND (settings->priv->backend)) - { - g_assert (settings->priv->delayed == NULL); - settings->priv->delayed = G_DELAYED_SETTINGS_BACKEND (settings->priv->backend); - } break; default: @@ -647,7 +640,7 @@ g_settings_get_property (GObject *object, break; case PROP_DELAY_APPLY: - g_value_set_boolean (value, settings->priv->delayed != NULL); + g_value_set_boolean (value, G_IS_DELAYED_SETTINGS_BACKEND (settings->priv->backend)); break; default: @@ -685,13 +678,7 @@ g_settings_constructed (GObject *object) } if (settings->priv->backend == NULL) - { - settings->priv->backend = g_settings_backend_get_default (); - - /* The default had better not be delayed, otherwise we also need to set - * settings->priv->delayed. */ - g_assert (!G_IS_DELAYED_SETTINGS_BACKEND (settings->priv->backend)); - } + settings->priv->backend = g_settings_backend_get_default (); g_settings_backend_watch (settings->priv->backend, &listener_vtable, G_OBJECT (settings), @@ -2267,19 +2254,20 @@ g_settings_set_strv (GSettings *settings, void g_settings_delay (GSettings *settings) { + GDelayedSettingsBackend *delayed = NULL; + g_return_if_fail (G_IS_SETTINGS (settings)); - if (settings->priv->delayed) + if (G_IS_DELAYED_SETTINGS_BACKEND (settings->priv->backend)) return; - settings->priv->delayed = - g_delayed_settings_backend_new (settings->priv->backend, - settings, - settings->priv->main_context); + delayed = g_delayed_settings_backend_new (settings->priv->backend, + settings, + settings->priv->main_context); g_settings_backend_unwatch (settings->priv->backend, G_OBJECT (settings)); g_object_unref (settings->priv->backend); - settings->priv->backend = G_SETTINGS_BACKEND (settings->priv->delayed); + settings->priv->backend = G_SETTINGS_BACKEND (delayed); g_settings_backend_watch (settings->priv->backend, &listener_vtable, G_OBJECT (settings), settings->priv->main_context); @@ -2299,7 +2287,7 @@ g_settings_delay (GSettings *settings) void g_settings_apply (GSettings *settings) { - if (settings->priv->delayed) + if (G_IS_DELAYED_SETTINGS_BACKEND (settings->priv->backend)) { GDelayedSettingsBackend *delayed; @@ -2322,7 +2310,7 @@ g_settings_apply (GSettings *settings) void g_settings_revert (GSettings *settings) { - if (settings->priv->delayed) + if (G_IS_DELAYED_SETTINGS_BACKEND (settings->priv->backend)) { GDelayedSettingsBackend *delayed; @@ -2347,7 +2335,7 @@ g_settings_get_has_unapplied (GSettings *settings) { g_return_val_if_fail (G_IS_SETTINGS (settings), FALSE); - return settings->priv->delayed && + return G_IS_DELAYED_SETTINGS_BACKEND (settings->priv->backend) && g_delayed_settings_backend_get_has_unapplied ( G_DELAYED_SETTINGS_BACKEND (settings->priv->backend)); }