I want to express my deep +1 here. The locales already provide us with the time format information; using gettext to duplicate the information there is error prone, redundant, and inconsistent.

The thing I don't fully understand is

  +        setlocale( LC_TIME, getenv("LC_MESSAGES") );


So by default it should behave like $LANG, but if $LC_MESSAGES is set, it should behave like messages instead of general $LANG? That seems a bit counterintuitive, if someone sets e. g. LANG=de_DE.utf8 and LC_MESSAGES=en_US.utf8, the defined behaviour of LC_* and LANG) is to display the German time format, not the English one.

Also, I don't think we should test for the mere existence of an AM_PM locale definition. The more correct approach would be to check what T_FMT prescribes for a locale. https://launchpadlibrarian.net/56917658/is24h.c is a little test program (from https://launchpad.net/bugs/652976) which we found to be very reliable.

Thanks!

I think we probably just need to jump on the new GDateTime train, right?

On 2011-06-07 15:50, Martin Pitt wrote:
> The thing I don't fully understand is
> 
>   +        setlocale( LC_TIME, getenv("LC_MESSAGES") );
> 
> So by default it should behave like $LANG, but if $LC_MESSAGES is set, it
> should behave like messages instead of general $LANG? That seems a bit
> counterintuitive, if someone sets e. g. LANG=de_DE.utf8 and
> LC_MESSAGES=en_US.utf8, the defined behaviour of LC_* and LANG) is to display
> the German time format, not the English one.

The German time _format_, absolutely. In the proposed code I use LC_MESSAGES merely to make "Tue" today's displayed weekday in your example instead of "Di".

