Created attachment 63200 [details] [review]
patch

Note, this patch is leaving the GValue creation and destruction to the caller, following the behavior of g_object_set_property.

Created attachment 63201 [details] [review]
v2: header and exported symbol included

Created attachment 63205 [details] [review]
v2: header and exported symbol included

Correct one attached now, I have too many patches in my home directory...

thanks Johan.
would you mind also writing a test program for this function, so we can get it properly covered by the test suite?

Created attachment 75371 [details] [review]
v3: updated for head, with test and documentation included

Comment on attachment 75371 [details] [review]
v3: updated for head, with test and documentation included

>diff -u -d -r1.47 objects.sgml
>--- docs/reference/gobject/tmpl/objects.sgml	5 Jun 2006 17:34:49 -0000	1.47
>+++ docs/reference/gobject/tmpl/objects.sgml	25 Oct 2006 14:14:50 -0000
>@@ -999,6 +999,19 @@
> @value: return location for the property value
> 
> 
>+<!-- ##### FUNCTION g_object_set_properties ##### -->
>+<para>
>+Sets properties on an object.
>+</para>
>+<para>
>+Note that g_object_set_properties() is really intended for language
>+bindings, g_object_set() is much more convenient for C programming.

i don't think that disclaimer is accurate, the interface is nice and good to use for generic code beyond language bindings (in know that gimp in particular deals with arrays of GParameter for instance). there can conceivably be a disclaimer here that says that g_object_set() is easier to use for most C programmers, but that's about it.

