From 55036522eaa5013f61918e6a210127105052a67a Mon Sep 17 00:00:00 2001 From: Damien Elmes Date: Sat, 6 Aug 2022 12:40:33 +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 | 312 ++++++++++++++++++ .../java/com/ichi2/anki/CoroutineHelpers.kt | 66 ++-- .../main/java/com/ichi2/anki/DatabaseCheck.kt | 14 +- .../main/java/com/ichi2/anki/DeckPicker.kt | 184 +++++------ .../main/java/com/ichi2/anki/Preferences.kt | 13 +- .../src/main/java/com/ichi2/anki/Sync.kt | 96 +++--- .../preferences/GeneralSettingsFragment.kt | 10 +- .../scopedstorage/MigrateEssentialFiles.kt | 7 +- .../java/com/ichi2/libanki/ChangeManager.kt | 15 +- .../java/com/ichi2/anki/DeckPickerTest.kt | 29 +- .../java/com/ichi2/anki/RobolectricTest.kt | 11 + .../anki/dialogs/CreateDeckDialogTest.kt | 29 +- 18 files changed, 590 insertions(+), 326 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 6f6d6803a88a..b7783d086f15 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 b258159cbed0..08df32ebf38f 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 06d78fc0795a..43510a41dfc6 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/BackendImporting.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/BackendImporting.kt @@ -19,16 +19,16 @@ package com.ichi2.anki import anki.import_export.ImportResponse +import com.ichi2.anki.CollectionManager.withCol import com.ichi2.libanki.exportAnkiPackage import com.ichi2.libanki.importAnkiPackage 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 @@ -36,7 +36,7 @@ fun DeckPicker.importApkgs(apkgPaths: List) { }, ) { undoableOp { - col.importAnkiPackage(apkgPath) + importAnkiPackage(apkgPath) } } showSimpleMessageDialog(summarizeReport(col.tr, report)) @@ -69,16 +69,17 @@ fun DeckPicker.exportApkg( withMedia: Boolean, deckId: Long? ) { - 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 ee302b20221b..76f3179f4e51 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 a8ee83f72e90..0d81c4d42f03 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/CardBrowser.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/CardBrowser.kt @@ -1282,8 +1282,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 872d3b2d9861..1c7d609f7b04 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 @@ * 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 static CollectionHelper getInstance() { */ 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 synchronized Collection getColSafe(Context context) { */ 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 enum CollectionOpenFailure { @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 000000000000..ee47f1508f29 --- /dev/null +++ b/AnkiDroid/src/main/java/com/ichi2/anki/CollectionManager.kt @@ -0,0 +1,312 @@ +/*************************************************************************************** + * 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 { + private var backend: Backend? = null + 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 { + _ensureOpen() + block(collection!!) + } + } + + /** + * Execute the provided block if the collection is already open. See [withCol] for more. + */ + 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 { + _ensureBackend() + 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 { + _discardBackend() + } + } + + private fun _discardBackend() { + _ensureClosed() + if (backend != null) { + backend!!.close() + backend = null + } + } + + /** + * Open the backend if it's not already open. + */ + private suspend fun ensureBackend() { + withQueue { + _ensureBackend() + } + } + + private fun _ensureBackend() { + if (backend == null) { + backend = BackendFactory.getBackend(AnkiDroidApp.getInstance()) + } + } + + /** + * If the collection is open, close it. + */ + suspend fun ensureClosed(save: Boolean = true) { + withQueue { + _ensureClosed(save = save) + } + } + + private fun _ensureClosed(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 { + _ensureOpen() + } + } + + private fun _ensureOpen() { + _ensureBackend() + if (collection == null || collection!!.dbClosed) { + val path = collectionPath() + collection = + collection(AnkiDroidApp.getInstance(), path, false, true, backend) + } + } + + fun collectionPath(): String { + val dir = CollectionHelper.getCurrentAnkiDroidDirectory(AnkiDroidApp.getInstance()) + CollectionHelper.initializeAnkiDroidDirectory(dir) + return File(dir, "collection.anki2").getAbsolutePath() + } + + @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) { + // fails when running robolectric test outside junit + false + } else { + throw exc + } + } + } + + // using TimeManager breaks a sched test that makes assumptions about the time + @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 + 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 ${elapsed}ms:\n%s", caller) + } + } + } + + /** + * True if the collection is open. Unsafe, as it has the potential to race. + */ + @JvmStatic + fun isOpenUnsafe(): Boolean { + return logUIHangs { + runBlocking { + withQueue { + collection != null && !collection!!.dbClosed + } + } + } + } + + @JvmStatic + fun setColForTests(col: Collection?) { + runBlocking { + withQueue { + if (col == null) { + _ensureClosed() + } + 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. + */ + suspend fun withNewSchema(block: CollectionV16.() -> T): T { + return withCol { + if (BackendFactory.defaultLegacySchema) { + // Temporarily update to the latest schema. + _discardBackend() + BackendFactory.defaultLegacySchema = false + _ensureOpen() + try { + (collection!! as CollectionV16).block() + } finally { + BackendFactory.defaultLegacySchema = true + _discardBackend() + } + } 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 { + _ensureClosed() + _ensureBackend() + importCollectionPackage(backend!!, collectionPath(), colpkgPath) + } + } +} diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/CoroutineHelpers.kt b/AnkiDroid/src/main/java/com/ichi2/anki/CoroutineHelpers.kt index 70c5b5f6c6eb..e4c3a9245571 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 440842f2e202..640ceb743501 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 88a4039ffe3c..47ef63fdead4 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt @@ -59,6 +59,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 @@ -202,6 +204,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) } @@ -569,83 +575,100 @@ 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) - 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 + } + + 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) + 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 - } - }) + // 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 + } - 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() } - updateSearchDecksIconVisibility() return super.onCreateOptionsMenu(menu) } - private fun updateSearchDecksIconVisibility() { - searchDecksIcon?.isVisible = colIsOpen() && col.decks.count() >= 10 + @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) @@ -1230,8 +1253,8 @@ open class DeckPicker : if (BackendFactory.defaultLegacySchema) { legacyUndo() } else { - launchCatchingCollectionTask { col -> - if (!backendUndoAndShowPopup(col)) { + launchCatchingTask { + if (!backendUndoAndShowPopup()) { legacyUndo() } } @@ -1960,8 +1983,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() @@ -2204,7 +2227,9 @@ open class DeckPicker : mFocusedDeck = current } - updateSearchDecksIconVisibility() + launchCatchingTask { + updateSearchDecksIconVisibility() + } } // Callback to show study options for currently selected deck @@ -2275,15 +2300,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 ) } @@ -2687,32 +2712,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 5516754cf858..0527e8e045c7 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/Preferences.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/Preferences.kt @@ -132,13 +132,12 @@ class Preferences : AnkiActivity() { } 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) + } } override fun onSaveInstanceState(outState: Bundle) { diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/Sync.kt b/AnkiDroid/src/main/java/com/ichi2/anki/Sync.kt index ea74974e3ca3..a868630fdb90 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 0dcc9904b834..e4d223b6868c 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/preferences/GeneralSettingsFragment.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/preferences/GeneralSettingsFragment.kt @@ -18,12 +18,10 @@ package com.ichi2.anki.preferences import androidx.preference.ListPreference import androidx.preference.PreferenceCategory import androidx.preference.SwitchPreference -import com.ichi2.anki.CollectionHelper -import com.ichi2.anki.CrashReportService -import com.ichi2.anki.Preferences -import com.ichi2.anki.R +import com.ichi2.anki.* import com.ichi2.utils.AdaptionUtil.isRestrictedLearningDevice import com.ichi2.utils.LanguageUtil +import kotlinx.coroutines.runBlocking import java.util.* class GeneralSettingsFragment : SettingsFragment() { @@ -95,9 +93,7 @@ class GeneralSettingsFragment : SettingsFragment() { // TODO recreate the activity and keep its previous state instead of just closing it languageSelection.setOnPreferenceChangeListener { _, newValue -> LanguageUtil.setDefaultBackendLanguages(newValue as String) - val helper = CollectionHelper.getInstance() - helper.closeCollection(true, "language change") - helper.discardBackend() + runBlocking { CollectionManager.discardBackend() } (requireActivity() as Preferences).closePreferences() true } 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 4f8cbdc05bfb..09414e75a9c9 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 71f6ef98f7a0..dea1e800b2e3 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 9e8af07fdf1b..e1eae01e23eb 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.* @@ -320,15 +321,17 @@ 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, - `is`(false) + d.optionsMenu?.hasVisibleItems(), + equalTo(false) ) } finally { disableNullCollection() @@ -337,15 +340,17 @@ 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, - `is`(true) + "Options menu displayed when collection is accessible", + d.optionsMenu?.hasVisibleItems(), + equalTo(true) ) } finally { InitialActivityWithConflictTest.revokeWritePermissions() @@ -355,11 +360,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, @@ -372,11 +380,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, @@ -614,7 +625,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) { @@ -635,11 +646,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 2d7190daa011..bf0cbfae2465 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 839f52da905c..cb33fd88465d 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.MatcherAssert import org.hamcrest.core.Is 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"))