GNOME Bugzilla – Bug 571548
generic typelib attributes
Last modified: 2015-02-07 16:59:05 UTC
Right now the typelib has some code for key/value string annotations associated with a GIBaseInfo (an offset in the typelib), but there is no support higher in the toolchain. We should add full support from C code all the way into typelib; this gives both us and people higher up the stack (from bindings to applications) an easy way to add features later. My proposal, in C code: (annotation gobject.singleton 1) (annotation dbus.service org.gnome.Rhythmbox) (annotation gi.ref-function foo_attrs_ref) So these are freeform string-value pairs, with some ad-hoc namespacing (we could go the full reverse DNS org.foo.Bar namespacing too if people think we should). In the .gir: <class name="Foo" ...> <annotation name="gobject.singleton" value="1"/> ... </class> <record name="Attrs"> <annotation name="gi.ref-function" value="foo_attrs_ref"> <function name="foo_attrs_ref"> ... The API: const gchar * g_base_info_get_annotation (GIBaseInfo *info, const gchar *name); already exists. The question is if it works, since no one can really have been using it.
This looks good, I think putting at the GIBaseInfo makes most sense. I'm not sure about namespacing, perhaps it full reverse DNS makes sense. However org.gtk.gobject.singleton is a bit long.
Created attachment 129102 [details] [review] partial patch for annotation parsing This patch attempts to handle parsing and writing annotations to XML, but it appears to trigger a bug in cPython. Needs work/investigation.
Created attachment 129173 [details] [review] Bug 571548 - Generic annotations We now support an extensible mechanism where arbitrary key-value pairs may be associated with almost all items, including objects, methods, and properties. These annotations appear in both the .gir and the .typelib.
(In reply to comment #3) > Created an attachment (id=129173) [edit] > Bug 571548 - Generic annotations > > We now support an extensible mechanism where arbitrary key-value > pairs may be associated with almost all items, including objects, > methods, and properties. > > These annotations appear in both the .gir and the .typelib. > Looks pretty good. - Possible naming conflict, gtk-doc syntax annotation vs typelib annotation, ideally one of them should be called something else. But what? - Should we add a get_annotations() method? - Should get_annotation be called get_annotation_by_name() - There's no gtk-doc syntax for generic annotations - Remove the g_printerr? + *size_p += ALIGN_VALUE (strlen (key_str) + 1, 4); + *size_p += ALIGN_VALUE (strlen (value_str) + 1, 4); 4 -> sizeof(guint32) ?
(In reply to comment #4) > - Possible naming conflict, gtk-doc syntax annotation vs typelib annotation, > ideally one of them should be called something else. But what? I think what we want to do is make the gtk-doc metadata unified with the introspection metadata. The gtk-doc manual should list (or link to) all of the introspection ones, gtk-doc itself should understand each one intelligently, etc. So it seems this patch would require a gtk-doc patch to appear nicely. That's something to keep in mind, unless we have another way to do it? > - Should we add a get_annotations() method? There's _iterate_annotations, should be sufficient I think? > - Should get_annotation be called get_annotation_by_name() If we were going to add e.g. a _by_value lookup it could have the suffix. > - Remove the g_printerr? Will fix. > + *size_p += ALIGN_VALUE (strlen (key_str) + 1, 4); > + *size_p += ALIGN_VALUE (strlen (value_str) + 1, 4); > > 4 -> sizeof(guint32) ? Hm...perhaps, but there are a ton of other ALIGN_VALUEs like this that I didn't change in this patch. I didn't change them in the typelib sizeof() patch because they're relatively easy to find.
Created attachment 129352 [details] [review] Bug 571548 - Generic annotations We now support an extensible mechanism where arbitrary key-value pairs may be associated with almost all items, including objects, methods, and properties. These annotations appear in both the .gir and the .typelib.
This one just fixes the g_printerr (though it was commented) and removes trailing whitespace to make git happy.
I really think that we need to avoid the 'annotation' name conflict before this goes in. However, I do not have any good suggestions at this point. In worst case we could just end up calling the typelib annotations 'custom annotations', but I'd avoid that if possible. If you look at the meaning for the word annotation it better suitable for typelib annotation and not gtk-doc annotation. But we've already started to call the gtk-doc annotations by that name, it'd be a bit confusing for everybody to change it now (but it's better to change earlier than later of course)
Oh, I see what you meant by conflict with gtk-doc annotation; before I thought you were referring to things like Since: which come from gtk-doc before. There's the "attribute" term but it seems more general/vague to me.
Maybe "metadata"? The other option is to be careful in the docs to distinguish between "builtin annotations" and "generic annotations". I guess I'm leaning towards either renaming to attribute, or distinguishing in the docs. Slightly closer to the latter.
Renaming to attribute now unless there are objections.
Created attachment 129961 [details] [review] Bug 571548 - Generic attributes We now support an extensible mechanism where arbitrary key-value pairs may be associated with almost all items, including objects, methods, and properties. These attributes appear in both the .gir and the .typelib.
It would be really nice to allow attributes on parameters and return values. For example, for the gdbus on gobject-introspection stuff I'm working on (will post about this later), I'd like to be able to do e.g. /** * my_frobnicator_poke_path: * @frobnicator: A #MyFrobnicator. * @object_path: (gdbus.signature o): An object path. * * Manipulate an object path. * * Returns: (gdbus.signature o): A new object path. Free with g_free(). * * Attributes: (gdbus.method PokePath) */ gchar * my_frobnicator_poke_path (MyFrobnicator *frobnicator, const gchar *object_path) { return g_strdup_printf ("/another/object/path/here%s", object_path); } to hint that the D-Bus type should be an object path, not a string. There's also a couple of bugs in the patch that got committed. I will attach patch that a) fixes the bugs in the original patch; and b) adds the feature described above.
Created attachment 163677 [details] [review] Proposed patch Proposed patch. The fact that return values are not wrapped in its own type is a bit problematic (but not as big of a problem as I originally thought) - either way, there's new API on GICallableInfo to walk the attributes for a return-value. Thanks for looking at this.
Review of attachment 163677 [details] [review]: I'd rather see this split up into two pieces, one for new functionality and another one for bug fixes. I mostly reviewed the API additions in this review. ::: girepository/gicallableinfo.c @@ +264,3 @@ + +const gchar * +g_callable_info_get_return_attribute (GICallableInfo *info, This needs to be documented and added to gi-sections.txt @@ +279,3 @@ + +static int +cmp_attribute (const void *av, Don't just copy/paste code from gibaseinfo.[ch] make the available outside of it so we don't end up duplicating code. @@ +322,3 @@ + +gboolean +g_callable_info_iterate_return_attributes (GICallableInfo *info, This needs to be documented and added to gi-sections.txt ::: giscanner/annotationparser.py @@ +631,3 @@ node.doc = tag.comment + for key in options: This kind of annotation syntax should probably be documented somewhere, there's a wiki page on live.gnome.org which is the best resource for now, until I move it to docbook.
Created attachment 163685 [details] [review] Patch 1/2 (bug-fixes)
Created attachment 163686 [details] [review] Patch 2/2 (Feature)
(In reply to comment #15) > Review of attachment 163677 [details] [review]: > > I'd rather see this split up into two pieces, one for new functionality and > another one for bug fixes. Done. > +const gchar * > +g_callable_info_get_return_attribute (GICallableInfo *info, > > This needs to be documented and added to gi-sections.txt Fixed. > +static int > +cmp_attribute (const void *av, > > Don't just copy/paste code from gibaseinfo.[ch] make the available outside of > it so we don't end up duplicating code. Fixed. > +gboolean > +g_callable_info_iterate_return_attributes (GICallableInfo *info, > > This needs to be documented and added to gi-sections.txt Fixed. > ::: giscanner/annotationparser.py > @@ +631,3 @@ > node.doc = tag.comment > > + for key in options: > > This kind of annotation syntax should probably be documented somewhere, there's > a wiki page on live.gnome.org which is the best resource for now, until I move > it to docbook. Sure, I can do that once the patch is merged.
Review of attachment 163686 [details] [review]: The rest looks good, just commit it with the removal of the duplication between gibaseinfo & gicallableinfo.c. Assuming you get a review on the first patch. ::: girepository/gicallableinfo.c @@ +288,3 @@ + +static AttributeBlob * +find_first_attribute (GICallableInfo *info, This is mostly duplicated in gibaseinfo.c, please join the two and add a _gi_real_info_find_first_attribute() or so to girepository-private.h
Created attachment 163688 [details] [review] Updated Patch 2/2 (feature)
(In reply to comment #19) > Review of attachment 163686 [details] [review]: > > The rest looks good, just commit it with the removal of the duplication between > gibaseinfo & gicallableinfo.c. > Assuming you get a review on the first patch. Thanks for the review! > ::: girepository/gicallableinfo.c > @@ +288,3 @@ > + > +static AttributeBlob * > +find_first_attribute (GICallableInfo *info, > > This is mostly duplicated in gibaseinfo.c, please join the two and add a > _gi_real_info_find_first_attribute() or so to girepository-private.h Good point - this can make the patch simpler and we won't have to expose the compare function used in bsearch() if we do this. I've fixed this in the updated patch.
Review of attachment 163688 [details] [review]: This looks good, thanks!
(In reply to comment #18) > > ::: giscanner/annotationparser.py > > @@ +631,3 @@ > > node.doc = tag.comment > > > > + for key in options: > > > > This kind of annotation syntax should probably be documented somewhere, there's > > a wiki page on live.gnome.org which is the best resource for now, until I move > > it to docbook. > > Sure, I can do that once the patch is merged. OK, I've done that now, see http://live.gnome.org/GObjectIntrospection/Annotations and http://live.gnome.org/action/info/GObjectIntrospection/Annotations?action=diff&rev2=45&rev1=44 for the diff.
(In reply to comment #16) > Created an attachment (id=163685) [details] [review] > Patch 1 [details]/2 (bug-fixes) Colin: Please review! Thanks!
Review of attachment 163685 [details] [review]: I don't quite understand this one - from my reading the nodes should be ordered by offset. Try this simple patch to print them: # diff --git a/girepository/girmodule.c b/girepository/girmodule.c # index b380912..123b534 100644 # --- a/girepository/girmodule.c # +++ b/girepository/girmodule.c # @@ -403,6 +403,14 @@ g_ir_module_build_typelib (GIrModule *module, # } # # offset_ordered_nodes = g_list_reverse (offset_ordered_nodes); # + { # + GSList *link; # + for (link = offset_ordered_nodes; link; link = link->next) # + { # + GIrNode *node = link->data; # + g_printerr ("node: %s offset: %d\n", node->name, node->offset); # + } # + } # # g_message ("header: %d entries, %d attributes", header->n_entries, header->n_attributes); # # -- # 1.7.0.1
(In reply to comment #25) > Review of attachment 163685 [details] [review]: > > I don't quite understand this one - from my reading the nodes should be ordered > by offset. Try this simple patch to print them: I of course did verify this before asserting that your assumption was wrong. And, no, the nodes were not in order. I don't know why. But I'd like to move forward with my patch set anyway...
Review of attachment 163688 [details] [review]: Very minor comments. ::: girepository/gibaseinfo.c @@ +494,3 @@ + * @blob_offset: The offset for the blob to find the first attribute for. + * + * Searches the first #AttributeBlob for @blob_offset and returns it "Searches for" ::: girepository/gicallableinfo.c @@ +299,3 @@ + * + * Both the @name and @value should be treated as constants + * and must not be freed. Hmm...I guess we can't note that in the C type system? Sort of an evil trap...but this is a low level API so I'm fine with defaulting to efficient. Can you suffix the variables with say _readonly maybe? name_readonly value_readonly?
Review of attachment 163685 [details] [review]: I think I understand this now - it's no longer true *after* your next patch when subnodes can have attributes. So, looks fine then.
(In reply to comment #26) > (In reply to comment #25) > > Review of attachment 163685 [details] [review] [details]: > > > > I don't quite understand this one - from my reading the nodes should be ordered > > by offset. Try this simple patch to print them: > > I of course did verify this before asserting that your assumption was wrong. > And, no, the nodes were not in order. I don't know why. But I'd like to move > forward with my patch set anyway... Will attach output of building the whole tree with this patch $ git diff diff --git a/girepository/girmodule.c b/girepository/girmodule.c index ae40d5f..0c7a980 100644 --- a/girepository/girmodule.c +++ b/girepository/girmodule.c @@ -411,6 +411,13 @@ g_ir_module_build_typelib (GIrModule *module, entry++; } + GList *l; + for (l = nodes_with_attributes; l != NULL; l = l->next) + { + GIrNode *node = l->data; + g_printerr ("node: %s offset: %d\n", node->name, node->offset); + } + /* GIBaseInfo expects the AttributeBlob array to be sorted on the field (offset) */ nodes_with_attributes = g_list_sort (nodes_with_attributes, node_cmp_offset_func); which goes on top of my previous patch.
(In reply to comment #29) > Will attach output of building the whole tree with this patch Based on comment 28, there's no need for this.
Hey, thanks for the review. (In reply to comment #27) > ::: girepository/gibaseinfo.c > @@ +494,3 @@ > + * @blob_offset: The offset for the blob to find the first attribute for. > + * > + * Searches the first #AttributeBlob for @blob_offset and returns it > > "Searches for" Will fix. > ::: girepository/gicallableinfo.c > @@ +299,3 @@ > + * > + * Both the @name and @value should be treated as constants > + * and must not be freed. > > Hmm...I guess we can't note that in the C type system? Sort of an evil > trap...but this is a low level API so I'm fine with defaulting to efficient. > Can you suffix the variables with say _readonly maybe? name_readonly > value_readonly? This is pretty standard C usage, not sure it's worth going all hungarian-notation-ish over... And I don't think it's really much different from g_hash_table_iter_next() or the various GVariant getters or similar APIs. FWIW, I just copy-pasted this from gbaseinfo.c's g_base_info_iterate_attributes().
Committed as http://git.gnome.org/browse/gobject-introspection/commit/?id=751ffa016e410a031028282e63f98a94cc444b7b and http://git.gnome.org/browse/gobject-introspection/commit/?id=11cfe386c37ced44a8e3efb5556bde3a43a11171 after rebasing and verifying 'make check' still passes.
(In reply to comment #31) > > ::: girepository/gicallableinfo.c > > @@ +299,3 @@ > > + * > > + * Both the @name and @value should be treated as constants > > + * and must not be freed. > > > > Hmm...I guess we can't note that in the C type system? Sort of an evil > > trap...but this is a low level API so I'm fine with defaulting to efficient. > > Can you suffix the variables with say _readonly maybe? name_readonly > > value_readonly? > > This is pretty standard C usage, not sure it's worth going all > hungarian-notation-ish over... And I don't think it's really much different > from g_hash_table_iter_next() or the various GVariant getters or similar APIs. > FWIW, I just copy-pasted this from gbaseinfo.c's > g_base_info_iterate_attributes(). Let me know if you need me to do anything here and I'l do it for both GICallableInfo and GIBaseInfo. Thanks.
Comment on attachment 163688 [details] [review] Updated Patch 2/2 (feature) Committed with a spelling fix (s/Searches/Searches for/).
(In reply to comment #33) > > Let me know if you need me to do anything here and I'l do it for both > GICallableInfo and GIBaseInfo. Thanks. Not a big deal, don't worry about it.
[Mass-moving gobject-introspection tickets to its own Bugzilla product - see bug 708029. Mass-filter your bugmail for this message: introspection20150207 ]