Currently, when returning an error, PDB functions return just an error state. It would be possible to change this in a compatible way by optionally returning a string parameter that holds an error message. Callers would then have to check number and type of return arguments. If the first parameter is an error status, and a second string parameter exists, this would be taken as the error message.

I am setting the 2.6 milestone on this one. Perhaps someone wants to address this. Otherwise we can always postpone it later...

Changing version to "Current SVN" as this request is not specific to any particular version.

Note that some work on this feature has already been done in trunk. Errors in the core are now all handled using GError. What needs to be done is passing the error to the plug-in.

We need an API proposal for this feature. As we don't want to break backwards compatibility, we must not change any of the existing PDB wrappers in libgimp. The current error handling (throwing up an error dialog) should also stay as the default behavior so that old plug-ins continue to behave as usual.

Something like the following proposal could work.

First, the plug-in tells GIMP that it will handle PDB errors:

  gimp_procedural_db_set_error_handler (GIMP_PDB_ERROR_HANDLER_PLUG_IN);

Then, whenever it does a PDB call it would have to check the return value and in case the call fails, it retrieves the error:

  if (! gimp_image_flatten (image))
    {
       gchar *error = gimp_procedural_db_get_error ();

       // this is a stupid example,
       // of course the plug-in should do something more sensible
       gimp_message ("%s", error);
       g_free (error);
    }

Later it could reset the error handler to the default value:

  gimp_procedural_db_set_error_handler (GIMP_PDB_ERROR_HANDLER_GIMP);


Would this also work reasonably for scripts? Can someone think of a better API? Better function and/or enum names? Can this be implemented at all?

This is just about what I had in mind too. The current error handler
and last error message would simply live in the GimpPlugInProcFrame
in the core, and I see no reason why this wouldn't work for plug-ins.
The Script-fu process would simply set the error handler just as
the temp_proc gets called and unset it before returning.

I wonder if the functions should be in message.psd or
plugin.pdb instead, The gimp_procedural_db_foo() functions
currently don't do any such "current plugin" stuff.

You are right, it would be nicer to have an API with the gimp_plugin prefix for this. Can you think of good procedure names?

I think i prefer gimp_message, since gimp_plugin currently only deals
with completely different stuff, while we already have another meessage
handler API in gimp_message.

The procedures prefixed with gimp_message deal with message handling in the core while this API is about error handling in the context of procedures called from the plug-in. So I think this should be in the gimp_plugin prefix.

As far as I can see we see it is pretty clear how we want to solve this, so we're just waiting for someone to do it. Until someone does, let's keep this on the Future milestone.

This is 90% implemented and it would be stupid not to finish the remaining bits for 2.6. We can still push it to 2.8 if we don't make it in time for the release.

have needed this for years! looking forward to it!

Created attachment 116460 [details] [review]
untested patch, mainly for API review

Created attachment 116714 [details] [review]
return error description as part of the return values

Mitch and me figured that it would be better if the error message was pushed down to the caller as part of the procedure return values. This patch implements this. As before, the first return is always the status. But if it is GIMP_PDB_CALLING_ERROR, GIMP_PDB_EXECUTION_ERROR or GIMP_PDB_CANCEL, then the next return value can be string describing the cause of the failure.

The patch includes a small change to the Python bindings. With this patch the raised Exception includes the error message. This can be used to test the patch.

Created attachment 116732 [details] [review]
full patch including changes to app/pdb

Created attachment 116734 [details] [review]
next iteration of the patch including changes discussed on IRC

