Undo, Redo Bug at Multi-line Text Input

[dodo1004]

I found a bug at Undo, Redo at Multi-line Text Input

After I copy (Ctrl-c) all of texts
move down new bottom line and paste (Ctrl-v) 3 time
and undo (ctrl-z) 3time and redo (ctrl-y) 1time

then it always crushed.
It is a writing access violation exception

it can be reproduced again and again.

and stopped at this line (memmove)

static bool STB_TEXTEDIT_INSERTCHARS(STB_TEXTEDIT_STRING* obj, int pos, const ImWchar* new_text, int new_text_len)
{
    const int text_len = obj->CurLenW;
    if (new_text_len + text_len + 1 > obj->Text.Size)
        return false;

    const int new_text_len_utf8 = ImTextCountUtf8BytesFromStr(new_text, new_text + new_text_len);
    if (new_text_len_utf8 + obj->CurLenA + 1 > obj->BufSizeA)
        return false;

    ImWchar* text = obj->Text.Data;
    if (pos != text_len)
===>       memmove(text + pos + new_text_len, text + pos, (size_t)(text_len - pos) * sizeof(ImWchar));
    memcpy(text + pos, new_text, (size_t)new_text_len * sizeof(ImWchar));

    obj->CurLenW += new_text_len;
    obj->CurLenA += new_text_len_utf8;
    obj->Text[obj->CurLenW] = '\0';

    return true;
}

[ratchetfreak]

You can fix the formatting by surrounding your code with 3 backticks `. You can also set the language for highlighting.

[dodo1004]

thanks I changed the formating

[ocornut]

I cannot seem to repro. Are you running Master?
There was a bug fix around similar code in #681

[dodo1004]

Yes I clone it from master.
I will make a video for easily making it again

[dodo1004]

First copy all of texts in Multi-line Text Input window
and paste 3 time and undo 3time and redo 1time then application will be crushed.

ctrl-c (whole area) > ctrl-v ( at last line) > ctrl-v > ctrl -v > ctrl-z > ctrl-z > ctrl-z > ctrl-y

(I know about #681 but it is not that version, I think that it remains some problems)

• I removed the video - now it is checked out

[ocornut]

OK i got it. Sorry I was doing CTRL+V again instead of CTRL+Y.

[dodo1004]

nope, it is good to be checked a bug.
oh. it is not good... it is a bug.. I do not know what I do say. in this case.
just I am looking forward to your great job, and I want it would be a help for imgui

[ocornut]

It's closely related to the same function as #681 which is a rarely exercised path. By default we have 999 characters worth for the various undo-buffers and when this overflow that code path tries to remove older entries.

After 3 pastes and 2 undoes

After 3 pastes and 3 undoes

At this point I think the undo stack state is incorrect.. but I'm still confused by this code. Will investigate.

By the way, I should definitively increase that number from 999 to maybe 10K, that's still a low footprint. But only when we have fixed the bug :)

[dodo1004]

We know a deep debugging is hard for everyone.
I do not know I gave you just a burden only.
I will find something to help you too.

[ocornut]

I am quite confused by the undo-stack system in stb_textedit.h, will probably end up asking Sean Barrett but I don't want to bother him before I obtain more information. I suspect there are a few other bugs in there.

Just doing Copy, Paste, Undo, Redo, Undo, Redo, Undo, Redo... Seems to exhaust the char buffer which I don't think should happen. I think the fix for that is to add this in stb_text_redo:

   if (r.insert_length) {
      // easy case: need to insert n characters
      STB_TEXTEDIT_INSERTCHARS(str, r.where, &s->undo_char[r.char_storage], r.insert_length);
++      s->redo_char_point += r.insert_length;
   }

But that doesn't fix the bug you are mentioning above. All of this intimately connected so I need to understand the logic behind stb_textedit_discard_undo stb_textedit_discard_redo before. Right now struggling to make sense of it.

[ocornut]

Other fix in stb_textedit_discard_redo
Replace

++state->redo_point;
STB_TEXTEDIT_memmove(state->undo_rec + state->redo_point-1, state->undo_rec + state->redo_point, (size_t) ((size_t)(STB_TEXTEDIT_UNDOSTATECOUNT - state->redo_point)*sizeof(state->undo_rec[0])));

By

STB_TEXTEDIT_memmove(state->undo_rec + state->redo_point, state->undo_rec + state->redo_point-1, (size_t) ((size_t)(STB_TEXTEDIT_UNDOSTATECOUNT - state->redo_point)*sizeof(state->undo_rec[0])));
++state->redo_point;

Test code:
A. Add #define STB_TEXTEDIT_UNDOSTATECOUNT 10 in imgui_internal.h

B. Display state:

#include "imgui_internal.h"
{
    ImGui::Begin("stb_textedit.h");
    ImGuiStb::StbUndoState& state = GImGui->InputTextState.StbState.undostate;
    ImGui::Text("undo_point: %d\nredo_point: %d\nundo_char_point: %d\nredo_char_point: %d\n", state.undo_point, state.redo_point, state.undo_char_point, state.redo_char_point);
    for (int n = 0; n < 10; n++)
    {
        ImGuiStb::StbUndoRecord& rec = state.undo_rec[n];
        ImVec4 text_col = (n >= state.undo_point && n < state.redo_point) ? ImVec4(0.7f,0.7f,0.7f,1.0f) : ImVec4(1.0f,1.0f,1.0f,1.0f);
        ImGui::TextColored(text_col, "[%02d] where %d, insert %d, delete %d, char_storage %d", n, rec.where, rec.insert_length, rec.delete_length, rec.char_storage);
    }
    ImGui::End();
}

I am not 100% sure and need to do more testing. It may be completely wrong.

[ocornut]

I am applying the fix now. Looking ok but awaiting further user testing. Let me know if you stumble on anything wrong.