GNOME.org
 

Re: [PATCH] config: rework code to use GKeyFile instead of GHashTable+GValue


Hi,
there is still one minor problem, which was partially present in the previous implementation:
if the client requests the value for a configuration parameter that is not present in the configuration, it gets a default value and no hint that the parameter did not exist. This was already the case in the previous code, however, when in need to know if the parameter actually existed, the client could use grl_config_get, which would return a GValue (if the parameter existed) or NULL (otherwise). This was used in the filesystem plugin to check the maximum depth allowed for search operations. In this plugin, if this limit is not set through a config parameter, a sane default is used by the plugin, but with this patch this situation can no longer be detected, since grl_config_get does not exist any more.
My proposal to fix this situation is to add one more parameter to the 'get' APIs. The new parameter would be a pointer to a location where the result should be stored, and the function result would be a boolean to let clients know if the parameter did exist in the configuration. 

So, for example, we would have this:
gboolean grl_config_get_string (GrlConfig *config, const gchar *param, gchar **value); 

Instead of this:
gchar *grl_config_get_string (GrlConfig *config, const gchar *param);

Sounds good?

Iago

On Tue, 18 Jan 2011 13:14:26 +0000, llandwerlin gmail com wrote:

From: Lionel Landwerlin <lionel g landwerlin linux intel com>
Signed-off-by: Lionel Landwerlin <lionel g landwerlin linux intel com> 
---
 src/data/grl-config.c     |  118
+++++++++++++++++---------------------------
 src/data/grl-config.h     |   11 ++--
 src/grl-plugin-registry.c |    8 ++-
 3 files changed, 56 insertions(+), 81 deletions(-)

diff --git a/src/data/grl-config.c b/src/data/grl-config.c
index 07866b5..62a4fe5 100644
--- a/src/data/grl-config.c
+++ b/src/data/grl-config.c
@@ -32,6 +32,8 @@
 #include "grl-config.h"
 #include "grl-log.h"

+#define GROUP_NAME "none"
+
 #define GRL_LOG_DOMAIN_DEFAULT  config_log_domain
 GRL_LOG_DOMAIN(config_log_domain);

@@ -39,7 +41,7 @@ GRL_LOG_DOMAIN(config_log_domain);
(G_TYPE_INSTANCE_GET_PRIVATE ((o), GRL_TYPE_CONFIG, GrlConfigPrivate)) 

 struct _GrlConfigPrivate {
-  GHashTable *config;
+  GKeyFile *config;
 };

 static void grl_config_dispose (GObject *object);
@@ -48,15 +50,6 @@ static void grl_config_finalize (GObject *object);
 G_DEFINE_TYPE (GrlConfig, grl_config, G_TYPE_OBJECT);

 static void
