After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 662241 - Gsettings "change-event" not fully introspectable
Gsettings "change-event" not fully introspectable
Status: RESOLVED FIXED
Product: pygobject
Classification: Bindings
Component: introspection
Git master
Other Linux
: Normal normal
: ---
Assigned To: Martin Pitt
Python bindings maintainers
Depends on:
Blocks:
 
 
Reported: 2011-10-19 21:27 UTC by marmuta
Modified: 2013-02-28 08:07 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Minimal python sample (258 bytes, application/octet-stream)
2011-10-19 21:27 UTC, marmuta
  Details
g-i: Add test case for signal with a C array and length argument (9.97 KB, patch)
2013-02-27 11:17 UTC, Martin Pitt
none Details | Review
Fix marshalling of C arrays with explicit length in signal arguments (8.95 KB, patch)
2013-02-27 12:46 UTC, Martin Pitt
committed Details | Review

Description marmuta 2011-10-19 21:27:05 UTC
Created attachment 199482 [details]
Minimal python sample

The current parameters of the call-back for Gio.Settings."change-event" appear to be
(settings, keys, n_keys), where keys is always empty.

I believe they are meant to be
(settings, keys), with "keys" actually containing the list of changes keys.

There is also this assert, when the event fires:
/usr/lib/python2.7/dist-packages/gi/types.py:43: Warning: g_value_get_boxed: assertion `G_VALUE_HOLDS_BOXED (value)' failed
  return info.invoke(*args, **kwargs)


Output of the attached python sample:
/usr/lib/python2.7/dist-packages/gi/types.py:43: Warning: g_value_get_boxed: assertion `G_VALUE_HOLDS_BOXED (value)' failed
  return info.invoke(*args, **kwargs)
<Settings object at 0x7f675bd61780 (GSettings at 0xb41b70)> [] 1


Ubuntu bug report
https://bugs.launchpad.net/ubuntu/+source/gobject-introspection/+bug/878505

