Created attachment 109501 [details] [review]
add support for PLD Linux system timezone

Comment on attachment 109501 [details] [review]
add support for PLD Linux system timezone

Thanks, here are some comments

First, please add a note in the big comment in the middle of the file to explain the format used in PLD Linux. I'd suggest PLD Linux should change the config format used if possible...

Also, you need to update files_to_check to get some file monitoring love.

>+/* This works for PLD Linux */
>+static char *
>+system_timezone_read_etc_sysconfig_timezone (void)
>+{
>+        char *zone_info_area, *time_zone, *tz;
>+
>+	zone_info_area = system_timezone_read_key_file (ETC_SYSCONFIG_TIMEZONE,
>+							"ZONE_INFO_AREA");
>+	time_zone = system_timezone_read_key_file (ETC_SYSCONFIG_TIMEZONE,
>+						   "TIME_ZONE");
>+
>+	if (!time_zone)
>+		tz = NULL;
>+	else if (!zone_info_area)
>+		tz = g_strdup (time_zone);
>+	else
>+		tz = g_strjoin ("/", zone_info_area, time_zone, NULL);
>+
>+	g_free (zone_info_area);
>+	g_free (time_zone);
>+
>+	return tz;
>+}

Sounds better to do this:

time_zone = system_timezone_read_key_file (...);
if (!time_zone)
  return NULL;

zone_info_area = system_timezone_read_key_file (...);

and then continue with the code. This way we read the file only once if TIME_ZONE is not there.

>+static gboolean
>+system_timezone_write_etc_sysconfig_timezone (const char  *tz,
>+					      GError     **error)
>+{
>+	gboolean ret;
>+
>+	if (g_strrstr (tz, "/") == NULL) {

Please use strchr()

>+		ret = system_timezone_write_key_file (ETC_SYSCONFIG_TIMEZONE,
>+						      "TIME_ZONE", tz, error);
>+
>+		if (ret)
>+			ret = system_timezone_write_key_file (ETC_SYSCONFIG_TIMEZONE,
>+							      "ZONE_INFO_AREA", "", error);
>+	} else {
>+		gchar **items, *zone_info_area;
>+		gint	i, nitems;
>+
>+		items = g_strsplit (tz, "/", 0);
>+		nitems = g_strv_length (items);

(just a small doubt here: does nitems contain the NULL terminating the array or not?)

>+
>+		ret = system_timezone_write_key_file (ETC_SYSCONFIG_TIMEZONE,
>+						      "TIME_ZONE", items[nitems - 1], error);
>+
>+		if (ret) {
>+			for (i = 0; i < nitems - 1; i++) {
>+				if (i == 0)
>+					zone_info_area = items[0];
>+				else
>+					zone_info_area = g_strjoin ("/", zone_info_area, items[i], NULL);
>+			}

Do this:

buffer = items[nitems - 1]
items[nitems - 1] = NULL
zone_info_area = g_strjoinv ("/", items)
items[nitems - 1] = buffer

>+			ret = system_timezone_write_key_file (ETC_SYSCONFIG_TIMEZONE,
>+							      "ZONE_INFO_AREA", zone_info_area, error);
>+		}
>+
>+	        g_free (zone_info_area);
>+		g_strfreev (items);
>+	}
>+
>+	return ret;
>+}

Created attachment 109837 [details] [review]
second try

Thanks for your comments, but I followed your suggestion and changed a bit PLD Linux configuration file. Now, information about current time zone is stored in one key, but I couldn't change location of this file. Anyway, it makes the patch much simplier.

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

bugzilla.gnome.org is being replaced by gitlab.gnome.org. We are closing all old bug reports in Bugzilla which have not seen updates for many years.

If you can still reproduce this issue in a currently supported version of GNOME (currently that would be 3.38), then please feel free to report it at https://gitlab.gnome.org/GNOME/gnome-panel/-/issues/

Thank you for reporting this issue and we are sorry it could not be fixed.