_lastDuration is not a good way to keep track an on going animation. Maybe use _onGoingAnimationDuration, and the property should be null when there is not animation on going. to prevent the previous animation accidentally leak to the next one

Sounds good. Will have a look at this.

The gif that shows interruption of the animateTo looks worse than i expected. Both the page and tab bar have a big screw. I think we should probably make it uninterruptible.

Ok, the only thing that changed in this PR was to make TabBar get interrupted. TabBarView was always getting interrupted because Scrollable widget inside TabBarView was cancelling the animation.
If we should make them all uninterruptible, I guess I should do something somewhere in Scrollable widget. I'll have a look at it.

I presume all the changes regarding to _originalChildrenWithKey is due to the drag interruption?

Yep, that's right.

I remembered _originalChildrenWithKey is still needed while we don't support drag interruption. For instance, we're moving from 3rd tab to 1st tab with 10 seconds of duration, and if we click the 3rd tab, it still interrupts the animation. So we need to keep the original children in global.

what would happen prior to this pr?

Say we're moving from 3rd tab to 1st tab with 10 seconds. That means _childenWithKey are mingled in _warpToCurrentIndex(). While the animation is running, if we click the 3rd tab, we enter _warpToCurrentIndex() again. If we don't use this global original children, what would happen is we mingle up the already mingled up _childrenWithKey, so when all the animations finish, _childrenWithKey settles in non-original order.

The thing is, if we click the tab, TabBar always gets interrupted. If we keep the condition in _handleTabControllerAnimationTick the same, only TabBarView will not get interrupted and it will look like below:

However by changing the condition, we make both of them move to the same destination:

ah I see, so the tabbarview will not get interrupt while tabbar will. what if we make the tabbar uninterruptible? (by do a condition check in _handleTap)

I can surely do that, but then user will have no way to interrupt the animation if it's taking long. Would that be okay?

I believe that is the original intention but we only do it partially. I am open to make it interruptible, however, I can't think of a good way to make the animation to look ok(no weird jump or sudden content changes).

I also think there's no way to make the interrupted animation look ok as of now. Then I think it'd be better we make TabBar click also uninterruptible.

what would happen prior to this pr?

is this still needed?

Yes, because while _warpToCurrentIndex() is running and user clicks another tab, we enter _handleTabControllerAnimationTick() and we need to allow it to go all the way to _warpToCurrentIndex() again. If not, TabBarView animation doesn't stop and doesn't reflect the new index. This is the case when _controller.index and _currentIndex are different.

This wasn't needed before because all the animations had same duration, so even without the condition I added, prior animation would finish before the new animation finishes, which is triggered by tab click, so it eventually reached _warpToCurrentIndex() again.

Is it possible that the tab controller has already been animate for a period of time?

Yes, if we call tabController.animateTo(1, duration: Duration(seconds: 10)) and after 1 second we call tabController.animateTo(3) it will come in here again while tabcontroller has been animating

In that case i think we are fine, I more worry about that we are relying on tabController to notify this method as soon as it starts animating. If not, we are risking of animation may not be in sync because we use the duration instead of the remaining time of the animation in tab controller

why this change?

This changed was needed when we allowed user drag interruption during the tab controller animation. Since we now don't allow that to happen, we can revert it back to floor(). However in a similar logic 5 lines below this we use round(), and changing floor() to round() doesn't seem to have any side effect.

line 1344 definitely has to be round() because we want to achieve the snapping effect. I think this is ok.

line 1344 definitely has to be round() because we want to achieve the snapping effect. I think this is ok.

Instead of exposing this, which may only be useful when the animation first started (if the animation is already on the flight for a while, knowing the duration alone is not enough)

I am thinking maybe the tabcontroller should expose a new api to register a new callback, something like onAnimationStarted. The callback will take in the duration, curve and index. In the tabview, it will register a callback to also start animation on _pageController

Surely considerable. I was also not very fond of exposing ongoingAnimationDuration and ongoingAnimationCurve just for this purpose :( I'll try that out and see how it looks.
Perhaps I may also add onAnimationEnd along with onAnimationStart?

I mean doing the check here

if (_index == value)
   return;

We were already doing it here! :)

