Code review comment for lp:~gunnarhj/indicator-datetime/days-months

Revision history for this message
Charles Kerr (charlesk) wrote :

Overall I agree with Martin on this -- it's kind of hacky, but a net plus for indicator-datetime.

Gunnar, as we discussed in #ubuntu-devel this patch needs to be updated to fit the GMenuified version of datetime that's about to land.

 1. Some of the time formatting is about the same as before: the header (ie, the date shown in the panel) and the first menuitem (today's date) are implemented about the same as before, so updating the utils.c patch for these two pieces should be pretty easy.

 2. However the appointment and location menuitems are very different. To avoid sending excess menuitem updates over the bus, we pass date format strings over the bus for these menuitems and let the client code do its own periodic calls to strftime(). For these, you'll want to modify the code in datetime/src/service.c such that the "x-canonical-time-format" properties set in create_appointments_section() and create_locations_section() contain your conversions.

Comments on format_date_time():

 1. when formatted_token is NULL, it's probably better to use the unformatted token than to bomb out. That way the user will get a partially formatted string instead of nothing, and will get a hint about which part of the formatting went wrong.

 2. splitting on " " seems a little brittle, as it'll fail on unconventional format strings. It might be better to create a GString to hold the output, then walk the input string, watching for '%'. During the walk the GString would be fed the normal characters from the input string, and be fed formatted strings whenever any of dm_names were reached in the input string. (This is just one idea, it doesn't have to be done exactly this way; I'm just giving an example on how to pass even on something like "%A,%B")

« Back to merge proposal