[PATCH] Re: gEDA-dev: libgeda, gschem: New print dialog

[Patrick Bernaud]

Hi Peter,

Peter TB Brett writes:
 > > [...]
 > > Try it out and tell me what you think!
 > 
 > I have a new version of the patch that's much 'nicer' (it uses GTK subclassing 
 > properly), but Sourceforge keeps timing out when I try and upload it, so I 
 > can't attach it to the tracker item!
 > 

That's a nice dialog. A few things though:

1) libgeda:

  - you indeed have introduced a memory leak with the print_command
    field in TOPLEVEL. The field is initialized with a const string
    but may later be replaced with a malloc-ed string. 

    To fix it, dup the const string and assign it to
    toplevel->print_command at init, frees the field in
    g_rc_print_command() and updates s_toplevel_delete() to clean it
    up.

  - f_print_stream() is used nowhere but in f_print.c. Make it
    private?

  - in f_print_file(), the comment:
    '/* dots are breaking my filename selection hack hack !!!! */'
    is quite cryptic. It is not from you but do you understand it?
    does it still apply?

  - it appears the return value of f_print_header() is not
    significant: it always return 0. Changing it to return nothing?

  - have use considered using GError in functions of f_print.c? I
    think it will make a nice addition to signal problem to caller
    and ultimately in gschem.


2) gschem:

  - in g_rc_print_command() GLib's g_strdup() would be a good
    replacement for the strlen+malloc+memcpy.

  - have you considered the possibility of not connecting the toplevel
    to the print dialog? It would further separate code for action
    and GUI. Plus you do not modify the toplevel before the user
    validates the dialog

    I mean: adding properties for paper size, type, orientation
    and output type (but not toplevel) to PrintDialog. In
    x_print_setup(), using gtk_dialog_run() and waiting for the user
    to click a button and depending on the return code retrieve the
    options from the dialog. Contents of print_dialog_action_print()
    would be moved to x_print_setup().

    Not sure I am clear here. I can elaborate if you want.

  - if the print fail, the user is only warned by a line in the
    message dialog. Opening a message dialog if something goes wrong
    might be a good idea.


Regards,


Patrick