After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 754797 - location: Make timestamp property writable on construction
location: Make timestamp property writable on construction
Status: RESOLVED FIXED
Product: geocode-glib
Classification: Other
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: geocode-glib maintainer(s)
geocode-glib maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2015-09-09 18:15 UTC by Zeeshan Ali
Modified: 2015-09-10 13:11 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
location: Make timestamp prop writable on construction (3.52 KB, patch)
2015-09-09 18:15 UTC, Zeeshan Ali
none Details | Review
location: Make timestamp prop writable on construction (4.13 KB, patch)
2015-09-10 12:44 UTC, Zeeshan Ali
none Details | Review
location: Make timestamp prop writable on construction (4.09 KB, patch)
2015-09-10 12:56 UTC, Zeeshan Ali
none Details | Review
location: Make timestamp prop writable on construction (4.10 KB, patch)
2015-09-10 13:04 UTC, Zeeshan Ali
committed Details | Review

Description Zeeshan Ali 2015-09-09 18:15:31 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.
Comment 1 Zeeshan Ali 2015-09-09 18:15:36 UTC
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.
Comment 2 Jonas Danielsson 2015-09-09 18:51:40 UTC
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"?
Comment 3 Zeeshan Ali 2015-09-09 19:08:33 UTC
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.
Comment 4 Zeeshan Ali 2015-09-09 19:09:42 UTC
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?
Comment 5 Jonas Danielsson 2015-09-09 19:21:33 UTC
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.
Comment 6 Zeeshan Ali 2015-09-09 19:27:59 UTC
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?
Comment 7 Jonas Danielsson 2015-09-09 19:31:27 UTC
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.
Comment 8 Zeeshan Ali 2015-09-09 19:36:26 UTC
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).
Comment 9 Jonas Danielsson 2015-09-10 11:42:10 UTC
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?
Comment 10 Bastien Nocera 2015-09-10 12:02:46 UTC
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.
Comment 11 Bastien Nocera 2015-09-10 12:06:16 UTC
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
Comment 12 Zeeshan Ali 2015-09-10 12:36:00 UTC
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().
Comment 13 Bastien Nocera 2015-09-10 12:38:28 UTC
Right, then 0 as a default value then. But need to use constructed instead.
Comment 14 Zeeshan Ali 2015-09-10 12:42:52 UTC
(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.
Comment 15 Zeeshan Ali 2015-09-10 12:44:10 UTC
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.
Comment 16 Bastien Nocera 2015-09-10 12:46:16 UTC
(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".
Comment 17 Zeeshan Ali 2015-09-10 12:56:41 UTC
Created attachment 311067 [details] [review]
location: Make timestamp prop writable on construction

v3: Update docs.
Comment 18 Bastien Nocera 2015-09-10 13:00:01 UTC
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.
Comment 19 Zeeshan Ali 2015-09-10 13:02:22 UTC
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.
Comment 20 Zeeshan Ali 2015-09-10 13:04:44 UTC
Created attachment 311069 [details] [review]
location: Make timestamp prop writable on construction

v4: Fix minr issues from last review.
Comment 21 Bastien Nocera 2015-09-10 13:05:25 UTC
Review of attachment 311069 [details] [review]:

Aye.
Comment 22 Zeeshan Ali 2015-09-10 13:11:30 UTC
Attachment 311069 [details] pushed as 11d6c40 - location: Make timestamp prop writable on construction