Fix TabBarView desynchronized after animation interruption

[bleroux]

Description

This PR fixes a DrivenScrollActivity issue which surfaced in TabBar/TabBarView desynchronization (see #132293).

A comment in DrivenScrollActivity constructor states that _end will not be called if the animation controller is disposed.
While investigating this tab bar view issue, I noticed that the _end method of a first DrivenScrollActivity was called after this activity was disposed (and in the tab bar view case, this leads to goBallistic and goIdle being called and inteferring with a new DrivenScrollActivity that just started).

Filing this PR as a WIP to gather feedback because It seems strange that this issue never surfaced before so maybe there is something wrong on how TabBarView calls PageController.animateTo(), but after hours of investigation It seems that DrivenScrollActivity._end being called when it is not expected is a good candidate (and the small change in this PR fixes the TabBarView issue). It is also possible that this issue requires very specific conditions to surface.

In the TabBarView case this issue surfaced after #122021 that made it possible to interrupt a pending tab bar view animation and start a new one (before that PR, a tab bar view animation had to end before a new one is started).

Related Issue

Fixes #132293.

Tests

Adds 1 tests.
I will add a second one in scroll_activity_test.dart if this fix is valid.

[Piinks]

I think this approach makes sense, just a question below for clarity. Thanks for your patience while I did a bit of digging on this. It reminded me of #14452

[Piinks]

Should there be a check to see if the controller has been disposed like the comment said? Or was that comment incorrect?

[bleroux]

From my understanding, the comment I removed states that _end should not be called at all but I agree that it could be read as _end will have no effect if the _controller is disposed.

Looking at the Git history this code had very few changes since its introduction (#9575).
But looking more closely it looks like at this time, DrivenScrollActivity._end was implemented this way:

  void _end() {
    delegate?.goBallistic(0.0);
  }

And the DrivenScrollActivity.dispose method was the same as now:

  @override
  void dispose() {
    _completer.complete();
    _controller.dispose();
    super.dispose();
  }

But the ActivityScroll.dispose method was:

  void dispose() {
    _delegate = null;
  }

Which means _end had no effect once dispose was called.

So it looks like a bug was introduced during the null safety migration, see #64672.
It changed ActivityScroll.dispose to:

  void dispose() { }

So to answer your question:

Should there be a check to see if the controller has been disposed like the comment said? Or was that comment incorrect?

Yes there should be a check, but AFAIK, there is no standard way to check if the animation controller was disposed so I chose to rely on the completer state.

I plan to update the PR to replace the check on completer with a check on an ‘_isDisposed’ instance variable, that will make the code more self explanatory. I will also add a narrowed test.

[bleroux]

I investigated this further:

• The comment (.whenComplete(_end); // won't trigger if we dispose _controller first) is right, it is expected that the whenComplete parameter is not called when a running animation controller is disposed.
• But in our case, the animation controller completes just before being disposed (this has to be on the exact same frame). So at the time the _end function is called the animation is disposed.
• It seems the issue surfaced because of Futures resolution order : _controller.whenComplete is executed before the new activity start but the Future it returned is executed after the start of the new activity. I tried hard to write a more narrowed test (one not relying on TabBarController) but I was not able to figure out one exposing the same Future ordering.

@Piinks I updated the PR with an _isDisposed flag and some comments.

[Piinks]

Wow! Excellent digging into the history behind this. That you for being so thorough. I think this is nearly ready, just a small tweak below. :)

[Piinks]

Looks like we have the same condition in BallisticScrollActivity.

I think having the _isDisposed flag makes sense, it's not dissimilar from the mounted check that is available elsewhere.

Can you move the isDisposed flag to the super class - ScrollActivity - so we can use it the same way in _end for BallisticScrollActivity and DrivenScrollActivity?

[bleroux]

Great!
I pushed an update where _isDisposed is moved to ScrollActivity.

[Piinks]

[auto-submit]

auto label is removed for flutter/flutter/132748, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

[Piinks]

Hmm I think a rebase would fix the issue here, but it is not enabled for me to be able to update it from here.

[auto-submit]

auto label is removed for flutter/flutter/132748, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

[Piinks]

Checking on Google testing

[Piinks]

The failure here is an instance of https://github.com/flutter/flutter/wiki/Understanding-Google-Testing#my-cl-reached-max-number-of-retry-why-and-how-do-i-resolve-this

I am going to set the check to green manually.

[auto-submit]

auto label is removed for flutter/flutter/132748, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.