Skip to content

Commit

Permalink
at least with O, we no longer can rely on superstate being empty. Imp…
Browse files Browse the repository at this point in the history
…lement proper parceling of both super and wrapped state
  • Loading branch information
mtotschnig committed Jun 9, 2017
1 parent 4182d8f commit c9f9a44
Showing 1 changed file with 42 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import android.graphics.Canvas;
import android.graphics.drawable.Drawable;
import android.os.Build;
import android.os.Parcel;
import android.os.Parcelable;
import android.util.AttributeSet;
import android.util.Log;
Expand Down Expand Up @@ -1121,17 +1122,14 @@ public void setMultiChoiceModeListener(MultiChoiceModeListener listener) {

@Override
public Parcelable onSaveInstanceState() {
Parcelable superState = super.onSaveInstanceState();
if (superState != BaseSavedState.EMPTY_STATE) {
throw new IllegalStateException("Handling non empty state of parent class is not implemented");
}
return mList.onSaveInstanceState();
return new SavedState(super.onSaveInstanceState(), mList.onSaveInstanceState());
}

@Override
public void onRestoreInstanceState(Parcelable state) {
super.onRestoreInstanceState(BaseSavedState.EMPTY_STATE);
mList.onRestoreInstanceState(state);
SavedState ss = (SavedState) state;
super.onRestoreInstanceState(ss.getSuperState());
mList.onRestoreInstanceState(ss.wrappedState);
}

@TargetApi(Build.VERSION_CODES.ICE_CREAM_SANDWICH)
Expand All @@ -1155,4 +1153,41 @@ public void setStackFromBottom(boolean stackFromBottom) {
public boolean isStackFromBottom() {
return mList.isStackFromBottom();
}

static class SavedState extends BaseSavedState {
private Parcelable wrappedState;

/**
* Constructor called from {@link StickyListHeadersListView#onSaveInstanceState()}
*/
SavedState(Parcelable superState, Parcelable wrappedState) {
super(superState);
this.wrappedState = wrappedState;
}

/**
* Constructor called from {@link #CREATOR}
*/
private SavedState(Parcel in) {
super(in);
wrappedState = in.readParcelable(null);
}

@Override
public void writeToParcel(Parcel out, int flags) {
super.writeToParcel(out, flags);
out.writeValue(wrappedState);
}

public static final Parcelable.Creator<SavedState> CREATOR
= new Parcelable.Creator<SavedState>() {
public SavedState createFromParcel(Parcel in) {
return new SavedState(in);
}

public SavedState[] newArray(int size) {
return new SavedState[size];
}
};
}
}

38 comments on commit c9f9a44

@Maxr1998
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I finally got to test this fix on Android O in my own build, and I can confirm it works perfectly. Thanks a lot!

@nvquockhtn
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fix still causes crash in some devices

Caused by android.os.BadParcelableException: ClassNotFoundException when unmarshalling: %an
at android.os.Parcel.readParcelableCreator(Parcel.java:2105)
at android.os.Parcel.readParcelable(Parcel.java:2055)
at se.emilsjolander.stickylistheaders.StickyListHeadersListView$SavedState.(StickyListHeadersListView.java:1173)
at se.emilsjolander.stickylistheaders.StickyListHeadersListView$SavedState.(StickyListHeadersListView.java:1157)
at se.emilsjolander.stickylistheaders.StickyListHeadersListView$SavedState$1.createFromParcel(StickyListHeadersListView.java:1185)
at se.emilsjolander.stickylistheaders.StickyListHeadersListView$SavedState$1.createFromParcel(StickyListHeadersListView.java:1183)
at android.os.Parcel.readParcelable(Parcel.java:2062)
at android.os.Parcel.readValue(Parcel.java:1971)
at android.os.Parcel.readSparseArrayInternal(Parcel.java:2284)
at android.os.Parcel.readSparseArray(Parcel.java:1693)
at android.os.Parcel.readValue(Parcel.java:2028)
at android.os.Parcel.readMapInternal(Parcel.java:2255)
at android.os.Bundle.unparcel(Bundle.java:223)
at android.os.Bundle.getSparseParcelableArray(Bundle.java:1237)
at android.support.v4.app.FragmentManagerImpl.moveToState(FragmentManager.java:1208)
at android.support.v4.app.FragmentManagerImpl.moveFragmentToExpectedState(FragmentManager.java:1528)
at android.support.v4.app.FragmentManagerImpl.moveToState(FragmentManager.java:1595)
at android.support.v4.app.FragmentManagerImpl.dispatchCreate(FragmentManager.java:2893)
at android.support.v4.app.FragmentController.dispatchCreate(FragmentController.java:190)
at android.support.v4.app.FragmentActivity.onCreate(FragmentActivity.java:353)
at android.support.v7.app.AppCompatActivity.onCreate(AppCompatActivity.java:85)

@NLLAPPS
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get it too. Can it be related to proguard?

@sandy-8925
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I believe it's due to "wrappedState = in.readParcelable(null)" in the SavedState constructor. Even Lint flags it in Android Studio.

@Cylix
Copy link

@Cylix Cylix commented on c9f9a44 Jun 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, seems it is coming from there, Android Studio tells to use wrappedState = in.readParcelable(getClass().getClassLoader()); instead

@mtotschnig
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks for the reports. I tried to follow the approach used by RecyclerView making use of ParcelableCompat, which now introduces a dependency on the support library (55096d3). I hope this is correct. Unfortunately I cannot test it, since I have not yet been able to observe the problem. Anybody able to provide a sample app with a concrete scenario that causes the crash?

@NLLAPPS
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has it been pushed to repo or do we need to use it from the source? I cannot recreate the crash at the moment.

@mtotschnig
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NLLAPPS As mentioned on emilsjolander#477, it is available from Jitpack. If you already, included it from there, you might have to call gradle with --refresh-dependencies, in order to pick up the latest build from there.

@NLLAPPS
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll push a beta soon. I should start getting some reports if there is a crash/issue.
I've noticed you are using support-v4:22.0.0 but mine is 26.0.0-beta2. I am not familiar with this but how would having 2 different versions impact the app? Would gradle include both?

@Macarse
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mtotschnig why don't you start versioning your fork?

@mtotschnig
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NLLAPPS Since StickylListHeaders is still supposed to work on API 7, it needs to include the older version of the support library, since newer ones have raised the bar. This should be handled gracefully by Gradle.

@mtotschnig
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Macarse I probably will have to do that, although I'd hope that somebody else would take over from @emilsjolander since my fork contains other customizations.

@Macarse
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mtotschnig oh, good to know, I thought you forked just with the O fix.

Unfortunately, the best thing to do with StickyListHeaders is fork, fix the O bug and migrate out of it asap.

@mtotschnig
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just ran into the issue, and unfortunately could observe that also my with latest trial (55096d3) problem is not solved. Will investigate further.

@mtotschnig
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally, I think I got it right: 7ef4793

@Cylix
Copy link

@Cylix Cylix commented on c9f9a44 Jun 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work, really great!

@mtotschnig
Copy link
Owner Author

@mtotschnig mtotschnig commented on c9f9a44 Jun 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have pushed a tag and documented difference with upstream: https://github.com/mtotschnig/StickyListHeaders/releases/tag/2.7.1

Use it with
compile 'com.github.mtotschnig:StickyListHeaders:2.7.1'

@NLLAPPS
Copy link

@NLLAPPS NLLAPPS commented on c9f9a44 Jul 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There might be an issue with this. I cannot reproduce it but I have stack traces form users.
I don't even know if it is something to do with my implementation but the issue started to occur after adding it

Have a look at the below log

java.lang.NullPointerException: Attempt to invoke virtual method 'void CircleImageView.setImageResource(int)' on a null object reference at ckv.a(RecordingListAdapter.java:192) at clk.a(RecordingListFragment.java:491) at clk.a(RecordingListFragment.java:79) **at clk$17.onItemLongClick(RecordingListFragment.java:440)** at android.widget.AbsListView.showContextMenuForChildInternal(AbsListView.java:3324) at android.widget.AbsListView.showContextMenuForChild(AbsListView.java:3310) at android.view.ViewGroup.showContextMenuForChild(ViewGroup.java:791) at android.view.View.showContextMenu(View.java:5838) at android.widget.TextView.showContextMenu(TextView.java:8561) at android.view.View.performLongClickInternal(View.java:5757) at android.view.View.performLongClick(View.java:5711) at android.widget.TextView.performLongClick(TextView.java:9416) at android.view.View.performLongClick(View.java:5729) at android.view.View$CheckForLongPress.run(View.java:22518) at android.os.Handler.handleCallback(Handler.java:751) at android.os.Handler.dispatchMessage(Handler.java:95) at android.os.Looper.loop(Looper.java:154) at android.app.ActivityThread.main(ActivityThread.java:6290) at java.lang.reflect.Method.invoke(Native Method) at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:886) at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:776)

