Skip to content
This repository has been archived by the owner. It is now read-only.

Fix for NullPointerException via TitlePageIndicator.onTouchEvent - fakeD... #257

Open
wants to merge 2 commits into
base: master
from

Conversation

@daedamr
Copy link

commented Sep 19, 2013

...ragBy

Possible fix for: #72
This is our top crash, affecting about 40k users. What happens is that the mVelocityTracker in ViewPager is null, resulting in a NPE. mVelocityTracker is set to null in endDrag() which is called from other places other than endFakeDrag().

My fix calls every time beginFakeDrag() before fake dragging, to be sure that a fresh fake drag movement is started every time, and mVelocityTracker is not null (because from docs I understand the correct flow for doing a fake drag is beginFakeDrag() -> fakeDragBy() -> endFakeDrag()).

Per beginFakeDrag java docs:

A fake drag can be useful if you want to synchronize the motion of the ViewPager
     * with the touch scrolling of another view, while still letting the ViewPager
     * control the snapping motion and fling behavior. (e.g. parallax-scrolling tabs.)
     * Call {@link #fakeDragBy(float)} to simulate the actual drag motion. Call
     * {@link #endFakeDrag()} to complete the fake drag and fling as necessary.
@SimonVT

This comment has been minimized.

Copy link
Contributor

commented Sep 19, 2013

This is wrong, but it's probably the only way to work around it until they fix ViewPager (did you file an issue?).

@daedamr

This comment has been minimized.

Copy link
Author

commented Sep 19, 2013

Issues for this were already logged: #224 and #72.
For Android ViewPager I did not log the issue. This is pretty old and I see it mentioned only as an issue for this library.

@SimonVT

This comment has been minimized.

Copy link
Contributor

commented Sep 19, 2013

The issue is that ViewPager shouldn't be nulling out mVelocityTracker while a fake drag is in progress. An issue should be opened for this on b.android.com.

Now that I think about it, how would this commit even work? mVelocityTracker is used in ViewPager#endFakeDrag as well, it would still crash if it's null.

@daedamr

This comment has been minimized.

Copy link
Author

commented Sep 20, 2013

Yes, you are right, NPE would have still reproduced. Removed the endFakeDrag. So the only workaround here is to always call beginFakeDrag I think...
The NPE on endFakeDrag (called in TitlePageIndicator.java:615) would still happen though...and I cannot think at a workaround for that yet.
Android bug logged: https://code.google.com/p/android/issues/detail?id=60271&thanks=60271&ts=1379674446

@SimonVT

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2013

beginFakeDrag does nothing if isFakeDragging returns true, that wouldn't fix the issue either.

@daedamr

This comment has been minimized.

Copy link
Author

commented Sep 20, 2013

beginFakeDrag does nothing if mIsBeingDragged is true, and mIsBeingDragged and isFakeDragging are not in sync, not from what I see. Are you sure?

Our problems I think starts because endDrag() (where mIsBeingDragged is made false and mVelocityTracker is made null) is called somehow while a fakedrag is in process, so this may be right...

http://grepcode.com/file/repository.grepcode.com/java/ext/com.google.android/android/4.0.1_r1/android/support/v4/view/ViewPager.java

This is just a blind fix as I investigated based on crash logs and could not reproduce the problem. Other workaround than calling beginFakeDrag before fakeDragBy I cannot think of yet (this assuring mVelocityTracker is not null). I agree this will only hide the problem, and not fixed it. Fixing the android https://code.google.com/p/android/issues/detail?id=60271&thanks=60271&ts=1379674446 bug is the best solution.

@KennyGoers

This comment has been minimized.

Copy link

commented Sep 20, 2013

Any further opinions on this? I'm having the same problem with CirclePageIndicator, and yes, will it seems it is a problem with the ViewPager I don't think I'm likely to get a hot fix from the Android group on this.

@Macarse

This comment has been minimized.

Copy link

commented Sep 20, 2013

+1.

I am also interested in this fix.

@ice6

This comment has been minimized.

Copy link

commented Sep 23, 2013

+1.

It seems that it is android's bug

@KennyGoers

This comment has been minimized.

Copy link

commented on 26d8f4c Nov 13, 2013

A bit of FYI, I put in this code as a preventative fix in both the TitlePageIndicator and the CirclePageIndicator and it has no effect on the crashes as reported to me by Crashlytics.

@bubbleguuum

This comment has been minimized.

Copy link

commented Nov 21, 2013

I can reproduce this crash easily in my own app.

It happens when touching (a tap for example) the ViewPager while a fake drag is in process.
To reprod, call repeated fakeDragBy() for say 2s at regular interval in the main thread, and tap the ViewPager between these 2 seconds.
It will instacrash at the line mentioned in the first post.

It is due to mVelocityTracker being set to null a the start of onInterceptTouchEvent():

  if (action == MotionEvent.ACTION_CANCEL || action == MotionEvent.ACTION_UP) {
            // Release the drag.
            if (DEBUG) Log.v(TAG, "Intercept done!");

            mIsBeingDragged = false;
            mIsUnableToDrag = false;
            mActivePointerId = INVALID_POINTER;
            if (mVelocityTracker != null) {
                mVelocityTracker.recycle();
                mVelocityTracker = null;
            }
            return false;
        }

the code above nulling mVelocityTracker is exactly the same than in endDrag().

So it can be rewritten as:

  if (action == MotionEvent.ACTION_CANCEL || action == MotionEvent.ACTION_UP) {
            // Release the drag.
            if (DEBUG) Log.v(TAG, "Intercept done!");

            mActivePointerId = INVALID_POINTER;
            endDrag();
            return false;
        }

with endDrag() modified to do nothing when fake dragging is enabled:

 private void endDrag() {

        if(isFakeDragging()) {
            //endFakeDrag();   uncomment to cancel current fake drag on touch
            return ;
        }

        mIsBeingDragged = false;
        mIsUnableToDrag = false;

        if (mVelocityTracker != null) {
            mVelocityTracker.recycle();
            mVelocityTracker = null;
        }
    }

NOTE: you can choose optionally to uncomment the endFakeDrag() call if you want a touch event to end a fake dragging in process. But if you do that, you must check for isFakeDragging() before calling fakeDragBy() and endFakeDrag() in the app code!

Finally, modify endFakeDrag() so endDrag() executes correctly when it is called:

 public void endFakeDrag() {
        ...
        mFakeDragging = false; // make sure endDrag() runs normally
        endDrag();
    }

I've tested this fix and it works. It is possible there is a better or different solution but the code of ViewPager is not exactly a piece of cake =).
Addtionnaly this fix work with 2 other calls to endDrag(), which I don't know if they possibly can get executed during fake dragging.

@bubbleguuum

This comment has been minimized.

Copy link

commented Nov 21, 2013

And even simplier fix:

public class FixedViewPager extends ViewPager {

    public FixedViewPager(Context context) {
        super(context);
    }

    public FixedViewPager(Context context, AttributeSet attrs) {
        super(context, attrs);
    }


    @Override
        public boolean onInterceptTouchEvent(MotionEvent ev) {

        // prevent NPE if fake dragging and touching ViewPager
        if(isFakeDragging()) return false;  

        return super.onInterceptTouchEvent(ev);

    }
}
bmaslakov added a commit to MatthieuLJ/ViewPagerParallax that referenced this pull request Nov 30, 2013
@KennyGoers

This comment has been minimized.

Copy link

commented Dec 11, 2013

How is this working out for people? I'm going to give it a go as a preventative, will report back as this is a common occurrence in my app.

vykintas pushed a commit to vinted/Android-ViewPagerIndicator that referenced this pull request Dec 29, 2014
@kiruwka

This comment has been minimized.

Copy link

commented Feb 3, 2015

@KennyGoers did you have any luck with the suggested workaround ? Thanks.

@gaop

This comment has been minimized.

Copy link

commented Mar 11, 2015

change the code:

if (mViewPager.isFakeDragging() || mViewPager.beginFakeDrag()) {
mViewPager.fakeDragBy(deltaX);
}

to below:

if (mViewPager.beginFakeDrag() || mViewPager.isFakeDragging()) {
mViewPager.fakeDragBy(deltaX);
}

@joeykrim

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2015

Using the directions given by @bubbleguuum, I'm able to reliable trigger the crash at com.viewpagerindicator.TitlePageIndicator.onTouchEvent (TitlePageIndicator.java:573).
"It happens when touching (a tap for example) the ViewPager while a fake drag is in process.
To reprod, call repeated fakeDragBy() for say 2s at regular interval in the main thread, and tap the ViewPager between these 2 seconds."

Using the quick fix from @gaop and @vykintas (vinted@86eb1ee), swapping the order of isFakeDragging() and beginFakeDrag(), the crash goes away. Although the patched behavior isn't 100% as expected, this bug appears to be a fringe case in my situation with an unclear expected behavior, and the solution from @gaop works reliable!

Thanks everybody for the great details!

@tasomaniac

This comment has been minimized.

Copy link

commented Sep 8, 2015

@joeykrim swapping the order doesn't make sense because when you call beginFakeDrag() it calls mFakeDragging = true and isFakeDragging() will be always true.

@yhercouet

This comment has been minimized.

Copy link

commented Jul 4, 2016

Any update on this? I am also getting the crashes.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
You can’t perform that action at this time.