Created attachment 287301 [details] [review]
freebase: Fix typo in search query

The string array matches the GDataFreebaseSearchFilterType, using the
FilterNodeType was unintended, and TYPE_CONTAINER happened to match
GDATA_FREEBASE_SEARCH_FILTER_ALL.

Created attachment 287302 [details] [review]
freebase: Translate meters to feet on geolocated queries

Since support was added, the service started giving "backend errors" when
performing "within radius" queries in meters, although queries in feet do
work. Start using this, and translate the value given in meters.

Created attachment 287303 [details] [review]
freebase: Fix locale-dependent issues in geolocated queries

Latitude/longitude are really expected with a '.' decimal point by the
service, breaking on locales that would use anything different. Use
g_ascii_formatd which always uses the '.' decimal point.

Review of attachment 287301 [details] [review]:

Looks good, but unit tests are needed! Something like: https://git.gnome.org/browse/libgdata/tree/gdata/tests/tasks.c#n81

Review of attachment 287302 [details] [review]:

::: gdata/services/freebase/gdata-freebase-search-query.c
@@ +47,3 @@
 
+#define FEET_PER_METER 3.2808
+#define METERS_TO_FEET(m) ((guint64) ((gdouble) (m) * FEET_PER_METER))

This will truncate rather than rounding. We should probably use round() here.

@@ +252,3 @@
+		g_string_append_printf (str, "(within radius:%" G_GUINT64_FORMAT "ft lon:%.4f lat:%.4f)",
+					METERS_TO_FEET(node->location.radius),
+					node->location.lon, node->location.lat);

Have you filed a bug with Google about this? Not supporting (or rather, breaking support for) metres is ridiculous, and is hopefully a simple and fixable bug at their end.

Please file a bug with them and link to it in a FIXME comment here. Then we can revert to using metres when they fix their servers.

Review of attachment 287303 [details] [review]:

Whoops, good catch.

(In reply to comment #5)
> Review of attachment 287302 [details] [review]:
> @@ +252,3 @@
> +        g_string_append_printf (str, "(within radius:%" G_GUINT64_FORMAT "ft
> lon:%.4f lat:%.4f)",
> +                    METERS_TO_FEET(node->location.radius),
> +                    node->location.lon, node->location.lat);
> 
> Have you filed a bug with Google about this? Not supporting (or rather,
> breaking support for) metres is ridiculous, and is hopefully a simple and
> fixable bug at their end.

Apparently Freebase is keeping bugs in their own Jira... that'd be https://bugs.freebase.com/browse/API-110

> 
> Please file a bug with them and link to it in a FIXME comment here. Then we can
> revert to using metres when they fix their servers.

Will get to this and the unit tests later today :)

Created attachment 287769 [details] [review]
tests: Add tests for Freebase search API

(In reply to comment #7)
> 
> Apparently Freebase is keeping bugs in their own Jira... that'd be
> https://bugs.freebase.com/browse/API-110

They were extremely responsive and fixed this bug, I'm marking that patch as obsolete.

FWIW, the patch to add tests for the search API applies on top of the tests' one in bug #737539

Review of attachment 287769 [details] [review]:

Looks good to me apart from that one niggle.

::: gdata/tests/freebase.c
@@ +72,3 @@
+	result = gdata_freebase_service_search (GDATA_FREEBASE_SERVICE (service), query, NULL, &error);
+	g_assert_no_error (error);
+	g_assert_nonnull (result);

Same comment as in the other bugs: I don’t really want to bump libgdata’s GLib dependency for this macro. Can you use g_assert() instead please?

(In reply to comment #9)
> (In reply to comment #7)
> > 
> > Apparently Freebase is keeping bugs in their own Jira... that'd be
> > https://bugs.freebase.com/browse/API-110
> 
> They were extremely responsive and fixed this bug, I'm marking that patch as
> obsolete.

Also: go them, woo! \o/

The tests were fixed and everything pushed :)

Attachment 287301 [details] pushed as 43dd188 - freebase: Fix typo in search query
Attachment 287303 [details] pushed as 021370e - freebase: Fix locale-dependent issues in geolocated queries
Attachment 287769 [details] pushed as 8400bc6 - tests: Add tests for Freebase search API