I remember this issue but I don't remember why it exists. Maybe just unimplemented.

Looking at the code in gi/boxed.c, there are at least a couple tasks:

* boxed_new() and boxed_init() basically have to be merged in some way so the logic can choose to either new() then init() or do both at once by calling a constructor with args. Or maybe new() returns a flag for whether it "consumed" the js argv, and if not init() is also called.

* need to think through some logic for how to choose among overloaded constructors; the hard cases would be if two constructors take the same number of args, and the provided JS args can be plausibly converted to either C arg. 
Worst-case there might be both "new_foo(char*)" and "new_bar(char*)" or something.
At that point it almost seems like g-i needs annotations to disambiguate. I think we should throw an error if there's ambiguity, rather than "picking one at random." This means C API additions could break us though.

* possibly, or alternatively, constructors with something after the "_new" are done as static methods, while "new Foo" in JS always does the plain new(), so

soup_uri_new (const char *uri_string);
--> new imports.gi.Soup.URI(uri_string)

and

soup_uri_new_with_base(SoupURI *base, const char *uri_string
--> imports.gi.Soup.URI.new_with_base(base, uri_string)

A side effect here is that a zero-args constructor, if it's named new_whatever() instead of new(), would NOT be used anymore for the JS "new Foo()" syntax.

Anyway this seems like an OK approach that's faster and more deterministic than what's in gjs already. A patch series could do something like:

patch 1: instead of picking zero-args constructor, invoke the "nothing after the _new()" constructor always and pass args to it. If all constructors are new_something(), then pick the zero-args one if there's only one. If there's >1 zero-args new_something() then throw error.
patch 2: define the other constructors as properties on the JS constructor ("static methods" in JS are props on the constructor such as  URI.new_with_base())
patch 3: maybe do patch 2 for GObject also (but not patch 1; I think the "key-value dict of properties" is more useful and more back compat for GObject)


Alternative/elaboration to discuss: rather than defaulting to the plain _new(), have a flag in gobject-introspection for "default constructor"; if no annotation marks the default constructor, g-i-scanner could mark the plain _new(), but libraries could annotate a _new_somethingorother() as the default if they liked. Useful if plain _new() is deprecated for example. This might also allow us to get the default constructor without iterating over all methods in the object as we do now.


btw, the workaround for this kind of issue is to write a little glue function in C:
 
 create_uri(const char *uri_string) {
    return soup_uri_new(uri_string;
 }

and then introspect that. If you already have a C component to your app this is trivial.

However, fixing gjs itself is very welcome.

note that libsoup is known to not be especially usable from gjs. You'll run into additional problems later on. attachment 138065 [details] [review] on bug 688090 shows some workarounds. In particular, last I checked, gjs let you invoke gobject constructors as though they were static methods, so you can do:

    msg = Soup.Message.new("GET", "http://google.com/");


> Worst-case there might be both "new_foo(char*)" and "new_bar(char*)" or
> something.

not a boxed case, but: gtk_button_new_with_label(), gtk_button_new_with_mnemonic(), and gtk_button_new_from_stock() all take just a const char *.

> * possibly, or alternatively, constructors with something after the "_new" are
> done as static methods

probably a good idea

> Alternative/elaboration to discuss: rather than defaulting to the plain _new(),
> have a flag in gobject-introspection for "default constructor"; if no
> annotation marks the default constructor, g-i-scanner could mark the plain
> _new(), but libraries could annotate a _new_somethingorother() as the default
> if they liked. Useful if plain _new() is deprecated for example.

The Rename-To annotation would work there.

Soup.Message.new worked out actually, (I was trying with Soup.URI.new but it seems that only works with GObjects). Thanks a lot for the input Danw.

Created attachment 155560 [details] [review]
[boxed] Add more flexible constructor support

This patch should be mostly okay, I tested it with Soup.URI() which selects
the right constructor when passing in one or two constructors. Still need
to extend the everything gir so we can test this properly.
I didn't do the parameter type checking/validation as I couldn't easily find
a good boxed constructor to test. This is nicely factored so it can be reused
for GObjects, but I didn't write that part yet.

Created attachment 164716 [details] [review]
[boxed] Add constructors as static methods.

This patch adds all boxed constructors as static methods
on the object. For instance Gtk.IconSet.new_from_pixbuf(pixbuf);

I'd like to point out that supporting this would mean supporting (out caller-allocates) with no problem - just treat (out caller-allocates) as (in) (or (in-out)) and let JS preallocate the struct / boxed to be passed to the function.

This is the pattern used for St.ThemeNode.get_color(), which is
gboolean st_theme_node_get_color (StThemeNode*, gchar*, gboolean, ClutterColor*)
Since this function is not specially annotated, you use it like
let color = new Clutter.Color()
node.get_color(property, inherit, color);

(if instead it were correctly annotated with (out caller-allocates), gjs would segfault)

While this magically works for Clutter.Color (which, according to the gir file, has only one constructor with 4 arguments), it fails with other structs and boxed types.

(Note that we may need a default constructor doing memset(obj, sizeof(obj), 0), for boxed and unregistered structs with no constructor at all, like GtkTreeIter)

Created attachment 175745 [details] [review]
boxed: Add constructors as static methods

This patch adds all boxed constructors as static methods
on the object. For instance Gtk.IconSet.new_from_pixbuf(pixbuf);

This is updated to current GIT HEAD.

Created attachment 207298 [details] [review]
boxed: introduce support for complex constructors

For types that don't have a zero-args constructor, but have a
default constructor (called new), we can now invoke it directly
with "new BoxedType", instead of "BoxedType.new", by delegating
inside the native constructor.
This affects also GVariant, which is made to delegate to an internal
JS function, and thus can be packed with new GLib.Variant()

Created attachment 216613 [details] [review]
Introduce a nicer constructor for GVariant

Inside the native constructor, delegate to the JS packing function,
so that "new GLib.Variant()" is possible.

The general boxed constructor support is not backwards-compatible with
hash-style construction, but this one is (as it just throws in older gjs)

Can anyone review this patch?

(In reply to comment #10)
> Can anyone review this patch?

It's an API break, so we can't introduce it this cycle.

Actually, and assuming Alberto refers to the small GVariant only patch, it's not an API break, just an API addition (new GLib.Variant() current throws, I doubt anyone relies on that...). Still, we're API frozen, so not happening until 3.7.1

It's 3.7.2 now, and I still want to land some form of GBoxed construction this cycle, because it is lame one needs Ns.Boxed.new().
I reworked the patch in a form that is backward compatible with current usage (at least, I can run unmodified shell on top of it), I hope this one will be accepted.

Created attachment 229419 [details] [review]
Add more tests for boxed types

gjs needs a boxed type that cannot be trivially allocated but has also
a complex constructor.

Created attachment 229420 [details] [review]
boxed: introduce support for complex constructors

For types that don't have a zero-args constructor, but have a
default constructor (called new), we can now invoke it directly
with "new BoxedType", instead of "BoxedType.new", by delegating
inside the native constructor.
This affects also GVariant, which is made to delegate to an internal
JS function, and thus can be packed with new GLib.Variant()

Not obsoleting the GVariant patch, but this one effectively replaces that.

Comment on attachment 175745 [details] [review]
boxed: Add constructors as static methods

Obsolete per Giovanni's comment

Review of attachment 229419 [details] [review]:

Minor coding style bits, please commit though after fixing.

::: tests/scanner/regress.c
@@ +1965,3 @@
+
+  boxed = g_slice_new (RegressTestBoxedD);
+  boxed->a_string = g_strdup(a_string);

Missing space between identifier and paren

@@ +1977,3 @@
+
+  ret = g_slice_new (RegressTestBoxedD);
+  ret->a_string = g_strdup(boxed->a_string);

Ditto

@@ +1993,3 @@
+regress_test_boxed_d_get_magic (RegressTestBoxedD *boxed)
+{
+  return strlen(boxed->a_string) + boxed->a_int;

Again

Comment on attachment 229419 [details] [review]
Add more tests for boxed types

Attachment 229419 [details] pushed as 5153efd - Add more tests for boxed types

Review of attachment 229420 [details] [review]:

One major reservation here.  We could consider doing this patch with just the () -> "new" change.

::: gi/boxed.c
@@ +1128,3 @@
+                if (priv->zero_args_constructor < 0 &&
+                    g_callable_info_get_n_args((GICallableInfo*) func_info) == 0)
+                    priv->zero_args_constructor = i;

But see here's what I find really gross about this - what happens when the boxed has two zero argument constructors?  We pick the first one from the order returned by  g_struct_info_get_n_methods()...which in turn is derived from g-ir-scanner, which just sorts them in alphabetical order (hey, at least it has an ordering...).

So if someone later adds a second zero-argument constructor with an alphabetically-earlier name, people's scripts could just start blowing up in a very un-obvious way; the constructor may return a boxed with different properties.

That's the kind of unfortunate "unintentional ABI" that introspection makes way too easy to happen unfortunately =(

I dunno.  Maybe we could add an annotation like (constructor default)?

@@ +1132,3 @@
+                if (priv->default_constructor < 0 &&
+                    strcmp(g_base_info_get_name ((GIBaseInfo*) func_info), "new") == 0)
+                    priv->default_constructor = i;

I'm much more fine with this change, since you can only have one method named "new".

Well, the first zero args constructor is what gjs uses right now, and changing that would break backward compatibility.

(In reply to comment #20)
> Well, the first zero args constructor is what gjs uses right now, and changing
> that would break backward compatibility.

Ah....I missed that.  Urgh.  =/  Ok.

Review of attachment 229420 [details] [review]:

Ok, this looks pretty good then.

Attachment 229420 [details] pushed as 5251248 - boxed: introduce support for complex constructors