>+</para>
>+
>+@object: a #GObject
>+@n_parameters: number of parameters to set
>+@parameters: parameters
>+
> <!-- ##### FUNCTION g_object_new_valist ##### -->
> <para>
> Creates a new instance of a #GObject subtype and sets its properties.
>Index: gobject/gobject.c
>===================================================================
>RCS file: /cvs/gnome/glib/gobject/gobject.c,v
>retrieving revision 1.82
>diff -u -d -r1.82 gobject.c
>--- gobject/gobject.c	30 Sep 2006 13:59:01 -0000	1.82
>+++ gobject/gobject.c	25 Oct 2006 14:14:52 -0000
>@@ -1325,6 +1325,59 @@
>   g_object_unref (object);
> }
> 
>+void
>+g_object_set_properties (GObject    *object,
>+                         guint       n_parameters,
>+                         GParameter *parameters)
>+{
>+  GObjectNotifyQueue *nqueue;
>+  int i;
>+
>+  g_return_if_fail (G_IS_OBJECT (object));
>+  
>+  g_object_ref (object);
>+  nqueue = g_object_notify_queue_freeze (object, &property_notify_context);
>+  
>+  for (i = 0; i < n_parameters; i++)
>+    {
>+      GValue *value = &parameters[i].value;
>+      const gchar *name = parameters[i].name;
>+      GParamSpec *pspec;
>+      
>+      pspec = g_param_spec_pool_lookup (pspec_pool,
>+                                        name,
>+                                        G_OBJECT_TYPE (object),
>+                                        TRUE);
>+      if (!pspec)
>+        {
>+          g_warning ("%s: object class `%s' has no property named `%s'",
>+                     G_STRFUNC,
>+                     G_OBJECT_TYPE_NAME (object),
>+                     name);
>+          break;
>+        }
>+      if (!(pspec->flags & G_PARAM_WRITABLE))
>+        {
>+          g_warning ("%s: property `%s' of object class `%s' is not writable",
>+                     G_STRFUNC,
>+                     pspec->name,
>+                     G_OBJECT_TYPE_NAME (object));
>+          break;
>+        }
>+      if (pspec->flags & (G_PARAM_CONSTRUCT | G_PARAM_CONSTRUCT_ONLY))

this condition was fixed in g_object_set_valist(), please update your patch accordingly.

>+        {
>+          g_warning ("%s: construct property \"%s\" for object `%s' can't be set after construction",
>+                     G_STRFUNC, pspec->name, G_OBJECT_TYPE_NAME (object));
>+          break;
>+        }
>+
>+      object_set_property (object, pspec, value, nqueue);
>+    }
>+
>+  g_object_notify_queue_thaw (object, nqueue);
>+  g_object_unref (object);
>+}
>+
> gpointer
> g_object_connect (gpointer     _object,
> 		  const gchar *signal_spec,
>Index: gobject/gobject.h
>===================================================================
>RCS file: /cvs/gnome/glib/gobject/gobject.h,v
>retrieving revision 1.38
>diff -u -d -r1.38 gobject.h
>--- gobject/gobject.h	23 Aug 2006 08:46:21 -0000	1.38
>+++ gobject/gobject.h	25 Oct 2006 14:14:52 -0000
>@@ -174,6 +174,9 @@
> void        g_object_get_property             (GObject        *object,
> 					       const gchar    *property_name,
> 					       GValue         *value);
>+void        g_object_set_properties           (GObject        *object,
>+					       guint	       n_parameters,
>+					       GParameter     *properties);


hm, the patch mostly looks good, but why is there only a _set_ variant and not also a _get_ variant of this function?

> void        g_object_freeze_notify            (GObject        *object);
> void        g_object_notify                   (GObject        *object,
> 					       const gchar    *property_name);
>Index: tests/gobject/properties.c
>===================================================================
>RCS file: tests/gobject/properties.c
>diff -N tests/gobject/properties.c
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ tests/gobject/properties.c	25 Oct 2006 14:14:52 -0000
>@@ -0,0 +1,185 @@
>+/* GObject - GLib Type, Object, Parameter and Signal Library
>+ * Copyright (C) 2006 Johan Dahlin
>+ *
>+ * This library is free software; you can redistribute it and/or
>+ * modify it under the terms of the GNU Lesser General Public
>+ * License as published by the Free Software Foundation; either
>+ * version 2 of the License, or (at your option) any later version.
>+ *
>+ * This library is distributed in the hope that it will be useful,
>+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
>+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>+ * Lesser General Public License for more details.
>+ *
>+ * You should have received a copy of the GNU Lesser General
>+ * Public License along with this library; if not, write to the
>+ * Free Software Foundation, Inc., 59 Temple Place, Suite 330,
>+ * Boston, MA 02111-1307, USA.
>+ */
>+
>+#include <string.h>
>+
>+#include <glib-object.h>
>+
>+#define TEST_TYPE_OBJECT          (test_object_get_type ())
>+typedef struct _TestObject        TestObject;
>+typedef struct _TestObjectClass   TestObjectClass;
>+
>+enum {
>+  PROP_0,
>+  PROP_STRING,
>+  PROP_INT,
>+};
>+
>+struct _TestObject
>+{
>+  GObject parent_instance;
>+  gchar * prop_string;
>+  int prop_int;
>+};
>+
>+struct _TestObjectClass
>+{
>+  GObjectClass parent_class;
>+};
>+
>+G_DEFINE_TYPE (TestObject, test_object, G_TYPE_OBJECT);
>+
>+static void
>+test_set_property (GObject         *object,
>+                   guint            prop_id,
>+                   const GValue    *value,
>+                   GParamSpec      *pspec)
>+{
>+  TestObject *test = (TestObject*)object;
>+
>+  switch (prop_id)
>+    {
>+    case PROP_STRING:
>+      test->prop_string = g_strdup (g_value_get_string (value));
>+      break;
>+    case PROP_INT:
>+      test->prop_int = g_value_get_int (value);
>+      break;
>+    default:
>+      G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);
>+      break;
>+    }
>+}
>+
>+static void
>+test_get_property (GObject         *object,
>+                   guint            prop_id,
>+                   GValue          *value,
>+                   GParamSpec      *pspec)
>+{
>+  TestObject *test = (TestObject*)object;
>+  
>+  switch (prop_id)
>+    {
>+    case PROP_STRING:
>+      g_value_set_string (value, test->prop_string);
>+      break;
>+    case PROP_INT:
>+      g_value_set_int (value, test->prop_int);
>+      break;
>+    default:
>+      G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);
>+      break;
>+    }
>+}
>+
>+static void
>+test_finalize (GObject *object)
>+{
>+  TestObject *test = (TestObject*)object;
>+
>+  g_free (test->prop_string);
>+  
>+  G_OBJECT_CLASS (test_object_parent_class)->finalize (object);
>+}
>+
>+static void
>+test_object_class_init (TestObjectClass *class)
>+{
>+  GObjectClass *gobject_class = G_OBJECT_CLASS (class);
>+
>+  gobject_class->finalize = test_finalize;
>+  gobject_class->set_property = test_set_property;
>+  gobject_class->get_property = test_get_property;
>+
>+  g_object_class_install_property (gobject_class,
>+                                   PROP_STRING,
>+                                   g_param_spec_string ("string", NULL, NULL, NULL,
>+                                                        G_PARAM_READWRITE));
>+
>+  g_object_class_install_property (gobject_class,
>+                                   PROP_INT,
>+                                   g_param_spec_int ("int", NULL, NULL,
>+                                                     G_MININT, G_MAXINT, 0,
>+                                                     G_PARAM_READWRITE));
>+}
>+
>+static void
>+test_object_init (TestObject *test_object)
>+{
>+}
>+
>+static void
>+test_set_properties (GObject *test)
>+{
>+  GParameter *parameters;
>+  GValue *values;
>+
>+  parameters = g_new0 (GParameter, 2);
>+  values = g_new0 (GValue, 2);
>+
>+  g_value_init (&values[0], G_TYPE_STRING);
>+  g_value_set_static_string (&values[0], "foobar");
>+  parameters[0].name = "string";
>+  parameters[0].value = values[0];
>+    
>+  g_value_init (&values[1], G_TYPE_INT);
>+  g_value_set_int (&values[1], 42);
>+  parameters[1].name = "int";
>+  parameters[1].value = values[1];
>+
>+  g_object_set_properties (test, 2, parameters);

since you wrote a fairly generic proeprty test program here, what about extending it at this point to also run the same tests for the varargs variants and a _get_properties version?

>+
>+  g_value_unset (&values[0]);
>+  g_value_unset (&values[1]);
>+  g_free (values);
>+  g_free (parameters);
>+  
>+  g_assert (((TestObject*)test)->prop_int == 42);
>+  g_assert (!strcmp (((TestObject*)test)->prop_string, "foobar"));
>+}
>+
>+int
>+main (int   argc,
>+      char *argv[])
>+{
>+  GObject *obj;
>+  char *s;
>+  int i;
>+  
>+  g_log_set_always_fatal (g_log_set_always_fatal (G_LOG_FATAL_MASK) |
>+                          G_LOG_LEVEL_WARNING |
>+                          G_LOG_LEVEL_CRITICAL);
>+  g_type_init ();
>+
>+  obj = g_object_new (TEST_TYPE_OBJECT, NULL);
>+
>+  /* Default values */
>+  g_object_get (obj, "int", &i, NULL);
>+  g_assert (i == 0);
>+  
>+  g_object_get (obj, "string", &s, NULL);
>+  g_assert (s == NULL);
>+
>+  test_set_properties (obj);
>+  
>+  g_object_unref (obj);
>+  
>+  return 0;
>+}

