On Fri, 2010-01-29 at 13:03 -0500, DJ Delorie wrote: > Well, here it is. Yes, I know it's not in git's pretty format, but > it's a bunch of local commits that make no sense separately. Discuss, > then I'll commit it all.
I'm interested in the diffstat (re-ordered to show the biggest changes): src/action.c | 666 +++++++++++++++++++++++++++++++++++++++++++- src/hid/lesstif/dialogs.c | 346 ++++++++++++++++++++++ src/buffer.c | 287 ++++++++++++++++++ src/hid/gtk/gtkhid-main.c | 220 ++++++++++++++ src/netlist.c | 136 ++++++++ src/misc.c | 108 +++++++ tools/gnet-pcblf.scm | 114 +++++++ src/change.c | 35 +- src/misc.h | 16 + [snip lots of small changes]. Its not actually as big as you might have expected. There is a fair bit more in the (already huge) action.c though. (7057 lines pre-patch!). We ought to consider breaking action.c up at some stage! Minor nit - "pcblf" is not a great name for the backend. Outside our circle, "lf" is probably not something many will understand, and might end up being one of those FAQ's like "Ben mode". Netlist(Add,name,pin,defer) Adds the given pin to the given name. If defer, don't tell the GUI about it - must then call NetlistChanged() manually. Rather than defer, I'd have probably gone with the pattern: Netlist(freeze) Netlist(..updates in here..) Netlist(..updates in here..) ... Netlist(thaw) It saves a lot of "implementation detail" being repeated each line with the "defer" argument) Freeze / thaw is a common idiom with event driven updates in various MVC schemes. It would necessitate the update mechanism to the GUI being a bit smarter though - allowing it to aggregate change notifications. ElementAddIf() could be named a little clearer. "AddIf" implies the rule would take a predicate. Also, in the case where the element exists.. the operation performed is not "Add". It it weren't for the "Add"ing of new elements, I'd have suggested "UpdateElement". Why not just call it "Element(...)". Similar naming nits with the "ElementAdd{start,finish}". New commands: "Attributes" "ElementAddStart" "ElementAddIf" "ElementAddDone" <-- Name relating to "Add" doesn't really reflect its function I don't like the suggested behaviour of selecting errant elements from the board. I think that should be a separate action, one which can be called at any time (e.g. from a menu). This does require the concept of an "elements list" to persist within the PCB design. That provides other possible workflows too - such as allowing the GUI to provide a "tetris mode" for elements which _aren't_ yet in the design, rather than plonking them down automatically. "ElementSetAttr" "ExecCommand" <--- these are the ones which give me most concern "Import" <--- these are the ones which give me most concern The "ExecCommand", especially - gives PCB ".cmd" files some rather dangerous powers. We could perhaps keep execution within the "Import" command an implementation detail? I like that the new actions appear to have non-zero return codes for failure status. This means (when dbus.c is fixed!), we can use those to get some direct feedback for programs such as xgsch2pcb which might want to respond to these things interactively. _______________________________________________ geda-user mailing list geda-user@moria.seul.org http://www.seul.org/cgi-bin/mailman/listinfo/geda-user