Closed Bug 1314322 Opened 3 years ago Closed 3 years ago

Add more general way to override RecyclerView item animations

Categories

(Firefox for Android :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 52
Tracking Status
firefox52 --- fixed

People

(Reporter: twointofive, Assigned: twointofive)

References

Details

Attachments

(4 files)

The remove animation I added in bug 1116415 (TabsListLayoutAnimator) works, but only because remove animations always run first (so I didn't need to track what other animations were being added), and because I was only overriding the remove animation.  We need something more general to be reusable and able to handle overriding more than one animation.

The approach I'm taking is essentially that of Wasabeef's recyclerview-animators:
https://github.com/wasabeef/recyclerview-animators
and specifically the BaseItemAnimator class:
https://github.com/wasabeef/recyclerview-animators/blob/master/animators/src/main/java/jp/wasabeef/recyclerview/animators/BaseItemAnimator.java

The differences in the version here are:
* based on the most recent DefaultItemAnimator, which adds the SimpleItemAnimator class and support for reusing viewHolders in change animations, along with setSupportsChangeAnimations (which I seem to need - see followup comment) and other things I haven't had need to use yet (like payloads).

* I've removed Wasabeef's support for per-viewHolder animation overrides.  It won't be difficult to add it in if we need it, but I don't see any current need.

* I'm requiring callers to call resetAnimation to reset any currently running animation on their view (animateAdd/preAnimateAdd or animateRemove/preAnimateRemove) - in my case we can have a swipe animation trigger an animateRemove, in which case we want to be able to detect that and not cancel the currently running swipe animation.  More generally if you want to pick up any of the currently running animation's values to take them over, you need to be able to call endAnimation on the currently running animation yourself.

* I'm allowing callers to return a boolean from preAnimate(Add|Remove), indicating whether they want to run the animation or not.
(This is just for the record (something I noticed while testing DefaultItemAnimatorBase).

The add animation I've added to TabsListLayoutAnimator (which is really a move slide in from the right) illustrates an underlying general issue with the DefaultItemAnimator implementation if you use setSupportsChangeAnimations(true) in that case (which we don't in production - this is just a test scenario).  The setup is adding a new tab, which causes the currently selected tab to move.  If the SELECTED for the new tab and the UNSELECTED for the existing tab arrive after the animations have been queued, then the animateChange triggered by the SELECTED/UNSELECTED changes the add animation on the new tab into a change animation and changes the move animation on the existing tab into a change animation as well (or it was already a change animation) - but whereas before the add animation (which is now canceled) was waiting on the  move/change animation to finish, in the new runPendingAnimations run the two change animations run at the same time, so the add winds up running over the move animation.)
Sebastian, as I mentioned over email, this new DefaultItemAnimatorBase could just replace the NoChangeItemAnimator I introduced in bug 1313144.  The animateChange fix from that bug is baked into DefaultItemAnimatorBase here since I think that's the right way to do it in generality, and the first commit here covers moving the animation resets out of onBindViewHolder as in 1313144 (needed for testing this bug).

This patch uses DefaultItemAnimatorBase to replace the current TabsListLayoutAnimator and add a new animation, which is looking good, and I've also tested by using DefaultItemAnimatorBase to create an add animation for the new (unpublished) TabStripView I'm working on, which looks good.  So I'm not seeing any issues, and (if accepted) I think this is the way forward for RecyclerView animations.  On the other hand, the bug 1313144 work is more basic and targeted, so safer.  Any thoughts on what you'd prefer at this point in the release cycle?
See Also: → 1313144
To clarify here:
> * I'm requiring callers to call resetAnimation to reset any currently
> running animation on their view (animateAdd/preAnimateAdd or
> animateRemove/preAnimateRemove) - in my case we can have a swipe animation
> trigger an animateRemove, in which case we want to be able to detect that
> and not cancel the currently running swipe animation.  More generally if you
> want to pick up any of the currently running animation's values to take them
> over, you need to be able to call endAnimation on the currently running
> animation yourself.
> 
The alternative is to call resetAnimation on every animateAdd/animateRemove before calling the user's preAnimateAdd/preAnimateRemove override, which is the way Wasabeef's animator works.  And I'm saying sometimes you want to know the values of the current animation before calling resetAnimation yourself (you can't call resetAnimation after preAnimate(Add|Remove) because preAnimate sometimes needs to preset positions/alphas for its animation, which you would then be squashing).

And a couple more notes on wasabeef's approach:
* you create a new add or a remove animation by reimplementing preAnimate(Add|Remove)Impl and animate(Add|Remove)Impl.  The "pre" versions get called in the initial animate(Add|Remove) and are used to setup the initial position of the animation (if it's not the current position); the non-pre versions are called (by runPendingAnimations) to actually run the animation.  Examples in the third commit here.

* the main thing is that all of the animation bookkeeping is moved into the Default(Add|Remove)VpaListener - as long as implementors add a listener to their animation in animate(Add|Remove)Impl, the listener will take care of calling animateAddStarted, adding the animation to the (private) running animations list, resetting everything when the animation ends, etc.
I originally added support to DefaultItemAnimatorBase for overriding all of the animations, not just add and remove.  But it does add a little extra complexity, and it's not clear that we'll ever need to actually reimplement a move or change animation (none of Wasabeef's animations do), so I'm just submitting this version for the record in case we ever want it.
Comment on attachment 8806411 [details]
Bug 1314322 - Pre: Don't reset view attributes in onBindViewHolder.

https://reviewboard.mozilla.org/r/89848/#review89626
Attachment #8806411 - Flags: review?(s.kaspari) → review+
Comment on attachment 8806412 [details]
Bug 1314322 - Add DefaultItemAnimatorBase.

https://reviewboard.mozilla.org/r/89850/#review89658
Attachment #8806412 - Flags: review?(s.kaspari) → review+
Comment on attachment 8806413 [details]
Bug 1314322 - Create new add animation for tabs list.

https://reviewboard.mozilla.org/r/89852/#review89660
Attachment #8806413 - Flags: review?(s.kaspari) → review+
Pushed by twointofive@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/87ef83e2f8f2
Pre: Don't reset view attributes in onBindViewHolder. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/770c20d40680
Add DefaultItemAnimatorBase. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/10353378f4fa
Create new add animation for tabs list. r=sebastian
Duplicate of this bug: 1313144
You need to log in before you can comment on or make changes to this bug.