GNOME Bugzilla – Bug 726486
Add support for the Freebase service
Last modified: 2014-05-27 21:24:15 UTC
Even though with a funky name, I think this google service (https://developers.google.com/freebase/index) would be a great addition to libgdata, it allows for searching for well known topics, and the returned data is well structured and reliable. Some uses I can think of in the gnome desktop could be: - Fetching stock images for cities in Clocks - Certain geolocation features for Maps (there's eg "restaurant" topic and others) - Miscellaneous movie/song/band information on Videos/Music - A more able search provider than "look this up on google" I'm attaching a few patches to implement this. One of the things worth pointing out is that this API maybe stands out a bit from the rest of libgdata, I've used GDataQuery and GDataEntry as the base objects for queries/results, but most of the API in those is practically ignored. Still, libgdata looks like the best place for this API.
Created attachment 272103 [details] [review] gdata: Allow creating GDataEntries out of JSON content Freebase is hardly based on JSON, and GDataEntry seems the most sensible base for the different server responses in that service over GDataFeed.
Created attachment 272104 [details] [review] Add Freebase service This service is, according to the main site, a "A community-curated database of well-known people, places, and things", it allows searching for and offering information about a wide range of topics, in a well-structured and uniform manner. The most low-level API is the MQL query interface, that is a JSON-based language, queries consist of a data graph (according to their data schema) with blank places, that will be filled in in the reply.
Created attachment 272105 [details] [review] freebase: Add Topic query API With this API, structured data can be obtained about any Freebase ID, including localized text and references to images.
Created attachment 272106 [details] [review] freebase: Add search API This API enables searching for search terms, returning amongst other info the Freebase IDs usable on the topic API.
Created attachment 272107 [details] [review] freebase: Add helper function to retrieve images This helper function can be used together with the topic API, if a returned element is an image, this function can be used to retrieve a GInputStream to the image at a requested maximum size.
Created attachment 272108 [details] [review] demos: Add freebase demo This is a simple command line app that puts some of the api to work.
Review of attachment 272103 [details] [review]: This is unrelated to Freebase and should be pushed immediately. :-)
Review of attachment 272104 [details] [review]: This is looking good. Most of the comments below are very minor and nitpicky (sorry). ::: gdata/gdata.h @@ +144,3 @@ #include <gdata/services/tasks/gdata-tasks-task.h> +/* Google FreeBase */ s/FreeBase/Freebase/ ::: gdata/gdata.symbols @@ +1032,3 @@ +gdata_freebase_result_get_type +gdata_freebase_result_new +gdata_freebase_result_get All these need adding to gdata-sections.txt in the documentation. ::: gdata/services/freebase/gdata-freebase-query.c @@ +20,3 @@ +/** + * SECTION:gdata-freebase-query + * @short_description: GData FreeBase query object s/FreeBase/Freebase/ throughout the patch, since that seems to be what Google call it. The same for the class names: I’d prefer GDataFreebaseQuery rather than GDataFreeBaseQuery. Sorry! @@ +27,3 @@ + * + * For more details of Google FreeBase API, see the <ulink type="http" url="https://developers.google.com/freebase/v1/"> + * online documentation</ulink>. It would be useful to briefly explain what MQL is, and to explicitly link to the MQL documentation (https://developers.google.com/freebase/v1/mql-overview) as well. @@ +29,3 @@ + * online documentation</ulink>. + * + * Since: 0.XX.X I generally use ‘Since: UNRELEASED’ for this. Can you please change it throughout the patchset? Thanks. @@ +76,3 @@ + * GDataFreeBaseQuery:variant: + * + * Variant containing the MQL query. Some documentation on the meaning of the GVariant’s type would be very helpful. This documentation comment needs a ‘Since: UNRELEASED’. @@ +97,3 @@ + GDataFreeBaseQueryPrivate *priv = GDATA_FREEBASE_QUERY (self)->priv; + + if (priv->variant) The preferred libgdata coding style is: if (priv->variant != NULL) { … } @@ +157,3 @@ + copy = json_node_copy (priv->query_node); + + /* FIXME: Add limit */ FIXME comment here. @@ +198,3 @@ + * @variant: a variant containing the MQL query structure + * + * Creates a new #GDataFreeBaseQuery with the MQL query provided in a serialized form as @variant. What’s the serialisation format for MQL queries as GVariants? @@ +200,3 @@ + * Creates a new #GDataFreeBaseQuery with the MQL query provided in a serialized form as @variant. + * + * Return value: a new #GDataFreeBaseQuery This needs a (transfer full) annotation, as does gdata_freebase_query_new(). ::: gdata/services/freebase/gdata-freebase-result.c @@ +2,3 @@ +/* + * GData Client + * Copyright (C) Peteris Krisjanis 2013 <pecisk@gmail.com> Copyright line is out of date. @@ +29,3 @@ + * online documentation</ulink>. + * + * Since: 0.XX.X UNRELEASED. @@ +80,3 @@ + "Variant", "Variant holding the raw result.", + G_VARIANT_TYPE ("a{smv}"), NULL, + G_PARAM_READABLE | G_PARAM_STATIC_STRINGS)); This property needs a documentation comment. Please include some explanation of what the GVariant format/type is! @@ +132,3 @@ + json_node_free (root); + + return priv->result != NULL; Please put brackets around this to make it a little clearer. @@ +144,3 @@ +get_entry_uri (const gchar *id) +{ + return g_strconcat (URLBASE "/", id, NULL); A comment with a link to the relevant part of the Freebase documentation would be useful here. Also, you could move the '/' into the definition of URLBASE itself. @@ +173,3 @@ + **/ +GVariant * +gdata_freebase_result_get (GDataFreeBaseResult *self) Perhaps this function could be named gdata_freebase_result_dup_variant() instead? Ending the name with '_get' seems a bit odd to me. Using 'dup' instead of 'get' also makes the memory management more obvious. @@ +181,3 @@ + priv = self->priv; + + if (!priv->result) Please use an explicit comparison against NULL: 'if (priv->result == NULL)'. ::: gdata/services/freebase/gdata-freebase-result.h @@ +68,3 @@ + GDATA_FREEBASE_RESULT_ERROR_MALFORMED_REPLY, + GDATA_FREEBASE_RESULT_ERROR_UNKNOWN_PROPERTY +} GDataFreeBaseResultError; This needs a documentation comment. @@ +75,3 @@ + +GDataFreeBaseResult *gdata_freebase_result_new (void) G_GNUC_WARN_UNUSED_RESULT G_GNUC_MALLOC; +GVariant *gdata_freebase_result_get (GDataFreeBaseResult *result); This needs G_GNUC_WARN_UNUSED_RESULT since it returns a reference. ::: gdata/services/freebase/gdata-freebase-service.c @@ +25,3 @@ + * + * #GDataFreeBaseService is a subclass of #GDataService for communicating with the Google FreeBase API. It supports queries + * in MQL format, that allows highly flexible on any topic. 'that allows highly flexible on any topic' doesn't make sense to me. @@ +83,3 @@ + * + * The developer key your application has registered with the Freebase API. For more information, see the <ulink type="http" + * url="https://developers.google.com/freebase/v1/how-tos/authorizing">online documentation</ulink>. This needs a 'Since: UNRELEASED'. @@ +149,3 @@ + * @authorizer: (allow-none): a #GDataAuthorizer to authorize the service's requests, or %NULL + * + * Creates a new #GDataFreeBaseService using the given #GDataAuthorizer. If @authorizer is %NULL, all requests are made as an unauthenticated user. You should document and assert that at least one of @developer_key and @authorizer must be non-NULL. If both are allowed to be NULL, all requests will fail (if I've read the Freebase documentation properly). @@ +151,3 @@ + * Creates a new #GDataFreeBaseService using the given #GDataAuthorizer. If @authorizer is %NULL, all requests are made as an unauthenticated user. + * + * Return value: a new #GDataFreeBaseService, or %NULL; unref with g_object_unref() The 'or %NULL' is incorrect, since NULL is only returned on assertion failure. @@ +192,3 @@ + entry = gdata_service_query_single_entry (GDATA_SERVICE (self), get_freebase_authorization_domain (), "mqlread", + GDATA_QUERY (query), GDATA_TYPE_FREEBASE_RESULT, cancellable, error); + return GDATA_FREEBASE_RESULT (entry); The GDATA_FREEBASE_RESULT() cast will fail if (entry == NULL), which it does if @error is set. @@ +207,3 @@ + * + * For more details, see gdata_freebase_service_query(), which is the synchronous version of + * this function. You should mention that gdata_service_query_single_entry_finish() should be called to get the result.
Review of attachment 272105 [details] [review]: ::: gdata/gdata.symbols @@ +1056,3 @@ +gdata_freebase_topic_result_get_type +gdata_freebase_topic_result_new +gdata_freebase_topic_result_get_object These need adding to gdata-sections.txt to get into the documentation. ::: gdata/services/freebase/gdata-freebase-service.c @@ +250,3 @@ + entry = gdata_service_query_single_entry (GDATA_SERVICE (self), get_freebase_authorization_domain (), "topic", + GDATA_QUERY (query), GDATA_TYPE_FREEBASE_TOPIC_RESULT, cancellable, error); + return GDATA_FREEBASE_TOPIC_RESULT (entry); The GDATA_FREEBASE_TOPIC_RESULT() cast will fail on error (if @entry is NULL). @@ +262,3 @@ + * + * Queries information about a topic, identified through a Freebase ID. @self and @query are all reffed when this + * function is called, so can safely be unreffed after this function returns. This should mention to use gdata_service_query_single_entry_finish() to complete the async call. ::: gdata/services/freebase/gdata-freebase-topic-query.c @@ +30,3 @@ + * online documentation</ulink>. + * + * Since: 0.15.0 'Since: UNRELEASED'. @@ +77,3 @@ + "Language in ISO-639-1 format.", + NULL, + G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS)); This needs a documentation comment. @@ +139,3 @@ +#define APPEND_SEP g_string_append_c (query_uri, (*params_started == FALSE) ? '?' : '&'); *params_started = TRUE; + + if (priv->lang) { 'if (priv->lang != NULL)' please. @@ +191,3 @@ + * embedded in the result of a gdata_freebase_service_query() call. + * + * Return value: a new #GDataFreeBaseTopicQuery This needs (transfer full). @@ +204,3 @@ + * gdata_freebase_topic_query_set_language: + * @self: a #GDataFreeBaseTopicQuery + * @lang: Language used on the topic query, in ISO-639-1 format s/Language/language/ This should have (allow-none) and mention that NULL may be used to unset the language preference. @@ +236,3 @@ + * Gets the language set on the topic query, or %NULL if unset. + * + * Return value: The language used on the query. Needs (allow-none). @@ +248,3 @@ + + priv = self->priv; + return priv->lang; You could shorten most of this to just 'return self->priv->lang;'. ::: gdata/services/freebase/gdata-freebase-topic-result.c @@ +2,3 @@ +/* + * GData Client + * Copyright (C) Peteris Krisjanis 2013 <pecisk@gmail.com> Copyright line is wrong. @@ +19,3 @@ + +/** + * SECTION:gdata-freebase-result Should this be 'SECTION:gdata-freebase-topic-result'? @@ +27,3 @@ + * + * For more details of Google FreeBase API, see the <ulink type="http" url="https://developers.google.com/freebase/v1/"> + * online documentation</ulink>. It would be useful to briefly explain what a topic query is, and what a topic is. @@ +133,3 @@ +static void +value_array_add (GDataFreeBaseTopicValueArray *array, + GDataFreeBaseTopicValue *value) Please don't line up function arguments like this: it's not part of libgdata's coding style. @@ +170,3 @@ + + if (reader_error) { + *error = g_error_copy (reader_error); You should use g_propagate_error() here in case @error is NULL. @@ +207,3 @@ + g_free (value->text); + g_free (value->lang); + g_free (value->creator); What about value->property? @@ +217,3 @@ + gchar *str; + + if (error && *error) This should be: if (error != NULL && *error != NULL). @@ +229,3 @@ + + if (error) + *error = g_error_copy (reader_error); Use g_propagate_error() here. @@ +262,3 @@ + g_value_init (value, G_TYPE_STRING); + g_value_set_string (value, json_reader_get_string_value (reader)); + break; I think you can simplify this lot by using json_node_get_value(). @@ +317,3 @@ + } + + return object != NULL; This should have brackets around it for clarity. @@ +345,3 @@ + + g_value_init (value, G_TYPE_POINTER); + g_value_set_pointer (value, object); Couldn't this be g_value_set_boxed()? @@ +359,3 @@ + value->text = reader_dup_member_string (reader, "text", error); + value->lang = reader_dup_member_string (reader, "lang", error); + value->creator = reader_dup_member_string (reader, "creator", NULL); Why doesn't this final line set the error? @@ +422,3 @@ + +static void +object_add_value (GDataFreeBaseTopicObject *object, const gchar *property, GDataFreeBaseTopicValueArray *array) You should document that this consumes @array (i.e. @array is transfer full). In fact, there are very few comments throughout this file in general. It makes the code a bit hard to follow. :-( @@ +438,3 @@ + gint i; + + members = json_reader_list_members (reader); I think it would be more efficient to use json_reader_count_elements() and iterate over that integer range using json_reader_read_element() and json_reader_get_member_name(). @@ +462,3 @@ + break; + } else if (array) { + object_add_value (object, members[i], array); You should document that ownership of @array@ is transferred into object_add_value() here. @@ +485,3 @@ + + if (strcmp (member_name, "id") == 0) { + priv->object = object_new (json_reader_get_string_value (reader)); It would be a good idea to add a 'g_assert (priv->object == NULL);' before this, as a sanity check. @@ +498,3 @@ + * gdata_freebase_topic_result_new: + * @id: (allow-none): the tasklist's ID, or %NULL + *_ Spurious underscore. @@ +503,3 @@ + * Return value: (transfer full): a new #GDataFreeBaseTopicResult; unref with g_object_unref() + * + * Since: 0.15.0 'Since: UNRELEASED' @@ +506,3 @@ + */ +GDataFreeBaseTopicResult * +gdata_freebase_topic_result_new (void) Where's the @id argument? @@ +512,3 @@ + +GDataFreeBaseTopicObject * +gdata_freebase_topic_result_get_object (GDataFreeBaseTopicResult *self) I think it would be better to call this gdata_freebase_topic_result_dup_object(), to make the memory management more obvious. @@ +520,3 @@ + +GDataFreeBaseTopicObject * +gdata_freebase_topic_object_ref (GDataFreeBaseTopicObject *object) All of these functions need documentation comments, plus introspection annotations please. :-) @@ +541,3 @@ + +GList * +gdata_freebase_topic_object_list_properties (const GDataFreeBaseTopicObject *object) I'd prefer to return a GPtrArray here, since it uses less memory than a GList and is easier to iterate over. It can also be refcounted. @@ +566,3 @@ + GDataFreeBaseTopicValueArray *array; + + g_return_val_if_fail (object != NULL, NULL); I think you need a 'g_return_val_if_fail (property != NULL, NULL);' as well. @@ +580,3 @@ + GDataFreeBaseTopicValueArray *array; + + g_return_val_if_fail (object != NULL, NULL); I think you need assertions for @property and @item too. @@ +600,3 @@ + +const gchar * +gdata_freebase_topic_value_get_property (GDataFreeBaseTopicValue *value) I think you need a non-null assertion for @value. @@ +641,3 @@ + +void +gdata_freebase_topic_value_get_value (GDataFreeBaseTopicValue *value, GValue *gvalue) Perhaps this should be called gdata_freebase_topic_value_copy_value() instead? @@ +689,3 @@ + g_return_val_if_fail (value != NULL, FALSE); + + return strcmp (value->property, "/common/topic/image") == 0; This needs brackets around it to clarify things. ::: gdata/services/freebase/gdata-freebase-topic-result.h @@ +60,3 @@ + * Since: 0.XX.X + */ + Extraneous blank line.
Review of attachment 272106 [details] [review]: ::: Makefile.am @@ +108,3 @@ + && sed "s/GDATA_TYPE_DATA_FREE_BASE/GDATA_TYPE_FREEBASE/" gdata/services/freebase/gdata-freebase-enums.h.tmp2 > gdata/services/freebase/gdata-freebase-enums.h \ + && rm -f gdata/services/picasaweb/gdata-freebase-enums.h.tmp \ + && rm -f gdata/services/picasaweb/gdata-freebase-enums.h.tmp2) There shouldn't be any mention of 'picasaweb' in here. ::: gdata/gdata.symbols @@ +1054,3 @@ +gdata_freebase_search_result_item_get_notable_name +gdata_freebase_search_result_item_get_notable_id +gdata_freebase_search_result_item_get_score This all needs adding to gdata-sections.txt. ::: gdata/services/freebase/gdata-freebase-search-query.c @@ +55,3 @@ + NODE_VALUE, + NODE_LOCATION +}; For clarity and type-safety, could you please make this a typedef, and use that typedef instead of guint in FilterNode, below? @@ +61,3 @@ + + struct { + guint type; Can this also be a typedeffed enum please? @@ +63,3 @@ + guint type; + GDataFreeBaseSearchFilterType filter_type; + GPtrArray *child_nodes; Could you mention that this contains owned FilterNodes please? @@ +67,3 @@ + + struct { + guint type; And this. @@ +73,3 @@ + + struct { + guint type; And this. @@ +82,3 @@ +struct _GDataFreeBaseSearchQueryPrivate { + FilterNode *filter; + GList *filter_stack; It would be useful to document that it's a GList of unowned FilterNodes. @@ +120,3 @@ + "Whether the search terms should be stemmed", + FALSE, + G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS)); These properties need documentation comments. Can you please give a brief explanation of what stemming is? @@ +197,3 @@ + GString *str) +{ + if (node->type == NODE_CONTAINER) { This should be a switch block, not an if block. @@ +212,3 @@ + g_string_append (str, ")"); + } else if (node->type == NODE_VALUE) { + if (strchr (node->value.property, ' ')) { Why not just unconditionally quote the property value? If not, shouldn't you be checking node->value.value whether it contains a space, not node->value.property? Shouldn't node->value.value have some escaping applied? @@ +246,3 @@ + GString *str = g_string_new (NULL); + + build_filter_string (priv->filter, str); Why not pass query_uri directly to build_filter_string()? @@ +285,3 @@ + /* Fallback to English */ + if (lang == NULL) + lang = "en"; This code looks like it's duplicated from one of the previous patches. Could the two be factored out? @@ +324,3 @@ + * gdata_freebase_search_query_add_filter() or gdata_freebase_search_query_add_location(). + * + * Return value: a new #GDataFreeBaseQuery; unref with g_object_unref() Needs a (transfer full). @@ +340,3 @@ + * @filter_type: filter type + * + * Opens a container of filter rules, those are applied according to the behavior specified by @filter_type Missing a full stop at the end of the sentence. Could you mention that all open_filter() calls need to be paired with close_filter() calls? @@ +359,3 @@ + node->container.child_nodes = g_ptr_array_new_with_free_func ((GDestroyNotify) _free_filter_node); + + if (priv->filter_stack) { if (priv->filter_stack != NULL) { @@ +389,3 @@ + + if (!priv->filter_stack) + return; I think this should be a g_assert_not_reached(), to match with open_filter(). @@ +400,3 @@ + * @value: match string + * + * Adds a property filter to the query. property filters are usually nested in s/usually/always/? Same for add_location() etc. @@ +420,3 @@ + g_warning ("A filter container must be opened before through " + "gdata_freebase_search_query_open_filter()"); + return; This could also be a g_assert_not_reached(). Same for add_location() etc. @@ +472,3 @@ + * gdata_freebase_search_query_set_language: + * @self: a #GDataFreeBaseSearchQuery + * @lang: Language used on the search terms and results, in ISO-639-1 format This needs (allow-none), and to specify that NULL means an unset language. @@ +504,3 @@ + * Gets the language set on the search query, or %NULL if unset. + * + * Return value: The language used on the query. Needs (allow-none). @@ +522,3 @@ + * gdata_freebase_search_query_set_stemmed: + * @self: a #GDataFreeBaseSearchQuery + * @stemmed: #TRUE to perform stemming on the search results s/#TRUE/%TRUE/ @@ +524,3 @@ + * @stemmed: #TRUE to perform stemming on the search results + * + * Sets whether stemming is performed on the provided search terms. Please explain what stemming is. @@ +537,3 @@ + + priv = self->priv; + stemmed = stemmed == TRUE; No need to try and normalise the boolean value like this, since TRUE is defined as !FALSE. @@ +552,3 @@ + * Returns whether the #GDataFreeBaseSearchQuery will perform stemming on the search terms. + * + * Return value: #TRUE if the #GDataFreeBaseSearchQuery performs stemming s/#TRUE/%TRUE/ ::: gdata/services/freebase/gdata-freebase-search-result.c @@ +2,3 @@ +/* + * GData Client + * Copyright (C) Peteris Krisjanis 2013 <pecisk@gmail.com> Copyright line is wrong. @@ +54,3 @@ + +struct _GDataFreeBaseSearchResultPrivate { + GPtrArray *items; Please document that this contains owned GDataFreeBaseSearchResultItems. @@ +55,3 @@ +struct _GDataFreeBaseSearchResultPrivate { + GPtrArray *items; + gint total_hits; Why isn't this unsigned? @@ +78,3 @@ +item_new (void) +{ + return g_new0 (GDataFreeBaseSearchResultItem, 1); Could you use g_slice_new0() for all the small struct allocations in this patch set? The rest of libgdata uses it, and it would be good to keep memory allocations consistent. Also, GSlice is supposed to be better for frequent, fixed-size allocations. @@ +131,3 @@ + } +#define ITEM_SET_DOUBLE(id,field) \ + if (!inner_error) { \ The (!inner_error) checks are unnecessary here, since whenever inner_error is set, the function returns immediately afterwards. @@ +195,3 @@ +#undef ITEM_SET_STRING + + return inner_error == NULL; This will always return TRUE, I think. @@ +200,3 @@ +/** + * gdata_freebase_search_result_new: + * @id: (allow-none): the tasklist's ID, or %NULL This argument doesn't exist. @@ +220,3 @@ + * Returns the number of items contained in this result. + * + * Returns: The number of items Can this ever be negative? If so, please document when and why. If not, please make the function return value unsigned. @@ +241,3 @@ + * Returns the total number of hits found for the search query. + * + * Returns: the total number of hits. Same here: why is this signed? @@ +268,3 @@ + **/ +const GDataFreeBaseSearchResultItem * +gdata_freebase_search_result_get_item (GDataFreeBaseSearchResult *self, gint i) @i should be unsigned. @@ +284,3 @@ + * @item: a #GDataFreeBaseSearchResultItem + * + * Returns the machine-encoded ID of the search result item. This is a more s/machine-encoded ID/machine-encoded ID (MID)/ @@ +285,3 @@ + * + * Returns the machine-encoded ID of the search result item. This is a more + * unique identifier, usable interchangeably with Freebase IDs. What does 'more unique' mean? More unique than what? You can't have varying degrees of uniqueness! @@ +360,3 @@ + * returns the Freebase ID of this topic. + * + * Returns: (transfer none): The topic the result item is most notable of, or %NULL. There are extraneous spaces in this line. It also needs an (allow-none). @@ +378,3 @@ + * returns the human readable name of this topic. + * + * Returns: (transfer none): The human readable topic name, or %NULL (allow-none). @@ +393,3 @@ + * @item: a #GDataFreeBaseSearchResultItem + * + * Returns the score of this search result item, the higher the better. What's a score? Some measure of relevance to the search terms? ::: gdata/services/freebase/gdata-freebase-search-result.h @@ +44,3 @@ + * All the fields in the #GDataFreeBaseSearchResult structure are private and should never be accessed directly. + * + * Since: 0.15.0 Since: UNRELEASED.
Review of attachment 272107 [details] [review]: ::: gdata/gdata.symbols @@ +1029,3 @@ gdata_freebase_service_search gdata_freebase_service_search_async +gdata_freebase_service_get_image Please add it to gdata-sections.txt too. ::: gdata/services/freebase/gdata-freebase-service.c @@ +345,3 @@ + +static gchar * +compose_image_uri (GDataFreeBaseTopicValue *value, gint max_width, gint max_height) @max_width and @max_height should be unsigned. @@ +378,3 @@ + * @cancellable: (allow-none): optional #GCancellable object, or %NULL + * @max_width: maximum width of the image returned, or -1 + * @max_height: maximum height of the image returned, or -1 I think it would make more sense to use 0 instead of -1 as the 'unspecified' value. Otherwise a value of 0 makes no sense in @max_width or @max_height. ::: gdata/services/freebase/gdata-freebase-service.h @@ +92,3 @@ +GInputStream *gdata_freebase_service_get_image (GDataFreeBaseService *self, GDataFreeBaseTopicValue *value, + GCancellable *cancellable, gint max_width, gint max_height, GError **error); This should be G_GNUC_MALLOC G_GNUC_WARN_UNUSED_RESULT.
Review of attachment 272108 [details] [review]: Looking good; only a few minor changes. Thanks a lot for this patch set! ::: Makefile.am @@ +670,3 @@ +demos_freebase_freebase_cli_LDADD = \ + $(top_builddir)/gdata/libgdata.la \ + $(GTK_LIBS) \ I don't think it needs to link against GTK+. ::: demos/freebase/freebase-cli.c @@ +52,3 @@ + + if (error) { + g_critical ("Error querying FreeBase: %s", error->message); s/FreeBase/Freebase/. @@ +80,3 @@ + } else { + const GDataFreeBaseSearchResultItem *item; + gint count, i; Unsigned please! @@ +154,3 @@ + gdata_download_stream_get_download_uri (GDATA_DOWNLOAD_STREAM (stream))); + g_object_unref (stream); + } Should you print something if (stream == NULL)? @@ +164,3 @@ + value_object = gdata_freebase_topic_value_get_object (value); + g_print (" (ID: '%s')", gdata_freebase_topic_object_get_id (value_object)); + } What about other types?
Comment on attachment 272103 [details] [review] gdata: Allow creating GDataEntries out of JSON content Attachment 272103 [details] pushed as 7b73b33 - gdata: Allow creating GDataEntries out of JSON content
Created attachment 275249 [details] [review] Add Freebase service This service is, according to the main site, a "A community-curated database of well-known people, places, and things", it allows searching for and offering information about a wide range of topics, in a well-structured and uniform manner. The most low-level API is the MQL query interface, that is a JSON-based language, queries consist of a data graph (according to their data schema) with blank places, that will be filled in in the reply.
Created attachment 275250 [details] [review] freebase: Add Topic query API With this API, structured data can be obtained about any Freebase ID, including localized text and references to images.
Created attachment 275251 [details] [review] freebase: Add search API This API enables searching for search terms, returning amongst other info the Freebase IDs usable on the topic API.
Created attachment 275252 [details] [review] freebase: Add helper function to retrieve images This helper function can be used together with the topic API, if a returned element is an image, this function can be used to retrieve a GInputStream to the image at a requested maximum size.
Created attachment 275253 [details] [review] demos: Add freebase demo This is a simple command line app that puts some of the api to work.
Hey Philip, thanks for the extensive review :), and sorry this bug went so long under my radar, I've updated the patches applying almost every suggestion, some selected replies follow: (In reply to comment #8) > Review of attachment 272104 [details] [review]: > > +/* Google FreeBase */ > > s/FreeBase/Freebase/ Done all through > > ::: gdata/gdata.symbols > @@ +1032,3 @@ > +gdata_freebase_result_get_type > +gdata_freebase_result_new > +gdata_freebase_result_get > > All these need adding to gdata-sections.txt in the documentation. 100% docs coverage now, and checked with devhelp :) > + * For more details of Google FreeBase API, see the <ulink type="http" > url="https://developers.google.com/freebase/v1/"> > + * online documentation</ulink>. > > It would be useful to briefly explain what MQL is, and to explicitly link to > the MQL documentation (https://developers.google.com/freebase/v1/mql-overview) > as well. Yeah, some details were missing... I've extended the docs in several places, and double checked the introspection bits > > @@ +29,3 @@ > + * online documentation</ulink>. > + * > + * Since: 0.XX.X > > I generally use ‘Since: UNRELEASED’ for this. Can you please change it > throughout the patchset? Thanks. Done > The preferred libgdata coding style is: > > if (priv->variant != NULL) { I've checked for code nitpicks all through the patches > + > + /* FIXME: Add limit */ > > FIXME comment here. Got that one done :) > + * @authorizer: (allow-none): a #GDataAuthorizer to authorize the service's > requests, or %NULL > + * > + * Creates a new #GDataFreeBaseService using the given #GDataAuthorizer. If > @authorizer is %NULL, all requests are made as an unauthenticated user. > > You should document and assert that at least one of @developer_key and > @authorizer must be non-NULL. If both are allowed to be NULL, all requests will > fail (if I've read the Freebase documentation properly). There's actually some key-less quota for testing, so setting both to NULL is valid for testing environments, that's what freebase-cli does btw, so you can play with that right away. (In reply to comment #9) > + > + if (reader_error) { > + *error = g_error_copy (reader_error); > > You should use g_propagate_error() here in case @error is NULL. Didn't do that in the end... json_reader_get_error() returns a const GError*, and g_propagate_error() takes GError*, supposedly g_propagate_error() won't manipulate it, but I'm usually hesitant to get rid of constness. > > @@ +207,3 @@ > + g_free (value->text); > + g_free (value->lang); > + g_free (value->creator); > > What about value->property? Duh, forgotten :) > @@ +359,3 @@ > + value->text = reader_dup_member_string (reader, "text", error); > + value->lang = reader_dup_member_string (reader, "lang", error); > + value->creator = reader_dup_member_string (reader, "creator", NULL); > > Why doesn't this final line set the error? the "creator" member is optional depending on the context, I'm not fully sure it's worth to detect when this should make parsing fail or not. > > @@ +422,3 @@ > + > +static void > +object_add_value (GDataFreeBaseTopicObject *object, const gchar *property, > GDataFreeBaseTopicValueArray *array) > > You should document that this consumes @array (i.e. @array is transfer full). > > In fact, there are very few comments throughout this file in general. It makes > the code a bit hard to follow. :-( I've added a few extra comments, hope that helped. (In reply to comment #10) > Review of attachment 272106 [details] [review]: > > ::: Makefile.am > @@ +108,3 @@ > + && sed "s/GDATA_TYPE_DATA_FREE_BASE/GDATA_TYPE_FREEBASE/" > gdata/services/freebase/gdata-freebase-enums.h.tmp2 > > gdata/services/freebase/gdata-freebase-enums.h \ > + && rm -f gdata/services/picasaweb/gdata-freebase-enums.h.tmp \ > + && rm -f gdata/services/picasaweb/gdata-freebase-enums.h.tmp2) > > There shouldn't be any mention of 'picasaweb' in here. Ugh, typo... good catch > ::: gdata/services/freebase/gdata-freebase-search-query.c > @@ +55,3 @@ > + NODE_VALUE, > + NODE_LOCATION > +}; > > For clarity and type-safety, could you please make this a typedef, and use that > typedef instead of guint in FilterNode, below? Used a typedef all through > @@ +246,3 @@ > + GString *str = g_string_new (NULL); > + > + build_filter_string (priv->filter, str); > > Why not pass query_uri directly to build_filter_string()? build_filter_string() is called recursively to handle open/close blocks, I could add another wrapper that takes query_uri, but... :) > > @@ +285,3 @@ > + /* Fallback to English */ > + if (lang == NULL) > + lang = "en"; > > This code looks like it's duplicated from one of the previous patches. Could > the two be factored out? Actually language management is somewhat different in Search and Topic apis, the former takes a list of languages, and the latter gets a single one, I've updated the patch to reflect that, although both still have set_language() (singular) API > @@ +524,3 @@ > + * @stemmed: #TRUE to perform stemming on the search results > + * > + * Sets whether stemming is performed on the provided search terms. > > Please explain what stemming is. Added a few extra explanations > Could you use g_slice_new0() for all the small struct allocations in this patch > set? The rest of libgdata uses it, and it would be good to keep memory > allocations consistent. Also, GSlice is supposed to be better for frequent, > fixed-size allocations. Did that all through > @@ +285,3 @@ > + * > + * Returns the machine-encoded ID of the search result item. This is a more > + * unique identifier, usable interchangeably with Freebase IDs. > > What does 'more unique' mean? More unique than what? You can't have varying > degrees of uniqueness! Thing is... A Freebase resource can have multiple IDs, but only one MID. I've improved the wording there. > @@ +393,3 @@ > + * @item: a #GDataFreeBaseSearchResultItem > + * > + * Returns the score of this search result item, the higher the better. > > What's a score? Some measure of relevance to the search terms? Yeah pretty much, although it's not normalized to anything nor has any upper value. I've added a better explanation there (In reply to comment #12) > @@ +154,3 @@ > + gdata_download_stream_get_download_uri > (GDATA_DOWNLOAD_STREAM (stream))); > + g_object_unref (stream); > + } > > Should you print something if (stream == NULL)? Actually that should not happen if is_image(), removed that if() > > @@ +164,3 @@ > + value_object = gdata_freebase_topic_value_get_object > (value); > + g_print (" (ID: '%s')", > gdata_freebase_topic_object_get_id (value_object)); > + } > > What about other types? Those are handled generically through the gdata_freebase_topic_value_get_text() call, that will always return a textual value for every literal or compound type, for displaying purposes that's just fine.
Review of attachment 275249 [details] [review]: Looking great! I haven’t tried compiling/running the code yet, and will do so once I have finished reviewing all the patches. If that works out OK, I think this patch could be committed directly with the minor changes below. ::: gdata/gdata.h @@ +144,3 @@ #include <gdata/services/tasks/gdata-tasks-task.h> +/* Google FreeBase */ s/FreeBase/Freebase/ (Sorry! Might as well be consistent from the start.) ::: gdata/services/freebase/gdata-freebase-query.c @@ +79,3 @@ + * + * Variant containing the MQL query. The variant is a very generic container of type "a{smv}", + * containing (possibly nested) Freebase schema types and values. What’s a Freebase schema type or a schema value? This documentation still doesn’t make it very clear how to use the API. Perhaps link to the documentation at the top of gdata-freebase-service.c? @@ +170,3 @@ + + object = json_node_get_object (copy); + limit_node = json_node_init_int (json_node_alloc (), limit); Might be clearer to use: limit_node = json_node_new (JSON_NODE_VALUE); json_node_set_int (limit_node, limit); @@ +195,3 @@ + * @mql: a MQL query string + * + * Creates a new #GDataFreebaseQuery with the MQL query provided in @mql. This doesn’t explain what MQL is. Link to the documentation at the top of gdata-freebase-service.c. @@ +215,3 @@ + * Creates a new #GDataFreebaseQuery with the MQL query provided in a serialized form as @variant + * of type "a{smv}" containing the JSON data tree of a MQL query. One convenient way + * to build the variant is using json_gvariant_serialize() from a #JsonNode This needs to document that if @variant is a floating reference, it will be sunk. ::: gdata/services/freebase/gdata-freebase-result.c @@ +178,3 @@ + * data tree. This variant can be alternatively processed through json_gvariant_serialize(). + * + * Returns: the serialized result, or %NULL. This needs an (allow-none) (transfer full), and “; unref with g_variant_unref()”.
Review of attachment 275250 [details] [review]: A couple of niggles. ::: gdata/services/freebase/gdata-freebase-topic-query.c @@ +34,3 @@ + * online documentation</ulink>. + * + * Since: 0.15.0 s/0.15.0/UNRELEASED/ @@ +154,3 @@ + } else { + const gchar * const *user_languages; + gint i; This should be a guint. @@ +227,3 @@ + + g_return_if_fail (GDATA_IS_FREEBASE_TOPIC_QUERY (self)); + g_return_if_fail (!lang || strlen (lang) == 2); Is it wise to hard-code the limit to 2 characters here? ISO-639-2 and ISO-639-3 allow for 3-character codes as well as 2-character ones. ::: gdata/services/freebase/gdata-freebase-topic-result.c @@ +55,3 @@ + TYPE_KEY, + TYPE_URI +}; It would be clearer if this was a typedeffed enum, and all the guints for it below (e.g. _GDataFreebaseTopicValueArray.type) used the enum type instead. @@ +144,3 @@ + +static void +value_array_add (GDataFreebaseTopicValueArray *array, GDataFreebaseTopicValue *value) This should have a comment specifying it’s (transfer full) for @value. Even though it’s an internal function, it’s useful documentation to have (I think). :-) @@ +366,3 @@ + value->text = reader_dup_member_string (reader, "text", error); + value->lang = reader_dup_member_string (reader, "lang", error); + value->creator = reader_dup_member_string (reader, "creator", NULL); > the "creator" member is optional depending on the context, I'm not fully sure > it's worth to detect when this should make parsing fail or not. Sure. Can you add a brief comment about this please? @@ +430,3 @@ + +static void +object_add_value (GDataFreebaseTopicObject *object, const gchar *property, GDataFreebaseTopicValueArray *array) This should document that @array is (transfer full). @@ +552,3 @@ + * Creates and returns a new reference on @object. + * + * Returns: @object, with an extra reference. This should have (transfer full). @@ +567,3 @@ +/** + * gdata_freebase_topic_object_unref: + * @object: a #GDataFreebaseTopicResult This should be (transfer full). @@ +621,3 @@ + * Returns the number of values that @object holds for the given @property. + * + * Returns: The number of values contained for @property This needs to document that -1 is a valid return value, and what it means if that’s returned. How is -1 distinct from 0? @@ +651,3 @@ + * query. + * + * Returns: the total number of hits for this property Same as above: this needs to document the -1 return value. @@ +679,3 @@ + * #GDataFreebaseTopicValue. + * + * Returns: (transfer none): the value for this property/item This needs (allow-none) and documentation for what a NULL return value means. @@ +724,3 @@ + * Creates and returns a new reference on @value. + * + * Returns: @value, with an extra reference. (transfer full) @@ +739,3 @@ +/** + * gdata_freebase_topic_value_unref: + * @value: a #GDataFreebaseTopicValue (transfer full) @@ +831,3 @@ + * Returns the time at which this value was created. + * + * Returns: The creation time of @value How is the time represented? Seconds since the UNIX epoch? @@ +862,3 @@ + * gdata_freebase_topic_value_copy_value: + * @value: a #GDataFreebaseTopicValue + * @gvalue: (transfer full): an empty #GValue This should be (out caller-allocates).
Review of attachment 275251 [details] [review]: ::: gdata/services/freebase/gdata-freebase-search-query.c @@ +238,3 @@ + } + case NODE_VALUE: + g_string_append_printf (str, " %s:\"%s\"", node->value.property, node->value.value); Escaping? ::: gdata/services/freebase/gdata-freebase-search-result.c @@ +185,3 @@ + GDATA_FREEBASE_RESULT_ERROR, + GDATA_FREEBASE_RESULT_ERROR_MALFORMED_REPLY, + "Invalid JSON reply"); You could actually use the gdata_parser_error_*() methods here, which set a GDATA_SERVICE_ERROR_PROTOCOL_ERROR. You could then remove the GDATA_FREEBASE_RESULT_ERROR_MALFORMED_REPLY error code. @@ +253,3 @@ + * Gets an item from the search result. + * + * Returns: (transfer none): a search result item, or %NULL on invalid item. This needs an (allow-none).
Review of attachment 275252 [details] [review]: ::: gdata/gdata.symbols @@ +1029,3 @@ gdata_freebase_service_search gdata_freebase_service_search_async +gdata_freebase_service_get_image Please add this to gdata-sections.txt. ::: gdata/services/freebase/gdata-freebase-service.c @@ +419,3 @@ + * @error: (allow-none): a #GError, or %NULL + * + * Creates an input stream to an image object returned in a topic query, if @max_width and @max_height s/query, if/query. If/ @@ +424,3 @@ + * Return value: (transfer full): a #GInputStream opened to the image; unref with g_object_unref() + * + * Since: 0.XX.X ‘Since: UNRELEASED’. @@ +444,3 @@ + GDATA_FREEBASE_RESULT_ERROR_UNKNOWN_PROPERTY, + "Property '%s' does not hold an image", + gdata_freebase_topic_value_get_property (value)); You could make this a GDATA_SERVICE_ERROR_PROTOCOL_ERROR and remove the Freebase-specific error code. ::: gdata/services/freebase/gdata-freebase-service.h @@ +92,3 @@ +GInputStream *gdata_freebase_service_get_image (GDataFreeBaseService *self, GDataFreeBaseTopicValue *value, + GCancellable *cancellable, gint max_width, gint max_height, GError **error); This should be G_GNUC_MALLOC G_GNUC_WARN_UNUSED_RESULT.
Review of attachment 275253 [details] [review]: ::: Makefile.am @@ +662,3 @@ +demos_freebase_freebase_cli_CFLAGS = \ + $(WARN_CFLAGS) \ + $(GTK_CFLAGS) \ $(GTK_CFLAGS) here should be unnecessary. ::: demos/freebase/freebase-cli.c @@ +130,3 @@ + for (i = 0; i < properties->len; i++) { + const gchar *property; + gint hits, count, j; Looking at the declarations for gdata_freedbase_topic_object_get_property_*(), these variables should be gint64. ::: docs/reference/gdata-sections.txt @@ +2580,3 @@ gdata_freebase_service_search gdata_freebase_service_search_async +gdata_freebase_service_get_image Ah, looks like this should have been in the previous patch. :-)
Created attachment 275830 [details] [review] Add Freebase service This service is, according to the main site, a "A community-curated database of well-known people, places, and things", it allows searching for and offering information about a wide range of topics, in a well-structured and uniform manner. The most low-level API is the MQL query interface, that is a JSON-based language, queries consist of a data graph (according to their data schema) with blank places, that will be filled in in the reply.
Created attachment 275831 [details] [review] freebase: Add Topic query API With this API, structured data can be obtained about any Freebase ID, including localized text and references to images.
Created attachment 275832 [details] [review] demos: Add freebase demo This is a simple command line app that puts some of the api to work.
Oops, just noticed comments 22-24 :), will work on that and then reply in block
Created attachment 275853 [details] [review] Add Freebase service This service is, according to the main site, a "A community-curated database of well-known people, places, and things", it allows searching for and offering information about a wide range of topics, in a well-structured and uniform manner. The most low-level API is the MQL query interface, that is a JSON-based language, queries consist of a data graph (according to their data schema) with blank places, that will be filled in in the reply.
Created attachment 275854 [details] [review] freebase: Add Topic query API With this API, structured data can be obtained about any Freebase ID, including localized text and references to images.
Created attachment 275855 [details] [review] freebase: Add search API This API enables searching for search terms, returning amongst other info the Freebase IDs usable on the topic API.
Created attachment 275856 [details] [review] freebase: Add helper function to retrieve images This helper function can be used together with the topic API, if a returned element is an image, this function can be used to retrieve a GInputStream to the image at a requested maximum size.
Created attachment 275857 [details] [review] demos: Add freebase demo This is a simple command line app that puts some of the api to work.
Thanks Philip for the review :), I've fixed most things you've spotted, a few comments follow. Also, in the last patches I added: - "bool" type parsing in GDataFreebaseTopicResult - new gdata_freebase_topic_query_[gs]et_filter() functions, with docs. Should be useful to make more concise queries and save bandwidth. - more verbose help output in freebase-cli, with a few examples - fixed warning on GDataFreebaseQuery seen through freebase-cli (In reply to comment #20) > Review of attachment 275249 [details] [review]: > ::: gdata/gdata.h > @@ +144,3 @@ > #include <gdata/services/tasks/gdata-tasks-task.h> > > +/* Google FreeBase */ > > s/FreeBase/Freebase/ > > (Sorry! Might as well be consistent from the start.) Oops, missed that one :) > > ::: gdata/services/freebase/gdata-freebase-query.c > @@ +79,3 @@ > + * > + * Variant containing the MQL query. The variant is a very generic > container of type "a{smv}", > + * containing (possibly nested) Freebase schema types and values. > > What’s a Freebase schema type or a schema value? This documentation still > doesn’t make it very clear how to use the API. Perhaps link to the > documentation at the top of gdata-freebase-service.c? I improved the documentation in the new() functions, I now realized I forgot to add a reference to those there... (In reply to comment #21) > @@ +227,3 @@ > + > + g_return_if_fail (GDATA_IS_FREEBASE_TOPIC_QUERY (self)); > + g_return_if_fail (!lang || strlen (lang) == 2); > > Is it wise to hard-code the limit to 2 characters here? ISO-639-2 and ISO-639-3 > allow for 3-character codes as well as 2-character ones. Language support is unfortunately a bit irregular: the languages supported are mostly ISO-639-1, with the exceptions of "fil" (filipino) and "zxx" ("no linguistic content"), the 3-char variants are unhandled though, so I preferred to stick to the 2-char check. the supported languages can be checked through: https://www.googleapis.com/freebase/v1/search?help=langs&indent=true > > ::: gdata/services/freebase/gdata-freebase-topic-result.c > @@ +55,3 @@ > + TYPE_KEY, > + TYPE_URI > +}; > > It would be clearer if this was a typedeffed enum, and all the guints for it > below (e.g. _GDataFreebaseTopicValueArray.type) used the enum type instead. Did that all through. > > @@ +144,3 @@ > + > +static void > +value_array_add (GDataFreebaseTopicValueArray *array, GDataFreebaseTopicValue > *value) > > This should have a comment specifying it’s (transfer full) for @value. Even > though it’s an internal function, it’s useful documentation to have (I think). > :-) Sure :), added a comment on this one and object_add_value() > @@ +552,3 @@ > + * Creates and returns a new reference on @object. > + * > + * Returns: @object, with an extra reference. > > This should have (transfer full). > > @@ +567,3 @@ > +/** > + * gdata_freebase_topic_object_unref: > + * @object: a #GDataFreebaseTopicResult > > This should be (transfer full). Fixed for both value/object > > @@ +621,3 @@ > + * Returns the number of values that @object holds for the given @property. > + * > + * Returns: The number of values contained for @property > > This needs to document that -1 is a valid return value, and what it means if > that’s returned. How is -1 distinct from 0? I've just made it use 0 uniformly as return value, and improved docs over there. > @@ +831,3 @@ > + * Returns the time at which this value was created. > + * > + * Returns: The creation time of @value > > How is the time represented? Seconds since the UNIX epoch? Yeah, that's it. Improved docs there. Also noticed parsing for this property was missing... the timestamp needed the same error treatment than the "creator" member does, I've added parsing for this and documented the error=NULL situation for both members. (In reply to comment #22) > Review of attachment 275251 [details] [review]: > > ::: gdata/services/freebase/gdata-freebase-search-query.c > @@ +238,3 @@ > + } > + case NODE_VALUE: > + g_string_append_printf (str, " %s:\"%s\"", node->value.property, > node->value.value); > > Escaping? Doh! fixed. > > ::: gdata/services/freebase/gdata-freebase-search-result.c > @@ +185,3 @@ > + GDATA_FREEBASE_RESULT_ERROR, > + GDATA_FREEBASE_RESULT_ERROR_MALFORMED_REPLY, > + "Invalid JSON reply"); > > You could actually use the gdata_parser_error_*() methods here, which set a > GDATA_SERVICE_ERROR_PROTOCOL_ERROR. You could then remove the > GDATA_FREEBASE_RESULT_ERROR_MALFORMED_REPLY error code. Oops, I now notice I've forgotten this one (In reply to comment #23) > Review of attachment 275252 [details] [review]: I happened to squash some fixes for this patch on the next, much of that was actually fixed... my bad. > @@ +444,3 @@ > + GDATA_FREEBASE_RESULT_ERROR_UNKNOWN_PROPERTY, > + "Property '%s' does not hold an image", > + gdata_freebase_topic_value_get_property (value)); > > You could make this a GDATA_SERVICE_ERROR_PROTOCOL_ERROR and remove the > Freebase-specific error code. Leftover with the above... (In reply to comment #24) > Review of attachment 275253 [details] [review]: > > ::: Makefile.am > @@ +662,3 @@ > +demos_freebase_freebase_cli_CFLAGS = \ > + $(WARN_CFLAGS) \ > + $(GTK_CFLAGS) \ > > $(GTK_CFLAGS) here should be unnecessary. Removed.
Review of attachment 275853 [details] [review]: Looks good!
Review of attachment 275854 [details] [review]: Ready to commit with the tiny changes below. ::: gdata/services/freebase/gdata-freebase-topic-result.c @@ +65,3 @@ + gchar *id; + GHashTable *values; /* Hashtable of property->GDataFreebaseTopicValueArray */ + gint ref_count; ref_count should be volatile, since you modify it with atomic operators. @@ +86,3 @@ + gint64 timestamp; + GValue value; + gint ref_count; Same here. @@ +669,3 @@ + * Since: UNRELEASED + **/ +gint64 If it’s not possible for a negative value to be returned, the return type should probably be guint64 (i.e. unsigned). @@ +701,3 @@ + * Since: UNRELEASED + **/ +gint64 Same here.
Review of attachment 275855 [details] [review]: ::: gdata/services/freebase/gdata-freebase-search-result.c @@ +185,3 @@ + GDATA_FREEBASE_RESULT_ERROR, + GDATA_FREEBASE_RESULT_ERROR_MALFORMED_REPLY, + "Invalid JSON reply"); > Oops, I now notice I've forgotten this one Do you plan on changing this?
Review of attachment 275856 [details] [review]: ::: gdata/services/freebase/gdata-freebase-service.c @@ +444,3 @@ + GDATA_FREEBASE_RESULT_ERROR_UNKNOWN_PROPERTY, + "Property '%s' does not hold an image", + gdata_freebase_topic_value_get_property (value)); As with the previous patch, I think this could be replaced by a generic libgdata error, allowing us to eliminate the custom GError domain and make things simpler.
Review of attachment 275857 [details] [review]: Looks good, thanks!
I just had a bit of a play round with freebase-cli, and everything looks good. Thanks a lot for your work on this. Please make the fixes suggested above and then merge directly to master!
Attachment 275853 [details] pushed as 7d919fd - Add Freebase service Attachment 275854 [details] pushed as f4635b2 - freebase: Add Topic query API Attachment 275855 [details] pushed as 413e3cd - freebase: Add search API Attachment 275856 [details] pushed as 901a0ae - freebase: Add helper function to retrieve images Attachment 275857 [details] pushed as 59a8370 - demos: Add freebase demo
Sorry it took this long to get to doing this last round... hope it's not too disruptive for 3.13.2. I've finally remove all traces of freebase-specific errors resorting to gdata_parser_error*, and at least using generic errors when doing g_error_set() on gdata_freebase_service_get_image(), I've also marked the error string as translatable to make it consistent with the gdata_parser_error* family, and added the file to POTFILES.in. The other latest comments have been all fixed too, including a late fix on master, sorry about that...
(In reply to comment #42) > Sorry it took this long to get to doing this last round... hope it's not too > disruptive for 3.13.2. I've finally remove all traces of freebase-specific > errors resorting to gdata_parser_error*, and at least using generic errors when > doing g_error_set() on gdata_freebase_service_get_image(), I've also marked the > error string as translatable to make it consistent with the gdata_parser_error* > family, and added the file to POTFILES.in. > > The other latest comments have been all fixed too, including a late fix on > master, sorry about that... Great, thanks a lot. I've just released version 0.15.1 with the new code.