Allow the user to drag faster

[TomMalbran]

This is a simple fix for issues #4920 and #5999.

cc @njx @peterflynn

[njx]

I haven't looked closely at the code yet, but while trying it out I noticed weird jumpy behavior if the working set is long enough to scroll and you drag the item to the top or bottom so the working set autoscrolls. (That bug might have been there before, though :))

[njx]

One other thought: should we be using capture handlers for mousemove/up, so that if the user drags outside the container we still get the events? With the current setup, if my mouse slips outside the sidebar area, the drag stops, which seems wrong - I would expect it to capture as long as the mouse is down.

[TomMalbran]

The first bug is there in master too. Trying to reproduce it really hard to catch. And I am not quite sure what is going on, something in the calculations is a few pixels off or the mouse suddenly lands in the middle of each item.

The initial PR was listening for the events on the document, but you cloud still drag outside of the Brackets window, so I changed it. I can just change the $holder variable to any element and it should still work. But I am not sure how much sense it would make to drag an item so far away from it. I think is fine to restrict it to the sidebar, if you drag from the filename, is hard to end up outside of the sidebar.

[njx]

My general feeling is that as a matter of policy, for drag operations, you shouldn't stop on mouseleave - you should only stop on mouseup (and you should capture mousemove/mouseup so the drag works even if you're outside the original container). It avoids edge cases where you might have started the drag near the edge of the sidebar (e.g. if the filename is long), or if the mouse briefly leaves the window. It's true that it's probably not that common for the mouse to wander too far away, but this is the kind of thing where the interaction can feel flaky/unpolished if you do run into it.

[njx]

Unassigning myself and marking PR Triage Complete. I think it's worth changing this per my last comment before accepting.

[TomMalbran]

@njx I totally forgot about this PR. The change you ask is easy to do. I can just remove the mouseleave event. Let me try to do that fast and you can check it.

[TomMalbran]

@njx I did the changes you wanted.

[JeffryBooher]

I'm going to pick this up because I want to get the changes in for splitview

[njx]

Thanks @JeffryBooher, sorry I dropped the ball on this.

[JeffryBooher]

I'm not sure that this pull request makes the drag any faster. I compared Release 0.42 to this branch and there wasn't a noticable improvement in performance (although it feels fine to me in either case).

It does, however, address the issue that @njx mentioned about not the drag operation finalizing when the mouse slips outside the window. My concern now is that if the mouse is not inside the window, it continues to drag. This isn't a big deal but I don't know of any other application that does this -- most apps stop dragging when the mouse moves outside the window but don't finalize the drag operation until the mouse up. The drag operation would continue once the user moves the mouse back inside the window.

I also confirmed that the autoscroll "bump" occurs in release 0.42 so this isn't newly introduced by this change.

@njx what do you think? If we can live with the continued drag behavior then I think this branch is good to go.

[TomMalbran]

@JeffryBooher The point of this PR was not to address any performance issue, but to allow the user to drag faster. Without this branch, when you drag too fast, the item is dropped and you have to start again.

[njx]

Yup, as @TomMalbran mentioned the fix is that if the user drags fast it would stop dragging. See #4920. I just tried the branch out and it looks good to me.

We should probably file a separate bug on the autoscroll behavior after this lands - I'm noticing that the position of the dragged item gets messed up after you autoscroll - that bug was probably there before, but wasn't noticeable because you could never drag far enough for it to autoscroll quickly before it stopped due to the original bug.

[JeffryBooher]

@TomMalbran ah, the title threw me. It prevents dragged items from being dropped as a result of the mouse leaving the window.

[JeffryBooher]

Looks good. Merging

[peterflynn]

Boy, I was confused enough about how this fix worked that I couldn't stop myself from debugging the old code for a minute. For the record, the root cause of the bug was that a drag can move fast enough that the if (Math.abs(top) > height / 2) succeeds before we ever get into the if (!moved && Math.abs(top) > 3) block below. (I.e., we get a mouse event that falls outside the list item bounds without first getting a mouse event inside the bounds but > 3px away from the start pos). This causes us to move the item one notch before _suppressSortRedraw was set, and it turns out that flag is not just a performance or anti-flicker nicety -- it's essential for correctly computing the new values of $prevListItem & $nextListItem. Once those have bad values, no further mouse movement can result in any more dragging (until mouseup trigger drops() and you mousedown to try again).

So I think an alternative fix could have been to move the 3px-check to the top of drag(). Nothing wrong with the fix here, though! :-)

[TomMalbran]

@peterflynn That is right. The issue is that DocumentManager.swapWorkingSetIndexes() is called before _suppressSortRedraw is set to true. swapWorkingSetIndexes triggers workingSetSort which re-renders the entire list since _suppressSortRedraw is still false. And finally since we have new DOM objects, the values of $prevListItem & $nextListItem are no longer valid.