Created attachment 122530 [details] [review]
patch

Generalize it, enough to support GTypes. Test added for that, and Gtk.ListStore.set_column_types() also works now.

Great, thanks,

    TYPE_STRING = 64;
    TYPE_GINT = 64;

This is a bit confusing... are those the real GType values? (I guess not if they are both 64)

 arguments = g_new0(GArgument, length+1);

Should the function still return void* if it will always be GArgument*

  g_free(arguments);

When this is freed, you need to release each individual GArgument* as well, right?

         for (i = 0; i < g_type_info_get_array_length (type_info); i++)
             {

Brace shouldn't be on its own line

                            /* no way to recover here, and errors should
                             * not be possible.
                             */

Should release_arg_internal() return a boolean if it can never really fail?

Comment on attachment 122530 [details] [review]
patch

>+function testGType() {
>+    TYPE_STRING = 64;
>+    TYPE_GINT = 64;
>+    assertEquals(1, Everything.test_gtype_in(1, [TYPE_STRING]));
>+    assertEquals(2, Everything.test_gtype_in(2, [TYPE_STRING, TYPE_GINT]));
>+}
[...]
>+    case GI_TYPE_TAG_GTYPE:
>     case GI_TYPE_TAG_INT16: {
>         gint32 i;
>         if (!JS_ValueToInt32(context, value, &i))

Hmm, I think we need to figure out how to export GTypes, both builtin and dynamic ones I guess. IMO should deal with it separately and use strings/ints/booleans/arrays for testing the array stuff here rather than sneak int-to-gtype conversion in. (Besides, GType is sizeof(long) or so, so treating it like INT16 is going to be problematic.)


>-    if (element_type == GI_TYPE_TAG_UTF8) {
>-        return gjs_array_to_strv (context, array_value, length, arr_p);

gjs_array_to_strv() is now unused and could be deleted as well?

Created attachment 122589 [details] [review]
patch v2

With comments above addressed:
- gjs_array_to_stringv removed, this turned out to be non-trivial, I had to change some apis to allow this
- GType removed, I'll handle that in another bug.
- gjs_array_to_array should be leak free now, gjs_g_arg_release should do the right thing and release both the list and all individual arguments in the normal and error code path.
- gjs_arg_release_internal can currently only fail if you pass in a bad type tag, not sure if it make sense to change it to raise exceptions in case of errors, it's probably not something you'd like to catch in user code. Should either way to be done in another bug.

Created attachment 122729 [details] [review]
v3:

The old patch didn't actually work for arrays with more than one value.
I've reworked it to allocate the right amount of memory and added proper tests for strv, int, float and double.
This patch requires some modifications to gobject-introspection I haven't committed yet. I'm wondering if it's okay to implicitly use the ffi api.

(In reply to comment #5)
> Created an attachment (id=122729) [edit]
> v3:
> 
> The old patch didn't actually work for arrays with more than one value.
> I've reworked it to allocate the right amount of memory and added proper tests
> for strv, int, float and double.
> This patch requires some modifications to gobject-introspection I haven't
> committed yet. I'm wondering if it's okay to implicitly use the ffi api.
> 

This patch leaks and triggers a couple of valgrind warnings/errors. I have a new patch on another computer which fixes that with the exception that string array arguments still leaks. I think it would make sense for that patch to go in, and fix the string array leaking in another bug as the patch is already growing pretty big.

Comment on attachment 122729 [details] [review]
v3:

The patch no longer applies as r111 renamed and made gjs_value_to_g_arg_with_type_info public

Created attachment 125065 [details] [review]
v4: 

This is the latest patch I found in my tree, needs to be ported to current trunk.

*** Bug 609283 has been marked as a duplicate of this bug. ***

*** Bug 584568 has been marked as a duplicate of this bug. ***

Created attachment 153318 [details] [review]
Generalise array support

Here's my best first-effort at updating this patch to apply against master. Note, that arrays of GValue still don't seem to work, so I've most likely gotten something wrong - I've not spent any time debugging or testing this, beyond just getting it to build, but do intend to do so.

I'd really appreciate the comment/review of the original patch author, it was a bit of a task getting this to apply to master and I may have gotten some things wrong.

Chris: I can't really remember all the details. But I'll be happy as long as you include unittests for all the relevant features and make sure that it doesn't introduce any additional memory leaks (check using valgrind).

With this code, GValue arrays end up getting returned as arrays to pointers of GValues.

It seems like we'll need to special-case GValue for this to work, does that sound correct/reasonable?

Special casing GValue sounds fine to me.

Created attachment 153376 [details] [review]
Generalise array support (and special case GValue)

This patch is an updated version of #153318 that fixes the call of gjs_array_to_array from gi/value.c (probably) and also special cases GValue in gjs_array_to_array so that arrays of GValues as arguments work correctly.

I'm not sure if the problem happens just with GValues, or if arrays of all structs as arguments need to be special-cased in this way, but this at least makes clutter_actor_animatev work correctly.

Again, I've only done very limited testing on this, so any feedback would be greatly appreciated - I don't have as much time as I'd like to work on this :(

I'll try to add/run any necessary test cases asap.

I'd quite like to push this to a branch if possible - I have a gnome account already, would that be ok? Any suggested branch names?

I've pushed this into the branch 'generalised-array-args'.

hi

is anyone still working on this?

At least the GValue parsing part of this bug is still relevant with latest GJS master, and makes e.g. gtk_list_store_set_valuesv() impossible to use from JS.

We implement every kind of array known to gobject-introspection (including flat GValue arrays) nowadays, and gtk_list_store_set_valuesv works. I'd say this is fixed.

This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.