SimpleDragHandler dispose may be incomplete

[samreid]

In phetsims/equality-explorer#25 (comment) @pixelzoom said:

@samreid said:

The SimpleDragHandler.dispose was added by @jonathanolson in #647

@jonathanolson can you comment on why dispose does only some of the "cleanup" things that normally occur in endDrag? If dispose is called during a drag cycle, this.dragging remains true, the client's options.end callback is never called, and this.pointer still holds a reference to the pointer.

This has nothing to do with sim-specific code and is blocking my ability to fuzz test. So I commented out the failing assertion in the above commit.

@samreid @jonathanolson There appear to be 2 problems here:

(1) startEvent/endEvent pairing is not considering the possibility of an interrupted drag cycle.

(2) The cleanup done by SimpleDragHandler dispose appears to be incomplete, when compared with endDrag.

[samreid]

@jonathanolson replied:

@jonathanolson can you comment on why dispose does only some of the "cleanup" things that normally occur in endDrag? If dispose is called during a drag cycle, this.dragging remains true, the client's options.end callback is never called, and this.pointer still holds a reference to the pointer.

(2) The cleanup done by SimpleDragHandler dispose appears to be incomplete, when compared with endDrag.

Presumably if there's a chance of dispose() being called during a drag, we COULD add interrupt() to the dispose()? It feels slightly unclean to DO things other than dispose during a dispose call, and I'd consider requiring clients to make sure they don't dispose something while dragging.

As for the unchanged variables on the SimpleDragHandler, it shouldn't matter. If you dispose it, you shouldn't be accessing it afterwards or assuming things about its state.

So I can:

1. Add an assertion that prevents disposing a "dragging" SimpleDragHandler, or
2. Add an interrupt() call at the start of dispose().

... but will endDrag( null ) cause problems? I sure hope not, because that's the current recommended way of ending a drag cycle programmatically (for SimpleDragHandler), and is used in function-builder and unit-rates.

It should be permitted to call interrupt() yourself. Some things assume the presence of event.pointer and event.currentTarget (which it handles). In general, I think it's best to never assume there is an actual DOM input event for end drags.

[pixelzoom]

So I can:

1. Add an assertion that prevents disposing a "dragging" SimpleDragHandler, or
2. Add an interrupt() call at the start of dispose().

I would anticipate that (1) is going to break some existing code, since you're adding a brand new requirement.

If you go with (2), you'll need to do more than just call interrupt. You also need to add options.interrupt (or some similar name), so that clients have a hook to do things (like cleanup) if they are interrupted. Imo this was an oversight that should have been added when interrupt was added. For example, consider a client that links to a Property in options.start, then expects to unlink in options.end. If there's no options.interrupt, then there will be a memory leak if a drag cycle is interrupted.

[pixelzoom]

Regardless of which option you go with... SimpleDragHandler needs a callback for interrupt, and the semantics of "end drag" need to be more solidly defined. See #726.

[samreid]

Unassigning until we hear from @jonathanolson.

[jonathanolson]

You also need to add options.interrupt (or some similar name), so that clients have a hook to do things (like cleanup) if they are interrupted. Imo this was an oversight that should have been added when interrupt was added. For example, consider a client that links to a Property in options.start, then expects to unlink in options.end. If there's no options.interrupt, then there will be a memory leak if a drag cycle is interrupted.

That seems incorrect. Not only is the dragHandler.interrupted set to true, but the end callback WOULD be fired (as that's kind of the purpose of interrupt).

If you just care if the drag ends, use end. If you care if it's interrupted, check the handler.interrupted flag in your end callback.

If people would prefer me to also add interrupt and cancel callbacks (when drags end due to those different causes), it should be possible.

I would anticipate that (1) is going to break some existing code, since you're adding a brand new requirement.

Yes. If we go that route, presumably I'd run aqua testing and handle refactoring for all cases where it would break.

[pixelzoom]

@jonathanolson said:

That seems incorrect. Not only is the dragHandler.interrupted set to true, but the end callback WOULD be fired (as that's kind of the purpose of interrupt).

Confirmed what you said by reading the code. This was a case of me not understanding the semantics of "end", since it's not clearly documented anywhere. Apparently it's called by interrupt, but not called (for some reason) by cancel.

If you just care if the drag ends, use end. If you care if it's interrupted, check the handler.interrupted flag in your end callback.

No visibility annotations in SimpleDragHandler, so I assumed that interrupted was @private. Should I assume that everything is @public?

If people would prefer me to also add interrupt and cancel callbacks (when drags end due to those different causes), it should be possible.

Based on your clarification, I don't think that's necessary. What I do think is necessary is documentation. For example, I still don't know when it's appropriate to use cancel, or why it's touch-specific (the sole doc is "// touch cancel").

[jonathanolson]

I'll work at improving documentation in SimpleDragHandler to clarify everything. For reference, cancel is like up, it's a Scenery event triggered from things like DOM touchcancel events. Since we haven't really treated these differently than up events, it essentially triggers the same "end drag" behavior. It's separate from interruption, which is used from the Scenery side to force drags to end (if there is an ongoing drag).

[samreid]

Not sure how to help, unassigning for now but please reassign me if you need me.

[jonathanolson]

It seems like dispose() in the current version looks pretty complete. The only change I could imagine would be interrupting the listener in the disposal, but it still sounds weird.

@samreid should we discuss changes to the current SimpleDragHandler?

[samreid]

No visibility annotations in SimpleDragHandler, so I assumed that interrupted was @Private. Should I assume that everything is @public?

That looks like it has since been annotated.

The only change I could imagine would be interrupting the listener in the disposal, but it still sounds weird.

Adding this.interrupt() during dispose sounds like the right thing to do.

It also appears this issue was sparked by a comment from @pixelzoom in phetsims/equality-explorer#25 (comment) -- @pixelzoom what else do you think remains for this issue?

[pixelzoom]

Calling interrupt on dispose sounds appropriate if there is a drag cycle in progress. Otherwise dragging could continue after dispose. I don't know if that will cause other problems, or whether there should be a test in dispose that a drag cycle is in progress. The other option (which I think @jonathanolson said something about) would be to not allow dispose to be called during a drag cycle.

I can't tell what in phetsims/equality-explorer#25 (comment) was or was not addressed, so I have no idea if there's anything remaining to do here.

[samreid]

@jonathanolson do you think this issue can be closed?

[jonathanolson]

This looks complete to me, closing.