fix(fragment-stack-activity): reverts regression introduced in 1.2.10 causing major instabilities

[LucasGGamerM]

This fixes my previous issues with what was happening in Moshidon

[grishka]

I didn't do that change just for fun. It was a fix for a bug with state restoration after the process is killed and restarted. The problem with using View.generateViewId() for fragment containers is that internally it uses a counter in a static field. This obviously gets reset when the process is restarted. Then, if you're lucky, you eventually get multiple fragment containers with same IDs and mysterious bugs start happening. So obviously I fixed that by using my own counter that gets saved as part of the saved state.

Did you sync your sources with Mastodon upstream?

[LucasGGamerM]

Yes. But the fix this PR reverts broke all fragment animations, and caused a bunch of crashes with unknown causes. I merged all the commits that were made before now, but none fixed the huge problem that this commit was causing everywhere.

[grishka]

broke all fragment animations

That's new for me. I didn't see any broken animations.

caused a bunch of crashes with unknown causes

I said it at least 2 times already — the cause is known. Removing views from a RecyclerView and then returning them wrapped into new ViewHolders breaks stuff. It's obvious in hindsight. I fixed all 3 fragments where I was doing it. It no longer crashes with this particular exception. On Google Play, the crash numbers seem to have returned to the usual low.

Do you use View.generateViewId() anywhere in the code you added?

[LucasGGamerM]

No. I only replaced every call of generateViewId to View.generateViewId

[sk22]

i've neither looked into why, nor have i tried building mastodon-android from master & with appkit 1.2.13, but in my fork, using the latest appkit version, i can't navigate properly: navigating into thread fragments appears to sometimes work, but sometimes i just get a black screen. i can't navigate to hashtags at all - when i first click a hashtag (from the trending hashtags list), i get a black screen. i can go back, but once i click a hashtag again, the app crashes (from the top of my head, i think the exception said something about not being able to call "shouldIgnore…" on a null pointer. let me know if you need the crash lol) not sure if this is appkit's fault or whether megalodon messes something up. downgrading to 1.2.9 fixes these issues (though the "black screen" issue still appears from time to time, but i'm not sure if that's at all related)

[grishka]

downgrading to 1.2.9 fixes these issues (though the "black screen" issue still appears from time to time, but i'm not sure if that's at all related)

That was the fix for the black screen that would happen sometime after the app process is restarted and activity state is restored.

i can't navigate to hashtags at all - when i first click a hashtag (from the trending hashtags list), i get a black screen. i can go back, but once i click a hashtag again, the app crashes (from the top of my head, i think the exception said something about not being able to call "shouldIgnore…" on a null pointer. let me know if you need the crash lol)

I fixed that one in the last several commits. If you want to upgrade to appkit 1.2.13, you should also sync with mastodon-android. Especially commits 283b56be5b5bc4bedfcb87156a26ff5a99fe6c5a and a00afd5d7fc8ff864908ee50afd531200227b2d5. Also if you added your own tabbed screens, and copied that horrible view pager adapter, you should also change it the same way.

I'm honestly surprised it only started crashing now. Sorry for being dumb and not realizing how that code could break RecyclerView in subtle ways.

[LucasGGamerM]

Recycler views are complicated. But don't worry, we got you :D

[grishka]

And fragments. And state restoration. And especially restoring the state of fragments containing recycler views containing child fragments, lol.

[sk22]

If you want to upgrade to appkit 1.2.13, you should also sync with mastodon-android

i did,

Especially commits 283b56be5b5bc4bedfcb87156a26ff5a99fe6c5a and a00afd5d7fc8ff864908ee50afd531200227b2d5. Also if you added your own tabbed screens, and copied that horrible view pager adapter, you should also change it the same way.

..but i probably didn't apply those fixes to my own code. thanks a lot for the hint!

[sk22]

okay, update; the hashtag-related crash does also occur on upstream mastodon-android, but i can only reproduce it like this:

