From 31fd3349fe89f2cddbc4ac1cac4f99b4b8a0e5e8 Mon Sep 17 00:00:00 2001 From: Damien Elmes Date: Mon, 8 Aug 2022 13:29:20 +1000 Subject: [PATCH 01/19] 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 | 194 +++++----- .../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, 626 insertions(+), 331 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 2fcad5129479..0ef5e04aeecd 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 7880fe217cc0..2d619ed7f34c 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 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 227b1966d6fa..77e6bca1765d 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 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..d5b5ec6a0dc1 --- /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 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 fd5c75dc2f8e..7304c8224680 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 + } + + 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 + } - // 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() } 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 64d6507fb98e..e1da0d212273 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 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 f3e2755bd004..42d4a4593430 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 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 0a28c9948a3d..dfc53e7ca5ff 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 95f1765d4aca..80364e3c9b0b 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 0574fb0d3dc8..dbbe7adfe93f 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")) From e00e4db444de2d8456b60f342bcb20124d7e50bc Mon Sep 17 00:00:00 2001 From: Damien Elmes Date: Thu, 14 Jul 2022 21:09:32 +1000 Subject: [PATCH 02/19] Hook apkg/colpkg exports up to UI --- .../java/com/ichi2/anki/BackendImporting.kt | 38 +++++++++++++------ .../anki/export/ActivityExportingDelegate.kt | 19 +++++----- .../com/ichi2/libanki/BackendImportExport.kt | 7 ++-- 3 files changed, 41 insertions(+), 23 deletions(-) diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/BackendImporting.kt b/AnkiDroid/src/main/java/com/ichi2/anki/BackendImporting.kt index 2d619ed7f34c..e627b6ba765d 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/BackendImporting.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/BackendImporting.kt @@ -22,6 +22,7 @@ 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.exportCollectionPackage import com.ichi2.libanki.importAnkiPackage import com.ichi2.libanki.undoableOp import net.ankiweb.rsdroid.Translations @@ -64,23 +65,38 @@ private fun summarizeReport(tr: Translations, output: ImportResponse): String { return msgs.joinToString("\n") } -fun DeckPicker.exportApkg( +suspend fun AnkiActivity.exportApkg( apkgPath: String, withScheduling: Boolean, withMedia: Boolean, deckId: DeckId? ) { - launchCatchingTask { - withProgress( - extractProgress = { - if (progress.hasExporting()) { - text = progress.exporting - } - }, - ) { - withCol { - newBackend.exportAnkiPackage(apkgPath, withScheduling, withMedia, deckId) + withProgress( + extractProgress = { + if (progress.hasExporting()) { + text = progress.exporting + } + }, + ) { + withCol { + newBackend.exportAnkiPackage(apkgPath, withScheduling, withMedia, deckId) + } + } +} + +suspend fun AnkiActivity.exportColpkg( + colpkgPath: String, + withMedia: Boolean, +) { + withProgress( + extractProgress = { + if (progress.hasExporting()) { + text = progress.exporting } + }, + ) { + withCol { + newBackend.exportCollectionPackage(colpkgPath, withMedia, true) } } } diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/export/ActivityExportingDelegate.kt b/AnkiDroid/src/main/java/com/ichi2/anki/export/ActivityExportingDelegate.kt index 5a1d6a96289f..1bc2be8307b8 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/export/ActivityExportingDelegate.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/export/ActivityExportingDelegate.kt @@ -25,14 +25,11 @@ import androidx.activity.result.contract.ActivityResultContracts import androidx.annotation.StringRes import androidx.core.app.ShareCompat.IntentBuilder import androidx.core.content.FileProvider -import com.ichi2.anki.AnkiActivity -import com.ichi2.anki.DeckPicker -import com.ichi2.anki.R +import com.ichi2.anki.* import com.ichi2.anki.UIUtils.showSimpleSnackbar import com.ichi2.anki.UIUtils.showThemedToast import com.ichi2.anki.dialogs.ExportCompleteDialog.ExportCompleteDialogListener import com.ichi2.anki.dialogs.ExportDialog.ExportDialogListener -import com.ichi2.anki.exportApkg import com.ichi2.async.CollectionTask.ExportApkg import com.ichi2.async.TaskManager import com.ichi2.compat.CompatHelper @@ -103,11 +100,15 @@ class ActivityExportingDelegate(private val activity: AnkiActivity, private val exportListener ) } else { - // TODO: this code needs reworking so that the post-export dialogs can be - // shown correctly - (activity as DeckPicker).exportApkg(exportPath.path, includeSched, includeMedia, did) - // exportListener.actualOnPreExecute(activity) - // exportListener.actualOnPostExecute(activity, android.util.Pair(false, exportPath.path)) + activity.launchCatchingTask { + if (did == null && includeSched) { + activity.exportColpkg(exportPath.path, includeMedia) + } else { + activity.exportApkg(exportPath.path, includeSched, includeMedia, did) + } + val dialog = mDialogsFactory.newExportCompleteDialog().withArguments(exportPath.path) + activity.showAsyncDialogFragment(dialog) + } } } diff --git a/AnkiDroid/src/main/java/com/ichi2/libanki/BackendImportExport.kt b/AnkiDroid/src/main/java/com/ichi2/libanki/BackendImportExport.kt index c6fbb154e1b1..b584d0d01932 100644 --- a/AnkiDroid/src/main/java/com/ichi2/libanki/BackendImportExport.kt +++ b/AnkiDroid/src/main/java/com/ichi2/libanki/BackendImportExport.kt @@ -74,9 +74,8 @@ fun importCollectionPackage( } /** - * Export the collection into a .colpkg file. This closes the collection (since it may need to - * downgrade the schema for a legacy export), and col.reopen() must be called afterwards. - * * If legacy=false, a file targeting Anki 2.1.50+ is created. It compresses better and is faster to + * Export the collection into a .colpkg file. + * If legacy=false, a file targeting Anki 2.1.50+ is created. It compresses better and is faster to * create, but older clients can not read it. */ fun CollectionV16.exportCollectionPackage( @@ -84,11 +83,13 @@ fun CollectionV16.exportCollectionPackage( includeMedia: Boolean, legacy: Boolean = true ) { + close(save = true, downgrade = false, forFullSync = true) backend.exportCollectionPackage( outPath = outPath, includeMedia = includeMedia, legacy = legacy ) + reopen(afterFullSync = false) } /** From 631588301681ead4070e9ac2915130e61ffc0e9e Mon Sep 17 00:00:00 2001 From: Damien Elmes Date: Fri, 22 Jul 2022 23:24:56 +1000 Subject: [PATCH 03/19] Use backend to update sync status; trigger media sync on auto full sync + Update sync status after normal media sync completes Closes #11902 --- .../main/java/com/ichi2/anki/DeckPicker.kt | 14 +++++--- .../src/main/java/com/ichi2/anki/Sync.kt | 26 +++++++++----- .../com/ichi2/libanki/sync/BackendSync.kt | 5 +++ .../main/java/com/ichi2/utils/SyncStatus.kt | 34 +++++++++---------- 4 files changed, 49 insertions(+), 30 deletions(-) diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt b/AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt index 7304c8224680..6056386f2809 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt @@ -665,15 +665,20 @@ open class DeckPicker : @VisibleForTesting protected open suspend fun displaySyncBadge(menu: Menu) { - val syncStatus = withOpenColOrNull { SyncStatus.getSyncStatus(this) } + val auth = syncAuth() + val syncStatus = withOpenColOrNull { + SyncStatus.getSyncStatus(this, auth) + } if (syncStatus == null) { return } val syncMenu = menu.findItem(R.id.action_sync) when (syncStatus) { SyncStatus.BADGE_DISABLED, SyncStatus.NO_CHANGES, SyncStatus.INCONCLUSIVE -> { - BadgeDrawableBuilder.removeBadge(syncMenu) - syncMenu.setTitle(R.string.button_sync) + syncMenu?.let { + BadgeDrawableBuilder.removeBadge(it) + it.setTitle(R.string.button_sync) + } } SyncStatus.HAS_CHANGES -> { // Light orange icon @@ -1521,14 +1526,13 @@ open class DeckPicker : override fun sync(conflict: ConflictResolution?) { val preferences = AnkiDroidApp.getSharedPrefs(baseContext) val hkey = preferences.getString("hkey", "") - val hostNum = HostNumFactory.getInstance(baseContext).getHostNum() if (hkey!!.isEmpty()) { Timber.w("User not logged in") mPullToSyncWrapper.isRefreshing = false showSyncErrorDialog(SyncErrorDialog.DIALOG_USER_NOT_LOGGED_IN_SYNC) } else { if (!BackendFactory.defaultLegacySchema) { - handleNewSync(hkey, hostNum ?: 0, conflict) + handleNewSync(conflict) } else { Connection.sync( mSyncListener, diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/Sync.kt b/AnkiDroid/src/main/java/com/ichi2/anki/Sync.kt index a868630fdb90..272989a4ecc0 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/Sync.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/Sync.kt @@ -42,15 +42,22 @@ import net.ankiweb.rsdroid.Backend import net.ankiweb.rsdroid.exceptions.BackendSyncException import timber.log.Timber +fun DeckPicker.syncAuth(): SyncAuth? { + val preferences = AnkiDroidApp.getSharedPrefs(this) + val hkey = preferences.getString("hkey", null) + val hostNum = HostNumFactory.getInstance(baseContext).getHostNum() + return hkey?.let { + syncAuth { + this.hkey = hkey + this.hostNumber = hostNum ?: 0 + } + } +} + fun DeckPicker.handleNewSync( - hkey: String, - hostNum: Int, conflict: Connection.ConflictResolution? ) { - val auth = syncAuth { - this.hkey = hkey - this.hostNumber = hostNum - } + val auth = this.syncAuth() ?: return val deckPicker = this launchCatchingTask { try { @@ -121,6 +128,7 @@ private suspend fun handleNormalSync( SyncCollectionResponse.ChangesRequired.NO_CHANGES -> { // a successful sync returns this value deckPicker.showSyncLogMessage(R.string.sync_database_acknowledge, output.serverMessage) + deckPicker.refreshState() // kick off media sync - future implementations may want to run this in the // background instead handleMediaSync(deckPicker, auth) @@ -128,12 +136,10 @@ private suspend fun handleNormalSync( SyncCollectionResponse.ChangesRequired.FULL_DOWNLOAD -> { handleDownload(deckPicker, auth) - handleMediaSync(deckPicker, auth) } SyncCollectionResponse.ChangesRequired.FULL_UPLOAD -> { handleUpload(deckPicker, auth) - handleMediaSync(deckPicker, auth) } SyncCollectionResponse.ChangesRequired.FULL_SYNC -> { @@ -178,6 +184,8 @@ private suspend fun handleDownload( reopen(afterFullSync = true) } } + deckPicker.refreshState() + handleMediaSync(deckPicker, auth) } Timber.i("Full Download Completed") @@ -200,6 +208,8 @@ private suspend fun handleUpload( reopen(afterFullSync = true) } } + deckPicker.refreshState() + handleMediaSync(deckPicker, auth) } Timber.i("Full Upload Completed") deckPicker.showSyncLogMessage(R.string.sync_log_uploading_message, "") diff --git a/AnkiDroid/src/main/java/com/ichi2/libanki/sync/BackendSync.kt b/AnkiDroid/src/main/java/com/ichi2/libanki/sync/BackendSync.kt index 02cd6b5ccc0e..1f6ace09b6c4 100644 --- a/AnkiDroid/src/main/java/com/ichi2/libanki/sync/BackendSync.kt +++ b/AnkiDroid/src/main/java/com/ichi2/libanki/sync/BackendSync.kt @@ -18,6 +18,7 @@ package com.ichi2.libanki.sync import anki.sync.SyncAuth import anki.sync.SyncCollectionResponse +import anki.sync.SyncStatusResponse import com.ichi2.libanki.CollectionV16 fun CollectionV16.syncLogin(username: String, password: String): SyncAuth { @@ -39,3 +40,7 @@ fun CollectionV16.fullDownload(auth: SyncAuth) { fun CollectionV16.syncMedia(auth: SyncAuth) { return backend.syncMedia(input = auth) } + +fun CollectionV16.syncStatus(auth: SyncAuth): SyncStatusResponse { + return backend.syncStatus(input = auth) +} diff --git a/AnkiDroid/src/main/java/com/ichi2/utils/SyncStatus.kt b/AnkiDroid/src/main/java/com/ichi2/utils/SyncStatus.kt index eac41f2acbf0..bac7844847a5 100644 --- a/AnkiDroid/src/main/java/com/ichi2/utils/SyncStatus.kt +++ b/AnkiDroid/src/main/java/com/ichi2/utils/SyncStatus.kt @@ -16,10 +16,11 @@ package com.ichi2.utils +import anki.sync.SyncAuth +import anki.sync.SyncStatusResponse import com.ichi2.anki.AnkiDroidApp import com.ichi2.libanki.Collection -import timber.log.Timber -import java.util.function.Supplier +import net.ankiweb.rsdroid.BackendFactory enum class SyncStatus { INCONCLUSIVE, NO_ACCOUNT, NO_CHANGES, HAS_CHANGES, FULL_SYNC, BADGE_DISABLED; @@ -29,26 +30,16 @@ enum class SyncStatus { private var sMarkedInMemory = false @JvmStatic - fun getSyncStatus(getCol: Supplier): SyncStatus { - return try { - val col = getCol.get() - // may fail when the collection is closed for a full sync, - // as col.db is null - getSyncStatus(col) - } catch (e: Exception) { - Timber.w(e) - return INCONCLUSIVE - } - } - - @JvmStatic - fun getSyncStatus(col: Collection): SyncStatus { + fun getSyncStatus(col: Collection, auth: SyncAuth?): SyncStatus { if (isDisabled) { return BADGE_DISABLED } - if (!isLoggedIn) { + if (auth == null) { return NO_ACCOUNT } + if (!BackendFactory.defaultLegacySchema) { + return syncStatusFromRequired(col.newBackend.backend.syncStatus(auth).required) + } if (col.schemaChanged()) { return FULL_SYNC } @@ -59,6 +50,15 @@ enum class SyncStatus { } } + private fun syncStatusFromRequired(required: SyncStatusResponse.Required?): SyncStatus { + return when (required) { + SyncStatusResponse.Required.NO_CHANGES -> NO_CHANGES + SyncStatusResponse.Required.NORMAL_SYNC -> HAS_CHANGES + SyncStatusResponse.Required.FULL_SYNC -> FULL_SYNC + SyncStatusResponse.Required.UNRECOGNIZED, null -> TODO("unexpected required response") + } + } + private val isDisabled: Boolean get() { val preferences = AnkiDroidApp.getSharedPrefs(AnkiDroidApp.getInstance()) From 54fbca29317fd985a3ed1c0eac7822b1b1b90a98 Mon Sep 17 00:00:00 2001 From: Damien Elmes Date: Thu, 28 Jul 2022 14:19:32 +1000 Subject: [PATCH 04/19] Skip a flaky v3 test for 2 hours of the day --- .../src/test/java/com/ichi2/libanki/sched/SchedV2Test.kt | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/AnkiDroid/src/test/java/com/ichi2/libanki/sched/SchedV2Test.kt b/AnkiDroid/src/test/java/com/ichi2/libanki/sched/SchedV2Test.kt index 49e95ba2ba32..f2890c85422b 100644 --- a/AnkiDroid/src/test/java/com/ichi2/libanki/sched/SchedV2Test.kt +++ b/AnkiDroid/src/test/java/com/ichi2/libanki/sched/SchedV2Test.kt @@ -58,6 +58,8 @@ import org.junit.Test import org.junit.platform.commons.util.CollectionUtils import org.junit.runner.RunWith import java.lang.Exception +import java.time.Instant +import java.time.ZoneOffset import java.util.* import kotlin.Throws import kotlin.math.roundToLong @@ -447,6 +449,11 @@ open class SchedV2Test : RobolectricTest() { @Test @Throws(Exception::class) fun test_learnV2() { + if (v3 && Instant.now().atZone(ZoneOffset.UTC).getHour().let { it >= 2 && it < 4 }) { + // The backend shifts the current time around rollover, and expects the frontend to + // do so as well. This could potentially be done with TimeManager in the future. + assumeThat(v3, equalTo(false)) + } TimeManager.reset() val col = colV2 // add a note From 831572e0df332cfe8ff03d5bbf28c364b1e2c160 Mon Sep 17 00:00:00 2001 From: Damien Elmes Date: Thu, 28 Jul 2022 15:23:04 +1000 Subject: [PATCH 05/19] Add script to test all commits in a branch It skips lint-rules:test, as it still doesn't seem to work for me. --- tools/test-commits.sh | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) create mode 100755 tools/test-commits.sh diff --git a/tools/test-commits.sh b/tools/test-commits.sh new file mode 100755 index 000000000000..1d97eb584099 --- /dev/null +++ b/tools/test-commits.sh @@ -0,0 +1,19 @@ +#!/bin/bash +# Walk backwards from HEAD, testing each commit until the +# provided first commit (inclusive). + +set -e + +if [ "$1" = "" ]; then + echo "usage: test-commits.sh [first-commit-hash]" + exit 1 +fi + +first_commit="$1" + +while :; do + echo "testing $(git log --pretty=oneline -1)" + ./gradlew -q clean uninstallPlayDebug jacocoTestReport :api:lintRelease :AnkiDroid:lintPlayRelease ktlintCheck + [ $(git rev-parse HEAD) = $first_commit ] && break + git checkout HEAD^ +done From 23ba93183599ed3ba169e3a9e96508156d284112 Mon Sep 17 00:00:00 2001 From: Damien Elmes Date: Sun, 7 Aug 2022 22:42:58 +1000 Subject: [PATCH 06/19] Fix crash after changing theme Closes #11981 --- AnkiDroid/src/main/java/com/ichi2/anki/CoroutineHelpers.kt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/CoroutineHelpers.kt b/AnkiDroid/src/main/java/com/ichi2/anki/CoroutineHelpers.kt index e4c3a9245571..0762ae7020c9 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/CoroutineHelpers.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/CoroutineHelpers.kt @@ -42,6 +42,8 @@ fun AnkiActivity.launchCatchingTask( return lifecycle.coroutineScope.launch { try { block() + } catch (exc: CancellationException) { + // do nothing } catch (exc: BackendInterruptedException) { Timber.e("caught: %s", exc) showSimpleSnackbar(this@launchCatchingTask, exc.localizedMessage, false) From 3f221ed6a342a7752c92542e3df3bf726edcf767 Mon Sep 17 00:00:00 2001 From: Damien Elmes Date: Sun, 7 Aug 2022 23:55:28 +1000 Subject: [PATCH 07/19] Fix play icons not appearing with V16 Closes #11979 --- .../com/ichi2/anki/AbstractFlashcardViewer.kt | 23 +++++++++++-- .../com/ichi2/anki/cardviewer/CardHtml.kt | 11 ++++--- .../src/main/java/com/ichi2/libanki/Card.kt | 8 +++++ .../main/java/com/ichi2/libanki/Collection.kt | 4 +-- .../main/java/com/ichi2/libanki/Sound.java | 9 ++++- .../src/main/java/com/ichi2/libanki/Sound.kt | 33 +++++++++++++++++++ .../java/com/ichi2/libanki/TemplateManager.kt | 6 ++-- 7 files changed, 80 insertions(+), 14 deletions(-) diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/AbstractFlashcardViewer.kt b/AnkiDroid/src/main/java/com/ichi2/anki/AbstractFlashcardViewer.kt index 0ef5e04aeecd..8f11d28c4b75 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/AbstractFlashcardViewer.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/AbstractFlashcardViewer.kt @@ -92,6 +92,7 @@ import com.ichi2.ui.FixedEditText import com.ichi2.utils.AdaptionUtil.hasWebBrowser import com.ichi2.utils.AndroidUiUtils.isRunningOnTv import com.ichi2.utils.AssetHelper.guessMimeType +import com.ichi2.utils.BlocksSchemaUpgrade import com.ichi2.utils.ClipboardUtil.getText import com.ichi2.utils.Computation import com.ichi2.utils.HandlerUtils.executeFunctionWithDelay @@ -2257,7 +2258,9 @@ abstract class AbstractFlashcardViewer : // We play sounds through these links when a user taps the sound icon. fun filterUrl(url: String): Boolean { if (url.startsWith("playsound:")) { - controlSound(url) + launchCatchingTask { + controlSound(url) + } return true } if (url.startsWith("file") || url.startsWith("data:")) { @@ -2450,8 +2453,22 @@ abstract class AbstractFlashcardViewer : * Also, Check if the user clicked on the running audio icon * @param url */ - private fun controlSound(url: String) { - val replacedUrl = url.replaceFirst("playsound:".toRegex(), "") + @BlocksSchemaUpgrade("handle TTS tags") + private suspend fun controlSound(url: String) { + val replacedUrl = if (BackendFactory.defaultLegacySchema) { + url.replaceFirst("playsound:".toRegex(), "") + } else { + val tag = currentCard?.let { getAvTag(it, url) } + val filename = when (tag) { + is SoundOrVideoTag -> tag.filename + // not currently supported + is TTSTag -> null + else -> null + } + filename?.let { + Sound.getSoundPath(mBaseUrl, it) + } ?: return + } if (replacedUrl != mSoundPlayer.currentAudioUri || mSoundPlayer.isCurrentAudioFinished) { onCurrentAudioChanged(replacedUrl) } else { diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/cardviewer/CardHtml.kt b/AnkiDroid/src/main/java/com/ichi2/anki/cardviewer/CardHtml.kt index 5d342dfa8450..9fece041fba3 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/cardviewer/CardHtml.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/cardviewer/CardHtml.kt @@ -25,6 +25,7 @@ import com.ichi2.libanki.template.MathJax import com.ichi2.themes.HtmlColors import com.ichi2.themes.Themes.currentTheme import com.ichi2.utils.JSONObject +import net.ankiweb.rsdroid.BackendFactory import net.ankiweb.rsdroid.RustCleanup import timber.log.Timber import java.util.regex.Pattern @@ -43,7 +44,6 @@ class CardHtml( @RustCleanup("too many variables, combine once we move away from backend") private var questionSound: List? = null, private var answerSound: List? = null, - private val usingBackend: Boolean = answerSound != null ) { fun getSoundTags(sideFor: Side): List { if (sideFor == this.side) { @@ -136,9 +136,12 @@ class CardHtml( val renderOutput = card.render_output() val questionAv = renderOutput.question_av_tags val answerAv = renderOutput.answer_av_tags - - val questionSound = questionAv?.filterIsInstance(SoundOrVideoTag::class.java) - val answerSound = answerAv?.filterIsInstance(SoundOrVideoTag::class.java) + var questionSound: List? = null + var answerSound: List? = null + if (!BackendFactory.defaultLegacySchema) { + questionSound = questionAv.filterIsInstance(SoundOrVideoTag::class.java) + answerSound = answerAv.filterIsInstance(SoundOrVideoTag::class.java) + } // legacy (slow) function to return the answer without the front side fun getAnswerWithoutFrontSideLegacy(): String = removeFrontSideAudio(card, card.a()) diff --git a/AnkiDroid/src/main/java/com/ichi2/libanki/Card.kt b/AnkiDroid/src/main/java/com/ichi2/libanki/Card.kt index 1c5ea8b024d8..16554243cd14 100644 --- a/AnkiDroid/src/main/java/com/ichi2/libanki/Card.kt +++ b/AnkiDroid/src/main/java/com/ichi2/libanki/Card.kt @@ -243,6 +243,14 @@ open class Card : Cloneable { return "" } + fun questionAvTags(): List { + return render_output().question_av_tags + } + + fun answerAvTags(): List { + return render_output().answer_av_tags + } + /** * @throws net.ankiweb.rsdroid.exceptions.BackendInvalidInputException: If the card does not exist */ diff --git a/AnkiDroid/src/main/java/com/ichi2/libanki/Collection.kt b/AnkiDroid/src/main/java/com/ichi2/libanki/Collection.kt index 2265ef8e6973..6756272a67e1 100644 --- a/AnkiDroid/src/main/java/com/ichi2/libanki/Collection.kt +++ b/AnkiDroid/src/main/java/com/ichi2/libanki/Collection.kt @@ -1512,8 +1512,8 @@ open class Collection( return TemplateRenderOutput( qa["q"]!!, qa["a"]!!, - null, - null, + listOf(), + listOf(), c.model().getString("css") ) } diff --git a/AnkiDroid/src/main/java/com/ichi2/libanki/Sound.java b/AnkiDroid/src/main/java/com/ichi2/libanki/Sound.java index aa089fe3da64..a806be548b71 100644 --- a/AnkiDroid/src/main/java/com/ichi2/libanki/Sound.java +++ b/AnkiDroid/src/main/java/com/ichi2/libanki/Sound.java @@ -39,6 +39,8 @@ import com.ichi2.utils.DisplayUtils; import com.ichi2.utils.StringUtil; +import net.ankiweb.rsdroid.BackendFactory; + import java.lang.ref.WeakReference; import java.util.ArrayList; import java.util.Arrays; @@ -54,6 +56,8 @@ import androidx.annotation.VisibleForTesting; import timber.log.Timber; +import static com.ichi2.libanki.SoundKt.addPlayIcons; + //NICE_TO_HAVE: Abstract, then add tests fir #6111 /** @@ -230,6 +234,9 @@ private Boolean makeQuestionAnswerList() { */ @NonNull public static String expandSounds(String soundDir, String content) { + if (!BackendFactory.getDefaultLegacySchema()) { + return addPlayIcons(content); + } StringBuilder stringBuilder = new StringBuilder(); String contentLeft = content; @@ -570,7 +577,7 @@ public void stopSounds() { * @param sound -- path to the sound file from the card content. * @return absolute URI to the sound file. */ - private static String getSoundPath(String soundDir, String sound) { + public static String getSoundPath(String soundDir, String sound) { String trimmedSound = sound.trim(); if (hasURIScheme(trimmedSound)) { return trimmedSound; diff --git a/AnkiDroid/src/main/java/com/ichi2/libanki/Sound.kt b/AnkiDroid/src/main/java/com/ichi2/libanki/Sound.kt index 800e3dfab107..cef37b2574df 100644 --- a/AnkiDroid/src/main/java/com/ichi2/libanki/Sound.kt +++ b/AnkiDroid/src/main/java/com/ichi2/libanki/Sound.kt @@ -24,6 +24,8 @@ package com.ichi2.libanki +import com.ichi2.anki.CollectionManager.withCol + /** * Records information about a text to speech tag. */ @@ -52,5 +54,36 @@ open class AvTag /* Methods */ val AV_REF_RE = Regex("\\[anki:(play:(.):(\\d+))]") +val AV_PLAYLINK_RE = Regex("playsound:(.):(\\d+)") fun strip_av_refs(text: str) = AV_REF_RE.replace("", text) + +fun addPlayIcons(content: String): String { + return AV_REF_RE.replace(content) { match -> + val groups = match.groupValues + val side = groups[2] + val index = groups[3] + val playsound = "playsound:$side:$index" + """ + + Replay + """ + } +} + +/** Extract av tag from playsound:q:x link */ +suspend fun getAvTag(card: Card, url: String): AvTag? { + return AV_PLAYLINK_RE.matchEntire(url)?.let { + val values = it.groupValues + val questionSide = values[1] == "q" + val index = values[2].toInt() + val tags = withCol { + if (questionSide) { card.questionAvTags() } else { card.answerAvTags() } + } + if (index < tags.size) { + tags[index] + } else { + null + } + } +} diff --git a/AnkiDroid/src/main/java/com/ichi2/libanki/TemplateManager.kt b/AnkiDroid/src/main/java/com/ichi2/libanki/TemplateManager.kt index 402f948d49de..39566e52eb1a 100644 --- a/AnkiDroid/src/main/java/com/ichi2/libanki/TemplateManager.kt +++ b/AnkiDroid/src/main/java/com/ichi2/libanki/TemplateManager.kt @@ -265,10 +265,8 @@ class TemplateManager { @get:JvmName("getAnswerText") @set:JvmName("setAnswerText") var answer_text: str, - @RustCleanup("make non-null") - val question_av_tags: List?, - @RustCleanup("make non-null") - val answer_av_tags: List?, + val question_av_tags: List, + val answer_av_tags: List, val css: str = "" ) { From d8be7e8b62e65337781289ba135dfb1d67cc417b Mon Sep 17 00:00:00 2001 From: Damien Elmes Date: Mon, 8 Aug 2022 17:29:40 +1000 Subject: [PATCH 08/19] Re-enable card template editor GUI in V16 schema The unit tests were re-enabled some time back, but this was missed. --- AnkiDroid/src/main/java/com/ichi2/anki/NoteEditor.kt | 7 ------- 1 file changed, 7 deletions(-) diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/NoteEditor.kt b/AnkiDroid/src/main/java/com/ichi2/anki/NoteEditor.kt index 9784a23e1edc..687b4b796ce7 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/NoteEditor.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/NoteEditor.kt @@ -43,7 +43,6 @@ import androidx.annotation.CheckResult import androidx.annotation.RequiresApi import androidx.annotation.StringRes import androidx.annotation.VisibleForTesting -import androidx.appcompat.app.AlertDialog import androidx.appcompat.widget.AppCompatButton import androidx.appcompat.widget.PopupMenu import androidx.core.content.res.ResourcesCompat @@ -93,7 +92,6 @@ import com.ichi2.themes.StyledProgressDialog import com.ichi2.themes.Themes import com.ichi2.utils.* import com.ichi2.widget.WidgetStatus -import net.ankiweb.rsdroid.BackendFactory import timber.log.Timber import java.util.* import java.util.function.Consumer @@ -1136,11 +1134,6 @@ class NoteEditor : AnkiActivity(), DeckSelectionListener, SubtitleListener, Tags } private fun showCardTemplateEditor() { - if (!BackendFactory.defaultLegacySchema) { - // this screen needs rewriting for the new backend - AlertDialog.Builder(this).setTitle("Not yet supported on new backend").show() - return - } val intent = Intent(this, CardTemplateEditor::class.java) // Pass the model ID intent.putExtra("modelId", currentlySelectedModel!!.getLong("id")) From 3108e881c3a542a65e36a348963e2616298c7316 Mon Sep 17 00:00:00 2001 From: Damien Elmes Date: Mon, 8 Aug 2022 17:30:20 +1000 Subject: [PATCH 09/19] Rely on the backend to fetch the current deck id Fixes a startup crash when the current deck ID is set to an invalid value --- AnkiDroid/src/main/java/com/ichi2/libanki/DecksV16.kt | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/AnkiDroid/src/main/java/com/ichi2/libanki/DecksV16.kt b/AnkiDroid/src/main/java/com/ichi2/libanki/DecksV16.kt index 621175ca4d4a..061ae3b2540b 100644 --- a/AnkiDroid/src/main/java/com/ichi2/libanki/DecksV16.kt +++ b/AnkiDroid/src/main/java/com/ichi2/libanki/DecksV16.kt @@ -40,7 +40,6 @@ import anki.collection.OpChangesWithCount import anki.collection.OpChangesWithId import com.google.protobuf.ByteString import com.ichi2.libanki.Decks.ACTIVE_DECKS -import com.ichi2.libanki.Decks.CURRENT_DECK import com.ichi2.libanki.Utils.ids2str import com.ichi2.libanki.backend.BackendUtils import com.ichi2.libanki.backend.exception.DeckRenameException @@ -668,7 +667,7 @@ class DecksV16(private val col: CollectionV16) : /** The currently selected did. */ override fun selected(): DeckId { - return this.col.get_config_long(CURRENT_DECK) + return this.col.backend.getCurrentDeck().id } override fun current(): Deck { From c2f8fe812b459f7955dfc1322d1a62be5612d273 Mon Sep 17 00:00:00 2001 From: Damien Elmes Date: Thu, 11 Aug 2022 09:36:06 +1000 Subject: [PATCH 10/19] Fix expand state not updating in V16 This regressed in 9644f5724ca73a45f7f19a172516c36137352083 + Fix incorrect private find() implementation in CardContentProvider, and move into shared file. Closes #11999 --- .../src/main/java/com/ichi2/anki/DeckPicker.kt | 9 +++++++++ .../ichi2/anki/provider/CardContentProvider.kt | 12 ++---------- .../com/ichi2/libanki/sched/DeckDueTreeNode.kt | 15 +++++++++++++++ 3 files changed, 26 insertions(+), 10 deletions(-) diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt b/AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt index 6056386f2809..be8995a253a8 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt @@ -99,7 +99,9 @@ import com.ichi2.libanki.* import com.ichi2.libanki.Collection.CheckDatabaseResult import com.ichi2.libanki.importer.AnkiPackageImporter import com.ichi2.libanki.sched.AbstractDeckTreeNode +import com.ichi2.libanki.sched.DeckDueTreeNode import com.ichi2.libanki.sched.TreeNode +import com.ichi2.libanki.sched.findInDeckTree import com.ichi2.libanki.sync.CustomSyncServerUrlException import com.ichi2.libanki.sync.Syncer.ConnectionResultType import com.ichi2.libanki.utils.TimeManager @@ -1016,9 +1018,16 @@ open class DeckPicker : } @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) + @RustCleanup("make mDueTree a concrete DeckDueTreeNode") + @Suppress("UNCHECKED_CAST") fun toggleDeckExpand(did: DeckId) { if (!col.decks.children(did).isEmpty()) { + // update DB col.decks.collapse(did) + // update stored state + findInDeckTree(mDueTree!! as List>, did)?.run { + collapsed = !collapsed + } renderPage() dismissAllDialogFragments() } diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/provider/CardContentProvider.kt b/AnkiDroid/src/main/java/com/ichi2/anki/provider/CardContentProvider.kt index 4d8e324dda08..815201af10e6 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/provider/CardContentProvider.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/provider/CardContentProvider.kt @@ -38,6 +38,7 @@ import com.ichi2.libanki.exception.EmptyMediaException import com.ichi2.libanki.sched.AbstractSched import com.ichi2.libanki.sched.DeckDueTreeNode import com.ichi2.libanki.sched.TreeNode +import com.ichi2.libanki.sched.findInDeckTree import com.ichi2.libanki.utils.TimeManager import com.ichi2.utils.FileUtil.internalizeUri import com.ichi2.utils.JSONArray @@ -403,16 +404,7 @@ class CardContentProvider : ContentProvider() { val rv = MatrixCursor(columns, 1) val allDecks = col.sched.deckDueTree() val desiredDeckId = uri.pathSegments[1].toLong() - fun find(nodeList: List>, id: Long): DeckDueTreeNode? { - for (node in nodeList) { - if (node.value.did == id) { - return node.value - } - return find(node.children, id) - } - return null - } - find(allDecks, desiredDeckId)?.let { + findInDeckTree(allDecks, desiredDeckId)?.let { addDeckToCursor(it.did, it.fullDeckName, getDeckCountsFromDueTreeNode(it), rv, col, columns) } rv diff --git a/AnkiDroid/src/main/java/com/ichi2/libanki/sched/DeckDueTreeNode.kt b/AnkiDroid/src/main/java/com/ichi2/libanki/sched/DeckDueTreeNode.kt index 7a4f0d62588d..0b302dbe031d 100644 --- a/AnkiDroid/src/main/java/com/ichi2/libanki/sched/DeckDueTreeNode.kt +++ b/AnkiDroid/src/main/java/com/ichi2/libanki/sched/DeckDueTreeNode.kt @@ -117,3 +117,18 @@ class DeckDueTreeNode( return revCount > 0 || newCount > 0 || lrnCount > 0 } } + +/** Locate node with a given deck ID in a list of nodes. + * + * This could be converted into a method if AnkiDroid returned a top-level + * node instead of a list of nodes. + */ +fun findInDeckTree(nodes: List>, deckId: Long): DeckDueTreeNode? { + for (node in nodes) { + if (node.value.did == deckId) { + return node.value + } + return findInDeckTree(node.children, deckId) ?: continue + } + return null +} From 500b0e94efd724bb92d6ab6c1a45a66e511670d3 Mon Sep 17 00:00:00 2001 From: Damien Elmes Date: Fri, 12 Aug 2022 18:09:04 +1000 Subject: [PATCH 11/19] Fix deck picker options flickering + error on full sync https://github.com/ankidroid/Anki-Android/pull/11849#issuecomment-1211775736 Android's onCreateOptionsMenu does not play well with coroutines, as it expects the menu to have been fully configured by the time the routine returns. This results in flicker, as the menu gets blanked out, and then configured a moment later when the coroutine runs. To work around this, the current state is stored in the deck picker, so that we can redraw the menu immediately, and then make any potential changes in the background. Other changes: - refactored onCreateOptionsMenu to make it simpler - instead of the sdCardAvailable checks (which I assume is a proxy for "col is available"), the entire menu is wrapped in a group, and the visibility of the group is toggled depending on whether the col is available or not. This also fixes the error on a full sync. - there are three sets of unit tests (one for search icon, one for sync icon, one for entire menu) that have been a pain since I originally introduced this PR, and and I've sunk a number of hours into trying to get them to work properly at this point. The issue appears to be that when mixing coroutine calls and invalidateOptionsMenu(), onCreateOptionsMenu() is not getting called before trying to await the job, leading to a hang or stale data. I tried advancing robolectric, but it did not help. Maybe someone more experienced in this area can figure it out, but for now I've changed these routines to be more of a unit test and less of an integration test: rather than checking the menu itself, they directly invoke the function that updates the menu state, and check the state instead. This takes onCreateOptionsMenu() out of the loop, and avoids the problems (and probably allows these tests to be re-enabled on Windows as well). The sync tests I've removed, as the entire menu is hidden/shown now when the col is closed, so they are redundant. --- .../main/java/com/ichi2/anki/DeckPicker.kt | 216 ++++++++++-------- AnkiDroid/src/main/res/menu/deck_picker.xml | 112 +++++---- .../java/com/ichi2/anki/DeckPickerTest.kt | 69 +----- .../java/com/ichi2/anki/RobolectricTest.kt | 7 - .../anki/dialogs/CreateDeckDialogTest.kt | 73 ++---- 5 files changed, 211 insertions(+), 266 deletions(-) diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt b/AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt index be8995a253a8..b1e07f858bd4 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt @@ -96,6 +96,7 @@ import com.ichi2.async.Connection.CancellableTaskListener import com.ichi2.async.Connection.ConflictResolution import com.ichi2.compat.CompatHelper.Companion.sdkVersion import com.ichi2.libanki.* +import com.ichi2.libanki.Collection import com.ichi2.libanki.Collection.CheckDatabaseResult import com.ichi2.libanki.importer.AnkiPackageImporter import com.ichi2.libanki.sched.AbstractDeckTreeNode @@ -182,6 +183,10 @@ open class DeckPicker : private var mStartupError = false private var mEmptyCardTask: Cancellable? = null + /** See [OptionsMenuState]. */ + @VisibleForTesting + var optionsMenuState: OptionsMenuState? = null + @JvmField @VisibleForTesting var mDueTree: List>? = null @@ -579,51 +584,47 @@ open class DeckPicker : } override fun onCreateOptionsMenu(menu: Menu): Boolean { + Timber.d("onCreateOptionsMenu()") + mFloatingActionMenu.closeFloatingActionMenu() + menuInflater.inflate(R.menu.deck_picker, menu) + setupSearchIcon(menu.findItem(R.id.deck_picker_action_filter)) + // redraw menu synchronously to avoid flicker + updateMenuFromState(menu) + // ...then launch a task to possibly update the visible icons. // 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) - 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 - } + updateMenuState() + updateMenuFromState(menu) + } + return super.onCreateOptionsMenu(menu) + } - // 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 - } - }) + private fun setupSearchIcon(menuItem: MenuItem) { + menuItem.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 { + (menuItem.actionView as SearchView).run { + queryHint = getString(R.string.search_decks) + setOnQueryTextListener(object : SearchView.OnQueryTextListener { override fun onQueryTextSubmit(query: String): Boolean { - mToolbarSearchView!!.clearFocus() + clearFocus() return true } @@ -633,74 +634,84 @@ open class DeckPicker : return true } }) + } + searchDecksIcon = menuItem + } - displaySyncBadge(menu) - - // Show / hide undo - val undoName = withOpenColOrNull { - if (fragmented || !undoAvailable()) { - null - } else { - undoName(resources) - } - } + private fun updateMenuFromState(menu: Menu) { + menu.setGroupVisible(R.id.allItems, optionsMenuState != null) + optionsMenuState?.run { + menu.findItem(R.id.deck_picker_action_filter).isVisible = searchIcon + updateUndoIconFromState(menu.findItem(R.id.action_undo), undoIcon) + updateSyncIconFromState(menu.findItem(R.id.action_sync), syncIcon) + } + } - if (undoName == null) { - menu.findItem(R.id.action_undo).isVisible = false + private fun updateUndoIconFromState(menuItem: MenuItem, undoTitle: String?) { + menuItem.run { + if (undoTitle != null) { + isVisible = true + title = resources.getString(R.string.studyoptions_congrats_undo, undoTitle) } else { - val res = resources - menu.findItem(R.id.action_undo).isVisible = true - val undo = res.getString(R.string.studyoptions_congrats_undo, undoName) - menu.findItem(R.id.action_undo).title = undo + isVisible = false } + } + } - updateSearchDecksIconVisibility() + private fun updateSyncIconFromState(menuItem: MenuItem, syncIcon: SyncIconState) { + when (syncIcon) { + SyncIconState.Normal -> { + BadgeDrawableBuilder.removeBadge(menuItem) + menuItem.setTitle(R.string.button_sync) + } + SyncIconState.PendingChanges -> { + BadgeDrawableBuilder(resources) + .withColor(ContextCompat.getColor(this@DeckPicker, R.color.badge_warning)) + .replaceBadge(menuItem) + menuItem.setTitle(R.string.button_sync) + } + SyncIconState.FullSync, SyncIconState.NotLoggedIn -> { + BadgeDrawableBuilder(resources) + .withText('!') + .withColor(ContextCompat.getColor(this@DeckPicker, R.color.badge_error)) + .replaceBadge(menuItem) + if (syncIcon == SyncIconState.FullSync) { + menuItem.setTitle(R.string.sync_menu_title_full_sync) + } else { + menuItem.setTitle(R.string.sync_menu_title_no_account) + } + } } - return super.onCreateOptionsMenu(menu) } @VisibleForTesting - suspend fun updateSearchDecksIconVisibility() { - val visible = withOpenColOrNull { decks.count() >= 10 } ?: false - searchDecksIcon?.isVisible = visible + suspend fun updateMenuState() { + optionsMenuState = withOpenColOrNull { + val searchIcon = decks.count() >= 10 + val undoIcon = undoName(resources).let { + if (it.isEmpty()) { + null + } else { + it + } + } + val syncIcon = fetchSyncStatus(col) + OptionsMenuState(searchIcon, undoIcon, syncIcon) + } } - @VisibleForTesting - protected open suspend fun displaySyncBadge(menu: Menu) { + private fun fetchSyncStatus(col: Collection): SyncIconState { val auth = syncAuth() - val syncStatus = withOpenColOrNull { - SyncStatus.getSyncStatus(this, auth) - } - if (syncStatus == null) { - return - } - val syncMenu = menu.findItem(R.id.action_sync) - when (syncStatus) { + val syncStatus = SyncStatus.getSyncStatus(col, auth) + return when (syncStatus) { SyncStatus.BADGE_DISABLED, SyncStatus.NO_CHANGES, SyncStatus.INCONCLUSIVE -> { - syncMenu?.let { - BadgeDrawableBuilder.removeBadge(it) - it.setTitle(R.string.button_sync) - } + SyncIconState.Normal } SyncStatus.HAS_CHANGES -> { - // Light orange icon - BadgeDrawableBuilder(resources) - .withColor(ContextCompat.getColor(this, R.color.badge_warning)) - .replaceBadge(syncMenu) - syncMenu.setTitle(R.string.button_sync) - } - SyncStatus.NO_ACCOUNT, SyncStatus.FULL_SYNC -> { - if (syncStatus === SyncStatus.NO_ACCOUNT) { - syncMenu.setTitle(R.string.sync_menu_title_no_account) - } else if (syncStatus === SyncStatus.FULL_SYNC) { - syncMenu.setTitle(R.string.sync_menu_title_full_sync) - } - // Orange-red icon with exclamation mark - BadgeDrawableBuilder(resources) - .withText('!') - .withColor(ContextCompat.getColor(this, R.color.badge_error)) - .replaceBadge(syncMenu) + SyncIconState.PendingChanges } + SyncStatus.NO_ACCOUNT -> SyncIconState.NotLoggedIn + SyncStatus.FULL_SYNC -> SyncIconState.FullSync } } @@ -2152,6 +2163,7 @@ open class DeckPicker : @RustCleanup("backup with 5 minute timer, instead of deck list refresh") private fun updateDeckList(quick: Boolean) { + invalidateOptionsMenu() if (!BackendFactory.defaultLegacySchema) { // uses user's desktop settings to determine whether a backup // actually happens @@ -2241,10 +2253,6 @@ open class DeckPicker : scrollDecklistToDeck(current) mFocusedDeck = current } - - launchCatchingTask { - updateSearchDecksIconVisibility() - } } // Callback to show study options for currently selected deck @@ -2727,3 +2735,23 @@ open class DeckPicker : } } } + +/** Android's onCreateOptionsMenu does not play well with coroutines, as + * it expects the menu to have been fully configured by the time the routine + * returns. This results in flicker, as the menu gets blanked out, and then + * configured a moment later when the coroutine runs. To work around this, + * the current state is stored in the deck picker so that we can redraw the + * menu immediately. */ +data class OptionsMenuState( + var searchIcon: Boolean, + /** If undo is available, a string describing the action. */ + var undoIcon: String?, + var syncIcon: SyncIconState +) + +enum class SyncIconState { + Normal, + PendingChanges, + FullSync, + NotLoggedIn +} diff --git a/AnkiDroid/src/main/res/menu/deck_picker.xml b/AnkiDroid/src/main/res/menu/deck_picker.xml index 065683390bc4..041eba3c5574 100644 --- a/AnkiDroid/src/main/res/menu/deck_picker.xml +++ b/AnkiDroid/src/main/res/menu/deck_picker.xml @@ -1,60 +1,58 @@ - - - - - - - - - - - - + + + + + + + + + + + + + + diff --git a/AnkiDroid/src/test/java/com/ichi2/anki/DeckPickerTest.kt b/AnkiDroid/src/test/java/com/ichi2/anki/DeckPickerTest.kt index dfc53e7ca5ff..84d5f1f6f3f3 100644 --- a/AnkiDroid/src/test/java/com/ichi2/anki/DeckPickerTest.kt +++ b/AnkiDroid/src/test/java/com/ichi2/anki/DeckPickerTest.kt @@ -17,7 +17,8 @@ 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 kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.test.runTest import net.ankiweb.rsdroid.BackendFactory import org.apache.commons.exec.OS import org.hamcrest.MatcherAssert.* @@ -35,6 +36,7 @@ import java.util.* import kotlin.test.assertNotNull import kotlin.test.assertNull +@OptIn(ExperimentalCoroutinesApi::class) @RunWith(ParameterizedRobolectricTestRunner::class) @KotlinCleanup("fix IDE lint issues") @KotlinCleanup("replace `when` usages") @@ -319,18 +321,17 @@ class DeckPickerTest : RobolectricTest() { @Test @RunInBackground - fun doNotShowOptionsMenuWhenCollectionInaccessible() { - skipWindows() + fun doNotShowOptionsMenuWhenCollectionInaccessible() = runTest { try { enableNullCollection() val d = super.startActivityNormallyOpenCollectionWithIntent( DeckPickerEx::class.java, Intent() ) - runBlocking { d.createMenuJob?.join() } + d.updateMenuState() assertThat( "Options menu not displayed when collection is inaccessible", - d.optionsMenu?.hasVisibleItems(), - equalTo(false) + d.optionsMenuState, + equalTo(null) ) } finally { disableNullCollection() @@ -338,59 +339,17 @@ class DeckPickerTest : RobolectricTest() { } @Test - fun showOptionsMenuWhenCollectionAccessible() { - skipWindows() + fun showOptionsMenuWhenCollectionAccessible() = runTest { try { InitialActivityWithConflictTest.grantWritePermissions() val d = super.startActivityNormallyOpenCollectionWithIntent( DeckPickerEx::class.java, Intent() ) - runBlocking { d.createMenuJob?.join() } + d.updateMenuState() assertThat( "Options menu displayed when collection is accessible", - d.optionsMenu?.hasVisibleItems(), - equalTo(true) - ) - } finally { - InitialActivityWithConflictTest.revokeWritePermissions() - } - } - - @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, - equalTo(false) - ) - } finally { - disableNullCollection() - } - } - - @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, - equalTo(true) + d.optionsMenuState, + `is`(notNullValue()) ) } finally { InitialActivityWithConflictTest.revokeWritePermissions() @@ -619,7 +578,6 @@ class DeckPickerTest : RobolectricTest() { var databaseErrorDialog = 0 var displayedAnalyticsOptIn = false var optionsMenu: Menu? = null - var displaySyncBadge = false override fun showDatabaseErrorDialog(id: Int) { databaseErrorDialog = id @@ -642,10 +600,5 @@ class DeckPickerTest : RobolectricTest() { optionsMenu = menu return super.onPrepareOptionsMenu(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 80364e3c9b0b..5ee5bdc4bf69 100644 --- a/AnkiDroid/src/test/java/com/ichi2/anki/RobolectricTest.kt +++ b/AnkiDroid/src/test/java/com/ichi2/anki/RobolectricTest.kt @@ -55,7 +55,6 @@ 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 @@ -426,12 +425,6 @@ 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 dbbe7adfe93f..ffd420e130cf 100644 --- a/AnkiDroid/src/test/java/com/ichi2/anki/dialogs/CreateDeckDialogTest.kt +++ b/AnkiDroid/src/test/java/com/ichi2/anki/dialogs/CreateDeckDialogTest.kt @@ -22,24 +22,30 @@ import androidx.test.core.app.ActivityScenario import com.afollestad.materialdialogs.WhichButton import com.afollestad.materialdialogs.actions.getActionButton import com.afollestad.materialdialogs.input.getInputField +import com.ichi2.anki.CollectionManager.withCol import com.ichi2.anki.DeckPicker 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.ExperimentalCoroutinesApi import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.test.runTest import org.hamcrest.CoreMatchers.equalTo import org.hamcrest.MatcherAssert +import org.junit.Ignore import org.junit.Test import org.junit.runner.RunWith import org.robolectric.RobolectricTestRunner import java.util.* import java.util.concurrent.atomic.AtomicInteger import java.util.concurrent.atomic.AtomicReference +import kotlin.coroutines.resume +import kotlin.coroutines.suspendCoroutine import kotlin.test.assertEquals import kotlin.test.assertFalse -import kotlin.test.assertTrue +@OptIn(ExperimentalCoroutinesApi::class) @RunWith(RobolectricTestRunner::class) class CreateDeckDialogTest : RobolectricTest() { private var mActivityScenario: ActivityScenario? = null @@ -149,8 +155,13 @@ class CreateDeckDialogTest : RobolectricTest() { } @Test + @Ignore("this is difficult to test at the moment") fun searchDecksIconVisibilityDeckCreationTest() { - skipWindows() + // this is currently broken, as it has a few issues: + // - we need to await the completion of createMenuJob, as the menu is created asynchronously + // - the calls to `decks` should be made using withCol, and this routine should be asynchronous + // - when I attempted to implement this, I found the test hung. I'm guessing it might be some + // sort of deadlock, where a runBlocking() call is waiting for some UI state to update mActivityScenario!!.onActivity { deckPicker -> val decks = deckPicker.col.decks val deckCounter = AtomicInteger(1) @@ -186,59 +197,21 @@ class CreateDeckDialogTest : RobolectricTest() { // 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()) - updateSearchDecksIcon(deckPicker) - assertTrue(deckPicker.searchDecksIcon!!.isVisible) - - awaitJob(deckPicker.confirmDeckDeletion(decks.id("Deck0::Deck1"))) - - assertEquals(2, decks.count()) - updateSearchDecksIcon(deckPicker) - assertFalse(deckPicker.searchDecksIcon!!.isVisible) - } - createDeckDialog.createDeck(deckTreeName(0, 8, "Deck")) - - createDeckDialog = CreateDeckDialog(deckPicker, R.string.new_deck, CreateDeckDialog.DeckDialogType.DECK, null) - createDeckDialog.setOnNewDeckCreated { - assertEquals(12, decks.count()) - updateSearchDecksIcon(deckPicker) - assertTrue(deckPicker.searchDecksIcon!!.isVisible) - - awaitJob(deckPicker.confirmDeckDeletion(decks.id("Deck0::Deck1"))) - - assertEquals(2, decks.count()) - updateSearchDecksIcon(deckPicker) - assertFalse(deckPicker.searchDecksIcon!!.isVisible) - } - createDeckDialog.createDeck(deckTreeName(0, 10, "Deck")) - - createDeckDialog = CreateDeckDialog(deckPicker, R.string.new_deck, CreateDeckDialog.DeckDialogType.DECK, null) - createDeckDialog.setOnNewDeckCreated { - assertEquals(6, decks.count()) - updateSearchDecksIcon(deckPicker) - assertFalse(deckPicker.searchDecksIcon!!.isVisible) - } - createDeckDialog.createDeck(deckTreeName(0, 4, "Deck")) - - createDeckDialog = CreateDeckDialog(deckPicker, R.string.new_deck, CreateDeckDialog.DeckDialogType.DECK, null) - createDeckDialog.setOnNewDeckCreated { - assertEquals(12, decks.count()) - updateSearchDecksIcon(deckPicker) - assertTrue(deckPicker.searchDecksIcon!!.isVisible) - } - createDeckDialog.createDeck(deckTreeName(6, 11, "Deck")) + fun searchDecksIconVisibilitySubdeckCreationTest() = runTest { + val deckPicker = + suspendCoroutine { coro -> mActivityScenario!!.onActivity { coro.resume(it) } } + deckPicker.updateMenuState() + assertEquals(deckPicker.optionsMenuState!!.searchIcon, false) + // a single top-level deck with lots of subdecks should turn the icon on + withCol { + decks.id(deckTreeName(0, 10, "Deck")) } + deckPicker.updateMenuState() + assertEquals(deckPicker.optionsMenuState!!.searchIcon, true) } private fun deckTreeName(start: Int, end: Int, prefix: String): String { From 88885980c607816c814265a1a3e7e24a204e0100 Mon Sep 17 00:00:00 2001 From: Damien Elmes Date: Sat, 13 Aug 2022 12:05:05 +1000 Subject: [PATCH 12/19] Legacy undo & v2 queues must be cleared on backend op Fixes #12007 --- AnkiDroid/src/main/java/com/ichi2/libanki/ChangeManager.kt | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/AnkiDroid/src/main/java/com/ichi2/libanki/ChangeManager.kt b/AnkiDroid/src/main/java/com/ichi2/libanki/ChangeManager.kt index dea1e800b2e3..f57c37220396 100644 --- a/AnkiDroid/src/main/java/com/ichi2/libanki/ChangeManager.kt +++ b/AnkiDroid/src/main/java/com/ichi2/libanki/ChangeManager.kt @@ -93,7 +93,12 @@ object ChangeManager { * to notify change subscribers of the changes. */ suspend fun undoableOp(handler: Any? = null, block: CollectionV16.() -> T): T { return withCol { - this.newBackend.block() + val result = newBackend.block() + // any backend operation clears legacy undo and resets study queues if it + // succeeds + clearUndo() + reset() + result }.also { withContext(Dispatchers.Main) { ChangeManager.notifySubscribers(it, handler) From 676280f6a3920325f6fd3ff0fb1cb94772ee1383 Mon Sep 17 00:00:00 2001 From: Damien Elmes Date: Mon, 15 Aug 2022 02:42:08 +1000 Subject: [PATCH 13/19] Catch IO errors in media check with new schema Closes #12026 --- AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt b/AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt index b1e07f858bd4..46219d071748 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt @@ -61,6 +61,7 @@ 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.withCol import com.ichi2.anki.CollectionManager.withOpenColOrNull import com.ichi2.anki.InitialActivity.StartupFailure import com.ichi2.anki.InitialActivity.StartupFailure.* @@ -1465,7 +1466,14 @@ open class DeckPicker : override fun mediaCheck() { if (hasStorageAccessPermission(this)) { - TaskManager.launchCollectionTask(CheckMedia(), mediaCheckListener()) + if (!BackendFactory.defaultLegacySchema) { + launchCatchingTask { + val result = withProgress { withCol { media.check() } } + showMediaCheckDialog(MediaCheckDialog.DIALOG_MEDIA_CHECK_RESULTS, result) + } + } else { + TaskManager.launchCollectionTask(CheckMedia(), mediaCheckListener()) + } } else { requestStoragePermission() } From dd81dd41f442770e0023754094866a309e3c01a0 Mon Sep 17 00:00:00 2001 From: Damien Elmes Date: Mon, 15 Aug 2022 02:49:25 +1000 Subject: [PATCH 14/19] Remove wildcard import --- .../java/com/ichi2/anki/preferences/GeneralSettingsFragment.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 42d4a4593430..cf1c7c9a3599 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/preferences/GeneralSettingsFragment.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/preferences/GeneralSettingsFragment.kt @@ -17,7 +17,7 @@ package com.ichi2.anki.preferences import androidx.preference.ListPreference import androidx.preference.SwitchPreference -import com.ichi2.anki.* +import com.ichi2.anki.CollectionManager import com.ichi2.anki.CrashReportService import com.ichi2.anki.R import com.ichi2.anki.contextmenu.AnkiCardContextMenu From 8e095c10c745ba605ed52a0351c129f62f746fcd Mon Sep 17 00:00:00 2001 From: Damien Elmes Date: Mon, 15 Aug 2022 03:14:34 +1000 Subject: [PATCH 15/19] Unify isOnMainThread implementations --- .../java/com/ichi2/anki/CollectionManager.kt | 20 ++----------------- .../src/main/java/com/ichi2/utils/Threads.kt | 17 ++++++++++++++-- 2 files changed, 17 insertions(+), 20 deletions(-) diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/CollectionManager.kt b/AnkiDroid/src/main/java/com/ichi2/anki/CollectionManager.kt index d5b5ec6a0dc1..8cfad8c98301 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/CollectionManager.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/CollectionManager.kt @@ -17,18 +17,17 @@ 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 com.ichi2.utils.Threads 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 { /** @@ -224,21 +223,6 @@ object CollectionManager { 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) @@ -250,7 +234,7 @@ object CollectionManager { val start = System.currentTimeMillis() return block().also { val elapsed = System.currentTimeMillis() - start - if (isMainThread() && elapsed > 100) { + if (Threads.isOnMainThread && 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 diff --git a/AnkiDroid/src/main/java/com/ichi2/utils/Threads.kt b/AnkiDroid/src/main/java/com/ichi2/utils/Threads.kt index d55ac24e1bfd..859efb69464f 100644 --- a/AnkiDroid/src/main/java/com/ichi2/utils/Threads.kt +++ b/AnkiDroid/src/main/java/com/ichi2/utils/Threads.kt @@ -20,6 +20,7 @@ import android.os.Looper import androidx.annotation.UiThread import androidx.annotation.WorkerThread import timber.log.Timber +import java.lang.RuntimeException /** * Helper class for checking for programming errors while using threads. @@ -29,8 +30,20 @@ object Threads { /** * @return true if called from the application main thread */ - private val isOnMainThread: Boolean - get() = Looper.getMainLooper() == Looper.myLooper() + val isOnMainThread: Boolean + get() = + 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 + } + } /** * Checks that it is called from the main thread and fails if it is called from another thread. From b04e174a0281ac963ee714aff1bf557bc6171d06 Mon Sep 17 00:00:00 2001 From: Damien Elmes Date: Mon, 15 Aug 2022 03:16:53 +1000 Subject: [PATCH 16/19] Remove invalidateOptionsMenu() call Theoretically it should be unnecessary, as it's called in refreshState() --- AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt | 1 - 1 file changed, 1 deletion(-) diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt b/AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt index 46219d071748..b83db95e3494 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt @@ -2171,7 +2171,6 @@ open class DeckPicker : @RustCleanup("backup with 5 minute timer, instead of deck list refresh") private fun updateDeckList(quick: Boolean) { - invalidateOptionsMenu() if (!BackendFactory.defaultLegacySchema) { // uses user's desktop settings to determine whether a backup // actually happens From e5a052ff23b84f379e6a0483468c5727e0141699 Mon Sep 17 00:00:00 2001 From: Damien Elmes Date: Mon, 15 Aug 2022 04:54:44 +1000 Subject: [PATCH 17/19] Fix error when undoing in card browser --- AnkiDroid/src/main/java/com/ichi2/anki/CardBrowser.kt | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/CardBrowser.kt b/AnkiDroid/src/main/java/com/ichi2/anki/CardBrowser.kt index 77e6bca1765d..8a8af9f3f49d 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/CardBrowser.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/CardBrowser.kt @@ -2615,8 +2615,7 @@ open class CardBrowser : changes.card ) && handler !== this ) { - // executing this only for the refresh side effects; there may be a better way - Undo().runWithHandler(mUndoHandler) + mUndoHandler.actualOnPostExecute(this@CardBrowser, Computation.ok(NextCard.withNoResult(null))) } } From 0d051f5ecea251e71aa3a6b182bd9a3e220d7225 Mon Sep 17 00:00:00 2001 From: Damien Elmes Date: Mon, 15 Aug 2022 04:57:22 +1000 Subject: [PATCH 18/19] Shift launchCatchingTask to FragmentActivity; hook undo up in StudyOptions Change based on https://github.com/ankidroid/Anki-Android/pull/12011#issuecomment-1214170607 --- .../main/java/com/ichi2/anki/BackendUndo.kt | 6 +++-- .../java/com/ichi2/anki/CoroutineHelpers.kt | 24 +++++++++++++------ .../com/ichi2/anki/StudyOptionsFragment.kt | 13 +++++++++- 3 files changed, 33 insertions(+), 10 deletions(-) diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/BackendUndo.kt b/AnkiDroid/src/main/java/com/ichi2/anki/BackendUndo.kt index 76f3179f4e51..4693050130f5 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/BackendUndo.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/BackendUndo.kt @@ -16,13 +16,15 @@ package com.ichi2.anki +import androidx.fragment.app.FragmentActivity +import com.ichi2.anki.CollectionManager.TR import com.ichi2.anki.UIUtils.showSimpleSnackbar import com.ichi2.libanki.undoNew import com.ichi2.libanki.undoableOp import com.ichi2.utils.BlocksSchemaUpgrade import net.ankiweb.rsdroid.BackendException -suspend fun AnkiActivity.backendUndoAndShowPopup(): Boolean { +suspend fun FragmentActivity.backendUndoAndShowPopup(): Boolean { return try { val changes = withProgress() { undoableOp { @@ -31,7 +33,7 @@ suspend fun AnkiActivity.backendUndoAndShowPopup(): Boolean { } showSimpleSnackbar( this, - col.tr.undoActionUndone(changes.operation), + TR.undoActionUndone(changes.operation), false ) true diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/CoroutineHelpers.kt b/AnkiDroid/src/main/java/com/ichi2/anki/CoroutineHelpers.kt index 0762ae7020c9..8fa70366548c 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/CoroutineHelpers.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/CoroutineHelpers.kt @@ -18,6 +18,8 @@ package com.ichi2.anki import android.content.Context import androidx.appcompat.app.AlertDialog +import androidx.fragment.app.Fragment +import androidx.fragment.app.FragmentActivity import androidx.lifecycle.coroutineScope import anki.collection.Progress import com.ichi2.anki.UIUtils.showSimpleSnackbar @@ -36,27 +38,35 @@ import kotlin.coroutines.suspendCoroutine * Errors from the backend contain localized text that is often suitable to show to the user as-is. * Other errors should ideally be handled in the block. */ -fun AnkiActivity.launchCatchingTask( +fun FragmentActivity.launchCatchingTask( + errorMessage: String? = null, block: suspend CoroutineScope.() -> Unit ): Job { + val extraInfo = errorMessage ?: "" return lifecycle.coroutineScope.launch { try { block() } catch (exc: CancellationException) { // do nothing } catch (exc: BackendInterruptedException) { - Timber.e("caught: %s", exc) + Timber.e("caught: %s %s", exc, extraInfo) showSimpleSnackbar(this@launchCatchingTask, exc.localizedMessage, false) } catch (exc: BackendException) { - Timber.e("caught: %s", exc) + Timber.e("caught: %s %s", exc, extraInfo) showError(this@launchCatchingTask, exc.localizedMessage!!) } catch (exc: Exception) { - Timber.e("caught: %s", exc) + Timber.e("caught: %s %s", exc, extraInfo) showError(this@launchCatchingTask, exc.toString()) } } } +/** See [FragmentActivity.launchCatchingTask] */ +fun Fragment.launchCatchingTask( + errorMessage: String? = null, + block: suspend CoroutineScope.() -> Unit +): Job = requireActivity().launchCatchingTask(errorMessage, block) + private fun showError(context: Context, msg: String) { AlertDialog.Builder(context) .setTitle(R.string.vague_error) @@ -90,7 +100,7 @@ suspend fun Backend.withProgress( * Run the provided operation, showing a progress window until it completes. * Progress info is polled from the backend. */ -suspend fun AnkiActivity.withProgress( +suspend fun FragmentActivity.withProgress( extractProgress: ProgressContext.() -> Unit, onCancel: ((Backend) -> Unit)? = { it.setWantsAbort() }, op: suspend () -> T @@ -117,7 +127,7 @@ suspend fun AnkiActivity.withProgress( * Run the provided operation, showing a progress window with the provided * message until the operation completes. */ -suspend fun AnkiActivity.withProgress( +suspend fun FragmentActivity.withProgress( message: String = resources.getString(R.string.dialog_processing), op: suspend () -> T ): T = withProgressDialog( @@ -130,7 +140,7 @@ suspend fun AnkiActivity.withProgress( } private suspend fun withProgressDialog( - context: AnkiActivity, + context: FragmentActivity, onCancel: (() -> Unit)?, @Suppress("Deprecation") // ProgressDialog deprecation op: suspend (android.app.ProgressDialog) -> T diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/StudyOptionsFragment.kt b/AnkiDroid/src/main/java/com/ichi2/anki/StudyOptionsFragment.kt index 97df3cf0cd4e..7650ea4dd47d 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/StudyOptionsFragment.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/StudyOptionsFragment.kt @@ -50,6 +50,7 @@ import com.ichi2.themes.StyledProgressDialog.Companion.show import com.ichi2.utils.FragmentFactoryUtils.instantiate import com.ichi2.utils.HtmlUtils.convertNewlinesToHtml import com.ichi2.utils.KotlinCleanup +import net.ankiweb.rsdroid.BackendFactory import timber.log.Timber class StudyOptionsFragment : Fragment(), Toolbar.OnMenuItemClickListener { @@ -255,7 +256,17 @@ class StudyOptionsFragment : Fragment(), Toolbar.OnMenuItemClickListener { when (item.itemId) { R.id.action_undo -> { Timber.i("StudyOptionsFragment:: Undo button pressed") - Undo().runWithHandler(mUndoListener) + if (BackendFactory.defaultLegacySchema) { + Undo().runWithHandler(mUndoListener) + } else { + launchCatchingTask { + if (requireActivity().backendUndoAndShowPopup()) { + openReviewer() + } else { + Undo().runWithHandler(mUndoListener) + } + } + } return true } R.id.action_deck_or_study_options -> { From 432490c9cf52ab1e5be3bb914a806fcb9442570e Mon Sep 17 00:00:00 2001 From: Damien Elmes Date: Mon, 15 Aug 2022 14:51:54 +1000 Subject: [PATCH 19/19] Delay pop-up of progress window Closes #12027 --- .../java/com/ichi2/anki/CoroutineHelpers.kt | 28 ++++++++++++------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/CoroutineHelpers.kt b/AnkiDroid/src/main/java/com/ichi2/anki/CoroutineHelpers.kt index 8fa70366548c..714a0696b604 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/CoroutineHelpers.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/CoroutineHelpers.kt @@ -17,6 +17,7 @@ package com.ichi2.anki import android.content.Context +import android.view.WindowManager import androidx.appcompat.app.AlertDialog import androidx.fragment.app.Fragment import androidx.fragment.app.FragmentActivity @@ -24,7 +25,6 @@ import androidx.lifecycle.coroutineScope import anki.collection.Progress import com.ichi2.anki.UIUtils.showSimpleSnackbar import com.ichi2.libanki.Collection -import com.ichi2.themes.StyledProgressDialog import kotlinx.coroutines.* import net.ankiweb.rsdroid.Backend import net.ankiweb.rsdroid.BackendException @@ -139,23 +139,31 @@ suspend fun FragmentActivity.withProgress( op() } +@Suppress("Deprecation") // ProgressDialog deprecation private suspend fun withProgressDialog( context: FragmentActivity, onCancel: (() -> Unit)?, - @Suppress("Deprecation") // ProgressDialog deprecation op: suspend (android.app.ProgressDialog) -> T -): T { - val dialog = StyledProgressDialog.show( - context, null, - null, onCancel != null - ) - onCancel?.let { - dialog.setOnCancelListener { it() } +): T = coroutineScope { + val dialog = android.app.ProgressDialog(context).apply { + setCancelable(onCancel != null) + onCancel?.let { + setOnCancelListener { it() } + } + } + // disable taps immediately + context.window.setFlags(WindowManager.LayoutParams.FLAG_NOT_TOUCHABLE, WindowManager.LayoutParams.FLAG_NOT_TOUCHABLE) + // reveal the dialog after 600ms + val dialogJob = launch { + delay(600) + dialog.show() } - return try { + try { op(dialog) } finally { + dialogJob.cancel() dialog.dismiss() + context.window.clearFlags(WindowManager.LayoutParams.FLAG_NOT_TOUCHABLE) } }