fix: attach custom study dialog fragment factory in SinglePageActivity to avoid crash

[mikehardy]

There were two Activities that attached the custom factory for custom study dialog in onCreate, but there were three that needed it (SinglePageActivity hosting CongratsPage fragment hosting study options was missing)

If you didn't attach in onCreate then you would crash if the fragment/activity lifecycle had done a cycle on you due to background kill or don't keep activities on

Purpose / Description

Describe the problem or feature and motivation

Fixes

https://ankidroid.org/acra/app/1/bug/237277/report/ecbf20ba-e077-4952-ae40-14d56647ad99

java.lang.RuntimeException: Unable to start activity ComponentInfo{com.ichi2.anki/com.ichi2.anki.SingleFragmentActivity}: androidx.fragment.app.Fragment$InstantiationException: Unable to instantiate fragment com.ichi2.anki.dialogs.customstudy.CustomStudyDialog: could not find Fragment constructor
	at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:4164)
	at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:4322)
	at android.app.servertransaction.LaunchActivityItem.execute(LaunchActivityItem.java:103)
	at android.app.servertransaction.TransactionExecutor.executeCallbacks(TransactionExecutor.java:139)
	at android.app.servertransaction.TransactionExecutor.execute(TransactionExecutor.java:96)
	at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2685)
	at android.os.Handler.dispatchMessage(Handler.java:106)
	at android.os.Looper.loopOnce(Looper.java:230)
	at android.os.Looper.loop(Looper.java:319)
	at android.app.ActivityThread.main(ActivityThread.java:8919)
	at java.lang.reflect.Method.invoke(Native Method)
	at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:578)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1103)
Caused by: androidx.fragment.app.Fragment$InstantiationException: Unable to instantiate fragment com.ichi2.anki.dialogs.customstudy.CustomStudyDialog: could not find Fragment constructor
	at androidx.fragment.app.Fragment.instantiate(SourceFile:9)
	at androidx.fragment.app.FragmentContainer.instantiate(SourceFile:1)
	at androidx.fragment.app.FragmentManager$3.instantiate(SourceFile:18)
	at androidx.fragment.app.FragmentState.instantiate(SourceFile:3)
	at androidx.fragment.app.FragmentStateManager.<init>(SourceFile:13)
	at androidx.fragment.app.FragmentManager.restoreSaveStateInternal(SourceFile:253)
	at androidx.fragment.app.Fragment.restoreChildFragmentState(SourceFile:15)
	at androidx.fragment.app.Fragment.onCreate(SourceFile:4)
	at androidx.fragment.app.Fragment.performCreate(SourceFile:22)
	at androidx.fragment.app.FragmentStateManager.create(SourceFile:60)
	at androidx.fragment.app.FragmentStateManager.moveToExpectedState(SourceFile:133)
	at androidx.fragment.app.FragmentStore.moveToExpectedState(SourceFile:31)
	at androidx.fragment.app.FragmentManager.moveToState(SourceFile:28)
	at androidx.fragment.app.FragmentManager.dispatchStateChange(SourceFile:10)
	at androidx.fragment.app.FragmentManager.dispatchCreate(SourceFile:12)
	at androidx.fragment.app.FragmentController.dispatchCreate(SourceFile:5)
	at androidx.fragment.app.FragmentActivity.onCreate(SourceFile:13)
	at com.ichi2.anki.AnkiActivity.onCreate(SourceFile:13)
	at com.ichi2.anki.SingleFragmentActivity.onCreate(SourceFile:8)
	at android.app.Activity.performCreate(Activity.java:8975)
	at android.app.Activity.performCreate(Activity.java:8944)

Combined with this logcat showing the offender:

LOGCAT
--------- beginning of main
11-22 23:50:34.728 I/AnkiDroid(20104): Timber config: PRODUCTION
11-22 23:50:34.728 I/AnkiDroid(20104): initialize()
11-22 23:50:34.956 I/AnkiDroid(20104): Participating in analytics sample (sample percentage vs random: 10 1)
11-22 23:50:34.956 I/AnkiDroid(20104): setOptIn(): from false to true
11-22 23:50:34.956 I/AnkiDroid(20104): Not participating in analytics sample (sample percentage vs random: 10 99)
11-22 23:50:35.028 I/AnkiDroid(20104): Creating notification channel with id/name: General Notifications/AnkiDroid
11-22 23:50:35.028 I/AnkiDroid(20104): Creating notification channel with id/name: Synchronization/Synchronization
11-22 23:50:35.029 I/AnkiDroid(20104): Creating notification channel with id/name: Global Reminders/Cards due
11-22 23:50:35.029 I/AnkiDroid(20104): Creating notification channel with id/name: Deck Reminders/Reminders
11-22 23:50:35.029 I/AnkiDroid(20104): Creating notification channel with id/name: Scoped Storage/Storage migration
11-22 23:50:35.033 I/AnkiDroid(20104): AnkiDroidApp: Starting Services
11-22 23:50:35.052 I/AnkiDroid(20104): (Re)opening Database: /storage/emulated/0/Android/data/com.ichi2.anki/files/AnkiDroid/collection.anki2
11-22 23:50:35.078 I/AnkiDroid(20104): Executing Boot Service
11-22 23:50:35.089 I/AnkiDroid(20104): Setting theme to LIGHT
11-22 23:50:35.108 I/AnkiDroid(20104): SingleFragmentActivity::onCreate
11-22 23:50:35.109 I/AnkiDroid(20104): SingleFragmentActivity::CongratsPage::onAttach

