Clarification: "today" is in the timezone provided by the user, if any, otherwise Z - just like the behavior of the existing code.

Comment on attachment 309007 [details] [review]
Patch file

Thanks, looks mostly fine.

We should probably add a comment to the function docs giving some information on what formats it accepts (beyond just 'the most common formats'), i.e. also make it explicit that just passing in a time is acceptable as well now.

> 
>-  if (len < 4 || !g_ascii_isdigit (string[0]) || !g_ascii_isdigit (string[1])
>-      || !g_ascii_isdigit (string[2]) || !g_ascii_isdigit (string[3]))
>+  if (len < 4 || !g_ascii_isdigit (string[0]) || !g_ascii_isdigit (string[1]))
>     return NULL;

Here it would be nice to add a comment above that explains what the expected/allowed strings/formats/things are here (i.e. year or hour?)

 
>-  ret = sscanf (string, "%04d-%02d-%02d", &year, &month, &day);
>+  if (g_ascii_isdigit (string[2]) && g_ascii_isdigit (string[3])) {

I think we're now not bailing out when there are only 3 valid digits? (maybe add something to the unit test to check this)

> ymd_hms:
>+  if (year == -1 || month == -1 || day == -1) {
>+    GDateTime *g_date_now_utc, *g_date_now_in_given_tz;

Perhaps rename these to just 'now_utc' and 'now_in_given_tz', otherwise it looks like glib symbols.


>+    /* No date was supplied: make it today */
>+    g_date_now_utc = g_date_time_new_now_utc ();
>+    if (tzoffset != 0.0) {
>+      /* If a timezone offset was supplied, get the date of that timezone */
>+      gint offset_hours, offset_mins, offset_secs;
>+
>+      offset_hours = (gint) tzoffset;
>+      offset_mins = (gint) (60 * (tzoffset - offset_hours));
>+      offset_secs = (gint) (3600 * (tzoffset - offset_mins));
>+      g_date_now_in_given_tz =
>+          g_date_time_add_hours (g_date_now_utc, offset_hours);
>+      g_date_now_in_given_tz =
>+          g_date_time_add_minutes (g_date_now_in_given_tz, offset_mins);
>+      g_date_now_in_given_tz =
>+          g_date_time_add_seconds (g_date_now_in_given_tz, offset_secs);
>+    } else {
>+      g_date_now_in_given_tz = g_date_now_utc;
>+    }
>+    year = g_date_time_get_year (g_date_now_in_given_tz);
>+    month = g_date_time_get_month (g_date_now_in_given_tz);
>+    day = g_date_time_get_day_of_month (g_date_now_in_given_tz);
>+  }
>   return gst_date_time_new (tzoffset, year, month, day, hour, minute, second);

I think here you might be leaking the two GDateTime created earlier?

Also, did you check already if this code can be simplified e.g. by creating a GTimeZone or so?

Thanks for the comments! Fixed in the code, except:

(In reply to Tim-Philipp Müller from comment #2)
> Comment on attachment 309007 [details] [review] [review]
>  
> >-  ret = sscanf (string, "%04d-%02d-%02d", &year, &month, &day);
> >+  if (g_ascii_isdigit (string[2]) && g_ascii_isdigit (string[3])) {
> 
> I think we're now not bailing out when there are only 3 valid digits? (maybe
> add something to the unit test to check this)

We are later, in this line:
  /* if hour or minute fails, then we will use only ymd. */
  hour = g_ascii_strtoull (string, (gchar **) & string, 10);
  if (hour > 24 || *string != ':')
    goto ymd;

I checked it manually but didn't find it necessary to add it in the unit test, do you still think it applies?

> >+    /* No date was supplied: make it today */
> >+    g_date_now_utc = g_date_time_new_now_utc ();
> >+    if (tzoffset != 0.0) {
> >+      /* If a timezone offset was supplied, get the date of that timezone */
> >+      gint offset_hours, offset_mins, offset_secs;
> >+
> >+      offset_hours = (gint) tzoffset;
> >+      offset_mins = (gint) (60 * (tzoffset - offset_hours));
> >+      offset_secs = (gint) (3600 * (tzoffset - offset_mins));
> >+      g_date_now_in_given_tz =
> >+          g_date_time_add_hours (g_date_now_utc, offset_hours);
> >+      g_date_now_in_given_tz =
> >+          g_date_time_add_minutes (g_date_now_in_given_tz, offset_mins);
> >+      g_date_now_in_given_tz =
> >+          g_date_time_add_seconds (g_date_now_in_given_tz, offset_secs);
> >+    } else {
> >+      g_date_now_in_given_tz = g_date_now_utc;
> >+    }
> >+    year = g_date_time_get_year (g_date_now_in_given_tz);
> >+    month = g_date_time_get_month (g_date_now_in_given_tz);
> >+    day = g_date_time_get_day_of_month (g_date_now_in_given_tz);
> >+  }
> >   return gst_date_time_new (tzoffset, year, month, day, hour, minute, second);
> 
> I think here you might be leaking the two GDateTime created earlier?

oops, yes :)
 
> Also, did you check already if this code can be simplified e.g. by creating
> a GTimeZone or so?

I didn't find a way to create a GTimeZone from a numeric offset, suggestions more than welcome!

However, I am now reusing the two offsets created earlier (seconds don't apply - shouldn't have used it!), and also using g_date_time_get_ymd to squeeze three lines of code into one.
Also initializing gmt_offset_{min,hour} to -99 instead of -1, because (especially for the hour!) -1 is a very valid value after my mild refactor.

Attaching new patch next.

Created attachment 309017 [details] [review]
New patch.

Thanks, pushed:

commit 88f6334af616b32fb8851d79faa8d31df552fea4
Author: Vivia Nikolaidou <vivia@ahiru.eu>
Date:   Mon Aug 10 15:31:37 2015 +0300

    datetime: accept just a time as ISO 8601 string and use today's date then
    
    If no date and only a time is given in gst_date_time_new_from_iso8601_string(),
    assume that it is "today" and try to parse the time-only string. "Today" is
    assumed to be in the timezone provided by the user (if any), otherwise Z -
    just like the behavior of the existing code.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=753455