GNOME Bugzilla – Bug 50076
Time API to go with date API
Last modified: 2011-02-18 16:13:49 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.
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.
Moving GLib bugs with API keyword to the API freeze milestone
No way for 2.0
There has been a request to include the following feature enhancement to the new framework. http://bugs.gnome.org/show_bug.cgi?id=69140
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.
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
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.
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)
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)
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.
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.
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.
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)
FYI, there is some work at https://bugzilla.gnome.org/show_bug.cgi?id=344005
By "some work" I meant a probably complete solution :)
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.
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.
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.
(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).
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)
(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.
> + * 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?
(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.
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?
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.
(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.
> +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
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"));
Created attachment 162173 [details] New datetime.vapi with correct copyright. Fixed the copyright that got copied by misstake.
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)
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.
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
> * 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.
> > * 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?
.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.
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
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?
Do we really need to have bitfields in there?
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.
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...
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.
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.
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.
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.
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
Created attachment 164034 [details] [review] Adds gdatetime type to glib Updated with reviews
(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.
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?
(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?
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.
(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.
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.
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.
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.
Created attachment 164916 [details] [review] Adds gdatetime type to glib Updated: Renaming some leftovers from G_TIME_SPAN_* to G_DATE_TIME_*
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
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 ?
*** Bug 69140 has been marked as a duplicate of this bug. ***
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'.
(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_().
Created attachment 165732 [details] [review] Adds gdatetime type to glib Updated patch with ebassi and mclasen comments. Thanks for the reviews.
Created attachment 165734 [details] [review] gdatetime: adds accuracy field Adds GDateTimeAccuracy, allowing users to specify how precise the information on a GDateTime is.
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).
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.
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.
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.
> > 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.
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.
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.
The latest version of the patch seems to have lost the i18n-related fixes from comment 61 again.
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>
Created attachment 168659 [details] [review] datetime: Add GDateTime to the GType system Similarly to other GLib data structures, use a GBoxed type.
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.
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
Review of attachment 168658 [details] [review]: Looks like there is no way to get at the timezone info ?
Review of attachment 168659 [details] [review]: This one looks fine, obviously (not much to do wrong here...)
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.
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.
(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.
(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.
(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.
(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.
(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"; }
(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
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>
Review of attachment 168690 [details] [review]: Misses the glibintl.h addition, but I'm sure you'll add that when you commit it.
Comment on attachment 168659 [details] [review] datetime: Add GDateTime to the GType system committed to master as e35ed21f43f94443e5b137d85120b87542261c5b
Comment on attachment 168690 [details] [review] Add GDateTime to GLib committed to master as e1f13ee9ed38d4f14bf927b6fa3f28530afc3640
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.
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).
*** Bug 628137 has been marked as a duplicate of this bug. ***
see also the comments in bug 628137, although some of those may be addressed by the patch that Thiago thought he'd posted before...
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.)
Created attachment 169173 [details] [review] Avoid using c99 reserved identifier __year
Created attachment 169174 [details] [review] Walk the format string using g_utf8_next_char() and use gunichar
Created attachment 169202 [details] [review] Adds gdatetime type to glib I indeed attached the wrong patch here, sorry :(
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.
Pushed: • attachment 169173 [details] [review] • attachment 169174 [details] [review] • attachment 169315 [details] [review] to master.
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.
(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.
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?
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.
(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.
(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>".
(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.
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.
(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.
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.
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.
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.
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
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
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.
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.
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.
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
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.