Comment 48 for bug 594073

On 11/7/2016 12:42 PM, Nox_firegalaxy wrote:
> I beg your pardon but actually it would not be that hard if the current
> netlist export procedure would not mess up the undo/redo - which I would
> expect as an user.
>
> But it is how it is. So is there any good reason against your proposed
> idea of cleaning up the wire right after insertion? By the way I am
> curious why the undo/redo is only cleared some times.
> SCH_SCREEN::SchematicCleanUp is called several times without a
> correlated clearing of the redo/undo list (e.g. EndSegment,
> AddNoConnect, DisplayCurrentSheet). Are the no dangling pointers in
> those cases? Or is clearing the redo/undo list actually outdated?

I'm OK with cleaning up immediately after each wire or junction change
but it will require some careful coding to make sure the undo/redo
buffers are correct for wire lists. Fixing this first would eliminate
the need to call SchematicCleanUp() before exporting the netlist or any
other operation that requires a cleaned up wire list. I would help you
with this if I had time but I am desperately trying to get the new file
format done.

The undo/redo buffers are only cleared when SchematicCleanUp() makes any
changes to the wiring. This is the case that causes the pointer issues
so it was an easy fix to prevent undoing bad wire pointers from causing
a segfault. It may be as simple as calling SchematicCleanUp() before
saving wire and junction edits to the undo buffer.

>
> And would writing an own netlist generator not mean again the
> introduction of a parallel procedure?

Maybe. It might be possible to extend the current netlist generator to
use symbol time stamps instead of reference designators for net
highlighting purposes. It seems like a better solution given all of the
issue with annotation. Users aren't going to want dismiss an annotation
warning dialog when they want to highlight a net. JP is much more
familiar with the netlist generation code than I am. He may have some
ideas that I have overlooked.

>
>
> Am 07.11.2016 um 16:48 schrieb Wayne Stambaugh:
>
>> On 11/6/2016 7:27 AM, Nox_firegalaxy wrote:
>>> After looking some time again:
>>> One the one hand you are completely right. Neither killing the undo/redo
>>> list nor the need to annotate everything is something one wants for
>>> "simple" highlighting. On the other hand using the exact netlist export
>>> process garantees that the highlighting is consistent with the exported
>>> netlist. So is introducing a parallel procedure with temporary
>>> annotation (which hopefully does not break something else) and avoidence
>>> of the cleanup really the best option? What if someone changes the
>>> behaviour only in one of both procedures?
>> You're finally understanding why this hasn't been implemented yet. It
>> turns out that is a lot more difficult than it appears.
>>
>>> So imho if one wants to get it right one should change the netlist
>>> export to respect the undo/redo list - or is it good pratice that
>>> export/sync breaks the undo/redo?
>>> If I am not mistaken clearing the undo/redo list is done in (all
>>> eeschema files listed):
>>> LIB_EDIT_FRAME::OnImportPart
>>> LIB_EDIT_FRAME::LoadComponentFromCurrentLib
>>> LIB_EDIT_FRAME::CreateNewLibraryPart
>>> SCH_SCREEN::~SCH_SCREEN
>>> DIALOG_ERC::TestErc
>>> SCH_SCREENS::SchematicCleanUp
>> SchematicCleanUp() is going to be an issue. The reason it clears the
>> undo/redo buffer is there would be dangling wire pointers if it didn't.
>> This behavior could be fixed. There are several options none of which
>> are going to be trivial to implement. Rather than clear the undo/redo
>> buffer, you could create a complete wire list a push that to the undo
>> buffer. The problem with that is when the user attempts to undo the
>> changes done by SchematicCleanUp() it will appear that nothing was
>> undone. You could append the changes to a previous undo buffer wire
>> list assuming there is one but that could be tricky. The other option
>> would be to call SchematicCleanUp() on every wire or junction change and
>> add any clean up changes to the undo buffer along with the wire or
>> junction changes.
>>
>>> For the here discussed case only the later three are of interested.
>>> Interestingly SCH_SCREEN::SchematicCleanUp is called several times
>>> without a correlated ClearUndoRedoList (e.g. AddNoConnect, EndSegment).
>>> So my question is: Is ClearUndoRedoList really required/reasonable?
>>> Regarding the annotation it would be better if it would not be required
>>> but again: to avoid double doing and later divergence in behaviour I
>>> would argue that it is the lesser evil to make annotation mandetory for
>>> net highlighting - the user is still free to not use net highlighting at
>>> all. Maybe I am naive but from a users perspective not having net
>>> highlighting at all seems to me like a bigger issue than dealing with
>>> annotation and even more threating is a net highlighting which is
>>> potentially not consistent to netlist export.
>> Annotation is going to be an issue as well. The cleanest way to do it
>> is to make a temporary copy of the entire schematic and perform a full
>> reannotation. This way you would be ensured a valid netlist without
>> interfering with the users current annotation. I don't know what kind
>> of overhead would be incurred or if there would be enough of a
>> performance penalty to make this unusable for net highlighting. You
>> could also write a net list generator that uses the component time stamp
>> which should be unique rather than the reference designator. This
>> should be more reliable and you would have not to annotate.
>>
>>> P.S: Annotiation messes with undo/redo but neither clears the list nor
>>> does it track changes if i am not mistaken. Just apply some changes
>>> (e.g. rename parts), revert some changes, then run annotiation and play
>>> around with undo/redo.
>>>
>>> best regards
>>>
>>>
>>> Am 01.11.2016 um 13:06 schrieb jean-pierre charras:
>>>> Sorry to say that, but neither are good (and neither are acceptable).
>>>>
>>>> Unannotated components are not an issue to highlighting a connection.
>>>> I means the reference is not really used, although it cannot be fully ignored.
>>>>
>>>> In fact you have 2 issues, when calling prepareForNetlist() (that is currently designed to prepare a netlist for a board):
>>>> - it cleanup the schematic (merge collinear wires, and therefore change the wire list).
>>>> - it tests everything strange thing in annotation.
>>>>
>>>> Cleanup the schematic avoid side effects in connection calculation:
>>>> - It avoid false connections (This is a rare case, not very important here). But it has a side effect: if the wire list is modified, the undo/redo list is cleared (is has no matter when creating a netlist for a board, but it is is annoying in this case)
>>>> It could be avoided here.
>>>> - It calls CheckAnnotate().
>>>> CheckAnnotate() tests incorrect annotation (duplicates, not annotated and a few other things)
>>>> As I say previously, not annotated components are not blocking (other issues are, but they are very infrequent).
>>>> However annotating them before the schematic is finished is very annoying (think to not interchangeable units for instance).
>>>> Moreover, annotating components clears the undo/redo list.
>>>>
>>>> Therefore:
>>>> * prepareForNetlist(), when called to highlight a net, should not cleanup the schematic and should no test for not annotated items.
>>>> * Not annotated items (only to calculate the connection list) should receive a temporary annotation (for instance the full timestamp).
>>>> The reason is the fact if you have 2 different components having the same ref (say R?) and the same pin number (say 1) all connection to pins (R? pin 1) will be merged.
>>>>
>