stb_textedit: Undo weirdness after failed paste

[Unit2Ed]

We've noticed some weirdness caused by the undo_point being erroneously adjusted after a failed paste.

We believe this is happening because stb_textedit_paste_internal decrements state->undostate.undo_point if the paste fails, but the undo hadn't been created yet (contrary to the comment above the decrement). It looks like the undo it's trying to remove is the one created by stb_text_makeundo_insert, which won't have happened if the insertion failed.

This is the current version of stb_textedit_paste_internal:

// API paste: replace existing selection with passed-in text
static int stb_textedit_paste_internal(STB_TEXTEDIT_STRING *str, STB_TexteditState *state, STB_TEXTEDIT_CHARTYPE *text, int len)
{
   // if there's a selection, the paste should delete it
   stb_textedit_clamp(str, state);
   stb_textedit_delete_selection(str,state);
   // try to insert the characters
   if (STB_TEXTEDIT_INSERTCHARS(str, state->cursor, text, len)) {
      stb_text_makeundo_insert(state, state->cursor, len);
      state->cursor += len;
      state->has_preferred_x = 0;
      return 1;
   }
   // remove the undo since we didn't actually insert the characters
   if (state->undostate.undo_point)
      --state->undostate.undo_point;
   return 0;
}

We've fixed this locally by removing the decrement, but I wanted to check your thoughts on this.

EDIT:
Forgot to mention that we've seen a soft-lock caused by this too in the case where the undo_char buffer has been filled up followed by a failed paste. stb_text_create_undo_record will lock-up in this while loop because stb_textedit_discard_undo does nothing if undo_point==0, which can happen due to the erroneous decrement.

   // if we don't have enough free characters in the buffer, we have to make room
   while (state->undo_char_point + numchars > STB_TEXTEDIT_UNDOCHARCOUNT)
      stb_textedit_discard_undo(state);

[ocornut]

Hello,

I'm looking at this now:

Forgot to mention that we've seen a soft-lock caused by this

For maintainer: I can confirm this, ocornut/imgui#4038 has a repro which should be easy to replicate outside of Dear ImGui (by using same inputs sequence and same buffer size). Dear ImGui doesn't change the default value of STB_TEXTEDIT_UNDOCHARCOUNT/STB_TEXTEDIT_UNDOSTATECOUNT.

---

It looks like the undo it's trying to remove is the one created by stb_text_makeundo_insert, which won't have happened if the insertion failed.

The wording in comment is a little ambiguous, but the code might have been aimed at undoing the work of the stb_textedit_delete_selection() call that happened BEFORE the insertion, not the work of STB_TEXTEDIT_INSERTCHARS(). Deleting selection can insert an undo point but does not always.

The failure conditions are:

(A)

• pasting does happen WITHOUT a selection, stb_textedit_delete_selection() doesn't insert a undo point
• then the insertion fails
• code erroneously removed undo point

and

(B)

• pasting happens with a selection, but undo point cannot be created (deleted text larger than STB_TEXTEDIT_UNDOCHARCOUNT).
• then the insertion fails
• code erroneously removed undo point

Fix v1

Exactly as suggested above, deleting that block is a simple fix.

   // remove the undo since we didn't actually insert the characters
   if (state->undostate.undo_point)
      --state->undostate.undo_point;

On a paste failure it will however leave the selection deleted (which can be undone with a further ctrl+z).
Considering that paste failure are not common/gracious events to be honest it may be good enough and it has the advantage of being a trivial fix.

---

Fix v2

If we want paste failure to leave the buffer untouched (no deletion of selection) we'd need to essentially undo the deletion.

I first tried to compare value of undo_point before/after stb_textedit_delete_selection()... but this is NOT possible. In particular stb_text_create_undo_record() in the if (state->undo_point == STB_TEXTEDIT_UNDOSTATECOUNT) codepath may discard one to allow one to be created, leaving us with same count. AFAIK it is not possible for a caller of stb_textedit_delete_selection() to 100% tell if the function created an undo point or not (other than copying/restoring the whole StbUndoState structure), but adding a simple counter in the structure that gets incremented on each action can do the job:

1. Add int unique_created_records; to StbUndoState structure.

typedef struct
{
   ...
   int unique_created_records;
} StbUndoState;

2 Increment:

static StbUndoRecord *stb_text_create_undo_record(StbUndoState *state, int numchars)
{
   ...
   state->unique_created_records++;
   return &state->undo_rec[state->undo_point++];
}

3. Use to trigger undo

static int stb_textedit_paste_internal(STB_TEXTEDIT_STRING *str, STB_TexteditState *state, STB_TEXTEDIT_CHARTYPE *text, int len)
{
   // if there's a selection, the paste should delete it
   int unique_created_records_before_deletion;
   int unique_created_records_after_deletion;
   stb_textedit_clamp(str, state);
   unique_created_records_before_deletion = state->undostate.unique_created_records;
   stb_textedit_delete_selection(str,state);
   unique_created_records_after_deletion = state->undostate.unique_created_records;
   // try to insert the characters
   if (STB_TEXTEDIT_INSERTCHARS(str, state->cursor, text, len)) {
      stb_text_makeundo_insert(state, state->cursor, len);
      state->cursor += len;
      state->has_preferred_x = 0;
      return 1;
   }
   // remove the undo since we didn't actually insert the characters
   if (unique_created_records_after_deletion > unique_created_records_before_deletion)
      stb_text_undo(str, state);
   return 0;
}

With this a paste failure (no room for insertion) should leave buffer untouched IF there is room for deletion in the undo buffer in the first place. So in a way it is an improvement over V1.... or it is?

Considering that CTRL+A followed by CTRL+V may be common sequence leading to paste failure, and that "storing old text in undo buffer" is as likely to fail as actual new text insertion (because undo buffer is fixed size and cannot be resized): one could argue that fix v1 (always leave old text deleted on paste failure, might still be in undo stack) has the merit to be more consistent than fix v2 (only leave old text deleted on paste failure if couldn't be stored in undo stack). Neither are perfectly right but if I have to be honest, in 6+ years no one complained of storage limitations in the undo buffer other than bugs so I'd say Fix v1 is fine and has the merit to be simpler.

[rygorous]

Fix for this is merged into dev branch, will be in the next release.