Scroll bug when lines are selected in Multi-Line Input #725

New issue

Closed

opened 2016-07-06 04:37:23 +00:00 by dodo1004 · 5 comments

dodo1004 commented

2016-07-06 04:37:23 +00:00

(Migrated from github.com)

I found a wired bug recently and finally I can repro it again . so I report it here.
I think this time will be easier than last.
It has a scroll bug when lines are selected in multi line input.

I think that it may have selection memory and it is not updated when front memory is replaced.
and when we scroll the multi-line it goes unhandled state.

I made a video to explain it
https://youtu.be/gZCuNmdavpc

I found a wired bug recently and finally I can repro it again . so I report it here. I think this time will be easier than last. It has a scroll bug when lines are selected in multi line input. I think that it may have selection memory and it is not updated when front memory is replaced. and when we scroll the multi-line it goes unhandled state. I made a video to explain it https://youtu.be/gZCuNmdavpc

dodo1004 commented

2016-07-07 00:37:41 +00:00

(Migrated from github.com)

hey ocornut, did you check it out?

ocornut commented

2016-07-07 03:48:43 +00:00

(Migrated from github.com)

Not yet, sorry.

ocornut commented

2016-07-13 00:05:03 +00:00

(Migrated from github.com)

In the future please provide repro in the form of code.
This is a repro:

static char text[1024*16] =
    "/*\n"
    " The Pentium F00F bug, shorthand for F0 0F C7 C8,\n"
    " the hexadecimal encoding of one offending instruction,\n"
    " more formally, the invalid operand with locked CMPXCHG8B\n"
    " instruction bug, is a design flaw in the majority of\n"
    " Intel Pentium, Pentium MMX, and Pentium OverDrive\n"
    " processors (all in the P5 microarchitecture).\n"
    "*/\n\n"
    "label:\n"
    "\tlock cmpxchg8b eax\n";

static char text2[1024*16] =
    "/*\n"
    " The Pentium F00F bug, shorthand for F0 0F C7 C8,\n"
    " the hexadecimal encoding of one offending instruction,\n"
    " more formally, the invalid operand with locked CMPXCHG8B\n"
    " instruction bug, is a design flaw in the majority of\n"
    " Intel Pentium, Pentium MMX, and Pentium OverDrive\n"
    " processors (all in the P5 microarchitecture).\n"
    "*/\n\n"
    "label:\n"
    "\tlock cmpxchg8b eax\n"
    "/*\n"
    " The Pentium F00F bug, shorthand for F0 0F C7 C8,\n"
    " the hexadecimal encoding of one offending instruction,\n"
    " more formally, the invalid operand with locked CMPXCHG8B\n"
    " instruction bug, is a design flaw in the majority of\n"
    " Intel Pentium, Pentium MMX, and Pentium OverDrive\n"
    " processors (all in the P5 microarchitecture).\n"
    "*/\n\n"
    "label:\n"
    "\tlock cmpxchg8b eax\n";

ImGui::InputTextMultiline("Bug #725", text, 1024*32, ImVec2(-1.0f, ImGui::GetTextLineHeight() * 16));
if (ImGui::Button("Replace"))
    strcpy(text, text2);

The buffer size doesn't matter. I have now fixed 1 bug that blocked further input from happening (and it was by far the worst bug), but it is otherwise a rather complex issue and has remaining odd but minor bugs. This is going to be a bit complex to read but I'm mostly writing that for the future me here.

Basically you are changing the input buffer under the hood while the InputText widget is supposedly owning the editable text in its own copy of the state. This is isn't really something we should allow at all but so you should avoid it. However ImGui should still react reasonably to it.

Even with the new fix the remaining bug is: if you click on the scrollbar while the InputText is not active, but was last active, and the external text buffer and last internal buffer are mismatching, and this mismatch is causing a change of whether vertical scrollbar appear, then: clicking on the scrollbar with make the InputText starts rendering the last cached internal buffer which here is shorter, which is turns clear the scrollbar, which in turn loses your input.

It's not too bad because: you can click on the main text to switch back to the internal buffer, making the user issue easy to understand.

If you are going to replace the text buffer while text editing is in place, you'd need to deactivate the InputText widget by calling e.g. SetActiveId(0).
OR we could introduce a new flag to InputText to discard internal state and read back from input string. This is probably the saner approach to solve this.

