Created attachment 106525 [details]
example of sunset in 24 hour format

I poked at clock-location-tile.c and saw this comment at the Sunrise/Setset presentation code written by Vincent Untz:

/* FIXME: we need libgweather to give us the time,
 * not just a formatted string
 */

Doh. So unless there it's acceptable to do some string parsing in this case it looks like properly fixing this bug depends on that FIXME.

Fair to depends on: bug #520176 ?

Mayber related to bug 529991

Depends on libgweather returning different values.  Discussed in Bug#538787 [[http://bugzilla.gnome.org/show_bug.cgi?id=538787]]  Once this is committed and ironed out.  Fixing this shouldn't be bad at all.

*** Bug 557643 has been marked as a duplicate of this bug. ***

Footnote: it would be nice if the gweather applet would also resepect the date formatting when it displays the Sunrise & Sunset fields.  But I'm guessing that also depends on #518787 + gweather being able to get at the preferred date/time formating from clock....

Created attachment 137365 [details] [review]
Sunrise/Sunset time uses 12h/24h preference

Here's a patch (made against gnome-panel 2.26.2-1ubunutu1; ref LP: #197657).
This code requires the value APIs introduced in libgweather 2.25.2.

Some small issues:

+	if (clock_format == CLOCK_FORMAT_12)
+		format = _("%l:%M %p");
+	else
+		format = _("%H:%M");
+

This needs at least a translator comment explaining that a) this is a strftime format string and b) what it is used for.

 	tzset ();
+	if (weather_info_get_value_sunrise (info, &sunrise_time))
+		sunrise_str = convert_time_to_str (sunrise_time, clock_format);
+	else
+		sunrise_str = g_strdup ("???");

I don't think "???" is something we ever want to display. If we can't get a valid string, just don't show the time.

Thanks for the comments.

Re: translator comment; that's correct... that should have been added.  The same format strings also exist in (clock.c) without translation comment, which should also be commented.

Re: "???" text; the source file (clock-location-tile.c) already has a few instances of placing "???" into the output when there is an invalid string.  The new code I added duplicated similar logic, so I kept the same semantics of display.

If people like, I will submit a new patch with these changes.

Created attachment 142294 [details] [review]
Sunrise/Sunset time uses 12h/24h preference (revised)

This updated patch adds translation notes for the strftime format strings.

The "???" unknown time strings have been left, since much of the pre-existing code uses this for unknown time.  Some discussion by the main developers should probably be done to fix it over the entire 'clock' codebase.

Has Ted's patch been accepted?

Patch looks good to me now. As already mentioned, the strings are already present in the clock, so it doesn't add new strings.

IMO, we should get this in. Vincent ?

Review of attachment 142294 [details] [review]:

Please commit with the suggestions below.

::: applets/clock/clock-location-tile.c
@@ +505,3 @@
+convert_time_to_str (time_t now, ClockFormat clock_format)
+{
+	gchar *format;

const gchar *

@@ +522,3 @@
+ * for a 24-hour format.
+ * There should be little need to translate this string.
+ */

Please use the same translator comments as used in clock.c for the same strings.

@@ +636,3 @@
 	sys_timezone = getenv ("TZ");
 	setenv ("TZ", clock_location_get_timezone (location), 1);
 	tzset ();

Add empty line here.

@@ +648,3 @@
+				 sunrise_str, sunset_str);
+	g_free (sunrise_str);
+	g_free (sunset_str);

Add empty line after this.

Thanks, committed with those changes.