GNOME Bugzilla – Bug 696527
lib: Report accuracy of IP-based location search result
Last modified: 2013-04-03 17:32:09 UTC
See patches
Created attachment 239713 [details] [review] lib: Use glib-mkenums to register enums with glib Based on a similar patch in libvirt-glib from Christophe Fergeau <cfergeau@redhat.com>.
Created attachment 239714 [details] [review] lib: Report accuracy of IP-based location search result
These patches go on top of patches in bug#696526. I'll rebase if needde.
Created attachment 239720 [details] [review] lib: Use glib-mkenums to register enums with glib v2: I had actually broken the build for compiled languages as geocode-enum-types.h was not getting installed. Its now fixed and I think Makefile.am becomes *slightly* cleaner with this patch.
Review of attachment 239713 [details] [review]: ::: geocode-glib/Makefile.am @@ +54,3 @@ + +geocode-enum-types.h: $(gcglib_HEADERS) geocode-enum-types.h.template + $(GLIB_MKENUMS) --template geocode-enum-types.h.template $(gcglib_HEADERS) >geocode-enum-types.h Can you please do this without using template files? See totem-pl-parser for an example. It's uglier, but it doesn't create quite as many distcheck and build problems.
Review of attachment 239720 [details] [review]: ::: geocode-glib/Makefile.am @@ +50,3 @@ + +geocode-enum-types.h: $(GCGLIB_HEADER_FILES) geocode-enum-types.h.template + $(GLIB_MKENUMS) --template geocode-enum-types.h.template $(GCGLIB_HEADER_FILES) >geocode-enum-types.h Can you please do this without using template files? See totem-pl-parser for an example. It's uglier, but it doesn't create quite as many distcheck and build problems.
Review of attachment 239714 [details] [review]: ::: geocode-glib/geocode-ipclient.c @@ +459,3 @@ * geocode_ipclient_search: * @ipclient: a #GeocodeIpclient representing a query + * @accuracy: (out): place-holder for accuracy of the returned location or NULL %NULL. ::: geocode-glib/test-geoip.c @@ +44,3 @@ GError *error = NULL; GeocodeLocation *location; + GeocodeLocationAccuracy accuracy = -1; Given that there are no negative values in the enum, this will probably get declared as an unsigned by the compiler. Add a UNSET = -1; in the enum, and you should be good (and use that here).
Created attachment 239911 [details] [review] lib: Use glib-mkenums to register enums with glib Based on a similar patch in libvirt-glib from Christophe Fergeau <cfergeau@redhat.com>.
Created attachment 239912 [details] [review] lib: Report accuracy of IP-based location search result
Attachment 239911 [details] pushed as b291a72 - lib: Use glib-mkenums to register enums with glib The other patch needs rebasing, and to use gdouble for the accuracy. Having rough symbolic to/from accuracy in meters conversion functions is probably a good idea, which is why I left the enums work in here.
Review of attachment 239912 [details] [review]: ::: geocode-glib/geocode-ipclient.h @@ +65,3 @@ +/** + * GeocodeLocationAccuracy: This should go in geocode-location.h. @@ +93,3 @@ +GeocodeLocation *geocode_ipclient_search_finish (GeocodeIpclient *ipclient, + GAsyncResult *res, + GeocodeLocationAccuracy *accuracy, And the accuracy should be a property of the GeocodeLocation.
Created attachment 240456 [details] [review] lib: Report accuracy of location result(s) Currently only accuracy of IP-based location search is reported but we should also make use of this new API to report accuracy of other location(s) yielding searches too.
Review of attachment 240456 [details] [review]: Rest looks good. ::: geocode-glib/geocode-forward.c @@ +111,3 @@ + loc = geocode_location_new_with_description (longitude, + latitude, + GEOCODE_LOCATION_ACCURACY_UNKNOWN, I'm pretty sure that we have some accuracy data for this in the response. Can you file a bug so we can update it? ::: geocode-glib/geocode-ipclient.c @@ +345,3 @@ + str = json_object_get_string_member (object, "accuracy"); + return get_accuracy_from_string (str); + } else if (json_object_has_member (object, "street")) When you start using braces, use them everywhere. ::: geocode-glib/geocode-ipclient.h @@ -79,2 @@ GeocodeLocation *geocode_ipclient_search (GeocodeIpclient *ipclient, - GError **error); You'd line up the text, not the "*". So: foo **bar; foo *bar; (hope this came out right)
Review of attachment 240456 [details] [review]: ::: geocode-glib/geocode-forward.c @@ +111,3 @@ + loc = geocode_location_new_with_description (longitude, + latitude, + GEOCODE_LOCATION_ACCURACY_UNKNOWN, Yeah, I checked that yahoo API provides a bounding box end points and we already got code that calculates distance between two locations. ::: geocode-glib/geocode-ipclient.h @@ -79,2 @@ GeocodeLocation *geocode_ipclient_search (GeocodeIpclient *ipclient, - GError **error); Yeah, i know. Just a typo.
Created attachment 240482 [details] [review] lib: Minor coding-style fixes
Created attachment 240483 [details] [review] lib: Report accuracy of location result(s) Currently only accuracy of IP-based location search is reported but we should also make use of this new API to report accuracy of other location(s) yielding searches too (bug#697174).
Review of attachment 240482 [details] [review]: Fine.
Review of attachment 240483 [details] [review]: ::: geocode-glib/geocode-location.h @@ +53,3 @@ +#define GEOCODE_LOCATION_ACCURACY_CITY 25000 /* 25 KM */ +#define GEOCODE_LOCATION_ACCURACY_REGION 50000 /* 50 KM */ +#define GEOCODE_LOCATION_ACCURACY_COUNTRY 150000 /* 150 KM */ Line them up and we're good.
Review of attachment 240483 [details] [review]: ::: geocode-glib/geocode-location.h @@ +53,3 @@ +#define GEOCODE_LOCATION_ACCURACY_CITY 25000 /* 25 KM */ +#define GEOCODE_LOCATION_ACCURACY_REGION 50000 /* 50 KM */ +#define GEOCODE_LOCATION_ACCURACY_COUNTRY 150000 /* 150 KM */ you mean 1 space between GEOCODE_LOCATION_ACCURACY_UNKNOWN (longest string) and '-1'?
(In reply to comment #19) > Review of attachment 240483 [details] [review]: > > ::: geocode-glib/geocode-location.h > @@ +53,3 @@ > +#define GEOCODE_LOCATION_ACCURACY_CITY 25000 /* 25 KM */ > +#define GEOCODE_LOCATION_ACCURACY_REGION 50000 /* 50 KM */ > +#define GEOCODE_LOCATION_ACCURACY_COUNTRY 150000 /* 150 KM */ > > you mean 1 space between GEOCODE_LOCATION_ACCURACY_UNKNOWN (longest string) and > '-1'? The KM in the comments aren't lined up. Also, it's "km".
Attachment 240482 [details] pushed as 920edcc - lib: Minor coding-style fixes Attachment 240483 [details] pushed as a1deb1b - lib: Report accuracy of location result(s)