Details

Reviewers

Maniphest Tasks

Summary

Currently text fields where you enter numbers or names don't support undo and redo. This patch adds a relatively simple local undo / redo system for button text editing.

Since text field lengths are relatively short and the data short-lived, it makes sense to just store the entire text field for every undo step. But each undo step is a separate alloc to lower to total memory requirement for most quick interactions with text boxes.

Diff Detail

Repository

rB Blender

Branch

TEMP-UNDO-REDO-UI-v2 (branched from master)

Build Status

Buildable 8004
Build 8004: arc lint + arc unit

Event Timeline

Hans Goudey (HooglyBoogly) requested review of this revision.Apr 24 2020, 10:16 PM

Hans Goudey (HooglyBoogly) created this revision.

Hans Goudey (HooglyBoogly) added a task: T74453: Text field undo/redo.Apr 24 2020, 10:30 PM

Diff from latest master

stefano severi (Tetone) awarded a token.Apr 28 2020, 8:57 PM

Good start, requesting some changes:

Undo should clear the selection (currently it remains).
Split this into it's own file, eg interface_undo.c since this may be expanded later on and interface_handlers.c is on the large side (it can have ui_textedit_undo_stack_create, ui_textedit_undo_stack_destroy functions and manage the undo steps internally).

This revision now requires changes to proceed.Apr 29 2020, 2:13 PM

In D7518#182061, @Campbell Barton (campbellbarton) wrote:

Split this into it's own file, eg interface_undo.c

Wouldn't this require moving uiHandleButtonData to interface_intern.h? I'm happy to do that if you'd like. Otherwise pushing an undo state would have to decide exactly which information it would like to store.

In D7518#182252, @Hans Goudey (HooglyBoogly) wrote:

In D7518#182061, @Campbell Barton (campbellbarton) wrote:

Split this into it's own file, eg interface_undo.c

Wouldn't this require moving uiHandleButtonData to interface_intern.h? I'm happy to do that if you'd like. Otherwise pushing an undo state would have to decide exactly which information it would like to store.

No, as the API that handles this can take in arguments without having to know about uiHandleButtonData. The calls to pop the undo state in interface_handlers.c will need to clear the selection but they shouldn't need to do much else.

Campbell Barton (campbellbarton) added inline comments.May 1 2020, 7:11 AM

source/blender/editors/interface/interface_handlers.c
399–400	Note, this could also use a linked list, since it's not a performance bottleneck and it means there is no need for a fixed limit, as I doubt editing this text field would ever push memory limits.

Move undo to separate file
Use linked list to store states
Clear selection on undo / redo

Hans Goudey (HooglyBoogly) marked an inline comment as done.May 1 2020, 10:14 PM

This misses the interface_undo.c file.

Otherwise this mostly looks fine, although I'd prefer not to reuse UndoStack since from what I can see this isn't using other parts of BKE_undo_system.h API. A local struct declared in interface_undo.c should be enough.

This revision now requires changes to proceed.May 3 2020, 4:15 AM

Add missing file to diff
Fix unintentional UndoStack name reuse

Rebase on master

Harbormaster completed remote builds in B7956: Diff 24526.May 8 2020, 2:27 PM

Noticed an issue with uiHandleButtonData.maxlen use, this may change while editing text (See ui_textedit_string_ensure_max_length).

This could be solved by:

ui_textedit_redo / ui_textedit_undo could return a const string which is set using ui_textedit_string_set.
TextFieldState.text can be allocated to the strlen, no need to re-use these struct members or store invalid, pushing new undo steps can always clear future redo states.

This revision now requires changes to proceed.May 8 2020, 2:32 PM

Don't store maxlen, calculate the size of each new step instead
Don't reuse invalid redo steps

Thanks Campbell! I should have noticed that maxlen could change, oops!

Fixed modifier checks (alt key could be held for example, only use os-key on macOS).
Only clear selection when undo succeeds.
Separate ui_textedit_undo_stack_create from the initial push call - for simplicity.

Harbormaster completed remote builds in B7980: Diff 24596.May 11 2020, 5:57 AM

@Hans Goudey (HooglyBoogly), while applying this patch, I noticed the ui_textedit_undo_push_state are being handled for each kind of edit operation, making it possible to accidentally miss some actions (for example, auto-complete misses an undo-push).

Is there a good reason for this? The changed variable tracks when a string is changed, so the undo push could be moved to if (changed) { ... } block at the bottom of the function.

See: P1379.

Correct const in header

Harbormaster completed remote builds in B7981: Diff 24597.May 11 2020, 6:15 AM

use one call for pushing undo state

Harbormaster completed remote builds in B7993: Diff 24622.May 11 2020, 5:54 PM

Add missing NULL assignment after freeing the undo stack (was lost on update).
Clear redo steps before adding a new undo step (simplifies logic).
Allocate the text as part of the undo step (simplifies freeing).
Move undo/redo into a single function with a direction argument.
Use memcpy instead of BLI_strncpy (as size is already known).
Use 'ui' prefix for structs & name in a way that makes adding new undo stacks fit better.
Use __func__ for allocation names (as this text often gets missed when updating surrounding code).
Update comments.