GNOME Bugzilla – Bug 696528
lib: Turn GeocodeLocation into a proper GObject
Last modified: 2013-04-03 19:19:27 UTC
See patch
Created attachment 239715 [details] [review] lib: Turn GeocodeLocation into a proper GObject While I made this change under the wrong impression that gjs is not very happy about GeocodeLocation not being a gobject, this IMO is still a good change and I put some time into it so I'm sending this patch. I wasn't exactly sure if we want to have 'timestamp' property writable or not but since it was writable already, I left it as such. Perhaps it should be construct-only? I also took the liberty of changing all tabs to spaces in geocode-location.[ch]. Looking at geocode-ipclient.c, this seem to be the desired coding-style.
These patches go on top of patches in bug#696527. I'll rebase if needed.
(In reply to comment #1) > Created an attachment (id=239715) [details] [review] > lib: Turn GeocodeLocation into a proper GObject > > While I made this change under the wrong impression that gjs is not very > happy about GeocodeLocation not being a gobject Actually I might not be so wrong about this. When I try to access the description from my javascript app, I get this error: JS LOG: Failed to find your location: TypeError: malformed UTF-8 character sequence at offset 1 With the attached patch applied I can read the description fine with the same code.
(In reply to comment #3) > (In reply to comment #1) > > Created an attachment (id=239715) [details] [review] [details] [review] > > lib: Turn GeocodeLocation into a proper GObject > > > > While I made this change under the wrong impression that gjs is not very > > happy about GeocodeLocation not being a gobject > > Actually I might not be so wrong about this. When I try to access the > description from my javascript app, I get this error: > > JS LOG: Failed to find your location: TypeError: malformed UTF-8 character > sequence at offset 1 > > With the attached patch applied I can read the description fine with the same > code. Did you check that ->description for set to non-NULL? How did you create the object?
Review of attachment 239715 [details] [review]: ::: geocode-glib/geocode-forward.c @@ +771,3 @@ * Returns: (element-type GeocodeLocation) (transfer container): A list of * locations or %NULL in case of errors. Free the returned list with + * g_object_unref() when done. It still returns a list... ::: geocode-glib/geocode-glib.symbols @@ -27,3 @@ geocode_ipclient_search_async geocode_ipclient_search -geocode_ipclient_search_finish No newline at end of file What's the change on that line? ::: geocode-glib/geocode-ipclient.c @@ +430,3 @@ * Finishes a geolocation search operation. See geocode_ipclient_search_async(). * + * Returns: (transfer full): A #GeocodeLocation object or %NULL in case of As it was already an object, this sounds like a separate bug fix. @@ +465,3 @@ * Gets the geolocation data for an IP address from the server. * + * Returns: (transfer full): A #GeocodeLocation object or %NULL in case of Ditto. ::: geocode-glib/geocode-location.c @@ +21,3 @@ */ +#include <config.h> Separate patch for this. @@ +170,3 @@ + pspec = g_param_spec_double ("latitude", + "Latitude", + _("The latitude of this location in degrees"), No need to translate the property names. @@ +188,3 @@ + -180.0, + 180.0, + 0.0 /* default value */, we don't need that. @@ +205,3 @@ + 0, + G_MAXINT64, + 0.0 /* default value */, or that. @@ -80,3 @@ - - if (longitude < -180.0 || longitude > 180.0) { - g_warning ("Invalid longitude %lf passed, using 0.0 instead", longitude); Those warnings seem to be gone. ::: geocode-glib/geocode-location.h @@ +77,3 @@ +const char *geocode_location_get_description (GeocodeLocation *loc); + +gdouble geocode_location_get_latitude (GeocodeLocation *loc); Can you please indent all that?
Review of attachment 239715 [details] [review]: ::: geocode-glib/geocode-glib.symbols @@ -27,3 @@ geocode_ipclient_search_async geocode_ipclient_search -geocode_ipclient_search_finish No newline at end of file I actually have no clue. This change comes-up in `git diff` each time i add anything to symbols file. I have been using `git add -pu` so far but at some point I had to fail. :) ::: geocode-glib/geocode-location.c @@ +21,3 @@ */ +#include <config.h> If you prefer but this was only needed by glib/gi18n-lib just below. @@ +170,3 @@ + pspec = g_param_spec_double ("latitude", + "Latitude", + _("The latitude of this location in degrees"), Are you sure? gtk+ seems to do that. Actually it even translates nick. @@ +188,3 @@ + -180.0, + 180.0, + 0.0 /* default value */, default value or comment? @@ -80,3 @@ - - if (longitude < -180.0 || longitude > 180.0) { - g_warning ("Invalid longitude %lf passed, using 0.0 instead", longitude); * They have turned into g_return*if_fail in appropriate setter functions (that are being indirectly called by g_object_new call). IMO the correct behavior is not to change the existing value if invalid values are provided. In case of constructor, the default value will be kept: 0.0. * They were not present in the other constructor. ::: geocode-glib/geocode-location.h @@ +77,3 @@ +const char *geocode_location_get_description (GeocodeLocation *loc); + +gdouble geocode_location_get_latitude (GeocodeLocation *loc); indent how? indent all '(' to be in the same column?
(In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #1) > > > Created an attachment (id=239715) [details] [review] [details] [review] [details] [review] > > > lib: Turn GeocodeLocation into a proper GObject > > > > > > While I made this change under the wrong impression that gjs is not very > > > happy about GeocodeLocation not being a gobject > > > > Actually I might not be so wrong about this. When I try to access the > > description from my javascript app, I get this error: > > > > JS LOG: Failed to find your location: TypeError: malformed UTF-8 character > > sequence at offset 1 > > > > With the attached patch applied I can read the description fine with the same > > code. > > Did you check that ->description for set to non-NULL? How did you create the > object? From the location object returned from geocode_ipclient_search(): https://git.gnome.org/browse/gnome-maps/tree/src/mainWindow.js#n100 . I can't reproduce right now as for some reason this function doesn't return for me all of a sudden..
(In reply to comment #6) > Review of attachment 239715 [details] [review]: > > ::: geocode-glib/geocode-glib.symbols > @@ -27,3 @@ > geocode_ipclient_search_async > geocode_ipclient_search > -geocode_ipclient_search_finish > No newline at end of file > > I actually have no clue. This change comes-up in `git diff` each time i add > anything to symbols file. I have been using `git add -pu` so far but at some > point I had to fail. :) "git add -p" for it not to add newlines for you. > ::: geocode-glib/geocode-location.c > @@ +21,3 @@ > */ > > +#include <config.h> > > If you prefer but this was only needed by glib/gi18n-lib just below. I figured. > @@ +170,3 @@ > + pspec = g_param_spec_double ("latitude", > + "Latitude", > + _("The latitude of this location in > degrees"), > > Are you sure? gtk+ seems to do that. Actually it even translates nick. Yes. The documentation is only in English. > @@ +188,3 @@ > + -180.0, > + 180.0, > + 0.0 /* default value */, > > default value or comment? Remove the comment. > @@ -80,3 @@ > - > - if (longitude < -180.0 || longitude > 180.0) { > - g_warning ("Invalid longitude %lf passed, using 0.0 instead", > longitude); > > * They have turned into g_return*if_fail in appropriate setter functions (that > are being indirectly called by g_object_new call). IMO the correct behavior is > not to change the existing value if invalid values are provided. In case of > constructor, the default value will be kept: 0.0. > * They were not present in the other constructor. Fair enough. > ::: geocode-glib/geocode-location.h > @@ +77,3 @@ > +const char *geocode_location_get_description (GeocodeLocation *loc); > + > +gdouble geocode_location_get_latitude (GeocodeLocation *loc); > > indent how? indent all '(' to be in the same column? yes please.
Created attachment 239913 [details] [review] lib: Turn GeocodeLocation into a proper GObject While I made this change under the wrong impression that gjs is not very happy about GeocodeLocation not being a gobject, this IMO is still a good change and I put some time into it so I'm sending this patch. I wasn't exactly sure if we want to have 'timestamp' property writable or not but since it was writable already, I left it as such. Perhaps it should be construct-only? I also took the liberty of changing all tabs to spaces in geocode-location.[ch]. Looking at geocode-ipclient.c, this seem to be the desired coding-style.
Review of attachment 239913 [details] [review]: I think all the properties should all be construct-only, which means you could nuke the _set_*() functions. Any good reason you can think of to leave that writable after construction?
(In reply to comment #10) > Review of attachment 239913 [details] [review]: > > I think all the properties should all be construct-only, which means you could > nuke the _set_*() functions. > Any good reason you can think of to leave that writable after construction? Giving app the choice to set only essential properties and leave the rest to default values? Same could be achieved by providing multiple _new() methods though.
Created attachment 240457 [details] [review] lib: Turn GeocodeLocation into a proper GObject NM what i said in comment#11. Except for description (for which we already had a setter for and its being used by other modules), I turned all setters static so they are now merely helpers to prop setter function.
Review of attachment 240457 [details] [review]: ::: geocode-glib/geocode-location.c @@ +248,3 @@ + * <ulink url="http://en.wikipedia.org/wiki/Unix_epoch">Epoch</ulink>. + */ + pspec = g_param_spec_int64 ("timestamp", Use a uint64, I don't think we need to care about locations created before 1970. Especially as you truncate the available values! (Though I realise I'm the one who had int64 as the type in geocodelocation before. Doesn't mean it's right ;) @@ +254,3 @@ + 0, + G_MAXINT64, + 0.0, 0.0? I think you mean 0. In any case, set the default as (G_MAXUINT64 - 1), and handle it specially in geocode_location_set_timestamp() to mean "now" and get the timestamp as we used to in _new(). @@ -100,3 @@ - ret->accuracy = accuracy; - g_get_current_time (&tv); - ret->timestamp = tv.tv_sec; We're losing the timestamp in the new code. Please make it a construct property. ::: geocode-glib/geocode-location.h @@ +91,3 @@ +gdouble geocode_location_get_longitude (GeocodeLocation *loc); +gdouble geocode_location_get_accuracy (GeocodeLocation *loc); +gint64 geocode_location_get_timestamp (GeocodeLocation *loc); guint64.
Review of attachment 240457 [details] [review]: ::: geocode-glib/geocode-location.c @@ +254,3 @@ + 0, + G_MAXINT64, + 0.0, While default might be 0, since we initialize this in instance init using code will only see the value we set there. @@ +270,3 @@ + + g_get_current_time (&tv); + location->priv->timestamp = tv.tv_sec; here. @@ -100,3 @@ - ret->accuracy = accuracy; - g_get_current_time (&tv); - ret->timestamp = tv.tv_sec; no we are initializing that in instance init now.
Created attachment 240524 [details] [review] lib: Turn GeocodeLocation into a proper GObject * timestamp is now uint64 * default value of timestamp is now 0 not 0.0
Review of attachment 240524 [details] [review]: Looks good.
Attachment 240524 [details] pushed as 5372f2f - lib: Turn GeocodeLocation into a proper GObject