Add new on_drag_leave Enum trait to DragTool

[aaronayres35]

fixes #551

It seems like this fix is going to require a change in behavior and may potentially "break" existing code.

Currently the end_drag_on_leave flag does not actually lead to a drag being "ended" on a mouse leave. Instead it is canceled. My thoughts were to either add a cancel_drag_on_leave trait to do what end_drag_on_leave currently does (ie cancel the drag) and change end_drag_on_leave to actually end (see first commit). However, this requires one of these having preference over the other if both are True, and in actuality you would only ever want one behavior, so I removed end_drag_on_leave and made a new on_drag_leave enum trait.

This however is a change in the public api so we need to tread very carefully. I am unsure what exactly we want to go with here, and thus am leaving this as a WIP for now awaiting further discussion.

I have tested manually using this example https://github.com/enthought/chaco/blob/master/examples/demo/edit_line.py.
I will write unit test(s) once the approach has been decided. CI will fail as I have not yet updated the existing tests. Opened the PR early so other could see and discuss

[jwiggins]

I'm glad you pushed this as a WIP, because I think we perhaps should change nothing and instead add better documentation above the trait and in the docstring for drag_end (and maybe drag_cancel too).

I'll start by saying end_drag_on_leave is a bad name. #naming-things-is-hard, right? So a new trait with clear meaning might be a good long term fix. But would better documentation work too?

At least from my reading the DragTool code, drag_cancel is the callback for drags which end prematurely and drag_end is for drags which end normally. That's not at all clear just from reading the docstrings, so we should fix that. But I think if we start using drag_end for drags which ended prematurely that it's just as bad. We would have to introduce yet another callback method drag_finished (or something like that; naming things is hard).

[corranwebster]

I agree with John here - poorly named trait, documentation could be better (but it is explicitly called out in the drag_cancel docstring). If a code fix is needed, it would be to create a new cancel_drag_on_leave trait that is an alias for end_drag_on_leave (or vice-versa) and aim to remove end_drag_on_leave in Enable 6 or some similar future point.

My intuition is that the "ending" a drag normally when the mouse is still down will lead to an odd UX, but I could be convinced by a concrete example where the behaviour is desirable.

[aaronayres35]

My intuition is that the "ending" a drag normally when the mouse is still down will lead to an odd UX, but I could be convinced by a concrete example where the behaviour is desirable.

A concrete example use case, coming straight from the horse's mouth, would be "trying to enclose something with a bounding box near the border of an image".
You don't want the box to extend beyond the border, (hence you don't want end_drag_on_leave = False), but you also don't want to cancel if you drag outside the border.

[aaronayres35]

I dont think this note is correct? If I leave capture_mouse as true, but set end_drag_on_leave to true and mess around with examples, I am seeing mouse_leave events fired and the drags cancelled on leave?
Maybe I am missing something

[jwiggins]

It might be different between Qt and Wx.

[aaronayres35]

Completely orthogonal, but this just made me remember that chaco no longer lists wx as being supported in its ci/edmtool.py / wx is not exercised on its CI ...

[aaronayres35]

Okay, I've taken a new stab at this given the feedback.

Basically, the current implementation is:
if a user sets end_drag_on_leave = True the existing behavior is completely preserved aside from a deprecation warning being raised. The deprecation warning is raised when the trait is used, ie when a mouse leave event is triggered if it is set. Perhaps the deprecation warning would better live in a trait change handler to raise whenever the trait is changed? (as I am writing this I think that may make more sense... 🤔). The new on_drag_leave trait is effectively ignored (to avoid any potential conflicts). This way existing code in the wild can go on as is without risk of breaking. I've added the deprecation warning and added to comments / docstrings to make it more clear that end_drag_on_leave actually means drag_cancel will be called. Nonetheless the trait is kept exactly as is, we just warn about it being potentially confusing / misleading.

If a user does not explicitly set end_drag_on_leave = True, they are free to use the new feature. Namely, they can chose to set the new on_drag_leave enum trait to either "end" or "cancel" to do what you would expect given the choice. This provides the appropriate level of flexibility and lets users chose what they want. Whether you want to cancel or end on a leave or do nothing is context specific, and users should be free to decide on the appropriate behavior for their particular usecase.

finally, if neither trait is set, behavior is preserved as the on_drag_leave defaults to None.

I've added unit tests to check the on_drag_leave options work as expected, and that on_drag_leave does nothing if end_drag_on_leave is set.

Note that this means if users want the new feature to end on leave not cancel after having perviously had end_drag_on_leave = True, they have to set on_drag_leave = 'end' and remove the end_drag_on_leave = True.

[aaronayres35]

maybe I should make it more clear here that on_drag_leave = 'cancel' is equivalent to end_drag_on_leave = True not on_drag_leave = 'end'? That confusion is sort of the root problem at hand

[jwiggins]

Could you please add some more details to the drag_end callback like you did for drag_cancel?