Fifourche commented 3 years ago

To adress #18 and #78 ! I switched Animation to an EventedModel in a very basic way, to take it step by step (I'm not sure I get everything right).

As I understand we still need key_frames to be an EventedList so I kept that, but set allow_mutable to False for now, to prevent users from just setting it to a List, that would then not be evented. I think to take this into account I can make a property setter to convert a List to an EventedList, but I I think all callbacks would need to be reset then .

Curious to hear your opinions/ideas, as I'm stepping in something I don't know well !

codecov[bot] commented 3 years ago

Codecov Report

Merging #87 (3c8c2a6) into main (d75e45b) will increase coverage by 0.16%. The diff coverage is 52.94%.

@@            Coverage Diff             @@
##             main      #87      +/-   ##
==========================================
+ Coverage   66.95%   67.12%   +0.16%     
==========================================
  Files          17       17              
  Lines         802      806       +4     
==========================================
+ Hits          537      541       +4     
  Misses        265      265

Impacted Files	Coverage Δ
napari_animation/_qt/animation_widget.py	`27.67% <0.00%> (ø)`
napari_animation/animation.py	`80.14% <87.50%> (+0.60%)`	:arrow_up:
napari_animation/_tests/conftest.py	`100.00% <100.00%> (ø)`
napari_animation/_tests/test_animation.py	`100.00% <100.00%> (ø)`

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 d75e45b...3c8c2a6. Read the comment docs.

Fifourche commented 3 years ago

apparently, Codecov failed because it couldn't find https://github.com/napari/napari-animation/ ...! 😮

alisterburt commented 3 years ago

amazing work @Fifourche - this is pretty much there! I proposed a few changes to make it compatible with #88 at Fifourche/napari-animation#3, if you're happy with those changes and approve them then I think we can merge here! 🙂

tlambert03 commented 3 years ago

I did have one additional thought as I was working on #88 ... which is that if the Animation.frame attribute is pretty much just storing the "current" frame. Then you would get all of that for free by moving to SelectableEventedList. (and then you would have all of the events here to work with (and the equivalent of .frame would either be key_frame.selection.active or key_frame.selection._current). This doesn't need to stop you from using EventedModel here ... but if you don't need an event for .frame, it might make it unnecessary?

alisterburt commented 3 years ago

really interesting idea @tlambert03 , I didn't know about the SelectableEventedList, it looks great!

I think you're right - that approach would be simpler, especially for initialisation https://github.com/napari/napari-animation/blob/8c7119e69933bcba8f0263d3cab966f373a7cc24/napari_animation/animation.py#L69-L72

Animation.frame being out of sync with the selected index has caused problems for us before (e.g. #58)

@Fifourche - what are your thoughts here? You've put in a lot of work and I don't want it to feel wasted!

Fifourche commented 3 years ago

I'm all up for it ! :) we just wanted the right things to be evented, if it's easier without EventedModel then perfect ! It would probably have been more useful if we had a reasonable number of attributes which would have benefited from this, but that's not the case here.

Should I close this PR then ?

alisterburt commented 3 years ago

excellent! Let's go with Talley's suggestion then 🙂 it might take quite a bit of work to reconfigure everything as this is a relatively big change, would you like to take it on?

Re: closing the PR, let's leave it open for now, maybe others will want to weigh in

tlambert03 commented 3 years ago

@Fifourche ... it would be awesome if you'd like to take a stab at it! I'm more than happy to help out.

I made a very preliminary start at it here: https://github.com/napari/napari-animation/compare/main...tlambert-forks:selectable ... what needs to be done now is to go through the rest of the code, wherever self.key_frames[self.frame] is used, and replace it with self.key_frames.selection.active (or ._current?) ... I would try touch the KeyframeWidget itself as little as possible, since that whole thing can be replaced once we move to a SelectableEventedList.

Regarding "_current" vs "active"... the difference is subtle: "_current" really pertains to the current item as far as the GUI is concerned (like, if you press the up key, what is that up "relative to"?). "active" on the other hand is defined as a single selected item (this will very likely correspond to your .frame) ... but note that active in this case is not an index, but rather the keyframe itself.

napari / napari-animation

Switching Animation to an EventedModel #87

Codecov Report