GNOME Bugzilla – Bug 761889
GDateTime: %p does not always print AM/PM string
Last modified: 2017-04-03 16:31:57 UTC
This is another patch we ship in Endless. The problem is that if the locale does not provide a translation for AM/PM, the Time Format option in the control center Date & Time panel will do nothing when set to AM/PM. The patch fixes it by simply adding "AM" or "PM" in such case, when the translation is not available.
Created attachment 320925 [details] [review] wall-clock: Always consider that 12-h formats are available Instead of checking whether the 12-h formats are available, consider them always possible and simply fallback to a non-localized AM/PM string if the current locale does not provice one.
Any comments on this patch?
Ping? This still applies.
(In reply to Cosimo Cecchi from comment #3) > Ping? This still applies. There's no explanations as to why it's useful, how to reproduce the problem. Gut feeling is that the toggle shouldn't be available in g-c-c if there's no support for it in the locale.
The problem here is that many locales do not claim to support AM/PM (e.g. es_DO, or event pt_BR), but still there is a very reasonable expectation that an user is able to configure whether to use 12h or 24h time format. (Why forbid an user in pt_BR locale to choose 12h time format if they wish?) Instead of adding the same default format to every single locale that does not support it, this patch makes it so that the "AM" or "PM" literal string is used when the locale does not provide a different specification. (I can forward you a copy of the internal ticket that prompted this patch if you want more background.)
Review of attachment 320925 [details] [review]: > Always consider that 12-h formats are available 12h ::: libgnome-desktop/gnome-wall-clock.c @@ +328,1 @@ format_string = show_seconds ? _("%l:%M:%S %p") Better, would be to replace %p with AM/PM before passing it through date_time_format(). But even better would be to add AM/PM fallback support to g_date_time_format(). Which it already claims to have! " %p: either "AM" or "PM" according to the given time value, or the corresponding strings for the current locale. Noon is treated as "PM" and midnight as "AM". " I read that as falling back to AM/PM if the locale doesn't have any strings.
Once it's fixed in glib, we can remove the detection code from gnome-control-center as well.
Created attachment 348846 [details] testcase Test case to reproduce this. Run like this cosimo@endless:~$ LANG=en_US.utf8 gjs datetime-761889.js UTC time is 11:24 pm cosimo@endless:~$ LANG=pt_BR.utf8 gjs datetime-761889.js UTC time is 11:25 In the latter case, the expected output is "UTC time is 11:25 PM" instead.
Thanks Bastien, it does make a lot of sense to fix this in GDateTime itself.
Created attachment 349130 [details] [review] datetime: factor out a common function The 'p' and 'P' cases are exactly the same, except for one line. Factor out a common function for both.
Created attachment 349131 [details] [review] datetime: factor out fallback AM/PM function We will reuse this.
Created attachment 349132 [details] [review] datetime: add fallback logic for AM/PM specifier When the locale does not provide any string for AM/PM, fallback to the default.
Review of attachment 349130 [details] [review]: ++
Review of attachment 349131 [details] [review]: Good to push with this change. ::: glib/gdatetime.c @@ +344,3 @@ #endif /* HAVE_LANGINFO_TIME */ +static const gchar * Would be useful to have a brief comment here: /* Format AM/PM indicator if the locale does not have a localized version. */
Review of attachment 349132 [details] [review]: ::: glib/gdatetime.c @@ +2217,3 @@ + + if (!ampm || ampm[0] == '\0') + ampm = (gchar *) get_fallback_ampm (g_date_time_get_hour (datetime)); I don’t like this (gchar *) cast. Could you make `ampm` const, and rework some of the rest of the assignments in format_ampm() so heap-allocated and non-heap-allocated memory are not conflated in the same variable?
Attachment 349130 [details] pushed as 9163306 - datetime: factor out a common function Attachment 349131 [details] pushed as 62edcf0 - datetime: factor out fallback AM/PM function
Created attachment 349184 [details] [review] datetime: don't conflate heap/non-heap allocations in same variable Use an extra variable instead of casting out the const specifier.
Created attachment 349185 [details] [review] datetime: add fallback logic for AM/PM specifier When the locale does not provide any string for AM/PM, fallback to the default.
Thanks, Philip. I pushed those two and attached new patches for your suggestion.
Review of attachment 349184 [details] [review]: Thanks.
Review of attachment 349185 [details] [review]: ++
Attachment 349184 [details] pushed as 6e19258 - datetime: don't conflate heap/non-heap allocations in same variable Attachment 349185 [details] pushed as 69e448b - datetime: add fallback logic for AM/PM specifier