Re: [gnome-db] gda_holder_set_value_static_str

[Massimo Cora' <maxcvs email it>]

Hi Vivien,

Vivien Malerba wrote:
> 
> I've applied the patch because it does still pass the checks, but I've
> got a few remarks which, I'm sure you can correct quickly ;)
> * once the is_freeable flag is set to FALSE, there is no way it can be
> back to TRUE

it can be reset to TRUE if a user calls gda_holder_take_value ().

> * the  real_gda_holder_set_const_value() function seems to return NULL
> all the time (the inline doc is not helpfull)

ok I slightly modified the doc and added a gboolean value_changed so
that we always return the previous stored value, or NULL in case of error.

> * I'm not sure the copy function is correct because it both copies the
> is_freeable flag and the priv->value
> 

well, say for instance that we create a GValue with a static string. We
should copy it with the is_freeable flag set to false, or we may have
some problems trying to free its value.
I see we don't use it into gda-holder.c or within the *take_value
functions. Probably we should warn user of this and avoid to copy the
is_freeable?

thanks and regards,
Massimo



Index: libgda/gda-holder.c
===================================================================
--- libgda/gda-holder.c	(revision 3221)
+++ libgda/gda-holder.c	(working copy)
@@ -816,6 +816,10 @@
  * of which can prevent the change from happening) which can be connected to to have a greater control
  * of which values @holder can have, or implement some business rules.
  *
+ * Note3: if user previously set this holder with gda_holder_take_static_value () the GValue
+ * stored internally will be forgiven and replaced by the @value. User should then
+ * take care of the 'old' static GValue.
+ *
  * Returns: TRUE if value has been set
  */
 gboolean
@@ -910,6 +914,8 @@
 	/* new valid status */
 	holder->priv->invalid_forced = FALSE;
 	holder->priv->valid = newvalid;
+	/* we're setting a non-static value, so be sure to flag is as freeable */
+	holder->priv->is_freeable = TRUE;
 
 	/* check is the new value is the default one */
 	holder->priv->default_forced = FALSE;
@@ -953,7 +959,8 @@
 }
 
 static GValue *
-real_gda_holder_set_const_value (GdaHolder *holder, const GValue *value, GError **error)
+real_gda_holder_set_const_value (GdaHolder *holder, const GValue *value, 
+								 gboolean *value_changed, GError **error)
 {
 	gboolean changed = TRUE;
 	gboolean newvalid;
@@ -972,11 +979,11 @@
 	current_val = gda_holder_get_value (holder);
 	if (current_val == value)
 		changed = FALSE;
-	else if ((!current_val || gda_value_is_null ((GValue *)current_val)) && newnull)
+	else if ((!current_val || gda_value_is_null (current_val)) && newnull) 
 		changed = FALSE;
 	else if (value && current_val &&
-		 (G_VALUE_TYPE (value) == G_VALUE_TYPE ((GValue *)current_val)))
-		changed = gda_value_differ (value, (GValue *)current_val);
+		 (G_VALUE_TYPE (value) == G_VALUE_TYPE (current_val))) 
+		changed = gda_value_differ (value, current_val);
 		
 	/* holder's validity */
 	newvalid = TRUE;
@@ -994,7 +1001,7 @@
 		newvalid = FALSE;
 		changed = TRUE;
 	}
-
+/*
 #ifdef DEBUG_HOLDER
 	g_print ("Changed holder %p (%s): value %s --> %s \t(type %d -> %d) VALID: %d->%d CHANGED: %d\n", 
 		 holder, holder->priv->id,
@@ -1004,13 +1011,24 @@
 		 value ? G_VALUE_TYPE (value) : 0, 
 		 was_valid, newvalid, changed);
 #endif
+*/
+	
 
 	/* end of procedure if the value has not been changed, after calculating the holder's validity */
 	if (!changed) {
 		holder->priv->invalid_forced = FALSE;
 		holder->priv->valid = newvalid;
-		return NULL;
+#ifdef DEBUG_HOLDER		
+		g_print ("Holder is not changed, returning NULL\n");
+#endif		
+
+		/* set the changed status */
+		*value_changed = FALSE;
+		return holder->priv->value;
 	}