This is on Ubuntu 11.10, with gir1.2-glib-2.0 1.30.0-0ubuntu2
Comment 1 Martin Pitt 2012-03-06 15:44:49 UTC
Indeed it looks like the g-i scanner gets the annotation wrong. The source does

  /**
   * GSettings::change-event:
   * @settings: the object on which the signal was emitted
   * @keys: (array length=n_keys) (element-type GQuark) (allow-none):
   *        an array of #GQuark<!-- -->s for the changed keys, or %NULL
   * @n_keys: the length of the @keys array, or 0
[...]

which comes out as

          <parameter name="keys" transfer-ownership="none" allow-none="1">
            <doc xml:whitespace="preserve">an array of #GQuark&lt;!-- --&gt;s for the changed keys, or %NULL</doc>
            <array length="1" zero-terminated="0">
              <type name="GLib.Quark"/>
            </array>
          </parameter>
          <parameter name="n_keys" transfer-ownership="none">
            <doc xml:whitespace="preserve">the length of the @keys array, or 0</doc>
            <type name="gint"/>
          </parameter>

I. e. it thinks keys is null-terminated.
Comment 2 Martin Pitt 2012-03-06 15:45:45 UTC
*brown paperbag* sorry, that was bogus of course. The .gir looks fine.
Comment 3 Martin Pitt 2012-03-06 15:50:51 UTC
Moving to pygobject, it doesn't call the signal handler correctly.
Comment 4 Sebastian Pölsterl 2012-04-21 13:58:05 UTC
Tried with pygobject 3.2.0: the callback is never called.
Comment 5 Martin Pitt 2013-02-27 09:50:02 UTC
The problem is that the GValue that is supposed to transmit the GLib.Quarks of the changed keys is passed on as a bare gpointer:

  • #3 g_value_get_boxed
    at gboxed.c line 433
  • #4 _pygi_argument_from_g_value
    at pygi-argument.c line 2050
  • #5 pygi_signal_closure_marshal
    at pygi-signal-closure.c line 141
  • #6 g_closure_invoke
    at gclosure.c line 777
  • #7 signal_emit_unlocked_R
    at gsignal.c line 3584
  • #8 g_signal_emit_valist
    at gsignal.c line 3338
  • #9 g_signal_emit
    at gsignal.c line 3384
  • #10 settings_backend_changed
    at gsettings.c line 348

(gdb) p value->g_type
$2 = 68
(gdb) call g_type_name(68)
$3 = (const gchar *) 0x2aaaad17ae85 "gpointer"
(gdb) call g_value_get_pointer(value)
$5 = (void *) 0x7fffffffa6a0
(gdb) p *((int*) 0x7fffffffa6a0)
$7 = 171

This basically does what it says on the tin:

          <parameter name="keys" transfer-ownership="none" allow-none="1">
            <doc xml:whitespace="preserve">an array of #GQuark&lt;!-- --&gt;s for the changed keys, or %NULL</doc>
            <array length="1" zero-terminated="0" c:type="gpointer">
              <type name="GLib.Quark"/>
            </array>
          </parameter>

When I fix the obvious problem in _pygi_argument_from_g_value() (using g_value_get_pointer()), I'm stuck at _pygi_argument_to_array(), which is unable to determine the array's length field:

** (process:18605): CRITICAL **: Unable to determine array length for 0x7fffffffa6a0

(gdb) call g_type_info_get_array_fixed_size(type_info)
$3 = -1

Without this, we have no chance of picking apart the array.
Comment 6 Martin Pitt 2013-02-27 11:17:23 UTC
Created attachment 237510 [details] [review]
g-i: Add test case for signal with a C array and length argument

Argh, I was led to a totally wrong path. Of course g_type_info_get_array_fixed_size() returns -1, it's not fixed size, it's g_type_info_get_array_length() and that works fine. We just fail to pass the other arguments to _pygi_argument_to_array() somehow, so this is within pygobject.

Not sure whether we want this new test case in gobject-introspection, I'll ask Colin. I attach it here to avoid losing it.
Comment 7 Martin Pitt 2013-02-27 12:46:19 UTC
Created attachment 237519 [details] [review]
Fix marshalling of C arrays with explicit length in signal arguments

That's the fix, review appreciated. This obsoletes the original "minimal python example" as it incorporates it in a new test case.
Comment 8 Martin Pitt 2013-02-27 14:41:57 UTC
Comment on attachment 237519 [details] [review]
Fix marshalling of C arrays with explicit length in signal arguments

I kind of accidentally pushed this when pushing another fix; I forgot to back this out until review.

But anyway, I believe it's good. Review still appreciated, though, we can always fix stuff in a followup commit.
Comment 9 Simon Feltman 2013-02-28 02:59:18 UTC
Review of attachment 237519 [details] [review]:

I think it would be nice to break this down a bit more and have the callers of _pygi_argument_to_array first use smaller specialized functions to determine array length depending on their context. So for instance signal closure marshaling might use:

gssize length = _pygi_argument_get_array_length_with_g_values (&arg, &args, callable_info, &arg_type);
arg.v_pointer = _pygi_argument_to_array (&arg, length, signal_info, &type_info, &free_array);

and pygi-closure can use something like:

gssize length = _pygi_argument_get_array_length_with_gi_arguments (&arg, &args, callable_info, &arg_type);
arg.v_pointer = _pygi_argument_to_array (&arg, length, signal_info, &type_info, &free_array);

Basically trying to separate how array length is determined from how arrays are converted. This will allow some amount of re-use for _pygi_marshal_to_py_array. And of course, this also doesn't need to happen right now...

::: gi/pygi-argument.c
@@ +2068,3 @@
+            else
+                /* e. g. GSettings::change-event */
+                arg.v_pointer = g_value_get_pointer (value);

Can you explain this some more?
Comment 10 Martin Pitt 2013-02-28 08:07:32 UTC
(In reply to comment #9)
> I think it would be nice to break this down a bit more and have the callers of
> _pygi_argument_to_array first use smaller specialized functions to determine
> array length depending on their context.

Indeed, this would be more modular and a bit easier to maintain.

> ::: gi/pygi-argument.c
> @@ +2068,3 @@
> +            else
> +                /* e. g. GSettings::change-event */
> +                arg.v_pointer = g_value_get_pointer (value);
> 
> Can you explain this some more?

It's defined like this:

  g_settings_signals[SIGNAL_CHANGE_EVENT] =
    g_signal_new ("change-event", G_TYPE_SETTINGS,
                  G_SIGNAL_RUN_LAST,
                  G_STRUCT_OFFSET (GSettingsClass, change_event),
                  g_signal_accumulator_true_handled, NULL,
                  NULL,
                  G_TYPE_BOOLEAN, 2, G_TYPE_POINTER, G_TYPE_INT);

i. e. it is passed a GPointer GValue, not a GBoxed as usual. So g_value_get_boxed() on the "keys" argument just throws a critical and fails.