Add basic Undo/Redo functionality to Entry widget

[nullst]

Description:

This pull request introduces a basic Undo/Redo functionality in the Entry widget, fixing issue #436 . The realization works as is for me, but I have some questions, presented slightly below. I am also working on writing tests for the new functionality.

Basic description

If Entry.HistoryDisabled is false, whenever a user performs an action like, e.g., "typed rune", "paste from clipboard", or "erase selection by pressing backspace", the full state of the entry widget is saved and appended to an action log. The state includes full text, cursor position, and scroll position. When user calls "Undo" on a history-enabled widget, the state is restored from the action log. Redo is implemented in a similar way.

To improve user experience, a sequence of "typed rune" actions performed in a quick succession is considered a single action, so Undo does not cancel just one letter at a time. Same for a sequence of deletions (e.g., pressing backspace several times).

Calling SetText() erases the history, with a new SetTextUndoable() function that adds a new event to the history log instead.

For simplicity, undo action resets the current selection to empty.

Justification for the crude mechanism

Better history implementation would store only user actions and roll them back in a smart way when requested. Saving the whole widget state is slower and wasteful in terms of memory, but this is the approach used here. Why?

• Resistant to modifications of Entry internals. RichText had just been introduced in Fyne 2.1, and I expect further changes to Entry internals to follow soon. A smarter approach working with diffs between RichText trees may become obsolete as RichText progresses.
• This is a first draft. Internal optimizations may follow later. Designing public APIs is separate from the implementation details.
• Performance of undo is not a bottleneck. For short texts or short-lived entry widgets, the overhead is negligeable. For a long-lived entry with long text, well, this would not be the only performance issue (see issue Focused text entry consumes 40%-80% of CPU in my app #1946 etc).

Some requests for feedback

Design questions

1. As mentioned, selection is just reset during the undo. Is this OK?
2. When some text is selected and user types the rune "k", the selected text is replaced by the letter "k", and currently this is registered as a single action. Should it be split into two? Erasing the selection, then typing k.
3. I created fyne.ShortcutUndo and fyne.ShortcutRedo globally and on desktop bound them to Ctrl+Z and Ctrl+Y respectively. Is this OK? I'm not sure about Ctrl+Y.
4. I added "Undo" and "Redo" menu items to the right-click menu of an entry with history enabled. But I'm not sure how common this is. Should we only show "Undo" by default? Maybe show no items and rely on user implicit knowledge of Ctrl+Z? Should there be a separator line between undo and the clipboard actions?
5. Should history be disabled for password entries?
6. What should I do with scroll position? I'm currently saving Entry.scroll.Offset and Entry.content.scroll.Offset and restoring them. This is wrong if entry has been resized, for example. In terms of user experience, ideally the scroll position should not be modified if the part of the text modified by the Undo() is already visible to the user, but this is impossible to implement nicely with a crude history mechanism.
7. This pull request probably adds more data races. For example, restoreHistorySnapshot() would ideally restore the widget state completely while holding propertyLock(), but the current implementation of updateText() is not particularly conductive to this (and setFieldsAndRefresh in general). Someone should take a look if there are severe problems with concurrency in this code, I'm not very experienced with this.

Implementation questions

• Since the undo stack behavior changes based on the delay between successive TypedRune() events, for the purpose of testing I used kind of a "dependency injection": Entry.timestamper is usually a reference to time.Now function, but may be changed in internal tests. Is this acceptable?
• I've added some basic tests for Undo/Redo, but I don't know if that's enough.

Public API changes

Added to widget.Entry:

• CanUndo(), CanRedo() func() bool: return true if entry history is enabled and the corresponding action can be performed. Can be used by applications for disabling menu items or toolbar buttons.
• Undo(), Redo() func(): perform undo/redo.
• SetTextUndoable func(text string): sets the text of the entry to the provided string and saves this as a separate action in the action log.

Modified behavior in widget.Entry:

• SetText() now erases the action log if history is enabled.

New shortcuts:

• fyne.ShortcutUndo, fyne.ShortcutRedo: new shortcuts with no associated data (just struct{}). On desktop they are bound to Ctrl+Z and Ctrl+Y.

Checklist:

• Tests included.
• Lint and formatter run with no errors.
• Tests all pass.
• Public APIs match existing style.
• Any breaking changes have a deprecation path or have been discussed.

[andydotxyz]

This is amazing to have worked on thanks!

If Entry.HistoryEnabled is true

Is it likely that developers would want to disable undo/redo? I'm not sure this is needed. If it is then certainly should not be disabled by default!

You're right that Entry will be changing as we move the ability to edit from a single style to rich styles.
The approach you take here is clean, I had not thought about doing it this way - but when you store only the string content it won't work if there is any rich text metadata available - which will happen before next release.

I wonder if you may be right about memory concerns. If someone loads a large text file then every time someone types a key it will add the whole amount again. So a 100k text file will be taking 10MB after just 100 keystrokes.

Just on API naming did you consider CanUndo instead of IsUndoAvailable? Last thought, SetTextUndoable I am not sure about - just like how history should be on by default I think we should maintain the history of SetText as app developers may do it on keystroke (there are some examples of this I think we have seen). Instead of that complex thought, how about SetText maintains history but a ResetUndoHistory or similar could be used if that was required?

[nullst]

Some thoughts as a partial response. Sorry for the length!

Is it likely that developers would want to disable undo/redo? I'm not sure this is needed. If it is then certainly should not be disabled by default!

This flag is present mostly to mitigate the disadvantages of the current undo implementation (lots of memory duplication etc). I think you are right that history tracking should be enabled by default.

Just on API naming did you consider CanUndo instead of IsUndoAvailable?

That's better, thanks!

Last thought, SetTextUndoable I am not sure about - just like how history should be on by default I think we should maintain the history of SetText as app developers may do it on keystroke.

I would argue otherwise. It seems to me that, typically, Entry.SetText invoked by an application corresponds to a user action elsewhere in the application (not in the entry itself), and thus if the action is to be undone, this should also be initiated from elsewhere in the application, not from the entry itself, since more things need to be changed. From this I reasoned that the default behavior of SetText should reset Undo history, with a special "undoable" variant for exceptional use cases like the ones you mentioned. Sorry for a messy sentence, it's hard to justify a gut feeling, but here are some thoughts/examples:

• switching notes in Fyne notes application calls SetText on the note-title and note-body entries to replace the text. Calling "Undo" at, say, note title entry after switching entries would put the title of the previous note in it while not modifying the note body. This is undesirable. If anything is to be undone, it's the whole "switching notes" action, and this should be the responsibility of the application, not of a single entry.
• If the entry is normally empty and the goal of an entry is to collect user input (imagine an entry in a form), a natural reason for an application to call SetText() on this entry would be if some new information has allowed the application to infer a better "default" value than just an empty string. For example, a registration form to some service could ask the user to put in full name in some field, and based on the input fill a default suggestion for the profile name. I would expect this not to be undoable back to the empty string.

Not sure how convincing this is, it's just in my (very limited) experience with GUI application, all the situations in which SetText() is used are similar to one of the two situations sketched above, and making SetText() cancellable by default would: (a) make existing apps behave strangely; (b) make it easy to accidentally write similar strange behavior for new applications.

In general, if the entry content depends on something else in the application (and why else would SetText() be called?), restoring the entry state directly, without any coordination with the other parts of the application, is a very suspicious action and, I think, should not be allowed unless the develop explicitly requests this by calling SetTextUndoable.

[nullst]

As for the future RichText compatibility, would it be reasonable to add a DeepCopy method to the RichTextSegment interface that returns a deep copy of the segment? Then Undo mechanism could be implemented almost as simply as for plain text. (Making a deep copy by force using reflect package is possible, but not good).

This would have further benefits: in the rich text modification functions (deleteFromTo and insertAt) we can record which elements of theRichText.Segments slice were changed/added/deleted during those operations, and let the history log store only the (copies of) modified segments. This would be easier than computing substantial diffs between states, and maybe still valuable. Still extremely far from optimal since RichText.Segments is kind of a tree, not just a slice (see ParagraphSegment, ListSegment).

(Maybe in the future Entry would be smart enough to represent very long texts as several different TextSegments as an optimization, and potentially this could improve the performance generally, including Undo if realized as above.)

[andydotxyz]

• switching notes in Fyne notes application calls SetText on the note-title and note-body

Good argument :) I agree now thanks.

