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 50076 - Time API to go with date API
Time API to go with date API
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: general
2.22.x
Other All
: Normal enhancement
: ---
Assigned To: gtkdev
gtkdev
: 69140 628137 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2000-12-15 23:38 UTC by Havoc Pennington
Modified: 2011-02-18 16:13 UTC
See Also:
GNOME target: ---
GNOME version: Unversioned Enhancement


Attachments
Adds gdatetime type to glib (107.51 KB, patch)
2010-05-05 17:44 UTC, Thiago Sousa Santos
none Details | Review
Adds gdatetime type to glib (104.54 KB, patch)
2010-05-24 12:03 UTC, Thiago Sousa Santos
none Details | Review
DateTime vapi file for Vala (3.70 KB, text/x-vala)
2010-05-28 07:46 UTC, Jukka-Pekka Iivonen
  Details
New datetime.vapi with correct copyright. (3.70 KB, text/x-vala)
2010-05-28 07:49 UTC, Jukka-Pekka Iivonen
  Details
Updated gdatetime patch (98.09 KB, patch)
2010-05-28 11:35 UTC, Thiago Sousa Santos
none Details | Review
datetime.vapi updated for the latest GDateTime patch (3.52 KB, text/plain)
2010-05-31 11:15 UTC, Jukka-Pekka Iivonen
  Details
Adds gdatetime type to glib (83.38 KB, patch)
2010-06-14 19:18 UTC, Thiago Sousa Santos
none Details | Review
Adds gdatetime type to glib (81.38 KB, patch)
2010-06-15 13:48 UTC, Thiago Sousa Santos
needs-work Details | Review
Adds gdatetime type to glib (81.05 KB, patch)
2010-06-18 19:05 UTC, Thiago Sousa Santos
none Details | Review
Adds gdatetime type to glib (83.26 KB, patch)
2010-06-23 18:28 UTC, Thiago Sousa Santos
none Details | Review
Adds gdatetime type to glib (82.86 KB, patch)
2010-06-28 16:17 UTC, Thiago Sousa Santos
none Details | Review
Adds gdatetime type to glib (82.86 KB, patch)
2010-06-29 17:14 UTC, Thiago Sousa Santos
none Details | Review
Adds gdatetime type to glib (82.94 KB, patch)
2010-06-29 18:30 UTC, Thiago Sousa Santos
needs-work Details | Review
Adds gdatetime type to glib (85.68 KB, patch)
2010-07-12 12:44 UTC, Thiago Sousa Santos
none Details | Review
gdatetime: adds accuracy field (29.93 KB, patch)
2010-07-12 12:45 UTC, Thiago Sousa Santos
none Details | Review
Adds gdatetime with gregorian proleptic internal representation (98.09 KB, patch)
2010-07-16 11:35 UTC, Thiago Sousa Santos
none Details | Review
Adds gdatetime with gregorian proleptic internal representation (98.09 KB, patch)
2010-07-16 11:38 UTC, Thiago Sousa Santos
none Details | Review
Add GDateTime to GLib (98.52 KB, patch)
2010-08-24 17:28 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
datetime: Add GDateTime to the GType system (2.24 KB, patch)
2010-08-24 17:28 UTC, Emmanuele Bassi (:ebassi)
committed Details | Review
Add GDateTime to GLib (102.92 KB, patch)
2010-08-24 22:15 UTC, Emmanuele Bassi (:ebassi)
committed Details | Review
Avoid using c99 reserved identifier __year (3.11 KB, patch)
2010-08-31 16:32 UTC, Christian Hergert
committed Details | Review
Walk the format string using g_utf8_next_char() and use gunichar (2.46 KB, patch)
2010-08-31 16:34 UTC, Christian Hergert
committed Details | Review
Adds gdatetime type to glib (93.19 KB, patch)
2010-08-31 23:25 UTC, Thiago Sousa Santos
none Details | Review
gdatetime: Use proleptic gregorian (12.19 KB, patch)
2010-09-02 01:16 UTC, Thiago Sousa Santos
committed Details | Review
datetime: Rework time zone support in constructors (40.14 KB, patch)
2010-09-15 13:02 UTC, Emmanuele Bassi (:ebassi)
committed Details | Review
datetime: Allow setting fractionary seconds in new_full() (3.48 KB, patch)
2010-09-15 13:02 UTC, Emmanuele Bassi (:ebassi)
committed Details | Review
datetime: Update modifiers for DST changes (11.37 KB, patch)
2010-09-15 13:02 UTC, Emmanuele Bassi (:ebassi)
committed Details | Review
datetime: Rename g_date_time_printf() to g_date_time_format() (4.20 KB, patch)
2010-09-15 13:40 UTC, Emmanuele Bassi (:ebassi)
committed Details | Review
datetime: Fix hashing (2.62 KB, patch)
2010-09-15 13:48 UTC, Emmanuele Bassi (:ebassi)
committed Details | Review

Description Havoc Pennington 2000-12-15 23:38:55 UTC
Implement time handling that works with GDate. Someone actually submitted
a patch for this IIRC, and we haven't responded yet. See gtk-devel-list.
Comment 1 Havoc Pennington 2001-01-29 19:27:49 UTC
Setting all outstanding bugs against 1.3.x to be due for the 2.0.0 milestone;
will go through and move some of them to API freeze milestone and set milestone
to none for punted features.
Comment 2 Havoc Pennington 2001-01-29 19:29:16 UTC
Moving GLib bugs with API keyword to the API freeze milestone
Comment 3 Havoc Pennington 2001-02-15 21:22:06 UTC
No way for 2.0
Comment 4 Ryan McDougall 2004-01-15 19:50:46 UTC
There has been a request to include the following feature enhancement
to the new framework.
http://bugs.gnome.org/show_bug.cgi?id=69140
Comment 5 Ryan McDougall 2004-01-15 19:53:30 UTC
Havoc, since we are well beyond the timeframe when this bug was
originially filed, it would be worth while to decide whether this
feature should be included in up-comming versions.
Comment 6 Tim-Philipp Müller 2004-04-24 15:49:43 UTC
I still think this is something worth having in GLib. 
 
I have tried to find the existing patch, but wasn't very successful. The 
closest to a patch I could find is a suggestion for an API in form of a header 
file. Here are the relevant list messages that I was able to locate: 
 
 http://mail.gnome.org/archives/gtk-devel-list/2000-November/msg00288.html  
(header file with API suggestion) 
 http://mail.gnome.org/archives/gtk-devel-list/2001-January/msg00079.html 
(requirements) 
 http://mail.gnome.org/archives/gtk-devel-list/2000-November/msg00262.html 
 http://mail.gnome.org/archives/gtk-devel-list/2000-November/msg00253.html 
(http://mail.gnome.org/archives/gtk-app-devel-list/1999-April/msg00218.html) 
 
Cheers 
 -Tim 
 
Comment 7 Philippe Verdy 2005-09-17 10:02:37 UTC
Please update the "show_bug.cgi" servlet that generates this page so that it 
will correctly encode people names (found in their mail From header) that have 
non ASCII characters, so that it won't break the UTF-8 encoding of this page.
-> recode them instrad of generating them blindly.
Comment 8 Philippe Verdy 2005-09-17 10:06:45 UTC
Some more tests for support of non ASCII characters in messages:
àÀ (should display a with grave and A with grave)
èÈ (should display e with grave and E with grave)
äÄ (should display a with grave and A with diaeresis)
ç (should display c with cedilla)
ä (should display a with diaeresis)
€ (should display euro)
Comment 9 Philippe Verdy 2005-09-17 10:08:59 UTC
Some more tests for support of non ASCII characters in messages:
àÀ (should display a with grave and A with grave)
èÈ (should display e with grave and E with grave)
äÄ (should display a with grave and A with diaeresis)
ç (should display c with cedilla)
ä (should display a with diaeresis)
€ (should display euro)

(this time the message in the input form was composed with UTF-8 selected)
Comment 10 Philippe Verdy 2005-09-17 10:16:55 UTC
See the two mesage above: depending on which charset is selected in the 
browser when the input form is displayed and used, the input message is sent 
to the server using that charset, and this information is not kept by your 
server.

It should use that browser provided info (within the form submission data) to 
correctly recode the form data into UTF-8 before storing.

Then the page should be displaed using an explicit UTF-8 encoding... Use the 
following as the first subelement in the <head> element of the generated HTML 
page):
<meta http-equiv="Content-Type" content="text/html; charset="UTF-8">

You shoudl also generate a leading UTF-8 BOM to force some browsers to detect 
this UTF-8 encoding soon, before even having to parse the HTML page. It will 
then allow decoding correctly the page title. Don't let users figure out which 
charset to select in their browser to display this page in bugzilla, (because 
browsers will also try to use heuristic charset detection that can fail quite 
often) as this will break the correct rendering of the rest of the page.

May be all this is a bug of bugzilla itself, and it has already been corrected.
Comment 11 Olav Vitters 2005-09-17 11:06:42 UTC
Philippe: That is all offtopic for this bug. UTF-8 is set for this page, see the
server headers. Yes, I know if you override these server settings you'll still
be able to enter another encoding.
For specific suggestions (how to force the browser to submit in UTF-8 without
changing every <form> we have), please comment in bug 96376. If you have some
magic way to change an unknown encoding -> UTF-8, add that as well.
Comment 12 James Stephenson 2007-10-31 13:59:02 UTC
Is this issue still in future plans for GLib?  I'm surprised that, within the past 4 years, there's been no action on this one.
Comment 13 Havoc Pennington 2007-11-01 15:05:47 UTC
Like most enhancement requests, it's in the future plans if someone does the work on it ;-) (both design writeup/discussion, coding, and adjustments in response to patch review)
Comment 14 Thiago Sousa Santos 2010-05-03 10:51:44 UTC
FYI, there is some work at https://bugzilla.gnome.org/show_bug.cgi?id=344005
Comment 15 Thiago Sousa Santos 2010-05-03 20:04:47 UTC
By "some work" I meant a probably complete solution :)
Comment 16 Thiago Sousa Santos 2010-05-05 17:44:34 UTC
Created attachment 160365 [details] [review]
Adds gdatetime type to glib

Took Christian Hergert's work from bug #344005 and
added its gdatetime to the glib tree.
Comment 17 Christian Hergert 2010-05-06 08:04:17 UTC
It would be nice if people could run the unit tests provided in the patch.  I've only done testing against PST/PDT.

Ideally, for something as important as datetime, we should have a way to automate running tests for every possible timezone.  However, I'm not sure if that's possible without changing the system clock.
Comment 18 Jukka-Pekka Iivonen 2010-05-19 07:12:08 UTC
The patch looks a very nice enhancement to me and I'm looking forward to see it merged :-) Just one thing for me is missing: g_date_time_new_full could take an optional timezone. At least I need to create datetimes with different timezone than the local time or utc. E.g. I need to create a timestamp for a price information of an item I downloaded from a web site. The server provides last update time and I know the timezone of the server so I can easily create timestamps by just parsing the html and filling in the timezone without converting the dates to utc manually first.

