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

[Peter TB Brett]

On Tuesday 01 August 2006 19:59, Patrick Bernaud wrote:

> 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.

Sure, I'll fix it.

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

No: this was deliberately left public because I can think of situations where 
libgeda user applications might want to output postscript to stdout, for 
instance.  In fact, in one of my earlier versions of this patch there were no 
f_print_file() or f_print_command() functions at all: the print dialog called 
f_print_stream() directly.  Having the full set of options in the library is 
nice, I think.

>   - 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?

No, I have no idea -- that chunk of code is moved verbatim out of the old 
f_print().

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

Not within the remit of this patch -- I'm only changing the frontend of the 
printing code.

>   - 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.

Not within the remit of this patch -- I'm trying to keep the libgeda printing 
interface as similar as possible, because otherwise it'll require a lot more 
refactoring than it already does.

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

Okay, thanks for the tip.

>   - 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.

I'll have a look tomorrow morning and see what I can do.  I'm not at all sure 
how to populate the paper sizes dialog with that setup -- pass in a GPtrArray 
as a property?

How do I set a return code in print_dialog_action_print()?

>   - 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.

I'll add an error dialog, but I'll keep the log output as well.

Thanks for the (extremely constructive) criticism -- it's very useful!

Peter

-- 
Fisher Society publicity officer            http://tinyurl.com/o39w2
CUSBC novices, match and league secretary   http://tinyurl.com/mwrc9
Quake II build tools maintainer             http://tinyurl.com/fkldd

v2sw6YShw7$ln5pr6ck3ma8u6/8Lw3+2m0l7Ci6e4+8t4Eb8Aen5+6g6Pa2Xs5MSr5p4
  hackerkey.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://www.seul.org/pipermail/geda-dev/attachments/20060801/b05d17b3/attachment.pgp