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

Include period as delimiter & support Ctrl+Delete in InputText #6067

Closed
wants to merge 4 commits into from

Conversation

ajweeks
Copy link
Contributor

@ajweeks ajweeks commented Jan 9, 2023

This change includes period (.) as a delimiter so that pressing ctrl+arrow key or ctrl+backspace/delete in InputText fields doesn't skip past them.

Hitting ctrl+backspace with the cursor at the end of a buffer containing a command in the form cmd.shadows.enabled| will, after this change, result in cmd.shadows.| where previously the entire buffer would be cleared.

This change also introduces support for ctrl+delete, matching the behavior of ctrl+backspace but in the opposite direction. shift+delete still works to cut text with this change.

These are standard behaviors in all text editors, so this change makes InputText widgets better match what users' intuitions are.

In the following video you can see the effect of consecutively pressing ctrl+backspace, and then once again from the beginning with ctrl+delete:

2023_01_09_input_text.mp4

static int is_word_boundary_from_right(ImGuiInputTextState* obj, int idx)
{
if (obj->Flags & ImGuiInputTextFlags_Password) return 0;
if (is_separator(obj->TextW[idx])) return 1;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line was added so that ctrl+backspace only deletes one character when the cursor is in front of a separator, which matches the behavior of nearly all text editors. Without this line, the buffer cmd.| will be fully cleared on ctrl+backspace, where with this line only the period will be deleted.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@restore deleted

if (is_wordmove_key_down)
state->OnKeyPressed(STB_TEXTEDIT_K_WORDRIGHT | STB_TEXTEDIT_K_SHIFT);
else if (is_osx && io.KeySuper && !io.KeyAlt && !io.KeyCtrl)
state->OnKeyPressed(STB_TEXTEDIT_K_LINEEND | STB_TEXTEDIT_K_SHIFT);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little bit unsure about this line, and can't test as I don't have a Mac, though I think it's logical to pass LINEEND, where the equivalent code for ctrl+backspace uses LINESTART (see L4348)

@ocornut
Copy link
Owner

ocornut commented Jan 10, 2023

Hello,
Thanks for the PR.
Could you please massage the commit history so that the two different features are in separate commits? (and force-push)
I can imagine easily merging the CTRL+Delete change but other will need more review.

@ajweeks
Copy link
Contributor Author

ajweeks commented Jan 12, 2023

I've broke the changes into two cleaner commits now.

@ocornut
Copy link
Owner

ocornut commented Jan 25, 2023

Thank you!

I'm a little bit unsure about this line, and can't test as I don't have a Mac, though I think it's logical to pass LINEEND, where the equivalent code for ctrl+backspace uses LINESTART (see L4348)

I've tested this on a Mac with a Delete key and surprisingly even though Super+Backspace is supported, Super+Delete does nothing, so I haven't added that part of the code (and added corresponding comment).

Merged that commit as f4ef420

ocornut added a commit to ocornut/imgui_test_engine that referenced this pull request Jan 25, 2023
…y tests for now)

Intent to test further changes such as ocornut/imgui#6067
@ocornut
Copy link
Owner

ocornut commented Jan 25, 2023

As for "include period delimiter", I believe this break other common behavior.

I started gathering behavior for Visual Studio (windows), Firefox (windows), Xcode (mac), Firefox (mac)

Behaviors

        /*
        // Action   VS(Win)                     Firefox(Win)        XCode(OSX)                  Firefox(OSX)
        //------------------------------------------------------------------------------------------------------
        //         |Hello                      |Hello              |Hello                       ==
        // Ctrl->   Hello |world                ==                  Hello|                      ==
        // Ctrl->   Hello world|.               (skip)              Hello world|.               ==
        // Ctrl->   Hello world. |Foo           ==                  (skip)                      (skip)
        // Ctrl->   Hello world. Foo|.bar       (skip)              Hello world. Foo|.bar       (skip)
        // Ctrl->   Hello world. Foo.|bar       ==                  (skip)                      (skip)
        // Ctrl->   Hello world. Foo.bar|       (skip)              Hello world. Foo.bar|       ==
        // Ctrl->   Hello world. Foo.bar!!!|    ==                  (next line word)            (eol)
        //
        //          Hello world. Foo.bar!!!|    ==                  Hello world. Foo.bar!!!|    ==
        // Ctrl<-   Hello world. Foo.bar|!!!    (skip)              (skip)                      (skip)
        // Ctrl<-   Hello world. Foo.|bar!!!    ==                  Hello world. Foo.|bar!!!    (skip)
        // Ctrl<-   Hello world. Foo|.bar!!!    (skip)              (skip)                      (skip)
        // Ctrl<-   Hello world. |Foo.bar!!!    ==                  Hello world. |Foo.bar!!!    ==
        // Ctrl<-   Hello world|. Foo.bar!!!    (skip)              (skip)                      (skip)
        // Ctrl<-   Hello |world. Foo.bar!!!    ==                  Hello |world. Foo.bar!!!    ==
        // Ctrl<-   |Hello world. Foo.bar!!!    ==                  Hello world. Foo.bar!!!     ==
        //
        // (*) Firefox: tested GitHub, Google, WhatsApp text fields with same results.
        */

Which opens the question of which behavior we should follow : coding env or web env?
I'd tend for coding env as a reference.

I also started working on a Regression Test for this:
ocornut/imgui_test_engine@890bab1

In the Test (Windows only for now) the blocks under #if IMGUI_BROKEN_TESTS are those where our behavior doesn't match Visual Studio in current version. Your patch fixes some of tests but breaks others.

At a first step we should get the Windows behavior to pass all Windows tests, then we can work on adding Mac tests and fix that too. The system allows for simulating Mac behavior if you set our config flags.

Would you like to work on finishing this and make it pass all tests? (replace #if IMGUI_BROKEN_TESTS with #if 1 while working locally, then by #if IMGUI_VERSION_NUM > xxxx once a fix is pushed).

@ajweeks
Copy link
Contributor Author

ajweeks commented Jan 28, 2023

Great, I'm on it :) I agree that we should aim to support the behaviour seen in most coding environments.

Requires imgui_test_engine/pull/9 to pass tests
@ajweeks
Copy link
Contributor Author

ajweeks commented Jan 29, 2023

I've got the current tests passing on both Windows and Mac (simulated), only once I made some changes to imgui_test_engine though (imgui_test_engine/pull/9)

I think it would be good however to have more tests for Mac (as your comments point out), and also some tests of the super key? But I'm fairly confident the Windows behaviour is now matching exactly what I would expect from VS.

I also have to say, the test engine repro is pretty amazing :) It looks like an amazingly-handy tool and it seems to be really well designed! Nice work!

ocornut pushed a commit that referenced this pull request Mar 22, 2023
@ocornut
Copy link
Owner

ocornut commented Mar 22, 2023

Merged as 821814b
I went ahead and didn't add more tests for OSX, will revisit when needed.

@ocornut ocornut closed this Mar 22, 2023
@ajweeks ajweeks deleted the patch-1 branch March 26, 2023 14:39
kjblanchard pushed a commit to kjblanchard/imgui that referenced this pull request May 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants