GNOME Bugzilla – Bug 677757
datetime: allow GstDateTime where not all fields are set
Last modified: 2012-06-12 23:33:25 UTC
Current Gstremer use two different types for date. The G_TYPE_DATE and GST_TYPE_DATE_TIME. In some cases it make hard to implement some thing if we do not know what to expect short date like "YYYY" or long datetime "YYYY-MM-DD HH:MM:SS". This is initial patch. It allow to store only Year to GstDateTime. The we need to megrate all code from G_TYPE_DATE to GST_TYPE_DATE_TIME.
Created attachment 216032 [details] [review] gstdatetime: allow to allocate GstDateTime with only year. To remove G_TYPE_DATE from gstreamer we need to allow store to GstDateTime only year. Signed-off-by: Oleksij Rempel <bug-track@fisher-privat.net>
Just for clarity, let me re-state the goal here: Tags often contain partial dates or date times, e.g. just a year, or year and month, or year month and day but no time. We want to be able to (a) signal this correctly to applications, and (b) deserialise and re-serialise tags properly, without loss of date/time information, but also without making up information that's not there. "2012" is not the same as "2012-01-01" and when we read DATE=2012 we want to later be able to write DATE=2012 again.
Created attachment 216063 [details] [review] [RFC] patch v1 This patch is my proposal for this issue. The previous API is not changed, so it even should not brake any thing. i added 3 functions: gst_date_time_set_partial_flags gst_date_time_get_partial_flags gst_date_time_new_ymd use case: ret = sscanf (value, "%04d-%02d-%02dT%02d:%02d:%02d" &year, &month, &day, &hour, &minute, &second); if (ret = 3) datetime = gst_date_time_new_ymd(year, month, day); else if (ret = 1) { datetime = gst_date_time_new_ymd(year, 1, 1); gst_date_time_set_partial_flags(datetime, GST_DATE_TIME_HAS_NO_MONTH | GST_DATE_TIME_HAS_NO_DAY); } ... same with gst_date_time_new or gst_date_time_new_local_time example on restoring the date: flags = gst_date_time_get_partial_flags(datetime); if(!flags & GST_DATE_TIME_HAS_NO_YEAR) year = gst_date_time_get_year(datetime); ... do what you need... else ... stop processing or skip year and continue with month ...
Thanks for working on this! Couple of comments: - I don't think I'm a fan of exposing this API via flags, for two reasons: it's cumbersome to use both for date time creators and consumers (esp. the new + set_partial_flag thing is not very nice IMHO); and it kind of allows options that don't really make sense (everything valid but the month, for example). We can still use the flags, but keep them inside the .c file. - the flags are also not exposed properly for bindings, but that's a moot point because of the above (should be an enum, see for example GstObjectFlags) - I think creation should take place with one function call. This might mean we need to add _new_y(), _new_ym(), _new_ymd(), and _new_ymdt() (perhaps better named, dunno). Alternatively allow passing of 0 month/day/ time/offset to _new_ymd()/_new_date() and _new(). Or both. - I think it would be enough to just differentiate between "have time" and "no time" for now. I'm not sure we really need to accommodate e.g. "hour only". - could add _new_from_iso8601_string() and _to_iso8601_string() utility functions (would be enough to handle YYYY, YYYY-MM, YYYY-MM-DD and YYYY-MMTHH:MM[:SS]Z and YYYY-MMTHH:MM[:SS]+/-XXXX or so for now IMHO (separate commit for this; should start with the most common variants, can add others later). - as for how to let a GstDateTime "consumer" query what fields are valid, I'm undecided: either _has_year(), _has_month(), has_day(), has_time() functions, or some kind of get_valid_fields() which returns an enum like GST_DATE_TIME_VALID_FIELDS_YEAR, GST_DATE_TIME_VALID_FIELDS_YEAR_MONTH, or _YYYY and _YYYY_MM etc., but that's not really much nicer either. I think I prefer _has_foo(). Will ask for more opinions tomorrow.
The _has_foo() functions would be "cumulative" then so to say, so _has_time() would imply that we have everything, and _has_month() would imply that there's a year as well.
Created attachment 216067 [details] [review] [RFC] patch v2 Ok, this is second try. do you mean some thing like this?
Created attachment 216086 [details] [review] [RFC] patch v3 New version 3, changes: - i added gst_date_time_has_foo(), how __tim requested. - also gst_date_time_get_foo calls gst_date_time_has_foo and warns if filed. - add gst_date_time_new_xyz for all combinations.
Ok, so after discussion on IRC we want: - The generic constructor gst_date_time_new() should accept -1 for unset fields. Year is always assumed to be set, but should have g_return_val_if_fail() for >0. The gtk-doc markup and description for _new() needs to be updated and extended. - if month is unset, day and time must be unset too - if day is unset, time must be unset too - hour/minute are always either both set or both not set - seconds may not be set/known (so we can differentiate HH:MM and HH:MM:SS) (if this is too messy, we can as a first step just make seconds=0 if HH/MM are set and seconds are not, and fix it up later; up to you) - add constructors for y / ym / ymd (as done in patch, only with gtk-doc markup for each) - add new _has_month(), _has_day(), has_time() and _has_time_with_seconds() functions - protect gst_date_time_get_foo() with g_return_val_if_fail (gst_date_time_has_foo (datetime), -1) - fix up all users of GstDateTime :) - it would be ok to get rid of all GDate uses for tags afterwards and just use GstDateTime for that.
Created attachment 216233 [details] [review] patch v4 I hope i didn't forgot any thing. There was lots of writing, so it can easily be.
> I hope i didn't forgot any thing. There was lots of writing, so it can > easily be. There was a step-by-step checklist in my previous comment... Anyway: commit 02a99c6ae8a77da7c02652e8cd0f69bc2bb38060 Author: Tim-Philipp Müller <tim.muller@collabora.co.uk> Date: Wed Jun 13 00:30:48 2012 +0100 docs: update for new datetime api commit 659a41b8961689b286024366f83f8ca172493307 Author: Tim-Philipp Müller <tim.muller@collabora.co.uk> Date: Wed Jun 13 00:28:00 2012 +0100 win32: update .def file for latest API commit a13ed36c6a0c5c420378218d3421263d98c49efe Author: Tim-Philipp Müller <tim.muller@collabora.co.uk> Date: Wed Jun 13 00:25:24 2012 +0100 docs: add new datetime API commit b45a44ef27ec8f9bb77a8f495c67ba1a371d05a8 Author: Tim-Philipp Müller <tim.muller@collabora.co.uk> Date: Wed Jun 13 00:21:32 2012 +0100 tests: add some basic unit tests for partial date time fields commit 5b37641cbcdec1ad744404800fca87fa8a7d60e9 Author: Tim-Philipp Müller <tim.muller@collabora.co.uk> Date: Tue Jun 12 23:52:02 2012 +0100 datetime: clean-ups and new API adjustments Remove constructors we don't want: gst_date_time_new_ymd_h() because we don't want to support hour-only for now; gst_date_time_new_ymd_hm() because we don't want to add constructors with time info where the caller doesn't have to think about what timezone the time is in. Lots of compulsive clean-up. Docs fixes. Replace has_minute() and has_hour() with has_time(). commit 000ebef2f4d649015f6689a13e32d016d126e78f Author: Oleksij Rempel <bug-track@fisher-privat.net> Date: Tue Jun 12 22:35:42 2012 +0200 datetime: allow GstDateTime where not all fields are set In order to deserialise and re-serialise dates and date times from tags properly, we need to be able to express partial dates (e.g. YYYY or YYYY-MM) and date times. We only support partial date times where all the more significant fields above the first unset field are set (e.g. YYYY-00-DD is not supported). Calling _get_foo() when foo is not set is not allowed any more, callers need to check which fields are set first. https://bugzilla.gnome.org/show_bug.cgi?id=677757