phetsims / expression-exchange

"Expression Exchange" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
2 stars 2 forks source link

unlink issues #130

Closed pixelzoom closed 7 years ago

pixelzoom commented 7 years ago

Related to https://github.com/phetsims/axon/issues/140. These are all "tried to removeListener on something that wasnt a listener" assertion failures.

(1) Expression

482  coinTerm.localViewBoundsProperty.lazyLink( this.setResizeNeededFlag.bind( this ) );
507 coinTerm.localViewBoundsProperty.unlink( this.setResizeNeededFlag.bind( this ) );

bind creates a new function instance, so lazyLink and unlink are being called with different functions. This can be fixed by creating one instance of the function and using it in these 2 places. I.e. in the constructor:

// @private
this.setResizeNeededFlagBound = this.setResizeNeededFlag.bind( this );

(2) ExpressionOverlayNode

204 expression.inProgressAnimationProperty.unlink( updateDragHandlerAttachmentState );

There is no corresponding link call. expression.inProgressAnimationProperty appears as a updateDragHandlerAttachmentMultilink, which is already disposed. It's likely that thisunlinkis vestigial (from a time before the Property was a dependency ofupdateDragHandlerAttachmentMultilink`) and can be deleted.

pixelzoom commented 7 years ago

The above 2 issues are fixed. While testing, I ran into an unrelated assertion failure (#129) and had to comment out that assertion in order to complete testing. But with ?ea&fuzzMouse, I see no more unlink issues.

@jbphet please review. Especially confirm that the fix for (2) above was OK.

pixelzoom commented 7 years ago

Hmmm... I just noticed that these issues were unlink failures, and https://github.com/phetsims/axon/issues/140 (Phase 2) is related to removing the guard from link. Why didn't these issues manifest during Phase 1? (https://github.com/phetsims/axon/issues/132).

jbphet commented 7 years ago

Changes look good, thanks. No idea why these didn't show up during Phase 1.

Closing.