Added ImGuiInputTextFlags_TabReturnsTrue for commit and navigate behavior for Input widgets

[zzzyap]

Usage Context:

• Some Input*() widgets have values that require commit value behavior instead of on changed input behavior. So when editing multiple Input*() widgets, as an option it is desirable to press tab for committing and navigating.

Notes:

• Behavior desired and implementation is similar to ImGuiInputTextFlags_EnterReturnsTrue where enter commits the value.
• Potential conflict of behavior with other tab behaviors.

[ocornut]

as an option it is desirable to press tab for committing and navigating.

Is your real use case that you are setting both ImGuiInputTextFlags_EnterReturnsTrue and ImGuiInputTextFlags_TabReturnsTrue together?

Have you checked the following functions: bool IsItemDeactivated(); and bool IsItemDeactivatedAfterEdit() ? They should provide you with the necessary signal.

What do you want to happen if the user CLICKS on another widget with a value has been typed in but no validated?

[zzzyap]

Yes, ImGuiInputTextFlags_TabReturnsTrue is usually set with ImGuiInputTextFlags_EnterReturnsTrue.
Such that:

• [Enter] commits and exits the input widget
• [Tab] commits and navigates to next input widget

If the user clicks out of the input box, the value that was there is discarded and reverted to previous.

In general, the "commit" behavior is desired over "on changed" so the value is applied once.

It is understandable that some widgets want to "discard and revert" on tab instead.

Potential conflict of behavior with other tab behaviors.

But in our usage, tab to commit is nice for editing lots of fields.

[ocornut]

In general, the "commit" behavior is desired over "on changed" so the value is applied once.

I think this is what IsItemDeactivatedAfterEdit() should provide, hence my question above?
If the behavior of this isn't clear, you can see how it behaves in Demo>Widgets>Querying Status.

[zzzyap]

Sorry missed those questions.
Haven't tried out bool IsItemDeactivated(); and bool IsItemDeactivatedAfterEdit() yet.
I would also assume that the value should be validated on leaving the widget. If it fails it discards the value and retains the previous value.
Will try it out soon.

EDIT: thinking about it from the user perspective, if the user clicks off the widget whilst editing, it should not be committed as neither enter or tab "commited" the value. I believe in this case discard and retain the old value would make sense.

[zzzyap]

Please correct me if i'm missing something.
bool IsItemDeactivatedAfterEdit() does provide the signal, but this doesn't really require it.

EDIT: Sample to demonstrate desired behavior

static float number[10];
for(int i = 0; i < 10; ++i)
{
    ImGui::PushID(i);
    ImGui::Text("Value = %f", number[i]); ImGui::SameLine();
    ImGui::InputFloat("", &number[i], 0, 0, "%.4f", ImGuiInputTextFlags_EnterReturnsTrue | 
ImGuiInputTextFlags_TabReturnsTrue);
    ImGui::PopID();
}

Which produces this behavior with the following input 123 <TAB> 456 <TAB> 789 <CLICK> 123 <ENTER>

EDIT:
I noticed that my initial comment with the pull request could be confusing, some clarifications:

• When editing multiple Input*() widgets it is desirable to press tab for commiting and navigating.
• When tab to commit and navigate is set, "on changed" input behavior should be disabled as "on commit" is desired.
• When clicking off these widgets the value is considered not commited and will be discarded.

[ocornut]

I am not sure understand this last post or the sample. You are not testing the return value of InputText() so passing ImGuiInputTextFlags_TabReturnsTrue is not useful.

IsItemDeactivatedAfterEdit() should be a general mechanism that can work on every widget regardless of the key involved.

[zzzyap]

Sorry the above post is to demonstrate the desired behavior.
I must have misunderstood as this change is primarily about having tab return true so that the result can be handled.
Regarding IsItemDeactivatedAfterEdit() it is definitely something the user can add to handle the optional discard value (on click), which is what I mean by this doesn't require it.

Sample for Text,