> Also, I don't think we should test for the mere existence of an AM_PM locale
> definition. The more correct approach would be to check what T_FMT prescribes
> for a locale. https://launchpadlibrarian.net/56917658/is24h.c is a little test
> program (from https://launchpad.net/bugs/652976) which we found to be very
> reliable.

I can make an attempt to replace my 'hackish' test for 12 or 24 hours time by help of that.

Ray, I had a quick look at GDateTime, but wasn't able to figure out how it would simplify this code snippet.

> The German time _format_, absolutely. In the proposed code I use LC_MESSAGES
> merely to make "Tue" today's displayed weekday in your example instead of "Di".

Ah, of course. I misread that.

> Ray, I had a quick look at GDateTime, but wasn't able to figure out how it
> would simplify this code snippet.

If we want to include seconds, then g_date_time_format("%X") would be by far the easiest, and a correct implementation. However, when we wrote above code snippet we investigated a number of different ways to just show hours and minute, and unfortunately strftime(), GDateTime etc. make that pretty hard. You can't rely on ':' as a separator; you could of course use some regexp magic to get the first two groups of numbers and the separator in between, which should be pretty reliable. Or do some iteration over the string, and remember the first group of non-digit characters until you hit a digit again, and g_strsplit() on that. But we still found nl_langinfo(T_FMT) to give us the information with the smallest overhead.

Created attachment 189442 [details] [review]
Time display on greeter

Patch modified to use "nl_langinfo(T_FMT)" as suggested by Martin. If you want to test it, a gdm package for Ubuntu Natty, in which the new patch was applied, is available at https://launchpad.net/~gunnarhj/+archive/misc

Review of attachment 189442 [details] [review]:

::: gdm-2.32.1//gui/simple-greeter/gdm-clock-widget.c
@@ +61,3 @@
 
+int
+is_24h ()

should have (void) here instead of ()

@@ +68,3 @@
+
+        for (i = 0; formats_24h[i]; ++i) {
+                if (strstr(t_fmt, formats_24h[i])) {

need space after strstr and != NULL at the end to match coding style

@@ +81,3 @@
         char       *tooltip_format;
 
+        setlocale(LC_TIME, "");

space after setlocale before (

@@ -64,3 @@
         char       *tooltip_format;
 
-        if (clock->priv->should_show_date && clock->priv->should_show_seconds) {

If we aren't going to support should_show_date and should_show_seconds anymore they need to get removed from the priv structure.

@@ +82,3 @@
 
+        setlocale(LC_TIME, "");
+        if ( is_24h() ) {

No spaces on parentheses interior. space after is_24h before ()

@@ +103,3 @@
         time_t     t;
         struct tm *tm;
+        char       showed_time[32];

s/showed/displayed/ ?

@@ +119,3 @@
         }
+
+        setlocale( LC_TIME, getenv("LC_MESSAGES") );

spaces

@@ +120,3 @@
+
+        setlocale( LC_TIME, getenv("LC_MESSAGES") );
+        strftime(weekday, sizeof(weekday), "%a", tm);

need to check return value

@@ +121,3 @@
+        setlocale( LC_TIME, getenv("LC_MESSAGES") );
+        strftime(weekday, sizeof(weekday), "%a", tm);
+        sprintf(buf, "%s %s", weekday, showed_time);

I don't think you can just concatenate things like this.  It's not going to localize well.  What about languages where weekday shows up on the right (or whatever)?

@@ +127,3 @@
         g_free (utf8);
 
         if (tooltip_format != NULL) {

tooltip_format is never null now right?

Oh, I meant to say "thanks for looking into this" in the overview part of the review but was a little too quick to hit Publish.

Created attachment 189998 [details] [review]
Time display on greeter

Thanks for your comments, Ray! I have made the requested changes except for one (see below) together with a couple of additional changes of symbol names. The latest patch applies to gdm-3.0.4.

On 2011-06-14 03:03, Ray Strode wrote:
> @@ +121,3 @@
> +        setlocale( LC_TIME, getenv("LC_MESSAGES") );
> +        strftime(weekday, sizeof(weekday), "%a", tm);
> +        sprintf(buf, "%s %s", weekday, showed_time);
> 
> I don't think you can just concatenate things like this.  It's not going to
> localize well.  What about languages where weekday shows up on the right (or
> whatever)?

Well, I rather see weekday and time as two time related components that are displayed side by side. Assuming that there are no format rules for weekday plus time only, localization does not apply to the result of the concatenation, does it?

Please also take into consideration that LC_TIME and LC_MESSAGES may have different values. Assume for instance that LC_TIME is "sv_SE.UTF-8", while the display language is French. If we would use gettext and let the translators fine tune the format, a French translator's ideas would determine the Swedish time format. Could that ever be right? ;-)

Created attachment 201724 [details] [review]
Time display on greeter

We are considering to redefine the LANG environment variable in Ubuntu to represent the display language instead of regional formats. During a test session I noticed a need due to that possible change to modify the patch that is attached to this bug report. Since LANG has represented the display language in GNOME all along, I realized that the patch never has worked as intended in GNOME. :(

I just added a modified patch, that I think works in GNOME as well.

(In reply to comment #9)
> Created an attachment (id=201724) [details] [review]
> Time display on greeter
> 
> We are considering to redefine the LANG environment variable in Ubuntu 

I don't think Ubuntu gets to define what LANG means - thats' been done already by Posix: http://pubs.opengroup.org/onlinepubs/7908799/xbd/envvar.html

On 2012-11-26 03:58, Matthias Clasen wrote:
> On 2011-11-20 02:30, Gunnar Hjalmarsson wrote:
>>
>> We are considering to redefine the LANG environment variable in Ubuntu 
> 
> I don't think Ubuntu gets to define what LANG means - thats' been done already
> by Posix:

Well, it's true, of course.

Previously a few language related LC_* variables were set in Ubuntu, so LANG indirectly denoted regional formats. Now, instead, we set a few formats related LC_* variables, so indirectly LANG denotes the display language. That's what I meant with the above not very clear wording.

I guess we should close this out now since we don't ship the simple-greeter anymore.