search

napari / napari-animation

A napari plugin for making animations
https://napari.github.io/napari-animation/
Other
79 stars 27 forks source link

Drop inserting in capture (changing in-place) and instead just add and then remove frames #233

Closed psobolewskiPhD closed 2 months ago

psobolewskiPhD commented 2 months ago

Closes: https://github.com/napari/napari-animation/issues/231

So in https://github.com/napari/napari/pull/7150 event handling order was changed in napari. This is making the key frame list manipulations for inserting a replacement frame not work correctly when the list change event callbacks trigger. I tried passing position='first' in the .connect() in various places and could not get it to resolve properly -- it's events of the SelectableEventedList, which are on the napari side I think.

I also tried to keep the name of the frame when insert=False, but the hash is still different so selection still fails.

In this PR I work around all this event stuff by removing the insert kwarg on capture_frame and no longer edit the frame in-place when replacing. Instead, the new frame is added and previous is removed. Note that the frame number was already incrementing even when replacing a frame (insert=False) and because one can re-order frames, the numbers lose meaning.

codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.

Project coverage is 86.08%. Comparing base (ba8ab53) to head (d1a07a0). Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
napari_animation/_qt/animation_widget.py 83.33% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #233 +/- ## ========================================== - Coverage 86.24% 86.08% -0.16% ========================================== Files 26 26 Lines 1076 1071 -5 ========================================== - Hits 928 922 -6 - Misses 148 149 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

alisterburt commented 2 months ago

Superseded by #234 - thanks for delving though!