Don't tie the essential files dialog to the activity lifecycle

[BrayanDSO]

Another problem with starting the service right away when starting the migration.

A service means that the app will be much less likely to get killed while migrating essential files. You'd think that this very good, and it is, but this also means that deck picker will be more likely to get recreated or finished and relaunched while essential file migration is taking place, unlocking the UI.

However while it will be more of a problem in this case, it is a problem now as well. For instance, if you change device theme while essential files are being migrated, you are going to see the “Collection not opened error”. This here is a more general problem with withProgress; it is a function that shows some UI that is tied to current activity and is commonly launched in its lifecycle scope, yet often the code inside of it is not UI-related, such as running migration. That is to say, it's misused. I think we can try to fix the simple cases using a dialog fragment, but we really want to properly move to view model & friends. Also note that:

• The launchCatchingTask {} block is canceled when the activity is destroyed, potentially ruining the job that is performed
• A block that is canceled in this way but still running will leak the activity

But when using a service for it we can expect the activity to be not only recreated, but also completely finished and relaunched. In this case we are going to lose our fragment.

I suppose that ideally we will have to introduce yet another state—for migrating essential files—and deal with it like we deal with the UI for media migration, recreating the progress dialog.

Originally posted by @oakkitten in https://github.com/ankidroid/Anki-Android/issues/13868#issuecomment-1584673040

[dae]

I suspect #13969 will solve the 'collection not opened' issue.

It's an interesting point you bring up @oakkitten. iOS's concurrency implementation doesn't use lifecycles, so this is not something I'm used to thinking about. I suspect for most of our operations, a lifecycle scope is what we want, but I can see why a global scope or custom-defined scope may be better suited for a migration.

That said, at least for the essential files part, I'm not sure it actually matters that much in practice. As far as I'm aware, a coroutine can only be interrupted at a suspend point. withQueue is defined as taking a blocking block, so we should be guaranteed that the entire essential files migration runs through to completion if it starts, even if the deck picker is restarted in the middle of the operation.

I'm not sure what would happen after that though - if withQueue ran to completion but then the cancellation were processed, the media migration might not end up getting started by deckpicker.kt:migrate(). Is the migration code able to handle a case where a restart happens and essential files have been migrated but media move has not been started yet?

[oakkitten]

Is the migration code able to handle a case where a restart happens and essential files have been migrated but media move has not been started yet?

It should survive, yes. If the essential file migration runs to completion, it will set the persistent flag telling that media migration should continue. If the app dies before the flag is set, we are back to square one except that there will be some junk in the last destination folder. That's not dealt with but should happen rarely enough in practice so we can safely ignore it I guess.

But you have to be careful. If you are going to refactor the code and for instance do the sensible thing and say something along

withQueue { val newLocation = moveFiles() } // Move in a background thread
updateCollectionPath(newLocation) // Change config in main thread

Then the job may complete but the config may fail to update because

Note that the result of withContext invocation is dispatched into the original context in a cancellable way with a prompt cancellation guarantee, which means that if the original coroutineContext in which withContext was invoked is cancelled by the time its dispatcher starts to execute the code, it discards the result of withContext and throws CancellationException.

As long as you are aware of of how coroutines work seeing code like above might be not a big deal, but for the sake of easier reasoning I would drop launchCatchingTask in favor of a method that's executed directly on a scope, like launch, such as CoroutineScope.launchCatching maybe, or perhaps separate launching and catching, maybe use withContext(NonCancellable) { } to be even more explicit here, etc

[dae]

and for instance do the sensible thing

I'd argue that's not a sensible change - preferences are thread-safe from what I gather, and if the preferences update is done outside of the queue, you introduce a race condition where some concurrent caller could end up reopening the collection at the old location, leading to data loss.

I would drop launchCatchingTask in favor of a method that's executed directly on a scope

Are you advocating that we change the 125-and-growing calls to launchCatchingTask to instead be lifecycle.coroutineScope.launchCatching? That's somewhat more cumbersome, and I'm not sure the extra clarity is worth it for the majority of use cases.

maybe use withContext(NonCancellable) { }

I'm not familiar with its use - if the following is what you're proposing and would not have side effects, I don't see the harm in adding it:

diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt b/AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt
index ed2c912fdc..d92abebf4c 100644
--- a/AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt
+++ b/AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt
@@ -2582,19 +2582,21 @@ open class DeckPicker :
         }

         launchCatchingTask {
-            val elapsedMillisDuringEssentialFilesMigration = measureTimeMillis {
-                withProgress(getString(R.string.start_migration_progress_message)) {
-                    loadDeckCounts?.cancel()
-                    val folders = prepareAndValidateSourceAndDestinationFolders(baseContext)
-                    CollectionManager.migrateEssentialFiles(baseContext, folders)
-                    updateDeckList()
-                    handleStartup()
-                    startMigrateUserDataService()
+            withContext(NonCancellable) {
+                val elapsedMillisDuringEssentialFilesMigration = measureTimeMillis {
+                    withProgress(getString(R.string.start_migration_progress_message)) {
+                        loadDeckCounts?.cancel()
+                        val folders = prepareAndValidateSourceAndDestinationFolders(baseContext)
+                        CollectionManager.migrateEssentialFiles(baseContext, folders)
+                        updateDeckList()
+                        handleStartup()
+                        startMigrateUserDataService()
+                    }
+                }
+                if (elapsedMillisDuringEssentialFilesMigration > 800) {
+                    showSnackbar(R.string.migration_part_1_done_resume)
+                    refreshState()
                 }
-            }
-            if (elapsedMillisDuringEssentialFilesMigration > 800) {
-                showSnackbar(R.string.migration_part_1_done_resume)
-                refreshState()
             }
         }
     }

That said, if we have code that will get into a bad state if a cancellation did happen, that code is broken - because there's always the risk that the app will terminate at the same point due to other issues like lack of memory, the device restarting, etc.

[oakkitten]

I'd argue that's not a sensible change - preferences are thread-safe from what I gather

Tread-safety with regards to data objects is nice to have when you really need it, but it frees the object from race conditions no more than slapping a volatile on an int i frees it from problems when you do things like i++. The code might be correct, but reasoning about whether or not it is becomes hard.

if the preferences update is done outside of the queue, you introduce a race condition where some concurrent caller could end up reopening the collection at the old location, leading to data loss

Which is why I also suggested (elsewhere) that database locking should be independent of threading.

Are you advocating that we change the 125-and-growing calls to launchCatchingTask to instead be lifecycle.coroutineScope.launchCatching? That's somewhat more cumbersome, and I'm not sure the extra clarity is worth it for the majority of use cases.

FWIW there's a shortcut lifecycleScope. On the second thought, though, it's probably best to rethink the approach completely. launchCatchingTask, through the necessity of displaying a dialog, operates with the context of an activity and its lifecycle. However, most requests that are made inside of it are related to the activity as an UI concept (=a screen, a window, something the user is looking at), while from the point of view of the code activities are much more shortlived. For instance, if you want to display a list of decks, using launchCatchingTask is a bad idea, as you would have to re-run it every time the activity is recreated. And if the task at hand fails for some reason and during that time the activity is recreated, the error dialog is lost.

The proper way would to launch these tasks on the view model, which we are not currently widely using. Reporting error events can be done though converting them to a resettable state or other means.

I'm not familiar with its use

I think a proper solution here would be to move essential file migration logic to the service, which I'm planning to do (which is to say, I'm planning to make a PR that closes this issue, at least as far as the problem that the title describes goes); just mentioned withContext(NonCancellable) for the sake of the broader discussion

[oakkitten]

Regarding launchCatchingTask, this might work like this. Define a view model for the error state. We say that we are in an error state when we have not sufficiently informed the user about the error at hand. This could mean that we didn't show a toast, didn't show a snackbar for more than N seconds, or didn't show an error dialog, or are showing it but the user hasn't dismissed it.

open class ErrorViewModel : ViewModel() {
    // Or simply use `Exception?`.
    // Note that this only holds one error;
    // if there's a need to display more errors the logic should be updated.
    sealed interface State {
        class Error(val e: Exception) : State
        object NoError : State
    }

    private val _state = MutableStateFlow<State>(State.NoError)
    val state: StateFlow<State> = _state

    fun setStateToNoError() { // Or maybe `userDimissedTheError` or something
        _state.update { State.NoError }
    }

    suspend fun updateStateOnError(block: suspend () -> Unit) {
        try {
            block()
        } catch (e: CancellationException) {
            throw e
        } catch (e: Exception) {
            _state.update { State.Error(e) }
        }
    }
}

