GNOME Bugzilla – Bug 697405
More detail on geocoding search results
Last modified: 2013-05-07 14:40:59 UTC
While the accuracy property of GeocodeLocation instances returned by geocode_forward_search (when using the free-form search) tell apps how accurate the geo coordinates are, apps still don't have any means to know what exactly these locations are. A location could be anything ranging from a POI (restaurant, bar, cafe etc) to a street, city, country or even continent. Apps like Maps need to know this for different things, like for example deciding zoom-level (especially in case of one result) and what kind of options to present to user when user selects the found location (foursquare or facebook check-in would be interesting for POI).
I don't think this should be done within the GeocodeLocation object. There's plenty of extra metadata that could be exported from the geocoding searches (say, timezone, nearest airport code). CoreLocation uses a separate object which contains a GeocodeLocation property within: http://developer.apple.com/library/ios/#documentation/CoreLocation/Reference/CLPlacemark_class/Reference/Reference.html#//apple_ref/doc/uid/TP40009574 We should probably do the same.
Hmm.. so this Placemark objects will be returned by geocode_forward_search() instead of GeocodeLocation objects?
Yes.
Created attachment 242485 [details] [review] lib: Add missing EOL Don't quite understand what the issue is really but this patch makes git happy w/o breaking anything.
Created attachment 242486 [details] [review] lib: Coding style fixes to geocode-forward.[ch] * Convert tabs to 8 spaces * Some lines needed indenting
Created attachment 242487 [details] [review] lib: Introducing GeocodePlace class Add a new class to represent particular places with a proper name and type. Also optionally includes all sorts of properties, like location, town, state, country etc.
Created attachment 242488 [details] [review] lib: geocode_forward_search*() returns places geocode_forward_search*() now return list of GeocodePlace objects rather than simple GeocodeLocation objects.
Created attachment 242489 [details] [review] lib: geocode_forward_search*() returns places v2: Remove an accidently added g_print statement.
(In reply to comment #8) > Created an attachment (id=242489) [details] [review] > lib: geocode_forward_search*() returns places > > v2: Remove an accidently added g_print statement. Bastien, As you can see I went against your wish to keep the intermediate step of creating a tree of locations. The reason is that I didn't quite understand your rationale since not only the new code looks a lot more simpler and readable but the test cases for location description don't break either. Actually, I added 1 new testcase for location description in this patch.
(In reply to comment #9) > Bastien, As you can see I went against your wish to keep the intermediate step > of creating a tree of locations. The tree allows to not print states when all the results are in the US, to not print countries when all the results are in the same country, etc. > The reason is that I didn't quite understand > your rationale since not only the new code looks a lot more simpler and > readable but the test cases for location description don't break either. You broke some of the tests: - if (g_strcmp0 (geocode_location_get_description (loc), "Paris, France") == 0) + g_strcmp0 (geocode_location_get_description (loc), + "Paris, Ile-de-France, France") == 0) We don't need the region, the two cities are in different countries, the region doesn't help differentiate them. > Actually, I added 1 new testcase for location description in this patch.
Review of attachment 242486 [details] [review]: Why?
Review of attachment 242485 [details] [review]: Please find out what the issue actually is, I'm not taking patches with "I don't know what this does" as the description.
Review of attachment 242487 [details] [review]: Looks alright overall. ::: geocode-glib/geocode-place.c @@ +32,3 @@ + * The #GeocodePlace instance represents a place on earth. While + * #GeocodeLocation is very generic and simply represents point on the planet, + * #GeocodePlace is more specific and can only represent specific places, e.g Why only? It represents a place, which might or might not have long/lat coords attributed. ::: geocode-glib/geocode-place.h @@ +105,3 @@ + * @GEOCODE_PLACE_TYPE_HISTORICAL_TOWN: A historical populated settlement that + * is no longer known by its original name. + * @GEOCODE_PLACE_TYPE_OCEAN: One of the five major bodies of water on the We don't have 80-char limits, please make this readable from the sources. @@ +111,3 @@ + * Type of the place. The values and their explanation is based on Yahoo places API: + * + * http://where.yahooapis.com/v1/placetypes?select=long&appid=YOUR_APP_ID Is that the best place for this? Do Nominatim's place types match? @@ +114,3 @@ + **/ +typedef enum { + GEOCODE_PLACE_TYPE_UNKNOWN = 0, Why is there a hole betwen unknown and town of 6 integers?
Review of attachment 242489 [details] [review]: As per comment in the bug.
Review of attachment 242486 [details] [review]: Isn't that the coding-style? I already did this for geocode-location.[ch] in commit 5372f2f 'lib: Turn GeocodeLocation into a proper GObject' as you ACK'ed that. If this is not right, geocode-ipclient.c and now geocode-location.[ch] needs to be changed then.
Review of attachment 242485 [details] [review]: The best I found out is that POSIX seems to require EOL at end of every line in a text file: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_205
Review of attachment 242487 [details] [review]: ::: geocode-glib/geocode-place.c @@ +32,3 @@ + * The #GeocodePlace instance represents a place on earth. While + * #GeocodeLocation is very generic and simply represents point on the planet, + * #GeocodePlace is more specific and can only represent specific places, e.g Should have updated better after changing the hierarchy (at first I this from Location class). ::: geocode-glib/geocode-place.h @@ +105,3 @@ + * @GEOCODE_PLACE_TYPE_HISTORICAL_TOWN: A historical populated settlement that + * is no longer known by its original name. + * @GEOCODE_PLACE_TYPE_OCEAN: One of the five major bodies of water on the No limits or 120-char instead? i-e should i not split at all? @@ +111,3 @@ + * Type of the place. The values and their explanation is based on Yahoo places API: + * + * http://where.yahooapis.com/v1/placetypes?select=long&appid=YOUR_APP_ID Don't know about the best place but need to be documented somewhere. Perhaps I should make it source-code-only comment?. About Nominatim, they don't match 1-1 (they have class and type within it) but looking at their classes/types, I'm sure we can map these when/if needed: Nominatim classes: https://github.com/openstreetmap/osm2pgsql/blob/master/output-gazetteer.c#L323 I was told that this is the list of types used by Nominatim but it doesn't match the types in results: http://taginfo.openstreetmap.org/keys/place#values @@ +114,3 @@ + **/ +typedef enum { + GEOCODE_PLACE_TYPE_UNKNOWN = 0, There is other holes too below but these codes are based on what Yahoo uses. My guess is that their API has some place-types that are only available in their BOSS Geo service and hence not reported with our app ID.
Review of attachment 242487 [details] [review]: ::: geocode-glib/geocode-place.h @@ +111,3 @@ + * Type of the place. The values and their explanation is based on Yahoo places API: + * + * http://where.yahooapis.com/v1/placetypes?select=long&appid=YOUR_APP_ID I talked to the Nominatim dev again and I was informed that I misunderstood. You can get types for each class using the URL: http://taginfo.openstreetmap.org/keys/${CLASS_NAME}#values Looking at the classes and some types, I think they have everything from yahoo API covered and might want to even add more types when/if we switch to Nominatim: http://taginfo.openstreetmap.org/keys/railway#values http://taginfo.openstreetmap.org/keys/boundary#values In case of bounrdy/administrative, we'll have to do some work to figure the diff between city, state, county.. and country but its not complicated.
Created attachment 243224 [details] [review] lib: Add missing EOL POSIX seems to require EOL at end of every line in a text file and thefore git doesn't like lack of EOL either: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_205 This change gets implicitly added each time you add new lines at the end of .symbols file (at least when using vim), which is annoying
Created attachment 243225 [details] [review] lib: Introducing GeocodePlace class Add a new class to represent particular places with a proper name and type. Also optionally includes all sorts of properties, like location, town, state, country etc.
Created attachment 243226 [details] [review] lib: geocode_forward_search*() returns places geocode_forward_search*() now return list of GeocodePlace objects rather than simple GeocodeLocation objects.
Review of attachment 243224 [details] [review]: Fine.
Review of attachment 243225 [details] [review]: needs-work based on the missing API docs, but looks good otherwise. ::: geocode-glib/geocode-place.c @@ +141,3 @@ + + default: + /* We don't have any other property... */ Remove that. @@ +210,3 @@ + + default: + /* We don't have any other property... */ You can remove that. @@ +227,3 @@ + +static void +geocode_place_finalize (GObject *gplace) No point in having both a dispose and a finalize function, merge them please. @@ +476,3 @@ + +GeocodePlaceType +geocode_place_get_place_type (GeocodePlace *place) API docs seems to be missing for a number of getter functions. @@ +506,3 @@ + * @place: A place + * + * Returns: (transfer none): The associated location object. Could returning a reference be useful?
Review of attachment 243226 [details] [review]: Nearly there. ::: geocode-glib/geocode-forward.c @@ +111,3 @@ longitude = g_ascii_strtod (g_hash_table_lookup (ht, "longitude"), NULL); latitude = g_ascii_strtod (g_hash_table_lookup (ht, "latitude"), NULL); + name = g_hash_table_lookup (ht, "line2"); Isn't name const char * here? @@ +541,3 @@ } else if (g_str_has_suffix (element_name, " attrs")) { + if (IS_EL("placeTypeName attrs")) { + char *code; indentation looks wrong, is this 8-chars? @@ +557,3 @@ + } else if (IS_EL("placeTypeName")) { + /* This string is also localized so we want the code rather + * than name, which we extract above. Makes it sound like you extract the name above. @@ +598,3 @@ + guint i; + + Remove one of the linefeeds here. @@ +677,3 @@ + * live with only street name for now. */ + if (place_type == GEOCODE_PLACE_TYPE_SUBURB) { + const char *street = g_hash_table_lookup (ht, "locality2"); indentation again.
Review of attachment 243225 [details] [review]: ::: geocode-glib/geocode-place.c @@ +506,3 @@ + * @place: A place + * + * Returns: (transfer none): The associated location object. Don't think so. Could actually be more inconvenient for apps to have to always unref.
Created attachment 243416 [details] [review] lib: Introducing GeocodePlace class Add a new class to represent particular places with a proper name and type. Also optionally includes all sorts of properties, like location, town, state, country etc.
Created attachment 243417 [details] [review] lib: geocode_forward_search*() returns places geocode_forward_search*() now return list of GeocodePlace objects rather than simple GeocodeLocation objects.
Review of attachment 243416 [details] [review]: Looks good.
Review of attachment 243417 [details] [review]: ::: geocode-glib/test-gcglib.c @@ +239,3 @@ + GeocodeLocation *loc; + GeocodePlace *place = l->data; + g_assert (g_strcmp0 (geocode_place_get_name (place), "Paris") == 0); Mention in the docs that using the location's description is a better idea to get a unique name for the location, and that the short name is useful for showing once a geocode search has been performed.
Attachment 243224 [details] pushed as e05c6e2 - lib: Add missing EOL Attachment 243416 [details] pushed as f87a2fe - lib: Introducing GeocodePlace class Attachment 243417 [details] pushed as a53d3b6 - lib: geocode_forward_search*() returns places
Review of attachment 243417 [details] [review]: ::: geocode-glib/test-gcglib.c @@ +239,3 @@ + GeocodeLocation *loc; + GeocodePlace *place = l->data; + g_assert (g_strcmp0 (geocode_place_get_name (place), "Paris") == 0); Done. Let me know if you want me to modify the docs.