GNOME Bugzilla – Bug 706130
Add suport for geo uri scheme
Last modified: 2013-12-16 21:26:54 UTC
Lets add support to parse and create geo: type URIs! Applications like Maps can take advantage of it. Patch forthcoming.
Created attachment 251831 [details] [review] Add support for the geo uri scheme This patch adds geocode-geouri object and functions on geocode-location to construct a location from uri. It specifies in documentation that the only supported scheme is geo. Unit tests also included. Thanks
Created attachment 251842 [details] [review] Add support for the geo uri scheme V2 Add geocode_reverse_new_for_uri.
Created attachment 253129 [details] [review] Add support for the geo uri scheme V3 Use g_ascci_dtostr in geocode_geouri_get_uri_from_location to be locale safe.
Review of attachment 253129 [details] [review]: ::: geocode-glib/geocode-geouri.c @@ +346,3 @@ + case PROP_CRS: + geocode_geouri_set_crs (geouri, + g_value_get_string (value)); indentation problem. @@ +361,3 @@ + GeocodeGeoURI *geouri = (GeocodeGeoURI *) ggeouri; + + g_clear_pointer (&geouri->priv->crs, g_free); g_free() would be enough, the pointer won't be reused after finalize and g_free() accepts NULL args @@ +457,3 @@ + g_object_class_install_property (ggeouri_class, PROP_UNCERTAINTY, pspec); + + /** Indentation. @@ +510,3 @@ + + geocode_geouri_parse_uri (geouri, uri, &error); + if (error != NULL) { Object creation should never fail, so you can either make it use the Initable interface (which can fail), or add a function that will check whether the constructed object is valid and usable (and that function would return an error). @@ +526,3 @@ + **/ +char * +geocode_geouri_get_uri_from_location (GeocodeLocation *location) This function should live in the GeocodeLocation header/object, even if implemented here. ::: geocode-glib/geocode-geouri.h @@ +69,3 @@ +char *geocode_geouri_get_uri_from_location (GeocodeLocation *location); + +GeocodeLocation *geocode_geouri_get_location (GeocodeGeoURI *geouri); This doesn't seem to be implemented? @@ +70,3 @@ + +GeocodeLocation *geocode_geouri_get_location (GeocodeGeoURI *geouri); +gdouble geocode_geouri_get_latitude (GeocodeGeoURI *geouri); Please align the function names. ::: geocode-glib/test-geouri.c @@ +52,3 @@ + guint i; + + for (i = 0; i < sizeof (uris) / sizeof(struct uri); i++) { G_N_ELEMENTS :)
Review of attachment 253129 [details] [review]: Thanks for review! I address the comments below. I have some general concerns/questions about my approach as well. Should this not be an object on its own? And instead be implemented as a part of geocode_location? So that the users of this API needn't bother with xyzURI objects. This implementation sorts of says that there can be support for other URI schemes as well, and that is why it has its own object so that geocode_location API does not grow with each new URI scheme. But it does so a bit halfheartedly. The function geocode_geouri_get_uri_from_location is problamatic in that regard, should there be a function of that type for each scheme added? So the question is. Does geocode want to support other location URI schemes? And if so should there instead be an URI interface that geouri implements instead? And we should then make the get_uri on the location take some kind of hint of which URI to give? Other issue: Geouri now has a concept of 'uncertainty' whereas location has 'accuracy'. Geouri has uncertainty because that is what the specification talks about. Should it instead use accuracy and instead of having the HAS_UNCERTAINTY property just go with GEOCODE_LOCATION_ACCURACY_UNKNOWN for when the u parameter is missing? What do you guys think? If current approach seems sane I have an updated patch in my local repository to post that address your review comments. Thanks again. ::: geocode-glib/geocode-geouri.c @@ +346,3 @@ + case PROP_CRS: + geocode_geouri_set_crs (geouri, + g_value_get_string (value)); Yes, will fix. @@ +361,3 @@ + GeocodeGeoURI *geouri = (GeocodeGeoURI *) ggeouri; + + g_clear_pointer (&geouri->priv->crs, g_free); Ok! @@ +457,3 @@ + g_object_class_install_property (ggeouri_class, PROP_UNCERTAINTY, pspec); + + /** Ok, will fix. (don't see exactly where, but will reindent with tab-width 8 and spaces) @@ +510,3 @@ + + geocode_geouri_parse_uri (geouri, uri, &error); + if (error != NULL) { Ok, will add geocode_geouri_parse to take the uri and leave the constructor to just create the object. Does this apply to geocode_location_new_for_uri as well? @@ +526,3 @@ + **/ +char * +geocode_geouri_get_uri_from_location (GeocodeLocation *location) Ok ::: geocode-glib/geocode-geouri.h @@ +69,3 @@ +char *geocode_geouri_get_uri_from_location (GeocodeLocation *location); + +GeocodeLocation *geocode_geouri_get_location (GeocodeGeoURI *geouri); No, I will remove it from header file. @@ +70,3 @@ + +GeocodeLocation *geocode_geouri_get_location (GeocodeGeoURI *geouri); +gdouble geocode_geouri_get_latitude (GeocodeGeoURI *geouri); Ok! ::: geocode-glib/test-geouri.c @@ +52,3 @@ + guint i; + + for (i = 0; i < sizeof (uris) / sizeof(struct uri); i++) { Yes! Thanks!
(In reply to comment #5) > Review of attachment 253129 [details] [review]: > > Thanks for review! > > I address the comments below. I have some general concerns/questions about my > approach as well. > > Should this not be an object on its own? And instead be implemented as a part > of geocode_location? So that the users of this API needn't bother with xyzURI > objects. > > This implementation sorts of says that there can be support for other URI > schemes as well, and that is why it has its own object > so that geocode_location API does not grow with each new URI scheme. But it > does so a bit halfheartedly. The function geocode_geouri_get_uri_from_location > is problamatic > in that regard, should there be a function of that type for each scheme added? > > So the question is. Does geocode want to support other location URI schemes? > And if so should there instead be an URI interface that geouri implements > instead? > And we should then make the get_uri on the location take some kind of hint of > which URI to give? > Oh, too long ago I wrote this patch I see. The get_uri already takes a parameter of what scheme to generate URI for. So that's good I guess. > Other issue: > Geouri now has a concept of 'uncertainty' whereas location has 'accuracy'. > Geouri has uncertainty because that is what the specification talks about. > Should it instead use accuracy and instead of having the HAS_UNCERTAINTY > property just go with GEOCODE_LOCATION_ACCURACY_UNKNOWN for when the u > parameter is missing? > > What do you guys think? If current approach seems sane I have an updated patch > in my local repository to post that address your review comments. > > Thanks again. > > ::: geocode-glib/geocode-geouri.c > @@ +346,3 @@ > + case PROP_CRS: > + geocode_geouri_set_crs (geouri, > + g_value_get_string (value)); > > Yes, will fix. > > @@ +361,3 @@ > + GeocodeGeoURI *geouri = (GeocodeGeoURI *) ggeouri; > + > + g_clear_pointer (&geouri->priv->crs, g_free); > > Ok! > > @@ +457,3 @@ > + g_object_class_install_property (ggeouri_class, PROP_UNCERTAINTY, > pspec); > + > + /** > > Ok, will fix. (don't see exactly where, but will reindent with tab-width 8 and > spaces) > > @@ +510,3 @@ > + > + geocode_geouri_parse_uri (geouri, uri, &error); > + if (error != NULL) { > > Ok, will add geocode_geouri_parse to take the uri and leave the constructor to > just create the object. > > Does this apply to geocode_location_new_for_uri as well? > > @@ +526,3 @@ > + **/ > +char * > +geocode_geouri_get_uri_from_location (GeocodeLocation *location) > > Ok > > ::: geocode-glib/geocode-geouri.h > @@ +69,3 @@ > +char *geocode_geouri_get_uri_from_location (GeocodeLocation *location); > + > +GeocodeLocation *geocode_geouri_get_location (GeocodeGeoURI *geouri); > > No, I will remove it from header file. > > @@ +70,3 @@ > + > +GeocodeLocation *geocode_geouri_get_location (GeocodeGeoURI *geouri); > +gdouble geocode_geouri_get_latitude (GeocodeGeoURI *geouri); > > Ok! > > ::: geocode-glib/test-geouri.c > @@ +52,3 @@ > + guint i; > + > + for (i = 0; i < sizeof (uris) / sizeof(struct uri); i++) { > > Yes! Thanks!
Created attachment 254209 [details] [review] Add support for the geo uri scheme V4 Updated after review. * Also fixed up geocode_reverse_new_for_uri to use geocode_location_new_for_uri
Created attachment 255720 [details] [review] Add support for geo uri scheme V5 Rebased against master, and ... * added some comments to the parsing in geo uri * renamed geocode_geouri_get_uri_from_location to _geocode_get_geo_uri_from_location and made it private
Hi, Sorry to be a bother. Is there any interest of this enhancement? Is the feature not wanted or is there a problem with the approach/implementation? I can rework it if there is interest. Or if it's not for geocode-glib then maybe it should be implemented in Maps. Thanks Jonas
(In reply to comment #9) > Hi, > > Sorry to be a bother. Is there any interest of this enhancement? Is the feature > not wanted or is there a problem with the approach/implementation? > > I can rework it if there is interest. Or if it's not for geocode-glib then > maybe it should be implemented in Maps. Shouldn't the fact that Bastien reviewed earlier versions of your patches and didn't object to the idea, be enough indication that we are interested. :) Also, I've shown interest outside this bug. Sorry for not replying earlier or reviewing your patch but I've been on vacation for a month now and still am busy with moving to another country.
Created attachment 259509 [details] [review] Add support for geo uri scheme V6 Yeah, sorry got too impatient :) Been following your move on G+ seems like it's going well. Here is a new version of the patch. Updates: * Remove the has-uncertainty property and goes with GEOCODE_LOCATION_ACCURACY_UNKNOWN for when there is no "u=" parameter. * Do not return NULL from geocode_{location|reverse}_new_for_uri, instead set the latitude/longitude to G_MAXDOUBLE. * Some small bugfixes
Review of attachment 259509 [details] [review]: Looks good otherwise to me. ::: geocode-glib/geocode-geouri.c @@ +39,3 @@ + gdouble longitude; + gdouble altitude; + gdouble uncertainty; seems very much like we are duplicating GeocodeLocation object. How about we add the CRS prop and parsing to GeocodeLocation instead? @@ +41,3 @@ + gdouble uncertainty; + + char *crs; /* Coordinate Reference System Identification */ I think we should have an enum for this.
Created attachment 259763 [details] [review] Add support for geo URI (RFC 5870) This patch adds parsing of geo URI to geocode-location It also adds functions on geocode-reverse to construct a gecode-reverse object from uri. It specifies in documentation that the only supported scheme is geo. RFC: http://www.rfc-editor.org/rfc/rfc5870.txt
Created attachment 259764 [details] [review] Add altitude property to GeocodeLocation Add altitude in meters relative to the CRS to geocode-location. Since the geo URI supports the parsing of the altitude, let's not make it go to waste.
So ok, the new version of the patch performs the parsing in the geocode-location object. And adds the CRS property and makes it an enum. Since geo uri also supports altitude I made a second patch that adds the altitude property to geocode-location. Jonas
Created attachment 263623 [details] [review] Add support for geo URI (RFC 5870) This patch adds parsing of geo URI to geocode-location It also adds functions on geocode-reverse to construct a gecode-reverse object from uri. It specifies in documentation that the only supported scheme is geo. RFC: http://www.rfc-editor.org/rfc/rfc5870.txt
Created attachment 263624 [details] [review] Add altitude property to GeocodeLocation Add altitude in meters relative to the CRS to geocode-location. Since the geo URI supports the parsing of the altitude, let's not make it go to waste.
Review of attachment 263623 [details] [review]: In the commit log: * Please end sentences with '.' and a empty line should follow after a para. * "It specifies in documentation", what is 'it' here? If you are talking about us/yourself here, I think this part is redundant. Apart from these nitpicks, I think addition of CRS should go into a separate patch. ::: geocode-glib/geocode-location.c @@ +265,3 @@ + + /** unindented ::: geocode-glib/geocode-location.h @@ +68,3 @@ + * @GEOCODE_LOCATION_CRS_WGS84: CRS is World Geodetic System, standard for Earth. + * + * Coordinate Reference System Identification for the location. 'for the location' -> 'for a location' @@ +128,3 @@ const char *description); +GeocodeLocation *geocode_location_new_for_uri (const char *uri); this is not indented like other functions here.
Review of attachment 263623 [details] [review]: ::: geocode-glib/geocode-location.c @@ +550,3 @@ + parse_geouri (loc, uri, &error); + if (error != NULL) { + g_debug ("Failed to parse geouri: %s\n", error->message); This should not be ignored like this but reported to application to deal with it as it likes. We need: * an Error param on this new fuction. * Make Location object implement GInitable and parse the uri in the 'constructed' implementation. I'm sorry this means a lot more work for you but _new functions in GObject world are supposed to be only thin wrappers around g_object_new() and GInitable is specifically meant for failable constructions.
Review of attachment 263624 [details] [review]: Ideally this would come before geo URI parsing so that geo URI parsing patch but its fine this way too.
(In reply to comment #19) > Review of attachment 263623 [details] [review]: > > ::: geocode-glib/geocode-location.c > @@ +550,3 @@ > + parse_geouri (loc, uri, &error); > + if (error != NULL) { > + g_debug ("Failed to parse geouri: %s\n", error->message); > > This should not be ignored like this but reported to application to deal with > it as it likes. We need: > > * an Error param on this new fuction. > * Make Location object implement GInitable and parse the uri in the > 'constructed' implementation. > > I'm sorry this means a lot more work for you but _new functions in GObject > world are supposed to be only thin wrappers around g_object_new() and GInitable > is specifically meant for failable constructions. No problem. There is no rush for this patch set, Maps URI handling is nice to have but not showstopper :) And I get to learn about GInitable. Thanks for the review.
(In reply to comment #20) > Review of attachment 263624 [details] [review]: > > Ideally this would come before geo URI parsing so that geo URI parsing patch > but its fine this way too. I'll re-spin with adding altitude and adding CRS in separate patches.
(In reply to comment #18) > Review of attachment 263623 [details] [review]: > > In the commit log: > > * Please end sentences with '.' and a empty line should follow after a para. > * "It specifies in documentation", what is 'it' here? If you are talking about > us/yourself here, I think this part is redundant. > > Apart from these nitpicks, I think addition of CRS should go into a separate > patch. > > ::: geocode-glib/geocode-location.c > @@ +265,3 @@ > > + > + /** > > unindented > > ::: geocode-glib/geocode-location.h > @@ +68,3 @@ > + * @GEOCODE_LOCATION_CRS_WGS84: CRS is World Geodetic System, standard for > Earth. > + * > + * Coordinate Reference System Identification for the location. > > 'for the location' -> 'for a location' > > @@ +128,3 @@ > const char > *description); > > +GeocodeLocation *geocode_location_new_for_uri (const char *uri); > > this is not indented like other functions here. Thanks for review! Will fix these!
Created attachment 263978 [details] [review] location: Add altitude property Add altitude in meters to geocode-location.
Created attachment 263979 [details] [review] location: Add CRS property Add a property defining the coordinate reference system of a geocode-location.
Created attachment 263980 [details] [review] location: Add support for parsing geo URI This patch adds parsing of geo URI to geocode-location. The parsing of a URI can fail and because of that this patch makes geocode-location implement the GInitable interface. The RFC for the geo URI scheme can be found at: http://www.rfc-editor.org/rfc/rfc5870.txt
Review of attachment 263978 [details] [review]: ack
Review of attachment 263979 [details] [review]: Other than that, its good. ::: geocode-glib/geocode-location.h @@ +71,3 @@ + */ +typedef enum { + GEOCODE_LOCATION_CRS_UNKNOWN = 0, Do we really need this? location is pretty unusable to apps if CRS is unknown.
(In reply to comment #27) > Review of attachment 263978 [details] [review]: > > ack I just realised that altitude has no public setter in this patch, and the only way to specify one would be with the geo uri in the commit after. Maybe I should respin and add a set_altitude method?
(In reply to comment #28) > Review of attachment 263979 [details] [review]: > > Other than that, its good. > > ::: geocode-glib/geocode-location.h > @@ +71,3 @@ > + */ > +typedef enum { > + GEOCODE_LOCATION_CRS_UNKNOWN = 0, > > Do we really need this? location is pretty unusable to apps if CRS is unknown. Agreed, it was really mostly something to return on g_return_val_if_fail in the getter method. I can remove it.
Review of attachment 263980 [details] [review]: ::: geocode-glib/geocode-location.c @@ +444,3 @@ + GeocodeLocation *location = (GeocodeLocation *) glocation; + + if (location->priv->construct_uri != NULL) { AFAIK we are supposed to do error-prone initialization in initable_init below. @@ +648,3 @@ + location = (GeocodeLocation *) initable; + + if (cancellable != NULL) { This doesn't need special handling. Cancellable dont' guarantee cancellation. ::: geocode-glib/geocode-location.h @@ +71,3 @@ + */ +typedef enum { + GEOCODE_LOCATION_URI_SCHEME_UNKNOWN = 0, Same thing here: Do we want to entertain unknown schemes? We can either assume 'geo' or error out on unknown scheme.
(In reply to comment #29) > (In reply to comment #27) > > Review of attachment 263978 [details] [review] [details]: > > > > ack > > I just realised that altitude has no public setter in this patch, and the only > way to specify one would be with the geo uri in the commit after. > > Maybe I should respin and add a set_altitude method? Well the other props don't have a setter so you shouldn't need one for this either. If you really do, we should add setters for all props in a patch before the one requiring it.
(In reply to comment #32) > (In reply to comment #29) > > (In reply to comment #27) > > > Review of attachment 263978 [details] [review] [details] [details]: > > > > > > ack > > > > I just realised that altitude has no public setter in this patch, and the only > > way to specify one would be with the geo uri in the commit after. > > > > Maybe I should respin and add a set_altitude method? > > Well the other props don't have a setter so you shouldn't need one for this > either. If you really do, we should add setters for all props in a patch before > the one requiring it. I guess its ok.
Review of attachment 263980 [details] [review]: ::: geocode-glib/geocode-location.c @@ +444,3 @@ + GeocodeLocation *location = (GeocodeLocation *) glocation; + + if (location->priv->construct_uri != NULL) { I followed the pattern that gsocket uses. It creates the socket in constructed and set a constructed_error if it fails. That error then gets checked in initable_init. I could change it and do all of it in initable_init as well. Then we could also do away with construct_error and use the error from the method argument directly. @@ +648,3 @@ + location = (GeocodeLocation *) initable; + + if (cancellable != NULL) { Same here, followed gsockets example. It just checks that if someone has set cancellable on the g_initable_new construct or using g_initable_init, we bail out, because we do not support cancellable. ::: geocode-glib/geocode-location.h @@ +71,3 @@ + */ +typedef enum { + GEOCODE_LOCATION_URI_SCHEME_UNKNOWN = 0, Agreed.
Created attachment 264011 [details] [review] location: Add altitude property Add altitude in meters to geocode-location.
Created attachment 264012 [details] [review] location: Add CRS property Add a property defining the coordinate reference system of a geocode-location.
Created attachment 264013 [details] [review] location: Add support for parsing geo URI This patch adds parsing of geo URI to geocode-location. The parsing of a URI can fail and because of that this patch makes geocode-location implement the GInitable interface. The RFC for the geo URI scheme can be found at: http://www.rfc-editor.org/rfc/rfc5870.txt
Created attachment 264014 [details] [review] location: Add support for parsing geo URI This patch adds parsing of geo URI to geocode-location. The parsing of a URI can fail and because of that this patch makes geocode-location implement the GInitable interface. The RFC for the geo URI scheme can be found at: http://www.rfc-editor.org/rfc/rfc5870.txt
Review of attachment 264011 [details] [review]: ack
Review of attachment 264012 [details] [review]: ack
Review of attachment 264014 [details] [review]: I'd divide this patches into 2 as I see 2 diff changes: * Implement Initable * URI parsing ::: geocode-glib/geocode-location.c @@ +43,3 @@ + * check the results before using the object. This is done automatically in + * geocode_location_new(), geocode_location_new_with_description() and + * geocode_location_new_for_uri(). You want to make functions a link by prepending '#' in front of their names here. Also I'll rephrase: "This is done automatically ..()." as: "Typically you will want to use ... functions that do all that for you. @@ +931,3 @@ + +/** + * geocode_location_get_uri: The name suggests a normal getter of the property so app developers will likely expect the same URI as given at construction time. We are generating the URI on demand depending on the scheme so better name it 'geocode_location_to_uri' to make it clear its not as simple getter. @@ +933,3 @@ + * geocode_location_get_uri: + * @loc: a #GeocodeLocation + * @scheme: the scheme of the requested URI This argument was removed but if we are changing this function from a getter to a URI generator, I think we should re-introduce that argument and remove the urischeme prop all together. Also then uri becomes construct-only. @@ +938,3 @@ + * #GeocodeLocation:uri-scheme property. + * + * Currently supported schemes: geo I don't think we need to specify that here. @@ +948,3 @@ + g_return_val_if_fail (GEOCODE_IS_LOCATION (loc), NULL); + + if (loc->priv->scheme == GEOCODE_LOCATION_URI_SCHEME_GEO) This condition is redundant. We need to ensure that uri scheme is never set to invalid values.
(In reply to comment #41) > Review of attachment 264014 [details] [review]: > > I'd divide this patches into 2 as I see 2 diff changes: > > * Implement Initable > * URI parsing Ok. > > ::: geocode-glib/geocode-location.c > @@ +43,3 @@ > + * check the results before using the object. This is done automatically in > + * geocode_location_new(), geocode_location_new_with_description() and > + * geocode_location_new_for_uri(). > > You want to make functions a link by prepending '#' in front of their names > here. Also I'll rephrase: > Hmm, I tried that and checked the docs, that just added a '#' in front the links to the functions, so I do not think it is needed. > "This is done automatically ..()." > > as: > > "Typically you will want to use ... functions that do all that for you. > Ok. > @@ +931,3 @@ > + > +/** > + * geocode_location_get_uri: > > The name suggests a normal getter of the property so app developers will likely > expect the same URI as given at construction time. We are generating the URI on > demand depending on the scheme so better name it 'geocode_location_to_uri' to > make it clear its not as simple getter. > Ok! > @@ +933,3 @@ > + * geocode_location_get_uri: > + * @loc: a #GeocodeLocation > + * @scheme: the scheme of the requested URI > > This argument was removed but if we are changing this function from a getter to > a URI generator, I think we should re-introduce that argument and remove the > urischeme prop all together. Also then uri becomes construct-only. > Sure. Should it still be READWRITE you think? Or just WRITABLE and CONSTRUCT_ONLY? > @@ +938,3 @@ > + * #GeocodeLocation:uri-scheme property. > + * > + * Currently supported schemes: geo > > I don't think we need to specify that here. > Ok, should it be spcified in new_for_uri constructor you think? > @@ +948,3 @@ > + g_return_val_if_fail (GEOCODE_IS_LOCATION (loc), NULL); > + > + if (loc->priv->scheme == GEOCODE_LOCATION_URI_SCHEME_GEO) > > This condition is redundant. We need to ensure that uri scheme is never set to > invalid values. But when switching to have a scheme argument it's ok, right? Thanks for review
(In reply to comment #42) > (In reply to comment #41) > > Review of attachment 264014 [details] [review] [details]: > > ::: geocode-glib/geocode-location.c > > @@ +43,3 @@ > > + * check the results before using the object. This is done automatically in > > + * geocode_location_new(), geocode_location_new_with_description() and > > + * geocode_location_new_for_uri(). > > > > You want to make functions a link by prepending '#' in front of their names > > here. Also I'll rephrase: > > > > Hmm, I tried that and checked the docs, that just added a '#' in front the > links to the functions, so I do not think it is needed. Then something is wrong, perhaps those functions didn't get documented or something? Check warnings/errors from doc build. > > @@ +933,3 @@ > > + * geocode_location_get_uri: > > + * @loc: a #GeocodeLocation > > + * @scheme: the scheme of the requested URI > > > > This argument was removed but if we are changing this function from a getter to > > a URI generator, I think we should re-introduce that argument and remove the > > urischeme prop all together. Also then uri becomes construct-only. > > > > Sure. Should it still be READWRITE you think? Or just WRITABLE and > CONSTRUCT_ONLY? Just WRITABLE as apps won't be able to read it anymore. > > @@ +938,3 @@ > > + * #GeocodeLocation:uri-scheme property. > > + * > > + * Currently supported schemes: geo > > > > I don't think we need to specify that here. > > > > Ok, should it be spcified in new_for_uri constructor you think? No, as long as the docs to 'scheme' parameter(s) link to the appropriate enum type, its obvious to app developer which schemes are supported. If you still think its needed, feel free to add this to the enum itself. > > @@ +948,3 @@ > > + g_return_val_if_fail (GEOCODE_IS_LOCATION (loc), NULL); > > + > > + if (loc->priv->scheme == GEOCODE_LOCATION_URI_SCHEME_GEO) > > > > This condition is redundant. We need to ensure that uri scheme is never set to > > invalid values. > > But when switching to have a scheme argument it's ok, right? Good point but we want to use a g_return_val_if_fail() instead. > Thanks for review NP, hope you don't get disheartened with repeated 'needs-work', we are almost there.
Created attachment 264247 [details] [review] location: Convert to GInitable In order to have constructors that can fail GeocodeLocation needs to implement the GInitable interface.
Created attachment 264248 [details] [review] location: Add parsing of geo URI This patch adds parsing of geo URI to geocode-location. The parsing of a URI can fail and because of that this patch makes geocode-location implement the GInitable interface. The RFC for the geo URI scheme can be found at: http://www.rfc-editor.org/rfc/rfc5870.txt
I don't think GInitable is a good idea. A GeocodeLocation object is pretty core to the way geocode-glib works, and such a GObject should never fail being created. The cases in which we could fail are very few compared to the case where it wouldn't. I would instead keep the current g_object_new(), and add a set_from_geouri() which you could fail, similarly to how GtkBuilder works. builder = gtk_builder_new (); if (gtk_builder_add_from_file (builder, "filename.ui", &error) < 0) etc.
(In reply to comment #46) > I don't think GInitable is a good idea. A GeocodeLocation object is pretty core > to the way geocode-glib works, and such a GObject should never fail being > created. The cases in which we could fail are very few compared to the case > where it wouldn't. > > I would instead keep the current g_object_new(), and add a set_from_geouri() > which you could fail, similarly to how GtkBuilder works. > > builder = gtk_builder_new (); > if (gtk_builder_add_from_file (builder, "filename.ui", &error) < 0) > etc. Yeah, I think I agree with this. Do you mean keep the current constructors, or do you mean add a geocode_location_new (), that does not take any arguments? Thanks!
Created attachment 264268 [details] [review] location: Add parsing of geo URI This patch adds parsing of geo URI to geocode-location. The RFC for the geo URI scheme can be found at: http://www.rfc-editor.org/rfc/rfc5870.txt
Review of attachment 264268 [details] [review]: Wa? Why did you ditch all the work on _new_from_uri?
(In reply to comment #49) > Review of attachment 264268 [details] [review]: > > Wa? Why did you ditch all the work on _new_from_uri? Never mind, just saw the comment from Bastien.
Review of attachment 264268 [details] [review]: Looks good. I guess not everyone cares about making life for app developers as easy as possible so ACK then.
(In reply to comment #51) > Review of attachment 264268 [details] [review]: > > Looks good. I guess not everyone cares about making life for app developers as > easy as possible so ACK then. One function vs. 2 function calls. And in most cases, for app developers it means that they can *never* have a GeocodeLocation instantiation not working, as opposed to depending on what mucking the library might be doing behind the developer's back. At least it's clear now.
(In reply to comment #52) > (In reply to comment #51) > > Review of attachment 264268 [details] [review] [details]: > > > > Looks good. I guess not everyone cares about making life for app developers as > > easy as possible so ACK then. > > One function vs. 2 function calls. If one function could do, why make them call two? No biggie still though, hence the reason I ack'ed the patch. > And in most cases, for app developers it > means that they can *never* have a GeocodeLocation instantiation not working, > as opposed to depending on what mucking the library might be doing behind the > developer's back. At least it's clear now. If app would want a location object thats set from a URI, they really don't need an instance that we failed to set from the URI. We don't take error argument for existing _new() so they can always instantiate w/o errors if thats what they want.
(In reply to comment #53) > (In reply to comment #52) > > (In reply to comment #51) > > > Review of attachment 264268 [details] [review] [details] [details]: > > > > > > Looks good. I guess not everyone cares about making life for app developers as > > > easy as possible so ACK then. > > > > One function vs. 2 function calls. > > If one function could do, why make them call two? No biggie still though, hence > the reason I ack'ed the patch. It doesn't change much when you're parsing a geouri as here, but you need to add error checking to each and every call that might be creating a new GeocodeLocation. > > And in most cases, for app developers it > > means that they can *never* have a GeocodeLocation instantiation not working, > > as opposed to depending on what mucking the library might be doing behind the > > developer's back. At least it's clear now. > > If app would want a location object thats set from a URI, they really don't > need an instance that we failed to set from the URI. We don't take error > argument for existing _new() so they can always instantiate w/o errors if thats > what they want. That means g_object_new() could fail as well.
Attachment 264011 [details] pushed as fdb74ad - location: Add altitude property Attachment 264012 [details] pushed as 525c356 - location: Add CRS property Attachment 264268 [details] pushed as df05549 - location: Add parsing of geo URI