GNOME Bugzilla – Bug 727861
Add extension to set name of location from geo uri
Last modified: 2014-05-14 12:14:20 UTC
Android and Google Maps uses "geo:0,0?q=lat,lng(label)" to specify a description to a location. We could use the same in geocode-glib.
Created attachment 273834 [details] [review] forward: Add description extension to geo uri Android and Google Maps support an extention to geo uri that allows specifying a label to a location. The form is geo:0,0?q=lat,lng(label) This patch adds parsing of that format to GeocodeLocation.
Created attachment 273835 [details] [review] forward: Document geo URI scheme usage
Created attachment 273875 [details] [review] forward: Add description extension to geo uri Android and Google Maps support an extention to geo uri that allows specifying a label to a location. The form is geo:0,0?q=lat,lng(label) This patch adds parsing of that format to GeocodeLocation.
Review of attachment 273875 [details] [review]: If this is valgrind clean in the tests, it looks good to commit when those comments are fixed. ::: geocode-glib/geocode-location.c @@ +215,3 @@ + int max_description_len = 512; + + if (loc->priv->latitude != 0 || loc->priv->longitude != 0) { Did we really use braces for one-line conditionals? @@ +242,3 @@ + goto err; + } + description_len = MIN (token_end - next_token, max_description_len); I don't like that. What is it trying to protect? If params is already overly long, I don't think there's much of a point truncating it.
Created attachment 276146 [details] [review] forward: fix memory leaks
Created attachment 276147 [details] [review] test-geouri: fix memory leaks
Review of attachment 273875 [details] [review]: I have fixed up the commits below and made sure it is valgrind clean. While doing so I discovered memory leaks that the patches above addresses. Those might be candidates for 3.12 as well.
Any comments on the documentation patch?
Review of attachment 273835 [details] [review]: ::: geocode-glib/geocode-location.c @@ +655,3 @@ + * An extension to set a description is also supported in the form of: + * + * - geo:0,0?q=latitude,longitude(description) A link to a reference site explaining where this extension comes from would be appreciated.
Review of attachment 276146 [details] [review]: Looks good
Review of attachment 276147 [details] [review]: Looks good
Review of attachment 273835 [details] [review]: Yeah, the only reference site I can find is the android one at: http://developer.android.com/guide/components/intents-common.html#Maps You think we should include that?
Comment on attachment 273875 [details] [review] forward: Add description extension to geo uri Attachment 273875 [details] pushed as 931bd04 - forward: Add description extension to geo uri
Attachment 276146 [details] pushed as a7fb78e - forward: fix memory leaks Attachment 276147 [details] pushed as 227618c - test-geouri: fix memory leaks
Created attachment 276523 [details] [review] forward: Document geo URI scheme usage
Review of attachment 276523 [details] [review]: Looks fine to commit after that change. ::: geocode-glib/geocode-location.c @@ +658,3 @@ + * - geo:0,0?q=latitude,longitude(description) + * + * It comes from the Android world and is documented <ulink url="http://developer.android.com/guide/components/intents-common.html#Maps">here.</ulink> No need for that sentence here. Above, you'd have: An <ulink url="http://developer.android.com/guide/components/intents-common.html#Maps">Android extension</ulink> to set a description is also supported in the form of:
Attachment 276523 [details] pushed as 538e625 - forward: Document geo URI scheme usage