I have setOnItemLongClickListener on RecordingListFragment.java:440 which then calls RecordingListFragment.java:491 which has adapter.toggleItemSelection and passes CircleImageView to the adapter after finding with v.findViewById(R.id.list_contact_photo)

But, notice the stack trace, it jumps to RecordingListFragment.java:79 which is entry point. So it seems RecordingListFragment is somehow recreated when item in the ListView long pressed under certain circumstances

@heatherSnepenger
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mtotschnig You're a life saver. Thanks!

@davethomas11
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey! Thank you so much for fixing this crash. Does anyone know why onSaveInstanceState is even being overridden?

To me, it seems completely unnecessary that it was being done in the first place. Wouldn't the list view have its onSaveInstanceState called because it is part of the view hierarchy?

@davethomas11
Copy link

@davethomas11 davethomas11 commented on c9f9a44 Sep 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am also getting "android.os.TransactionTooLargeException; data parcel size N bytes" exceptions in my Crashlytics. I have not been able to find the source of it yet. These are hard to track because they don't have a stack trace pointing to the source code that caused the binder transaction.

Seeing that onSaveInstanceState is overwritten has me concerned that this library could be a culprit, but I doubt it. More likely something in my code.

Is anyone else seeing Binder transaction issues in their Crashlytics as well?

