NF: simplify catchingLifecycleScope

[Arthur-Milchior]

My understanding is that this method will often be called either on fragments,
and using the fragment's activity. So it seems to make sense to simplify it,
ensuring that we avoid a parameter that will probably never change.

Actually, since activities are also LifecycleOwner, it may be possible to ensure
this method is even simpler, by defining it on activities instead of defining it
on LifecycleOwner. Unless we have a reason to use an activity distinct from the
LifecycleOwner.

Also cleaning from function I viewed at the same time

[dae]

Currently there are two parallel usages of coroutines - @divyansh-dxn has been exploring them to migrate the CollectionTasks, and I have been exploring them to make the backend integration easier. We thus currently have two different CoroutineHelpers.kt, and some overlapping functionality that should eventually be merged. I took a similar approach of adding this to an activity in my file as well, but maybe a fragment would be better (I don't know enough about Android's UI to properly understand the difference):

https://github.com/ankitects/Anki-Android/blob/0d2e46feb58c5bdbf5ed5254cc673db39c0043fa/AnkiDroid/src/main/java/com/ichi2/anki/CoroutineHelpers.kt#L39

Where we also differ is that my version applies special handling to some standard classes of errors, instead of relying on a message needing to be passed in. The backend code typically returns translated error messages for us already for example.

[thedroiddiv]

I agree with the changes. Since at the time of adding it dae's changes were also unmerged I had to create a new file in parallel. Now the code can be moved to a single file. I would be good to create the Fragment.catchingLifecycleScope on top of dae's implementation and remove the redundant part.

[thedroiddiv]

fun FragmentActivity.launchCatchingTask(
    errorMessage: String? = null,
    block: suspend CoroutineScope.() -> Unit
): Job {
    return lifecycle.coroutineScope.launch {
        try {
            block()
        } catch (exc: BackendInterruptedException) {
            Timber.e("caught: %s", exc, errorMessage)
            showSimpleSnackbar(this@launchCatchingTask, exc.localizedMessage, false)
        } catch (exc: BackendException) {
            Timber.e("caught: %s", exc, errorMessage)
            showError(this@launchCatchingTask, exc.localizedMessage!!)
        } catch (exc: Exception) {
            Timber.e("caught: %s", exc, errorMessage)
            showError(this@launchCatchingTask, exc.toString())
        }
    }
}

fun Fragment.launchCatchingTask(
    errorMessage: String? = null,
    block: suspend CoroutineScope.() -> Unit
) : Job = requireActivity().launchCatchingTask(errorMessage,block)

I think this could be better in com.ichi2.anki.CoroutineHelpers.kt. Adding extension on FragmentActivity, so can be picked up by Fragment.requireActivity. And AnkiActivity extends ComponentActivity extends FragmentActivity so no issues.

[Arthur-Milchior]

Done.
I don't know why anki would be more relevant than async as a package, but anki is already such a mess that I really don't care. And I agree that a single file makes perfect sense

[dae]

Was the a specific reason this code differs from the one I linked above? It loses the i18n messages from the backend, shows "an error occurred" when the user interrupts an operation instead of just "Interrupted", and doesn't wait for the user to confirm when an error has occurred, making it hard for them to screenshot it or copy it into a report. Why not combine the fragment and error message change with the existing function?

[Arthur-Milchior]

Right now, my goal was to simplify code and add an helper method. Then my second goal was to limit the number of files.
If @divyansh-dxn you want to delete your method and use only the other one, I'm fine with it. I'm not clear enough with coroutine to know how much complexity migrating to a method with less arguments would be.

[mikehardy]

Not sure what the intent is here, but it definitely took on conflicts

[ShridharGoel]

I think this should have the error message instead of requireActivity().

[Arthur-Milchior]

It was already using requireActivity
The goal of this PR was to simplify code, introduction useful utils for future code, and not do any actual change to behavior of the app.
So, even if you're right (I don't know, I have little context here), I don't believe it belongs to this PR

[david-allison]

Can we define Activity.catchingLifecycleScope so we can remove the requireActivity() call here

[Arthur-Milchior]

Done

By the way, why do we need to put taskHandler value outside of the catchingLifecycleScope if anyone know? Hopefully maybe @divyansh-dxn