static char value_buffer[10][256];
static char buffer[10][256];
for(int i = 0; i < 10; ++i)
{
    ImGui::PushID(i);
    ImGui::Text("Value = %s", value_buffer[i]); 
    ImGui::SameLine();
    bool result = ImGui::InputText("", buffer[i], 256, ImGuiInputTextFlags_EnterReturnsTrue | ImGuiInputTextFlags_TabReturnsTrue);
    bool deactivated = ImGui::IsItemDeactivated();
    bool deactivatedAfterEdit = ImGui::IsItemDeactivatedAfterEdit();
    if(result) // handle commit
    {
        memcpy(value_buffer[i], buffer[i], 256);
    }
    if(deactivated || deactivatedAfterEdit) // (optional) handle discard
    {
        memcpy(buffer[i], value_buffer[i], 256);
    }
    ImGui::SameLine();
    ImGui::Text("%i, %i, %i", result, deactivated, deactivatedAfterEdit); 
    ImGui::PopID();
}

ABC <TAB> ABC <CLICK> ABC <ENTER>

^ screen capture couldn't catch the flags but it is as follows

1, 0, 0
0, 1, 1
1, 1, 1

[ocornut]

I must have misunderstood as this change is primarily about having tab return true so that the result can be handled

Yes. What I mean is that there should be a generic way to handle that and it should be IsItemDeactivatedAfterEdit(). There are various requests to make Tab pass through all widgets so I'm not looking at a solution specific to InputText(), hence the tangent I am taking now.

1. It looks like a bug to me that IsItemDeactivatedAfterEdit() doesn't trigger on tabbing out. I'm am inspecting it.

2. Based on the // (optional) handle discard comment above it looks like you misunderstood the intent of IsItemDeactivatedAfterEdit() as well. It should return true on the frame a widget is deactivated, if during its activation it has modified the underlying value. Moroever you shouldn't need to handle "discard" as it should be handled by pressing ESC, but I understand you may be expecting another behavior.

Based on your example you should only need to do:

    bool result = ImGui::InputText("", buffer[i], 256);
    bool deactivatedAfterEdit = ImGui::IsItemDeactivatedAfterEdit();
    if (result || deactivatedAfterEdit) // handle commit
        memcpy(value_buffer[i], buffer[i], 256);

Or maybe even:

    if (ImGui::InputText("", buffer[i], 256) && ImGui::IsItemDeactivatedAfterEdit())
        memcpy(value_buffer[i], buffer[i], 256);

EDIT Let me dig into this a little more.. because maybe there is a bug with IsItemDeactivatedAfterEdit and tabbing.

[ocornut]

I confirm there is a bug with Tab handling of InputText() and the IsDeactivatedXXX flags.
In your use case notice that if you use Shift+Tab the return values are correct. It also seems to be correct for the last element of the window (when Tab wraps back to the down).

[ocornut]

Regression Test (If you are curious... )

Don't mind the syntax too much, I am still experimenting with the automation framework and each new test bring their own challenge in term of keeping this elegant.

