Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
dodo1004 opened this issue Jul 6, 2016 · 5 comments
Closed

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

dodo1004 opened this issue Jul 6, 2016 · 5 comments

Comments

@dodo1004
Copy link

dodo1004 commented Jul 6, 2016

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
Copy link
Author

dodo1004 commented Jul 7, 2016

hey ocornut, did you check it out?

@ocornut
Copy link
Owner

ocornut commented Jul 7, 2016

Not yet, sorry.

@ocornut ocornut added the bug label Jul 12, 2016
ocornut added a commit that referenced this issue Jul 12, 2016
… differs in a way where scrollbar existence differs. (#725)

Partial fix, won't stop ids from functioning because of a zombie id.
ocornut added a commit that referenced this issue Jul 12, 2016
@ocornut
Copy link
Owner

ocornut commented Jul 13, 2016

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
Copy link
Owner

ocornut commented Jul 14, 2016

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
Copy link
Owner

ocornut commented Jul 14, 2016

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants