kicad-developers team mailing list archive

[Maciej Sumiński <maciej.suminski@xxxxxxx>]

Hi Miles,

On 12/04/2017 09:23 AM, miles mccoo wrote:
>> Is there a document the explains Board commit? An email thread about it
> 
>>> perhaps?
>>
>> There are some comments in include/commit.h. Perhaps it should be extended.
>>
>>
> Yes, I think so. A small description of how this class is intended to be
> used. First do this as setup, then make some changes, if doing this kind of
> change, call these methods, if you're making these other kinds of changes,
> do this other stuff. Finally call push. After that you should see x,y, and
> z in the GUI...

Sounds reasonable, I will expand the description.

> I see that board_commit constructor takes either a frame or a pcb_tool.
> it's not obvious to me what a pcb_tool does.

I think the best way to follow KiCad's code is to have the Doxygen
documentation [1], it helps a lot understanding  the code.

>>> It seems to me that the broad purpose of BOARD_COMMIT (I'll call it BC)
>> is
>>> to handle the details for undo. When changing a wire,module,pad,... you
>>> tell BC what you're doing. Undo then magically happens. Another side
>> effect
>>> is that the canvas is redrawn when needed.
>>
>> Think about the BOARD_COMMIT as of the observer pattern. You make a
>> change to an observed object and create a commit to notify the observers
>> about the change and its kind.
>>
>>
> The observers are all hardcoded in the push method, yes? I'm used to
> subscription based.

True, probably it would be better to have the subscription based type.
It has been done quickly to resolve the GAL vs legacy synchronization.
There are cases when the observers need to run in a particular order
(e.g. you want to delete the object as the last step when it is removed,
otherwise other observers will get a stale pointer).

>>> 1) There will be cases when a utility/command developer doesn't care
>> about
>>> undo. They just want to make their changes and if the result is not
>>> desired, just revert. This would be particularly true for automated,
>> final
>>> processing. One that comes to mind is a teardrop generator
>>> <https://forum.kicad.info/t/yet-another-python-teardrop-scri
>> pt-adds-and-deletes-teardrops-to-a-pcb-v0-3-3/3388>
>>
>> Have a look at COMMIT::Revert() method, it does exactly what you are
>> asking for. You can gather all changes using COMMIT and then either
>> Push() or Revert(), depending on whether you want them to be applied or
>> not.
>>
>>
> 
> Looking around, I found the GAL->DisplayBoard method. The Revert and Push
> methods mentioned above still require me to know/remember what changed.
> 
> 
> 
>>> *Will BC update still work if you don't tell it about any changes?* I ask
>>> because BC was given as a better way to redraw the canvas
>>
>> No, BC handles only the changes it is notified about. If you want, you
>> can fully refresh state of all observers that would have been notified
>> by BC, but I think it is not very efficient.
>>
> 
> 
> perhaps something the python abstraction layer could do it track changes
> and pass them along to the BC.

Good idea, but I do not have any simple idea how to achieve this.

>>> 2) I see there are a bunch of Add, Remove,... functions that a script
>> would
>>> need to call. *Why don't the relevant objects just make these calls when
>>> they are changed? *Then the code for undo is handled once and developers
>>> can forget about it.
>>
>> Because you need to update view, connectivity and undo buffer. We would
>> like to keep actions decoupled from the model and it is a way of doing so.
>>
> 
> Perhaps allow these observers to subscribe to changes? When a module, pad,
> track,... changes, it can call a Add/Remove/... method of whatever
> observers registered themselves.
> 
> I guess I'm just repeating my same comment. It just seems like doing this
> code in (# classes)*(# methods/class) = 10*10 places = a couple hundred
> maybe. After that, you never worry about it again.
> 
> Compare that to #commands * # places each command makes a change to the
> model. Yet another thing to think about when developing an edit command.

Sure, but think about a few things:
- Do you want to trigger all observers on every intermediate step when
dragging an item? I assume 'no', then how do you handle this?
- How do you handle actions that affect more than one item, but should
be grouped to one undo entry?
- If you want to cancel an action, does it mean you need to explicitly
create a copy of the modified item before the modification?

> 
> 
> 
> 
>>> 3) I see there are pre and post modify methods. (ie Remove and Removed)
>> *What's
>>> the difference?* Why use one over the other?
>>
>> Remove() does the removal action for you, Removed() only notifies the
>> observers that an item has been removed and you clean up the object.
>>
>>
> I guess I gave a bad example.  There's Modify and Modified. To
> search/replace your answer:
> 
> Modify() does the modify action for you. Modified() only notifiies the
> observers that an item has been modified and you (not such what
> replacement) the object.
> 
> How does it know what to modify? location, width, layer,..?

It picks the safest option, update everything for an item.

> Looking closer at the method signatures and the code, it appears that diffs
> are tracked by cloning the relevant object for later diff. With Modify, the
> BC will make the clone for me. With Modified, I have to do the cloning
> myself.
> 
> later the before/after are compared.
> 
> 
> 
>> 4) Since there isn't a single global BC object, *are there reasons to have
>>> multiple objects?* Does it make sense to have two objects active at the
>>> same time? I'm picturing an editing operation that puts some changes in
>> BCa
>>> and others in BCc, but I can't think of a reason why.
>>
>> You can do that. I think BC are easier to handle as multiple objects. I
>> am not sure if this is the case now, but imagine a routine starts saving
>> items into a commit and then invokes another one that also does the same
>> - they will interfere with each other. With multiple objects you are in
>> control.
>>
>>
> 
> what kind of interference can happen? It it because both commands each
> create their own clones of the same object and it's unclear what to compare
> to what?

I think there are no cases in the code that I could point you to, but
with a global object only one tool at a time can work with a commit.
Given we use the coroutine approach and have a few tools running in
parallel, it is simply not safe:

Tool A: create a new commit, add item X, add item Y
Tool B: create a new commit (previous changes dropped? not pushed?),
remove item W, push
Tool A: revert (nothing reverted?)

Regards,
Orson

1.
http://ci.kicad-pcb.org/job/kicad-doxygen/ws/Documentation/doxygen/html/classes.html

> This all makes me think of another feature I've seen in other tools that
> can be super useful. save/restore. Somehow all UI events and command
> invocations are saved for later replay. Makes it much easier to reproduce
> fatals, to automate testing, and also to create macros.
> But that's another topic.
> 
> 
> Miles

Attachment: signature.asc
Description: OpenPGP digital signature