• open a trending hashtag
• switch accounts
• try to open a trending hashtag
• (boom)

similarly, opening posts appears to result in black screens after switching accounts

[grishka]

Ok, now I can reproduce it. Feels like this bug is related to clearing the back stack.

[grishka]

You don't even need the first step. Just switch accounts and shit breaks

[LucasGGamerM]

Shit's complicated. At least you can now reproduce all the funky behavior that was happening

[grishka]

This fixed that crash. I still don't know exactly why doing

activity.finish();
activity.startActivity(new Intent(activity, MainActivity.class));

was breaking stuff. I saw in debugger that new activity was created, too. So it worked correctly on the first glance.

Oddly enough, changing the theme from settings, which does activity.recreate(), doesn't have the same effect. Neither does the "don't keep activities" setting. So state restoration does work correctly. I have no idea why and how activities could affect each other like that. I've been developing for Android for over 10 years but it's the first time I'm observing this behavior. Maybe something to do with intent flags and/or launch mode?

[grishka]

@sk22 could you please confirm that my fix works for you?

[sk22]

this seems to have fixed it (in mastodon-android - i think i'll have to do some more work for my fork to make the new appkit versions not break everything)

[grishka]

I published an update, 2.1.5. We'll soon see how much better it is.

[grishka]

I added this exception to debug the one remaining condition where a view might end up returned to a RecyclerView more than once: https://github.com/grishka/appkit/commit/352a548b0a3fdb2770229fb0feb4b32de6af4c3e

Not on maven central yet, just for my own testing.

[LucasGGamerM]

Its a major improvement over the previous commits, but my app still crashes. I am looking at what I can do about it

[grishka]

Can you try with my last commit? I reverted the view type assignment thing in SingleViewRecyclerAdapter back to how it was, and added an exception to make sure my assumptions are correct.

[LucasGGamerM]

I tried it, and the major animation bugs are gone, but there is still a problem with opening the search, where the app still crashes with the same crash log as I had previously sent

[grishka]

New idea after poking around in debugger for 2 hours: something adds a view to ViewPager2's RecyclerView in the profile fragment from outside of RV's normal mechanisms. Since I don't see any calls to addView(), it might be the fragment manager?

[grishka]

Now we're getting somewhere. Why the hell does that RecyclerView have that id?

[grishka]

So it goes like this:

1. FragmentStackActivity wants to show a fragment, it creates a container view and assigns it ID 3
2. FragmentManager looks for a container to add the fragment into, but instead finds that RecyclerView inside ViewPager2 that for some reason has the same ID
3. It adds the SearchQueryFragment in there instead
4. RecyclerView doesn't understand wtf just happened and the app crashes

[grishka]

Oh well. One question: WHY?! Apparently it does this for state saving/restoration, but is there really no better way?

I have an idea: since the view tree is usually traversed in the order of view indexes increasing, including inside findViewById(), why not add fragment containers backwards, so that the right one will always be found first? The behavior is even documented:

Finds the first descendant view with the given ID, the view itself if the ID matches [getId()](https://developer.android.com/reference/android/view/View#getId()), or null if the ID is invalid (< 0) or there is no matching view in the hierarchy.

[grishka]

@sk22 @LucasGGamerM please confirm that this commit fixes everything for good for you

[LucasGGamerM]

I just tested, and everything seems to be working normally. Thank you!

[grishka]

Those changes to view pager adapters in tabbed fragments, by the way, ended up being largely unnecessary but they might still help in some other edge cases. In fact, none of the adapter methods were even called before the crash, and that's what got me on the right track. The only legitimate way a child view can be added to a RecyclerView is through an adapter, but if it somehow ends up containing a view without the adapter ever being involved... I assumed that calling RecyclerView::addView directly would throw an exception because it's clearly a very invalid operation, they couldn't have possibly overlooked that, right? Right?!?!