diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt b/AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt index f80b1e21969d..03a89eee7d68 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt @@ -95,6 +95,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 @@ -181,6 +182,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 @@ -578,51 +583,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 } @@ -632,74 +633,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 } } @@ -2151,6 +2162,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 @@ -2240,10 +2252,6 @@ open class DeckPicker : scrollDecklistToDeck(current) mFocusedDeck = current } - - launchCatchingTask { - updateSearchDecksIconVisibility() - } } // Callback to show study options for currently selected deck @@ -2726,3 +2734,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 e1eae01e23eb..bd849475661c 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") @@ -320,18 +322,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() @@ -339,59 +340,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, - `is`(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, - `is`(true) + d.optionsMenuState, + `is`(notNullValue()) ) } finally { InitialActivityWithConflictTest.revokeWritePermissions() @@ -626,7 +585,6 @@ class DeckPickerTest : RobolectricTest() { var databaseErrorDialog = 0 var displayedAnalyticsOptIn = false var optionsMenu: Menu? = null - var displaySyncBadge = false override fun showDatabaseErrorDialog(id: Int) { databaseErrorDialog = id @@ -649,10 +607,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 bf0cbfae2465..56fbb6e84b1a 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 cb33fd88465d..858e2f3828d1 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.MatcherAssert import org.hamcrest.core.Is +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 {