[Bug 753455] gstdatetime: gst_date_time_new_from_iso8601_string: Default date is "today"

[GStreamer (GNOME Bugzilla)]

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

--- Comment #3 from Vivia Nikolaidou <vivia at ahiru.eu> ---
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]
>  
> >-  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.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.