ocornut / imgui

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

BeginGroup causing popups to trigger BeginDragDropSource #2840

Open Unit2Ed opened 5 years ago

Unit2Ed commented 5 years ago

(you may also go to Demo>About Window, and click "Config/Build Information" to obtain a bunch of detailed information that you can paste here)

Version/Branch of Dear ImGui:

Version: 1.72 WIP Branch: docking

Back-end/Renderer/Compiler/OS

Back-ends: imgui_impl_win32.cpp + imgui_impl_dx11.cpp Compiler: MSVC 1916 Operating System: Windows 10

My Issue/Question:

We have a combo widget that's a drag drop source. When the widget is surrounded with BeginGroup/EndGroup, dragging on the scrollbar of the combo's popup (or any other widget within the popup) will incorrectly cause BeginDragDropSource to return true. Without BeginGroup/EndGroup, it doesn't start a drag.

Screenshots/Video

First combo shows expected behaviour, second shows it broken - notice the little square of the colour drag next to the cursor BeginGroupComboDrag

// Dragging the scroll bar on this combo will NOT start a drag-drop
if (ImGui::BeginCombo("Fine", "Preview")) 
{
    for (int i = 0; i < 10; ++i)
    {
        ImGui::PushID(i);
        ImGui::Selectable("Selectable");
        ImGui::PopID();
    }

    ImGui::EndCombo();
}

if (ImGui::BeginDragDropSource())
{
    ImVec4 col_rgb;
    ImGui::SetDragDropPayload(IMGUI_PAYLOAD_TYPE_COLOR_4F, &col_rgb, sizeof(float) * 4, ImGuiCond_Once);
    ImGui::EndDragDropSource();
}

// Dragging the scroll bar on this combo WILL start a drag-drop
ImGui::BeginGroup();
if (ImGui::BeginCombo("Broken", "Preview")) 
{
    for (int i = 0; i < 10; ++i)
    {
        ImGui::PushID(i);
        ImGui::Selectable("Selectable");
        ImGui::PopID();
    }

    ImGui::EndCombo();
}
ImGui::EndGroup();

if (ImGui::BeginDragDropSource(ImGuiDragDropFlags_SourceAllowNullID))
{
    ImVec4 col_rgb;
    ImGui::SetDragDropPayload(IMGUI_PAYLOAD_TYPE_COLOR_4F, &col_rgb, sizeof(float) * 4, ImGuiCond_Once);
    ImGui::EndDragDropSource();
}
ocornut commented 5 years ago

Hello Edward,

I can look into it but it also seems like an odd UX decision to make the whole thing a drag source. Have you considered using a small button (eg using an icon as label) next to the combo box?

Omar

Unit2Ed commented 5 years ago

Hi Omar, I was trying to be concise in the report, so the example is much simplified from the actual situation we're seeing it in, which is a fairly typical property editor (not unlike you'd find in Unreal, Unity or Visual Studio).

Adding a drag-source button/icon is something we've considered and we may still do, though there are screen-space concerns and aesthetic reasons why we don't want to do it if possible. It's also not suitable for all situations where we have a draggable element that may be in a group and could cause a popup can be generated. For example grid views that have a context popup, which can contain menu items (which unexpectedly become drag sources), or other elements on which you would naturally perform drag-like actions (eg number/text inputs).

We could find a way to not use groups to accomplish this, but they're obviously a very useful solution for bundling a few widgets together and treating them as one.

ocornut commented 5 years ago

Hello,

To clarify, in your Combo box, where do you expect to be dragging from? Mouse dragging after clicking on the closed combo box, or mouse dragging from individual elements?

It's also not suitable for all situations where we have a draggable element that may be in a group and could cause a popup can be generated. For example grid views that have a context popup, which can contain menu items (which unexpectedly become drag sources), or other elements on which you would naturally perform drag-like actions (eg number/text inputs).

If you have time to submit a little more details (e.g. gif/screens, no need for working repro) to understand the fuller context more in details it would generally help. Not absolutely necessary but it tends to facilitate designing better solutions.

At the moment the code in EndGroup() decide if the group is active based on wether the active item was submitted between BeginGroup() and EndGroup(). calls It's a little bit of a wonky magic trick we are performing here.

In EndGroup() we could decide to check if the active id was submitted within the same window: In the line of EndGroup() that does

const bool group_contains_curr_active_id = (group_data.BackupActiveIdIsAlive != g.ActiveId) && (g.ActiveIdIsAlive == g.ActiveId) && g.ActiveId;

By adding

&& g.ActiveIdWindow == window

But this is also why I asked my first question above. If you added this filter you would be able to drag from the closed combo box, but not from individual selectable elements. If you want to drag from the individual selectable elements then it feels like it makes much more sense to make each of those Selectable a drag source rather than the combo box.

Or, we could add more semantic/data along with g.ActiveId (e.g. g.ActiveIdWidgetTag, I don't know).

In either case we could have to clarify what groups are, perhaps add a bunch of flags to them.