After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 723095 - Parse Nominatim boundingbox attribute and set it as property on Place
Parse Nominatim boundingbox attribute and set it as property on Place
Status: RESOLVED FIXED
Product: geocode-glib
Classification: Other
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: geocode-glib maintainer(s)
geocode-glib maintainer(s)
Depends on:
Blocks: 722865
 
 
Reported: 2014-01-27 13:01 UTC by Jonas Danielsson
Modified: 2014-02-04 16:00 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
place: Add bounding-box property (12.36 KB, patch)
2014-01-27 13:01 UTC, Jonas Danielsson
accepted-commit_now Details | Review
forward: Parse Nominatim boundingbox attribute (2.32 KB, patch)
2014-01-27 13:01 UTC, Jonas Danielsson
accepted-commit_now Details | Review
place: Add bounding-box property (12.67 KB, patch)
2014-01-27 21:01 UTC, Jonas Danielsson
none Details | Review
forward: Parse Nominatim boundingbox attribute (2.32 KB, patch)
2014-01-27 21:01 UTC, Jonas Danielsson
none Details | Review
place: Add bounding-box property (12.70 KB, patch)
2014-01-27 22:34 UTC, Jonas Danielsson
needs-work Details | Review
forward: Parse Nominatim boundingbox attribute (4.02 KB, patch)
2014-01-27 22:34 UTC, Jonas Danielsson
none Details | Review
forward: Parse Nominatim boundingbox attribute (3.89 KB, patch)
2014-01-28 12:06 UTC, Jonas Danielsson
accepted-commit_now Details | Review
place: Add bounding-box property (13.00 KB, patch)
2014-01-29 12:08 UTC, Jonas Danielsson
committed Details | Review
forward: Parse Nominatim boundingbox attribute (5.72 KB, patch)
2014-01-29 12:08 UTC, Jonas Danielsson
needs-work Details | Review
forward: Parse Nominatim boundingbox attribute (6.58 KB, patch)
2014-02-04 07:46 UTC, Jonas Danielsson
committed Details | Review

Description Jonas Danielsson 2014-01-27 13:01:26 UTC
The boudingbox attribute from Nominatim can be useful to determine the proper zoom-level to use when working with Maps.
Comment 1 Jonas Danielsson 2014-01-27 13:01:51 UTC
Created attachment 267301 [details] [review]
place: Add bounding-box property
Comment 2 Jonas Danielsson 2014-01-27 13:01:55 UTC
Created attachment 267302 [details] [review]
forward: Parse Nominatim boundingbox attribute
Comment 3 Jonas Danielsson 2014-01-27 13:09:23 UTC
Review of attachment 267301 [details] [review]:

An alternative route would be to add the bounding-box property to GeocodeLocation instead. What are your thoughts?
Comment 4 Zeeshan Ali 2014-01-27 15:15:00 UTC
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).
Comment 5 Zeeshan Ali 2014-01-27 15:18:38 UTC
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).
Comment 6 Jonas Danielsson 2014-01-27 21:01:06 UTC
Created attachment 267342 [details] [review]
place: Add bounding-box property
Comment 7 Jonas Danielsson 2014-01-27 21:01:09 UTC
Created attachment 267343 [details] [review]
forward: Parse Nominatim boundingbox attribute
Comment 8 Jonas Danielsson 2014-01-27 22:34:40 UTC
Created attachment 267355 [details] [review]
place: Add bounding-box property
Comment 9 Jonas Danielsson 2014-01-27 22:34:43 UTC
Created attachment 267356 [details] [review]
forward: Parse Nominatim boundingbox attribute
Comment 10 Jonas Danielsson 2014-01-27 22:35:48 UTC
Boy is my face red. That approach did not work at all.

Very sorry for that, the above patches should do better.
Comment 11 Jonas Danielsson 2014-01-28 12:06:53 UTC
Created attachment 267400 [details] [review]
forward: Parse Nominatim boundingbox attribute
Comment 12 Zeeshan Ali 2014-01-28 13:21:01 UTC
(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.
Comment 13 Zeeshan Ali 2014-01-28 13:23:38 UTC
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.
Comment 14 Zeeshan Ali 2014-01-28 13:28:06 UTC
Review of attachment 267400 [details] [review]:

Would be nice to get test cases for forward and reverse.
Comment 15 Jonas Danielsson 2014-01-29 07:27:56 UTC
(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.
Comment 16 Jonas Danielsson 2014-01-29 07:28:17 UTC
(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!
Comment 17 Jonas Danielsson 2014-01-29 12:07:46 UTC
(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.
Comment 18 Jonas Danielsson 2014-01-29 12:08:30 UTC
Created attachment 267504 [details] [review]
place: Add bounding-box property
Comment 19 Jonas Danielsson 2014-01-29 12:08:33 UTC
Created attachment 267505 [details] [review]
forward: Parse Nominatim boundingbox attribute
Comment 20 Jonas Danielsson 2014-01-29 12:10:24 UTC
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
Comment 21 Zeeshan Ali 2014-01-29 14:27:28 UTC
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?
Comment 22 Zeeshan Ali 2014-01-29 14:28:57 UTC
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.
Comment 23 Zeeshan Ali 2014-01-30 16:25:30 UTC
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.
Comment 24 Jonas Danielsson 2014-02-04 07:46:07 UTC
Created attachment 268032 [details] [review]
forward: Parse Nominatim boundingbox attribute
Comment 25 Zeeshan Ali 2014-02-04 12:55:43 UTC
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.
Comment 26 Jonas Danielsson 2014-02-04 13:12:41 UTC
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.
Comment 27 Zeeshan Ali 2014-02-04 13:22:27 UTC
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