Undo, Redo Bug at Multi-line Text Input

dodo1004 commented

2016-06-26 13:46:32 +00:00

(Migrated from github.com)

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;
}

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 commented

2016-06-26 13:52:52 +00:00

(Migrated from github.com)

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

dodo1004 commented

2016-06-26 14:16:59 +00:00

(Migrated from github.com)

thanks I changed the formating

ocornut commented

2016-06-27 07:46:37 +00:00

(Migrated from github.com)

I cannot seem to repro. Are you running Master?
There was a bug fix around similar code in https://github.com/ocornut/imgui/issues/681

I cannot seem to repro. Are you running Master? There was a bug fix around similar code in https://github.com/ocornut/imgui/issues/681

dodo1004 commented

2016-06-27 09:07:26 +00:00

(Migrated from github.com)

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

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

dodo1004 commented

2016-06-27 09:29:34 +00:00

(Migrated from github.com)

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

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 commented

2016-06-27 10:03:28 +00:00

(Migrated from github.com)

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

dodo1004 commented

2016-06-27 10:26:18 +00:00

(Migrated from github.com)

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

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 commented

2016-06-27 10:55:35 +00:00

(Migrated from github.com)

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 :)

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 ![3pastes2undoes](https://cloud.githubusercontent.com/assets/8225057/16377367/e2899a38-3c65-11e6-90fe-ed594513645b.PNG) After 3 pastes and 3 undoes ![3pastes3undoes](https://cloud.githubusercontent.com/assets/8225057/16377374/ef3ab032-3c65-11e6-9a61-7efdc2459dcf.PNG) 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 commented

2016-06-27 11:20:04 +00:00

(Migrated from github.com)

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.

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 commented

2016-06-27 11:27:44 +00:00

(Migrated from github.com)

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.

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 commented

2016-06-27 12:04:21 +00:00

(Migrated from github.com)

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.

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(); } ``` ![textedit](https://cloud.githubusercontent.com/assets/8225057/16379098/e526444e-3c6f-11e6-8d77-1965933984cc.gif) I am not 100% sure and need to do more testing. It may be completely wrong.

ocornut commented

2016-07-14 11:09:08 +00:00

(Migrated from github.com)

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

Rows
Columns

Undo, Redo Bug at Multi-line Text Input #715