BUG: 'setCurrentPageIndex:animated:' no longer triggers 'onPageDidAppear'

[funnel20]

We're updating EAIntroView from version 2.7.4 to latest 2.11.0. We found a serious bug in setCurrentPageIndex:animated:.

Use case On page 1, we have placed a button NEXT. The button calls setCurrentPageIndex:animated: with animation enabled to scroll to page 2. On Page 2, we have implemented onPageDidAppear to start some animations. However, in version 2.11.0 the animations are not started.

Analysis While debugging we found that onPageDidAppear is never fired. It should be called in notifyDelegateWithPreviousPage:andCurrentPage:, which is called in checkIndexForScrollView: when the scrollView ends scrolling.

The error can be visualised in checkIndexForScrollView::

[self notifyDelegateWithPreviousPage:self.currentPageIndex andCurrentPage:newPageIndex];

In the debugger it appears that for this use case self.currentPageIndex already equals newPageIndex. Since both have the same value, the IF condition in notifyDelegateWithPreviousPage:andCurrentPage: fails, and any required (delegate) methods, such as onPageDidAppear, are not called.

Why is self.currentPageIndex already set to the new index? Because that's already done in setCurrentPageIndex:animated: before scrolling is even started:

- (void)setCurrentPageIndex:(NSUInteger)currentPageIndex animated:(BOOL)animated {
    if(![self pageForIndex:currentPageIndex]) {
        NSLog(@"Wrong currentPageIndex received: %ld",(long)currentPageIndex);
        return;
    }

    _currentPageIndex = currentPageIndex;

    [self scrollToPageForIndex:currentPageIndex animated:animated];
}

Solution Remove the following line from setCurrentPageIndex:animated:: _currentPageIndex = currentPageIndex;

Verification Animated = YES We tested the modified code while calling setCurrentPageIndex:animated: with animation enabled. Now onPageDidAppear is called properly.

Animated = NO We repeated the test. The first time onPageDidAppear was not called. A second run it was called. Huh?

More debugging showed that when animation is disabled, sometimes scrollViewDidEndScrollingAnimation: is called, but not always. Hence we enforced to always call checkIndexForScrollView: in order to call the delegate methods in scrollToPageForIndex:animated::

- (void)scrollToPageForIndex:(NSUInteger)newPageIndex animated:(BOOL)animated
{
    if(![self pageForIndex:newPageIndex]) {
        NSLog(@"Wrong newPageIndex received: %ld",(long)newPageIndex);
        return;
    }

    CGFloat offset = newPageIndex * self.scrollView.frame.size.width;
    CGRect pageRect = { .origin.x = offset, .origin.y = 0.0, .size.width = self.scrollView.frame.size.width, .size.height = self.scrollView.frame.size.height };
    [self.scrollView scrollRectToVisible:pageRect animated:animated];

    if(!animated) {
        [self scrollViewDidScroll:self.scrollView];

        // When animated, the scrollView delegates will call 'checkIndexForScrollView' that will first call the delegate methods and after it update _currentPageIndex.
        // When animation is disabled, sometimes 'scrollViewDidEndScrollingAnimation:' is called, but not always. Hence always call 'checkIndexForScrollView:' in order to call the delegate methods:
        [self checkIndexForScrollView:self.scrollView];
    }
}

Feedback I agree that the latter for animation disabled is more a work-around than a solution, as it's not clear why scrollViewDidEndScrollingAnimation: is sometimes called. Please advise if you have any suggestions.

[ealeksandrov]

Thanks for detailed investigation, I'll play with it later on this week.

[ealeksandrov]

Hmmm. It was a long dance around one line.

4f68750b9282c58c674e00d92fdf2ed475ce1b52 f1fdfd46b06d387e63ed366e169d7cbf8b914fa2 a5ce2c3336f4dfbc3cc1a02ef3ba9f8272fd9be6

https://github.com/ealeksandrov/EAIntroView/issues/67 https://github.com/ealeksandrov/EAIntroView/issues/144 https://github.com/ealeksandrov/EAIntroView/issues/174 https://github.com/ealeksandrov/EAIntroView/pull/177

Remove the following line from setCurrentPageIndex:animated:: _currentPageIndex = currentPageIndex;

This is bad for a reason mentioned in https://github.com/ealeksandrov/EAIntroView/issues/144:

This would be a rather unusual behavior for an UIKit component. Normally the initial state is effected immediately not after the animation has finished (UITabBarController.selectedIndex, UINavigationController.topViewController, ...).

The main issue is initial exposing of currentPageIndex setter while there are some actions triggered by pages scrolling. So currentPageIndex should be only observed without any intervention from outside. To change pages programmatically - scrollToPageForIndex:animated: should be used.

[ealeksandrov]

Decided to go ahead with this suggestions: https://github.com/ealeksandrov/EAIntroView/issues/174#issuecomment-233220893 & https://github.com/ealeksandrov/EAIntroView/pull/177

Fixing scrollToPageForIndex:animated: for animated:NO - fa98a3755d5e31c8f386e97da48521697f447ad8 Removing setCurrentPageIndex:animated: - 08dc45c14c7b8d8ac5fabd18053cd9c0045c084f

Will push 2.12.0 release shortly.