GNOME Bugzilla – Bug 754797
location: Make timestamp property writable on construction
Last modified: 2015-09-10 13:11:36 UTC
Bastien, Although I don't think of this as an API/ABI break since it won't break any existing code, nor it really adds new API but I can ask release team for exception if you think of it as such.
Created attachment 311019 [details] [review] location: Make timestamp prop writable on construction While this isn't exactly (nor it's likely to be) needed by geocode-glib itself but is needed in geoclue, where we need to set the timestamps on the derived GClueLocation object from GPS.
Review of attachment 311019 [details] [review]: Thanks! My feeling is a bit that it is kind of weird for a location to have a timestamp. Since a location has probably been that location for quite a long time. So maybe the best way would be for you to add a property to the derivied GClueLocation object? Where it might make more sense? And call it GPSTimestamp, or something? The commit log itself here makes a case that it is not Geocode material. ::: geocode-glib/geocode-location.c @@ +601,3 @@ + * + * Set this to 0 (which can only be done at construction time) to set it + * to system time. Why is this mechanism needed? If one just do not set it at all you get "system time", right? Why the extra trickery with setting it to 0? Isn't it more that the default is "system time"?
Review of attachment 311019 [details] [review]: Well I'm not adding a new property so whether timestamp makes sense or not, isn't quite relevant to this patch. ::: geocode-glib/geocode-location.c @@ +601,3 @@ + * + * Set this to 0 (which can only be done at construction time) to set it + * to system time. cause the default is '0' and gobject will call the proper setting with '0' if none is passed to g_object_new(). This will mean that all existing code will not get a 0 in new instances. _init() is called *before* the construct properties are set so that won't work.
Review of attachment 311019 [details] [review]: Oh and GClueLocation is not GPS-specific so wouldn't really want to add a new gps-spefici property when we already have a timestamp property in the base class. Besides what's this property for?
Review of attachment 311019 [details] [review]: Right now the property is the timestamp of the Geocode::Location object. Not the timestamp of a 'location' which would just be weird I feel.
Review of attachment 311019 [details] [review]: > Right now the property is the timestamp of the Geocode::Location object. And what does that mean exactly and why do we need timestamp of object rather than location it represents?
Review of attachment 311019 [details] [review]: In this context it is to know when we got this Place (location) from geocode-glib (and thus from OSM) so to know if it is stale or not. I guess. I do not really know why it is there, but that is the implication now.
Review of attachment 311019 [details] [review]: > In this context it is to know when we got this Place (location) from geocode-glib (and thus from OSM i-e timestamp of the location, not the object. So if you'd get a timestamp from OSM (for the sake of argument), you'll want to set that on the location (in which case it's not as unlikely for this change to be useful to geocode-glib itself).
Review of attachment 311019 [details] [review]: Ok. So my position is that the timestamp is a bit weird in it self. But it is there, so setting from construction might not be that weird. Bastien, how do you feel?
The timestamp is supposed to be the value, not when the location was created, but when the object was created, as is done in: CLLocation on iOS: https://developer.apple.com/library/ios/documentation/CoreLocation/Reference/CLLocation_Class/#//apple_ref/occ/instp/CLLocation/timestamp Web browsers: https://developer.mozilla.org/en-US/docs/Web/API/Position/timestamp or Android: http://developer.android.com/reference/android/location/Location.html Being able to convert the GClueLocation (exported across D-Bus) into a GeocodeLocation without losing metadata is useful. We could use that property to set the right creation time when loading places from the cache, or when converting it from Geoclue in applications (though I don't like the fact that applications would be responsible for that). Making the property writable at construction time is fine by me, but the implementation isn't good enough to me.
Review of attachment 311019 [details] [review]: ::: geocode-glib/geocode-location.c @@ +159,2 @@ static void +geocode_location_set_timestamp (GeocodeLocation *location, You need to do that in constructed. @@ +161,3 @@ + guint64 timestamp) +{ + if (timestamp != 0) { Compare to (guint64) -1 @@ +600,3 @@ * <ulink url="http://en.wikipedia.org/wiki/Unix_epoch">Epoch</ulink>. + * + * Set this to 0 (which can only be done at construction time) to set it And remove this, as it will be set automatically to the current time when constructed. @@ +609,2 @@ 0, G_MAXINT64, That should be G_MAXUINT64... @@ -603,3 @@ - g_get_current_time (&tv); - location->priv->timestamp = tv.tv_sec; Set it to (guint64) -1
Review of attachment 311019 [details] [review]: ::: geocode-glib/geocode-location.c @@ +159,2 @@ static void +geocode_location_set_timestamp (GeocodeLocation *location, I actually tried constructed first but somehow it didn't work last time. Now I tried again it works. I probably got confused with an issue in GClueLocation. @@ +161,3 @@ + guint64 timestamp) +{ + if (timestamp != 0) { But that is not the default value and hence constructed would see a '0' if this property is not set. @@ -603,3 @@ - g_get_current_time (&tv); - location->priv->timestamp = tv.tv_sec; Sure but gobject will overwrite it to default value after _init().
Right, then 0 as a default value then. But need to use constructed instead.
(In reply to Bastien Nocera from comment #13) > Right, then 0 as a default value then. But need to use constructed instead. Sure. Also since '0' can be passed as value for timestamp by caller of g_object_new() and constructed has no means of knowing if it's gobject system that set it to 0 or it was explicitly requested, I'll keep the new docs on the property.
Created attachment 311065 [details] [review] location: Make timestamp prop writable on construction v2: This makes use of _constructed to set the timestamp from system clock.
(In reply to Zeeshan Ali (Khattak) from comment #14) > (In reply to Bastien Nocera from comment #13) > > Right, then 0 as a default value then. But need to use constructed instead. > > Sure. Also since '0' can be passed as value for timestamp by caller of > g_object_new() and constructed has no means of knowing if it's gobject > system that set it to 0 or it was explicitly requested, I'll keep the new > docs on the property. You don't need to pass 0, in fact, you don't need to pass anything. You could say: "A value of 0 (zero) will be interpreted as the current time".
Created attachment 311067 [details] [review] location: Make timestamp prop writable on construction v3: Update docs.
Review of attachment 311067 [details] [review]: > nor it's likely to be "nor is it likely to be" Rest looks good. ::: geocode-glib/geocode-location.c @@ +173,3 @@ + + if (location->priv->timestamp != 0) + return; Indentation. @@ +219,3 @@ + case PROP_TIMESTAMP: + geocode_location_set_timestamp (location, I'm not sure we need a function for one line.
Review of attachment 311067 [details] [review]: ::: geocode-glib/geocode-location.c @@ +219,3 @@ + case PROP_TIMESTAMP: + geocode_location_set_timestamp (location, Me neither but I was trying to be consistent with rest of the property setters.
Created attachment 311069 [details] [review] location: Make timestamp prop writable on construction v4: Fix minr issues from last review.
Review of attachment 311069 [details] [review]: Aye.
Attachment 311069 [details] pushed as 11d6c40 - location: Make timestamp prop writable on construction