ocornut / imgui_test_engine

Dear ImGui Automation Engine & Test Suite
386 stars 40 forks source link

ImGuiTestOpFlags_NoFocusWindow flag ignored in function WindowBringToFront #24

Closed aandrejevas closed 1 year ago

aandrejevas commented 1 year ago

image Hello, I have this test and by running it I got suspicious that maybe the function WindowBringToFront is not working correctly.

The test works like this, at some point the engine clicks the button named "Launch", the window called "QAP" is focused. Because "Launch" has been clicked, another window opens called "Overlay", it is created with flag ImGuiWindowFlags_NoFocusOnAppearing so it is hidden behind window "QAP". In the test then I want to bring it to front without focusing it so I am calling the function WindowBringToFront with flag ImGuiTestOpFlags_NoFocusWindow. But the function focuses the "Overlay" window anyway.

image I checked the implementation of the WindowBringToFront function. I saw that it does not check if the flag ImGuiTestOpFlags_NoFocusWindow is set. So as it can be seen in the picture I added the check and then the test worked as expected.

ocornut commented 1 year ago

Hello, I think we can apply the change you are suggesting.

However I am really curious about your intent here.

In fact, while the first "NOT_SAME_AS_END_USER" FIXME in the function (pictured in your shot) n ought to suggest that this path could be replaced with a click on title bar (should be a possible but not big benefit), the second FIXME is actually because that BringWindowToDisplayFront() doesn't have a end-user equivalent. (I'm amending the comments now)

aandrejevas commented 1 year ago

The window called "Overlay" closes when it gains focus. So I am trying to avoid giving it focus through indirect means in tests, only when it is focused with a mouse click it should close. But I can not test the mouse click if the window "Overlay" is behind the window "QAP" so I am trying to bring it to the front without giving it focus. So that is my intent.

aandrejevas commented 1 year ago

Yes, this is complicated because ImGui::BringWindowToDisplayFront() is not an actually possible action for end-user. But in my case without the test suite, this problem does not exist. And that is because the window "QAP" is created with flag ImGuiWindowFlags_NoBringToFrontOnFocus so the user can't hide the window "Overlay", but the test engine indeed does hide it.

ocornut commented 1 year ago

I added the fix you mentioned but I'd keep to keep this open.

only when it is focused with a mouse click it should close.

This is a bit unusual, but I guess in theory in your app it may be more correct to test for something like IsWindowHovered() && IsMouseClicked() ? I guess this is a short-lived window (similar to a tooltip or popup) so you don't care about other ways of focusing such as Ctrl+Tabbing?

But in my case without the test suite, this problem does not exist. And that is because the window "QAP" is created with flag ImGuiWindowFlags_NoBringToFrontOnFocus so the user can't hide the window "Overlay", but the test engine indeed does hide it.

I would like to challenge this for the benefit of improving the test engine. Can you details/reformulate this paragraph? I am not sure I understand it.

ocornut commented 1 year ago

It would be nice if you can explain this further and provide a standalone test (with a minimal GuiFunc + TestFunc).

I am not sure how you expect "Overlay" to appear in front in your real app, since it is using ImGuiWindowFlags_NoFocusOnAppearing.

(May be related, but I also do believe out of the 7 calls to WindowBringToFront() in ImGuiTestContext most ought to be replaced with WindowFocus(). We occasionally use it for visibility but some of the uses are not required. If WindowBringToFront() is called while window is not focused it will bring to front. I am going to investigate if I can change most)

aandrejevas commented 1 year ago

https://github.com/ocornut/imgui_test_engine/issues/24#issuecomment-1520716715 Yes, the window "Overlay" is short lived, it closes after some seconds by itself. I also create it with the flag ImGuiWindowFlags_NoNav. IsWindowHovered() && IsMouseClicked() is maybe an even better solution because I would not have to use the flag ImGuiWindowFlags_NoFocusOnAppearing and so on. I would have to test it.

https://github.com/ocornut/imgui_test_engine/issues/24#issuecomment-1520730304 Yes, now that I think about it, the situation is weird but "Overlay" appears in front of my real app. Maybe because the real app is using ImGuiWindowFlags_NoBringToFrontOnFocus and first I draw the "Overlay", then the main app.

I will try to provide a standalone test soon.

ocornut commented 1 year ago

I pushed several commits now trying to reduce call to WindowBringToFront() in favor of WindowFocus(). It might make a difference.

Otherwise I would suggest:

Thanks!

aandrejevas commented 1 year ago

main_temp.txt Here, I created this minimal app to test this situation.

With the newest commits everything works.

ocornut commented 1 year ago

Thank you for putting work into this.

Btw the way, when I said "with a minimal GuiFunc + TestFunc" it is becayse you can share this in a simpler "copiable anywhere" way:

t = IM_REGISTER_TEST(e, "issues", "issues_0024");
t->GuiFunc = [](ImGuiTestContext* ctx)
{
    // Drawing
    static bool show_app_overlay = false;
    static double end_overlay;

    const ImGuiViewport* const viewport = ImGui::GetMainViewport();

    // Drawing
    if (show_app_overlay) {
        ImGui::SetNextWindowBgAlpha(0.35f);
        ImGui::SetNextWindowPos({ viewport->WorkPos.x + viewport->WorkSize.x - 10.f, viewport->WorkPos.y + 30.f }, ImGuiCond_Always, { 1.f, 0.f });
        if (ImGui::Begin("Overlay", &show_app_overlay, ImGuiWindowFlags_NoDecoration | ImGuiWindowFlags_NoMove | ImGuiWindowFlags_AlwaysAutoResize | ImGuiWindowFlags_NoSavedSettings | ImGuiWindowFlags_NoFocusOnAppearing | ImGuiWindowFlags_NoNav)) {

            const double current_time = ImGui::GetTime();
            if (ImGui::IsWindowFocused() || end_overlay < current_time) {
                show_app_overlay = false;
            }
            ImGui::TextColored(ImVec4{ 1.f, 0.f, 0.f, 1.f }, "%f", end_overlay - current_time);
        } ImGui::End();
    }

    ImGui::SetNextWindowPos(viewport->Pos);
    ImGui::SetNextWindowSize(viewport->Size);
    if (ImGui::Begin("QAP", nullptr, ImGuiWindowFlags_NoTitleBar | ImGuiWindowFlags_NoResize | ImGuiWindowFlags_NoCollapse | ImGuiWindowFlags_NoMove | ImGuiWindowFlags_NoBringToFrontOnFocus)) {

        if (ImGui::Button("Launch")) {
            show_app_overlay = true;
            end_overlay = ImGui::GetTime() + 5.;
        }
    }
    ImGui::End();
};
t->TestFunc = [](ImGuiTestContext* ctx)
{
    ctx->ItemClick("//QAP/Launch");
    ctx->MouseMove("//Overlay");
    ctx->MouseClick();
};

I will investigate this as a test case: we shouldn't need to focus "Overlay" here.

ocornut commented 1 year ago

I pushed 530329d reducing the amount of unnecessary focus. Please note that if the target location isn't visible, it would still perform a focus by default.