// Test the IsItemDeactivatedXXX flags(), bug mentioned in #2215
t = REGISTER_TEST("widgets", "widgets_inputtext_5_deactivate_flags");
t->GuiFunc = [](ImGuiTestContext* ctx)
{
    ImGuiTestGenericState& gs = ctx->GenericState;
    ImGui::Begin("Test Window", NULL, ImGuiWindowFlags_NoSavedSettings | ImGuiWindowFlags_AlwaysAutoResize);
    bool& b_ret = gs.BoolArray[0];
    bool& b_activated = gs.BoolArray[1];
    bool& b_deactivated = gs.BoolArray[2];
    bool& b_deactivated_after_edit = gs.BoolArray[3];
    b_ret |= ImGui::InputText("Field", gs.Str1, IM_ARRAYSIZE(gs.Str1));
    b_activated |= ImGui::IsItemActivated();
    b_deactivated |= ImGui::IsItemDeactivated();
    b_deactivated_after_edit |= ImGui::IsItemDeactivatedAfterEdit();
    ImGui::Text("Ret: %d", b_ret);
    ImGui::Text("IsItemActivated: %d", b_activated);
    ImGui::Text("IsItemDeactivated: %d", b_deactivated);
    ImGui::Text("IsItemDeactivatedAfterEdit: %d", b_deactivated_after_edit);
    ImGui::InputText("Dummy Sibling", gs.Str2, IM_ARRAYSIZE(gs.Str2));
    ImGui::End();
};
t->TestFunc = [](ImGuiTestContext* ctx)
{
    // Accumulate return values over several frames/action into each bool
    ImGuiTestGenericState& gs = ctx->GenericState;
    bool& b_ret = gs.BoolArray[0];
    bool& b_activated = gs.BoolArray[1];
    bool& b_deactivated = gs.BoolArray[2];
    bool& b_deactivated_after_edit = gs.BoolArray[3];

    // Testing activation flag being set
    ctx->SetRef("Test Window");
    ctx->ItemClick("Field");
    IM_CHECK(!b_ret && b_activated && !b_deactivated && !b_deactivated_after_edit);
    gs.clear();

    // Testing deactivated flag being set when canceling with Escape
    ctx->KeyPressMap(ImGuiKey_Escape);
    IM_CHECK(!b_ret && !b_activated && b_deactivated && !b_deactivated_after_edit);
    gs.clear();

    // Testing validation with Return after editing
    ctx->ItemClick("Field");
    memset(gs.BoolArray, 0, sizeof(gs.BoolArray));
    ctx->KeyCharsAppend("Hello");
    IM_CHECK(b_ret && !b_activated && !b_deactivated && !b_deactivated_after_edit);
    gs.clear();
    ctx->KeyPressMap(ImGuiKey_Enter);
    IM_CHECK(b_ret && !b_activated && b_deactivated && b_deactivated_after_edit);
    gs.clear();

    // Testing validation with Tab after editing
    ctx->ItemClick("Field");
    ctx->KeyCharsAppend(" World");
    gs.clear();
    ctx->KeyPressMap(ImGuiKey_Tab);
    IM_CHECK(b_ret && !b_activated && b_deactivated && b_deactivated_after_edit);
    gs.clear();
};

[ocornut]

Hello @zzzyap,

And sorry again to be hijacking the PR for what initially was a request to add the TabReturnsTrue flag, but we're heading toward a solution.

I have now pushed the fixes that made IsItemDeactivated() and IsItemDeactivatedAfterEdit() not reliable on the item being tabbed out.

In fact, prior to the fix if you were to run the app at very low framerate (e.g. 3 FPS, which I frequently do in order to catch flickering and shearing issues) you would notice that when tabbing forward with the Tab keys, both previous and current fields would draw as active for the frame. This is now fixed as well.

With those fixes in, my understanding is that you should not need the ImGuiInputTextFlags_TabReturnsTrue flag, as the problem can be solved in a more generic manner on all sorts of widgets.

When you have time, let me know if that's the case or if you feel there is still a use for ImGuiInputTextFlags_TabReturnsTrue.

[zzzyap]

Tried out the changes, I do prefer this solution.
The generic-ness allows for better user customized behavior.

As for nav (tab) commit, the sample below would now suffice without any modifications to the lib :)(excuse the code, its for testing purposes)

const int key_index = GImGui->IO.KeyMap[ImGuiKey_Tab];
const bool isTabbing = ImGui::IsKeyDown(key_index);
static char value_buffer[10][256];
static char buffer[10][256];
for(int i = 0; i < 10; ++i)
{
    ImGui::PushID(i);
    ImGui::Text("Value = %s", value_buffer[i]); 
    ImGui::SameLine();
    bool result = ImGui::InputText("", buffer[i], 256, ImGuiInputTextFlags_EnterReturnsTrue);
    bool deactivatedAfterEdit = ImGui::IsItemDeactivatedAfterEdit();
    if(result || (deactivatedAfterEdit && isTabbing)) // handle commit & (optional) handle nav commit
    {
        memcpy(value_buffer[i], buffer[i], 256);
    }
    else if(deactivatedAfterEdit) // (optional) handle discard
    {
        memcpy(buffer[i], value_buffer[i], 256);
    }
    ImGui::PopID();
}

Which produces our desired behavior.
123 <TAB> abc <CLICK> def <ENTER>

Closing this pr.
Thanks a bunch!

[ocornut]

Good!

Which user action do you expect to trigger the "discard" path?
Clicking outside or on another widget?
Doesn't the handling of ESC suffice to cover this action?

[zzzyap]

Mouse click, since that triggers the deactivated after edit.

[ocornut]

Mouse click, since that triggers the deactivated after edit.

I assume you are trying to maintain some habit formed by legacy code?
Because AFAIK it is very non-standard to undo a modification on a click.
Most OS and Web apps don't do this.

