Re: Provide a python script to dia on the command line

[Hans Breuer <hans breuer org>]

Hi Martin,
Am 16.09.2014 um 11:34 schrieb Martin Metzker:
> Hello list!
>
> I think I have a viable patch now, for command line provided python scripts
> in dia. Going over the changes:
> > > ... moved the definition of struct _PluginInfo from lib/plug-ins.c to
> > > lib/plug-ins.h, because I need this struct in app/app_procs.c.
> > not ok. To avoid this please use dia_plugin_get_symbol().
> Using dia_plugin_get_symbol() and dia_plugin_get_name() now. No changes to
> lib/plug.ins.{c,h} anymore.
ok.

> > > ... modified app/app_procs.c app_init() to include the -i option and stores
> > > the provided filename in a local variable python_script.
> > ok, though I would prefer another short option, maybe -r and --run-script
> > instead of --python?
> Done.
ok.

> > > ... added a block if(python_script) which checks if the python plug-in is
> > > loaded, and does run_script_oneshot(python_script) when sensible.
>
> Extended functionallity: run_script_oneshot() still exists as described and
> is still being used during initialization. For running the script provided
> via the command line, there is now run_script_w_context(), which makes the
> variables files, export_file_name, export_file_format, input_directory and
> output_directory accessible from within python.
This sounds complicated and I'm not sure I like the design to arbitrarily 
push some variables into the python namespace.

> To accomplish this, the
> code block in app/app_progs.c creates a GHashTable with these variables
> (files is collapsed to a ,-separated list), which is passed to
> run_script_w_context(), which propagates the information to the python
> interpreter before calling run_script_oneshot().
Here I'm very uncertain, starting with the extra function, the huge block 
of code changes in app_init() and the ad hoc API via GHashTable between 
Dia's core and the Python plug-in. Maybe there should be an official PyDia 
API instead, to accomplish the same goal?
While I don's share your big vision of multiple Python threads in Dia, I 
think these additions are moving us away from the goal to allow the 
"oneshot script" to interact with the GUI.

> > IMO the patch is not too big to be discussed (and finally be applied) in
> > one piece.
> I would be happy if that is still the case.
Sorry, not quite. The patch providing the initial functionality was closer 
to inclusion, than this one is.

> > There are minor coding style glitches (tab size, placement of braces) but
> > I can fix these before commiting.
> I modified all the code I touched. There seems to be a little inconsistency
> with indentation.
There definitely is - between plug-ins and core, somethimes even with-in 
single files, or even functions. But multiple line reformating should be 
separated from critical content addtions, IMHO.

> I kept to the dominant style of app/app_progs.c which is
> 3 spaces for function bodies and an additional 2 spaces for each {. My code
> layout should be at least consitently broken.
For app/app_procs.c and most of the core I'd say it is indendation of 2; 
braces at the same line as 'if' or 'else'. With plug-ins/python/python.c 
it's indent 4 AFAICT.

> In addition, at its very beginning, this patch includes
>
> -      ef->export_func(diagdata, ctx, outfname, infname, size);
> +      ef->export_func(diagdata, ctx, outfname, infname, (void *) size);
>
> which fixes a compile warning that kept annoying me.
This warning shows a design problem and should not be brushed under the 
carpet with a patch unrelated to that issue. I also like code which 
compiles cleanly, but this is one of at least two warnings I want to keep 
until the underlying problem is solved.

If I would have to commit the patch I'd separte it into two parts:

1) Initial version of running scripts from the commandline
 - Basically what was reviewed before, with the requested changes.
 - Additionally a simple but somehow useful test script for the
   new functionality
2) Extended parameter support for command line scripts
 - As I said I'm uncertain at the moment how to do it. I would
   probably experiment with differnt approaches, like using sys.argv;
   connecting to DiaApplication signals or ...
 - If it is to be done the way you propose it should be at least
   factored into it's own function.

HTH,
        Hans

-------- Hans "at" Breuer "dot" Org -----------
Tell me what you need, and I'll tell you how to
get along without it.                -- Dilbert