kicad-developers team mailing list archive

[jp charras <jp.charras@xxxxxxxxxx>]

Le 20/04/2017 à 07:59, Oliver Walters a écrit :
> Wayne,
> 
> Is the behaviour I have implemented acceptable?
> 
> Regards,
> Oliver

Hi Oliver,

I tested your latest patches.

I found 3 issues: a minor one a 2 more annoying.

The minor:
After clicking on OK button the DisplayExitDialog() function is still called.
This is useless, because the user has already chosen the OK button. (The cancel button does not call
TransferDataFromWindow)

The annoying:
1 - in complex hierarchies, for instance 4 times the same sheet, if a component is modified, the
undo command is stored 4 times (one by sheet instance) for the same basic sheet (or SCH_SCREEN)
instead of only one time,
certainly because it appears 4 times (one by sheet instance) in the grid table (but there is only
one component in schematic and only one basic sheet)
2 - After click OK, the transfer of changes to the schematic can be very time consuming (8 s in my
design)
This is due to the fact ApplyFieldChanges() called after OK calls ItemChanged().
ItemChanged() Is very time consuming, and this call is useless because the dialog is just closed.


> 
> On Wed, Apr 19, 2017 at 12:13 AM, Oliver Walters <oliver.henry.walters@xxxxxxxxx
> <mailto:oliver.henry.walters@xxxxxxxxx>> wrote:
> 
>     Wayne,
> 
>     I have now fixed this such that UNDO actions are pushed to the UNDO stack for the associated
>     sheet. All UNDO actions for a given sheet are grouped so a single Ctrl-Z will undo all
>     components changed in the table (for the given sheet).
> 
>     Please find patch _007 attached (must be appli ed atop all previous patches).
> 
>     Let me know if you see any other pressing issues.
> 
>     Regards,
>     Oliver
> 
>     On Tue, Apr 18, 2017 at 6:30 AM, Wayne Stambaugh <stambaughw@xxxxxxxxx
>     <mailto:stambaughw@xxxxxxxxx>> wrote:
> 
>         On 4/17/2017 4:18 PM, Oliver Walters wrote:
>         > So how do we proceed here? Is there a 'global' undo stack? If not:
> 
>         Unfortunately there is no global undo stack.  Undo stacks are maintained
>         for each unique SCH_SCREEN (schematic file) object.
> 
>         >
>         > A) don't allow changes made in the component table viewer to be undone
>         > B) Make an undo entry for each sheet that has changed symbols
>         >
>         > A) is easier but the user would need to quit-without-save to undo changes
> 
>         This is less than desirable
> 
>         >
>         > B) is more difficult and doesn't solve the undo operations getting out
>         > of order either, as the user could inject another operation on a given
>         > sheet.
> 
>         This would be my preference.  Out of order operations are already an
>         issue so this solution doesn't make that issue any worse.  Undo/redo is
>         only available for the current sheet so the user would have to change
>         sheets in order to undo anything changed in the component properties table.
> 
>         >
>         > Suggestions?
>         >
>         > On 18 Apr 2017 01:26, "Wayne Stambaugh" <stambaughw@xxxxxxxxx <mailto:stambaughw@xxxxxxxxx>
>         > <mailto:stambaughw@xxxxxxxxx <mailto:stambaughw@xxxxxxxxx>>> wrote:
>         >
>         >     On 4/17/2017 10:21 AM, jp charras wrote:
>         >     > Le 17/04/2017 à 04:11, Oliver Walters a écrit :
>         >     >> JP, others,
>         >     >>
>         >     >> After further investigation, I have worked out why the components
>         >     with duplicated references were
>         >     >> displaying incorrectly.
>         >     >>
>         >     >> Patch_004 is attached, Thomas can you confirm that it fixes the
>         >     display for you?
>         >     >>
>         >     >> Kind Regards,
>         >     >> Oliver
>         >     >>
>         >     >> On Mon, Apr 17, 2017 at 7:53 AM, Oliver Walters
>         >     <oliver.henry.walters@xxxxxxxxx <mailto:oliver.henry.walters@xxxxxxxxx>
>         <mailto:oliver.henry.walters@xxxxxxxxx <mailto:oliver.henry.walters@xxxxxxxxx>>
>         >     >
>         >     > Good work, Oliver!
>         >     >
>         >     > I found 2 issues (tested on W7)
>         >     >
>         >     > 1 - m_reloadTableButton is not correctly enabled/disabled.
>         >     > This is due to the way events are managed, and this is OS dependent.
>         >     > To avoid this issue, enable/disable it inside a wxUpdateUIEvent
>         >     attached to this button.
>         >     >
>         >     > 2 - ESC key and ENTER keys do not dismiss the dialog.
>         >     > This is due to the fact you do not have a wxStdDialogButtonSizer,
>         >     and no OK and Cancel button.
>         >     > Please, add it and use the OK button (as usual in a dialog) to
>         >     transfer changes to schematic (do not
>         >     > use a wxCloseEvent to manage that), and obviously Cancel just
>         >     closes the dialog.
>         >     > To do this transfer, just  override TransferDataFromWindow(), that
>         >     is called by wxWidgets when
>         >     > closing a dialog by the OK button.
>         >     >
>         >     > About other things, undo/redo lists should manage only changes
>         >     made inside the corresponding sheet,
>         >     > not in other sheets, to avoid inconsistencies and therefore crashes.
>         >     >
>         >
>         >     This is one of the reasons I've been reluctant to accept code that
>         >     attempts to change the state of a SCH_SCREEN object other than the
>         >     current SCH_SCREEN object.  It exposes a known flaw in our schematic
>         >     undo/redo design and I have yet to see anyone update the undo/redo
>         >     SCH_SCREEN stacks correctly.  I see the potential for serious issues if
>         >     you do not keep the undo/redo stacks properly synced.  Once you allow
>         >     the modification of information in the SCH_SCREEN object other than the
>         >     current one, you need to update the undo/redo stack for the appropriate
>         >     SCH_SCREEN object.  Otherwise, you wont be able to undo all of the
>         >     changes correctly.


-- 
Jean-Pierre CHARRAS