napari / napari-animation

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

Use napari's QtListView for KeyFramesListWidget #91

Closed tlambert03 closed 3 years ago

tlambert03 commented 3 years ago

This PR follows on the work that @Fifourche is doing in #90 and swaps out the KeyFramesListWidget for napari's QtListView.

This PR includes #90, so that PR should be merged first... but this gives a sense for how that PR can be leveraged to remove a big widget from the codebase.

I'm afraid I also committed the sin of touching unrelated things: specifically, I changed a number of the single-use methods like _init_x() to be used inline in the __init__ method. This is entirely personal preference... so @alisterburt, just say the word and I'll revert those changed. Even though it does make for a longer method, I find that it's much easier to follow the flow of QtWidget creation if I don't have to go search what each _init_something() method is doing individually. Also, it makes it a bit easier to see what order things are being added to the layout when they're all right next to each other.

codecov[bot] commented 3 years ago

Codecov Report

Merging #91 (a04d1b4) into main (03bc9db) will increase coverage by 7.60%. The diff coverage is 39.82%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #91      +/-   ##
==========================================
+ Coverage   67.32%   74.93%   +7.60%     
==========================================
  Files          18       18              
  Lines         860      718     -142     
==========================================
- Hits          579      538      -41     
+ Misses        281      180     -101     
Impacted Files Coverage Δ
napari_animation/_qt/animationslider_widget.py 33.33% <0.00%> (ø)
napari_animation/_qt/animation_widget.py 28.00% <10.25%> (+1.04%) :arrow_up:
napari_animation/_qt/frame_widget.py 31.25% <20.00%> (-0.67%) :arrow_down:
napari_animation/_qt/keyframeslist_widget.py 34.78% <34.78%> (+4.25%) :arrow_up:
napari_animation/key_frame.py 94.87% <66.66%> (-2.36%) :arrow_down:
napari_animation/_tests/test_animation.py 100.00% <100.00%> (ø)
napari_animation/_tests/test_utils.py 100.00% <100.00%> (ø)
napari_animation/animation.py 81.65% <100.00%> (+6.45%) :arrow_up:
napari_animation/utils.py 97.05% <100.00%> (-0.31%) :arrow_down:

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 03bc9db...a04d1b4. Read the comment docs.

tlambert03 commented 3 years ago

glad you like it. don't merge this just yet. I found a couple issues with _current vs active that I want to hammer out first (might actually need fixing upstream).

tlambert03 commented 3 years ago

ok, figured it out and fixed it here. There is a slight issue that I'll open upstream, which is that when you set an item in the list key_frames[i] = new_frame then the removed event isn't emitted for the previous item (instead a changed event is emitted... but that fails to remove the old item from the selection if it was selected before deletion). So the replace-keyframe option wasn't working. I do it in two steps now (i.e. first remove the old item, then insert the new one), and will follow up in napari.

Other than the regression of #69, this is ready to go. Can fix that here or in a new PR

tlambert03 commented 3 years ago

fixed the slider regression

alisterburt commented 3 years ago

amazing, thanks for this and fixing the regression too! let's get this merged 🚀