-free_val (GValue *val)
-{
-  if (val) {
-    g_value_unset (val);
-    g_free (val);
-  }
-}
-
-static void
 grl_config_class_init (GrlConfigClass *klass)
 {
   GObjectClass *gobject_class = (GObjectClass *)klass;
@@ -71,10 +64,9 @@ static void
 grl_config_init (GrlConfig *self)
 {
   self->priv = GRL_CONFIG_GET_PRIVATE (self);
-  self->priv->config = g_hash_table_new_full (g_str_hash,
-					      g_str_equal,
-					      g_free,
-					      (GDestroyNotify) free_val);
+  self->priv->config = g_key_file_new ();
+
+  g_key_file_load_from_data (self->priv->config, "[]\n", -1,
G_KEY_FILE_NONE, NULL);
 }

 static void
@@ -124,105 +116,87 @@ grl_config_new (const gchar *plugin, const
gchar *source)
 void
grl_config_set (GrlConfig *config, const gchar *param, const GValue *value) 
 {
-  GValue *copy;
   g_return_if_fail (GRL_IS_CONFIG (config));
-  copy = g_new0 (GValue, 1);
-  g_value_init (copy, G_VALUE_TYPE (value));
-  g_value_copy (value, copy);
- g_hash_table_insert (config->priv->config, g_strdup (param), copy); 
+
+  switch (G_VALUE_TYPE (value)) {
+  case G_TYPE_STRING:
+    g_key_file_set_string (config->priv->config, GROUP_NAME, param,
+                           g_value_get_string (value));
+    break;
+
+  case G_TYPE_FLOAT:
+    g_key_file_set_double (config->priv->config, GROUP_NAME, param,
+                           g_value_get_double (value));
+    break;
+
+  case G_TYPE_INT:
+    g_key_file_set_integer (config->priv->config, GROUP_NAME, param,
+                            g_value_get_int (value));
+    break;
+
+  case G_TYPE_BOOLEAN:
+    g_key_file_set_boolean (config->priv->config, GROUP_NAME, param,
+                            g_value_get_boolean (value));
+    break;
+
+  default:
+    g_return_if_reached ();
+    break;
+  }
 }

 void
 grl_config_set_string (GrlConfig *config, const gchar *param, const
gchar *value)
 {
-  GValue v = { 0 };
-  g_value_init (&v, G_TYPE_STRING);
-  g_value_set_string (&v, value);
-  grl_config_set (config, param, &v);
-  g_value_unset (&v);
+ g_key_file_set_string (config->priv->config, GROUP_NAME, param, value); 
 }

 void
grl_config_set_int (GrlConfig *config, const gchar *param, gint value) 
 {
-  GValue v = { 0 };
-  g_value_init (&v, G_TYPE_INT);
-  g_value_set_int (&v, value);
-  grl_config_set (config, param, &v);
+ g_key_file_set_integer (config->priv->config, GROUP_NAME, param, value); 
 }


 void
grl_config_set_float (GrlConfig *config, const gchar *param, gfloat value) 
 {
-  GValue v = { 0 };
-  g_value_init (&v, G_TYPE_FLOAT);
-  g_value_set_float (&v, value);
-  grl_config_set (config, param, &v);
+  g_key_file_set_double (config->priv->config, GROUP_NAME, param,
(gdouble) value);
 }

 void
 grl_config_set_boolean (GrlConfig *config, const gchar *param,
gboolean value)
 {
-  GValue v = { 0 };
-  g_value_init (&v, G_TYPE_BOOLEAN);
-  g_value_set_boolean (&v, value);
-  grl_config_set (config, param, &v);
-}
-
-const GValue *
-grl_config_get (GrlConfig *config, const gchar *param)
-{
-  g_return_val_if_fail (GRL_IS_CONFIG (config), NULL);
-  return g_hash_table_lookup (config->priv->config, param);
+ g_key_file_set_boolean (config->priv->config, GROUP_NAME, param, value); 
 }

-const gchar *
+gchar *
 grl_config_get_string (GrlConfig *config, const gchar *param)
 {
   g_return_val_if_fail (GRL_IS_CONFIG (config), NULL);
-  const GValue *value = grl_config_get (config, param);
-  if (!value || !G_VALUE_HOLDS_STRING (value)) {
-    return NULL;
-  } else {
-    return g_value_get_string (value);
-  }
+  return g_key_file_get_string (config->priv->config, GROUP_NAME,
param, NULL);
 }

 gint
 grl_config_get_int (GrlConfig *config, const gchar *param)
 {
   g_return_val_if_fail (GRL_IS_CONFIG (config), 0);
-  const GValue *value = grl_config_get (config, param);
-  if (!value || !G_VALUE_HOLDS_INT (value)) {
-    return 0;
-  } else {
-    return g_value_get_int (value);
-  }
+  return g_key_file_get_integer (config->priv->config, GROUP_NAME,
param, NULL);
 }

 gfloat
 grl_config_get_float (GrlConfig *config, const gchar *param)
 {
   g_return_val_if_fail (GRL_IS_CONFIG (config), 0.0);
-  const GValue *value = grl_config_get (config, param);
-  if (!value || !G_VALUE_HOLDS_FLOAT (value)) {
-    return 0.0;
-  } else {
-    return g_value_get_float (value);
-  }
+ return (gfloat) g_key_file_get_double (config->priv->config, GROUP_NAME, 
+                                         param, NULL);
 }

 gboolean
 grl_config_get_boolean (GrlConfig *config, const gchar *param)
 {
   g_return_val_if_fail (GRL_IS_CONFIG (config), FALSE);
-  const GValue *value = grl_config_get (config, param);
-  if (!value || !G_VALUE_HOLDS_BOOLEAN (value)) {
-    return FALSE;
-  } else {
-    return g_value_get_boolean (value);
-  }
+  return g_key_file_get_boolean (config->priv->config, GROUP_NAME,
param, NULL);
 }

 /**
@@ -348,7 +322,7 @@ grl_config_set_password(GrlConfig *config, const
gchar *password)
  *
  * Since: 0.1.4
  */
-const gchar *
+gchar *
 grl_config_get_plugin (GrlConfig *config)
 {
   return grl_config_get_string (GRL_CONFIG (config),
@@ -363,7 +337,7 @@ grl_config_get_plugin (GrlConfig *config)
  *
  * Since: 0.1.4
  */
-const gchar *
+gchar *
 grl_config_get_api_key (GrlConfig *config)
 {
   return grl_config_get_string (GRL_CONFIG (config),
@@ -378,7 +352,7 @@ grl_config_get_api_key (GrlConfig *config)
  *
  * Since: 0.1.4
  */
-const gchar *
+gchar *
 grl_config_get_api_token (GrlConfig *config)
 {
   return grl_config_get_string (GRL_CONFIG (config),
@@ -393,7 +367,7 @@ grl_config_get_api_token (GrlConfig *config)
  *
  * Since: 0.1.4
  */
-const gchar *
+gchar *
 grl_config_get_api_secret (GrlConfig *config)
 {
   return grl_config_get_string (GRL_CONFIG (config),
diff --git a/src/data/grl-config.h b/src/data/grl-config.h
index b2a0f63..f0f46d2 100644
--- a/src/data/grl-config.h
+++ b/src/data/grl-config.h
@@ -112,13 +112,13 @@ void grl_config_set_username (GrlConfig
*config, const gchar *secret);
void grl_config_set_password (GrlConfig *config, const gchar *secret); 

-const gchar *grl_config_get_plugin (GrlConfig *config);
+gchar *grl_config_get_plugin (GrlConfig *config);

-const gchar *grl_config_get_api_key (GrlConfig *config);
+gchar *grl_config_get_api_key (GrlConfig *config);

-const gchar *grl_config_get_api_token (GrlConfig *config);
+gchar *grl_config_get_api_token (GrlConfig *config);

-const gchar *grl_config_get_api_secret (GrlConfig *config);
+gchar *grl_config_get_api_secret (GrlConfig *config);

 const gchar *grl_config_get_username (GrlConfig *config);

@@ -139,9 +139,8 @@ void grl_config_set_float (GrlConfig *config,
const gchar *param, gfloat value);

 void grl_config_set_boolean (GrlConfig *config, const gchar *param,
gboolean value);
-const GValue *grl_config_get (GrlConfig *config, const gchar *param);
-const gchar *grl_config_get_string (GrlConfig *config, const gchar *param); +gchar *grl_config_get_string (GrlConfig *config, const gchar *param); 

 gint grl_config_get_int (GrlConfig *config, const gchar *param);

diff --git a/src/grl-plugin-registry.c b/src/grl-plugin-registry.c
index c33657f..6e20f0d 100644
--- a/src/grl-plugin-registry.c
+++ b/src/grl-plugin-registry.c
@@ -584,7 +584,7 @@ grl_plugin_registry_load_all (GrlPluginRegistry
*registry, GError **error)
"All configured plugin paths are invalid. " \ 
                           "Failed to load plugins.");
   }
-
+
   return loaded_one;
 }

@@ -882,7 +882,7 @@ grl_plugin_registry_add_config (GrlPluginRegistry
*registry,
                                 GrlConfig *config,
                                 GError **error)
 {
-  const gchar *plugin_id;
+  gchar *plugin_id;
   GList *configs = NULL;

   g_return_val_if_fail (config != NULL, FALSE);
@@ -899,7 +899,7 @@ grl_plugin_registry_add_config (GrlPluginRegistry
*registry,
     }
     return FALSE;
   }
-
+
configs = g_hash_table_lookup (registry->priv->configs, plugin_id); 
   if (configs) {
     /* Notice that we are using g_list_append on purpose to avoid
@@ -912,6 +912,8 @@ grl_plugin_registry_add_config (GrlPluginRegistry
*registry,
 			 configs);
   }

+  g_free (plugin_id);
+
   return TRUE;
 }
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]