thanks for the new patch, looks fairly good to go in already, except for the CONSTRUCT properties logic fix and that we really should have a _Get_variant as well.

(In reply to comment #6)

> >+<!-- ##### FUNCTION g_object_set_properties ##### -->
> >+<para>
> >+Sets properties on an object.
> >+</para>
> >+<para>
> >+Note that g_object_set_properties() is really intended for language
> >+bindings, g_object_set() is much more convenient for C programming.
> 
> i don't think that disclaimer is accurate, the interface is nice and good to
> use for generic code beyond language bindings (in know that gimp in particular
> deals with arrays of GParameter for instance). there can conceivably be a
> disclaimer here that says that g_object_set() is easier to use for most C
> programmers, but that's about it.

This was just copied from documentation of set/get_property. I think it's better to keep it consistent. Sure I agree that the language might be a bit strong, they're actually useful for C programming as well.

Created attachment 87574 [details] [review]
v4: Update to trunk, add g_object_get_properties, extends test to all g_gobject_set/get functions

Comment on attachment 87574 [details] [review]
v4: Update to trunk, add g_object_get_properties, extends test to all g_gobject_set/get functions

>--- docs/reference/gobject/tmpl/objects.sgml	(revisão 5484)
>+++ docs/reference/gobject/tmpl/objects.sgml	(cópia de trabalho)

can you please fix the wording here then to be less strong, and fix it consistently?


>--- tests/gobject/properties.c	(revisão 0)
>+++ tests/gobject/properties.c	(revisão 0)

thanks. nice extensive property set/get test case now ;)

