Drag and Drop helper "reorder drop target" api

[rokups]

This PR implements a convenience API for reordering of items we discussed in #143. In order to achieve good visuals i added ImGuiDragDropFlags_AcceptNoBorderPadding flag which disables expansion of rectangle marking drop target. I took a liberty to add a style variable as well. In my own project i simply commended out hardcoded padding as overall it looked better. I used ItemSpacing for constructing rectangle for reorders, however at times it can be quite hard to place items on. Not sure what to do about it.
https://user-images.githubusercontent.com/19151258/66028917-ca69b300-e506-11e9-984e-432cead8d197.gif

I reworked reorder API and it no longer reflects initial description. New API:

    // Reordering
    // - Call after each item that can be reordered.
    // - Returns -1/+1 if item should be swapped with the previous/next one, or 0 for no action.
    // - Only items within current or specified id scope participate in item reorder.
    // FIXME: "bool vertical" should be replaced with "ImGuiAxis axis", but ImGuiAxis is in imgui_internal.h
    IMGUI_API int           ItemReorder(ImGuiID id, bool vertical);
    IMGUI_API int           ItemReorder(const char* id, bool vertical);
    IMGUI_API int           ItemReorder(bool vertical);

And usage (error checking is ignored for brevity):

for (int i = 0; i < 10; i++)
{
    ImGui::Button(items[i]);
    if (int d = ImGui::ItemReorder(true))
        ImSwap(items[i], items[i + d]);
}

New API closely mimics behavior of "Drag to reorder items (simple)" in the demo, however it also solves several important issues:

• Gaps between reorderable items can exist and they do not create a reorder feedback loop.
• Reorder only happens when reorderable item is dragged over another reorderable item.
• Multiple distinct reorderable lists may exist side by side and they do not interfere with each other (nothing happens if reorderable item from list A is dragged on to reorderable item in list B).

From the point of view of UI user new API is much more intuitive and comfortable to use. We no longer must precisely aim at a spot between two items that is couple pixels high. Instead reorder happens as soon as mouse cursor reaches another reorderable item.

Complicated reordering as displayed in this gif is no longer possible, and that is a good thing. A sensible approach to achieve both reordering and reparenting would be mixing DragAndDrop and Reorder APIs. Reorderable item may show a button for reordering and may use a DragAndDrop API for reparenting.

Old code is archived in https://github.com/rokups/imgui/tree/reorder-api-dnd

[ocornut]

Suggestion: I'd aim to move the block code above in the loop, maybe start the loop with -1 or end with size()+1 and skip displaying the item but always display the reorder target.

[ocornut]

