From 036b882677edf417374ec13bbe6572e848d231fb Mon Sep 17 00:00:00 2001 From: Damien Elmes Date: Mon, 8 Aug 2022 13:29:20 +1000 Subject: [PATCH] Introduce CollectionManager for preventing concurrent access Currently there are many instances in the AnkiDroid codebase where the collection is accessed a) on the UI thread, blocking the UI, and b) in unsafe ways (eg by checking to see if the collection is open, and then assuming it will remain open for the rest of a method call. "Fix full download crashing in new schema case" (159a108dcb) demonstrates a few of these cases. This PR is an attempt at addressing those issues. It introduces a `withCol` function that is intended to be used instead of CollectionHelper.getCol(). For example, code that previously looked like: val col = CollectionHelper.getInstance().getCol(this) val count = col.decks.count() Can now be used like this: val count = withCol { decks.count() } The block is run on a background thread, and other withCol calls made in parallel will be queued up and executed sequentially. Because of the exclusive access, routines can safely close and reopen the collection inside the block without fear of affecting other callers. It's not practical to update all the legacy code to use withCol immediately - too much work is required, and coroutines are not accessible from Java code. The intention here is that this new path is gradually bought into. Legacy code can continue calling CollectionHelper. getCol(), which internally delegates to CollectionManager. Two caveats to be aware of: - Legacy callers will wait for other pending operations to complete before they receive the collection handle, but because they retain it, subsequent access is not guaranteed to be exclusive. - Because getCol() and colIsOpen() are often used on the main thread, they will block the UI if a background operation is already running. Logging has been added to help diagnose this, eg messages like: E/CollectionManager: blocked main thread for 2626ms: com.ichi2.anki.DeckPicker.onCreateOptionsMenu(DeckPicker.kt:624) Other changes: - simplified CoroutineHelpers - added TR function for accessing translations without a col reference - onCreateOptionsMenu() needed refactoring to avoid blocking the UI. - The blocking colIsOpen() call in onPrepareOptionsMenu() had to be removed. I can not reproduce the issue it reports, and the code checks for col in onCreateOptionsMenu(). - The subscribers in ChangeManager need to be cleared out at the start of each test, or two tests are flaky when run with the new schema. --- .../com/ichi2/anki/AbstractFlashcardViewer.kt | 4 +- .../java/com/ichi2/anki/BackendBackups.kt | 24 +- .../java/com/ichi2/anki/BackendImporting.kt | 17 +- .../main/java/com/ichi2/anki/BackendUndo.kt | 7 +- .../main/java/com/ichi2/anki/CardBrowser.kt | 4 +- .../java/com/ichi2/anki/CollectionHelper.java | 74 +--- .../java/com/ichi2/anki/CollectionManager.kt | 349 ++++++++++++++++++ .../java/com/ichi2/anki/CoroutineHelpers.kt | 66 ++-- .../main/java/com/ichi2/anki/DatabaseCheck.kt | 14 +- .../main/java/com/ichi2/anki/DeckPicker.kt | 192 +++++----- .../main/java/com/ichi2/anki/Preferences.kt | 13 +- .../src/main/java/com/ichi2/anki/Sync.kt | 96 +++-- .../preferences/GeneralSettingsFragment.kt | 8 +- .../scopedstorage/MigrateEssentialFiles.kt | 7 +- .../java/com/ichi2/libanki/ChangeManager.kt | 15 +- .../java/com/ichi2/anki/DeckPickerTest.kt | 25 +- .../java/com/ichi2/anki/RobolectricTest.kt | 11 + .../anki/dialogs/CreateDeckDialogTest.kt | 29 +- 18 files changed, 625 insertions(+), 330 deletions(-) create mode 100644 AnkiDroid/src/main/java/com/ichi2/anki/CollectionManager.kt diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/AbstractFlashcardViewer.kt b/AnkiDroid/src/main/java/com/ichi2/anki/AbstractFlashcardViewer.kt index 1860de92ac..2361ed5d44 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/AbstractFlashcardViewer.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/AbstractFlashcardViewer.kt @@ -853,8 +853,8 @@ abstract class AbstractFlashcardViewer : if (BackendFactory.defaultLegacySchema) { legacyUndo() } else { - return launchCatchingCollectionTask { col -> - if (!backendUndoAndShowPopup(col)) { + return launchCatchingTask { + if (!backendUndoAndShowPopup()) { legacyUndo() } } diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/BackendBackups.kt b/AnkiDroid/src/main/java/com/ichi2/anki/BackendBackups.kt index b258159cbe..08df32ebf3 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/BackendBackups.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/BackendBackups.kt @@ -18,47 +18,45 @@ package com.ichi2.anki -import com.ichi2.libanki.CollectionV16 +import com.ichi2.anki.CollectionManager.withCol import com.ichi2.libanki.awaitBackupCompletion import com.ichi2.libanki.createBackup import kotlinx.coroutines.* fun DeckPicker.performBackupInBackground() { - launchCatchingCollectionTask { col -> + launchCatchingTask { // Wait a second to allow the deck list to finish loading first, or it // will hang until the first stage of the backup completes. delay(1000) - createBackup(col, false) + createBackup(force = false) } } fun DeckPicker.importColpkg(colpkgPath: String) { launchCatchingTask { - val helper = CollectionHelper.getInstance() - val backend = helper.getOrCreateBackend(baseContext) - runInBackgroundWithProgress( - backend, + withProgress( extractProgress = { if (progress.hasImporting()) { text = progress.importing } }, ) { - helper.importColpkg(baseContext, colpkgPath) + CollectionManager.importColpkg(colpkgPath) } + invalidateOptionsMenu() updateDeckList() } } -private suspend fun createBackup(col: CollectionV16, force: Boolean) { - runInBackground { +private suspend fun createBackup(force: Boolean) { + withCol { // this two-step approach releases the backend lock after the initial copy - col.createBackup( - BackupManager.getBackupDirectoryFromCollection(col.path), + newBackend.createBackup( + BackupManager.getBackupDirectoryFromCollection(this.path), force, waitForCompletion = false ) - col.awaitBackupCompletion() + newBackend.awaitBackupCompletion() } } diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/BackendImporting.kt b/AnkiDroid/src/main/java/com/ichi2/anki/BackendImporting.kt index 7880fe217c..2d619ed7f3 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/BackendImporting.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/BackendImporting.kt @@ -19,6 +19,7 @@ package com.ichi2.anki import anki.import_export.ImportResponse +import com.ichi2.anki.CollectionManager.withCol import com.ichi2.libanki.DeckId import com.ichi2.libanki.exportAnkiPackage import com.ichi2.libanki.importAnkiPackage @@ -26,10 +27,9 @@ import com.ichi2.libanki.undoableOp import net.ankiweb.rsdroid.Translations fun DeckPicker.importApkgs(apkgPaths: List) { - launchCatchingCollectionTask { col -> + launchCatchingTask { for (apkgPath in apkgPaths) { - val report = runInBackgroundWithProgress( - col.backend, + val report = withProgress( extractProgress = { if (progress.hasImporting()) { text = progress.importing @@ -37,7 +37,7 @@ fun DeckPicker.importApkgs(apkgPaths: List) { }, ) { undoableOp { - col.importAnkiPackage(apkgPath) + importAnkiPackage(apkgPath) } } showSimpleMessageDialog(summarizeReport(col.tr, report)) @@ -70,16 +70,17 @@ fun DeckPicker.exportApkg( withMedia: Boolean, deckId: DeckId? ) { - launchCatchingCollectionTask { col -> - runInBackgroundWithProgress( - col.backend, + launchCatchingTask { + withProgress( extractProgress = { if (progress.hasExporting()) { text = progress.exporting } }, ) { - col.exportAnkiPackage(apkgPath, withScheduling, withMedia, deckId) + withCol { + newBackend.exportAnkiPackage(apkgPath, withScheduling, withMedia, deckId) + } } } } diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/BackendUndo.kt b/AnkiDroid/src/main/java/com/ichi2/anki/BackendUndo.kt index ee302b2022..76f3179f4e 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/BackendUndo.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/BackendUndo.kt @@ -17,17 +17,16 @@ package com.ichi2.anki import com.ichi2.anki.UIUtils.showSimpleSnackbar -import com.ichi2.libanki.CollectionV16 import com.ichi2.libanki.undoNew import com.ichi2.libanki.undoableOp import com.ichi2.utils.BlocksSchemaUpgrade import net.ankiweb.rsdroid.BackendException -suspend fun AnkiActivity.backendUndoAndShowPopup(col: CollectionV16): Boolean { +suspend fun AnkiActivity.backendUndoAndShowPopup(): Boolean { return try { - val changes = runInBackgroundWithProgress() { + val changes = withProgress() { undoableOp { - col.undoNew() + undoNew() } } showSimpleSnackbar( diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/CardBrowser.kt b/AnkiDroid/src/main/java/com/ichi2/anki/CardBrowser.kt index 864e858ea6..2032378a1a 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/CardBrowser.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/CardBrowser.kt @@ -1262,8 +1262,8 @@ open class CardBrowser : if (BackendFactory.defaultLegacySchema) { Undo().runWithHandler(mUndoHandler) } else { - launchCatchingCollectionTask { col -> - if (!backendUndoAndShowPopup(col)) { + launchCatchingTask { + if (!backendUndoAndShowPopup()) { Undo().runWithHandler(mUndoHandler) } } diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/CollectionHelper.java b/AnkiDroid/src/main/java/com/ichi2/anki/CollectionHelper.java index 872d3b2d98..1c7d609f7b 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/CollectionHelper.java +++ b/AnkiDroid/src/main/java/com/ichi2/anki/CollectionHelper.java @@ -51,15 +51,9 @@ import static com.ichi2.libanki.BackendImportExportKt.importCollectionPackage; * Singleton which opens, stores, and closes the reference to the Collection. */ public class CollectionHelper { - - // Collection instance belonging to sInstance - private Collection mCollection; // Name of anki2 file public static final String COLLECTION_FILENAME = "collection.anki2"; - // A backend instance is reused after collection close. - private @Nullable Backend mBackend; - /** * The preference key for the path to the current AnkiDroid directory *
@@ -143,54 +137,19 @@ public class CollectionHelper { */ private Collection openCollection(Context context, String path) { Timber.i("Begin openCollection: %s", path); - Backend backend = getOrCreateBackend(context); + Backend backend = BackendFactory.getBackend(context); Collection collection = Storage.collection(context, path, false, true, backend); Timber.i("End openCollection: %s", path); return collection; } - synchronized @NonNull Backend getOrCreateBackend(Context context) { - if (mBackend == null) { - mBackend = BackendFactory.getBackend(context); - } - return mBackend; - } - - - /** - * Close the currently cached backend and discard it. Useful when enabling the V16 scheduler in the - * dev preferences, or if the active language changes. The collection should be closed before calling - * this. - */ - public synchronized void discardBackend() { - if (mBackend != null) { - mBackend.close(); - mBackend = null; - } - } - /** * Get the single instance of the {@link Collection}, creating it if necessary (lazy initialization). * @param _context is no longer used, as the global AnkidroidApp instance is used instead * @return instance of the Collection */ - public synchronized Collection getCol(Context _context) { - // Open collection - Context context = AnkiDroidApp.getInstance(); - if (!colIsOpen()) { - String path = getCollectionPath(context); - // Check that the directory has been created and initialized - try { - initializeAnkiDroidDirectory(getParentDirectory(path)); - // Path to collection, cached for the reopenCollection() method - } catch (StorageAccessException e) { - Timber.e(e, "Could not initialize AnkiDroid directory"); - return null; - } - // Open the database - mCollection = openCollection(context, path); - } - return mCollection; + public synchronized Collection getCol(Context context) { + return CollectionManager.getColUnsafe(); } /** @@ -260,29 +219,15 @@ public class CollectionHelper { */ public synchronized void closeCollection(boolean save, String reason) { Timber.i("closeCollection: %s", reason); - if (mCollection != null) { - mCollection.close(save); - } + CollectionManager.closeCollectionBlocking(save); } - /** - * Replace the collection with the provided colpkg file if it is valid. - */ - public synchronized void importColpkg(Context context, String colpkgPath) { - Backend backend = getOrCreateBackend(context); - if (mCollection != null) { - mCollection.close(true); - } - String colPath = getCollectionPath(context); - importCollectionPackage(backend, colPath, colpkgPath); - getCol(context); - } /** * @return Whether or not {@link Collection} and its child database are open. */ public boolean colIsOpen() { - return mCollection != null && !mCollection.isDbClosed(); + return CollectionManager.isOpenUnsafe(); } /** @@ -621,13 +566,6 @@ public class CollectionHelper { @VisibleForTesting(otherwise = VisibleForTesting.NONE) public void setColForTests(Collection col) { - if (col == null) { - try { - mCollection.close(); - } catch (Exception exc) { - // may not be open - } - } - this.mCollection = col; + CollectionManager.setColForTests(col); } } diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/CollectionManager.kt b/AnkiDroid/src/main/java/com/ichi2/anki/CollectionManager.kt new file mode 100644 index 0000000000..d5b5ec6a0d --- /dev/null +++ b/AnkiDroid/src/main/java/com/ichi2/anki/CollectionManager.kt @@ -0,0 +1,349 @@ +/*************************************************************************************** + * Copyright (c) 2022 Ankitects Pty Ltd * + * * + * This program is free software; you can redistribute it and/or modify it under * + * the terms of the GNU General Public License as published by the Free Software * + * Foundation; either version 3 of the License, or (at your option) any later * + * version. * + * * + * This program is distributed in the hope that it will be useful, but WITHOUT ANY * + * WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A * + * PARTICULAR PURPOSE. See the GNU General Public License for more details. * + * * + * You should have received a copy of the GNU General Public License along with * + * this program. If not, see . * + ****************************************************************************************/ + +package com.ichi2.anki + +import android.annotation.SuppressLint +import android.os.Looper +import com.ichi2.libanki.Collection +import com.ichi2.libanki.CollectionV16 +import com.ichi2.libanki.Storage.collection +import com.ichi2.libanki.importCollectionPackage +import kotlinx.coroutines.* +import net.ankiweb.rsdroid.Backend +import net.ankiweb.rsdroid.BackendFactory +import net.ankiweb.rsdroid.Translations +import timber.log.Timber +import java.io.File +import java.lang.RuntimeException + +object CollectionManager { + /** + * The currently active backend, which is created on demand via [ensureBackend], and + * implicitly via [ensureOpen] and routines like [withCol]. + * The backend is long-lived, and will generally only be closed when switching interface + * languages or changing schema versions. A closed backend cannot be reused, and a new one + * must be created. + */ + private var backend: Backend? = null + + /** + * The current collection, which is opened on demand via [withCol]. If you need to + * close and reopen the collection in an atomic operation, add a new method that + * calls [withQueue], and then executes [ensureClosedInner] and [ensureOpenInner] inside it. + * A closed collection can be detected via [withOpenColOrNull] or by checking [Collection.dbClosed]. + */ + private var collection: Collection? = null + + @OptIn(ExperimentalCoroutinesApi::class) + private var queue: CoroutineDispatcher = Dispatchers.IO.limitedParallelism(1) + + /** + * Execute the provided block on a serial queue, to ensure concurrent access + * does not happen. + * It's important that the block is not suspendable - if it were, it would allow + * multiple requests to be interleaved when a suspend point was hit. + */ + private suspend fun withQueue(block: CollectionManager.() -> T): T { + return withContext(queue) { + this@CollectionManager.block() + } + } + + /** + * Execute the provided block with the collection, opening if necessary. + * + * Parallel calls to this function are guaranteed to be serialized, so you can be + * sure the collection won't be closed or modified by another thread. This guarantee + * does not hold if legacy code calls [getColUnsafe]. + */ + suspend fun withCol(block: Collection.() -> T): T { + return withQueue { + ensureOpenInner() + block(collection!!) + } + } + + /** + * Execute the provided block if the collection is already open. See [withCol] for more. + * Since the block may return a null value, and a null value will also be returned in the + * case of the collection being closed, if the calling code needs to distinguish between + * these two cases, it should wrap the return value of the block in a class (eg Optional), + * instead of returning a nullable object. + */ + suspend fun withOpenColOrNull(block: Collection.() -> T): T? { + return withQueue { + if (collection != null && !collection!!.dbClosed) { + block(collection!!) + } else { + null + } + } + } + + /** + * Return a handle to the backend, creating if necessary. This should only be used + * for routines that don't depend on an open or closed collection, such as checking + * the current progress state when importing a colpkg file. While the backend is + * thread safe and can be accessed concurrently, if another thread closes the collection + * and you call a routine that expects an open collection, it will result in an error. + */ + suspend fun getBackend(): Backend { + return withQueue { + ensureBackendInner() + backend!! + } + } + + /** + * Translations provided by the Rust backend/Anki desktop code. + */ + val TR: Translations + get() { + if (backend == null) { + runBlocking { ensureBackend() } + } + // we bypass the lock here so that translations are fast - conflicts are unlikely, + // as the backend is only ever changed on language preference change or schema switch + return backend!!.tr + } + + /** + * Close the currently cached backend and discard it. Useful when enabling the V16 scheduler in the + * dev preferences, or if the active language changes. Saves and closes the collection if open. + */ + suspend fun discardBackend() { + withQueue { + discardBackendInner() + } + } + + /** See [discardBackend]. This must only be run inside the queue. */ + private fun discardBackendInner() { + ensureClosedInner() + if (backend != null) { + backend!!.close() + backend = null + } + } + + /** + * Open the backend if it's not already open. + */ + private suspend fun ensureBackend() { + withQueue { + ensureBackendInner() + } + } + + /** See [ensureBackend]. This must only be run inside the queue. */ + private fun ensureBackendInner() { + if (backend == null) { + backend = BackendFactory.getBackend(AnkiDroidApp.getInstance()) + } + } + + /** + * If the collection is open, close it. + */ + suspend fun ensureClosed(save: Boolean = true) { + withQueue { + ensureClosedInner(save = save) + } + } + + /** See [ensureClosed]. This must only be run inside the queue. */ + private fun ensureClosedInner(save: Boolean = true) { + if (collection == null) { + return + } + try { + collection!!.close(save = save) + } catch (exc: Exception) { + Timber.e("swallowing error on close: $exc") + } + collection = null + } + + /** + * Open the collection, if it's not already open. + * + * Automatically called by [withCol]. Can be called directly to ensure collection + * is loaded at a certain point in time, or to ensure no errors occur. + */ + suspend fun ensureOpen() { + withQueue { + ensureOpenInner() + } + } + + /** See [ensureOpen]. This must only be run inside the queue. */ + private fun ensureOpenInner() { + ensureBackendInner() + if (collection == null || collection!!.dbClosed) { + val path = createCollectionPath() + collection = + collection(AnkiDroidApp.getInstance(), path, server = false, log = true, backend) + } + } + + /** Ensures the AnkiDroid directory is created, then returns the path to the collection file + * inside it. */ + fun createCollectionPath(): String { + val dir = CollectionHelper.getCurrentAnkiDroidDirectory(AnkiDroidApp.getInstance()) + CollectionHelper.initializeAnkiDroidDirectory(dir) + return File(dir, "collection.anki2").absolutePath + } + + @JvmStatic + fun closeCollectionBlocking(save: Boolean = true) { + runBlocking { ensureClosed(save = save) } + } + + /** + * Returns a reference to the open collection. This is not + * safe, as code in other threads could open or close + * the collection while the reference is held. [withCol] + * is a better alternative. + */ + @JvmStatic + fun getColUnsafe(): Collection { + return logUIHangs { runBlocking { withCol { this } } } + } + + private fun isMainThread(): Boolean { + return try { + Looper.getMainLooper().thread == Thread.currentThread() + } catch (exc: RuntimeException) { + if (exc.message?.contains("Looper not mocked") == true) { + // When unit tests are run outside of Robolectric, the call to getMainLooper() + // will fail. We swallow the exception in this case, and assume the call was + // not made on the main thread. + false + } else { + throw exc + } + } + } + + /** + Execute [block]. If it takes more than 100ms of real time, Timber an error like: + > Blocked main thread for 2424ms: com.ichi2.anki.DeckPicker.onCreateOptionsMenu(DeckPicker.kt:624) + */ + // using TimeManager breaks a sched test that makes assumptions about the time, so we + // access the time directly + @SuppressLint("DirectSystemCurrentTimeMillisUsage") + private fun logUIHangs(block: () -> T): T { + val start = System.currentTimeMillis() + return block().also { + val elapsed = System.currentTimeMillis() - start + if (isMainThread() && elapsed > 100) { + val stackTraceElements = Thread.currentThread().stackTrace + // locate the probable calling file/line in the stack trace, by filtering + // out our own code, and standard dalvik/java.lang stack frames + val caller = stackTraceElements.filter { + val klass = it.className + for ( + text in listOf( + "CollectionManager", "dalvik", "java.lang", + "CollectionHelper", "AnkiActivity" + ) + ) { + if (text in klass) { + return@filter false + } + } + true + }.first() + Timber.e("blocked main thread for %dms:\n%s", elapsed, caller) + } + } + } + + /** + * True if the collection is open. Unsafe, as it has the potential to race. + */ + @JvmStatic + fun isOpenUnsafe(): Boolean { + return logUIHangs { + runBlocking { + withQueue { + collection?.dbClosed == false + } + } + } + } + + /** + Use [col] as collection in tests. + This collection persists only up to the next (direct or indirect) call to `ensureClosed` + */ + @JvmStatic + fun setColForTests(col: Collection?) { + runBlocking { + withQueue { + if (col == null) { + ensureClosedInner() + } + collection = col + } + } + } + + /** + * Execute block with the collection upgraded to the latest schema. + * If it was previously using the legacy schema, the collection is downgraded + * again after the block completes. + */ + private suspend fun withNewSchema(block: CollectionV16.() -> T): T { + return withCol { + if (BackendFactory.defaultLegacySchema) { + // Temporarily update to the latest schema. + discardBackendInner() + BackendFactory.defaultLegacySchema = false + ensureOpenInner() + try { + (collection!! as CollectionV16).block() + } finally { + BackendFactory.defaultLegacySchema = true + discardBackendInner() + } + } else { + (this as CollectionV16).block() + } + } + } + + /** Upgrade from v1 to v2 scheduler. + * Caller must have confirmed schema modification already. + */ + suspend fun updateScheduler() { + withNewSchema { + sched.upgradeToV2() + } + } + + /** + * Replace the collection with the provided colpkg file if it is valid. + */ + suspend fun importColpkg(colpkgPath: String) { + withQueue { + ensureClosedInner() + ensureBackendInner() + importCollectionPackage(backend!!, createCollectionPath(), colpkgPath) + } + } +} diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/CoroutineHelpers.kt b/AnkiDroid/src/main/java/com/ichi2/anki/CoroutineHelpers.kt index 70c5b5f6c6..e4c3a92455 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/CoroutineHelpers.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/CoroutineHelpers.kt @@ -22,7 +22,6 @@ import androidx.lifecycle.coroutineScope import anki.collection.Progress import com.ichi2.anki.UIUtils.showSimpleSnackbar import com.ichi2.libanki.Collection -import com.ichi2.libanki.CollectionV16 import com.ichi2.themes.StyledProgressDialog import kotlinx.coroutines.* import net.ankiweb.rsdroid.Backend @@ -64,22 +63,7 @@ private fun showError(context: Context, msg: String) { .show() } -/** Launch a catching task that requires a collection with the new schema enabled. */ -fun AnkiActivity.launchCatchingCollectionTask(block: suspend CoroutineScope.(col: CollectionV16) -> Unit): Job { - val col = CollectionHelper.getInstance().getCol(baseContext).newBackend - return launchCatchingTask { - block(col) - } -} - -/** Run a blocking call in a background thread pool. */ -suspend fun runInBackground(block: suspend CoroutineScope.() -> T): T { - return withContext(Dispatchers.IO) { - block() - } -} - -/** In most cases, you'll want [AnkiActivity.runInBackgroundWithProgress] +/** In most cases, you'll want [AnkiActivity.withProgress] * instead. This lower-level routine can be used to integrate your own * progress UI. */ @@ -101,46 +85,46 @@ suspend fun Backend.withProgress( } /** - * Run the provided operation in the background, showing a progress - * window. Progress info is polled from the backend. + * Run the provided operation, showing a progress window until it completes. + * Progress info is polled from the backend. */ -suspend fun AnkiActivity.runInBackgroundWithProgress( - backend: Backend, +suspend fun AnkiActivity.withProgress( extractProgress: ProgressContext.() -> Unit, onCancel: ((Backend) -> Unit)? = { it.setWantsAbort() }, op: suspend () -> T -): T = withProgressDialog( - context = this@runInBackgroundWithProgress, - onCancel = if (onCancel != null) { - fun() { onCancel(backend) } - } else { - null - } -) { dialog -> - backend.withProgress( - extractProgress = extractProgress, - updateUi = { updateDialog(dialog) } - ) { - runInBackground { op() } +): T { + val backend = CollectionManager.getBackend() + return withProgressDialog( + context = this@withProgress, + onCancel = if (onCancel != null) { + fun() { onCancel(backend) } + } else { + null + } + ) { dialog -> + backend.withProgress( + extractProgress = extractProgress, + updateUi = { updateDialog(dialog) } + ) { + op() + } } } /** - * Run the provided operation in the background, showing a progress - * window with the provided message. + * Run the provided operation, showing a progress window with the provided + * message until the operation completes. */ -suspend fun AnkiActivity.runInBackgroundWithProgress( +suspend fun AnkiActivity.withProgress( message: String = resources.getString(R.string.dialog_processing), op: suspend () -> T ): T = withProgressDialog( - context = this@runInBackgroundWithProgress, + context = this@withProgress, onCancel = null ) { dialog -> @Suppress("Deprecation") // ProgressDialog deprecation dialog.setMessage(message) - runInBackground { - op() - } + op() } private suspend fun withProgressDialog( diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/DatabaseCheck.kt b/AnkiDroid/src/main/java/com/ichi2/anki/DatabaseCheck.kt index 440842f2e2..640ceb7435 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/DatabaseCheck.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/DatabaseCheck.kt @@ -16,10 +16,12 @@ package com.ichi2.anki +import com.ichi2.anki.CollectionManager.TR +import com.ichi2.anki.CollectionManager.withCol + fun DeckPicker.handleDatabaseCheck() { - launchCatchingCollectionTask { col -> - val problems = runInBackgroundWithProgress( - col.backend, + launchCatchingTask { + val problems = withProgress( extractProgress = { if (progress.hasDatabaseCheck()) { progress.databaseCheck.let { @@ -34,12 +36,14 @@ fun DeckPicker.handleDatabaseCheck() { }, onCancel = null, ) { - col.fixIntegrity() + withCol { + newBackend.fixIntegrity() + } } val message = if (problems.isNotEmpty()) { problems.joinToString("\n") } else { - col.tr.databaseCheckRebuilt() + TR.databaseCheckRebuilt() } showSimpleMessageDialog(message) } diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt b/AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt index fd5c75dc2f..7304c82246 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt @@ -60,6 +60,8 @@ import com.afollestad.materialdialogs.MaterialDialog import com.google.android.material.snackbar.Snackbar import com.ichi2.anim.ActivityTransitionAnimation.Direction.* import com.ichi2.anki.CollectionHelper.CollectionIntegrityStorageCheck +import com.ichi2.anki.CollectionManager.TR +import com.ichi2.anki.CollectionManager.withOpenColOrNull import com.ichi2.anki.InitialActivity.StartupFailure import com.ichi2.anki.InitialActivity.StartupFailure.* import com.ichi2.anki.StudyOptionsFragment.DeckStudyData @@ -203,6 +205,10 @@ open class DeckPicker : private lateinit var mCustomStudyDialogFactory: CustomStudyDialogFactory private lateinit var mContextMenuFactory: DeckPickerContextMenu.Factory + // stored for testing purposes + @VisibleForTesting + var createMenuJob: Job? = null + init { ChangeManager.subscribe(this) } @@ -570,92 +576,101 @@ open class DeckPicker : } } - override fun onPrepareOptionsMenu(menu: Menu): Boolean { - // Null check to prevent crash when col inaccessible - // #9081: sync leaves the collection closed, thus colIsOpen() is insufficient, carefully open the collection if possible - return if (CollectionHelper.getInstance().getColSafe(this) == null) { - false - } else super.onPrepareOptionsMenu(menu) - } - override fun onCreateOptionsMenu(menu: Menu): Boolean { - Timber.d("onCreateOptionsMenu()") - mFloatingActionMenu.closeFloatingActionMenu() - menuInflater.inflate(R.menu.deck_picker, menu) - val sdCardAvailable = AnkiDroidApp.isSdCardMounted() - menu.findItem(R.id.action_sync).isEnabled = sdCardAvailable - menu.findItem(R.id.action_new_filtered_deck).isEnabled = sdCardAvailable - menu.findItem(R.id.action_check_database).isEnabled = sdCardAvailable - menu.findItem(R.id.action_check_media).isEnabled = sdCardAvailable - menu.findItem(R.id.action_empty_cards).isEnabled = sdCardAvailable - - searchDecksIcon = menu.findItem(R.id.deck_picker_action_filter) - updateSearchDecksIconVisibility() - searchDecksIcon!!.setOnActionExpandListener(object : MenuItem.OnActionExpandListener { - // When SearchItem is expanded - override fun onMenuItemActionExpand(item: MenuItem?): Boolean { - Timber.i("DeckPicker:: SearchItem opened") - // Hide the floating action button if it is visible - mFloatingActionMenu.hideFloatingActionButton() - return true + // Store the job so that tests can easily await it. In the future + // this may be better done by injecting a custom test scheduler + // into CollectionManager, and awaiting that. + createMenuJob = launchCatchingTask { + val haveCol = withOpenColOrNull { true } ?: false + if (!haveCol) { + // avoid showing the menu if the collection is not open + return@launchCatchingTask } - // When SearchItem is collapsed - override fun onMenuItemActionCollapse(item: MenuItem?): Boolean { - Timber.i("DeckPicker:: SearchItem closed") - // Show the floating action button if it is hidden - mFloatingActionMenu.showFloatingActionButton() - return true - } - }) + Timber.d("onCreateOptionsMenu()") + mFloatingActionMenu.closeFloatingActionMenu() + menuInflater.inflate(R.menu.deck_picker, menu) + val sdCardAvailable = AnkiDroidApp.isSdCardMounted() + menu.findItem(R.id.action_sync).isEnabled = sdCardAvailable + menu.findItem(R.id.action_new_filtered_deck).isEnabled = sdCardAvailable + menu.findItem(R.id.action_check_database).isEnabled = sdCardAvailable + menu.findItem(R.id.action_check_media).isEnabled = sdCardAvailable + menu.findItem(R.id.action_empty_cards).isEnabled = sdCardAvailable - mToolbarSearchView = searchDecksIcon!!.actionView as SearchView - mToolbarSearchView!!.queryHint = getString(R.string.search_decks) - mToolbarSearchView!!.setOnQueryTextListener(object : SearchView.OnQueryTextListener { - override fun onQueryTextSubmit(query: String): Boolean { - mToolbarSearchView!!.clearFocus() - return true - } + searchDecksIcon = menu.findItem(R.id.deck_picker_action_filter) + updateSearchDecksIconVisibility() + searchDecksIcon!!.setOnActionExpandListener(object : MenuItem.OnActionExpandListener { + // When SearchItem is expanded + override fun onMenuItemActionExpand(item: MenuItem?): Boolean { + Timber.i("DeckPicker:: SearchItem opened") + // Hide the floating action button if it is visible + mFloatingActionMenu.hideFloatingActionButton() + return true + } + + // When SearchItem is collapsed + override fun onMenuItemActionCollapse(item: MenuItem?): Boolean { + Timber.i("DeckPicker:: SearchItem closed") + // Show the floating action button if it is hidden + mFloatingActionMenu.showFloatingActionButton() + return true + } + }) + + mToolbarSearchView = searchDecksIcon!!.actionView as SearchView + mToolbarSearchView!!.queryHint = getString(R.string.search_decks) + mToolbarSearchView!!.setOnQueryTextListener(object : SearchView.OnQueryTextListener { + override fun onQueryTextSubmit(query: String): Boolean { + mToolbarSearchView!!.clearFocus() + return true + } + + override fun onQueryTextChange(newText: String): Boolean { + val adapter = mRecyclerView.adapter as Filterable? + adapter!!.filter.filter(newText) + return true + } + }) - override fun onQueryTextChange(newText: String): Boolean { - val adapter = mRecyclerView.adapter as Filterable? - adapter!!.filter.filter(newText) - return true - } - }) - if (colIsOpen() && !CollectionHelper.getInstance().isCollectionLocked) { displaySyncBadge(menu) // Show / hide undo - if (fragmented || !col.undoAvailable()) { + val undoName = withOpenColOrNull { + if (fragmented || !undoAvailable()) { + null + } else { + undoName(resources) + } + } + + if (undoName == null) { menu.findItem(R.id.action_undo).isVisible = false } else { val res = resources menu.findItem(R.id.action_undo).isVisible = true - val undo = res.getString(R.string.studyoptions_congrats_undo, col.undoName(res)) + val undo = res.getString(R.string.studyoptions_congrats_undo, undoName) menu.findItem(R.id.action_undo).title = undo } + + updateSearchDecksIconVisibility() } return super.onCreateOptionsMenu(menu) } - /** - * Show [searchDecksIcon] if there are more than 10 decks. - * Otherwise, hide it if there are less than 10 decks - * or if a exception is thrown while getting the decks count (e.g. corrupt collection) - */ - private fun updateSearchDecksIconVisibility() { - searchDecksIcon?.isVisible = try { - col.decks.count() >= 10 - } catch (e: Exception) { - false - } + @VisibleForTesting + suspend fun updateSearchDecksIconVisibility() { + val visible = withOpenColOrNull { decks.count() >= 10 } ?: false + searchDecksIcon?.isVisible = visible } @VisibleForTesting - protected open fun displaySyncBadge(menu: Menu) { + protected open suspend fun displaySyncBadge(menu: Menu) { + val syncStatus = withOpenColOrNull { SyncStatus.getSyncStatus(this) } + if (syncStatus == null) { + return + } val syncMenu = menu.findItem(R.id.action_sync) - when (val syncStatus = SyncStatus.getSyncStatus { col }) { + when (syncStatus) { SyncStatus.BADGE_DISABLED, SyncStatus.NO_CHANGES, SyncStatus.INCONCLUSIVE -> { BadgeDrawableBuilder.removeBadge(syncMenu) syncMenu.setTitle(R.string.button_sync) @@ -1240,8 +1255,8 @@ open class DeckPicker : if (BackendFactory.defaultLegacySchema) { legacyUndo() } else { - launchCatchingCollectionTask { col -> - if (!backendUndoAndShowPopup(col)) { + launchCatchingTask { + if (!backendUndoAndShowPopup()) { legacyUndo() } } @@ -1970,8 +1985,8 @@ open class DeckPicker : if (!userAcceptsSchemaChange(col)) { return@launchCatchingTask } - runInBackgroundWithProgress { - CollectionHelper.getInstance().updateScheduler(this@DeckPicker) + withProgress { + CollectionManager.updateScheduler() } showThemedToast(this@DeckPicker, col.tr.schedulingUpdateDone(), false) refreshState() @@ -2214,7 +2229,9 @@ open class DeckPicker : mFocusedDeck = current } - updateSearchDecksIconVisibility() + launchCatchingTask { + updateSearchDecksIconVisibility() + } } // Callback to show study options for currently selected deck @@ -2285,15 +2302,15 @@ open class DeckPicker : if (!BackendFactory.defaultLegacySchema) { dismissAllDialogFragments() // No confirmation required, as undoable - return launchCatchingCollectionTask { col -> - val changes = runInBackgroundWithProgress { + return launchCatchingTask { + val changes = withProgress { undoableOp { - col.newDecks.removeDecks(listOf(did)) + newDecks.removeDecks(listOf(did)) } } showSimpleSnackbar( this@DeckPicker, - col.tr.browsingCardsDeleted(changes.count), + TR.browsingCardsDeleted(changes.count), false ) } @@ -2697,32 +2714,3 @@ open class DeckPicker : } } } - -/** Upgrade from v1 to v2 scheduler. - * Caller must have confirmed schema modification already. - */ -@KotlinCleanup("move into CollectionHelper once it's converted to Kotlin") -@Synchronized -fun CollectionHelper.updateScheduler(context: Context) { - if (BackendFactory.defaultLegacySchema) { - // We'll need to temporarily update to the latest schema. - closeCollection(true, "sched upgrade") - discardBackend() - BackendFactory.defaultLegacySchema = false - // Ensure collection closed if upgrade fails, and schema reverted - // even if close fails. - try { - try { - getCol(context).sched.upgradeToV2() - } finally { - closeCollection(true, "sched upgrade") - } - } finally { - BackendFactory.defaultLegacySchema = true - discardBackend() - } - } else { - // Can upgrade directly - getCol(context).sched.upgradeToV2() - } -} diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/Preferences.kt b/AnkiDroid/src/main/java/com/ichi2/anki/Preferences.kt index 64d6507fb9..e1da0d2122 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/Preferences.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/Preferences.kt @@ -215,13 +215,12 @@ class Preferences : AnkiActivity(), SearchPreferenceResultListener { } fun restartWithNewDeckPicker() { - // PERF: DB access on foreground thread - val helper = CollectionHelper.getInstance() - helper.closeCollection(true, "Preference Modification: collection path changed") - helper.discardBackend() - val deckPicker = Intent(this, DeckPicker::class.java) - deckPicker.addFlags(Intent.FLAG_ACTIVITY_CLEAR_TOP or Intent.FLAG_ACTIVITY_NEW_TASK) - startActivityWithAnimation(deckPicker, ActivityTransitionAnimation.Direction.DEFAULT) + launchCatchingTask { + CollectionManager.discardBackend() + val deckPicker = Intent(this@Preferences, DeckPicker::class.java) + deckPicker.addFlags(Intent.FLAG_ACTIVITY_CLEAR_TOP or Intent.FLAG_ACTIVITY_NEW_TASK) + startActivityWithAnimation(deckPicker, ActivityTransitionAnimation.Direction.DEFAULT) + } } // ---------------------------------------------------------------------------- diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/Sync.kt b/AnkiDroid/src/main/java/com/ichi2/anki/Sync.kt index ea74974e3c..a868630fdb 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/Sync.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/Sync.kt @@ -31,10 +31,11 @@ import anki.sync.SyncAuth import anki.sync.SyncCollectionResponse import anki.sync.syncAuth import com.ichi2.anim.ActivityTransitionAnimation +import com.ichi2.anki.CollectionManager.TR +import com.ichi2.anki.CollectionManager.withCol import com.ichi2.anki.dialogs.SyncErrorDialog import com.ichi2.anki.web.HostNumFactory import com.ichi2.async.Connection -import com.ichi2.libanki.CollectionV16 import com.ichi2.libanki.createBackup import com.ichi2.libanki.sync.* import net.ankiweb.rsdroid.Backend @@ -51,12 +52,12 @@ fun DeckPicker.handleNewSync( this.hostNumber = hostNum } val deckPicker = this - launchCatchingCollectionTask { col -> + launchCatchingTask { try { when (conflict) { - Connection.ConflictResolution.FULL_DOWNLOAD -> handleDownload(deckPicker, col, auth) - Connection.ConflictResolution.FULL_UPLOAD -> handleUpload(deckPicker, col, auth) - null -> handleNormalSync(deckPicker, col, auth) + Connection.ConflictResolution.FULL_DOWNLOAD -> handleDownload(deckPicker, auth) + Connection.ConflictResolution.FULL_UPLOAD -> handleUpload(deckPicker, auth) + null -> handleNormalSync(deckPicker, auth) } } catch (exc: BackendSyncException.BackendSyncAuthFailedException) { // auth failed; log out @@ -68,10 +69,12 @@ fun DeckPicker.handleNewSync( } fun MyAccount.handleNewLogin(username: String, password: String) { - launchCatchingCollectionTask { col -> + launchCatchingTask { val auth = try { - runInBackgroundWithProgress(col.backend, {}, onCancel = ::cancelSync) { - col.syncLogin(username, password) + withProgress({}, onCancel = ::cancelSync) { + withCol { + newBackend.syncLogin(username, password) + } } } catch (exc: BackendSyncException.BackendSyncAuthFailedException) { // auth failed; clear out login details @@ -98,11 +101,9 @@ private fun cancelSync(backend: Backend) { private suspend fun handleNormalSync( deckPicker: DeckPicker, - col: CollectionV16, auth: SyncAuth ) { - val output = deckPicker.runInBackgroundWithProgress( - col.backend, + val output = deckPicker.withProgress( extractProgress = { if (progress.hasNormalSync()) { text = progress.normalSync.run { "$added\n$removed" } @@ -110,7 +111,7 @@ private suspend fun handleNormalSync( }, onCancel = ::cancelSync ) { - col.syncCollection(auth) + withCol { newBackend.syncCollection(auth) } } // Save current host number @@ -122,17 +123,17 @@ private suspend fun handleNormalSync( deckPicker.showSyncLogMessage(R.string.sync_database_acknowledge, output.serverMessage) // kick off media sync - future implementations may want to run this in the // background instead - handleMediaSync(deckPicker, col, auth) + handleMediaSync(deckPicker, auth) } SyncCollectionResponse.ChangesRequired.FULL_DOWNLOAD -> { - handleDownload(deckPicker, col, auth) - handleMediaSync(deckPicker, col, auth) + handleDownload(deckPicker, auth) + handleMediaSync(deckPicker, auth) } SyncCollectionResponse.ChangesRequired.FULL_UPLOAD -> { - handleUpload(deckPicker, col, auth) - handleMediaSync(deckPicker, col, auth) + handleUpload(deckPicker, auth) + handleMediaSync(deckPicker, auth) } SyncCollectionResponse.ChangesRequired.FULL_SYNC -> { @@ -158,27 +159,24 @@ private fun fullDownloadProgress(title: String): ProgressContext.() -> Unit { private suspend fun handleDownload( deckPicker: DeckPicker, - col: CollectionV16, auth: SyncAuth ) { - deckPicker.runInBackgroundWithProgress( - col.backend, - extractProgress = fullDownloadProgress(col.tr.syncDownloadingFromAnkiweb()), + deckPicker.withProgress( + extractProgress = fullDownloadProgress(TR.syncDownloadingFromAnkiweb()), onCancel = ::cancelSync ) { - val helper = CollectionHelper.getInstance() - helper.lockCollection() - try { - col.createBackup( - BackupManager.getBackupDirectoryFromCollection(col.path), - force = true, - waitForCompletion = true - ) - col.close(save = true, downgrade = false, forFullSync = true) - col.fullDownload(auth) - } finally { - col.reopen(afterFullSync = true) - helper.unlockCollection() + withCol { + try { + newBackend.createBackup( + BackupManager.getBackupDirectoryFromCollection(path), + force = true, + waitForCompletion = true + ) + close(save = true, downgrade = false, forFullSync = true) + newBackend.fullDownload(auth) + } finally { + reopen(afterFullSync = true) + } } } @@ -188,22 +186,19 @@ private suspend fun handleDownload( private suspend fun handleUpload( deckPicker: DeckPicker, - col: CollectionV16, auth: SyncAuth ) { - deckPicker.runInBackgroundWithProgress( - col.backend, - extractProgress = fullDownloadProgress(col.tr.syncUploadingToAnkiweb()), + deckPicker.withProgress( + extractProgress = fullDownloadProgress(TR.syncUploadingToAnkiweb()), onCancel = ::cancelSync ) { - val helper = CollectionHelper.getInstance() - helper.lockCollection() - col.close(save = true, downgrade = false, forFullSync = true) - try { - col.fullUpload(auth) - } finally { - col.reopen(afterFullSync = true) - helper.unlockCollection() + withCol { + close(save = true, downgrade = false, forFullSync = true) + try { + newBackend.fullUpload(auth) + } finally { + reopen(afterFullSync = true) + } } } Timber.i("Full Upload Completed") @@ -220,19 +215,18 @@ private fun cancelMediaSync(backend: Backend) { private suspend fun handleMediaSync( deckPicker: DeckPicker, - col: CollectionV16, auth: SyncAuth ) { // TODO: show this in a way that is clear it can be continued in background, // but also warn user that media files will not be available until it completes. // TODO: provide a way for users to abort later, and see it's still going val dialog = AlertDialog.Builder(deckPicker) - .setTitle(col.tr.syncMediaLogTitle()) + .setTitle(TR.syncMediaLogTitle()) .setMessage("") .setPositiveButton("Background") { _, _ -> } .show() try { - col.backend.withProgress( + CollectionManager.getBackend().withProgress( extractProgress = { if (progress.hasMediaSync()) { text = @@ -243,8 +237,8 @@ private suspend fun handleMediaSync( dialog.setMessage(text) }, ) { - runInBackground { - col.syncMedia(auth) + withCol { + newBackend.syncMedia(auth) } } } finally { diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/preferences/GeneralSettingsFragment.kt b/AnkiDroid/src/main/java/com/ichi2/anki/preferences/GeneralSettingsFragment.kt index f3e2755bd0..42d4a45934 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/preferences/GeneralSettingsFragment.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/preferences/GeneralSettingsFragment.kt @@ -17,12 +17,13 @@ package com.ichi2.anki.preferences import androidx.preference.ListPreference import androidx.preference.SwitchPreference -import com.ichi2.anki.CollectionHelper +import com.ichi2.anki.* import com.ichi2.anki.CrashReportService import com.ichi2.anki.R import com.ichi2.anki.contextmenu.AnkiCardContextMenu import com.ichi2.anki.contextmenu.CardBrowserContextMenu import com.ichi2.utils.LanguageUtil +import kotlinx.coroutines.runBlocking import java.util.* class GeneralSettingsFragment : SettingsFragment() { @@ -102,10 +103,7 @@ class GeneralSettingsFragment : SettingsFragment() { // so do it if the language has changed. languageSelection.setOnPreferenceChangeListener { newValue -> LanguageUtil.setDefaultBackendLanguages(newValue as String) - - val helper = CollectionHelper.getInstance() - helper.closeCollection(true, "language change") - helper.discardBackend() + runBlocking { CollectionManager.discardBackend() } requireActivity().recreate() } diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/servicelayer/scopedstorage/MigrateEssentialFiles.kt b/AnkiDroid/src/main/java/com/ichi2/anki/servicelayer/scopedstorage/MigrateEssentialFiles.kt index 4f8cbdc05b..09414e75a9 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/servicelayer/scopedstorage/MigrateEssentialFiles.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/servicelayer/scopedstorage/MigrateEssentialFiles.kt @@ -21,6 +21,7 @@ import androidx.annotation.VisibleForTesting import androidx.core.content.edit import com.ichi2.anki.AnkiDroidApp import com.ichi2.anki.CollectionHelper +import com.ichi2.anki.CollectionManager import com.ichi2.anki.exception.RetryableException import com.ichi2.anki.model.Directory import com.ichi2.anki.servicelayer.* @@ -32,6 +33,7 @@ import com.ichi2.compat.CompatHelper import com.ichi2.libanki.Collection import com.ichi2.libanki.Storage import com.ichi2.libanki.Utils +import kotlinx.coroutines.runBlocking import net.ankiweb.rsdroid.BackendFactory import org.apache.commons.io.FileUtils import timber.log.Timber @@ -239,10 +241,7 @@ internal constructor( * This will temporarily open the collection during the operation if it was already closed */ private fun closeCollection() { - val instance = CollectionHelper.getInstance() - // this opens col if it wasn't closed - val col = instance.getCol(context) - col.close() + runBlocking { CollectionManager.ensureClosed() } } /** Converts the current AnkiDroid collection path to an [AnkiDroidDirectory] instance */ diff --git a/AnkiDroid/src/main/java/com/ichi2/libanki/ChangeManager.kt b/AnkiDroid/src/main/java/com/ichi2/libanki/ChangeManager.kt index 71f6ef98f7..dea1e800b2 100644 --- a/AnkiDroid/src/main/java/com/ichi2/libanki/ChangeManager.kt +++ b/AnkiDroid/src/main/java/com/ichi2/libanki/ChangeManager.kt @@ -29,11 +29,13 @@ package com.ichi2.libanki +import androidx.annotation.VisibleForTesting import anki.collection.OpChanges import anki.collection.OpChangesAfterUndo import anki.collection.OpChangesWithCount import anki.collection.OpChangesWithId import anki.import_export.ImportResponse +import com.ichi2.anki.CollectionManager.withCol import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.withContext import java.lang.ref.WeakReference @@ -69,7 +71,12 @@ object ChangeManager { } } - internal fun notifySubscribers(changes: T, initiator: Any?) { + @VisibleForTesting + fun clearSubscribers() { + subscribers.clear() + } + + internal fun notifySubscribers(changes: T, initiator: Any?) { val opChanges = when (changes) { is OpChanges -> changes is OpChangesWithCount -> changes.changes @@ -84,8 +91,10 @@ object ChangeManager { /** Wrap a routine that returns OpChanges* or similar undo info with this * to notify change subscribers of the changes. */ -suspend fun undoableOp(handler: Any? = null, block: () -> T): T { - return block().also { +suspend fun undoableOp(handler: Any? = null, block: CollectionV16.() -> T): T { + return withCol { + this.newBackend.block() + }.also { withContext(Dispatchers.Main) { ChangeManager.notifySubscribers(it, handler) } diff --git a/AnkiDroid/src/test/java/com/ichi2/anki/DeckPickerTest.kt b/AnkiDroid/src/test/java/com/ichi2/anki/DeckPickerTest.kt index 0a28c9948a..dfc53e7ca5 100644 --- a/AnkiDroid/src/test/java/com/ichi2/anki/DeckPickerTest.kt +++ b/AnkiDroid/src/test/java/com/ichi2/anki/DeckPickerTest.kt @@ -17,6 +17,7 @@ import com.ichi2.testutils.BackupManagerTestUtilities import com.ichi2.testutils.DbUtils import com.ichi2.utils.KotlinCleanup import com.ichi2.utils.ResourceLoader +import kotlinx.coroutines.runBlocking import net.ankiweb.rsdroid.BackendFactory import org.apache.commons.exec.OS import org.hamcrest.MatcherAssert.* @@ -319,14 +320,16 @@ class DeckPickerTest : RobolectricTest() { @Test @RunInBackground fun doNotShowOptionsMenuWhenCollectionInaccessible() { + skipWindows() try { enableNullCollection() val d = super.startActivityNormallyOpenCollectionWithIntent( DeckPickerEx::class.java, Intent() ) + runBlocking { d.createMenuJob?.join() } assertThat( "Options menu not displayed when collection is inaccessible", - d.prepareOptionsMenu, + d.optionsMenu?.hasVisibleItems(), equalTo(false) ) } finally { @@ -336,14 +339,16 @@ class DeckPickerTest : RobolectricTest() { @Test fun showOptionsMenuWhenCollectionAccessible() { + skipWindows() try { InitialActivityWithConflictTest.grantWritePermissions() val d = super.startActivityNormallyOpenCollectionWithIntent( DeckPickerEx::class.java, Intent() ) + runBlocking { d.createMenuJob?.join() } assertThat( - "Options menu is displayed when collection is accessible", - d.prepareOptionsMenu, + "Options menu displayed when collection is accessible", + d.optionsMenu?.hasVisibleItems(), equalTo(true) ) } finally { @@ -354,11 +359,14 @@ class DeckPickerTest : RobolectricTest() { @Test @RunInBackground fun doNotShowSyncBadgeWhenCollectionInaccessible() { + skipWindows() try { enableNullCollection() val d = super.startActivityNormallyOpenCollectionWithIntent( DeckPickerEx::class.java, Intent() ) + waitForAsyncTasksToComplete() + runBlocking { d.createMenuJob?.join() } assertThat( "Sync badge is not displayed when collection is inaccessible", d.displaySyncBadge, @@ -371,11 +379,14 @@ class DeckPickerTest : RobolectricTest() { @Test fun showSyncBadgeWhenCollectionAccessible() { + skipWindows() try { InitialActivityWithConflictTest.grantWritePermissions() val d = super.startActivityNormallyOpenCollectionWithIntent( DeckPickerEx::class.java, Intent() ) + waitForAsyncTasksToComplete() + runBlocking { d.createMenuJob?.join() } assertThat( "Sync badge is displayed when collection is accessible", d.displaySyncBadge, @@ -607,7 +618,7 @@ class DeckPickerTest : RobolectricTest() { private class DeckPickerEx : DeckPicker() { var databaseErrorDialog = 0 var displayedAnalyticsOptIn = false - var prepareOptionsMenu = false + var optionsMenu: Menu? = null var displaySyncBadge = false override fun showDatabaseErrorDialog(id: Int) { @@ -628,11 +639,11 @@ class DeckPickerTest : RobolectricTest() { } override fun onPrepareOptionsMenu(menu: Menu): Boolean { - prepareOptionsMenu = super.onPrepareOptionsMenu(menu) - return prepareOptionsMenu + optionsMenu = menu + return super.onPrepareOptionsMenu(menu) } - override fun displaySyncBadge(menu: Menu) { + override suspend fun displaySyncBadge(menu: Menu) { displaySyncBadge = true super.displaySyncBadge(menu) } diff --git a/AnkiDroid/src/test/java/com/ichi2/anki/RobolectricTest.kt b/AnkiDroid/src/test/java/com/ichi2/anki/RobolectricTest.kt index 95f1765d4a..80364e3c9b 100644 --- a/AnkiDroid/src/test/java/com/ichi2/anki/RobolectricTest.kt +++ b/AnkiDroid/src/test/java/com/ichi2/anki/RobolectricTest.kt @@ -55,6 +55,7 @@ import net.ankiweb.rsdroid.testing.RustBackendLoader import org.hamcrest.Matcher import org.hamcrest.MatcherAssert import org.hamcrest.Matchers +import org.hamcrest.Matchers.equalTo import org.junit.* import org.robolectric.Robolectric import org.robolectric.Shadows @@ -90,6 +91,8 @@ open class RobolectricTest : CollectionGetter { open fun setUp() { TimeManager.resetWith(MockTime(2020, 7, 7, 7, 0, 0, 0, 10)) + ChangeManager.clearSubscribers() + // resolved issues with the collection being reused if useInMemoryDatabase is false CollectionHelper.getInstance().setColForTests(null) @@ -168,6 +171,7 @@ open class RobolectricTest : CollectionGetter { TimeManager.reset() } + runBlocking { CollectionManager.discardBackend() } } /** @@ -312,6 +316,7 @@ open class RobolectricTest : CollectionGetter { /** Call this method in your test if you to test behavior with a null collection */ protected fun enableNullCollection() { + CollectionManager.closeCollectionBlocking() CollectionHelper.LazyHolder.INSTANCE = object : CollectionHelper() { override fun getCol(context: Context): Collection? { return null @@ -421,6 +426,12 @@ open class RobolectricTest : CollectionGetter { col } + /** The coroutine implemention on Windows/Robolectric seems to inexplicably hang sometimes */ + fun skipWindows() { + val name = System.getProperty("os.name") ?: "" + assumeThat(name.startsWith("Windows"), equalTo(false)) + } + @Throws(ConfirmModSchemaException::class) protected fun upgradeToSchedV2(): SchedV2 { col.changeSchedulerVer(2) diff --git a/AnkiDroid/src/test/java/com/ichi2/anki/dialogs/CreateDeckDialogTest.kt b/AnkiDroid/src/test/java/com/ichi2/anki/dialogs/CreateDeckDialogTest.kt index 0574fb0d3d..dbbe7adfe9 100644 --- a/AnkiDroid/src/test/java/com/ichi2/anki/dialogs/CreateDeckDialogTest.kt +++ b/AnkiDroid/src/test/java/com/ichi2/anki/dialogs/CreateDeckDialogTest.kt @@ -27,6 +27,7 @@ import com.ichi2.anki.R import com.ichi2.anki.RobolectricTest import com.ichi2.libanki.DeckManager import com.ichi2.libanki.backend.exception.DeckRenameException +import kotlinx.coroutines.runBlocking import org.hamcrest.CoreMatchers.equalTo import org.hamcrest.MatcherAssert import org.junit.Test @@ -149,6 +150,7 @@ class CreateDeckDialogTest : RobolectricTest() { @Test fun searchDecksIconVisibilityDeckCreationTest() { + skipWindows() mActivityScenario!!.onActivity { deckPicker -> val decks = deckPicker.col.decks val deckCounter = AtomicInteger(1) @@ -159,7 +161,7 @@ class CreateDeckDialogTest : RobolectricTest() { assertEquals(deckCounter.get(), decks.count()) - deckPicker.updateDeckList() + updateSearchDecksIcon(deckPicker) assertEquals(deckPicker.searchDecksIcon!!.isVisible, decks.count() >= 10) // After the last deck was created, delete a deck @@ -169,7 +171,7 @@ class CreateDeckDialogTest : RobolectricTest() { assertEquals(deckCounter.get(), decks.count()) - deckPicker.updateDeckList() + updateSearchDecksIcon(deckPicker) assertFalse(deckPicker.searchDecksIcon!!.isVisible) } } @@ -178,20 +180,31 @@ class CreateDeckDialogTest : RobolectricTest() { } } + private fun updateSearchDecksIcon(deckPicker: DeckPicker) { + deckPicker.updateDeckList() + // the icon normally is updated in the background usually; force it to update + // immediately so that the test can continue + runBlocking { + deckPicker.createMenuJob?.join() + deckPicker.updateSearchDecksIconVisibility() + } + } + @Test fun searchDecksIconVisibilitySubdeckCreationTest() { + skipWindows() mActivityScenario!!.onActivity { deckPicker -> var createDeckDialog = CreateDeckDialog(deckPicker, R.string.new_deck, CreateDeckDialog.DeckDialogType.DECK, null) val decks = deckPicker.col.decks createDeckDialog.setOnNewDeckCreated { assertEquals(10, decks.count()) - deckPicker.updateDeckList() + updateSearchDecksIcon(deckPicker) assertTrue(deckPicker.searchDecksIcon!!.isVisible) awaitJob(deckPicker.confirmDeckDeletion(decks.id("Deck0::Deck1"))) assertEquals(2, decks.count()) - deckPicker.updateDeckList() + updateSearchDecksIcon(deckPicker) assertFalse(deckPicker.searchDecksIcon!!.isVisible) } createDeckDialog.createDeck(deckTreeName(0, 8, "Deck")) @@ -199,13 +212,13 @@ class CreateDeckDialogTest : RobolectricTest() { createDeckDialog = CreateDeckDialog(deckPicker, R.string.new_deck, CreateDeckDialog.DeckDialogType.DECK, null) createDeckDialog.setOnNewDeckCreated { assertEquals(12, decks.count()) - deckPicker.updateDeckList() + updateSearchDecksIcon(deckPicker) assertTrue(deckPicker.searchDecksIcon!!.isVisible) awaitJob(deckPicker.confirmDeckDeletion(decks.id("Deck0::Deck1"))) assertEquals(2, decks.count()) - deckPicker.updateDeckList() + updateSearchDecksIcon(deckPicker) assertFalse(deckPicker.searchDecksIcon!!.isVisible) } createDeckDialog.createDeck(deckTreeName(0, 10, "Deck")) @@ -213,7 +226,7 @@ class CreateDeckDialogTest : RobolectricTest() { createDeckDialog = CreateDeckDialog(deckPicker, R.string.new_deck, CreateDeckDialog.DeckDialogType.DECK, null) createDeckDialog.setOnNewDeckCreated { assertEquals(6, decks.count()) - deckPicker.updateDeckList() + updateSearchDecksIcon(deckPicker) assertFalse(deckPicker.searchDecksIcon!!.isVisible) } createDeckDialog.createDeck(deckTreeName(0, 4, "Deck")) @@ -221,7 +234,7 @@ class CreateDeckDialogTest : RobolectricTest() { createDeckDialog = CreateDeckDialog(deckPicker, R.string.new_deck, CreateDeckDialog.DeckDialogType.DECK, null) createDeckDialog.setOnNewDeckCreated { assertEquals(12, decks.count()) - deckPicker.updateDeckList() + updateSearchDecksIcon(deckPicker) assertTrue(deckPicker.searchDecksIcon!!.isVisible) } createDeckDialog.createDeck(deckTreeName(6, 11, "Deck"))