GNOME Bugzilla – Bug 795735
Fix comparison for GVariant property values
Last modified: 2018-05-04 17:25:25 UTC
Created attachment 371593 [details] [review] patch This was causing g_param_value_defaults to return 1 for GVariant values even when the value is clearly different from the default. This was showing up as gtk-builder-tool stripping non-default values for GtkActionable::action-target from ui files.
Review of attachment 371593 [details] [review]: Ouch, nice catch. Please push to master and glib-2-56.
It would be good to add a unit test for this too.
Pushed by Matthias as: • master: 566e64a660549cd49741f244f6362af47eae3757 • glib-2-56: 623f92ed2df41265c11c3ca1e03176033045d4de unfortunately neither were pushed with bug references, so archaeology on this is going to be hard in future. Matthias, are you not using `git bz push`? Leaving the bug open until the unit test is written.
So it turns out there is a test which covers this already, and it’s now failing. From https://gitlab.gnome.org/GNOME/glib/-/jobs/28588: G_TEST_SRCDIR='/opt/gnome/source/glib/tests' G_TEST_BUILDDIR='/opt/gnome/build/glib/tests' G_DEBUG='gc-friendly' MALLOC_CHECK_='2' /opt/gnome/build/glib/tests/gobject/paramspec-test-gobject --- stdout --- /paramspec/char: OK /paramspec/string: OK /paramspec/override: OK /paramspec/gtype: OK /paramspec/variant: --- stderr --- ** ERROR:../../source/glib/tests/gobject/paramspec-test.c:241:test_param_spec_variant: assertion failed: (g_param_value_defaults (pspec, &value)) The code in the test looks good to me. Looking back at the code in gparamspecs.c, it does look quite suspicious that param_variant_values_cmp() is comparing the GVariant* //pointers// with < and >. Shouldn’t it be comparing their values, perhaps using g_variant_compare()?
Created attachment 371658 [details] [review] gobject: Reimplement g_param_values_cmp() for GParamSpecVariant The existing implementation was completely incorrect (despite the fix in commit 566e64a66) — it always compared GVariants by pointer, rather than by value. Reimplement it to compare them by value where possible, depending on their type. The core of this implementation is g_variant_compare(). See the documentation and tests for further details of the new sort order. This adds documentation and tests. Signed-off-by: Philip Withnall <withnall@endlessm.com>
Since this change caused the paramspec-test test to fail on master and glib-2-56, I’ve reverted it on both branches (revert commits a06117d06, 69924c45b) because it’s making the CI unusable and doesn’t actually fix the root cause of the bug.
Review of attachment 371658 [details] [review]: ::: gobject/gparamspecs.c @@ +1148,3 @@ } +/* g_variant_compare() can only be used with scalar types. */ But... you are not using g_variant_compare, you use g_variant_equal, which can handle these types, I believe.
Review of attachment 371658 [details] [review]: ::: gobject/gparamspecs.c @@ +1148,3 @@ } +/* g_variant_compare() can only be used with scalar types. */ I am using both. g_variant_compare() is on the last line of param_variant_values_cmp(). If g_variant_compare() can be used, it’s used. Otherwise g_variant_equal() is used, and the return value (-1 or 1) for non-equal values is documented as undefined.
Review of attachment 371658 [details] [review]: ::: gobject/gparamspecs.c @@ +1148,3 @@ } +/* g_variant_compare() can only be used with scalar types. */ Ah, I the last line of the function was scrolled off here. You do use compare at the end, so this makes perfect sense. @@ +1179,3 @@ + if (!g_variant_type_equal (g_variant_get_type (v1), g_variant_get_type (v2)) || + variant_is_incomparable (v1) || + variant_is_incomparable (v2)) variant_is_incomparable only looks at the type, so it is enough to just check one of them.
Thanks, rebased and pushed to master. I will push to glib-2-56 shortly. Attachment 371658 [details] pushed as cc4de80 - gobject: Reimplement g_param_values_cmp() for GParamSpecVariant
Pushed to glib-2-56 as: 0489f609c (HEAD -> glib-2-56, origin/glib-2-56) gobject: Reimplement g_param_values_cmp() for GParamSpecVariant