Then we add an activity that can consume view model updates:

open class ActivityThatCanShowErrors : AppCompatActivity() {
    protected val errorViewModel: ErrorViewModel by viewModels()

    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)

        lifecycleScope.launch {
            errorViewModel.state
                // Probably do not use `flowWithLifecycle` or `repeatOnLifecycle` here.
                // Doing so will show multiple dialogs.
                .filterIsInstance<ErrorViewModel.State.Error>() // Or use `if`
                .collect { error ->
                    // Or show a dialog fragment and call `setStateToNoError()` at once
                    AlertDialog.Builder(this@ActivityThatCanShowErrors)
                        .setTitle("Error")
                        .setMessage(error.e.localizedMessage)
                        .setNegativeButton("Dismiss") { _, _ ->
                            errorViewModel.setStateToNoError()
                        }
                        .show()
                }
        }
    }
}

Now in an activity that overrides it you can run this:

class MainActivity : ActivityThatCanShowErrors() {
    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)

        errorViewModel.viewModelScope.launch { 
            errorViewModel.updateStateOnError { 
                // Dangerous code here!
            } 
        }
    }
}

This logic can be refactored into a single method that can replace many launchCatchingTask calls.

BUT

This is largely useless unless your only goal is to silently perform some action. In real-life scenarios, you want to be calling methods of the current fragment or activity view models instead, since you have some state to change. The problem is, for launching, you probably want to use the scope of the view model the methods of which you are calling. Especially because this view model can have a shorter lifespan than that of errorViewModels! So, suppose you have a view model like this:

class ThisViewModel : ViewModel() {
    private val _result = MutableStateFlow<Int?>(null)
    val result: StateFlow<Int?> = _result

    suspend fun calculateResult() {
        _result.update { 1 / 0 }
    }
}

Then you would do:

class MainFragment : FragmentThatCanShowErrors() {
    private val thisViewModel: ThisViewModel by viewModels()

    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)

        thisViewModel.viewModelScope.launch { // More narrowly scoped model
            errorViewModel.updateStateOnError { // More widely scoped model
                thisViewModel.calculateResult()
            }
        }

        // Do something with the result
    }
}

But this is too wordy. I personally prefer to tell the view model “to launch” the update. Now here I'm not sure. There might be a way for a view model to somehow reference another view model, perhaps via factories, with the idea being that ThisViewModel or other models can reference ErrorViewModel. But with this guaranteeing that the target activity or a fragment actually works with ErrorViewModel becomes a problem. (Very slight one, but nevertheless.) One way to solve this that I see is using (yet again!) context receivers. Namely,

First, you define a error view model provider and have FragmentThatCanShowErrors implement it:

interface ErrorViewModelProvider {
    val errorViewModel: ErrorViewModel
}

Then, you add a method to ThisViewModel that is explicitly aware and takes the provider of ErrorViewModel:

class ThisViewModel : ViewModel() {
    ...

    context(ErrorViewModelProvider)
    fun launchCalculationOfResult() = viewModelScope.launch {
        errorViewModel.updateStateOnError { // I think this is very nice as it clearly shows the intention
            calculateResult()
        }
    }
}

Then you make your fragments and stuff implement the provider

open class FragmentThatCanShowErrors : Fragment(), ErrorViewModelProvider {
    override val errorViewModel: ErrorViewModel by viewModels()
    ...
}

And finally in your actual fragments, just say

class MainFragment : FragmentThatCanShowErrors() {
    private val thisViewModel: ThisViewModel by viewModels()

    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)

        thisViewModel.launchCalculationOfResult() // Much simplicity, such wow!
    }
}

P.S. Code is only partially tested and may contain typos and stuff but you should get the general idea

[dae]

I am not very familiar with UI handling in Android, and have no experience with ViewModels, so I don't feel particularly qualified to judge their appropriateness here. All I can go off are some assumptions, and some brief Googling. The name appears to imply they're intended for modelling the state associated with particular views, but in the context of a migration, isn't the state of the migration something relevant to the entire app and not a particular view? One view may be tasked with presenting that state to the user, but if we're allowing users to navigate around the app while migration is underway, collection access may come from multiple different views. Wouldn't a global scope/global object be more appropriate here?