[zzzyap]

Yup its non-standard, but that's something for me to convince them otherwise.
In general the text boxes are used as the display as well, so if the user doesn't commit it should display what was previously there.

[ocornut]

Various refactor done in 2018 and the recent one should make #701 a little easier to do now (even if ideally we should completely remove the U16 buffer).

[zzzyap]

Related to this, I'm not too sure if this is a bug or not.

After reverting this pr from our local fork (which is based on this commit b1af4d3) and testing it with other input widgets, I noticed the following.
After the call IsItemDeactivatedAfterEdit() (which is signaled correctly) the value is discarded.

Example: calling the following ImGui::InputFloat("", &temp); with some debug output for InputTextState
Note: TempBuffer is temp as a string after "commit".

(Expected) Value set on enter.
If ImGui::IsItemDeactivatedAfterEdit() && ImGui::IsKeyDown(GImGui->IO.KeyMap[ImGuiKey_Enter])
typing 123 <ENTER>

(Not expected) Value discarded on tab.
If ImGui::IsItemDeactivatedAfterEdit() && ImGui::IsKeyDown(GImGui->IO.KeyMap[ImGuiKey_Tab])
typing 234 <TAB>

[ocornut]

@zzzyap The post above is a little confusing, could you narrow down a repro?
For a start

If ImGui::IsItemDeactivatedAfterEdit() && ImGui::IsKeyDown(GImGui->IO.KeyMap[ImGuiKey_Tab])

is likely wrong, Tab is processed on the frame after the press (otherwise we can't handle Shift+Tab).

[zzzyap]

const ubiBool defaultCommit = ImGui::InputText(label, buf, buf_size, flags | ImGuiInputTextFlags_EnterReturnsTrue, callback, user_data);
// OR
const ubiBool defaultCommit = ImGui::InputFloat(label, value, step, step_fast, format, flags | ImGuiInputTextFlags_EnterReturnsTrue);

followed by

    const ubiBool deactivatedAfterEdit = ImGui::IsItemDeactivatedAfterEdit();
    const ubiBool tabNavCommit = deactivatedAfterEdit && ImGui::IsKeyDown(ImGui::GetIO().KeyMap[ImGuiKey_Tab]);
    if(defaultCommit)
    {
        // correct for both widgets
    }
    else if(tabNavCommit)
    {
        // correct for text widget, `value` for float widget here is discarded. 
        // workaround ImGui::GetCurrentContext()->InputTextState.TextA.Data has the float as text here.
    }
    else if(deactivatedAfterEdit)
    {
        // discard
    }

[ocornut]

I think your problem is that ImGuiInputTextFlags_EnterReturnsTrue is not handled properly by InputFloat(), which does:

if (InputText("", buf, IM_ARRAYSIZE(buf), flags))
   value_changed = DataTypeApplyOpFromText(buf, g.InputTextState.InitialTextA.Data, data_type, data_ptr, format);

Please confirm this is the root of your issue and we can open another issue for it.

Testing IsKeyDown(..... Tab) also is incorrect as the key may be released already, if your application runs at low framerate you may miss it.

[zzzyap]

Yes.
When testing I did hit that block of code.
I guess the issue is that, for InputText the user can handle this custom behavior, however for things that route to InputText the user cannot add custom behavior.
Looking back the original pr, the flag ImGuiInputTextFlags_TabReturnsTrue would be passed thru to InputText which is called by InputFloat, thus this never showed up.

As for the tab (testing in the example sln), I believe it is correctly signaled, it's within the frame that it is set from the wndproc. (using win32)

[ocornut]

I guess the issue is that, for InputText the user can handle this custom behavior, however for things that route to InputText the user cannot add custom behavior

Some of the InputTextFlags_ values are valid for InputFloat,InputScalar, etc. But we need to rework its handling so it supports ImGuiInputTextFlags_EnterReturnsTrue properly. Currently it doesn't. It's not a big amount of work but needs to be carefully done in InputScalar.

As for the tab (testing in the example sln), I believe it is correctly signaled, it's within the frame that it is set from the wndproc. (using win32)

My bad. You are right, the deactivation itself is process within the frame. However the activation of the next/prev widget may happen on the next frame.