I guess we should use the default constructor (*_new) when instantiating structs, if available.

Created attachment 168424 [details] [review]
Use API constructor if available, slice alloc otherwise

Attached a patch that looks up the first constructor with no arguments to construct the boxed type. If such a constructor cannot be found, the slice alloc will be used. It could be nice to try to match the supplied arguments to the constructor with introspected constructors to find the right one (i.e. for opaque boxed types which don't have default constructors). Another issue might be that for each construction, the right construction has to be found again. Ideally, this should be cached, but to me its unclear where this can be done.

Review of attachment 168424 [details] [review]:

We also need some tests, see test_everything.py

Thanks and sorry for the late review

::: gi/pygi-boxed.c
@@ +82,3 @@
+_boxed_construct (GIBaseInfo *info)
+{
+    GIStructInfo *sinfo;

struct_info would be more in line with the existing code

@@ +86,3 @@
+    gint i;
+
+    if (!GI_IS_STRUCT_INFO (info)) {

better assert

@@ +95,3 @@
+    for (i = 0; i < num_methods; ++i)
+    {
+        GIFunctionInfo *func;

function_info

@@ +99,3 @@
+        GICallableInfo *callable;
+        GITypeInfo return_type;
+        GArgument return_value;

GIArgument

@@ +118,3 @@
+        g_callable_info_load_return_type (callable, &return_type);
+
+        if (!g_type_info_is_pointer (&return_type)) {

Is there a need for this if we already know it's a constructor?

@@ +136,3 @@
+            return NULL;
+        }
+

We have several *Info to unref

Review of attachment 168424 [details] [review]:

We also need some tests, see test_everything.py

Thanks and sorry for the late review

::: gi/pygi-boxed.c
@@ +82,3 @@
+_boxed_construct (GIBaseInfo *info)
+{
+    GIStructInfo *sinfo;

struct_info would be more in line with the existing code

@@ +86,3 @@
+    gint i;
+
+    if (!GI_IS_STRUCT_INFO (info)) {

better assert

@@ +95,3 @@
+    for (i = 0; i < num_methods; ++i)
+    {
+        GIFunctionInfo *func;

function_info

@@ +99,3 @@
+        GICallableInfo *callable;
+        GITypeInfo return_type;
+        GArgument return_value;

GIArgument

@@ +118,3 @@
+        g_callable_info_load_return_type (callable, &return_type);
+
+        if (!g_type_info_is_pointer (&return_type)) {

Is there a need for this if we already know it's a constructor?

@@ +136,3 @@
+            return NULL;
+        }
+

We have several *Info to unref

Created attachment 176645 [details] [review]
Construct boxed types using default API constructor first

When invoking the default python constructor for a boxed type object,
first try to construct the boxed type from a default constructor
provided by the API. If the constructor does not exist, or fails, try
to slice allocate the boxed object.

Created attachment 176646 [details] [review]
Construct boxed types using default API constructor first

When invoking the default python constructor for a boxed type object,
first try to construct the boxed type from a default constructor
provided by the API. If the constructor does not exist, or fails, try
to slice allocate the boxed object.

Sorry for the late wait and the double patch. The latest should address your comments. Note that I removed the NULL check as this is already checked before.

Now that I think of it, why isn't this working for GtkTreePath?

http://git.gnome.org/browse/pygobject/tree/gi/types.py#n65

Note that we still need tests, a fully opaque boxed should be added to Regress.[hc] in gobject-introspection and a python test to tests/test_gi.py

Something like this should work, but we still need test cases, including testing subclassing:

diff --git a/gi/types.py b/gi/types.py
index cc43d02..8f01033 100644
--- a/gi/types.py
+++ b/gi/types.py
@@ -189,6 +189,11 @@ class StructMeta(type, MetaClassHelper):
         cls._setup_methods()
         cls._setup_constructors()
 
+        for method_info in cls.__info__.get_methods():
+            if method_info.is_constructor() and method_info.get_name() == 'new':
+                cls.__new__ = staticmethod(Constructor(method_info))
+                break
+
 class Enum(int):
     __info__ = None
     def __init__(self, value):

Created attachment 178754 [details] [review]
Construct structs using default API constructor

If the struct has something that looks like a default constructor,
use it instead of trying to directly allocate it, as it will fail
if the struct fields are not exposed.

A test was pushed recently by Martin Pitt in 5eca5ff2c9509ec96158fe43

Attachment 178754 [details] pushed as 7c2f48b - Construct structs using default API constructor