Created attachment 370808 [details] [review]
gdatetime: Add g_date_time_get_timezone() accessor

This is a trivial method to get the GTimeZone for the GDateTime.

Signed-off-by: Philip Withnall <withnall@endlessm.com>

Created attachment 370809 [details] [review]
gtimezone: Add g_time_zone_get_identifier() accessor

This is a non-trivial accessor which gets the identifier string used to
create the GTimeZone — unless the string passed to g_time_zone_new() was
invalid, in which case the identifier will be `UTC`.

Implementing this required reworking how timezone information was loaded
so that the tz->name is always set at the same time as tz->t_info, so
they are in sync. Previously, the tz->name was unconditionally set to
whatever was passed to g_time_zone_new(), and then not updated if the
tz->t_info was eventually set to the default UTC information.

This includes tests for the new g_time_zone_get_identifier() API, and
for the g_date_time_get_timezone() API added in the previous commit.

Signed-off-by: Philip Withnall <withnall@endlessm.com>

Created attachment 370810 [details] [review]
gdatetime: Fix a leak in g_date_time_new_week()

This was a small leak of a GDateTime instance from an internal helper
function, which was using it to calculate week numbers, and then forgot
to free it.

Signed-off-by: Philip Withnall <withnall@endlessm.com>

Review of attachment 370809 [details] [review]:

One minor comment, otherwise:

Reviewed-by: Georges Basile Stavracas Neto <gbsneto@gnome.org>

::: glib/gtimezone.c
@@ +1925,3 @@
+ * this function.
+ *
+ * Returns: identifier for this timezone

Returns: (transfer none)(nullable):

