1314322 - Add more general way to override RecyclerView item animations

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Tom Klein]

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.

Comment 1

[Tom Klein]

Attachment: Bug 1314322 - Pre: Don't reset view attributes in onBindViewHolder.

Review commit: https://reviewboard.mozilla.org/r/89848/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/89848/

Comment 2

[Tom Klein]

Attachment: Bug 1314322 - Add DefaultItemAnimatorBase.

Review commit: https://reviewboard.mozilla.org/r/89850/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/89850/

Comment 3

[Tom Klein]

Attachment: Bug 1314322 - Create new add animation for tabs list.

Make the TabsListLayoutAnimator a DefaultItemAnimatorBase.

Review commit: https://reviewboard.mozilla.org/r/89852/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/89852/

Comment 4

[Tom Klein]

(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.)

Comment 5

[Tom Klein]

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?

Comment 6

[Tom Klein]

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.

Comment 7

[Tom Klein]

Attachment: [unused] DefaultItemAnimatorBase.java with overrides for all animations, not just add and remove

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 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8806411 [details]
Bug 1314322 - Pre: Don't reset view attributes in onBindViewHolder.

https://reviewboard.mozilla.org/r/89848/#review89626

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8806412 [details]
Bug 1314322 - Add DefaultItemAnimatorBase.

https://reviewboard.mozilla.org/r/89850/#review89658

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8806413 [details]
Bug 1314322 - Create new add animation for tabs list.

https://reviewboard.mozilla.org/r/89852/#review89660

Comment 11

[Tom Klein]

Comment on attachment 8806412 [details]
Bug 1314322 - Add DefaultItemAnimatorBase.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/89850/diff/1-2/

Comment 12

[Tom Klein]

Comment on attachment 8806413 [details]
Bug 1314322 - Create new add animation for tabs list.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/89852/diff/1-2/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
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

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/87ef83e2f8f2
https://hg.mozilla.org/mozilla-central/rev/770c20d40680
https://hg.mozilla.org/mozilla-central/rev/10353378f4fa