I remember last time i check ScrollEndNotification is called controller.index = ? with the same value as controller.index. If we have the check on line 216, it should not have enter this block. Then this check is not necessary.

Same as above comment, we already had that check but still went all the way to notifying AnimationStartedListener, so we need this check.

In that case we still need to notify the listener, if we think like a tabcontroller, there is no reason to block this because someone does ask us to change our index.

Either we fixes the tabview to not trigger this weird case or ignore this notify.

Ok, if we think AnimationStartedListener as IndexChangeStartedListener, I guess notifying the listener does make sense.

is this true? if you set the index directly, the duration should be null which should cause a warping effect on the tabbarview

we should also mention the duration maybe null, and what does it mean to be null

Oh, the way I implemented it was to pass in kTabScrollDuration and Curves.ease to AnimationStartedListener when we're just setting the index:

So the duration and curve given to AnimationStartedListener are effectively NonNull. Would it be better we pass in null instead?

I think we need to put null to correctly reflect what is actually happening inside tabcontroller. it is up to the observers to decide how to react to null duration(for example giving it kTabScrollDuration)

Since now the animation maybe called with null duration which is not really an animation, maybe we should find a better name here....

How about IndexChangeStartedListener? I wonder if there's any naming standard in Flutter 🤔

let's make it takes in a destination index as well.

Ah I didn't notice there is also a scroll controller in tab bar, but it seems like we can just use the _controller.removeAnimationStartedListener(_handleTabControllerAnimationStarted); alone will be enough?
Something like..

_handleTabControllerAnimationStarted(int destinationIndex, Duration duration, Curve curve) {
   if (duration == null)
      duration = kTabScrollDuration;
   _scrollToIndex(destinationIndex, duration, curve);
}

void _scrollToIndex(int destinationIndex, Duration duration, Curve curve) {
    final double offset = _tabCenteredScrollOffset(destinationIndex);
    _scrollController.animateTo(offset, duration: duration, curve: curve);
  }

I think this should be enough to keep tabcontroller and scroll controller in sync so we don't need _handleTabControllerTick and _handleTabControllerAnimationTick.
Let me know if i miss something.

First, we need _handleTabControllerAnimationTick() because tabbar scroll should keep in sync when user is dragging TabBarView. In that case, none of _handleTabControllerTick() or _handleTabControllerAnimationStarted() is called, so we need to do it in _handleTabControllerAnimationTick().

Second, we need _handleTabControllerTick() because TabController's _notifyAnimationStartedListeners() is not called when user drags and ballistic scroll ends. However, notifyListeners() is called, and we have to update TabBar's _currentIndex() when ballistic scroll ends, so we should do it in _handleTabControllerTick().

I forgot about dragging yes, I agree _handleTabControllerAnimationTick is needed.

The _handleTabControllerTick though probably not needed if _notifyAnimationStartedListeners is called right? Although we need to figure out why the ballistic drag update does not drag the tabcontroller all the way to the index. Either case, _handleTabControllerAnimationStarted or _handleTabControllerAnimationTick should bring the scroll controller in tab bar to the right place?

Yep, we wouldn't need _handleTabControllerTick if we call _notifyAnimationStartedListeners. I'll change this too.

we may need to take care of where duration is null

Yep, currently the duration and curve given are effectively NonNull because I passed in default values when not present. If we decide to pass in null instead, I'll take care of it here :)

Since we add the destination Index, let's use it here

The duration and curve may be null, which indicates the index is updated without any animation.

Done!

why do we need this additional pump?

This was needed because of the changed condition in _handleTabControllerIndexChangeStarted. Previously, it always returned when _warpUnderwayCount > 0. But now we allow it to be interrupted when current warping index and new target index is different, so when _controller.index = _pageController.page.round() is called in ScrollEndNotification, _warpToCurrentIndex was called and it ran a meaningless _pageController.animateToPage once. This animation gets cancelled right away, so it doesn't do anything to UI but it kinda "dragged" a frame.

This is a bug because we actually don't need to run _warpToCurrentIndex in this case. So I changed the order of setting _controller.index and _currentIndex here so that it wouldn't call unnecessary warping again.

Is this needed?

Oh I thought tester.pumpAndSettle() is needed to really end the test by finishing all the animations going on. Isn't this needed?