GNOME Bugzilla – Bug 723095
Parse Nominatim boundingbox attribute and set it as property on Place
Last modified: 2014-02-04 16:00:01 UTC
The boudingbox attribute from Nominatim can be useful to determine the proper zoom-level to use when working with Maps.
Created attachment 267301 [details] [review] place: Add bounding-box property
Created attachment 267302 [details] [review] forward: Parse Nominatim boundingbox attribute
Review of attachment 267301 [details] [review]: An alternative route would be to add the bounding-box property to GeocodeLocation instead. What are your thoughts?
Review of attachment 267301 [details] [review]: It belongs here IMO. A place can have dimensions but location is supposed to represent only a particular point (although it has an accuracy circle associated to it but thats still different and actually for geoclue purposes).
Review of attachment 267302 [details] [review]: ack. Don't you need pass a param to Nominatim to get the bbox or do we already pass that? ::: geocode-glib/geocode-forward.c @@ +768,3 @@ + + /* The Nominatim bounding box is formated as (bottom, top, left, right) + * and GeocodeBoundingBox as (top, bottom, left, right). */ Extra points for adding this comment but I think the second line is redundant. If you want to keep it, you want to change: "GeocodeBoundingBox as" to "geocode_bounding_box_new expects (top, bottom, left, right).
Created attachment 267342 [details] [review] place: Add bounding-box property
Created attachment 267343 [details] [review] forward: Parse Nominatim boundingbox attribute
Created attachment 267355 [details] [review] place: Add bounding-box property
Created attachment 267356 [details] [review] forward: Parse Nominatim boundingbox attribute
Boy is my face red. That approach did not work at all. Very sorry for that, the above patches should do better.
Created attachment 267400 [details] [review] forward: Parse Nominatim boundingbox attribute
(In reply to comment #10) > Boy is my face red. That approach did not work at all. > > Very sorry for that, the above patches should do better. When submitting new version of the same patches for review, its helpful for reviewers if you tell abit what is the difference.
Review of attachment 267355 [details] [review]: ::: geocode-glib/geocode-place.c @@ +521,3 @@ + pspec = g_param_spec_object ("bounding-box", + "Bounding Box", + "A bounding box for the place", I'll put this string in the docs above too. @@ +1142,3 @@ +{ + g_return_val_if_fail (GEOCODE_IS_PLACE (place), NULL); + g_return_val_if_fail (place->priv->bbox != NULL, NULL); This should be done in the setter and it should be normal for this function to return NULL if boundaries of the place are unknown.
Review of attachment 267400 [details] [review]: Would be nice to get test cases for forward and reverse.
(In reply to comment #12) > (In reply to comment #10) > > Boy is my face red. That approach did not work at all. > > > > Very sorry for that, the above patches should do better. > > When submitting new version of the same patches for review, its helpful for > reviewers if you tell abit what is the difference. yeah, sorry! I messed up how the bounding box was represented in JSON, asumed it was a string but it was an Array. So it needs special treatment in the "from JSON to hash_table"-code.
(In reply to comment #14) > Review of attachment 267400 [details] [review]: > > Would be nice to get test cases for forward and reverse. Agreed, will add! Thx for review!
(In reply to comment #14) > Review of attachment 267400 [details] [review]: > > Would be nice to get test cases for forward and reverse. (In reply to comment #16) > (In reply to comment #14) > > Review of attachment 267400 [details] [review] [details]: > > > > Would be nice to get test cases for forward and reverse. > > Agreed, will add! > > Thx for review! (In reply to comment #16) > (In reply to comment #14) > > Review of attachment 267400 [details] [review] [details]: > > > > Would be nice to get test cases for forward and reverse. > > Agreed, will add! > > Thx for review! Although, Nominatim reverse reply does not seem to include a bounding-box, so we are out of luck there.
Created attachment 267504 [details] [review] place: Add bounding-box property
Created attachment 267505 [details] [review] forward: Parse Nominatim boundingbox attribute
New in this revision: * Treat bounding-box same as location and use g_value_dup_object when setting, as well as g_clear_object in dispose. * Add g_object_unref on bounding box after setting it. * Add test of bbox in test-gcglib
Review of attachment 267505 [details] [review]: Almost there :) ::: geocode-glib/geocode-reverse.c @@ +169,3 @@ + json_reader_end_element (reader); + } + g_hash_table_insert (ht, g_strdup (members[i]), coords); the type of keys and values in a g_hashtable is supposed to be the same. Its any ugly hack otherwise. Can't we add these coordinates as strings with separate keys?
Review of attachment 267504 [details] [review]: ack ::: geocode-glib/geocode-place.c @@ +1136,3 @@ + * Gets the bounding box for the @place. + * + * Returns: A #GeocodeBoundingBox for the @place. more points for adding ", or NULL if boundaries are unknown.
Review of attachment 267505 [details] [review]: ::: geocode-glib/geocode-reverse.c @@ +156,3 @@ } + } else if (g_strcmp0 (members[i], "boundingbox") == 0) { + GeocodeBoundingBox *bbox; This line is redundant and introduces 'unused variable' warning at compile time.
Created attachment 268032 [details] [review] forward: Parse Nominatim boundingbox attribute
Review of attachment 268032 [details] [review]: almost there :) ::: geocode-glib/geocode-reverse.c @@ +135,3 @@ + if (json_reader_is_value (reader)) { + value = json_reader_get_string_value (reader); + if (value && *value == '\0') does json_reader_is_value() returning true means that value can't be NULL and hence we don't need to check it again here? I'd prefer this change go in a sperate patch with its justification in the log.
Review of attachment 268032 [details] [review]: ::: geocode-glib/geocode-reverse.c @@ +135,3 @@ + if (json_reader_is_value (reader)) { + value = json_reader_get_string_value (reader); + if (value && *value == '\0') No it does not. It checks if the reader is currently on a 'value' as opposed to an array or an object. It needs to be there because when it gets to the array 'boundingbox' it must not read a string value, that will corrupt it all.
Review of attachment 268032 [details] [review]: ack then ::: geocode-glib/geocode-reverse.c @@ +135,3 @@ + if (json_reader_is_value (reader)) { + value = json_reader_get_string_value (reader); + if (value && *value == '\0') ah ok