Re: gEDA-user: PCB LF patch: single "foward annotate" sch->pcb button

[Peter Clifton <pcjc2@xxxxxxxxx>]

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@xxxxxxxxxxxxxx
http://www.seul.org/cgi-bin/mailman/listinfo/geda-user