>--- gobject/gobject.c	(revisão 5484)
>+++ gobject/gobject.c	(cópia de trabalho)
>@@ -1330,6 +1330,104 @@ g_object_get_property (GObject	   *objec
>   g_object_unref (object);
> }
> 
>+void
>+g_object_get_properties (GObject    *object,
>+                         guint       n_parameters,
>+                         GParameter *parameters)
>+{
>+  int i;
>+
>+  g_return_if_fail (G_IS_OBJECT (object));
>+  
>+  g_object_ref (object);
>+  
>+  for (i = 0; i < n_parameters; i++)
>+    {
>+      GValue *value = &parameters[i].value;
>+      const gchar *name = parameters[i].name;
>+      GParamSpec *pspec;
>+      
>+      pspec = g_param_spec_pool_lookup (pspec_pool,
>+                                        name,
>+                                        G_OBJECT_TYPE (object),
>+                                        TRUE);
>+      if (!pspec)
>+	{
>+	  g_warning ("%s: object class `%s' has no property named `%s'",
>+		     G_STRFUNC,
>+		     G_OBJECT_TYPE_NAME (object),
>+		     name);
>+	  break;

this can be s/break/continue/

>+	}
>+      if (!(pspec->flags & G_PARAM_READABLE))
>+	{
>+	  g_warning ("%s: property `%s' of object class `%s' is not readable",
>+		     G_STRFUNC,
>+		     pspec->name,
>+		     G_OBJECT_TYPE_NAME (object));
>+	  break;

this can be s/break/continue/

the reason we have "break" in the get_valist variant is that we can't continue to parse the property arguments once an individual property wasn't recognized.
with the array at hand though, we implement best effort and continue to get properties.

>+	}
>+      
>+      object_get_property (object, pspec, value);
>+
>+    }
>+
>+  g_object_unref (object);
>+}
>+
>+void
>+g_object_set_properties (GObject    *object,
>+                         guint       n_parameters,
>+                         GParameter *parameters)
>+{
>+  GObjectNotifyQueue *nqueue;
>+  int i;
>+
>+  g_return_if_fail (G_IS_OBJECT (object));
>+  
>+  g_object_ref (object);
>+  nqueue = g_object_notify_queue_freeze (object, &property_notify_context);
>+  
>+  for (i = 0; i < n_parameters; i++)
>+    {
>+      GValue *value = &parameters[i].value;
>+      const gchar *name = parameters[i].name;
>+      GParamSpec *pspec;
>+      
>+      pspec = g_param_spec_pool_lookup (pspec_pool,
>+                                        name,
>+                                        G_OBJECT_TYPE (object),
>+                                        TRUE);
>+      if (!pspec)
>+        {
>+          g_warning ("%s: object class `%s' has no property named `%s'",
>+                     G_STRFUNC,
>+                     G_OBJECT_TYPE_NAME (object),
>+                     name);
>+          break;

this should stay "break" btw, because the settable properties in the array may have a certain dependency, so continuing to set properties could be possibly harmfull.

>+        }
>+      if (!(pspec->flags & G_PARAM_WRITABLE))
>+        {
>+          g_warning ("%s: property `%s' of object class `%s' is not writable",
>+                     G_STRFUNC,
>+                     pspec->name,
>+                     G_OBJECT_TYPE_NAME (object));
>+          break;
>+        }
[...]

the rest looks good. please apply and attach a new patch for objects.sgml.

This was fixed in bug #709865, which introduced the g_object_getv() and g_object_setv() methods, as well as a new g_object_new_with_properties() constructor.

*** This bug has been marked as a duplicate of bug 709865 ***