GNOME Bugzilla – Bug 770633
extend geo url scheme
Last modified: 2019-03-20 10:41:35 UTC
Created attachment 334514 [details] [review] initial patch set to extend geo url scheme similar patch can be found here: 737979 Adding support for unknown parameters inside RFC5870 geo url When looking onto the RFC5870, it's possible to add custom parameters. Currently geocode-glib returns an error when this happens Extend Android geo url support OSM for example returns Android defined geo urls in the format "geo:latitude,longitude?z=zoom". My patch allows parsing of those urls too, even there is no zoom property implemented yet. I also added support for multiple arguments (like "?q=...&z=...") Outstanding questions: * currently I have written the parameter splitter in a way it supports at maximum 256 arguments. (based on the old code). Probably using strchr which then alwayssearchs for the next argument would be a saner implementation * would there be concerns about adding support for zoom levels? * what about url's like "geo:0,0?q=1600+Amphitheatre+Parkway%2C+CA" as stated in 737979? I didn't implemented yet because of some concerns about returning only a description (LAT & LON would be undefined) patch can also be found on github: https://github.com/pointhi/geocode-glib/tree/set_from_uri_improvements
Hi! Thank you for the patch! Might I ask what the use-case for this is? This is not a generic geouri parser. It is a way of getting a Geocode::Location from a geouri. If the geouri-parameter does not map against a Geocode::Location property we should not add that parameter to Geocode::Location I think. That would be putting the cart before the horse. So I think making sure we can parse valid geouri is fine. But I do not want to add stuff to geocode::location (like zoom) just because it exists in a geouri. Make sense?
As use case for the zoom property: for example gnome-maps uses geocode-glib, and could then open the map with the correnct zoom level when clicking onto an url in a format like "geo:latitude,longitude?z=zoom" But I understand the concern, so I didn't extracted it (only allowed it inside the geo url). Another Idea would be adding a generic key:value structure where we can store all those geo uri parameters. But as said by you, geocode::location wasn't intended for such data.
What is required to get this patch merged?
(In reply to Thomas Pointhuber from comment #3) > What is required to get this patch merged? How is this patch generated? It seems hard to review it here in the ui. And geocode-location.c appears twice in splinter. Could you re-generate it using git? I would like to see a new patch, that focuses on not error out on unknown parameters. But do not talk about zoom at all. Adding of zoom should not take part here. And geouri should not be the driver. If there is someone who wants to add functionality that makes use of zoom in geocode-glib then we can discuss adding it.
Created attachment 337606 [details] [review] squashed patch set to allow parsing of unknow parameters allow unknown parameters inside geo url (RFC5870 and Android)
Review of attachment 337606 [details] [review]: ::: geocode-glib/geocode-location.c @@ +286,3 @@ + goto err; + } else if (g_str_has_prefix (parameters[i], "z=")) { + Why do we need to talk about zoom here? @@ +729,3 @@ * * - geo:0,0?q=latitude,longitude(description) + * - geo:latitude,longitude?z=zoom In what way is zoom "supported"?
When you look into: https://developer.android.com/guide/components/intents-common.html#Maps you would see that only 'q' and 'z' are valid arguments (from what I found out), compared to RFC5870 which allows any argument. In the current state, the parser is written in a way only to accept strings which looks valid, even a not valid one could theoretically be parsed correctly. Like "geo:13.36,42.42?u=5" which we could parse without problems, but the 'u' is not defined as valid argument for Android geo strings so it's currently returning an error. On the other hand 'z' is valid, so we have to allow it in the current implementation explicit.
Hy, what's the current state of this PR? As said before, The current PR is written in a way to only parse inputs which are written in a documented format, even when geocode-glib doesn't support those specific features yet. I need to know if that's the way how we would support those queries, or if geocode-glib should try to always return results, even there is some unspecified formatting in the input data. Regards, Thomas
Review of attachment 337606 [details] [review]: Please split off the different parts of this feature into separate patches, one patch per attachment (use git-bz if necessary to attach the patches). ::: geocode-glib/geocode-location.c @@ +248,3 @@ + * the same as for query strings (using '&' as delimiter) + */ + parameters = g_strsplit (params, "&", 256); Why is this 256? @@ +335,3 @@ int ret = TRUE; + parameters = g_strsplit (params, ";", 256); Again with 256. @@ +363,3 @@ val = u + 2; /* len of 'u=' */ loc->priv->accuracy = g_ascii_strtod (val, &endptr); + if (*endptr != '\0' && *endptr != ';') Why wouldn't we want to error out if there's a trailing ";"?
(In reply to Thomas Pointhuber from comment #8) > Hy, > > what's the current state of this PR? > > As said before, The current PR is written in a way to only parse inputs > which are written in a documented format, even when geocode-glib doesn't > support those specific features yet. > > I need to know if that's the way how we would support those queries, or if > geocode-glib should try to always return results, even there is some > unspecified formatting in the input data. As already mentioned, we're probably not interested in having a generic geouri parser in geocode-glib. Instead, you could extend the parser to not choke on URIs it does not fully understand, while making sure it can still be used for its original purpose.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/geocode-glib/issues/18.