ocornut / imgui

Dear ImGui: Bloat-free Graphical User interface for C++ with minimal dependencies
MIT License
59.43k stars 10.13k forks source link

Selectable in a table in a popup closes popups (cause flickering and moves the popup) #4936

Open wolframw opened 2 years ago

wolframw commented 2 years ago

Version/Branch of Dear ImGui:

Version: Dear ImGui 1.86 (18600) Branch: master

Back-end/Renderer/Compiler/OS

Back-ends: imgui_impl_win32.cpp + imgui_impl_dx12.cpp Compiler: MSVC++ 2019 (_MSC_VER=1929, _MSVC_LANG=201402) Operating System: Windows 10

My Issue/Question:

I'm creating a dialog to open or save files. The dialog is implemented as a modal popup with a table that contains selectable rows (the files) using Selectable. Whenever I select one of the files, the content of the application window flashes and the popup moves a few pixels to the right. Once it reaches the right border it starts moving down until it reaches the bottom border.

This problem does not occur in normal (non-popup) windows or with tables whose rows are not created using Selectable. I was able to reproduce this with the OpenGL and the DX12 backends, both on Windows 10 (I haven't tried any other backends yet).

Screenshots/Video

https://user-images.githubusercontent.com/5099115/150696413-eb571920-cc80-499f-b3cc-d5a6ccba689d.mp4 Each flicker corresponds to one click.

Standalone, minimal, complete and verifiable example: Open example_win32_directx12\main.cpp and replace all code between ImGui::NewFrame() and ImGui::Render() with the following:

static const char *items[] = { "Test 1", "Test 2", "Test 3", "Test 4" };
static bool showPopup = true;

ImVec2 workSize = ImGui::GetMainViewport()->WorkSize;
ImGui::SetNextWindowSize( ImVec2{ workSize.x / 1.5f, workSize.y / 1.5f }, ImGuiCond_Appearing );

ImGui::OpenPopup( "TablePopup" );
if ( ImGui::BeginPopupModal( "TablePopup", &showPopup, ImGuiWindowFlags_NoSavedSettings ) ) {
    if ( ImGui::BeginTable( "Table", 1, ImGuiTableFlags_NoSavedSettings, ImGui::GetContentRegionAvail() ) ) {
        ImGui::TableSetupColumn( "Item Name" );
        ImGui::TableHeadersRow();

        for ( auto const& item : items ) {
            ImGui::TableNextRow();
            ImGui::TableSetColumnIndex( 0 );
            ImGui::Selectable( item, false, ImGuiSelectableFlags_SpanAllColumns );
        }

        ImGui::EndTable();
    }

    ImGui::EndPopup();
}
ocornut commented 2 years ago

Thanks for your report. That's a really "fun" bug.

// Automatically close popups
if (pressed && (window->Flags & ImGuiWindowFlags_Popup) && !(flags & ImGuiSelectableFlags_DontClosePopups) && !(g.LastItemData.InFlags & ImGuiItemFlags_SelectableDontClosePopup))
    CloseCurrentPopup();

It happens because by default Selectable() are closing their parent popup. Passing ImGuiSelectableFlags_DontClosePopups to Selectable() is an immediate fix for it. So this behave a per current specs but I agree it is extremely puzzling and perhaps not desirable. For ImGuiSelectableFlags_DontClosePopups, linking to #1468, #1134, #751

This opens two questions:

ocornut commented 2 years ago

Why does the popup when reopened (immediately on the next frame by OpenPopup()) moves to the right?

The code that position newly reappearing popups in FindBestWindowPosForPopup() does:

    if (window->Flags & ImGuiWindowFlags_Popup)
    {
        ImRect r_avoid = ImRect(window->Pos.x - 1, window->Pos.y - 1, window->Pos.x + 1, window->Pos.y + 1);
        return FindBestWindowPosForPopupEx(window->Pos, window->Size, &window->AutoPosLastDirection, r_outer, r_avoid, ImGuiPopupPositionPolicy_Default);
    }

Which seems quite bizarre to me. I backtracked in history and found this was first added in d84b5737 very early in popup development. It might have had something to do with the assumption that popup would be opened from a right-mouse click and window was initially assigned that position before being moved. I suspect we should be able to remove this code, will investigate this further.

ocornut commented 2 years ago

What this intended to solve was to make mouse-position based popup open at a position that allows reopening them with the same or another mouse click. This is fairly standard behavior. I think it should be written differently. I pushed a fix b17b2fb

Keeping this open at the first question is still up in the air.

wolframw commented 2 years ago

It happens because by default Selectable() are closing their parent popup. Passing ImGuiSelectableFlags_DontClosePopups to Selectable() is an immediate fix for it. So this behave a per current specs but I agree it is extremely puzzling and perhaps not desirable. For ImGuiSelectableFlags_DontClosePopups, linking to #1468, #1134, #751

Thanks a lot, that works for me.

AACodek commented 2 years ago

I believe the behavior should be changed to not do anything by default. I've been using ImGui for a while off and on over the years. It was unexpected to me that a selection would close a popup. The whole point of me using this framework is that everything on this side of the UI is straight forward, and state is easily reasoned. Just by linearly walking the code.

IE: if (ImGui::Button("do action")) action();

or modal where the non imgui would have a callback for onclosed. this is handled via do_close

uint32_t current_selection;
/* user is viewing this in another window, nicely in the code above (not shown here) */

if (ImGui::BeginPopupModal()) {
    static structure data;
    if (data.IsSetup == false) { data.Setup(current_selection); }
   data.Filter.Draw(); 
   if (ImGui::BeginTable("Table", ...))
   {
      foreach(item) { // filtering
         bool selected = item->ID == current_selection;
         if (ImGui::Selectable("Jokes On You, Brah. do_close isn't going to happen", &selected)) {
            data.selected = item->ID;
         }
      }
      ImGui::EndTable();
   }

   if (ImGui::Button("Cancel") || ImGui::IsKeyPressed(ImGui::GetKeyIndex(ImGuiKey_Escape)) ) data.do_close = true;
   if (ImGui::Button("OK")) { data.dostuff(); data.do_close = true; }

   if (data.do_close) { data.cleanup(); ImGui::CloseCurrentPopup(); }
   ImGui::EndPopup();
} 

Now I've got a lot of buggy code, and no Idea when the Modal was really closed.

ocornut commented 1 month ago

FYI it is now possible to control this for Selectable, MenuItem() using the public API:

ImGui::PushItemFlag(ImGuiItemFlags_AutoClosePopups, false);
ImGui::MenuItem(....);
ImGui::PopItemFlags();

I will consider changing the default but you can technically now change the default globally with this (but it may raise bugs with shared/third-party code). Leaving this open.