(Or doesn't GLib require that?)

Review of attachment 370808 [details] [review]:

Looks good.

Reviewed-by: Georges Basile Stavracas Neto <gbsneto@gnome.org>

Review of attachment 370810 [details] [review]:

Reviewed-by: Georges Basile Stavracas Neto <gbsneto@gnome.org>

Review of attachment 370809 [details] [review]:

::: glib/gtimezone.c
@@ +1925,3 @@
+ * this function.
+ *
+ * Returns: identifier for this timezone

(transfer none) is implied by the const (gobject-introspection understands this).

(nullable) is incorrect: the return value can never be NULL. Return values are (not nullable) by default.

So the lack of annotations is correct here.

I’d like to get a second opinion from GLib maintainers (Matthias/Emmanuele) before pushing this though.

Review of attachment 370808 [details] [review]:

Okay.

Review of attachment 370809 [details] [review]:

Looks good overall, but I'm not so sure about this:

```
unless the string passed to g_time_zone_new() was invalid, in which case the identifier will be `UTC`
```

Should we warn, instead? I know that g_time_zone_new() does not have a GError** argument, but I'm not entirely sure we should drop stuff on the floor.

Review of attachment 370810 [details] [review]:

Sure.

(In reply to Emmanuele Bassi (:ebassi) from comment #10)
> Review of attachment 370809 [details] [review] [review]:
> 
> Looks good overall, but I'm not so sure about this:
> 
> ```
> unless the string passed to g_time_zone_new() was invalid, in which case the
> identifier will be `UTC`
> ```
> 
> Should we warn, instead? I know that g_time_zone_new() does not have a
> GError** argument, but I'm not entirely sure we should drop stuff on the
> floor.

That would be an API break: g_time_zone_new() already does this silent-UTC-fallback thing. I would like to improve that, but I think that retaining consistency between g_time_zone_get_identifier() and the current behaviour of g_time_zone_new() is important.

Let’s discuss improving g_time_zone_new()’s error handling behaviour in bug #677522?

All pushed to master. Let's defer talking about error handling for g_time_zone_new() to bug #677522. Thanks for the reviews!

Attachment 370808 [details] pushed as 9ddd17d - gdatetime: Add g_date_time_get_timezone() accessor
Attachment 370809 [details] pushed as 8945227 - gtimezone: Add g_time_zone_get_identifier() accessor
Attachment 370810 [details] pushed as 68f6d39 - gdatetime: Fix a leak in g_date_time_new_week()

gnome-shell no longer displays local time in the top panel on FreeBSD. It always displays UTC time regardless of the system timezone.

$ date
Sun Apr 22 00:32:45 CST 2018

$ python3 -c 'from gi.repository import GnomeDesktop; print(GnomeDesktop.WallClock().get_clock())'
Sat Apr 21, 16‎∶32

'git bisect' shows that it is broken by attachment 370809 [details] [review]. It seems that that the patch expects /etc/localtime to be a symlink, but the assumption is seldom true on FreeBSD. FreeBSD installer copies the timezone file from /usr/share/zoneinfo to /etc/localtime when setting system timezone, so /etc/localtime is a regular file.

(In reply to Ting-Wei Lan from comment #14)
> 'git bisect' shows that it is broken by attachment 370809 [details] [review]
> [review]. It seems that that the patch expects /etc/localtime to be a
> symlink, but the assumption is seldom true on FreeBSD. FreeBSD installer
> copies the timezone file from /usr/share/zoneinfo to /etc/localtime when
> setting system timezone, so /etc/localtime is a regular file.

That’s a pain. How are you supposed to find the current timezone on FreeBSD then? Why don’t they symlink?

I’m happy to accept a patch to improve this (which could mean to fall back to the previous behaviour when compiling for FreeBSD), but I can’t really come up with one myself as I have no way to compile or test one.

Created attachment 371498 [details] [review]
gtimezone: Fallback to /var/db/zoneinfo on FreeBSD

The timezone setup utility of FreeBSD, tzsetup, which is run during the
installation, creates /etc/localtime by copying the chosen timezone file
from /usr/share/zoneinfo. Although it can correctly deal with the case
where /etc/localtime is a symlink, it is not the default and there is no
user interface to change the default copying behaviour.

Fortunately, tzsetup has been modified to write the name of the chosen
timezone to /var/db/zoneinfo in 2009, so we can know the name of the
current timezone by reading it. DragonflyBSD also seems to do the same
thing in its tzsetup.

https://svnweb.freebsd.org/changeset/base/198267

Created attachment 371499 [details] [review]
tests: Fix GDateTime format tests on non-English locales

It seems that the test expects g_date_time_format to return formatted
results in English, and there is no setlocale (LC_ALL, "") call in the
file so the test does run in the default C locale. However, gettext
seems to read the value of LC_MESSAGES from the environment by itself.
Even if the value of LC_MESSAGES locale is C because of not calling
setlocale, gettext still translates the name of the month according to
the LC_MESSAGES environment variable, causing g_date_time_format_locale
to fail on the "%b" test case because it cannot convert UTF-8 text
returned by get_month_name_with_day to ASCII.

To avoid the test failure, we set the LC_MESSAGES environment variable
to C before format tests and restore it at the end of the function.

Review of attachment 371498 [details] [review]:

::: glib/gtimezone.c
@@ +447,3 @@
+          g_clear_error (&read_link_err);
+
+          /* Fallback to the content of /var/db/zoneinfo if /etc/localtime is

Perhaps this code should be `#if defined (__FreeBSD__) || defined (__DragonFly__) || …`, since it’s definitely not going to help on Linux. I don’t mind either way, though, since the code is not going to be actively harmful on Linux. Up to you.

Review of attachment 371499 [details] [review]:

Can you give an example of what environment causes this to fail?

::: glib/tests/gdatetime.c
@@ +1325,3 @@
  * enough.
  */
+  gchar *oldmessages;

Please use underscores to separate words. ‘Messages’ is also not obviously referring to `LC_MESSAGES`, so I’d call this `old_lc_messages`.

@@ +1357,3 @@
+  oldmessages = (gchar*) g_getenv ("LC_MESSAGES");
+  if (oldmessages != NULL)
+    oldmessages = g_strdup (oldmessages);

g_strdup(NULL) is guaranteed to return NULL, so you can eliminate the if:

oldmessages = g_strdup (g_getenv ("LC_MESSAGES"));

@@ +1358,3 @@
+  if (oldmessages != NULL)
+    oldmessages = g_strdup (oldmessages);
+  g_setenv ("LC_MESSAGES", "C", TRUE);

C.UTF-8 instead, to ensure the character encoding is predictable?

Created attachment 371554 [details] [review]
tests: Fix GDateTime format tests on non-English locales

It seems that the test expects g_date_time_format to return formatted
results in English, and there is no setlocale (LC_ALL, "") call in the
file so the test does run in the default C locale. However, gettext
seems to read the value of LC_MESSAGES from the environment by itself.
Even if the value of LC_MESSAGES locale is C because of not calling
setlocale, gettext still translates the name of the month according to
the LC_MESSAGES environment variable, causing g_date_time_format_locale
to fail on the "%b" test case because it cannot convert UTF-8 text
returned by get_month_name_with_day to ASCII.

To avoid the test failure, we set the LC_MESSAGES environment variable
to C before format tests and restore it at the end of the function.

(In reply to Philip Withnall from comment #18)
> Review of attachment 371498 [details] [review] [review]:
> 
> ::: glib/gtimezone.c
> @@ +447,3 @@
> +          g_clear_error (&read_link_err);
> +
> +          /* Fallback to the content of /var/db/zoneinfo if /etc/localtime
> is
> 
> Perhaps this code should be `#if defined (__FreeBSD__) || defined
> (__DragonFly__) || …`, since it’s definitely not going to help on Linux. I
> don’t mind either way, though, since the code is not going to be actively
> harmful on Linux. Up to you.

I am not going to change it. I don't like OS checks and I want to avoid adding one unless it is really required.

(In reply to Philip Withnall from comment #19)
> Review of attachment 371499 [details] [review] [review]:
> 
> Can you give an example of what environment causes this to fail?

I was running the test with zh_TW.UTF-8 locale on FreeBSD. The test was running in C locale (confirmed by calling setlocale), and g_get_charset(NULL) returns false because C is an ASCII locale which doesn't support UTF-8. However, get_month_name_with_day uses gettext, and gettext seems to use LC_MESSAGES environment variable to decide that it should translate "October" to "十月". Since g_date_time_locale_format_locale is called with locale_is_utf8 set to FALSE, it wants to converts the string returned by get_month_name_with_day to the locale encoding, ASCII, which is not possible. The conversion fails and g_date_time_locale_format_locale returns NULL.

> 
> ::: glib/tests/gdatetime.c
> @@ +1325,3 @@
>   * enough.
>   */
> +  gchar *oldmessages;
> 
> Please use underscores to separate words. ‘Messages’ is also not obviously
> referring to `LC_MESSAGES`, so I’d call this `old_lc_messages`.

OK, I was following the style of 'oldlocale' in the file so I didn't add underscores. It is now fixed in the new attachment.

> 
> @@ +1357,3 @@
> +  oldmessages = (gchar*) g_getenv ("LC_MESSAGES");
> +  if (oldmessages != NULL)
> +    oldmessages = g_strdup (oldmessages);
> 
> g_strdup(NULL) is guaranteed to return NULL, so you can eliminate the if:
> 
> oldmessages = g_strdup (g_getenv ("LC_MESSAGES"));

Fixed in attachment 371554 [details] [review]. 

> 
> @@ +1358,3 @@
> +  if (oldmessages != NULL)
> +    oldmessages = g_strdup (oldmessages);
> +  g_setenv ("LC_MESSAGES", "C", TRUE);
> 
> C.UTF-8 instead, to ensure the character encoding is predictable?

I know C.UTF-8 is better because Unicode support is almost always required. However, there is no C.UTF-8 locale on FreeBSD. Using C.UTF-8 probably doesn't harm because gettext will simply ignore it, but it also doesn't improve things on systems not supporting it.

Review of attachment 371554 [details] [review]:

OK.

Created attachment 371585 [details] [review]
gtimezone: Fallback to /var/db/zoneinfo on FreeBSD

The timezone setup utility of FreeBSD, tzsetup, which is run during the
installation, creates /etc/localtime by copying the chosen timezone file
from /usr/share/zoneinfo. Although it can correctly deal with the case
where /etc/localtime is a symlink, it is not the default and there is no
user interface to change the default copying behaviour.

Fortunately, tzsetup has been modified to write the name of the chosen
timezone to /var/db/zoneinfo in 2009, so we can know the name of the
current timezone by reading it. DragonflyBSD also seems to do the same
thing in its tzsetup.

https://svnweb.freebsd.org/changeset/base/198267

I updated the zoneinfo patch to resolve a merge conflict (from bug #111848). Can you retest it please Ting-Wei?

I’ll push the tests/gdatetime patch shortly.

Comment on attachment 371554 [details] [review]
tests: Fix GDateTime format tests on non-English locales

Attachment 371554 [details] pushed as a0c7f85 - tests: Fix GDateTime format tests on non-English locales

(In reply to Philip Withnall from comment #24)
> I updated the zoneinfo patch to resolve a merge conflict (from bug #111848).
> Can you retest it please Ting-Wei?

Yes, I tested it and it works fine on my FreeBSD desktop system.

Thanks for retesting, I have pushed to master.

Attachment 371585 [details] pushed as 50677e9 - gtimezone: Fallback to /var/db/zoneinfo on FreeBSD