Re: [Tracker] localtime branch review

[Jürg Billeter <j bitron ch>]

Hi Martyn,

On Fri, 2010-02-19 at 11:54 +0000, Martyn Russell wrote:
> JÃrg recently worked on the localtime branch to fix some of the issues 
> we have with timezones and being able to convert between time/date 
> values appropriately for the current locale. This is the review of that 
> branch:
>
>    http://git.gnome.org/browse/tracker/log/?h=localtime
>
> --
>
> 1. The functions tracker_date_time_value_init() and _copy() are static 
> and internal, shouldn't they avoid using the tracker_ namespace?

Right, I will change this.

> 2. We should probably add some offset checking here for 
> tracker_date_time_set(), offset must have a maximum. Assuming offset is 
> in seconds it can only be 24 * 60 * 60 right?

Yes, it can never be more, I will add a check.

> 3. tracker_date_time_set_from_string(), 
> tracker_date_time_get_local_date() and 
> tracker_date_time_get_local_time() all need parameter checking, 
> g_return_...()

I will add parameter checks.

> 4. tracker-type-utils-test.c should probably test the offset too for 
> tracker_string_to_date().

Probably makes sense, yes. I will add this.

> 5. So from what I understand this approach uses localtime for composed 
> tables and date+time columns for decomposed tables right? Just out of 
> interest, why do we have two columns for date/time in the decomposed 
> tables (just trying to understand that's all)?

Do you mean class tables when you say composed tables? (We normally use
decomposed as term to describe the whole database schema in 0.7,
compared to 0.6). We use the same approach for class and property
tables: we use three columns in both cases, one for UTC, one for local
date, and one for local time of day. The UTC column is the main column -
in master we don't have the other two columns.

The split into local date and local time has been chosen to enable use
of indexes for local time queries. A single column representing a local
timestamp would not be very useful for indexing.

> Other than that, I couldn't see any problems with the branch. Good work 
> JÃrg.

Thanks for the review,
JÃrg