Would be nice is the demo also implemented the other form of dragging (which doesn't use the drag and drop api)

[ocornut]

1 << 20 should be 1 << 14

[ocornut]

Inconsistent naming between _AcceptNoBorderPadding vs style.AcceptDropPadding ?

I think Border could be renamed, is a reference to how we draw this drop target but we could imagine it would became a solid thin rectangle, so we should focus on semantic e.g. _AcceptNoPadding

And style.DragDropAcceptPadding.
But I'm not sure we need this style variable at all for now unless you have a clear use for it.

[ocornut]

Not sure I understand the logic there, why is g.FontSize used? Is that assuming a certain size for items?
For trees with indentation we'll also want way for the drop target so cover whole width (like ImGuiTreeNodeFlags_SpanFullWidth) so maybe we need more use of this api before designing the thing.

[rokups]

why is g.FontSize used

I intended to get a line height here. Is there a better way? Admittedly font sizes are bit confusing to me, lots of different variables that affect font size.

[ocornut]

Why a "line height" for a separator between item? Shouldn't the separator be very thin (roughly ~style.ItemSpacing.y?).

Text-only line are g.FontSize high, most lines with framed widgets are g.FontSize + style.FramePadding.y * 2

[ocornut]

Ah I think you want to use window->DC.PrevLineSize.y here?

[ocornut]

Thank you @rokups.
I have posted a couple of comments. I made in the code/commit itself as an experiment in reviewing but I'm thinking perhaps proper paragraph would be more helpful. (let me know!)

[rokups]

I made in the code/commit itself as an experiment in reviewing but I'm thinking perhaps proper paragraph would be more helpful.

In-code comments are awesome, please keep doing that 👍

[rokups]

I was thinking and thinking and thinking about this and came to a conclusion that this PR is no good. Proposed helper API implements a terrible UX for reordering. My main issues:

1. Inserted drag area between items is pretty small. Sometimes it takes trying to drop item on to it.
2. In case of trees multiple drop areas overlap each other at the boundaries where tree levels meet. You can observe it somewhere in the following gif. Moving mouse horizontally makes drop area longer or shorter on horizontal axis. Longer area belongs to a parent while shorter one to the last child of that parent.
   

As a possible solution i was considering using simple reordering using mouse drag delta to reorder children within a single parent and use drag&drop api for reparenting items to arbitrary parents. That would require something like holding down a modifier key to enable drag source so that drag delta reodering does not break.

What do you think? There surely must be a better way.

[ocornut]

Hello Rokas,

(1)
Attached a patch on top of your branch (you may want to apply on your side)
patch.zip

(2)
I will merge the "simple" reorder demo asap as well as switching to TreeNode instead of Bullet+Indent, so the branch will have minor fixes to apply.

(3)
On the topic you raise on your last post.
I think the demo is really promising. I agree with the issue you mention but that's the standard behavior used by many other applications, so I think we should just aim to improve it.

• "Inserted drag area between items is pretty small" perhaps the reorder target could be taller, overlapping previous and subsequent item. Note that in BeginDragDropTargetCustom() we already use the target surface derived from DragDropTargetRect to solve overlapping items and giving priority to the smaller surface, or it may just work as is. (Might need a more explicit flag to not assume the fact that other target will be bigger, I don't know).

• Minor: Dragged element may need some feedback at the source location (there's an internal field ImGuiSelectableFlags_DrawHoveredWhenHeld which I've been only using for header elements in the Tables branch, which might become a default behavior, or a public flag for Selectable?)

• Minor: the reorder target perhaps should just be a thick line instead of a rectangle?

• We probably need to come with a mechanism to signify that a drag is not allowed. Altering hovering state of underlying widget? Altering mouse cursor? Altering tooltip?
  That same mechanism could be used when e.g. dragging in the concealed area behind a modal window. GLFW will expose a "not allowed" cursor (currently missing, see Additional Cursors glfw/glfw#427) which may help but we could design a fallback for it.

[ocornut]

I have now merged the "simple reordering" part of the demo, the one not actually using the Drag and Drop api, so your PR will conflict, should be an easy rebase+fix+force push hopefully!

[rokups]

perhaps the reorder target could be taller, overlapping previous and subsequent item.
the reorder target perhaps should just be a thick line instead of a rectangle?

These are very good ideas!

One last thing. What do we do about this bit?

Dragged element may need some feedback at the source location

Maybe this feedback could be an indicator were user is parenting a node?

We probably need to come with a mechanism to signify that a drag is not allowed.

Do we? I mean it is the default state. 99% widgets are not draggable.

[AnimatorJeroen]

Hi ocornut and rokups. This PR looks really nice. Do you have an idea when it will be committed? I tried to replace the 3 modified files: imgui.cpp, imgui.h and imgui_demo.cpp to use drag and drop in my personal project with imgui v1.74, but I get errors with imgui_draw.cpp: "Out-of-line definition of 'CalcCustomRectUV' does not match any declaration in 'ImFontAtlas' ". Thanks in advance, Jeroen

[rokups]

Hey @AnimatorJeroen. This is more of a proof of concept. I am not sure it is a right way to do reordering. No idea if we are going to rework it and merge it. I rebased this branch on to master without any issues and no changes were required. Maybe you could provide full error messages?

[AnimatorJeroen]

Hi Rokups, sorry for the late reply. I use v 1.75 WIP now but I guess I am doing something wrong here. When I replace imgui.cpp, imgui.h and imgui_demo.cpp with your modified versions, I get 17 errors. Thank you!

1>imgui.cpp
1>C:\dev\windows animation app prototype\animation app prototype\vendor\imgui\imgui.cpp(3887,24): error C2039: 'WindowsSortBuffer': is not a member of 'ImGuiContext'
1>C:\dev\windows animation app prototype\animation app prototype\vendor\imgui\imgui_internal.h(989): message : see declaration of 'ImGuiContext'
1>C:\dev\windows animation app prototype\animation app prototype\vendor\imgui\imgui.cpp(4125,24): error C2039: 'WindowsSortBuffer': is not a member of 'ImGuiContext'
1>C:\dev\windows animation app prototype\animation app prototype\vendor\imgui\imgui_internal.h(989): message : see declaration of 'ImGuiContext'
1>C:\dev\windows animation app prototype\animation app prototype\vendor\imgui\imgui.cpp(4126,24): error C2039: 'WindowsSortBuffer': is not a member of 'ImGuiContext'
1>C:\dev\windows animation app prototype\animation app prototype\vendor\imgui\imgui_internal.h(989): message : see declaration of 'ImGuiContext'
1>C:\dev\windows animation app prototype\animation app prototype\vendor\imgui\imgui.cpp(4132,51): error C2039: 'WindowsSortBuffer': is not a member of 'ImGuiContext'
1>C:\dev\windows animation app prototype\animation app prototype\vendor\imgui\imgui_internal.h(989): message : see declaration of 'ImGuiContext'
1>C:\dev\windows animation app prototype\animation app prototype\vendor\imgui\imgui.cpp(4136,5): error C2039: 'WindowsSortBuffer': is not a member of 'ImGuiContext'
1>C:\dev\windows animation app prototype\animation app prototype\vendor\imgui\imgui_internal.h(989): message : see declaration of 'ImGuiContext'
1>C:\dev\windows animation app prototype\animation app prototype\vendor\imgui\imgui.cpp(4137,39): error C2039: 'WindowsSortBuffer': is not a member of 'ImGuiContext'
1>C:\dev\windows animation app prototype\animation app prototype\vendor\imgui\imgui_internal.h(989): message : see declaration of 'ImGuiContext'
1>C:\dev\windows animation app prototype\animation app prototype\vendor\imgui\imgui.cpp(7364,1): error C2244: 'BeginTooltipEx': unable to match function definition to an existing declaration
1>C:\dev\windows animation app prototype\animation app prototype\vendor\imgui\imgui.cpp(7363): message : see declaration of 'BeginTooltipEx'

[rokups]

You should not replace files like that. You should either use all files from my branch or apply changes from this PR manually. Replacing half of library with older versions leads to incompatibilities as you can see.

[rokups]

I updated first post detailing rework pushed to the branch of this PR. Rework is quite different from initially proposed code so please read first post again. Previous code has been archived in another branch.

People following #143 may find this code useful.