2008-08-16  Sven Neumann  <sven@gimp.org>

	If a procedure call fails, pass a string describing the error as
	the second return value. First step towars fixing bug #344818.

	* app/pdb/gimpprocedure.[ch] (gimp_procedure_get_return_values):
	added a GError parameter. If it is set, pass the error message to
	the return values.

	* app/pdb/gimppdberror.h: added some more error codes.

	* app/pdb/gimppdb.c
	* app/xcf/xcf.c: pass errors to gimp_procedure_get_return_values().

	* app/plug-in/gimpplugin-message.c (gimp_plug_in_handle_proc_run):
	show a different error message for execution vs. calling errors.

	* app/plug-in/gimpplugin-progress.c
	(gimp_plug_in_progress_cancel_callback): pass the error
	GIMP_PDB_CANCELLED to gimp_procedure_get_return_values().

	* app/plug-in/gimppluginmanager-call.[ch]
	(gimp_plug_in_manager_call_run): removed the 'destroy_return_vals'
	parameter.

	* app/plug-in/gimppluginprocedure.c: destroy the return values here.

	* app/plug-in/gimppluginprocframe.c: pass an error to
	gimp_procedure_get_return_values().

	* tools/pdbgen/app.pl
	* tools/pdbgen/pdb/fileops.pdb: generate code that passes the
	error to gimp_procedure_get_return_values().

	* app/pdb/*-cmds.c: regenerated.
	
	* plug-ins/pygimp/pygimp-pdb.c: extract the error message from the
	return values and pass it to the exception that is thrown.

Created attachment 116748 [details] [review]
keep the error message in libgimp and provide a getter for it

I've committed something similar now:

2008-08-16  Sven Neumann  <sven@gimp.org>

	Next step towards fixing bug #344818:

	* libgimp/gimp.[ch]: keep the last error status and error message
	in libgimp. Added new functon gimp_pdb_get_error() that allows to
	retrieve it.

	* libgimp/gimp.def: updated.

	* plug-ins/pygimp/gimpmodule.c (pygimp_vectors_import_from_file)
	(pygimp_vectors_import_from_string): use the new function to get
	a more useful error message.

Created attachment 116759 [details] [review]
convert error message in procedure return values to a GError

This patch takes care of converting error messages in procedure return values to a GError. It also changes the file-png plug-in to push error messages to the return values instead of using g_message(). You can test this by attempting to save a PNG file to a folder where you don't have write permissions.

I've committed this patch (with some minor changes) to trunk:

2008-08-17  Sven Neumann  <sven@gimp.org>

	Next step towards fixing bug #344818:

	* app/pdb/gimpprocedure.c (gimp_procedure_execute): if the error
	has not already been set, construct one from the error message
	that is optionally passed with the return values.

	* plug-ins/common/file-png.c: changed to pass an error message
	with the return values instead of calling g_message() in case of
	an error.

We should review the changes in the file-png plug-in to see if this can be done easier before we apply this to all (file) plug-ins.

2008-08-19  Sven Neumann  <sven@gimp.org>

	Complements the fix for bug #344818:

	* libgimpbase/gimpbaseenums.[ch]: added new enum GimpPDBErrorHandler.

	* tools/pdbgen/enums.pl: regenerated.

	* app/plug-in/gimpplugin.[ch]: added error_handler to GimpPlugIn.

	* app/plug-in/gimpplugin-message.c (gimp_plug_in_handle_proc_run):
	only display an error message for a failed procedure call if the
	plug-in's error-handler is set to GIMP_PDB_ERROR_HANDLER_INTERNAL.
	
	* tools/pdbgen/pdb/plug_in.pdb: added PDB getter and setter for
	the plug-in's error-handler.

	* app/pdb/plug-in-cmds.c
	* app/pdb/internal-procs.c
	* libgimp/gimpenums.c.tail
	* libgimp/gimpplugin_pdb.[ch]: regenerated.

	* plug-ins/common/file-compressor.c
	* plug-ins/file-uri/uri.c: set the error-handler to
	GIMP_PDB_ERROR_HANDLER_PLUGIN as these plug-ins are forwarding the
	error with their return values.


A few file plug-ins still need to be touched before this bug can be closed.

Overall there are certainly a lot more places where error handling could be improved. But since we don't want to introduce lots of string changes at this point, we should be careful. We can continue this crusade after 2.6.

All file plug-ins touched, and a few other plug-ins as well. Closing as FIXED.