i prefer this API:

g_object_class_install_properties (GObjectClass  *class,
                                   GParamSpec   **pspecs,
                                   gint           pspecs_length);

that does this, more or less:

  g_assert (pspecs[0] == NULL);

  for (i = 1; i < pspecs_length; i++)
    g_object_install_property (class, i, pspecs[i]);


could also do it using NULL termination.



weird that we have the gap in position zero, but no biggy, i guess.  is there any reason, though, that we can't make 0 be a valid property id number?

> i prefer this API:
> 
> g_object_class_install_properties (GObjectClass  *class,
>                                    GParamSpec   **pspecs,
>                                    gint           pspecs_length);
> 
> that does this, more or less:
> 
>   g_assert (pspecs[0] == NULL);
> 
>   for (i = 1; i < pspecs_length; i++)
>     g_object_install_property (class, i, pspecs[i]);
> 
> 
> could also do it using NULL termination.

I agree - it's more elegant, and allows us to avoid multiple argument checking (especially for classes installing a lot of properties, like ClutterActor).

> weird that we have the gap in position zero, but no biggy, i guess.  is there
> any reason, though, that we can't make 0 be a valid property id number?

I think GObject special-cases 0, but I have literally no idea why; I should probably check in the sources...(In reply to comment #1)

pspec = 0 is special-cased for an "unattached" pspec and for pspecs installed by interfaces. I would have probably used -1 to indicate this state, but it's too late to change it now.

Created attachment 168738 [details] [review]
gobject: Add install_properties()

Since we added g_object_notify_by_pspec(), an efficient way to install
and notify properties relies on storing the GParamSpec pointers inside
a static arrays, like we do for signal identifiers.

Instead of multiple calls to g_object_class_install_property(), we
should have a single function to take the static array of GParamSpecs
and iterate it.

Comment on attachment 168738 [details] [review]
gobject: Add install_properties()

Looks good to me.
If anything, I'd maybe call the parameter n_pspecs, to match the pspecs that it counts.

Created attachment 168774 [details] [review]
gobject: Add install_properties()

Same as attachment 168738 [details] [review], but addressing Matthias point and adding the symbol to gobject.symbols and the API reference.

sorry for the nitpick, but why did you elect to change the order of the parameters?

most of our APIs put the length field after the pointer...

actually, in libgobject, the length of the array comes before:

gclosure.h:

typedef void (*GClosureMarshal) (GClosure       *closure,
                                 GValue         *return_value,
                               → guint           n_param_values,
                                 const GValue   *param_values,
                                 gpointer        invocation_hint,
                                 gpointer        marshal_data);

gsignal.h:

guint g_signal_newv (const gchar        *signal_name,
                     GType               itype,
                     GSignalFlags        signal_flags,
                     GClosure           *class_closure,
                     GSignalAccumulator  accumulator,
                     gpointer            accu_data,
                     GSignalCMarshaller  c_marshaller,
                     GType               return_type,
                   → guint               n_params,
                     GType              *param_types);

gtype.h:

struct _GTypeValueTable
{
  ...
  gchar* (*collect_value) (GValue       *value,
                         → guint         n_collect_values,
                           GTypeCValue  *collect_values,
                           guint         collect_flags);
  gchar   *lcopy_format;
  gchar* (*lcopy_value)   (const GValue *value,
                         → guint         n_collect_values,
                           GTypeCValue  *collect_values,
                           guint         collect_flags);
}

in gtk there are inconsistencies, sometimes in the same file like in gtkliststore.h: in insert_with_values() the length comes after, but in the constructor and the column type setter for sub-classes, it comes before.

I honestly prefer the length to be before, as it feels natural from a left-to-right reader: you're declaring the size of the following argument.

I have no problems with swapping around, though.

my preference is length-after but your consistency argumentis pretty convincing and consistency (at least within libgobject)  certainly trumps my preference :)

Comment on attachment 168774 [details] [review]
gobject: Add install_properties()

as per meeting

Attachment 168774 [details] pushed as 9cd43d7 - gobject: Add install_properties()

I just saw this commit and I have a couple of questions which wasn't discussed.

Sorry if this is the wrong place or the wrong time to complain/nag/review this.

- If you call install_properties() for a subclass which has super class which  already implements set/get_property vtable methods it will not warn. The solution is to compare the set/get_property methods with the parent method and warn if they don't differ.
- Is the order the properties are installed ever important? If yes then a g_slist_reverse() is missing
- G_PARAM_CONSTRUCT_ONLY | G_PARAM_READWRITE should be an error, or the GParamFlags doc strings should be updated. Could warn about invalid flag combinations for overridden parameters, but that might be overkill.
- Should the GParamFlag validation be tied to install_properties(). Bw-compat might prevent it from being used elsewhere.
- The result of g_type_parent() could be cached. At the very least the parameter to g_type_parent() should be oclass_type which is fetched outside of the loop.

I also wonder if it would make sense to wrap the check for already existing psecs at the begin of install_property_internal() inside a #ifndef G_DISABLE_CHECKS

Also the current doc-blob for g_object_class_install_properties() has a typo and mentions "g_object_notify_pspec()" instead of "g_object_notify_by_pspec()".

Otherwise great stuff!

we should probably open a new bug about improving install_propert(y|ies)...

(In reply to comment #12)
> I just saw this commit and I have a couple of questions which wasn't discussed.
> 
> Sorry if this is the wrong place or the wrong time to complain/nag/review this.
> 
> - If you call install_properties() for a subclass which has super class which 
> already implements set/get_property vtable methods it will not warn. The
> solution is to compare the set/get_property methods with the parent method and
> warn if they don't differ.

that's generally the case any way, isn't it?

actually, since the GObject class init function is going to be called during the initialization phase, I'm not sure how it's possible to have set_property/get_property to be NULL - except by explicitly setting them to NULL in the sub-class.

> - Is the order the properties are installed ever important? If yes then a
> g_slist_reverse() is missing

no, it's not important.

> - G_PARAM_CONSTRUCT_ONLY | G_PARAM_READWRITE should be an error, or the
> GParamFlags doc strings should be updated. Could warn about invalid flag
> combinations for overridden parameters, but that might be overkill.

uh? construct-only does not imply writable: construct-only *requires* writable, and accepts readable.

> - Should the GParamFlag validation be tied to install_properties(). Bw-compat
> might prevent it from being used elsewhere.

see below.

> - The result of g_type_parent() could be cached. At the very least the
> parameter to g_type_parent() should be oclass_type which is fetched outside of
> the loop.

true.

(In reply to comment #13)
> I also wonder if it would make sense to wrap the check for already existing
> psecs at the begin of install_property_internal() inside a #ifndef
> G_DISABLE_CHECKS

we'd have to change install_property_internal() to return a boolean value, otherwise g_return_if_fail() will just go back up one level.

the reason I preferred to duplicate the checks was to make the patch the least intrusive as possible. obviously, the whole property installation check phase can be improved.

> Also the current doc-blob for g_object_class_install_properties() has a typo
> and mentions "g_object_notify_pspec()" instead of "g_object_notify_by_pspec()".

ouch. fixing it.