Re: [PATCH] two small patches for the hippo python bindings

["Tomeu Vizoso" <tomeu tomeuvizoso net>]

On Tue, Oct 21, 2008 at 8:07 PM, Owen Taylor <otaylor redhat com> wrote:
> On Tue, 2008-10-21 at 14:15 +0200, Tomeu Vizoso wrote:
>> Hi,
>>
>> needed to access hippo_canvas_box_sort() from python and also found a
>> crasher when hippo_canvas_box_find_box_child() returned NULL.
>
> Couple of minor problems; I know these aren't all new problems, some are
> problems in existing code that you are moving about.
>
> +    if (box_child)
> +        return py_hippo_canvas_box_child_new(box_child);
> +    else
> +        return Py_None;
>
> When returning one of the constants, you always need to increase the
> refcount first. So:
>
>        Py_INCREF(Py_None);
>        return Py_None;
>
> (There is a handy Py_RETURN_NONE macro, but we don't seem to be using it
> elsewhere, so probably better to avoid it for consistency.)
>
> +static int
> +marshal_compare_child_func(HippoCanvasItem *a, HippoCanvasItem *b, void
> *data)
> +{
> +    PyObject *py_a, *py_b;
> +       PyObject *compare = (PyObject*) data;
>
> This looks like the editor was set to 4-space tabs. Fixed 8-space tabs
> are standard for all of GNOME.
>
> +       py_a = pygobject_new(G_OBJECT(a));
>
> I'd suggest checking the return value here. It can return NULL, though
> that is unlikely.
>
> +       int retval = 0;
> [...]
> +       retval = PyInt_AsLong(retobj);
> +        if (retval == -1 && PyErr_Occurred())
> +            PyErr_Print();
> [...]
> +    return retval;
>
> Would recommend:
>
>  long retval;
> [...]
>  return retval < 0 ? -1 : (retval == 0 ? 0 : 1);
>
> Otherwise looks good to me.

Thanks for all the hints. The patches attached should address the
raised concerns.

Regards,

Tomeu

Index: python/hippo.override
===================================================================
--- python/hippo.override	(revision 7294)
+++ python/hippo.override	(working copy)
@@ -28,6 +28,53 @@
 
 extern Pycairo_CAPI_t *Pycairo_CAPI;
 
+static int
+marshal_compare_child_func(HippoCanvasItem *a, HippoCanvasItem *b, void *data)
+{
+    PyObject *py_a, *py_b;
+    PyObject *compare = (PyObject*) data;
+    PyObject *retobj;
+    PyGILState_STATE state;
+    long retval = 0;
+
+    state = pyg_gil_state_ensure();
+
+    py_a = pygobject_new(G_OBJECT(a));
+    if (py_a == NULL) {
+        if (PyErr_Occurred())
+            PyErr_Print();
+        pyg_gil_state_release(state);
+        return 0;
+    } 
+
+    py_b = pygobject_new(G_OBJECT(b));
+    if (py_b == NULL) {
+        if (PyErr_Occurred())
+            PyErr_Print();
+        Py_DECREF(py_a);
+        pyg_gil_state_release(state);
+        return 0;
+    } 
+    
+    retobj = PyEval_CallFunction(compare, "(OO)", py_a, py_b);
+
+    Py_DECREF(py_a);
+    Py_DECREF(py_b);
+
+    if (retobj == NULL) {
+        PyErr_Print();
+    } else {
+        retval = PyInt_AsLong(retobj);
+        if (retval == -1 && PyErr_Occurred())
+            PyErr_Print();    
+    }
+
+    Py_XDECREF(retobj);
+
+    pyg_gil_state_release(state);
+    
+    return retval < 0 ? -1 : (retval == 0 ? 0 : 1);
+}
 %%
 modulename hippo
 %%
@@ -182,38 +229,6 @@
 }
 %%
 override hippo_canvas_box_insert_sorted kwargs
-static int
-marshal_canvas_box_insert_sorted(HippoCanvasItem *a, HippoCanvasItem *b, void *data)
-{
-    PyObject *py_a, *py_b;
-	PyObject *compare = (PyObject*) data;
-	PyObject *retobj;
-	PyGILState_STATE state;	
-	int retval = 0;
-	
-    state = pyg_gil_state_ensure();
-	py_a = pygobject_new(G_OBJECT(a));    
-	py_b = pygobject_new(G_OBJECT(b));
-    
-    retobj = PyEval_CallFunction(compare, "(OO)", py_a, py_b);
-
-	Py_DECREF(py_a);
-	Py_DECREF(py_b);
-
-    if (retobj == NULL) {
-        PyErr_Print();
-    } else {
-    	retval = PyInt_AsLong(retobj);
-        if (retval == -1 && PyErr_Occurred())
-            PyErr_Print();    
-    }
-	
-    Py_XDECREF(retobj);
-
-    pyg_gil_state_release(state);
-    
-    return retval;
-}
 static PyObject *
 _wrap_hippo_canvas_box_insert_sorted(PyGObject *self, PyObject *args, PyObject *kwargs)
 {
@@ -235,12 +250,34 @@
 	hippo_canvas_box_insert_sorted(HIPPO_CANVAS_BOX(self->obj), 
 								   HIPPO_CANVAS_ITEM(item->obj),
 								   flags,
-								   marshal_canvas_box_insert_sorted,
+								   marshal_compare_child_func,
 								   compare);
 	Py_INCREF(Py_None);
 	return Py_None;
 }
 %%
+override hippo_canvas_box_sort kwargs
+static PyObject *
+_wrap_hippo_canvas_box_sort(PyGObject *self, PyObject *args, PyObject *kwargs)
+{
+    static char *kwlist[] = { "compare_func", NULL };
+    PyObject *compare;
+
+    if (!PyArg_ParseTupleAndKeywords(args, kwargs,"O:HippoCanvasBox.sort", kwlist,
+				     &compare))
+        return NULL;
+    if (!PyCallable_Check(compare)) {
+        PyErr_SetString(PyExc_TypeError, "parameter must be callable");
+        return NULL;
+    }
+
+    hippo_canvas_box_sort(HIPPO_CANVAS_BOX(self->obj), 
+			  marshal_compare_child_func,
+			  compare);
+    Py_INCREF(Py_None);
+    return Py_None;
+}
+%%
 override hippo_canvas_box_align kwargs
 static PyObject *
 _wrap_hippo_canvas_box_align(PyGObject *self, PyObject *args, PyObject *kwargs)

Index: python/hippo.override
===================================================================
--- python/hippo.override	(revision 7294)
+++ python/hippo.override	(working copy)
@@ -339,7 +339,12 @@
     
     box_child = hippo_canvas_box_find_box_child(HIPPO_CANVAS_BOX(self->obj), HIPPO_CANVAS_ITEM(child->obj));
 
-    return py_hippo_canvas_box_child_new(box_child);
+    if (box_child)
+        return py_hippo_canvas_box_child_new(box_child);
+    else {
+	Py_INCREF(Py_None);
+	return Py_None;
+    }
 }
 %%
 override hippo_canvas_box_get_layout_children noargs