@mtotschnig
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@davethomas11 The list that is wrapped by StickyListHeadersListView is not itself part of the view hierarchy, hence onSaveInstanceState must be delegated to it. emilsjolander#255 documented the bug that arises when this delegation is missing.

@sandy-8925
Copy link

@sandy-8925 sandy-8925 commented on c9f9a44 Sep 13, 2017 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@davethomas11
Copy link

@davethomas11 davethomas11 commented on c9f9a44 Sep 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see addView(mList); being called on line 233? Is the view removed at some point maybe? Or is the StickyListHeadersListView not part of the hierarchy?

@davethomas11
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sandy-8925 Do you have another suggested library?

@sandy-8925
Copy link

@sandy-8925 sandy-8925 commented on c9f9a44 Sep 13, 2017 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sandy-8925
Copy link

@sandy-8925 sandy-8925 commented on c9f9a44 Sep 13, 2017 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NLLAPPS
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is another patch at edisonw@3dd88a2

@mtotschnig
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@davethomas11 comment
c9f9a44#commitcomment-24268832 got me thinking. Instead of wrapping the state of the wrapped list, simply setting and id on the wrapped list, might trigger the default view state handling.

@mtotschnig
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gave that approach a quick try, and it works! Will implement that solution on my branch eventually.

@davethomas11
Copy link

@davethomas11 davethomas11 commented on c9f9a44 Sep 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow cool! I learned something new, love it. Views with out IDs can't be saved in onSaveInstanceState because the system has no way to reference them.

Nicely done!

@davethomas11
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mtotschnig You probably know this, but make sure the ID you add is a proper Android resource and not an integer in your code. Ie.

<?xml version="1.0" encoding="utf-8"?>
<resources>
    <item
        type="id"
        name="id_name" />
</resources>

That way there is no ID collision. Then the aapt tool can generate an appropriate ID during the build process.

@mtotschnig
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my test, I used android.R.id.list, but I thought like you did that it is necessary to define a library specific id via xml. If we were only supporting API 17+ we could use https://developer.android.com/reference/android/view/View.html#generateViewId()

@mtotschnig
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BUT: Even with one id defined in XML, there still is a problem if multiple StickyListHeaderListViews are used in one activity, which might easily be the case with a view pager, for example. So in the end, I think, it is necessary to stick with the implemented approach of wrapping the state of the wrapped list.

@davethomas11
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, I like it. Thanks for pointing that ID generating method out.

@davethomas11
Copy link

@davethomas11 davethomas11 commented on c9f9a44 Sep 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, it sounds like the safest way to do it.

https://stackoverflow.com/questions/1714297/android-view-setidint-id-programmatically-how-to-avoid-id-conflicts

Shows a way to implement for pre API 17 but it does not look safe to me starting with an id of 1. Someone may have also implemented the same method for generating ids somewhere else in their code.

( It's a few answers down has a score > 300 )

@davethomas11
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, put some thought into now that I understand the issue. I agree, stick with the current method of wrapping the saved state.

@davethomas11
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though I'd like to think that using the same ID in different fragments in a view pager is handled properly in the framework code? Would they not expect that to happen if a user were to use similar parts of a layout?

But that still does not solve the problem of multiple StickyListHeaderListViews in the same layout.

Worth investigating and understanding how ID collisions are handled during the save state process. Let you know if I find anything interesting.

Please sign in to comment.