Here is a fix (in plug-ins/pygimp/pygimp-image.c). 
(However, this bug applies to many other calls as well)

static PyObject *
img_add_layer(PyGimpImage *self, PyObject *args)
{
    PyGimpLayer *lay;
    int pos = -1;
	
    if (!PyArg_ParseTuple(args, "O!|i:add_layer", &PyGimpLayer_Type, &lay,
			  &pos))
	return NULL;
    
    {
	int s = gimp_image_add_layer(self->ID, lay->ID, pos);
	if( s ) {
	    return PyInt_FromLong(s);
	}
    }
    PyErr_SetString(pygimp_error, "layer add failed");
    return NULL;
}

Perhaps you could attach a diff (diff -u). That would make it a lot easier to
review the proposed changes.

Created attachment 34386 [details] [review]
diff for proposed patch

That diff still doesn't show the changes. Please use "diff -u".

Created attachment 34464 [details] [review]
diff for proposed patch

That change doesn't look very correct. gimp_image_add_layer() returns a
gboolean, not an integer. However, since I'm not involved with pygimp, I might
just misinterpret the code.

The return status from gimp_image_add_layer is what is returned in the current
code. It is an int with possible values 0/1.

My change is that 0 is never returned - instead python returns an "error state".
This is more in line with the failure you get when you try to do the same thing
using pdb.gimp_image_add_layer directly. right now if you forget to check the
return status, you never know an error occured.

If you are saying one can simply return 1, I guess this is right too. I wanted
to suggest a minimal change.

I guess a larger change is needed then. Neither 0 nor 1 make any sense here as
return values.

Comment on attachment 34464 [details] [review]
diff for proposed patch

neo, do you mean that TRUE and FALSE are better return values?

If not, I don't follow your last comment.

Well, I don't understand the Python bindings good enough but this code doesn't
look right to me. It would return PyInt_FromLong(s) where s is the boolean
return value from gimp_image_add_layer(). Does that make sense to you?

If Python has a boolean type, that would certainly be better.

Dave.

This would make more sense:

static PyObject *
img_add_layer(PyGimpImage *self, PyObject *args)
{
    PyGimpLayer *lay;
    int pos = -1;

    if (!PyArg_ParseTuple(args, "O!|i:add_layer", &PyGimpLayer_Type, &lay,     
                     
                          &pos))
        return NULL;

    if (!gimp_image_add_layer(self->ID, lay->ID, pos)) {
        PyErr_SetString(pygimp_error, "could not add layer to image");
        return NULL;
    }

    Py_INCREF(Py_True);
    return Py_True;
}

However, I'm not sure such a change should be made in the stable branch, as it
does change current behavior.

I'm calling this FIXED, since the original request is fulfilled. Other API needs
to be taken care of the same way still though.

2005-03-03  Manish Singh  <yosh@gimp.org>

        * plug-ins/pygimp/gimpmodule.c
        * plug-ins/pygimp/pygimp-display.c
        * plug-ins/pygimp/pygimp-drawable.c
        * plug-ins/pygimp/pygimp-image.c
        * plug-ins/pygimp/pygimp-parasite.c
        * plug-ins/pygimp/pygimp-pdb.c
        * plug-ins/pygimp/pygimp-tile.c: Throw exceptions on failures for
        libgimp wrappers (fixes bug #160136), and make the exception strings   
     
        a lot more descriptive to aid debugging. Also return proper Bools when
        appropriate. Some new API wrapped as well. Still a work in progress.