I was not familiar with StateFlow either, but by the looks of it, it's a reactive data container that's thread safe? That looks like it could be useful for making the state of the media migration available to the UI.

Any thoughts on this page, which I stumbled across in my brief Googling? https://www.techyourchance.com/you-dont-need-android-viewmodel/

[oakkitten]

in the context of a migration, isn't the state of the migration something relevant to the entire app and not a particular view?

Yes, as we are using a service we can read its state directly, and we use StateFlow for it since it's just so convenient.

https://github.com/ankidroid/Anki-Android/blob/56601041636f06e3e177749e73b75051fcb76568/AnkiDroid/src/main/java/com/ichi2/anki/dialogs/MigrationProgressDialogFragment.kt#L74-L80

Note flowWithLifecycle, which makes the UI changes to pause when the fragment is stopped. How cool is this?

Any thoughts on this page, which I stumbled across in my brief Googling?

That's a very weird article, the author is arguing against view models, but only lists its pros, and the hoops you are going to jump through to avoid it. Only in the summary there's a link to another article that lists these issues (skimmed so may have missed some):

• No save and restore (to disk, when application is killed due to low memory or w/e) (But view model can handle this??)
• App will use more RAM “since it is so simple to retain data in Activities and Fragments using ViewModel approach” (???)
• “ViewModel has its own lifecycle. Therefore, it will make the overall lifecycle of the application more complex.” (???)

I think view models, the way they are implemented in Android, are cool because:

• They are relatively simple to grasp. Not super simple but simple enough
• It's industry standard
• It's built in and comes with all the bells and whistles (simple Kotlin API, lifecycle scope, saving to disk)
• It makes it hard to write shitty code that mixes concerns

By the last point I mean that once you start with view models you quickly get the main “problem” of view models, namely that you can't access Context from it. And you shouldn't. I think this very quickly puts you onto the path of separating UI and business logic, which should lead to all sorts of benefits but the one I like the most is that kinda pushes you into creation of “perfect code”, which is my own term for functions that are so good (simple, doing one thing) that the only thing that can happen to them is their deletion from the codebase because they are not needed. (Not that you need view models to separate concerns.)

[oakkitten]

Also I think of view models as of effectively a cache. How about this:

class NotReallyAViewModel {
    val deckList: StateFlow<...> = ...

    fun launchFetchingDeckList() = lifecycleScope.lauch {
        deckList.update { ... }
    }
}

If you have this, you have an always available deck list (sans the first ever fetch), nearly in sync. And as long as you don't miss a call to the launchFetchingDeckList, which you can perform as much as you want, whatever is collecting the flow will always be up-to-date.

[oakkitten]

That said, at least for the essential files part, I'm not sure it actually matters that much in practice. As far as I'm aware, a coroutine can only be interrupted at a suspend point.

By the way, there's quite a few suspension points in sync logic, and that one's long-running. It currently works most of the time because we handle configuration changes manually, but those do not include all changes, such as theme changes, so you should expect to see various issues when e.g. device goes into battery saver mode while syncing

Pro tip: sync code shouldn't be aware of the deck picker or any UI elements....

[dae]

In case you didn't mean to be, a heads up for next time: "pro tip" is somewhat condescending.

I think you're right that the activity lifecycle is not the best choice for the syncing code, and have no objections to you improving the code once scoped storage is done.

[oakkitten]

I often see “pro tip” used quite un-seriously

A piece of advice from an expert or professional. The phrase is often used humorously to introduce unnecessary or obvious advice.

— https://idioms.thefreedictionary.com/pro+tip

[mikehardy]

It is condescending because it can be misinterpreted as advice directed from a professional to a non-professional, or that something is or should be obvious when it apparently isn't (possibly also therefore being kind of arrogant)

"I often see all" sorts of things "used quite un-seriously" that are nonetheless condescending, arrogant, irritating, you name it.

The resistance to feedback is quite strong, I will continue to drop comments in when I see it pointedly requesting either in the specific (the normal case) or in the general case (this time) to onboard feedback constructively vs contrarily.

[oakkitten]

I am sorry. In no way I meant this to sound condescending. You know I am not a professional by any standards, and also English is not my mother tongue. By “pro tip” I only wanted to say “stating what we all are well aware of” or something along the lines of that.