Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The undo/redo system is overly complex and partially broken #78

Closed
pyscripter opened this issue Oct 29, 2021 · 5 comments
Closed

The undo/redo system is overly complex and partially broken #78

pyscripter opened this issue Oct 29, 2021 · 5 comments

Comments

@pyscripter
Copy link
Contributor

pyscripter commented Oct 29, 2021

The SynEdit undo/redo system looks like incoherent patchwork:

  • The code in the SynEdit repo contains no fewer than 24 change reasons (TSynChangeReason enumeration) many of which are not used.
  • It is overly complex and understanding how it works even superficially requires persistence and a significant time investment
  • It is partially broken as the note in the SynEdit.pas header states.
  • The code is split between the huge SynEdit.pas unit and SynEditTextBuffer.pas
  • Implementing a new editor command that changes the editor buffer and is undoable, requires a good understanding of how the undo/redo command works, thus limiting the potential for contributors to implement new features
  • It makes the implementation of major features as as multi-cursor support very hard

However it should be said that the underlying implementation is quite clever and efficient. (ChangeReason to group undo actions, InitialChangeReason to monitor the modified status of the buffer, etc.)

The refactoring in commit 35eb4bc and some of the earlier work done, gave the undo/redo system a facelift. Separated a large part of the implementation into its own unit SynEditUndo.pas, consolidated the change reasons down to 10 and simplified the implementation significantly. But the underlying issue still remains with SynEdit.pas littered with code related to undo/redo.

Proposal

Replace the undo/redo system with a new one that keeps some of the nice aspects of the current system but at the same time totally decouples the implementation from the basic editor functionality implemented in the main unit SynEdit.pas. The key idea is to record the changes to the buffer via the three events (LinePut, LinesInserted, LinesDeleted) and undo them as appropriate. This is robust and foolproof. There is no need for calls to AddChange in SynEdit.pas, so the implementation of new commands is very much simplified. Whenever you make changes to the editor buffer, these changes will be undoable automatically. It is also backwards compatible. The public interface to the undo/redo system will remain unchanged.

@pyscripter
Copy link
Contributor Author

pyscripter commented Oct 29, 2021

Implemented almost from scratch.

  • No more change reasons
  • Total separation of undo/redo from SynEdit.pas
  • SynEdit.pas is 400 lines shorter.
  • Tested undo/redo with different commands and with chained editors.

@JaFi-cz Please test and report back.

@JaFi-cz
Copy link
Contributor

JaFi-cz commented Oct 29, 2021

I am stacked now with my job work. I will implement it in the next PSPad build and we will see.

@MShark67
Copy link
Contributor

Just wanted to mention that I've tested these new changes with several techniques which would cause the old undo system to fail (mostly related to hard tabs and autoindents) and this new one passed with no issues. Nicely done!

@pyscripter
Copy link
Contributor Author

@MShark67 Just saw the above. Please keep on trying to break it.

@pyscripter
Copy link
Contributor Author

Forgot to say that in the same commit there were quite a few fixes related hard tabs and auto-intend that I discovered while testing the new undo redo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants