Fifourche commented 3 years ago

This PR is a follow up on the #87 ! Starting from @tlambert03 suggestions, I'm trying to switch Animation.key_frames to a SelectableEventedList, and change the widgets accordingly.

This PR is functional and pass tests, but there is still a lot to discuss. Main modifications are :

Switched key_frames to a SelectableEventedList
Decided to mainly use the key_frames.selection._current reference (as opposed to the active one) to manage selected key frame, and linked it to Animation.current_key_frame. Still I defined an active_key_frame attribute in case.
I unfroze KeyFrame, as we want to modify some of its contents (from the GUI via the steps widget for instance).
Added an __eq__ to KeyFrame as value equality is checked during some SelectableEventedList methods. Also added one on ViewerState.
I modified as little as possible the KeyFramesListWidget as suggested in #87
Tried to manage callbacks there, so it's functional but I guess it's still way unoptimal.
I added an update of displayed steps and ease widgets when the slider's moving.

I'm happy to start a discussion on this ! 😅

codecov[bot] commented 3 years ago

Codecov Report

Merging #90 (e55f88c) into main (8c7119e) will decrease coverage by 0.56%. The diff coverage is 48.38%.

@@            Coverage Diff             @@
##             main      #90      +/-   ##
==========================================
- Coverage   67.89%   67.32%   -0.57%     
==========================================
  Files          18       18              
  Lines         816      860      +44     
==========================================
+ Hits          554      579      +25     
- Misses        262      281      +19

Impacted Files	Coverage Δ
napari_animation/_qt/animation_widget.py	`26.95% <0.00%> (-0.73%)`	:arrow_down:
napari_animation/_qt/frame_widget.py	`31.91% <0.00%> (-2.97%)`	:arrow_down:
napari_animation/_qt/keyframeslist_widget.py	`30.52% <23.80%> (-0.16%)`	:arrow_down:
napari_animation/animation.py	`75.20% <68.42%> (+0.91%)`	:arrow_up:
napari_animation/key_frame.py	`97.22% <91.66%> (-2.78%)`	:arrow_down:
napari_animation/_tests/conftest.py	`100.00% <100.00%> (ø)`
napari_animation/_tests/test_animation.py	`100.00% <100.00%> (ø)`
napari_animation/_tests/test_interpolation.py	`100.00% <100.00%> (ø)`
napari_animation/_hookimpls.py	`66.66% <0.00%> (-33.34%)`	: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 8c7119e...e55f88c. Read the comment docs.

tlambert03 commented 3 years ago

This is great @Fifourche! I was able to pretty much immediately use this to swap out a huge chunk of code (see #91).

I think my main comment surrounds the Animation.current_key_frame property. In that property, you convert the _current keyframe object into an integer with self.key_frames.index(self.key_frames.selection._current) ... but then almost everywhere else where that property is used, it gets converted back to the object itself:

key_frame_idx = self.animation.current_key_frame
self.stepsSpinBox.setValue(self.animation.key_frames[key_frame_idx].steps)

While I know that that's more like the current pattern of "animation.frame is an integer index"... I think that the SelectableEventedList kind of allows us not to track that index at all. Instead, just directly use self.key_frames.selection._current:

self.stepsSpinBox.setValue(self.animation.key_frames.selection._current.steps)

and, if you really need to know what the index is somewhere, then use index() at that point.

but other than that, I'm impressed! great work

Fifourche commented 3 years ago

Wow, thanks a lot ! ^^ I agree with everything you said ! It's true that it's easier indeed to pass the current KeyFrame around rather than the playing with its index, but I thought we might want to keep an attribute with a similar use as frame had, i.e. a way for the user to set the current key_frame programmatically by giving an index ! I agree this should be decoupled from the way to pass current KeyFrame internally.

Also, I'm not sure to understand what would be the use case of having current AND active ; wouldn't it be simpler to just have one of both ?

I'll sleep on it, and I'll check #91 !

tlambert03 commented 3 years ago

I thought we might want to keep an attribute with a similar use as frame had, i.e. a way for the user to set the current key_frame programmatically by giving an index !

sure, you could expose that... but it's not hard for them to just use active = key_frames[n]. In napari, for layers if they wanted to select the nth layer, I think we'd just tell them to use layers.selection.active = layers[n] (rather than creating a new API to do the indexing for them)

Also, I'm not sure to understand what would be the use case of having current AND active ; wouldn't it be simpler to just have one of both ?

yep! I agree. I would only use one (i only had them both in there to follow your pattern). I think most cases, it will be active that makes the most sense... so maybe use that until you run into limitations.

Fifourche commented 3 years ago

So should I try to fix the codecov related problems before merging this one ? #91 should fix them anyways while changing for some nicer things...

alisterburt commented 3 years ago

awesome work @Fifourche and thanks for reviewing @tlambert03 !!

I'm going to merge this as is because Talley's followup #91 contains his suggested changes, no worries for the codecov! Great job

napari / napari-animation

Switch key_frames to a SelectableEventedList #90

Codecov Report