ENB: big problems with undo/redo tree operations

[Edward K. Ream]

While working on issue #3435, I see that u.saveTree can't possibly work! About a dozen commands and methods call u.saveTree indirectly. Undo and redo for these may be dangerous!

This Engineering Notebook post discusses what to do about this situation.

tl;dr: See the summary.

The problem

A cff shows that about a dozen commands and script helpers call u.beforeChangeTree and u.afterChangeTree. Both these methods call u.saveTree(p).

Alas, u.saveTree has no chance of working. As a result, Leo's undo logic is dangerously flawed in about a dozen places.

The section << about u.saveTree >> discusses u.createVnodeUndoInfo, u.saveTree's helper. These comments do not describe u.createVnodeUndoInfo correctly. Worse, u.createVnodeUndoInfo itself has no chance of solving the original problem! Details omitted.

Solutions

Imo, the following commands should not be undoable: refresh-from-disk, read-at-auto-nodes, read-at-file-nodes, and read-at-shadow-nodes. These commands are "one-way" updates. Rewriting their undo code would be fraught. Bugs in the undo/redo logic could be devastating.

Various commands and methods create a single new node. Their undo logic can mimic that of undo/redo insert-node. Undoing these commands would delete the newly-created node. Redo would undo the delete.

The abbrev.expand_tree method might be a unique case. I'm not yet sure what to do.

Summary

- We must retire u.saveTree, u.beforeChangeTree, and u.afterChangeTree.

- Several commands should not be undoable.

- Commands that insert a single node can use the undo/redo logic of insert-node.

All of your comments are welcome.

Edward

[Edward K. Ream]

On Thursday, July 13, 2023 at 7:07:10 AM UTC-5 Edward K. Ream wrote:

Various commands and methods create a single new node. Their undo logic can mimic that of undo/redo insert-node. Undoing these commands would delete the newly-created node. Redo would undo the delete.

All those commands relate to git-diff. On second thought, none of them need to be undoable.

Why? Because these commands create the last top-level node of the outline. I never want to undo the new node immediately, which is the only time an undo command would work!

Instead, I delete the git-related node in some distant future.

Summary

Not all commands need to be undoable.

I shall eliminate the undo logic for all commands using u.beforeChangeTree, and u.afterChangeTree. This policy will help Félix complete his work on leoJS.

I'll reconsider this choice only if it causes practical problems.

Edward

[Edward K. Ream]

On Thu, Jul 13, 2023 at 7:49 AM Edward K. Ream <edre...@gmail.com> wrote:

> On second thought, none of [the git-diff commands] need to be undoable.

Probably still true, but it may be possible to undo them easily enough.

All code that contains changed undo/redo code will be covered with new unit tests.

Edward

[Edward K. Ream]

On Thursday, July 13, 2023 at 8:57:57 AM UTC-5 Edward K. Ream wrote:

On Thu, Jul 13, 2023 at 7:49 AM Edward K. Ream <edre...@gmail.com> wrote:

> On second thought, none of [the git-diff commands] need to be undoable.

Otoh, undoing an accidental abbreviation is essential. It's the only hard case, but I am confident that u.before/afterChangeGroup can do the job.

Edward

[Félix]

There's no shame in having big data-changing commands not be undoable! Big software packages often have such commands!

On the other hand, it should be somewhat indicated when an action is not undoable. (perhaps add "This action is not undoable" in the string where the user is asked about refreshing from disk when an external file change is detected. idem for when an opened .leo file changes, and other non-undoable actions, when possible.) 

But the most important thing is to wipe clear the undo stack when such an action occurs, as undoing past an non-undoable action will corrupt the logical integrity and state of the outline with nonsensical structures and content. (as all other big software that i know also do as a standard)

Félix

[Edward K. Ream]

On Thursday, July 13, 2023 at 11:53:17 AM UTC-5 Edward K. Ream wrote:

> Otoh, undoing an accidental abbreviation is essential.

Happily, the existing code handles non-tree abbreviations.

Fully undoing a tree abbreviation would be harder. Happily, it's not necessary. Undo does work just as with non-tree abbreviations, but the inserted tree remains. The user can easily delete the tree.

Edward

[Edward K. Ream]

On Thursday, July 13, 2023 at 5:43:26 PM UTC-5 Félix wrote:

There's no shame in having big data-changing commands not be undoable! Big software packages often have such commands!

On the other hand, it should be somewhat indicated when an action is not undoable. (perhaps add "This action is not undoable" in the string where the user is asked about refreshing from disk when an external file change is detected. idem for when an opened .leo file changes, and other non-undoable actions, when possible.) 

But the most important thing is to wipe clear the undo stack when such an action occurs

Thanks for these comments! I'll do as you suggest.

Edward