And just for completeness G_TIME_SPAN_WEEK could be added.
Comment 19 Christian Hergert 2010-05-19 09:19:37 UTC
(In reply to comment #18)
> The patch looks a very nice enhancement to me and I'm looking forward to see it
> merged :-) Just one thing for me is missing: g_date_time_new_full could take an
> optional timezone.

Unfortunately, I don't know of a way to get accurate tz info for timezones other than the local one (other than shipping a tz database and parsing it).
Comment 20 Dan Winship 2010-05-19 13:15:05 UTC
the system almost certainly already includes a tz database, and it's not *that* hard to parse. libgweather has some code (gweather-timezone.c:parse_tzdata, which specifically extracts just the offset and dst offset for the current year)
Comment 21 Christian Dywan 2010-05-19 13:59:01 UTC
(In reply to comment #16)
> Created an attachment (id=160365) [details] [review]
> Adds gdatetime type to glib

> It would be nice if people could run the unit tests provided in the patch.

The tests pass on my machine (pure x86_64 linux, de_DE.UTF-8, GMT+1, daylight saving time).

+gchar*
+g_date_time_format_for_display (GDateTime *datetime) /* IN */

This is... delicate. The introduction of g_file_size_format_for_display with its hard-coded caused a lot of fervent discussion. File managers don't agree on their default time display. And there are preferences because users often have different preferences.

+g_date_time_new_full (gint year,   /* IN */

Hm... should there be a timezone argument?

+GDateTime*
+g_date_time_parse (const gchar *input) /* IN */
+{
+  /* TODO: Implement parsing with locale support */
+  g_warn_if_reached ();
+  return NULL;
+}

This seems odd. Isn't GDateTime supposed to be immutable?

> + * g_date_time_parse_with_format:

This is *not* fully covering the range of strptime. Notably %a and %b.

I wonder if an argument to secify the locale would be a good idea, if for instance the date is known to originate in  a certain foreign locale.

> + * g_date_time_printf:

I notice small additions (elaborations) to 'strftime' documentation. Nice. Looks compatible in terms of functionality. Probably a good candidate for multiple eyes to double-check on.

> +  /* FIXME we use gint64, we shold expand these limits */
>+  if (year < 1970)
>+    return 0;
>+  else if (year > 2037)
>+    return G_MAXINT64;

The comments in bug 344005 made me think this were already solved. I think it should be fixed, otherwise it will lead to funny confusions if it's changed sometime later.

>+  TEST_PRINTF ("%a", "Sat");

I would personally prefer an array and a loop instead of the macros, so you define 'format' and 'expected' cleanly in a list. Might be somewhat matter of taste.
That said, I like the extensive test coverage a lot.

>+static gpointer
>+gdatetime_copy (gpointer boxed)

Looks redundant to me, the signature is virtually identical.
Comment 22 Christian Dywan 2010-05-19 14:46:36 UTC
> + * g_date_time_get_julian:
> [...]
> + * Retrieves the julian period, day, hour, mintute, and second which @datetime

Typo.

> +g_date_time_get_dmy (GDateTime *datetime, /* IN */

Thought: what about making this _get_gregorian as an analogue to _get_julian? We could then also have _get_persian and _get_chinese.

Similarly, g_date_time_add_full could be called g_date_time_add_gregorian if we want that kind of naming. While I think it is reasonable to default to Gregorian measurement, it might still be helpful to reflect that in the API if all it takes is a few letters.

> +g_date_time_get_day_of_week (GDateTime *datetime) /* IN */
> [...]
> +  /* See Calendar FAQ Section 2.6 for algorithm information */

An URL would be a good idea here.

+gboolean
+g_date_time_equal (gconstpointer dt1, /* IN */
+                   gconstpointer dt2) /* IN */
+{
+  const GDateTime *a, *b;
+
+  a = dt1;
+  b = dt2;
+
+  /* TODO: Check timezone offset */
+
+  return ((a->period == b->period) &&
+          (a->julian == b->julian) &&
+          (a->usec == b->usec));
+}

A TODO that looks like it should really be addressed. Differing timezones shouldn't be neglected.

> +void
> +g_date_time_diff (GDateTime *begin,    /* IN */

The name _diff is awkward. How about calling it _difference or _span?

> + * information, a best attempt is made to calculate the difference.

Wording: "an attempt"? My non-native-English ears expect "the best" or "an attempt".

> +  if (begin->period != 0 || end->period != 0)
> +    {
> +      g_warning ("%s only supports current julian epoch", G_STRFUNC);

Missing localization. And should be "%s only supports the current julian epoch".

> +GDateTime*
> +g_date_time_date (GDateTime *datetime) /* IN */

I think the name here is slightly confusing. How about "_midnight" or "_day"?

> > +GDateTime*
> > +g_date_time_parse (const gchar *input) /* IN */
> > +{
> > +  /* TODO: Implement parsing with locale support */
> > +  g_warn_if_reached ();
> > +  return NULL;
> > +}
> >
> This seems odd. Isn't GDateTime supposed to be immutable?

Nevermind that, correcting myself, I somehow mixed this up. There is no mutilation involved.

However I'm unsure what I expect that function to do. Does it guess if, say, American locale uses "military" or "john doe" time format? Where would it be used?
Comment 23 Dan Winship 2010-05-19 15:04:26 UTC
(In reply to comment #22)
> Thought: what about making this _get_gregorian as an analogue to _get_julian?
> We could then also have _get_persian and _get_chinese.

get_julian doesn't get the date in the Julian calendar, it gets the Julian day, which is an entirely unrelated concept invented by someone who just wanted to confuse you.

There is another bug (bug 344005) about adding an API for non-Gregorian calendars.
Comment 24 Thiago Sousa Santos 2010-05-24 12:03:43 UTC
Created attachment 161855 [details] [review]
Adds gdatetime type to glib

Updated patch from above's comments.

The important part here is that the timezone handling was rewriten
to use Olson's database (would this work on windows? how?)

Also, one question I have is what to do when we use one of _add_*
functions and the resulting datetime changes timezone (moving in or out
of DST), do we need to recheck after every _add_* if we need to replace
the timezone with a new one?
Comment 25 Christian Dywan 2010-05-25 13:42:26 UTC
Taking notes for the important changes, in case anyone else finds it helpful to avoid re-reading the whole patch:

> g_date_time_format_for_display and g_date_time_parse were removed.
> g_date_time_equal was implemented.
> g_date_time_diff -> g_date_time_difference
> g_date_time_date -> g_date_time_day

> +g_date_time_new_full (gint year,   /* IN */
Now takes an Olson's database timezone name string. Unit tests were added/ updated.


> + * g_date_time_parse_with_format:

Still *not* fully covering the range of strptime including %a and %b. Still open question, should it take a locale argument? Should we consider omitting this function *for now*?

> +  /* FIXME we use gint64, we shold expand these limits */
>+  if (year < 1970)
>+    return 0;
>+  else if (year > 2037)
>+    return G_MAXINT64;

Still needs to be addressed.
Comment 26 Thiago Sousa Santos 2010-05-25 13:53:08 UTC
(In reply to comment #25)
> Taking notes for the important changes, in case anyone else finds it helpful to
> avoid re-reading the whole patch:
> 
> > g_date_time_format_for_display and g_date_time_parse were removed.
> > g_date_time_equal was implemented.
> > g_date_time_diff -> g_date_time_difference
> > g_date_time_date -> g_date_time_day
> 
> > +g_date_time_new_full (gint year,   /* IN */
> Now takes an Olson's database timezone name string. Unit tests were added/
> updated.

Added basic ones, more should de added.

> 
> 
> > + * g_date_time_parse_with_format:
> 
> Still *not* fully covering the range of strptime including %a and %b. Still
> open question, should it take a locale argument? Should we consider omitting
> this function *for now*?

I didn't know exactly how to write the algorithm to consider the week day considering we might have the day of the month or of the year.

I agree on dropping it for now.

> 
> > +  /* FIXME we use gint64, we shold expand these limits */
> >+  if (year < 1970)
> >+    return 0;
> >+  else if (year > 2037)
> >+    return G_MAXINT64;
> 
> Still needs to be addressed.

I'll update this in future patches once we solve the problems I pointed above about windows and _add* functions.
Comment 27 Christian Dywan 2010-05-25 14:00:35 UTC
> +static gboolean
> +parse_tzdata (const gchar *tzname, gint64 start, gboolean is_utc,
> +	      gboolean *_is_dst, gint64 *_offset, gchar ** _name)
> +g_time_zone_new_from_epoch (const gchar* tzname, gint64 epoch, gboolean is_utc)

Curly brackets are misplaced in these functions.

> +g_time_zone_new_from_epoch (const gchar* tzname, gint64 epoch, gboolean is_utc)
> [...]
> +  } else {
> +    /* TODO error report */
> +  }

Should probably emit a g_warning () if tzname != NULL so users/ administrators see why timezones are off.

+g_date_time_new_full (gint year,   /* IN */

I think this should fail if the timezone is invalid, ie:

    if (tzname != NULL && dt->tz == NULL) return NULL
Comment 28 Jukka-Pekka Iivonen 2010-05-28 07:46:15 UTC
Created attachment 162172 [details]
DateTime vapi file for Vala

Thanks for the improvements! Great work! I just created Vala vapi file (attached) to check how this works from Vala. Just a few comments:

* g_date_time_now, g_date_time_today and g_date_time_utc_now could be renamed as g_date_time_new_now, g_date_time_new_today and g_date_time_new_utc_now to simplify binding. (a minor issue)
* Bug: There is a bug in weekdays_abbr[], replace 'Thur' with 'Thu'.
* Some date types differ (your patch uses gint) from GLib's GDate ones. E.g. GDateMonth, GDateWeekday etc. See g_date_time_get_day_of_week, g_date_time_get_month etc. Maybe we should be consistent?
* g_date_time_printf is missing a few format options like '%c', see 'man date'.
* There should be a way to convert a date to another timezone, other than utc or local. Maybe g_date_time_set_timezone (GDateTime *dt, gchar *tzone)?
* g_date_time_add should not take in a pointer. It makes the api hard to use from Vala. See below:

	// Rather complicated compared to just 'dt.add (TimeSpan.HOUR)'?
	var ts = TimeSpan.HOUR;
	dt = dt.add (ref ts);
	print ("%s\n", dt.printf ("%a %b %d %X %z %Y"));
Comment 29 Jukka-Pekka Iivonen 2010-05-28 07:49:17 UTC
Created attachment 162173 [details]
New datetime.vapi with correct copyright.

Fixed the copyright that got copied by misstake.
Comment 30 Thiago Sousa Santos 2010-05-28 11:35:42 UTC
Created attachment 162209 [details] [review]
Updated gdatetime patch

Updated patch with recent comments

Summary of what changed:
* Dropped _parse_with_format
* Added a g_warning for when reading tzdata fails
* Renamed _day, _now, _utc_now with _new_ prefix
* Removed typo in weekdays_abbr
* Do not use GTimeSpan as a pointer when it is a input argument in g_date_time_add

Open questions:
* How should this work on Windows?
* What should be the limits in _to_epoch? Billions of years ahead? Can we still use mktime in this case?
* Didn't understand the problem with the curly brackets in parse_tzdata and g_time_zone_new_from_epoch
* We can't use GDateYear, why should we use GDateMonth/GDateDay? (Not that I really care)
* How do we get locale printed time to add %c to _printf function?
* What would be a better name for g_date_time_set_timezone? GDateTime is immutable, I think _set_ is a little misleading here, but I couldn't think of another name.
* What should happen when _add* functions take the GDateTime to another timezone (might happen that it falls in/out of DST times)
Comment 31 Paolo Bonzini 2010-05-31 10:52:21 UTC
Windows APIs provides information only about the current time zone.  There is some kind of database in the registry (and I think it is documented since Windows Server 2008) but I don't know how complete it is and it doesn't use Olson names.

All you can do for Windows is to ship the database with glib, I suppose.
Comment 32 Jukka-Pekka Iivonen 2010-05-31 11:15:12 UTC
Created attachment 162368 [details]
datetime.vapi updated for the latest GDateTime patch

Here is an updated datetime.vapi which uses the API in the latest patch.

>* We can't use GDateYear, why should we use GDateMonth/GDateDay? (Not that I
really care)

Maybe _get_day_of_week should still return GDateWeekday?

>* What would be a better name for g_date_time_set_timezone? GDateTime is
immutable, I think _set_ is a little misleading here, but I couldn't think of
another name.

How about GDateTime* g_date_time_new_in_timezone (GDateTime* dt, gchar *tzone)?

>* What should happen when _add* functions take the GDateTime to another
timezone (might happen that it falls in/out of DST times)

What other similar libraries do? 

Some links for competitor intelligence here:

Apple:
http://developer.apple.com/mac/library/documentation/cocoa/Conceptual/DatesAndTimes/Articles/dtDates.html

Windows:
http://msdn.microsoft.com/en-us/library/system.datetime_members%28v=VS.71%29.aspx

Android:
http://developer.android.com/reference/java/util/Calendar.html

Perl:
http://search.cpan.org/dist/DateTime/lib/DateTime.pm

Python:
http://docs.python.org/library/datetime.html
Comment 33 Christian Dywan 2010-06-01 09:37:56 UTC
> * Didn't understand the problem with the curly brackets in parse_tzdata and
> g_time_zone_new_from_epoch

To explain by example, the code is formatted like this:

+  if (!tzname) {
+    tzname = "localtime";
+  }

but should be like this to match GLib style:

+  if (!tzname)
+    {
+      tzname = "localtime";
+    }

> > * What would be a better name for g_date_time_set_timezone? GDateTime is
> > immutable, I think _set_ is a little misleading here, but I couldn't think of
> > another name.
> How about GDateTime* g_date_time_new_in_timezone (GDateTime* dt, gchar *tzone)?

How about replacing g_date_time_to_local and g_date_time_to_utc with g_date_time_to_timezone and g_date_time_new_utc_now with g_date_time_new_in_timezone? On top of that, we could add G_TIMEZONE_UTC and G_TIMEZONE_LOCAL.
Comment 34 Christian Dywan 2010-06-01 11:23:25 UTC
> > * What should happen when _add* functions take the GDateTime to another
> > timezone (might happen that it falls in/out of DST times)
> What other similar libraries do? 

Thanks for the references. I read through them now to compare roughly how they work:

Apparently Python has a notion of "timezone awareness", which translates to lack of built-in timezone support. An unaware datetime ignores timezone, daylight saving and any other special cases. A "tzinfo" class has to be written by another module or the application itself.

Perl has a DateTime resembling GDateTime semantics, including timezone names. Days and months are 1-based and have separate 0-based functions.
A "Locale" parameter allows overriding the locale. While I think having local versus non-local is useful, I doubt converting from random locales is common, and systems are unlikely to provide them.
A DateTime is by default "floating" which means it is not timezone-aware unless explicitly specified. As they clearly state that any calculation with this is undefined, I don't see any benefit over UTC.
Perl partially supports CLDR. I don't think we want that.

Android/ Java has Calendar objects. A "lenient GregorianCalendar interprets MONTH == JANUARY, DAY_OF_MONTH == 32 as February 1", while non-lenient requires valid values.
A Calendar can be created without passing all values, which makes it "insufficient". Depending on the choice of calendar system it assumes defaults such as year 1970, month 1 and day 1. Not relevant for GDateTime as constructors are clearly defined. "Inconsistent" state is similarly possible because a Calendar is mutable and can lead to ambiguous values.
There is a 13th month UNDECIMBER, notably for lunar calendars.

Pondering after reading the above:

What about a way to obtain a timezone name from an offset such as "+0630"?

What happens with invalid dates, such as "time gaps" between DST and non-DST? Do we attempt to return a date in such a case or do we yield an error/ NULL?

Should days and months be documented to match #GDateDay and #GDateMonth?
Comment 35 Christian Dywan 2010-06-01 12:20:06 UTC
.NET notably has no %c format. It has a CultureInfo and GetDateTimeFormats interface, comparable to Locale in Perl and can be used to parse time strings.
Curiously there is a concept of "file times" based on 1601-01-01 12:00 UTC.
The construcor is for example DateTime(1979, 7, 28, 5, 23, 15, 16), which is hard to read.
A confusing aspect of .NET DateTime is that timezone is implicit, and for example ToLocalTime() converts "assuming" that the date is UTC, and otherwise basically yeilds undefined results.
^^ I have to admit, I had a very hard time with the MSDN doc layout. I failed for instance to find out how "Substract" and "Addition" affect the timezone. If it was there, my apologies.
Comment 36 Jukka-Pekka Iivonen 2010-06-01 12:33:28 UTC
Just for completeness these two simple functions seems to be missing but exists in some of the competitor's implementations:

* gint g_date_time_get_quarter (GDateTime *dt); // Returning 1-4
* gint g_date_time_get_day_of_quarter (GDateTime *dt); // Returning 1-92
Comment 37 Christian Dywan 2010-06-01 12:45:00 UTC
Cocoa has a NSDate and NSTimeZone, which has a notion of systemTimeZone, which is cached and resetSystemTimeZone to clear the cache in case the timezone changes.
There is knownTimeZoneNames and other ways to list and lookup time zones. It is not stated how the names of time zones are specified.
The NSDateComponents interface consists of days, month, etc. like GDateTime but is oblivious of locale by itself, without NSDate.
It is possible to have "out of bounds" dates similar to "inconsistent" dates on Android. Calculations are always "lenient".

Does or should GDateTime cache the local timezone?

Should we have a way to list all time zones?
Comment 38 Behdad Esfahbod 2010-06-02 21:56:40 UTC
Do we really need to have bitfields in there?
Comment 39 Christian Hergert 2010-06-02 22:07:22 UTC
That is really just legacy from when I was trying to fit everything in 64 bits.  Once I added initial timezone support that was no longer an option.  

I'm not particularly tied to the bit-fields anymore now that we are an opaque type.  However, since it works, I don't see anything wrong with keeping the allocation overhead small.
Comment 40 Thiago Sousa Santos 2010-06-11 10:58:16 UTC
So there are lots of questions open here and its likely that answering all this will be hard enough already.

I suggest we try a minimalistic approach to easy things out, taking each step slowly.

We could start with a simple minimalist API + a opaque struct.

I'll try making a patch later today. Meanwhile I accept suggestions on what should be on the minimalist API:

* Ref/unref/new function (obviously)
* No adding function
* No timezone databases, plain simple offsets from gmt passed as arguments
* get/set functions for hour/minute/seconds...
Comment 41 Thiago Sousa Santos 2010-06-14 19:18:51 UTC
Created attachment 163621 [details] [review]
Adds gdatetime type to glib

It seems it is quite hard to have a minimal API using only plain C.

This patch:
* removes the 'add' functions from previous one.
* adds a configure check and option to verify zoneinfo database

Seems the tzdata database (olson's database) is widely used:
http://en.wikipedia.org/wiki/Tz_database#Use_in_software_systems

The main question is still how to make it work for windows? Use
native API with ifdefs, ship it with glib or let the user
pass --with-zoneinfo-dir="path" when configuring?

With this solved I guess we have a minimal working solution we
can improve step by step.
Comment 42 Thiago Sousa Santos 2010-06-14 19:40:40 UTC
If someone has an idea of minimal API or a solution to avoid having tzdata database dependency. Probably this would limit support to UTC and system's timezones. But I'd consider this a good start for gdatetime.
Comment 43 Christian Dywan 2010-06-15 09:48:39 UTC
If timezone data were optional, code could be written on one system and fail elsewhere due to unknown timezones. So that the minimum it would have to be very clear to expect and handle that case. For instance (assuming we would parse "local" and "UTC" in parse_tzdata):

> g_date_time_new_full
> ...
> Available timezone names depend on the system, so values other than "local" and "UTC" can fail, in which case %NULL is returned.
>
> Return value: the newly created #GDateTime or %NULL if @timezone is not known


Another option I see, perhaps in combination with the above, would be to teach parse_tzdata about "+0630" style strings, so applications could parse timezone offsets or even lookup timezone names from elsewhere and convert them to strings.
Comment 44 Thiago Sousa Santos 2010-06-15 13:48:24 UTC
Created attachment 163679 [details] [review]
Adds gdatetime type to glib

This patch is a simplification that only supports local and utc timezones,
I guess this is a minimal API that we could focus on getting in first.

I'll be waiting for reviews.
Comment 45 Emmanuele Bassi (:ebassi) 2010-06-18 14:16:59 UTC
Review of attachment 163679 [details] [review]:

::: glib/gdatetime.c
@@ +216,3 @@
+/**
+ * g_time_zone_new:
+ * @offset: The offset in microseconds from GMT time

ugh, that's just... ugh.

do you really expect the timezone to be defined in terms of *microseconds*? I know that it's convenient internally, but seriously: use minutes (and just because there are these odd fractionary timezones, otherwise I'd push for hours).

@@ +227,3 @@
+GTimeZone *
+g_time_zone_new (gint64 offset, gboolean is_dst, const gchar * name)
+{

break the arguments into separate lines

@@ +1097,3 @@
+                      gint second, /* IN */
+                      gint microsecond, /* IN */
+                      GTimeZone *timezone) /* IN */

please, remove these comments on the direction of the arguments: they just make the code hard to read. we have gtk-doc annotations - but we always assume "in" as the default direction

::: glib/gdatetime.h
@@ +17,3 @@
+ *
+ * Copyright (C) 2009-2010 Christian Hergert <chris@dronelabs.com>
+/* gdatetime.h

missing single include header guard

@@ +39,3 @@
+GTimeZone *   g_time_zone_new                    (gint64 offset,
+                                                  gboolean is_dst,
+                                                  const gchar *name);

why a gint64 offset? is it in milliseconds - because that would be kind of odd. it would stil be odd in seconds. unless you expect this kind of function call:

  g_time_zone_new (G_TIME_SPAN_HOUR * 2.5, ...)

which to me seems very clunky. I'd prefer a:

  g_time_zone_new (gint offset_from_Zulu_in_minutes,...)

also, why is the name is needed?

@@ +45,3 @@
+gint          g_date_time_compare                (gconstpointer   dt1,
+                                                  gconstpointer   dt2);
+GDateTime *   g_date_time_copy                   (GDateTime      *datetime);

since you have a _copy() it might probably be good to have a _free() as well

@@ +107,3 @@
+GDateTime *   g_date_time_to_utc                 (GDateTime      *datetime);
+GDateTime *   g_date_time_new_today                  (void);
+void          g_date_time_unref                  (GDateTime      *datetime);

you should put g_date_time_unref() near ref().

@@ +109,3 @@
+void          g_date_time_unref                  (GDateTime      *datetime);
+GDateTime *   g_date_time_new_utc_now                (void);
+

new_now, new_utc_now
Comment 46 Thiago Sousa Santos 2010-06-18 19:05:25 UTC
Created attachment 164034 [details] [review]
Adds gdatetime type to glib

Updated with reviews
Comment 47 Thiago Sousa Santos 2010-06-18 19:10:12 UTC
(In reply to comment #45)
> Review of attachment 163679 [details] [review]:
> 
> ::: glib/gdatetime.c
> @@ +216,3 @@
> +/**
> + * g_time_zone_new:
> + * @offset: The offset in microseconds from GMT time
> 
> ugh, that's just... ugh.
> 
> do you really expect the timezone to be defined in terms of *microseconds*? I
> know that it's convenient internally, but seriously: use minutes (and just
> because there are these odd fractionary timezones, otherwise I'd push for
> hours).

Agreed, used minutes.

> 
> @@ +227,3 @@
> +GTimeZone *
> +g_time_zone_new (gint64 offset, gboolean is_dst, const gchar * name)
> +{
> 
> break the arguments into separate lines

Done.

> 
> @@ +1097,3 @@
> +                      gint second, /* IN */
> +                      gint microsecond, /* IN */
> +                      GTimeZone *timezone) /* IN */
> 
> please, remove these comments on the direction of the arguments: they just make
> the code hard to read. we have gtk-doc annotations - but we always assume "in"
> as the default direction

Done.

> 
> ::: glib/gdatetime.h
> @@ +17,3 @@
> + *
> + * Copyright (C) 2009-2010 Christian Hergert <chris@dronelabs.com>
> +/* gdatetime.h
> 
> missing single include header guard

Added.

> 
> @@ +39,3 @@
> +GTimeZone *   g_time_zone_new                    (gint64 offset,
> +                                                  gboolean is_dst,
> +                                                  const gchar *name);
> 
> why a gint64 offset? is it in milliseconds - because that would be kind of odd.
> it would stil be odd in seconds. unless you expect this kind of function call:
> 
>   g_time_zone_new (G_TIME_SPAN_HOUR * 2.5, ...)
> 
> which to me seems very clunky. I'd prefer a:
> 
>   g_time_zone_new (gint offset_from_Zulu_in_minutes,...)
> 
> also, why is the name is needed?

Changed to gint in seconds.
The name is used in 'printf' like functions, to print the abbreviated timezone name, like UTC, BRT...

> 
> @@ +45,3 @@
> +gint          g_date_time_compare                (gconstpointer   dt1,
> +                                                  gconstpointer   dt2);
> +GDateTime *   g_date_time_copy                   (GDateTime      *datetime);
> 
> since you have a _copy() it might probably be good to have a _free() as well

Added _ref/_unref functions to GTimeZone

> 
> @@ +107,3 @@
> +GDateTime *   g_date_time_to_utc                 (GDateTime      *datetime);
> +GDateTime *   g_date_time_new_today                  (void);
> +void          g_date_time_unref                  (GDateTime      *datetime);
> 
> you should put g_date_time_unref() near ref().

Done.

> 
> @@ +109,3 @@
> +void          g_date_time_unref                  (GDateTime      *datetime);
> +GDateTime *   g_date_time_new_utc_now                (void);
> +
> 
> new_now, new_utc_now

Done.
Comment 48 Thiago Sousa Santos 2010-06-23 18:28:05 UTC
Created attachment 164421 [details] [review]
Adds gdatetime type to glib

Another patch updated. Again this only supports local and UTC timezones.

* Adds g_time_zone_get_* functions.
* Fixes g_date_time_difference to use utc offsets

3 questions:

* Do we want this GTimeSpan? How is it useful? Shouldn't we drop it and use
  gint64, also replace the functions that have it as an output parameter with
  ones that have a default return?

* Should we use GDateTime with NULL timezone as a 'floating time'? Meaning
  it has no defined timezone? XMP and EXIF spec for example, does allow this
  and demands that the applications make no assumptions about the timezone
  of a datetime without them. Just asking before we take NULL timezones as UTC
  and are unable to undo it.

* What else is needed to get this basic API in?
Comment 49 Christian Dywan 2010-06-24 08:52:19 UTC
(In reply to comment #48)
> Created an attachment (id=164421) [details] [review]
> Adds gdatetime type to glib
>
> * Should we use GDateTime with NULL timezone as a 'floating time'? Meaning
>   it has no defined timezone? XMP and EXIF spec for example, does allow this
>   and demands that the applications make no assumptions about the timezone
>   of a datetime without them. Just asking before we take NULL timezones as UTC
>   and are unable to undo it.

Where is this mentioned in these specs, can you quote it? Perl has the concept of a floating timezone, but to me it wasn't clear what the use case is over UTC. In what situation would it behave differently?
Comment 50 Thiago Sousa Santos 2010-06-24 12:03:33 UTC
This is from the XMP spec:

"
A date-time value which is represented using a subset of ISO RFC 8601 formatting, as described in http://www.w3.org/TR/Note-datetime.html. The following formats are supported:

YYYY
YYYY-MM
YYYY-MM-DD
YYYY-MM-DDThh:mmTZD
YYYY-MM-DDThh:mm:ssTZD
YYYY-MM-DDThh:mm:ss.sTZD

YYYY = four-digit year
MM	= two-digit month (01=January)
DD	= two-digit day of month (01 through 31)
hh	= two digits of hour (00 through 23)
mm	= two digits of minute (00 through 59)
ss	= two digits of second (00 through 59)
s	= one or more digits representing a decimal fraction of a second
TZD = time zone designator (Z or +hh:mm or -hh:mm)

The time zone designator is optional in XMP. When not present, the time zone is unknown, and software should not assume anything about the missing time zone.
"

For handling this spec it would be useful for representing those datetimes without timezones, otherwise when we parse those dates we would handle them if they were in UTC, which might be a problem when we write them again to XMP, the value would be different.
Comment 51 Dan Winship 2010-06-24 13:33:58 UTC
(In reply to comment #49)
> Where is this mentioned in these specs, can you quote it? Perl has the concept
> of a floating timezone, but to me it wasn't clear what the use case is over
> UTC. In what situation would it behave differently?

If I have a timestamp of "13:22:01 UTC", and I convert it to local time, I get "09:22:01 EDT". If I have a *floating* timestamp of "13:22:01", and I convert it to local time, I get "13:22:01 EDT". And if I convert it to New Zealand time I get "13:22:01 NZST". And if I convert it to Neptune time, I get "13:22:01 ♆ST", etc.
Comment 52 Christian Dywan 2010-06-24 14:03:20 UTC
Thanks for elaborating. Especially the examples are something I would find helpful to see in the documentation. So floating means, a timezone meanginful in the context of the library. That makes sense to me to have then.
Comment 53 Thiago Sousa Santos 2010-06-28 16:17:03 UTC
Created attachment 164823 [details] [review]
Adds gdatetime type to glib

Updated gdatetime to not use GTimeSpan, as it doesn't have
a meaningful purpose. Use gint64 instead.

Also didn't add the timezoneless semantic to NULL timezones,
keep it as UTC just like it was. After some IRC discussion, there
aren't any useful situations for applications to use that.
Comment 54 Thiago Sousa Santos 2010-06-29 17:14:13 UTC
Created attachment 164908 [details] [review]
Adds gdatetime type to glib

Fixes a small bug on offset getting.
Also add to docs that the offset is in minutes.
Comment 55 Thiago Sousa Santos 2010-06-29 18:30:14 UTC
Created attachment 164916 [details] [review]
Adds gdatetime type to glib

Updated: Renaming some leftovers from G_TIME_SPAN_* to
G_DATE_TIME_*
Comment 56 Emmanuele Bassi (:ebassi) 2010-07-08 21:57:46 UTC
Review of attachment 164916 [details] [review]:

::: glib/gdatetime.c
@@ +60,3 @@
+ * #GDateTime is reference counted and should be freed using
+ * g_date_time_unref().
+ *

the documentation should also say that a GDateTime is an immutable object, that can only be created, referenced or copied but not modified; it should also reference GTimeZone.

also, it should say that this object deprecated #GDate.

@@ +75,3 @@
+#define USEC_PER_HOUR        (G_GINT64_CONSTANT (3600000000))
+#define USEC_PER_MILLISECOND (G_GINT64_CONSTANT (1000))
+#define USEC_PER_DAY         (G_GINT64_CONSTANT (86400000000))

why do we have these USEC_PER_* macros here and in the public API? we should just pick one

@@ +222,3 @@
+ * Creates a new timezone with the specified parameters, unref with
+ * #g_time_zone_unref after usage.
+ *

wrong gtk-doc annotation for the function

@@ +286,3 @@
+
+  return g_time_zone_new (gmt_offset (&tt, t) / 60, tt.tm_isdst > 0,
+      g_strdup (buffer));;

this is cute, but highly unmaintainable. please, expand it to its statements.

@@ +419,3 @@
+ *
+ * Gets the timezone of this #GDateTime, or NULL if there isn't one.
+ *

missing annotation for NULL

@@ +421,3 @@
+ *
+ * Return value: the #GTimeZone of this #GDateTime
+ */

missing Since: 2.26

@@ +435,3 @@
+ *
+ * Return value: The name of this #GTimeZone
+ */

missing Since: 2.26

@@ +470,3 @@
+ * Return value: TRUE if the timezone is in daylight saving times, FALSE
+ *               otherwise.
+ */

missing gtk-doc annotations for TRUE, FALSE and NULL

@@ +488,3 @@
+ * Return value: the newly created #GDateTime which should be freed with
+ *   g_date_time_unref().
+ */

missing Since: 2.26

@@ +567,3 @@
+ * g_date_time_day:
+ * @datetime: a #GDateTime
+ *

I'm not entirely sure this function is useful. at a minimum, the name is really obscure.

@@ +772,3 @@
+                     gint      *month,    /* OUT */
+                     gint      *year)     /* OUT */
+{

remove the OUT annotations, and use gtk-doc introspection annotations instead

@@ +831,3 @@
+                        gint      *minute,   /* OUT */
+                        gint      *second)   /* OUT */
+{

remove the OUT comment and use gtk-doc introspection annotations instead

@@ +1574,3 @@
+  else if (year > 2037)
+    return G_MAXINT64;
+

yes, we should fix this FIXME before committing the patch
Comment 57 Matthias Clasen 2010-07-09 00:30:40 UTC
Review of attachment 164916 [details] [review]:

::: glib/gdatetime.c
@@ +119,3 @@
+} G_STMT_END
+#define GET_AMPM(d,l)         (g_date_time_get_hour (d) < 12 ?              \
+                               (l ? "am" : "AM") : (l ? "pm" : "PM"))

I think these strings need to be translated as well.

@@ +123,3 @@
+#define WEEKDAY_FULL(d)       (Q_(weekdays_full [g_date_time_get_day_of_week (datetime)]))
+#define MONTH_ABBR(d)         (Q_(months_abbr [g_date_time_get_month (datetime)]))
+#define MONTH_FULL(d)         (Q_(months_full [g_date_time_get_month (datetime)]))

This is not going to work. The string literals need to be marked for extraction. Also, these string arrays cause unnecessary relocations.

@@ +125,3 @@
+#define MONTH_FULL(d)         (Q_(months_full [g_date_time_get_month (datetime)]))
+#define GET_PREFERRED_DATE(d) (g_date_time_printf ((d), Q_("GDateTime|%m/%d/%y")))
+#define GET_PREFERRED_TIME(d) (g_date_time_printf ((d), Q_("GDateTime|%H:%M:%S")))

Hiding a g_date_time_printf in these macros is a bit wierd. In particular since the macros are used inside g_data_time_printf...

@@ +158,3 @@
+  "GDateTime|Friday",
+  "GDateTime|Saturday",
+  "GDateTime|Sunday"

I can see that the abbreviations may need context, but I don't really think that the full day and month names need that.

@@ +179,3 @@
+static const gchar* months_full[] = {
+  NULL,
+  "January",

Why is January special here ?
Comment 58 Matthias Clasen 2010-07-09 00:33:04 UTC
*** Bug 69140 has been marked as a duplicate of this bug. ***
Comment 59 Tim-Philipp Müller 2010-07-09 08:30:04 UTC
I was wondering, especially if it is supposed to be a replacement for GDate as well, if it would be useful to be able to create GDateTimes with only partial data, ie. make it explicit that no time is specified (or no day or month, even). Certain functions that require this data could then either g_return_if_fail() if essential fields are not set or use fallback assumptions such at time=0:00:000 etc..

I would be interested in that functionality for tag handling in GStreamer. Currently we use GDate, but it is impossible to express that only a date or date+month was provided, so we can't differentiate between '2008' and '2008-01-01'.
Comment 60 Christian Persch 2010-07-09 10:11:19 UTC
(In reply to comment #57)
> Review of attachment 164916 [details] [review]:
> @@ +158,3 @@
> +  "GDateTime|Friday",
> +  "GDateTime|Saturday",
> +  "GDateTime|Sunday"

Also, shouldn't be using Q_() for new code; if context is required this should use C_().
Comment 61 Thiago Sousa Santos 2010-07-12 12:44:34 UTC
Created attachment 165732 [details] [review]
Adds gdatetime type to glib

Updated patch with ebassi and mclasen comments.

Thanks for the reviews.
Comment 62 Thiago Sousa Santos 2010-07-12 12:45:48 UTC
Created attachment 165734 [details] [review]
gdatetime: adds accuracy field

Adds GDateTimeAccuracy, allowing users to specify how precise
the information on a GDateTime is.
Comment 63 Dan Winship 2010-07-12 16:20:44 UTC
More review... most of this was written before Emmanuele's and Matthias's most-recent comments, so some of it may not apply to the latest revision... you can figure it out. :)


Using a days-since-X representation internally is nice for performing
certain calculations, and also if we want to support translations to
non-Gregorian calendars (bug 344005). However...

    1) The conversions between Gregorian DMY and Julian day are
       utterly opaque, and in fact, more or less magical. Eg, what
       does the "153" in the formula represent? If we must have
       magical algorithms (for efficiency), it would be nice to at
       least have pointers to where the magic is explained. (The
       algorithms seem to come from the calendaring FAQ that you
       mention at least once, but that doesn't actually explain any of
       the math either.)

       g_date_time_get_week_number() also needs to be taken out and
       shot. (Love those variable names.) The fact that it returns
       multiple values, and that none of its callers actually use it
       to compute the week number also confuses things.

    2) Julian days start at noon, not midnight, so you're misusing the
       notation (and g_datetime_get_julian thus gives answers that are
       wrong according to the formal definition of Julian day). The
       wikipedia entry for Julian day gives a few midnight-based
       variants, although there doesn't seem to be a standard name for
       "Julian day minus 0.5". (MJD has a more-recent epoch, and CJD
       is in local time rather than UTC.)

       Actually, I guess if we say that datetime->julian is just a
       Julian Day Number and isn't meant to identify the *time* at all,
       then maybe it's ok... it at least should have a comment.

       OTOH, certain Gregorian calendar computations are simplified by
       using proleptic Gregorian 0001-01-01 (or really 0000-12-31) as
       the epoch, which Wikipedia says is called "rata die". (SoupDate
       uses this.) If we care at all about representing BCE dates,
       then the Julian epoch is nicer, but... do we care? Particularly
       keeping in mind that even the Julian calendar is only accurate
       (in terms of being able to be correctly mapped to contemporary
       dates) back to 4 CE.

       OTOH, converting between any two epochs is just a single
       addition/subtraction operation, so...

       At any rate, regardless of the choice of internal representation,
       the docs should explicitly state what the range of supported
       dates is.

    3) The split into datetime->julian and datetime->period adds
       additional mathematical complication (and several methods just
       refuse to operate on dates with non-0 period) without adding
       *any* benefit. (Even if you feel it's important for
       g_date_time_get_julian() to have a period/day-within-period
       split, you don't actually need to have that split internally;
       just keep a single days-since-X value internally and split it
       into period/day-within-period in get_julian().)

    4) Several functions that should be taking advantage of the Julian
       day representation instead convert it back to DMY first, thus
       negating the advantage of having previously converted to JDN.
       Eg, assuming I've got my math right, all
       g_date_time_get_day_of_week() needs to do is:

           int
	   g_date_time_get_day_of_week (GDateTime *datetime)
	   {
	     return ((datetime->julian + 1) % 7) + 1;
	   }

       (ignoring the noon-vs-midnight issue). But instead, the current
       code converts the Julian day back into Gregorian DMY and then
       more-or-less reconverts it back to Julian day again. Likewise,
       g_datetime_to_epoch() just needs to subtract the Julian day
       number of the Unix epoch from datetime->julian, and then
       convert from days and microseconds to seconds, rather than
       doing everything it does now.



GTimeZone still needs work. In particular, the fact that it treats,
eg, Eastern Daylight Time and Eastern Standard Time as two separate
time zones is a problem (and doesn't match either the Olsen tzdb
definition of time zone OR the "everyday" definition of time zone).
Eg, if you create a GDateTime corresponding to "Sun Jul 4 12:32:29 EDT
2010", and then g_date_time_add() 6 months to it, it's still going to
claim to be "EDT" rather than "EST".

I'm not sure what the best way to do a "simple" timezone API is.



The API seems somewhat scattered and random. What is the use case of
g_date_time_is_daylight_savings, for instance?
g_date_time_is_leap_year() seems like it would only be needed by
calendar-drawing apps, but they would also need a (public)
g_date_time_get_week_number() function. (But really, on the subject of
drawing calendars, see the discussion in bug 344005; you need a much
more explicit what-months-are-there-and-what-days-do-they-have API if
you want to support non-Julio-Gregorian calendars.)

In one of the comments above I mentioned creating a date and then
adding 6 months to it, but GDateTime doesn't actually have any API for
doing that; you can only add microseconds, and there's no way to get
from that to adding months, because not all months have the same
number of microseconds. (GDate, OTOH, has methods for doing arithmetic
with days, months, and years.)

You should look at what methods exist in the current GDate API, and
how they're used, and at what APIs are recommended in the
non-Gregorian calendar bug.



"Epoch" does not mean specifically 1970-01-01. g_date_time_to_epoch
and g_date_time_new_from_epoch should be renamed to something like
to/from_unix_epoch or to/from_unix or to/from_time_t, etc.

I think it would also be good to not use "julian" in the name of the
function that returns the Julian day number too, since already in this
bug people have made the Julian day vs Julian calendar mistake. Maybe
"g_date_time_get_jdn" (Julian Day Number). There should be a JDN-based
constructor too.

Also, the to/from time_t and to/from Julian day methods should have
parallel naming.

I think that we really do want to squash this bug together with the
non-Gregorian calendar bug as well; our naming choices are already
being limited by the fact that "GDate" and "GTime" are taken; we don't
want to make a further name grab for this bug and then discover that
we need yet another rename/deprecation to be able to get the
international stuff to work right. The comments/API suggestions
currently in bug 344005 may be useful, or they may need to be updated
to match this API more. (It would be nice if converting a GDateTime
to/from a Gregorian/Persian/Hebrew/etc calendar date used a similar
API to converting to/from a time_t value or a Julian day.)


Don't use const return values, except for "const char *". Eg, the way
the API is now, it's not possible to g_timezone_ref() the return value
of g_date_time_get_time_zone().

There are lots of indentation problems with function prototypes and
declarations. (Also, with the addition of gdatetime.h in the Makefile;
Makefiles should always use tabs for indentation, not spaces.)

The day/month name strings (weekdays_abbr, etc) need to be wrapped
with N_().

The use of bitfields in GDateTime will slow things down, and not
really improve memory usage all that much.

Several methods need to be clearer about the range/type of
inputs/outputs. Eg, g_date_time_add() needs to specify that @timespan
is in microseconds, and likewise with the return value of
g_date_time_difference(). g_date_time_get_day_of_week() needs to
specify that it returns ISO week day numbers, not 0-based ones like
libc uses.

I'm not sure ADD_SEC/ADD_USEC/TO_JULIAN/etc really need to be macros
rather than inline functions.

Since gmt_offset() is only used for the local timezone, on most
systems that don't have tm_gmtoff, you can use "extern int timezone"
rather than fiddling with mktime. IIRC, the sign of timezone is the
opposite of the sign of tm_gmtoff, but you can google to find code
examples...

Also, for all the libc time methods, you should check if the _r
variants (gmtime_r, etc) are available, and use them instead of the
non-_r ones if so, because the non-_r ones may be unsafe to use from
multiple threads at once.

As mentioned earlier, it should be possible to compute
g_date_time_to_epoch() directly (not via DMY) but the code there also
needs to take datetime->tz into account (or else document that it
doesn't).
Comment 64 Thiago Sousa Santos 2010-07-16 11:35:51 UTC
Created attachment 166016 [details] [review]
Adds gdatetime with gregorian proleptic internal representation

This new patch reworks the gdatetime internals to use the gregorian proleptic calendar, as suggested by Dan. And the math really got simpler.

Also improved the docs a little and squashed together the accuracy patch.

It should also be noted that there are no public functions for mathematical manipulations of dates, as this requires some decisions on how timezones would work when you add/subtract dates and fall in/out of DST, for example.
Comment 65 Thiago Sousa Santos 2010-07-16 11:38:43 UTC
Created attachment 166017 [details] [review]
Adds gdatetime with gregorian proleptic internal representation

This new patch reworks the gdatetime internals to use the gregorian proleptic calendar, as suggested by Dan. And the math really got simpler.

Also improved the docs a little and squashed together the accuracy patch.

It should also be noted that there are no public functions for mathematical manipulations of dates, as this requires some decisions on how timezones would work when you add/subtract dates and fall in/out of DST, for example.
Comment 66 Thiago Sousa Santos 2010-07-16 11:51:31 UTC
I'll try to reply here to the suggested changes.

First of all, thanks for the review :)

For points 1, 2, 3 and 4:
I updated the code to use gregorian proleptic internally and all the problems pointed were gone, math is simpler now, removed the get_week_number ugly function and also improved the docs a little regarding ranges of parameters.

(In reply to comment #63)
> 
> GTimeZone still needs work. In particular, the fact that it treats,
> eg, Eastern Daylight Time and Eastern Standard Time as two separate
> time zones is a problem (and doesn't match either the Olsen tzdb
> definition of time zone OR the "everyday" definition of time zone).
> Eg, if you create a GDateTime corresponding to "Sun Jul 4 12:32:29 EDT
> 2010", and then g_date_time_add() 6 months to it, it's still going to
> claim to be "EDT" rather than "EST".
> 
> I'm not sure what the best way to do a "simple" timezone API is.

Me neither, note that there are no mathematical functions in the public API, this requires us to take some decisions and I thought it would be better after we settle on the representation part. (Which is what this patch attempts to do).

Then we need to discuss how and what tz database to use, and these operations results.

> 
> 
> 
> The API seems somewhat scattered and random. What is the use case of
> g_date_time_is_daylight_savings, for instance?
> g_date_time_is_leap_year() seems like it would only be needed by
> calendar-drawing apps, but they would also need a (public)
> g_date_time_get_week_number() function. (But really, on the subject of
> drawing calendars, see the discussion in bug 344005; you need a much
> more explicit what-months-are-there-and-what-days-do-they-have API if
> you want to support non-Julio-Gregorian calendars.)

I don't have a strong opinion on keeping/dropping these functions, but I think the design pointed
by Christian Hergert is a good option. Just as a reminder: keep a GDateTime as a basic date+time+timezone representation and have calendar objects built on top of it. GDateTime could provide convenience functions for conversions that would help implementing those calendars.

> 
> In one of the comments above I mentioned creating a date and then
> adding 6 months to it, but GDateTime doesn't actually have any API for
> doing that; you can only add microseconds, and there's no way to get
> from that to adding months, because not all months have the same
> number of microseconds. (GDate, OTOH, has methods for doing arithmetic
> with days, months, and years.)
> 
> You should look at what methods exist in the current GDate API, and
> how they're used, and at what APIs are recommended in the
> non-Gregorian calendar bug.
> 
> 
> 
> "Epoch" does not mean specifically 1970-01-01. g_date_time_to_epoch
> and g_date_time_new_from_epoch should be renamed to something like
> to/from_unix_epoch or to/from_unix or to/from_time_t, etc.

Fixed.

> 
> I think it would also be good to not use "julian" in the name of the
> function that returns the Julian day number too, since already in this
> bug people have made the Julian day vs Julian calendar mistake. Maybe
> "g_date_time_get_jdn" (Julian Day Number). There should be a JDN-based
> constructor too.

I kept only Gregorian calendar functions in the public API for now.

> 
> Also, the to/from time_t and to/from Julian day methods should have
> parallel naming.
> 
> I think that we really do want to squash this bug together with the
> non-Gregorian calendar bug as well; our naming choices are already
> being limited by the fact that "GDate" and "GTime" are taken; we don't
> want to make a further name grab for this bug and then discover that
> we need yet another rename/deprecation to be able to get the
> international stuff to work right. The comments/API suggestions
> currently in bug 344005 may be useful, or they may need to be updated
> to match this API more. (It would be nice if converting a GDateTime
> to/from a Gregorian/Persian/Hebrew/etc calendar date used a similar
> API to converting to/from a time_t value or a Julian day.)
> 
> 
> Don't use const return values, except for "const char *". Eg, the way
> the API is now, it's not possible to g_timezone_ref() the return value
> of g_date_time_get_time_zone().

Fixed.

> 
> There are lots of indentation problems with function prototypes and
> declarations. (Also, with the addition of gdatetime.h in the Makefile;
> Makefiles should always use tabs for indentation, not spaces.)

Yeah, I know, still haven't got around to learn the glib codestyle

> 
> The day/month name strings (weekdays_abbr, etc) need to be wrapped
> with N_().

Fixed.

> 
> The use of bitfields in GDateTime will slow things down, and not
> really improve memory usage all that much.

Fixed.

> 
> Several methods need to be clearer about the range/type of
> inputs/outputs. Eg, g_date_time_add() needs to specify that @timespan
> is in microseconds, and likewise with the return value of
> g_date_time_difference(). g_date_time_get_day_of_week() needs to
> specify that it returns ISO week day numbers, not 0-based ones like
> libc uses.

Tried improving this.

> 
> I'm not sure ADD_SEC/ADD_USEC/TO_JULIAN/etc really need to be macros
> rather than inline functions.

Haven't changed this, there's only ADD_DAYS left at the current patch. Could change, no strong opinion.

> 
> Since gmt_offset() is only used for the local timezone, on most
> systems that don't have tm_gmtoff, you can use "extern int timezone"
> rather than fiddling with mktime. IIRC, the sign of timezone is the
> opposite of the sign of tm_gmtoff, but you can google to find code
> examples...

Yes, I checked some code online, but could not find a way to check if this variable is available or not.

> 
> Also, for all the libc time methods, you should check if the _r
> variants (gmtime_r, etc) are available, and use them instead of the
> non-_r ones if so, because the non-_r ones may be unsafe to use from
> multiple threads at once.

Fixed.

> 
> As mentioned earlier, it should be possible to compute
> g_date_time_to_epoch() directly (not via DMY) but the code there also
> needs to take datetime->tz into account (or else document that it
> doesn't).

Fixed.
Comment 67 Christian Dywan 2010-07-16 14:53:44 UTC
> > Since gmt_offset() is only used for the local timezone, on most
> > systems that don't have tm_gmtoff, you can use "extern int timezone"
> > rather than fiddling with mktime. IIRC, the sign of timezone is the
> > opposite of the sign of tm_gmtoff, but you can google to find code
> > examples...
> 
> Yes, I checked some code online, but could not find a way to check if this
> variable is available or not.

Look for AC_RUN_IFELSE as used already in configure.in. You can run a C snippet that tries to use the symbol.
Comment 68 Christian Dywan 2010-07-16 15:10:26 UTC
Review of attachment 166017 [details] [review]:

::: glib/gdatetime.c
@@ +2004,3 @@
+#define __G_DATE_TIME_C__
+#include "galiasdef.c"
+

That can be removed now, since the aliasing is gone in git master.
Comment 69 Javier Jardón (IRC: jjardon) 2010-07-20 12:26:07 UTC
Review of attachment 166017 [details] [review]:

::: glib/gdatetime.c
@@ +45,3 @@
+#include "gdatetime.h"
+
+#include "galias.h"

Also, this can be removed too, since the aliasing is gone.
Comment 70 Matthias Clasen 2010-08-01 04:40:30 UTC
The latest version of the patch seems to have lost the i18n-related fixes from comment 61 again.
Comment 71 Emmanuele Bassi (:ebassi) 2010-08-24 17:28:00 UTC
Created attachment 168658 [details] [review]
Add GDateTime to GLib

GDateTime is an opaque data type containing a date and time
representation. It's immutable once created and reference
counted.

https://bugzilla.gnome.org/show_bug.cgi?id=50076

Based on the code by: Christian Hergert <chris@dronelabs.com>

Signed-off-by: Emmanuele Bassi <ebassi@linux.intel.com>
Comment 72 Emmanuele Bassi (:ebassi) 2010-08-24 17:28:19 UTC
Created attachment 168659 [details] [review]
datetime: Add GDateTime to the GType system

Similarly to other GLib data structures, use a GBoxed type.
Comment 73 Emmanuele Bassi (:ebassi) 2010-08-24 17:36:57 UTC
here's another take for this patch.

attachment 168658 [details] [review] contains an updated version of attachment 166017 [details] [review] with the i18n changes of comment 61 back in place.

further changes:

  • fixed the aliasing stuff as per comment 68 and comment 69
  • constification of the accessors and modifiers
  • clean up of the coding style (and removal of the out of place annotation for the arguments direction)
  • fix the retrieval of the timezone info file, to accomodate the TZDIR environment variable and the fact that Fedora places "localtime" under /etc without a symbolic link.
  • removal of the test units for ref/unref, as they relied on direct access to the opaque GDateTime data structure and other nasty things.

I also split the commit into two, one for GLib and another for GObject. the documentation commit is missing since I'm not sure where we should place this: under the same page as GDate and GTimeVal or under a new section? using a new section would avoid making the "Date and Time" section grow far too much.

I need to re-read the various comments because I'm sure I missed some remark somewhere.
Comment 74 Matthias Clasen 2010-08-24 18:47:04 UTC
Review of attachment 168658 [details] [review]:

Did we want to deprecate GDate at the same time ?

::: glib/gdatetime.c
@@ +58,3 @@
+#define C_(Context,String) g_dpgettext (NULL, Context "\004" String, strlen (Context) + 1)
+#endif
+

This should probably go if we include it, since we know that C_ will be defined if glibintl.h is included.

@@ +124,3 @@
+#define GET_AMPM(d,l)         ((g_date_time_get_hour (d) < 12)  \
+                                ? (l ? _("am") : _("AM"))       \
+                                : (l ? _("pm") : _("PM")))

These definitively need a translator comment, or better, context. "AM" by itself is pretty hard to translate...

@@ +180,3 @@
+      return _("November");
+    case 12:
+      return _("December");

If we use context for the abbreviations below, we should probably use context here as well, for consistency.

@@ +230,3 @@
+    {
+    case 1:
+      return _("Monday");

here too

@@ +350,3 @@
+  if (tzname != NULL)
+    {
+      const gchar *tz_dir = g_getenv ("TZDIR");

we should add TZDIR to the "Environment variables" section in the docs, in running.sgml

@@ +468,3 @@
+  gboolean is_dst;
+  gchar *name = NULL;
+

do we need some ifdef G_OS_UNIX here ? parse_tzdata looks damn unix-specific
Comment 75 Matthias Clasen 2010-08-24 18:51:20 UTC
Review of attachment 168658 [details] [review]:

Looks like there is no way to get at the timezone info ?
Comment 76 Matthias Clasen 2010-08-24 18:54:04 UTC
Review of attachment 168659 [details] [review]:

This one looks fine, obviously (not much to do wrong here...)
Comment 77 Matthias Clasen 2010-08-24 18:55:15 UTC
Review of attachment 168658 [details] [review]:

Once concern here is that we're adding quite a few strings very late in the game. I guess we'd need to reassure translators that these are only used in g_datetime_printf, which is new, and thus having them not 100% translated will not do much harm in 2.32.
Comment 78 Emmanuele Bassi (:ebassi) 2010-08-24 20:13:08 UTC
Review of attachment 168658 [details] [review]:

::: glib/gdatetime.c
@@ +58,3 @@
+#define C_(Context,String) g_dpgettext (NULL, Context "\004" String, strlen (Context) + 1)
+#endif
+

actually, no: I checked glibintl.h and C_ is not defined there. should I add it?

@@ +124,3 @@
+#define GET_AMPM(d,l)         ((g_date_time_get_hour (d) < 12)  \
+                                ? (l ? _("am") : _("AM"))       \
+                                : (l ? _("pm") : _("PM")))

I agree, I'll add it ASAP.

@@ +180,3 @@
+      return _("November");
+    case 12:
+      return _("December");

agree.

@@ +350,3 @@
+  if (tzname != NULL)
+    {
+      const gchar *tz_dir = g_getenv ("TZDIR");

agree.

@@ +468,3 @@
+  gboolean is_dst;
+  gchar *name = NULL;
+

win32 does not have it; we can probably have tzdata shipped with the win32 binaries - and that mostly means abstracting the "get me the zoneinfo file path" into something OS-specific.
Comment 79 Emmanuele Bassi (:ebassi) 2010-08-24 20:14:14 UTC
(In reply to comment #77)
> Review of attachment 168658 [details] [review]:
> 
> Once concern here is that we're adding quite a few strings very late in the
> game. I guess we'd need to reassure translators that these are only used in
> g_datetime_printf, which is new, and thus having them not 100% translated will
> not do much harm in 2.32.

sounds fair enough to me.
Comment 80 Emmanuele Bassi (:ebassi) 2010-08-24 20:27:03 UTC
(In reply to comment #78)

> win32 does not have it; we can probably have tzdata shipped with the win32
> binaries - and that mostly means abstracting the "get me the zoneinfo file
> path" into something OS-specific.

alternatively, the win32 code could always define TZDIR to the right place.
Comment 81 Matthias Clasen 2010-08-24 20:50:50 UTC
(In reply to comment #78)
> Review of attachment 168658 [details] [review]:
> 
> ::: glib/gdatetime.c
> @@ +58,3 @@
> +#define C_(Context,String) g_dpgettext (NULL, Context "\004" String, strlen
> (Context) + 1)
> +#endif
> +
> 
> actually, no: I checked glibintl.h and C_ is not defined there. should I add
> it?
> 

Yes, I think if we use context internally, glibintl.h should have a C_ definition.
Comment 82 Matthias Clasen 2010-08-24 20:51:45 UTC
(In reply to comment #80)
> (In reply to comment #78)
> 
> > win32 does not have it; we can probably have tzdata shipped with the win32
> > binaries - and that mostly means abstracting the "get me the zoneinfo file
> > path" into something OS-specific.
> 
> alternatively, the win32 code could always define TZDIR to the right place.

We should make sure that the code handles the absence of zoneinfo data in some minimally functional way, I guess.
Comment 83 Emmanuele Bassi (:ebassi) 2010-08-24 20:58:28 UTC
(In reply to comment #75)
> Review of attachment 168658 [details] [review]:
> 
> Looks like there is no way to get at the timezone info ?

I think the plan is to expose this later, and just deal with the representation of the date and time to avoid landing a massive API. right now you can get the UTC offset of the timezone, and ideally we could expose the timezone name used to construct the GDateTime by adding:

  G_CONST_RETURN gchar *
  g_date_time_get_timezone (const GDateTime *datetime)
  {
    if (datetime->tz != NULL)
      return datetime->tz->name;

    return "UTC";
  }
Comment 84 Matthias Clasen 2010-08-24 21:14:19 UTC
(In reply to comment #83)
> (In reply to comment #75)
> > Review of attachment 168658 [details] [review] [details]:
> > 
> > Looks like there is no way to get at the timezone info ?
> 
> I think the plan is to expose this later, and just deal with the representation
> of the date and time to avoid landing a massive API. right now you can get the
> UTC offset of the timezone, and ideally we could expose the timezone name used
> to construct the GDateTime by adding:
> 
>   G_CONST_RETURN gchar *
>   g_date_time_get_timezone (const GDateTime *datetime)
>   {
>     if (datetime->tz != NULL)
>       return datetime->tz->name;
> 
>     return "UTC";
>   }

Makes sense, but should maybe be called g_date_time_get_timezone_name ? 
so that we don't run into trouble if we decide to expose a timezone type later
Comment 85 Emmanuele Bassi (:ebassi) 2010-08-24 22:15:10 UTC
Created attachment 168690 [details] [review]
Add GDateTime to GLib

GDateTime is an opaque data type containing a date and time
representation. It's immutable once created and reference
counted.

https://bugzilla.gnome.org/show_bug.cgi?id=50076

Based on the code by: Christian Hergert <chris@dronelabs.com>

Signed-off-by: Emmanuele Bassi <ebassi@linux.intel.com>
Comment 86 Matthias Clasen 2010-08-24 22:55:00 UTC
Review of attachment 168690 [details] [review]:

Misses the glibintl.h addition, but I'm sure you'll add that when you commit it.
Comment 87 Emmanuele Bassi (:ebassi) 2010-08-24 23:12:27 UTC
Comment on attachment 168659 [details] [review]
datetime: Add GDateTime to the GType system

committed to master as e35ed21f43f94443e5b137d85120b87542261c5b
Comment 88 Emmanuele Bassi (:ebassi) 2010-08-24 23:12:49 UTC
Comment on attachment 168690 [details] [review]
Add GDateTime to GLib

committed to master as e1f13ee9ed38d4f14bf927b6fa3f28530afc3640
Comment 89 Emmanuele Bassi (:ebassi) 2010-08-24 23:14:48 UTC
This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.
Comment 90 Dan Winship 2010-08-27 16:12:12 UTC
Um... something bad happened here. The patch in comment 65 does not match the description there or in comment 66. And then Emmanuele based his patch on that patch, but apparently believing that it already addressed all the comments I made in comment 63 (which it doesn't, at all).
Comment 91 Dan Winship 2010-08-27 16:14:54 UTC
*** Bug 628137 has been marked as a duplicate of this bug. ***
Comment 92 Dan Winship 2010-08-27 16:15:37 UTC
see also the comments in bug 628137, although some of those may be addressed by the patch that Thiago thought he'd posted before...
Comment 93 Morten Welinder 2010-08-31 15:50:12 UTC
g_date_time_new_full will require changes in order to support timezones.

"yyyymmdd hhmmss" is not unique on the day where daylight savings go away.
There needs to be an indication of whether it is pre-change or post-change.
(This is why "struct tm" has the tm_isdst field.)
Comment 94 Christian Hergert 2010-08-31 16:32:51 UTC
Created attachment 169173 [details] [review]
Avoid using c99 reserved identifier __year
Comment 95 Christian Hergert 2010-08-31 16:34:07 UTC
Created attachment 169174 [details] [review]
Walk the format string using g_utf8_next_char() and use gunichar
Comment 96 Thiago Sousa Santos 2010-08-31 23:25:59 UTC
Created attachment 169202 [details] [review]
Adds gdatetime type to glib

I indeed attached the wrong patch here, sorry :(
Comment 97 Thiago Sousa Santos 2010-09-02 01:16:39 UTC
Created attachment 169315 [details] [review]
gdatetime: Use proleptic gregorian

Patch that changes the internal representation to use the proleptic
gregorian calendar. That was the biggest change in the missing patch.

There's probably some other minor changes. I'll try to run through
them tomorrow.
Comment 98 Emmanuele Bassi (:ebassi) 2010-09-06 10:56:54 UTC
Pushed:

  • attachment 169173 [details] [review]attachment 169174 [details] [review]attachment 169315 [details] [review]

to master.
Comment 99 Allison Karlitskaya (desrt) 2010-09-07 03:30:23 UTC
I did a read-through of GDateTime on a plane ride recently and I came up with these issues:

 - hash and equal seem like they can not safely be used as a pair with
   GHashTable since only one of them does timezone conversion; it appears that
   equality as per the equal function does not imply equal hash values

 - hashing appears quite weak.  consider the case of hashing many times from
   the same day -- it looks like they'd all have the same value.

 - no way to create a time with microseconds specified (you need to create a
   time then add microseconds) -- maybe not important.

 - why do we copy timezones internally when they are refcounted and immutable?

 - why do we have a (public) copy function for datetime at all when it is
   refcounted and immutable?

 - what does this mean?
    """
 * Since the exact precision cannot always be known due to incomplete
 * historic information, an attempt is made to calculate the difference.
    """

 - we could use a function that parses from ISO-whatever strings (the sort
   found in email headers) to GDateTime -- not sure if this is possible, given
   our concept of timezones, however.  It should definitely be possible to
   get a GDateTime in UTC, at least.

 - g_date_time_to_local() is broken in the case of converting from some
   timezone that is not local and not UTC.

 - timezone handling in general is quite broken.  consider this code:

      dt = g_date_time_new_now ();
      printf ("%s\n", g_date_time_printf (dt, "%R"));
      dt = g_date_time_add_months (dt, 6);
      printf ("%s\n", g_date_time_printf (dt, "%R"));

   this prints out the same time twice even though exactly 6 months from now
   it will actually be an hour later (due to daylight savings time).

   this strongly suggests that we should make the following changes:

     - keep the time internally in UTC

     - mmap the timezone file

     - convert between UTC and localtime as the public API is used 


some of these issues may be fixed since I did my read-through or I may be completely ignorant.  Sorry in either case. :)

One thing I am glad to see, though, is the tossing of the Julian stuff.
Comment 100 Emmanuele Bassi (:ebassi) 2010-09-14 17:22:42 UTC
(In reply to comment #99)
> I did a read-through of GDateTime on a plane ride recently and I came up with
> these issues:
> 

I generally agree with all the issues you've highlighted, and I've almost fixed them.

>  - timezone handling in general is quite broken.  consider this code:
> 
>       dt = g_date_time_new_now ();
>       printf ("%s\n", g_date_time_printf (dt, "%R"));
>       dt = g_date_time_add_months (dt, 6);
>       printf ("%s\n", g_date_time_printf (dt, "%R"));
> 
>    this prints out the same time twice even though exactly 6 months from now
>    it will actually be an hour later (due to daylight savings time).

I've slept on this, and I don't think it's correct to assume that adding 6 months to the date is going to change the time. if I add six months from now (2010-09-14, 18:18) I still expect the time to be 18:18 -- and if the DST is in effect on the given date, I'll know it by checking with g_date_time_get_is_daylight_saving().

>    this strongly suggests that we should make the following changes:
> 
>      - keep the time internally in UTC

this is true...

>      - mmap the timezone file

... and this too.

>      - convert between UTC and localtime as the public API is used 

again, true.

but the net result should be that we do not copy the time zone information from the source to the destination DateTime - but we check the time zone data for each function that creates a new DateTime from an existing one. this way, the "is_dst" information is always up to date.
Comment 101 Allison Karlitskaya (desrt) 2010-09-14 17:39:41 UTC
What is your opinion of what should happen when adding an hour to the time at 1:30am on the night of "leap forward"?  I'd argue 3:30am, but if I understand your argument correctly, you're arguing that we'd end up with the non-existent time 2:30am.  What would the daylight savings time flag be in that case?
Comment 102 Allison Karlitskaya (desrt) 2010-09-14 17:50:53 UTC
After looking at the GDateTime source, I understand what you mean by 'is_dst' (as a field member of GTimeZone) and I disagree that this should be this way.

I think a GTimeZone should refer to a physical place, at no particular point in time.  Put another way, a GTimeZone should correspond exactly to a file in the zoneinfo database.  'is_dst' would be something that we can query from a timezone, given a particular (UTC) time.  It also means that a timezone wouldn't have an 'offset' either, but one could be queried, given a specific UTC time.

That would let us freely copy GTimeZone between GDateTime instances without modification when adding intervals.
Comment 103 Dan Winship 2010-09-14 20:26:01 UTC
(In reply to comment #102)
> I think a GTimeZone should refer to a physical place, at no particular point in
> time.  Put another way, a GTimeZone should correspond exactly to a file in the
> zoneinfo database.

Except that zoneinfo timezones don't correspond to "a physical place at no particular point in time". They correspond to "a physical place from 1970 *until today*". But next year the same physical place may correspond to a different zoneinfo timezone than it does this year. This happens at least once a year somewhere. And because of the way zoneinfo works, in some cases, when a timezone splits, it's the people who *didn't* change their rules who have to switch to a new timezone name. (Eg, if New York decided to switch to GMT+5:30, then America/New_York would henceforth mean GMT+5:30, and everyone else in the Eastern Time zone would have to switch to a newly-created America/Philadelphia.)

We don't want people presenting zoneinfo timezones to users in their UIs. This may imply that we don't want to use zoneinfo timezones in GDateTime.
Comment 104 Emmanuele Bassi (:ebassi) 2010-09-14 23:29:15 UTC
(In reply to comment #103)
> (In reply to comment #102)
> > I think a GTimeZone should refer to a physical place, at no particular point in
> > time.  Put another way, a GTimeZone should correspond exactly to a file in the
> > zoneinfo database.
> 
> Except that zoneinfo timezones don't correspond to "a physical place at no
> particular point in time". They correspond to "a physical place from 1970
> *until today*".

yep, that's the main issue.

if you use zoneinfo then you *must* use the seconds from the epoch just to extract the current offset of the timezone from UTC, and whether the timezone is in DST or not.

this makes storing times in UTC particularly tricky, as you have to create a DateTime with an arbitrary timezone, get the seconds from the epoch, get the timezone information, then change the internal representation to take that into account.

> We don't want people presenting zoneinfo timezones to users in their UIs.

virtually all UIs use geo-location information, in the form of:

  • continent/city (linux, OS X)
  • coordinates on a map, and nearest city (linux, OS X, Windows)
  • offset from UTC and continent/city (Windows)

mobile phones UIs use continent/city, or just a list of cities.

> This may imply that we don't want to use zoneinfo timezones in GDateTime.

we can expose GTimeZone as a time and say:

  GTimeZone *g_time_zone_new (gint     offset_from_utc,
                              gboolean is_dst);

and let the developer care about how to get that information; we could also provide the Olson zoneinfo mapping:

  GTimeZone *g_time_zone_new_from_name (const gchar *olson_name,
                                        gint64       epoch,
                                        gboolean     epoch_is_utc);

if we want to get clever. but the fact is that the only stable, recognized and legal names for timezones are in the form of "UTC±<hh>:<mm>".
Comment 105 Allison Karlitskaya (desrt) 2010-09-15 03:13:47 UTC
(In reply to comment #103)
> Except that zoneinfo timezones don't correspond to "a physical place at no
> particular point in time". They correspond to "a physical place from 1970
> *until today*".

I don't really agree with that, for 2.5 reasons.

  1) The zoneinfo for America/Toronto (at least) goes back to 1894.

  2) It predicts the future up to 2499.

  3) (half reason) I doubt we care about to-the-hour precision for times 
     outside of this range.

For the case where the timezone information will change, I think nobody will fault us for going with what the installed information tells us will happen (which, as you say is proven wrong for at least one time zone once a year, but usually remains a true assumption for the other 1721).



I think that if we're going to use zoneinfo we should do it properly.  If not, we should go with UTC+/-[offset] and maybe even consider excluding support for a DST flag entirely (we can't get that piece of information from ISO date strings and it really confuses the math even otherwise).  We could then use some primitive method for getting that information for the current time on the current machine (libc offers a few methods for this) and then we don't need to worry about zoneinfo at all.

But really, I think having full zoneinfo support here would be very cool.
Comment 106 Allison Karlitskaya (desrt) 2010-09-15 03:15:45 UTC
One more thing worth mentioning: timezones really mess with our desire to use "midnight" as a way of expressing "just date" since midnight UTC gets dragged into the day before for half of the timezones on the planet.
Comment 107 Emmanuele Bassi (:ebassi) 2010-09-15 10:53:06 UTC
(In reply to comment #106)
> One more thing worth mentioning: timezones really mess with our desire to use
> "midnight" as a way of expressing "just date" since midnight UTC gets dragged
> into the day before for half of the timezones on the planet.

unless we just give up on normalizing the time to UTC, and instead assume that the time is always relative to the timezone. this actually simplifies *a lot* the code -- much more than I'd have expected. we can always go back to UTC because we always know the offset.
Comment 108 Emmanuele Bassi (:ebassi) 2010-09-15 13:02:08 UTC
Created attachment 170331 [details] [review]
datetime: Rework time zone support in constructors

Timezone handling is complicated. Really complicated.

In order to simplify it a little bit, we need to expose the GTimeZone
structure.

First of all, we allow creating time zone information directly from the
offset and the DST state, and then pass it to the g_date_time_new_full()
constructor. We also need to clean up the mess that is UTC-vs.-localtime
for the other constructors.

We also allow creating a GTimeZone from the Olson zoneinfo database
names; a time zone created like this will be "floating": it will just
reference the zoneinfo file - which are mmap()'ed, kept in a cache and
refcounted. Once the GTimeZone has been associated with a GDateTime, it
will be "anchored" to it: the offset will be resolved, as well as the
DST state.
Comment 109 Emmanuele Bassi (:ebassi) 2010-09-15 13:02:19 UTC
Created attachment 170332 [details] [review]
datetime: Allow setting fractionary seconds in new_full()

Otherwise we'll have to do:

  dt = g_date_time_new_full (Y, M, D, h, m, s, tz);
  tmp = g_date_time_add_usec (dt, usec);
  g_date_time_unref (dt);
  dt = tmp;

With its additional allocations.
Comment 110 Emmanuele Bassi (:ebassi) 2010-09-15 13:02:32 UTC
Created attachment 170333 [details] [review]
datetime: Update modifiers for DST changes

If a DateTime gets modified to cross the DST state from its previous
state then we want to update the DateTime to compensate for the new
offset.

In other words, if we have a DateTime defined as:

  DateTime({ y: 2009, m: 8, d: 15, hh: 3, mm: 0, tz: 'Europe/London' });

and we add six months to it, the hour must be changed to 60 minutes
behind, as the DST comes into effect.
Comment 111 Emmanuele Bassi (:ebassi) 2010-09-15 13:40:48 UTC
Created attachment 170337 [details] [review]
datetime: Rename g_date_time_printf() to g_date_time_format()

The function does not use any printf() modifiers, so using printf() is a
misnomer.

Prior art: strftime, g_ascii_formatd
Comment 112 Morten Welinder 2010-09-15 13:47:08 UTC
Review of attachment 170332 [details] [review]:

The interface will still need changes to support these two
different times:

# TZ=US/Eastern perl -e 'print scalar localtime 1289109599, "\n";'
Sun Nov  7 01:59:59 2010
# TZ=US/Eastern perl -e 'print scalar localtime 1289113199, "\n";'
Sun Nov  7 01:59:59 2010
Comment 113 Emmanuele Bassi (:ebassi) 2010-09-15 13:48:30 UTC
Created attachment 170339 [details] [review]
datetime: Fix hashing

Convert to the epoch, just like we do when checking for equality, so
that timezones are correctly handled.
Comment 114 Emmanuele Bassi (:ebassi) 2010-09-15 13:51:54 UTC
the patches in:

  attachment 170331 [details] [review]
  attachment 170332 [details] [review]
  attachment 170333 [details] [review]
  attachment 170337 [details] [review]
  attachment 170339 [details] [review]

address all the points raised by Ryan. the test suite passes - and I've added more cases to verify the changes.

a review would be very welcome.
Comment 115 Allison Karlitskaya (desrt) 2010-09-17 19:19:35 UTC
I accepted all those patches, plus added a monster patch of my own.

I think we're done here.  GDateTime in its current state is what will go into the 2.26.0 release.
Comment 116 Morten Welinder 2010-09-17 20:34:46 UTC
The basic is constructor, g_date_time_new, is still incomplete and thus not
suitable for a stable release.  See comment 112.

Try adding tests for

  a = 2010-11-07 00:30 US/Eastern
  b = 2010-11-07 02:30 US/Eastern
  verify that b-a is 3 hours

  c = a + 1h
  d = b - 1h
  verify that d-c is 1 hour

  verify that c != d

  verify that c.hour == d.hour
  Ditto for year,month,day,minute,second
Comment 117 Allison Karlitskaya (desrt) 2010-09-18 00:22:51 UTC
After considerable discussion of this point on #gtk+ we decided that probably nobody cares about that distinction.  For that reason, the normal _new() call is being kept simple.

I actually had a _full() version of the calls (that took a 'is_dst' flag) that I removed after the discussion.

You clearly are someone who cares about this, though.  Can you please open a separate bug for this topic?  There is quite some conversation we could have about the precise nature of this new API and this bug is already way too huge.