Approach

Implement CustomStudyListener in SinglePageActivity as a temporary solution to fix this crash, before a better refactoring that fixes the area

How Has This Been Tested?

1- turn on developer options -> don't keep activities so we trigger lifecycle changes when app backgrounds
2- create new deck with one card, review the card, see the congrats page
3- tap the link for custom study to open the dialog
4- background the app, foreground the app
5- see crash without patch, no crash with patch

I also tested other pathways that I knew use other fragment factories like the BrowserOptions one (card browser -> actions bar menu -> options -> change stuff / do lifecycle transitions) and this did not break those, so our fragment factory delegation is working correctly

Learning (optional, can help others)

Our fragment factory here is pretty complicated but...I guess it works.

Fragment lifecycle requires that we attach that fragment factory in Activity.onCreate if there is no default constructor though, otherwise the default fragment factory will crash while trying to call the default constructor

(similar to #17488 but here it is not feasible to make a default constructor - tried that, ugly change)

Checklist

Please, go through these checks before submitting the PR.

• You have a descriptive commit message with a short title (first line, max 50 chars).
• You have commented your code, particularly in hard-to-understand areas
• You have performed a self-review of your own code
• UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
• UI Changes: You have tested your change using the Google Accessibility Scanner

[david-allison]

I don't like this; we should move to a much simpler model. For now, adding the listener to the problematic page would be cleaner

I don't support turning a 'method not implemented' into a runtime no-op which doesn't appear in ACRA

---

Simpler model

Inlining the interface, we have

        // CustomStudyListener
        fun onExtendStudyLimits()
        fun showDialogFragment(newFragment: DialogFragment)
        fun dismissAllDialogFragments()
        fun startActivity(intent: Intent)

        // CreateCustomStudySessionListener
        fun hideProgressBar()
        fun onCreateCustomStudySession()
        fun showProgressBar()

• [hide/show]ProgressBar can be replaced with launchCatchingTask
• startActivity, dismissAllDialogFragments and showDialogFragment by requireAnkiActivity()

This leaves us with:

onExtendStudyLimits() 
onCreateCustomStudySession() 

Inside the Fragment, both of these are called as a terminal action, and should be using the Fragment Result API

---

~~We're in this position because we over-abstract the 'rebuildCram' function, which is ~5 lines of code, and is NOT terminal when called from StudyOptionsFragment~~

[mikehardy]

For now, adding the listener to the problematic page would be cleaner

That's a reasonable stance, and the work here is just a couple minutes, doing it the completely-opposite-way as you suggest is also just the work of a couple minutes, and I was able to reproduce the crash so it's an easy one

Will alter style and re-push

All the other commentary is - I think - best suited to a new issue peeled off this one, based on that comment. It's a real refactor, I'm looking for minimal-surgical-intervention to keep it easy to understand + easily testable + cherry-pickable

[david-allison]

Definitely out of scope for this PR.

Apologies I didn't make that clear.

[mikehardy]

The amount of work involved made it clear 😆 - no worries, didn't spend an extra second on it
Just re-pushed with the lightest possible implementation of the listener so that the factory could be attached

Appears to pass all my manual testing, same as other style

[lukstbit]

LGTM as a quick fix.

The complete solution is to remove the need for a factory. The fragment shouldn't require the collection to be passed in(whatever access it needs should be done through CollectionManager directly) and the listener could be replaced with the FragmentResults apis.

Also we should refrain from adding functionality that is not general to SingleFragmentActivity, my understanding is that the activity was intended as a simple container for our backend pages fragments.

[mikehardy]

@lukstbit totally agreed on the smelly-ness of this temp fix, David's already got a couple PRs up that trim down the custom tag dialog factory/listener/callback/everything needs so all this will hopefully be evicted with a good refactor shortly