Bug 94157 - Expose the timestamp in the dbus api
Summary: Expose the timestamp in the dbus api
Status: RESOLVED FIXED
Alias: None
Product: GeoClue
Classification: Unclassified
Component: General (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Geoclue Bugs
QA Contact: Geoclue Bugs
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-02-15 11:52 UTC by Emilio Pozuelo Monfort
Modified: 2016-04-20 17:49 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Expose the location timestamp (2.11 KB, patch)
2016-02-15 11:52 UTC, Emilio Pozuelo Monfort
Details | Splinter Review
Expose the location timestamp (2.63 KB, patch)
2016-02-15 15:07 UTC, Emilio Pozuelo Monfort
Details | Splinter Review
gclue-service-location: create the location object at once (2.42 KB, patch)
2016-03-08 17:03 UTC, Emilio Pozuelo Monfort
Details | Splinter Review
Expose the location's timestamp (3.25 KB, patch)
2016-03-08 17:03 UTC, Emilio Pozuelo Monfort
Details | Splinter Review
where-am-i: show the timestamp if available (1.07 KB, patch)
2016-03-08 17:03 UTC, Emilio Pozuelo Monfort
Details | Splinter Review
where-am-i: show the timestamp if available (1.26 KB, patch)
2016-03-08 17:43 UTC, Emilio Pozuelo Monfort
Details | Splinter Review
where-am-i: show the timestamp if available (1.26 KB, text/plain)
2016-03-08 17:45 UTC, Emilio Pozuelo Monfort
Details
Expose the location timestamp (3.25 KB, patch)
2016-03-15 12:47 UTC, Emilio Pozuelo Monfort
Details | Splinter Review
where-am-i: show the timestamp if available (1.46 KB, patch)
2016-03-15 12:47 UTC, Emilio Pozuelo Monfort
Details | Splinter Review
Expose the location timestamp (4.37 KB, patch)
2016-04-19 14:44 UTC, Emilio Pozuelo Monfort
Details | Splinter Review
where-am-i: show the timestamp if available (1.50 KB, patch)
2016-04-19 14:47 UTC, Emilio Pozuelo Monfort
Details | Splinter Review
gclue-service-location: create the location object at once (2.43 KB, patch)
2016-04-19 14:50 UTC, Emilio Pozuelo Monfort
Details | Splinter Review
Add a gclue_location_new_full() function (4.86 KB, patch)
2016-04-20 11:42 UTC, Emilio Pozuelo Monfort
Details | Splinter Review
Expose the location timestamp over D-Bus (4.26 KB, patch)
2016-04-20 11:43 UTC, Emilio Pozuelo Monfort
Details | Splinter Review
where-am-i: show the timestamp (1.62 KB, patch)
2016-04-20 11:43 UTC, Emilio Pozuelo Monfort
Details | Splinter Review
Expose the location timestamp over D-Bus (6.04 KB, patch)
2016-04-20 12:42 UTC, Emilio Pozuelo Monfort
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Emilio Pozuelo Monfort 2016-02-15 11:52:06 UTC
The timestamp is not exposed in the Location interface. The attached patch adds it. However where-am-i gives me 0 atm, not sure if that's because the provider is not setting the timestamp properly or because of a problem with my patch. I need to investigate that.
Comment 1 Emilio Pozuelo Monfort 2016-02-15 11:52:51 UTC
Created attachment 121761 [details] [review]
Expose the location timestamp
Comment 2 Philip Withnall 2016-02-15 12:38:36 UTC
Comment on attachment 121761 [details] [review]
Expose the location timestamp

Review of attachment 121761 [details] [review]:
-----------------------------------------------------------------

::: demo/where-am-i.c
@@ +97,5 @@
>          desc = gclue_location_get_description (location);
>          if (strlen (desc) > 0)
>                  g_print ("Description: %s\n", desc);
> +        timestamp = gclue_location_get_timestamp (location);
> +        g_print ("Timestamp: %lu\n", timestamp);

Add units to the output?

::: src/org.freedesktop.GeoClue2.Location.xml
@@ +78,5 @@
> +
> +    <!--
> +        Timestamp:
> +
> +        The timestamp when the location was determined.

What units is this in?

Probably makes sense to also document:
 • GeoClue doesn’t guarantee that the timestamps for successive Locations are monotonically increasing (they might stay the same, or even decrease if the backend does something weird).
 • The timestamp might be significantly older than the current time (from g_get_current_time(), for example) due to using a cached location while waiting for a new GPS lock, for example.
 • Clarify that the time is the time of measurement, not the time the Location object was sent to the client.
Comment 3 Emilio Pozuelo Monfort 2016-02-15 15:07:45 UTC
Created attachment 121767 [details] [review]
Expose the location timestamp

(In reply to Philip Withnall from comment #2)
> Comment on attachment 121761 [details] [review] [review]
> Expose the location timestamp
> 
> Review of attachment 121761 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: demo/where-am-i.c
> @@ +97,5 @@
> >          desc = gclue_location_get_description (location);
> >          if (strlen (desc) > 0)
> >                  g_print ("Description: %s\n", desc);
> > +        timestamp = gclue_location_get_timestamp (location);
> > +        g_print ("Timestamp: %lu\n", timestamp);
> 
> Add units to the output?

Done. Also now conditionally printing (as it happens with other properties) and using G_GNUC_GUINT64_FORMAT.

> ::: src/org.freedesktop.GeoClue2.Location.xml
> @@ +78,5 @@
> > +
> > +    <!--
> > +        Timestamp:
> > +
> > +        The timestamp when the location was determined.
> 
> What units is this in?

Fixed.

> Probably makes sense to also document:
>  • GeoClue doesn’t guarantee that the timestamps for successive Locations
> are monotonically increasing (they might stay the same, or even decrease if
> the backend does something weird).
>  • The timestamp might be significantly older than the current time (from
> g_get_current_time(), for example) due to using a cached location while
> waiting for a new GPS lock, for example.
>  • Clarify that the time is the time of measurement, not the time the
> Location object was sent to the client.

Noted.

Thanks.
Comment 4 Emilio Pozuelo Monfort 2016-03-07 16:56:52 UTC
Ping?
Comment 5 Zeeshan Ali 2016-03-07 17:19:29 UTC
(In reply to Emilio Pozuelo Monfort from comment #4)
> Ping?

You haven't yet uploaded the new patch after review comments addressed so this is waiting on you. :)
Comment 6 Zeeshan Ali 2016-03-07 17:22:47 UTC
(In reply to Zeeshan Ali from comment #5)
> (In reply to Emilio Pozuelo Monfort from comment #4)
> > Ping?
> 
> You haven't yet uploaded the new patch after review comments addressed so
> this is waiting on you. :)

Ah! You attached the patch while answering the review so I missed that. Had to re-read the bug a few times to realize. In future, please comment on review separately than when attaching the new patch. Oh and if you could please reply to inline comments using the 'review' link (i-e in splinter tool), that would be very helpful.

I'll look at your patch soon..
Comment 7 Emilio Pozuelo Monfort 2016-03-07 17:24:09 UTC
(In reply to Zeeshan Ali from comment #6)
> (In reply to Zeeshan Ali from comment #5)
> > (In reply to Emilio Pozuelo Monfort from comment #4)
> > > Ping?
> > 
> > You haven't yet uploaded the new patch after review comments addressed so
> > this is waiting on you. :)
> 
> Ah! You attached the patch while answering the review so I missed that. Had
> to re-read the bug a few times to realize. In future, please comment on
> review separately than when attaching the new patch. Oh and if you could
> please reply to inline comments using the 'review' link (i-e in splinter
> tool), that would be very helpful.

OK. And no worries :)

> I'll look at your patch soon..

Great, thanks!
Comment 8 Zeeshan Ali 2016-03-07 18:12:04 UTC
Comment on attachment 121767 [details] [review]
Expose the location timestamp

Review of attachment 121767 [details] [review]:
-----------------------------------------------------------------

Where is the patch to actually set the timetamp on D-Bus location objects?

::: src/org.freedesktop.GeoClue2.Location.xml
@@ +85,5 @@
> +
> +        Note that GeoClue can't guarantee that the timestamp will always
> +        monotonically increase, as a backend may not respect that.
> +        Also note that a timestamp can be old as compared to what
> +        g_get_current_time() returns, e.g. because of a cached location.

We can't assume the audience to know about g_get_current_time().

"old as compared to what g_get_current_time() returns" -> "very old".
Comment 9 Zeeshan Ali 2016-03-07 18:13:18 UTC
Comment on attachment 121767 [details] [review]
Expose the location timestamp

Review of attachment 121767 [details] [review]:
-----------------------------------------------------------------

::: demo/where-am-i.c
@@ +74,4 @@
>  {
>          GClueLocation *location;
>          gdouble altitude, speed, heading;
> +        guint64 timestamp;

Showing of the timestamp in demo app, is a separate logical change so please put changes to this file in a separate patch.
Comment 10 Emilio Pozuelo Monfort 2016-03-08 17:03:16 UTC
Created attachment 122163 [details] [review]
gclue-service-location: create the location object at once
Comment 11 Emilio Pozuelo Monfort 2016-03-08 17:03:40 UTC
Created attachment 122164 [details] [review]
Expose the location's timestamp
Comment 12 Emilio Pozuelo Monfort 2016-03-08 17:03:59 UTC
Created attachment 122165 [details] [review]
where-am-i: show the timestamp if available
Comment 13 Emilio Pozuelo Monfort 2016-03-08 17:05:43 UTC
Comment on attachment 121767 [details] [review]
Expose the location timestamp

Review of attachment 121767 [details] [review]:
-----------------------------------------------------------------

Thanks for the review. These should be addressed in the new patches.
Comment 14 Emilio Pozuelo Monfort 2016-03-08 17:09:59 UTC
FWIW where-am-i now prints:

Client object: /org/freedesktop/GeoClue2/Client/1

New location:
Latitude:    38.002582°
Longitude:   -1.139290°
Accuracy:    1000.000000 meters
Timestamp: 1457456967 seconds since epoch
Comment 15 Zeeshan Ali 2016-03-08 17:13:35 UTC
(In reply to Emilio Pozuelo Monfort from comment #14)
> FWIW where-am-i now prints:
> 
> Client object: /org/freedesktop/GeoClue2/Client/1
> 
> New location:
> Latitude:    38.002582°
> Longitude:   -1.139290°
> Accuracy:    1000.000000 meters
> Timestamp: 1457456967 seconds since epoch

Cool, two comments already. :)

* Can you please align the value with other values above it?

* Could we please print the date in a human-friendly format? g_date_time_new_from_unix_utc() and g_date_time_format() should be helpful there.
Comment 16 Emilio Pozuelo Monfort 2016-03-08 17:27:59 UTC
(In reply to Zeeshan Ali from comment #15)
> (In reply to Emilio Pozuelo Monfort from comment #14)
> > FWIW where-am-i now prints:
> > 
> > Client object: /org/freedesktop/GeoClue2/Client/1
> > 
> > New location:
> > Latitude:    38.002582°
> > Longitude:   -1.139290°
> > Accuracy:    1000.000000 meters
> > Timestamp: 1457456967 seconds since epoch
> 
> Cool, two comments already. :)
> 
> * Can you please align the value with other values above it?

Hah, sure. I didn't notice that!

> * Could we please print the date in a human-friendly format?
> g_date_time_new_from_unix_utc() and g_date_time_format() should be helpful
> there.

Yeah I thought about that, but I thought the raw timestamp could be useful for debugging purposes. I could print both the timestamp and a friendly formatted time though, wdyt?
Comment 17 Zeeshan Ali 2016-03-08 17:34:59 UTC
(In reply to Emilio Pozuelo Monfort from comment #16)
> (In reply to Zeeshan Ali from comment #15)
> > (In reply to Emilio Pozuelo Monfort from comment #14)
> > > FWIW where-am-i now prints:
> > > 
> > > Client object: /org/freedesktop/GeoClue2/Client/1
> > > 
> > > New location:
> > > Latitude:    38.002582°
> > > Longitude:   -1.139290°
> > > Accuracy:    1000.000000 meters
> > > Timestamp: 1457456967 seconds since epoch
> > 
> > Cool, two comments already. :)
> > 
> > * Can you please align the value with other values above it?
> 
> Hah, sure. I didn't notice that!
> 
> > * Could we please print the date in a human-friendly format?
> > g_date_time_new_from_unix_utc() and g_date_time_format() should be helpful
> > there.
> 
> Yeah I thought about that, but I thought the raw timestamp could be useful
> for debugging purposes. I could print both the timestamp and a friendly
> formatted time though, wdyt?

Or add an option to switch to unix timestamp?
Comment 18 Emilio Pozuelo Monfort 2016-03-08 17:43:29 UTC
Created attachment 122166 [details] [review]
where-am-i: show the timestamp if available
Comment 19 Emilio Pozuelo Monfort 2016-03-08 17:44:30 UTC
Don't want to make it too complex unnecessarily. Let's show a nice timestamp, and we can add the seconds since epoch if necessary.
Comment 20 Emilio Pozuelo Monfort 2016-03-08 17:45:37 UTC
Created attachment 122167 [details]
where-am-i: show the timestamp if available
Comment 21 Emilio Pozuelo Monfort 2016-03-08 17:45:57 UTC
New location:
Latitude:    37.989544°
Longitude:   -1.129570°
Accuracy:    1000.000000 meters
Timestamp:   Tue 08 Mar 2016 17:45:08 UTC
Comment 22 Zeeshan Ali 2016-03-08 22:15:58 UTC
(In reply to Emilio Pozuelo Monfort from comment #20)
> Created attachment 122167 [details]
> where-am-i: show the timestamp if available

Sure. I'll review tomorrow.
Comment 23 Bastien Nocera 2016-03-09 10:24:10 UTC
(In reply to Emilio Pozuelo Monfort from comment #19)
> Don't want to make it too complex unnecessarily. Let's show a nice
> timestamp, and we can add the seconds since epoch if necessary.

I'd show the nice date first, and the epoch between parenthesis.

Note though that GPS also exports microseconds, so I would expose this as a float instead.
Comment 24 Emilio Pozuelo Monfort 2016-03-09 10:27:57 UTC
(In reply to Bastien Nocera from comment #23)
> (In reply to Emilio Pozuelo Monfort from comment #19)
> > Don't want to make it too complex unnecessarily. Let's show a nice
> > timestamp, and we can add the seconds since epoch if necessary.
> 
> I'd show the nice date first, and the epoch between parenthesis.
> 
> Note though that GPS also exports microseconds, so I would expose this as a
> float instead.

But geocode only gives us the seconds as an int64, so we don't get that precision AFAICS.
Comment 25 Zeeshan Ali 2016-03-09 13:35:13 UTC
(In reply to Emilio Pozuelo Monfort from comment #24)
> (In reply to Bastien Nocera from comment #23)
> > (In reply to Emilio Pozuelo Monfort from comment #19)
> > > Don't want to make it too complex unnecessarily. Let's show a nice
> > > timestamp, and we can add the seconds since epoch if necessary.
> > 
> > I'd show the nice date first, and the epoch between parenthesis.
> > 
> > Note though that GPS also exports microseconds, so I would expose this as a
> > float instead.
> 
> But geocode only gives us the seconds as an int64, so we don't get that
> precision AFAICS.

True but would be nice to expose it as float so tomorrow if we have more precision internally, we don't have to change the API.
Comment 26 Emilio Pozuelo Monfort 2016-03-14 13:05:01 UTC
(In reply to Zeeshan Ali from comment #25)
> (In reply to Emilio Pozuelo Monfort from comment #24)
> > (In reply to Bastien Nocera from comment #23)
> > > (In reply to Emilio Pozuelo Monfort from comment #19)
> > > > Don't want to make it too complex unnecessarily. Let's show a nice
> > > > timestamp, and we can add the seconds since epoch if necessary.
> > > 
> > > I'd show the nice date first, and the epoch between parenthesis.
> > > 
> > > Note though that GPS also exports microseconds, so I would expose this as a
> > > float instead.
> > 
> > But geocode only gives us the seconds as an int64, so we don't get that
> > precision AFAICS.
> 
> True but would be nice to expose it as float so tomorrow if we have more
> precision internally, we don't have to change the API.

I don't think a float has the necessary precision for a timestamp with milliseconds or microseconds. It'd probably be better to expose this as a (tt) or as a (tu), as if it were a timespec struct.
Comment 27 Bastien Nocera 2016-03-14 13:07:06 UTC
(In reply to Emilio Pozuelo Monfort from comment #26)
> (In reply to Zeeshan Ali from comment #25)
> > (In reply to Emilio Pozuelo Monfort from comment #24)
> > > (In reply to Bastien Nocera from comment #23)
> > > > (In reply to Emilio Pozuelo Monfort from comment #19)
> > > > > Don't want to make it too complex unnecessarily. Let's show a nice
> > > > > timestamp, and we can add the seconds since epoch if necessary.
> > > > 
> > > > I'd show the nice date first, and the epoch between parenthesis.
> > > > 
> > > > Note though that GPS also exports microseconds, so I would expose this as a
> > > > float instead.
> > > 
> > > But geocode only gives us the seconds as an int64, so we don't get that
> > > precision AFAICS.
> > 
> > True but would be nice to expose it as float so tomorrow if we have more
> > precision internally, we don't have to change the API.
> 
> I don't think a float has the necessary precision for a timestamp with
> milliseconds or microseconds. It'd probably be better to expose this as a
> (tt) or as a (tu), as if it were a timespec struct.

D-Bus doesn't floats, just doubles, which I think would have enough precision.
Comment 28 Zeeshan Ali 2016-03-15 12:17:08 UTC
(In reply to Bastien Nocera from comment #27)
> (In reply to Emilio Pozuelo Monfort from comment #26)
> > (In reply to Zeeshan Ali from comment #25)
> > > (In reply to Emilio Pozuelo Monfort from comment #24)
> > > > (In reply to Bastien Nocera from comment #23)
> > > > > (In reply to Emilio Pozuelo Monfort from comment #19)
> > > > > > Don't want to make it too complex unnecessarily. Let's show a nice
> > > > > > timestamp, and we can add the seconds since epoch if necessary.
> > > > > 
> > > > > I'd show the nice date first, and the epoch between parenthesis.
> > > > > 
> > > > > Note though that GPS also exports microseconds, so I would expose this as a
> > > > > float instead.
> > > > 
> > > > But geocode only gives us the seconds as an int64, so we don't get that
> > > > precision AFAICS.
> > > 
> > > True but would be nice to expose it as float so tomorrow if we have more
> > > precision internally, we don't have to change the API.
> > 
> > I don't think a float has the necessary precision for a timestamp with
> > milliseconds or microseconds. It'd probably be better to expose this as a
> > (tt) or as a (tu), as if it were a timespec struct.
> 
> D-Bus doesn't floats, just doubles, which I think would have enough
> precision.

Just to be clear, I meant floats as in "floating point number" and assumed a double. :)
Comment 29 Emilio Pozuelo Monfort 2016-03-15 12:19:18 UTC
(In reply to Bastien Nocera from comment #27)
> D-Bus doesn't floats, just doubles, which I think would have enough
> precision.

(In reply to Zeeshan Ali from comment #28)
> Just to be clear, I meant floats as in "floating point number" and assumed a
> double. :)

OK, I didn't know there were no 32bit floats in dbus... Let's do that then.
Comment 30 Emilio Pozuelo Monfort 2016-03-15 12:47:01 UTC
Created attachment 122320 [details] [review]
Expose the location timestamp
Comment 31 Emilio Pozuelo Monfort 2016-03-15 12:47:39 UTC
Created attachment 122321 [details] [review]
where-am-i: show the timestamp if available
Comment 32 Emilio Pozuelo Monfort 2016-03-15 12:50:43 UTC
New output:

Client object: /org/freedesktop/GeoClue2/Client/1

New location:
Latitude:    38.079998°
Longitude:   -1.052984°
Accuracy:    10.000000 meters
Timestamp:   Tue 15 Mar 2016 13:45:36 CET (1458045936 seconds since the Epoch)

There's no g_date_time_new_from_double_local. There's _new_from_timeval_local, but then we'd need to split the timestamp into seconds and microseconds (besides, GTimeVal seems to-be-deprecated). So this seemed easier and better.
Comment 33 Philip Withnall 2016-03-22 10:28:09 UTC
(In reply to Emilio Pozuelo Monfort from comment #26)
> (In reply to Zeeshan Ali from comment #25)
> > (In reply to Emilio Pozuelo Monfort from comment #24)
> > > (In reply to Bastien Nocera from comment #23)
> > > > (In reply to Emilio Pozuelo Monfort from comment #19)
> > > > > Don't want to make it too complex unnecessarily. Let's show a nice
> > > > > timestamp, and we can add the seconds since epoch if necessary.
> > > > 
> > > > I'd show the nice date first, and the epoch between parenthesis.
> > > > 
> > > > Note though that GPS also exports microseconds, so I would expose this as a
> > > > float instead.
> > > 
> > > But geocode only gives us the seconds as an int64, so we don't get that
> > > precision AFAICS.
> > 
> > True but would be nice to expose it as float so tomorrow if we have more
> > precision internally, we don't have to change the API.
> 
> I don't think a float has the necessary precision for a timestamp with
> milliseconds or microseconds. It'd probably be better to expose this as a
> (tt) or as a (tu), as if it were a timespec struct.

I would suggest a pair of integers (seconds since the epoch; and microseconds within) rather than any kind of float/double. Integers give you consistent accuracy, rather than a variable error depending on the value you’re trying to approximate with your float mantissa and exponent.
Comment 34 Zeeshan Ali 2016-03-22 12:28:33 UTC
(In reply to Philip Withnall from comment #33)
> (In reply to Emilio Pozuelo Monfort from comment #26)
> > (In reply to Zeeshan Ali from comment #25)
> > > (In reply to Emilio Pozuelo Monfort from comment #24)
> > > > (In reply to Bastien Nocera from comment #23)
> > > > > (In reply to Emilio Pozuelo Monfort from comment #19)
> > > > > > Don't want to make it too complex unnecessarily. Let's show a nice
> > > > > > timestamp, and we can add the seconds since epoch if necessary.
> > > > > 
> > > > > I'd show the nice date first, and the epoch between parenthesis.
> > > > > 
> > > > > Note though that GPS also exports microseconds, so I would expose this as a
> > > > > float instead.
> > > > 
> > > > But geocode only gives us the seconds as an int64, so we don't get that
> > > > precision AFAICS.
> > > 
> > > True but would be nice to expose it as float so tomorrow if we have more
> > > precision internally, we don't have to change the API.
> > 
> > I don't think a float has the necessary precision for a timestamp with
> > milliseconds or microseconds. It'd probably be better to expose this as a
> > (tt) or as a (tu), as if it were a timespec struct.
> 
> I would suggest a pair of integers (seconds since the epoch; and
> microseconds within) rather than any kind of float/double. Integers give you
> consistent accuracy, rather than a variable error depending on the value
> you’re trying to approximate with your float mantissa and exponent.

We're using double for other numbers in the API so I'd stick with those here, unless there is any reason not to use double in this particular case? According to my quick calculations, we get 2^53 seconds which is equal to roughly 285616414 years. I'm pretty sure Geoclue will be long forgotten after all those years. :)
Comment 35 Philip Withnall 2016-03-22 14:02:27 UTC
(In reply to Zeeshan Ali from comment #34)
> We're using double for other numbers in the API so I'd stick with those
> here, unless there is any reason not to use double in this particular case?
> According to my quick calculations, we get 2^53 seconds which is equal to
> roughly 285616414 years. I'm pretty sure Geoclue will be long forgotten
> after all those years. :)

My point is not the range of a double, but its accuracy. A double can’t represent every integer exactly; let alone representing every microsecond exactly. When dealing with floating point numbers, there is almost always some quantisation error involved, which isn’t great when you’re trying to represent time with a fixed periodic increment; or when you’re trying to do arithmetic with the numbers without increasing the error.
Comment 36 Bastien Nocera 2016-03-22 14:12:03 UTC
(In reply to Philip Withnall from comment #35)
> (In reply to Zeeshan Ali from comment #34)
> > We're using double for other numbers in the API so I'd stick with those
> > here, unless there is any reason not to use double in this particular case?
> > According to my quick calculations, we get 2^53 seconds which is equal to
> > roughly 285616414 years. I'm pretty sure Geoclue will be long forgotten
> > after all those years. :)
> 
> My point is not the range of a double, but its accuracy. A double can’t
> represent every integer exactly; let alone representing every microsecond
> exactly. When dealing with floating point numbers, there is almost always
> some quantisation error involved, which isn’t great when you’re trying to
> represent time with a fixed periodic increment; or when you’re trying to do
> arithmetic with the numbers without increasing the error.

https://en.wikipedia.org/wiki/Double-precision_floating-point_format says that you can have no loss of precision for up to 15 decimal points. We'd already use 6 for the microseconds, leaving us with 9. The current date in seconds since epoch already uses 10 digits. 10 + 6 > 15. Loss of data.

So, either you use a 128-bit float, or simply split it in seconds and microseconds.

I should probably have checked that before thinking that a double would be big enough.
Comment 37 Philip Withnall 2016-03-22 14:37:55 UTC
(In reply to Bastien Nocera from comment #36)
> I should probably have checked that before thinking that a double would be
> big enough.

I go with the general rule that a floating point number should always set off alarm bells unless proven to be the best choice.
Comment 38 Zeeshan Ali 2016-03-22 16:53:28 UTC
Alright, thanks for educating me. :) Let's go with (tt) then?
Comment 39 Philip Withnall 2016-03-22 17:13:29 UTC
(In reply to Zeeshan Ali from comment #38)
> Alright, thanks for educating me. :) Let's go with (tt) then?

Sounds good to me.
Comment 40 Emilio Pozuelo Monfort 2016-04-19 14:44:41 UTC
Created attachment 123055 [details] [review]
Expose the location timestamp

This changes the timestamp to (tt).
Comment 41 Emilio Pozuelo Monfort 2016-04-19 14:47:03 UTC
Created attachment 123056 [details] [review]
where-am-i: show the timestamp if available
Comment 42 Emilio Pozuelo Monfort 2016-04-19 14:50:05 UTC
Created attachment 123057 [details] [review]
gclue-service-location: create the location object at once

I'm attaching this new version of the first patch as I rebased the branch on top of master, but it's basically unchanged.
Comment 43 Zeeshan Ali 2016-04-19 15:32:53 UTC
Comment on attachment 123055 [details] [review]
Expose the location timestamp

Review of attachment 123055 [details] [review]:
-----------------------------------------------------------------

Plead add " over D-Bus" to short log (or to description if shortlog goes beyond 50 chars).

::: src/gclue-service-location.c
@@ +162,4 @@
>                          (location, gclue_location_get_speed (loc));
>                  gclue_dbus_location_set_heading
>                          (location, gclue_location_get_heading (loc));
> +                timestamp = g_variant_new ("(tt)", (guint64) geocode_location_get_timestamp (g_loc), (guint64) 0);

This is going beyond 80 chars. Please break this line.
Comment 44 Zeeshan Ali 2016-04-19 15:39:08 UTC
Comment on attachment 123056 [details] [review]
where-am-i: show the timestamp if available

Review of attachment 123056 [details] [review]:
-----------------------------------------------------------------

Just some minor nitpicks:

'the' in shortlog is redundant, "Show location timestamp" with "If available" in description would be better IMO.

::: demo/where-am-i.c
@@ +100,5 @@
>                  g_print ("Description: %s\n", desc);
> +
> +        timestamp = gclue_location_get_timestamp (location);
> +        if (timestamp) {
> +                GDateTime *dt;

I prefer descriptive names so I'd prefer 'date_time'. 'tv' is fine since it's commonly used for TimeVal and even used in gettimeofday manpage.

@@ +101,5 @@
> +
> +        timestamp = gclue_location_get_timestamp (location);
> +        if (timestamp) {
> +                GDateTime *dt;
> +                gchar *format;

misleading name, the value is not a format string but rather formatted string. formatted_str or just str would be better.
Comment 45 Zeeshan Ali 2016-04-19 15:43:41 UTC
Comment on attachment 123057 [details] [review]
gclue-service-location: create the location object at once

Review of attachment 123057 [details] [review]:
-----------------------------------------------------------------

'gclue-' part in shortlog is redundant and makes the shortlog longer than recommended 50 chars limit.

::: src/gclue-service-location.c
@@ +93,3 @@
>  
>                  location = GCLUE_DBUS_LOCATION (object);
> +                loc = g_object_new (GCLUE_TYPE_LOCATION,

NACK, there is a reason why most gobjects provide _new() methods. I'd rather we add a gclue_location_full() instead. Also assuming we don't use gclue_location_new_with_description() anywhere else, remove that function while removing it's only usage.
Comment 46 Emilio Pozuelo Monfort 2016-04-20 11:42:38 UTC
Created attachment 123087 [details] [review]
Add a gclue_location_new_full() function
Comment 47 Emilio Pozuelo Monfort 2016-04-20 11:43:03 UTC
Created attachment 123088 [details] [review]
Expose the location timestamp over D-Bus
Comment 48 Emilio Pozuelo Monfort 2016-04-20 11:43:33 UTC
Created attachment 123089 [details] [review]
where-am-i: show the timestamp
Comment 49 Emilio Pozuelo Monfort 2016-04-20 11:43:51 UTC
All your comments should be addressed.
Comment 50 Zeeshan Ali 2016-04-20 12:25:41 UTC
Comment on attachment 123087 [details] [review]
Add a gclue_location_new_full() function

Review of attachment 123087 [details] [review]:
-----------------------------------------------------------------

ack
Comment 51 Zeeshan Ali 2016-04-20 12:27:23 UTC
Comment on attachment 123088 [details] [review]
Expose the location timestamp over D-Bus

Review of attachment 123088 [details] [review]:
-----------------------------------------------------------------

just one question.

::: src/gclue-service-location.c
@@ +164,5 @@
>                          (location, gclue_location_get_heading (loc));
> +                timestamp = g_variant_new
> +                        ("(tt)",
> +                         (guint64) geocode_location_get_timestamp (g_loc),
> +                         (guint64) 0);

is explicit conversion really needed here?
Comment 52 Zeeshan Ali 2016-04-20 12:27:52 UTC
Comment on attachment 123089 [details] [review]
where-am-i: show the timestamp

Review of attachment 123089 [details] [review]:
-----------------------------------------------------------------

ack
Comment 53 Emilio Pozuelo Monfort 2016-04-20 12:40:52 UTC
(In reply to Zeeshan Ali from comment #51)
> Comment on attachment 123088 [details] [review] [review]
> Expose the location timestamp over D-Bus
> 
> Review of attachment 123088 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> just one question.
> 
> ::: src/gclue-service-location.c
> @@ +164,5 @@
> >                          (location, gclue_location_get_heading (loc));
> > +                timestamp = g_variant_new
> > +                        ("(tt)",
> > +                         (guint64) geocode_location_get_timestamp (g_loc),
> > +                         (guint64) 0);
> 
> is explicit conversion really needed here?

The second one is, as otherwise the compiler is likely to make it a 32bit arg, and then g_variant_new will read garbage as it reads 64 bits.

The first cast isn't necessary as geocode_location_get_timestamp returns a guint64, but it doesn't hurt and the code looks better this way IMHO. But I can drop it before committing if you want.

BTW I forgot to add a change to this patch before attaching it here. Adding a timestamp argument to gclue_location_new_full(). Pretty simple, but I'll push the final version here just in case.
Comment 54 Emilio Pozuelo Monfort 2016-04-20 12:42:01 UTC
Created attachment 123090 [details] [review]
Expose the location timestamp over D-Bus

This includes all the changes. Sorry about that.
Comment 55 Zeeshan Ali 2016-04-20 17:49:00 UTC
All patches pushed, one with a small fix.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.