+	else {
+		*value_changed = TRUE;
+	}
 
 	/* check if we are allowed to change value */
 	GError *lerror = NULL;
@@ -1024,6 +1042,7 @@
 	/* new valid status */
 	holder->priv->invalid_forced = FALSE;
 	holder->priv->valid = newvalid;
+	/* we're setting a static value, so be sure to flag is as unfreeable */
 	holder->priv->is_freeable = FALSE;
 
 	/* check is the new value is the default one */
@@ -1042,7 +1061,8 @@
 		g_print ("Holder %p is alias of holder %p => propagating changes to holder %p\n",
 			 holder, holder->priv->full_bind, holder->priv->full_bind);
 #endif
-		return real_gda_holder_set_const_value (holder->priv->full_bind, value, error);
+		return real_gda_holder_set_const_value (holder->priv->full_bind, value, 
+												value_changed, error);
 	}
 	else {
 		if (holder->priv->value) {
@@ -1052,7 +1072,7 @@
 
 		if (value) {
 			if (newvalid) {
-				holder->priv->value = value;
+				holder->priv->value = (GValue*)value;
 			}
 		}
 
@@ -1066,6 +1086,7 @@
  * gda_holder_take_static_value
  * @holder: a #GdaHolder object
  * @value: a const value to set the holder to
+ * @value_changed: a boolean set with TRUE if the value changes, FALSE elsewhere.
  * @error: a place to store errors, or %NULL
  *
  * Sets the const value within the holder. If @holder is an alias for another
@@ -1084,15 +1105,18 @@
  * of which can prevent the change from happening) which can be connected to to have a greater control
  * of which values @holder can have, or implement some business rules.
  *
- * Returns: 
+ * Returns: NULL if an error occurred or if the previous GValue was NULL itself. It returns
+ * the static GValue user set previously, so that he can free it.
+ *
  */
 GValue *
-gda_holder_take_static_value (GdaHolder *holder, const GValue *value, GError **error)
+gda_holder_take_static_value (GdaHolder *holder, const GValue *value, gboolean *value_changed,
+							  GError **error)
 {
 	g_return_val_if_fail (GDA_IS_HOLDER (holder), FALSE);
 	g_return_val_if_fail (holder->priv, FALSE);
 
-	return real_gda_holder_set_const_value (holder, (GValue*) value, error);
+	return real_gda_holder_set_const_value (holder, value, value_changed, error);
 }
 
 /**
Index: libgda/gda-holder.h
===================================================================
--- libgda/gda-holder.h	(revision 3221)
+++ libgda/gda-holder.h	(working copy)
@@ -75,7 +75,7 @@
 gchar              *gda_holder_get_value_str           (GdaHolder *holder, GdaDataHandler *dh);
 gboolean            gda_holder_set_value               (GdaHolder *holder, const GValue *value, GError **error);
 gboolean            gda_holder_take_value              (GdaHolder *holder, GValue *value, GError **error);
-GValue             *gda_holder_take_static_value       (GdaHolder *holder, const GValue *value, GError **error);
+GValue             *gda_holder_take_static_value       (GdaHolder *holder, const GValue *value, gboolean *value_changed, GError **error);
 gboolean            gda_holder_set_value_str           (GdaHolder *holder, GdaDataHandler *dh, const gchar *value, GError **error);
 
 const GValue       *gda_holder_get_default_value       (GdaHolder *holder);
Index: ChangeLog
===================================================================
--- ChangeLog	(revision 3222)
+++ ChangeLog	(working copy)
@@ -1,3 +1,11 @@
+2008-10-02  Massimo Cora'  <maxcvs email it>
+
+	* libgda/gda-holder.c (real_gda_holder_set_value),
+	(real_gda_holder_set_const_value), (gda_holder_take_static_value):
+	* libgda/gda-holder.h:
+	added a changed_value boolean parameter on gda_holder_take_static_value.
+	Fixed some drawbacks with is_freeable flag.
+
 2008-10-02  Vivien Malerba <malerba gnome-db org>
 
 	* doc/C/Makefile.am: don't forget to distribute the data_select.xml file so