GNOME Bugzilla – Bug 666541
Add introspection support
Last modified: 2015-01-03 21:45:55 UTC
Created attachment 203887 [details] [review] add introspection support add Gobject introspection support to libgdict
I want this in master :)
(In reply to comment #1) > I want this in master :) Hi, the main goal I had in mind when I wrote this patch was adding dictionary integration to GNOME shell, but after discussing with ebassi it turned out that libgdict is probabily not suitable as search provider, since it works only with the old DICT protocol. Something new is required if we want other dictionary sources like Wiktionary (see for example the Dbpedia project and its Wiktionary spin-off described in http://lists.wikimedia.org/pipermail/wiktionary-l/2011-December/001100.html)
Well. Is it good reason to drop the patch? Dict protocol can be usable by someone else if not gnome-shell.
For example i going to implement language learning app. It would be nice to use gdict for this.
Review of attachment 203887 [details] [review]: looks generally okay, but a bit out of date. ::: configure.ac @@ +162,3 @@ +# Introspection +GOBJECT_INTROSPECTION_CHECK([0.9.3]) this should likely be updated, and made unconditional: we always want to generate introspection data. ::: libgdict/gdict-client-context.c @@ +504,3 @@ /** * gdict_client_context_new: + * @hostname: (allow-none): the hostname of a dictionary server, this should be (optional), now that we have it. @@ +530,3 @@ * gdict_client_context_set_hostname: * @context: a #GdictClientContext + * @hostname: (allow-none): the hostname of a Dictionary server, or %NULL this should be (optional). @@ +611,3 @@ * gdict_client_context_set_client: * @context: a #GdictClientContext + * @client: (allow-none): the client name to use, or %NULL this should be (optional). ::: libgdict/gdict-context.c @@ +332,3 @@ * gdict_context_match_word: * @context: a #GdictContext + * @database: (allow-none): a database name to search into, or %NULL for the this should be (optional) @@ +335,2 @@ * default database + * @strategy: (allow-none): a strategy name to use for matching, or %NULL for this should be (optional) @@ +376,3 @@ * gdict_context_define_word: * @context: a #GdictContext + * @database: (allow-none): a database name to search into, or %NULL for the this should be (optional) ::: libgdict/gdict-defbox.c @@ +2783,3 @@ * gdict_defbox_get_text: * @defbox: a #GdictDefbox + * @length: (out) (allow-none): return location for the text length or %NULL this should probably be (out) (nullable), but it's worth checking for annotations of similar API in GTK+. @@ +2819,3 @@ * gdict_defbox_set_font_name: * @defbox: a #GdictDefbox + * @font_name: (allow-none): a font description, or %NULL this should be (optional) ::: libgdict/gdict-source-chooser.c @@ +446,3 @@ * gdict_source_chooser_set_loader: * @chooser: a #GdictSourceChooser + * @loader: (allow-none): a #GdictSourceLoader or %NULL to unset it this should be (optional). ::: libgdict/gdict-source-loader.c @@ +417,3 @@ * gdict_source_loader_get_names: * @loader: a #GdictSourceLoader + * @length: (out) (allow-none): return location for the number of this should likely be (out) (nullable). ::: libgdict/gdict-source.c @@ +714,3 @@ * gdict_source_to_data: * @source: a #GdictSource + * @length: (out) (allow-none): return loaction for the length this should likely be (out) (nullable) @@ +820,3 @@ * gdict_source_set_description: * @source: a #GdictSource + * @description: (allow-none): a UTF-8 encoded description or %NULL this should be (optional) @@ +882,3 @@ * gdict_source_set_database: * @source: a #GdictSource + * @database: (allow-none): a UTF-8 encoded database name or %NULL this should be (optional) @@ +944,3 @@ * gdict_source_set_strategy: * @source: a #GdictSource + * @strategy: (allow-none): a UTF-8 encoded strategy or %NULL this should be (optional) ::: libgdict/gdict-strategy-chooser.c @@ +510,3 @@ * gdict_strategy_chooser_set_context: * @chooser: a #GdictStrategyChooser + * @context: (allow-none): a #GdictContext, or %NULL to unset the context this should be (optional) ::: libgdict/gdict-utils.c @@ +290,3 @@ * @widget: the widget that emits the error * @title: the primary error message + * @message: (allow-none): the secondary error message or %NULL this should be (optional)
Not an expert, but I think it is the other way around, allow-none should be replaced by nullable for (in) parameters, and by optional for (out) parameters. Anyway, I think there are a few annotations missing. I could update the patch if the original author is no longer interested.
Created attachment 291825 [details] [review] Original patch updated with review comments
Review of attachment 291825 [details] [review]: ::: configure.ac @@ +159,3 @@ +# Introspection +GOBJECT_INTROSPECTION_CHECK([0.9.3]) the new annotations come with a recent version of gobject-introspection, so the version check should be updated to reflect that. I think 1.42.0 is more appropriate. ::: libgdict/gdict-client-context.c @@ +513,3 @@ * + * Return value: (transfer full): the newly created #GdictClientContext object. + * You should free it using g_object_unref(). while you're modifying the API doc, you should remove the "You should free it using g_object_unref()" sentence. ::: libgdict/gdict-database-chooser.c @@ +1111,3 @@ * stock ID. * + * Return value: (transfer full): the newly packed button. I think this should be (transfer none): we're not returning a full reference, just a pointer; the button is owned by its container. ::: libgdict/gdict-source-chooser.c @@ +884,3 @@ * stock ID. * + * Return value: (transfer full): the newly packed button. same here, this should be (transfer none).
Created attachment 293350 [details] [review] Add introspection support to libgdict This patch incorporates comments from last review plus other minor fixes derived from the same comments.
looks good. Juan, you should probably change the author of the patch, since you substantially changed it from the original one from Stefano; you can add: Original patch by: Stefano Facchini <stefano.facchini@gmail.com> in the commit message. you should also add a reference to this bug.
Created attachment 293424 [details] [review] Add GObject introspection support to libgdict Thank you. I will commit this patch as soon as I can do ssh :)