GNOME Bugzilla – Bug 656440
GParamSpec and descendants are introspected poorly
Last modified: 2018-01-25 14:14:07 UTC
There are two issues: 1) GParamSpec is represented as class, but does not have ref/unref/setvalue/getvalue annotations, so bindings are unable to control lifetime of ParamSpec objects. 2) All GParamSpec descendants are represented as records, which is incosistent with GParamSpec being class.
Created attachment 193740 [details] [review] Avoid creating static methods with empty names For cases like g_param_spec_char() and GParamSpecChar class, scanner happily merged function as static method and assigned empty string as static method name. This patch avoids that, if pairing function to class results in empty name, no pairing is done and function is left global in the namespace.
Created attachment 193741 [details] [review] Fix reparsing of girs containing fundamentals Scanner's --reparse-validate option caused errors on girs which contained fundamental classes, because fundamental-related attributes of classes were not picked up from source gir.
Created attachment 193742 [details] [review] Fix special handling GParamSpec and derived classes in scanner Mark GParamSpec class as fundamental. All GObject types deriving from GParamSpec (like e.g. GParamSpecInt) are also marked as classes with GParamSpec as parent.
Created attachment 193743 [details] [review] Adjust GParamSpec-related annotations Mainly adds fundamental-related annotations to GParamSpec so that bindings know how to control its lifetime. Also un-(skip) many constructor and factory functions which were skipped previously from times when GParamSpec used to be introspected as non-boxed record. Note that GParamSpec uses its own sink mechanism. To handle it, g_param_spec_ref_sink() was chosen as ref-sink function instead of plain g_param_spec_ref(), and all constructors and factory methods returns are marked as (transfer none). This should instruct bindings to properly ref-sink newly created paramspec instances.
Created attachment 193744 [details] Diff of GObject-2.0.gir illustrating impact of pacthes above This is diff between original GObject-2.0.gir and the one produced with patches above applied. It is meant only for better understanding of what is the net result of the patches.
Review of attachment 193740 [details] [review]: I think this is OK, but I'd like to see this one include a test case for tests/scanner/regress.
Created attachment 193757 [details] [review] Avoid creating static methods with empty names The same patch as before + requested test
Comment on attachment 193757 [details] [review] Avoid creating static methods with empty names Obsoleted by patch inbug 572408
Review of attachment 193741 [details] [review]: One minor comment; feel free to commit either way. ::: giscanner/girparser.py @@ +246,3 @@ + 'set-value-func', 'get-value-func']: + func_name = node.attrib.get(_glibns(func_id)) + obj.__dict__[func_id.replace('-', '_')] = func_name The dynamic setting of __dict__ is clever, clearly we could do this more in the code, but I'm uncertain about doing it in just this one place. Up to you I guess.
Review of attachment 193741 [details] [review]: Committed with dynamic setting version. If dynamic setting of attrs using __dict__ is considered ok, we can start here and use it gradually as needed in other parts of scanner when they are modified.
setattr(obj, func_id.replace('-', '_'), func_name) is slightly better than modifying __dict__
(In reply to comment #11) > setattr(obj, func_id.replace('-', '_'), func_name) is slightly better than > modifying __dict__ Right, I did not know about setattr. In the end, I wonder whether it was really good idea to introduce this dynamic setting here. Should I change it to old-school static setting instead? obj.ref_func = node.attrib.get(_glibns('ref-func')) obj.unref_func = node.attrib.get(_glibns('unref-func')) obj.get_value_func = node.attrib.get(_glibns('get-value-func')) obj.set_value_func = node.attrib.get(_glibns('set-value-func'))
Review of attachment 193742 [details] [review]: Looks fine. ::: giscanner/gdumpparser.py @@ +250,3 @@ + elif (record.name.startswith('ParamSpec') + and not record.name in ('ParamSpecPool', 'ParamSpecClass', + 'ParamSpecTypeInfo')): Hmm...a little bit of a tradeoff here whether we whitelist ParamSpec subclasses or we blacklist things that aren't. I think I'm fine with this approach - it does seem less likely any new GParamSpecFoo structures will be added in the future where Foo isn't a type.
Review of attachment 193742 [details] [review]: Thanks for review, committed.
Review of attachment 193743 [details] [review]: ::: gobject/gparam.c @@ +185,2 @@ /** + * g_param_spec_ref: See below - bindings should be implementing these manually, so let's not remove the (skip). ::: gobject/gparam.h @@ +205,3 @@ + * Unref Func: g_param_spec_unref + * Set Value Func: g_value_set_param + * Get Value Func: g_value_get_param I'm not aware of either pygobject or gjs making use of these annotations - in fact I don't think they even make it into the typelib right now. So in practice, fundamental types should be hardcoded by bindings. Let's leave this out? ::: gobject/gparamspecs.c @@ +1604,3 @@ * Creates a new #GParamSpecChar instance specifying a %G_TYPE_CHAR property. * + * Returns: (transfer none): a newly created parameter specification Why (none)? It's not floating - it's a boxed. This should be (full) I think.
> ::: gobject/gparam.h > @@ +205,3 @@ > + * Unref Func: g_param_spec_unref > + * Set Value Func: g_value_set_param > + * Get Value Func: g_value_get_param > > I'm not aware of either pygobject or gjs making use of these annotations - in > fact I don't think they even make it into the typelib right now. Hmm. Lgi uses them, they are in typelib and they work well. I believe they were added because of gstreamer's GstMiniObject. Even regress.c has tests for them. > So in practice, fundamental types should be hardcoded by bindings. Let's leave > this out? Well, I would not advocate for adding fundamental object support into GI, but since we already have it, have testsuite for it and they are used in the wild - at least by gstreamer, why not support it? > ::: gobject/gparamspecs.c > @@ +1604,3 @@ > * Creates a new #GParamSpecChar instance specifying a %G_TYPE_CHAR property. > * > + * Returns: (transfer none): a newly created parameter specification > > Why (none)? It's not floating - it's a boxed. This should be (full) I think. It is not boxed. GParamSpecChar is object, it is GTypeInstance-compatible, its derived from GParamSpec which is fundamental GType, there is no GBoxed in the ancestor chain. That's also why I added those Ref Func: etc annotations above. And since GParamSpec uses its own floating ref scheme, we can utilize it; Ref Func: is g_param_spec_ref_sink, and when the constructor above is marked as (transfer none), binding is instructed to ref it, and it performs ref-sink, effectively sinking floating ref from newly created instance. I don't insist on this scheme; we can mark simple g_param_spec_ref: as Ref Func: and mark all constructors returns (transfer full), but then the paramspec instances created by bindings would be floating, which could have some bad consequences (see problems with gtk_action_get_proxies). The whole story of GParamSpec is like this:originally they were introspected as records (non-boxed ones), so all those creation functions has to be skipped. Then gcampagna pushed a patch which converted GParamSpec to a class (but all descendants remained records), and there was no information that that class has special ref/unref sematnics g_object_ref cannot be used on them). And because I internally use GParamSpec in Lgi implementation (on Lua-side of things, so I'm dependent on the interpretation of them in the typelib), I wrote these patches. If you wish, we can skip of course remove 'Ref Func: &company', but then any binding which wants to use them has to hardcode these functions. Which is IMHO shame when GI has perfectly appropriate and working construct to express this.
(In reply to comment #15) > Review of attachment 193743 [details] [review]: > > ::: gobject/gparam.c > @@ +185,2 @@ > /** > + * g_param_spec_ref: > > See below - bindings should be implementing these manually, so let's not remove > the (skip). > > ::: gobject/gparam.h > @@ +205,3 @@ > + * Unref Func: g_param_spec_unref > + * Set Value Func: g_value_set_param > + * Get Value Func: g_value_get_param > > I'm not aware of either pygobject or gjs making use of these annotations - in > fact I don't think they even make it into the typelib right now. gjs supports them, I wrote a patch some time ago (2010ish). Tomeu wrote one for pygobject, but it has not been committed yet.
[Mass-moving gobject-introspection tickets to its own Bugzilla product - see bug 708029. Mass-filter your bugmail for this message: introspection20150207 ]
I think this can be closed as obsolete.