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
Attachment:
pgpncf5rqv5sn.pgp
Description: PGP signature
_______________________________________________ geda-dev mailing list geda-dev@xxxxxxxxxxxxxx http://www.seul.org/cgi-bin/mailman/listinfo/geda-dev