ENB: Simplifying undo

[Edward K. Ream]

The goal of #1413 is to simplify Leo's undo api so that it is easier to write macro-like scripts.

The first thought was to continue the ongoing elimination of kwargs (keyword args) by eliminating kwargs from c.updateBodyPane, LeoBody.onBodyChanged, u.setUndoTypingParams, and others.

This is not going to be a typical refactoring! Imo, it would not be possible at all without the clone-find commands. Even with those commands, the complications are dizzying. In particular, LeoTree.onHeadChanged contains the comment "Tricky code: do not change without careful thought and testing".

The code is tricky because there are two paths through the code. The code can be called via event handlers (path one) or programmatically from Leo's core or user scripts (path two). The assumptions of the two paths are different.  As I write this, I see that it might be best to use two completely separate sets of code for each path, that is, one set for event handlers and one set for scripts. We shall see whether this is possible or desirable.

At present, unit tests might not actually find problems in the revised code, because unit tests run with the null (string-based) gui, not the actual Qt gui. Imo, the crucial unit tests will have to be run with the Qt gui.

Summary

Simplifying how scripts handle undo is worth a lot of work. This work will be tricky and dangerous.

I do plan to work on #1413 during the sabbatical, just for fun and as a break from endless study :-)

Edward

[Edward K. Ream]

On Saturday, November 7, 2020 at 6:42:42 AM UTC-6, Edward K. Ream wrote:

The goal of #1413 is to simplify Leo's undo api so that it is easier to write macro-like scripts.

The first thought was to continue the ongoing elimination of kwargs (keyword args) by eliminating kwargs from c.updateBodyPane, LeoBody.onBodyChanged, u.setUndoTypingParams, and others.

Work has gone surprisingly well. PR 1725 gives the full details.

The first phase is nearing completion.  It simplifies the logic concerning headlines. This is tricky because editing headlines involves ephemeral Qt "headline editor" widgets.

The key to eliminating kwargs was to create new u.before/afterChangeHeadline methods. These are specialized to headlines, thereby reducing the complexity of u.before/afterChangeNodeContents.

The old headline code contained extremely ugly hacks, centering around the tree.revertHeadline and tree.revertVnode ivars.  These ivars are now gone. However, it is now possible to have changes revert with the following sequence of actions:

- Ctrl-I: New headline
- type abc
- *click* another node: the headline appears to stick.
- Double click abc node: the headline reverts to newHeadline

Note: The reversion does not happen when using Ctrl-H instead of the double click.

I am not at all upset or worried about this glitch. In fact, I would be more worried if the reversion didn't happen. Clearly, there was a reason for the previous hacks :-) I am fairly confident that I can fix this problem more cleanly than before.

Summary

The first phase of the cleanup focused on headlines. More work remains.

The second phase will clean up all the code relating to changing body text. This is the main focus of #1413.

The second phase will not involve the difficulties with ephemeral headline widgets. Otoh, the second phase involves much more complex calling sequences.

The clone-find commands are essential for the second phase. There is no other way to organize complex search results. In effect, cff makes me smarter, because it augments my memory for detail after detail.

#1413 is a worthy part of the sabbatical. It promises to greatly simplify how Leo (and external scripts) handle changes to body text.

Edward

[Thomas Passin]

On Sunday, November 8, 2020 at 9:08:19 AM UTC-5, Edward K. Ream wrote:

The first phase is nearing completion.  It simplifies the logic concerning headlines. This is tricky because editing headlines involves ephemeral Qt "headline editor" widgets.

Ephemeral -  do you mean you instantiate a new widget each time?  When I've written this kind of thing I have usually reused the same one.  I haven't done it with QT, though.

 

... However, it is now possible to have changes revert with the following sequence of actions:

- Ctrl-I: New headline
- type abc
- *click* another node: the headline appears to stick.
- Double click abc node: the headline reverts to newHeadline

Note: The reversion does not happen when using Ctrl-H instead of the double click.

If the same widget were getting reused, I would have thought that in the double-click case it wasn't getting loaded with the current headline, so the old one would still be in the widget.

[Edward K. Ream]

On Sun, Nov 8, 2020 at 8:43 AM Thomas Passin <tbp1...@gmail.com> wrote:

On Sunday, November 8, 2020 at 9:08:19 AM UTC-5, Edward K. Ream wrote:

The first phase is nearing completion.  It simplifies the logic concerning headlines. This is tricky because editing headlines involves ephemeral Qt "headline editor" widgets.

Ephemeral -  do you mean you instantiate a new widget each time? 

Yes. This is baked into the Qt code. Qt now has so-called "persistent" headline editors, but this requires Qt 5.10+, and I don't want that new dependency.

Happily, I have discovered an elegant simplification of Leo's headline code, which eliminates all of the previous ugly hacks.  Stay tuned.

Edward

[Edward K. Ream]

On Sunday, November 8, 2020 at 8:08:19 AM UTC-6, Edward K. Ream wrote:

> Work has gone surprisingly well. PR 1725 gives the full details.

Yesterday was a very good day for Leo...

Breakthrough: Tracking headline changes for undo

qtree.endEditLabel now just closes the headline editor calling w.closeEditor! This in turn fires a callback, editingFinished_callback. This callback is very similar to the old qtree.onHeadChanged method, which no longer exists!

Why did I never think of this before? Probably because I was fixated on the base LeoTree.onHeadChanged method. But when using the qt gui there is never any need ever to call qtree.onHeadChanged directly!

Aside: I'm dithering about whether to prevent scripts from calling qtree.onHeadChanged. If a script called g.app.gui.onHeadChanged, it would in fact be calling LeoTree.onHeadChanged. But I doubt that would matter.

Simplifying undo

> The key to eliminating kwargs was to create new u.before/afterChangeHeadline methods. These are specialized to headlines, thereby reducing the complexity of u.before/afterChangeNodeContents.

These methods simplified editingFinished_callback. Furthermore, they simplified my thought processes, which probably was an essential precondition for the breakthrough.

Today I'll add similar u.before/afterChangeBody methods. It might be possible to simplify some of Leo's existing commands using those methods, but that remains to be seen.

Nevertheless, it would be perfectly fine to leave Leo's body-changing code unchanged. In other words, u.before/afterChangeBody will primarily be for the use of scripts.

Ahas re undo granularity

Many of Leo's body-changing commands eventually call u.setUndoTypingParams. This is clumsy, and the calling sequenced pretty much require lots of ugly kwargs.  However, in most cases (eventually) calling u.setUndoTypingParams is not required. Calling the upcoming u.before/afterChangeBody methods would suffice!

Indeed, u.setUndoTypingParams contains complex logic to handle the so-called "granularity" of changes to body text. This is extremely important when handling the endless changes created as the user types in the body pane. But Aha:

    undo granularity does not matter for commands that change body text!

In other words, Leo can, and should, create a new undo "bead" for each text-altering command, regardless of undo granularity. In other words, commands should just call u.before/afterChangeBody, not (circuitously) u.setUndoTypingParams.  In other words:

   u.setUndoTypingParams is really a callback for onTextChanged events!

This Aha will guide me if I do decide to refactor or eliminate ugly methods such as c.updateBodyPane and LeoBody.onBodyChanged. u.setUndoTypingParams is now deprecated for new commands, and in scripts. But to repeat, there is no real need to avoid u.setUndoTypingParams. Existing code is fine as it is.

Summary

Leo's headline code has collapsed. From now on, the new u.before/afterChangeHeadline methods are the recommended way of handling undo/redo (in headline text) in scripts, and in Leo's core.

Similarly, the upcoming u.before/afterChangeHeadline methods will be the recommended way of handling undo/redo (in body text) in scripts, and in Leo's core.

u.setUndoTypingParams is deprecated for commands involving body text. It is really a too-visible callback for qt onTextChanged events. But there is no need to change Leo's existing body-changing commands.

Leo's undo documentation will be revised to discuss the new undoer methods. This section needs examples, and the new methods will make the examples much simpler and easier to understand.

Edward

P.S. None of this work would have been possible 10 years ago. It would have been out of the question without Leo's clone-find and git-diff commands, git branches, and pylint. Focusing on eliminating kwargs was also an essential part of the discovery process.

Many clone-find commands were needed to reveal the Aha's discussed above. Even more clone-find commands were needed to untangle the relationships between c.updateBodyPane, LeoBody.onBodyChanged, and u.setUndoTypingParams.

I'll discuss a new pattern involving clone-find in another thread.

EKR