As for the future RichText compatibility, would it be reasonable to add a DeepCopy method to the RichTextSegment interface that returns a deep copy of the segment?

It would be worth looking at that yes. But you don't need to export the method as we're internal to widget package. But I think it would need to copy the whole tree.

(Maybe in the future Entry would be smart enough to represent very long texts as several different TextSegments as an optimization, and potentially this could improve the performance generally, including Undo if realized as above.)

Not sure if this is the right way to optimise or not, if we record the actual changes (and where the cursor was) we can avoid cloning trees and diffing, so don't need to update the data structure to work efficiently.

[nullst]

But you don't need to export the method as we're internal to widget package. But I think it would need to copy the whole tree.

Sure, that's a good point. Copying the tree is the "deep" part of the "deep copy" :)

if we record the actual changes (and where the cursor was) we can avoid cloning trees and diffing

It wouldn't be easy to do this for a (future) fully RichText-based Entry without any "deepCopy" method on RichTextSegments (assuming that RichText still provides deleteFromTo etc). For example, if a user deletes symbols from 100th to 400th of the text, and symbols 200-300 are a ListSegment with some items, we'll need to store a copy of that deleted list in the action log to be able to undo/redo this operation. I mean, either store a copy or compute some kind of a diff between trees, and store that.

Overall, I'm not sure an implementation of a smarter undo/redo mechanism is worth attempting before RichText has been stabilized and integrated into Entry.

[andydotxyz]

Overall, I'm not sure an implementation of a smarter undo/redo mechanism is worth attempting before RichText has been stabilized and integrated into Entry.

Probably true.
Should we hold back and get rich edit into Entry first, or would you be happy to monitor and create a new PR when that happens?

[nullst]

I think holding back until Entry embraces RichText fully is more sensible. With these modifications I now have an Undo/Redo functionality in an application I'm writing, which makes me content, and when RichText comes around, I'll try to port the modifications.

[andydotxyz]

OK thanks. Moved it to draft so we can skim over it in the list for now.

[andydotxyz]

We are tidying up old PRs and this one seems unlikely to merge soon.
However I know that RichEntry is aiming for the next major release - so are you able to help update this, then port it in @nullst or should it be closed and re-worked after the new widget changes?

[nullst]

However I know that RichEntry is aiming for the next major release - so are you able to help update this, then port it in @nullst or should it be closed and re-worked after the new widget changes?

I'm not sure if I'll have time to help port this code, so maybe this pull request should be closed.

I'd just like to mention that since the undo mechanism implemented here is quite crude (as explained in the original description), essentially the only thing that's needed to adapt the code for RichEntry is a way to save and restore the content of RichEntry. For the usual Entry the code here just saves the text string (in the historySnapshot function) and sets it back (in the restoreHistorySnapshot function).

It seems that a proper way would be to add a DeepClone() function to the RichTextSegment interface and implement it for each segment type. Then possibly some complications could arise with respect to saving/restoring the cursor position and the scroll position of the widget, but that should be simple.

[Jacalz]

This was replaced by #4251 a while back. Thanks for working on this @nullst.