Page MenuHome

Replace text editor operation based undo with binary diffing (avoids sync issues, more reliable)
ClosedPublic

Authored by Campbell Barton (campbellbarton) on Jul 11 2019, 8:12 AM.

Details

Summary

Today I've been looking into fixing undo for the text editing (for 2.8x release) and I've found to correctly fix this would require some very strange logic.

This fixes:

The root of the issue if what all undo steps that use an ID's need to apply on top of the last memfile step (since ID data blocks can be added and removed, we can never step into an operation on an ID data-block which doesn't exist).

This isn't a problem for undo systems that store their state on each step, however the text editor accumulates changes.

Using undo which works this way doesn't work well when mixing it with memfile-undo.

A fix for the current system could be to either make an exception where text is allowed to use the memfile-undo state from the future

Or loading the last memfile-undo state, then replaying all steps up until the current state, requiring is to keep all text editor undo steps after a memfile-step, also replaying the steps could become slow.

While both could be made to work they're going to introduce some quite awkward logic into the undo system just to support text editing undo.

Performance

This patch simply stores the entire text buffer for each state, de-duplicating data via BLI_array_store.

I was concerned performance could be an issue, however testing with a 40,000 line file (1.88mb), editing isn't noticeably slow. At 200,000 lines (9.5mb), I could notice a difference although it's still usable.

Note that the initial patch freed all lines and recreated them, it's since been been updated to use existing lines when available which improves performance in common cases such as entering & removing single characters.

Room for improvement

This patch could be improved in a few ways.

  • Currently a single BArrayStore is used for all undo steps, it would be better that each Text gets it's own.

Pros

  • Simple, easy to maintain undo logic.
  • Reliable.

    Current undo logic can fail at times, testing 2.79b I managed to get it to crash 3 times in a row (performing many random edits, undoing & redoing ~ thrashing).

    Not a great argument, since these bugs could be investigated, reported & fixed, nevertheless - after years of having a text editor, we didn't manage to make undo work all that reliably.
  • Extensions/Python operators.

    Currently any Python text operations don't get properly handled by undo, with this patch we wouldn't need to undo system to be aware of different text manipulation actions.

Cons

  • Performance (in practice I don't think it's so bad, nevertheless, it is worse in some cases).
  • Undo isn't aware of the location of changes. Knowing this information could be useful in the future, although currently we don't make use of it, noting it as a 'con'.

Since Blender isn't primarily a text editor, and this patch works acceptably, I'm proposing to use simpler logic to avoid having to maintain the current system.

Diff Detail

Repository
rB Blender
Branch
TEMP-TEXT-UNDO-MEMCMP
Build Status
Buildable 4089
Build 4089: arc lint + arc unit

Event Timeline

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)
  • Remove unused defines, comments
  • Remove more unused code
Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)
Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)
Campbell Barton (campbellbarton) retitled this revision from Use BLI_array_store for text undo to Use BLI_array_store for text undo (fixes undo sync issues).
Campbell Barton (campbellbarton) retitled this revision from Use BLI_array_store for text undo (fixes undo sync issues) to Replace text editor operation based undo with binary diffing (avoids sync issues, more reliable).

I think this is a good solution. Can you clarify what you mean by "for 2.8x release", I assume that means 2.81 or later and not 2.80?

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)
  • Speed up buffer conversion
  • Minor optimization for buffer conversion
Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)EditedJul 15 2019, 9:36 AM

For 2.80, current text editor undo is quite unstable.

Although I can make 2.79 release crash too, so it's not clear how seriously crashes in this area should be taken.

It just means any crashes in 2.80 text editor undo would be marked as *wontfix* unless they're trivial.


Update, added optimized buffer conversion in both directions, tested 200,000 line (9.5mb) text editing and it's usable, so I think performance isn't a concern anymore.

It's still not clear to me if this is fixing important regressions compared to 2.79 or not? Do you think this should be in 2.80 or 2.81?

My intention was to have this for 2.80 because this adds new bugs on-top of the existing bugs in 2.7x,
on first testing 2.80rc I managed to get a crash in about 10 seconds of thrashing undo/redo/editing.

Narrowed this down to: T67045: Crash undo/redo in the text editor


Big picture, undo in 2.8x is global, so you can hold down ctrl-z and roll back all changes in all modes.

If there are some random crashes in some parts of Blender I think it undermines undo in general.
Users should be able to rely on this not loosing their work.

It's the kind of thing we just don't want crashing _ever_.

Since this was something I knew was broken and intended to fix, it's not very nice to include something so broken in a stable release, especially when there is a fix available.

Of course the fix needs to be tested well too, or it could backfire.

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)

Ok, let's try to still add this then. I will review the code.

To avoid this kind of last-minute thing in the future, this should have been flagged as high priority earlier, and fixing undo should have had priority over some other changes that were done in the last months.

Agree undo should have been higher priority, I was aware of it at the time too.

There is pressure to work on user visible things which are obviously wrong (and need to be fixed for release anyway), which kept pushing fixes for less interesting (such as the text editor) back.


Updated patch.

  • Don't attempt to add text editor undo steps for nested operator calls (fixes harmless assert for T65909).
  • Remove unused function.
Brecht Van Lommel (brecht) requested changes to this revision.Jul 16 2019, 2:54 PM
Brecht Van Lommel (brecht) added inline comments.
source/blender/blenkernel/intern/text.c
1298

This assumes there is always a \n at the end of the buffer? That's not obviously a safe assumption.

1327

Same here.

1330

Why not calloc?

source/blender/editors/space_text/text_undo.c
62

I'm not a fan of all these #ifdefs, code is easier to read and refactor if there's just a single code path.

81

Please use longer more descriptive name.

85

Please add comment explaining what um_arraystore is for, and maybe find a more descriptive name.

178

0 -> false

182–190

The non-array store case is leaking memory here.

This revision now requires changes to proceed.Jul 16 2019, 2:54 PM
  • Update based on code-review
Campbell Barton (campbellbarton) added inline comments.
source/blender/blenkernel/intern/text.c
1298

Renamed these functions and added comments to denote them as being specifically for undo.

Since we already have more general functions for converting from/to buffers which should be used in all other cases.

1330

None of the other line allocations of TextLine calloc, so it's safe not to.

source/blender/editors/space_text/text_undo.c
62

Useful for testing and they're kept for edit-mesh since this is more complex/error prone code case.

removed.

  • Tweak comment
This revision is now accepted and ready to land.Jul 17 2019, 12:59 PM