(Otherwise, there is a proper fix but it's too hard to implement without hurting performances at the moment. It is yet another issue caused by the lack of native UTF-8 in stb_textedit.h and therefore this discussion raises the priority of implementing UTF-8 there and getting rid of all wchar in InputText which are causing a lot of pain. Somehow relates to #701 and so many things are hinging on adding this and simplifying the code)

In the future please provide repro in the form of code. This is a repro: ``` static char text[1024*16] = "/*\n" " The Pentium F00F bug, shorthand for F0 0F C7 C8,\n" " the hexadecimal encoding of one offending instruction,\n" " more formally, the invalid operand with locked CMPXCHG8B\n" " instruction bug, is a design flaw in the majority of\n" " Intel Pentium, Pentium MMX, and Pentium OverDrive\n" " processors (all in the P5 microarchitecture).\n" "*/\n\n" "label:\n" "\tlock cmpxchg8b eax\n"; static char text2[1024*16] = "/*\n" " The Pentium F00F bug, shorthand for F0 0F C7 C8,\n" " the hexadecimal encoding of one offending instruction,\n" " more formally, the invalid operand with locked CMPXCHG8B\n" " instruction bug, is a design flaw in the majority of\n" " Intel Pentium, Pentium MMX, and Pentium OverDrive\n" " processors (all in the P5 microarchitecture).\n" "*/\n\n" "label:\n" "\tlock cmpxchg8b eax\n" "/*\n" " The Pentium F00F bug, shorthand for F0 0F C7 C8,\n" " the hexadecimal encoding of one offending instruction,\n" " more formally, the invalid operand with locked CMPXCHG8B\n" " instruction bug, is a design flaw in the majority of\n" " Intel Pentium, Pentium MMX, and Pentium OverDrive\n" " processors (all in the P5 microarchitecture).\n" "*/\n\n" "label:\n" "\tlock cmpxchg8b eax\n"; ImGui::InputTextMultiline("Bug #725", text, 1024*32, ImVec2(-1.0f, ImGui::GetTextLineHeight() * 16)); if (ImGui::Button("Replace")) strcpy(text, text2); ``` The buffer size doesn't matter. I have now fixed 1 bug that blocked further input from happening (and it was by far the worst bug), but it is otherwise a rather complex issue and has remaining odd but minor bugs. This is going to be a bit complex to read but I'm mostly writing that for the future me here. Basically you are changing the input buffer under the hood while the InputText widget is supposedly owning the editable text in its own copy of the state. This is isn't really something we should allow at all but so you should avoid it. However ImGui should still react reasonably to it. Even with the new fix the remaining bug is: if you click on the scrollbar while the InputText is not active, but was last active, and the external text buffer and last internal buffer are mismatching, and this mismatch is causing a change of whether vertical scrollbar appear, then: clicking on the scrollbar with make the InputText starts rendering the last cached internal buffer which here is shorter, which is turns clear the scrollbar, which in turn loses your input. It's not too bad because: you can click on the main text to switch back to the internal buffer, making the user issue easy to understand. If you are going to replace the text buffer while text editing is in place, you'd need to deactivate the InputText widget by calling e.g. SetActiveId(0). OR we could introduce a new flag to InputText to discard internal state and read back from input string. This is probably the saner approach to solve this. (Otherwise, there is a proper fix but it's too hard to implement without hurting performances at the moment. It is yet another issue caused by the lack of native UTF-8 in stb_textedit.h and therefore this discussion raises the priority of implementing UTF-8 there and getting rid of all wchar in InputText which are causing a lot of pain. Somehow relates to #701 and so many things are hinging on adding this and simplifying the code)

ocornut commented

2016-07-14 11:34:37 +00:00

(Migrated from github.com)

If you are going to replace the text buffer while text editing is in place, you'd need to deactivate the InputText widget by calling e.g. SetActiveId(0).

I have also fixed a bug today where IsItemActive() wouldn't work on a multiline text editor, so you can now do if (IsItemActive()) SetActiveId(0); to apply the work-around / clearing of focus properly here when changing data under the hood.

Also note that the official way to change data while text edition is active is to use the callback flag and within the callback call DeleteChars()/InsertChars().

> If you are going to replace the text buffer while text editing is in place, you'd need to deactivate the InputText widget by calling e.g. SetActiveId(0). I have also fixed a bug today where IsItemActive() wouldn't work on a multiline text editor, so you can now do `if (IsItemActive()) SetActiveId(0);` to apply the work-around / clearing of focus properly here when changing data under the hood. Also note that the official way to change data while text edition is active is to use the callback flag and within the callback call DeleteChars()/InsertChars().

ocornut commented

2016-07-14 11:40:38 +00:00

(Migrated from github.com)

I am closing this as there's no immediate other actions to take.

I have fixed the biggest bug where input would stop functioning.
Fixed another issue to make it easier to explicitly lose focus.
Changing text under the hood is undefined behavior.
I have added two notes in the TODO list within imgui.cpp for eventually adding a Discard flag, and handling the scrollbar issue better when texts changes under the hood. Both will likely be implemented when getting it to wchar.

I am closing this as there's no immediate other actions to take. - I have fixed the biggest bug where input would stop functioning. - Fixed another issue to make it easier to explicitly lose focus. - Changing text under the hood is undefined behavior. - I have added two notes in the TODO list within imgui.cpp for eventually adding a Discard flag, and handling the scrollbar issue better when texts changes under the hood. Both will likely be implemented when getting it to wchar.