glue-viz / bqplot-image-gl

Jupyter widget for displaying images with a focus on astronomy
MIT License
19 stars 13 forks source link

fix: Allow for .next to be a MouseInteraction and clean up its event handlers #86

Closed maartenbreddels closed 2 years ago

maartenbreddels commented 2 years ago

fix: Allow for .next to be a MouseInteraction and clean up its event handlers

We now remove the .drag event handlers, but, because multiple MouseInteractions share the same DOM node and event name, this will remove all event handlers.

Therefore, if the 'parent' MouseInteraction reattaches its event handlers when .next changes this is not an issue.

Also, if .next changes quickly, we might attach a stale interaction because we awaited while in the meantime .next already changed. This caused event handlers to be added, and never removed.

codecov[bot] commented 2 years ago

Codecov Report

Merging #86 (85bc685) into master (3e6077f) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #86   +/-   ##
=======================================
  Coverage   55.38%   55.38%           
=======================================
  Files           7        7           
  Lines         130      130           
=======================================
  Hits           72       72           
  Misses         58       58           

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 3e6077f...85bc685. Read the comment docs.

maartenbreddels commented 2 years ago

Note that this does not seem to be a problem in Jupyter lab, which I am not sure if it can be considered a notebook issue, but I commented on this at March 2, 2022 1:18 PM

maartenbreddels commented 2 years ago

@kecnry This should fix the issues you mentioned in https://github.com/spacetelescope/jdaviz/pull/1013#issuecomment-1021350256 but it would be great if you can confirm it.

kecnry commented 2 years ago

Seems to have done the trick! Thanks @maartenbreddels!