Merge lp:~jjardon/indicator-datetime/fix-844741 into lp:indicator-datetime/0.3

[Javier Jardón]

[lp:~jjardon/indicator-datetime/fix-844741]

140. By Javier Jardón on 2011-09-30

timezone-completion: Do not get the version with every string request

[Andrea Cimitan (cimi)]

review: Approve

[Michal Hruby (mhr3)]

Thanks for the patch!

It's definitely better this way, although I think it'd be best if it were executed lazily - ie keep the get_version () method and set the priv->version variable there the first time it's used, otherwise "if (priv->version != NULL)" just return it.

[lp:~jjardon/indicator-datetime/fix-844741]

141. By Javier Jardón on 2011-10-04

timezone-completion: get the OS version lazily

[Javier Jardón (jjardon)]

Hi Michal, Here a new patch with your comments

review: Needs Resubmitting

[Michal Hruby (mhr3)]

My bad, I didn't actually notice the first time that the version variable in get_version is declared as static, which means that the process was spawned just once, although this could cause threading issues, it should be fairly safe, the only problem was the extra "g_free(version)", which should actually cause a segfault pretty much everytime (I wonder why it doesn't).

I think that this late in the cycle we should just remove the extra g_free and perhaps apply this thread-safe fix after release.

[Charles Kerr (charlesk)]

IMO this is overkill. The original get_version() function clearly intended to call lsb_release once and privately cache the result, so the only needed fix is to remove the g_free() and to follow the idiom by making get_version() return const.

review: Disapprove

[Charles Kerr (charlesk)]

Revision 149 fixes this in lp:indicator-datetime so I'm going to close this merge request.

Unmerged revisions

141. By Javier Jardón on 2011-10-04

timezone-completion: get the OS version lazily

140. By Javier Jardón on 2011-09-30

timezone-completion: Do not get the version with every string request

=== modified file 'src/timezone-completion.c'
--- src/timezone-completion.c	2011-09-06 12:47:37 +0000
+++ src/timezone-completion.c	2011-10-04 19:52:25 +0000
@@ -44,6 +44,7 @@
   guint          keypress_id;
   GCancellable * cancel;
   gchar *        request_text;
+  gchar *        version;
   GHashTable *   request_table;
 };
 
@@ -312,19 +313,17 @@
 }
 
 static gchar *
-get_version (void)
+get_version (TimezoneCompletion *self)
 {
-  static gchar *version = NULL;
-
-  if (version == NULL) {
-    gchar *stdout = NULL;
-    g_spawn_command_line_sync ("lsb_release -rs", &stdout, NULL, NULL, NULL);
-
-    if (stdout != NULL)
-      version = g_strstrip (stdout);
-    else
-      version = g_strdup("");
-  }
+  TimezoneCompletionPrivate *priv = self->priv;
+  gchar *version = NULL;
+
+  g_spawn_command_line_sync ("lsb_release -rs", &version, NULL, NULL, NULL);
+
+  if (version != NULL)
+    priv->version = g_strstrip (version);
+  else
+    priv->version = g_strdup("");
 
   return version;
 }
@@ -351,11 +350,11 @@
   priv->request_text = g_strdup (text);
 
   gchar * escaped = g_uri_escape_string (text, NULL, FALSE);
-  gchar * version = get_version ();
   gchar * locale = get_locale ();
-  gchar * url = g_strdup_printf (GEONAME_URL, escaped, version, locale);
+  if (priv->version == NULL)
+    get_version (completion);
+  gchar * url = g_strdup_printf (GEONAME_URL, escaped, priv->version, locale);
   g_free (locale);
-  g_free (version);
   g_free (escaped);
 
   GFile * file =  g_file_new_for_uri (url);
@@ -609,6 +608,7 @@
   priv = self->priv;
 
   priv->initial_model = GTK_TREE_MODEL (get_initial_model ());
+  priv->version = NULL;
 
   g_object_set (G_OBJECT (self),
                 "text-column", TIMEZONE_COMPLETION_NAME,
@@ -673,6 +673,11 @@
     priv->request_text = NULL;
   }
 
+  if (priv->version != NULL) {
+    g_free (priv->version);
+    priv->version = NULL;
+  }
+
   if (priv->request_table != NULL) {
     g_hash_table_destroy (priv->request_table);
     priv->request_table = NULL;