From 5676b32b2494ccfec59b7a328e5a803189d77700 Mon Sep 17 00:00:00 2001 From: David Allison <62114487+david-allison@users.noreply.github.com> Date: Thu, 9 Jan 2025 13:52:15 +0000 Subject: [PATCH] improvement(browser-columns): allow cards/notes mismatch If a user is editing options and changes 'cards/notes' then they should be editing columns for their provided selection If a user saves columns, then reverts the switch, columns should not be updated Issue 17780 --- .../browser/BrowserColumnSelectionFragment.kt | 25 ++++++++++++-- .../anki/browser/CardBrowserViewModel.kt | 14 ++++++-- .../anki/dialogs/BrowserOptionsDialog.kt | 19 +++++++---- .../anki/browser/CardBrowserViewModelTest.kt | 34 ++++++++++++++++--- 4 files changed, 76 insertions(+), 16 deletions(-) diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/browser/BrowserColumnSelectionFragment.kt b/AnkiDroid/src/main/java/com/ichi2/anki/browser/BrowserColumnSelectionFragment.kt index 6e26c402118e..14b12a117e1c 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/browser/BrowserColumnSelectionFragment.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/browser/BrowserColumnSelectionFragment.kt @@ -22,6 +22,8 @@ import android.view.KeyEvent import android.view.View import androidx.activity.OnBackPressedCallback import androidx.annotation.StringRes +import androidx.core.os.BundleCompat +import androidx.core.os.bundleOf import androidx.fragment.app.DialogFragment import androidx.fragment.app.activityViewModels import androidx.recyclerview.widget.ItemTouchHelper @@ -78,6 +80,12 @@ class BrowserColumnSelectionFragment : DialogFragment(R.layout.preferences_brows private lateinit var toolbar: MaterialToolbar + private val cardsOrNotes: CardsOrNotes + get() = + requireNotNull( + BundleCompat.getParcelable(requireArguments(), ARG_MODE, CardsOrNotes::class.java), + ) + // this is needed as requireActivity().onBackPressedDispatcher.onBackPressed() // closes the activity, but an actual back press closes the dialog private val onCloseDialogCallback = @@ -126,7 +134,7 @@ class BrowserColumnSelectionFragment : DialogFragment(R.layout.preferences_brows } Timber.d("save columns and close") - if (viewModel.updateColumns(columnAdapter.selected)) { + if (viewModel.updateColumns(columnAdapter.selected, cardsOrNotes)) { dismiss() true } else { @@ -175,7 +183,7 @@ class BrowserColumnSelectionFragment : DialogFragment(R.layout.preferences_brows // Create a RecyclerView with 2 types of elements: [ColumnWithSample] and [ColumnUsage] // Columns are draggable elements, the usage elements act as headings // TODO: runBlocking shouldn't be necessary here. - val (displayed, available) = runBlocking { viewModel.previewColumnHeadings() } + val (displayed, available) = runBlocking { viewModel.previewColumnHeadings(cardsOrNotes) } this.initiallySelectedColumns = displayed.map { it.columnType } val recyclerViewItems = @@ -238,6 +246,19 @@ class BrowserColumnSelectionFragment : DialogFragment(R.layout.preferences_brows // Although a user can reorder the non-displayed columns, this order is not persisted private val hasUnsavedChanges: Boolean get() = initiallySelectedColumns != columnAdapter.selected + + companion object { + const val ARG_MODE = "mode" + + fun createInstance(cardsOrNotes: CardsOrNotes): BrowserColumnSelectionFragment = + BrowserColumnSelectionFragment().apply { + Timber.d("Building 'Manage columns' dialog for %s mode", cardsOrNotes) + arguments = + bundleOf( + ARG_MODE to cardsOrNotes, + ) + } + } } /** diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/browser/CardBrowserViewModel.kt b/AnkiDroid/src/main/java/com/ichi2/anki/browser/CardBrowserViewModel.kt index 93deb4b12cde..8d057a8fe9d9 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/browser/CardBrowserViewModel.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/browser/CardBrowserViewModel.kt @@ -518,7 +518,10 @@ class CardBrowserViewModel( } @CheckResult - fun updateColumns(columns: List): Boolean { + fun updateColumns( + columns: List, + cardsOrNotes: CardsOrNotes, + ): Boolean { if (columns.isEmpty()) return false.also { Timber.d("updateColumns: no columns") } if (availableColumns == columns) return false.also { Timber.d("updateColumns: no changes") } @@ -891,8 +894,13 @@ class CardBrowserViewModel( * (1): An ordered list of columns which is displayed to the user * (2): A list of columns which are available to display to the user */ - suspend fun previewColumnHeadings(): Pair, List> { - val currentColumns = availableColumns + suspend fun previewColumnHeadings(cardsOrNotes: CardsOrNotes): Pair, List> { + val currentColumns = + when { + // if we match, use the loaded the columns + cardsOrNotes == this.cardsOrNotes -> availableColumns + else -> BrowserColumnCollection.load(sharedPrefs(), cardsOrNotes).columns + } var columnsWithSample = ColumnWithSample.loadSample(cards.firstOrNull(), cardsOrNotes) diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/dialogs/BrowserOptionsDialog.kt b/AnkiDroid/src/main/java/com/ichi2/anki/dialogs/BrowserOptionsDialog.kt index 4fc89ced6c37..2255e1857c6c 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/dialogs/BrowserOptionsDialog.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/dialogs/BrowserOptionsDialog.kt @@ -41,13 +41,20 @@ class BrowserOptionsDialog : AppCompatDialogFragment() { private val viewModel: CardBrowserViewModel by activityViewModels() + /** The unsaved value of [CardsOrNotes] */ + private val dialogCardsOrNotes: CardsOrNotes + get() { + @IdRes val selectedButtonId = + dialogView.findViewById(R.id.select_browser_mode).checkedRadioButtonId + return when (selectedButtonId) { + R.id.select_cards_mode -> CardsOrNotes.CARDS + else -> CardsOrNotes.NOTES + } + } + private val positiveButtonClick = { _: DialogInterface, _: Int -> - @IdRes val selectedButtonId = - dialogView.findViewById(R.id.select_browser_mode).checkedRadioButtonId - val newCardsOrNotes = - if (selectedButtonId == R.id.select_cards_mode) CardsOrNotes.CARDS else CardsOrNotes.NOTES - if (cardsOrNotes != newCardsOrNotes) { - viewModel.setCardsOrNotes(newCardsOrNotes) + if (cardsOrNotes != dialogCardsOrNotes) { + viewModel.setCardsOrNotes(dialogCardsOrNotes) } val newTruncate = dialogView.findViewById(R.id.truncate_checkbox).isChecked diff --git a/AnkiDroid/src/test/java/com/ichi2/anki/browser/CardBrowserViewModelTest.kt b/AnkiDroid/src/test/java/com/ichi2/anki/browser/CardBrowserViewModelTest.kt index 59cfdd8ca379..93f29b0ac2d0 100644 --- a/AnkiDroid/src/test/java/com/ichi2/anki/browser/CardBrowserViewModelTest.kt +++ b/AnkiDroid/src/test/java/com/ichi2/anki/browser/CardBrowserViewModelTest.kt @@ -427,6 +427,29 @@ class CardBrowserViewModelTest : JvmTest() { } } + @Test + fun `mode mismatch - changing columns`() { + // a user can use the options to update the inactive column set + // this should cause an update to the backend, but no frontend events should be obtained + runViewModelTest { + flowOfAvailableColumns.test { + ignoreEventsDuringViewModelInit() + assertThat("cardsOrNotes", cardsOrNotes, equalTo(CardsOrNotes.CARDS)) + + // we're in cards so editing NOTES should not update visible columns + setColumn(1, FSRS_STABILITY, CardsOrNotes.NOTES) + expectNoEvents() + + BrowserColumnCollection.load(sharedPrefs(), CardsOrNotes.NOTES).apply { + assertThat("notes is changed", this.columns[1], equalTo(FSRS_STABILITY)) + } + BrowserColumnCollection.load(sharedPrefs(), CardsOrNotes.CARDS).apply { + assertThat("cards is unchanged", this.columns[1], not(equalTo(FSRS_STABILITY))) + } + } + } + } + @Test fun `change card order to NO_SORTING is a no-op if done twice`() = runViewModelTest { @@ -858,7 +881,7 @@ class CardBrowserViewModelTest : JvmTest() { } runViewModelTest(notes = 0) { assertThat("no rows", rowCount, equalTo(0)) - this.previewColumnHeadings().apply { + this.previewColumnHeadings(CardsOrNotes.CARDS).apply { assertTrue("no sample values") { allColumns.all { it.sampleValue == null } } val (displayed, _) = this assertThat( @@ -871,7 +894,7 @@ class CardBrowserViewModelTest : JvmTest() { runViewModelNotesTest(notes = 0) { assertThat("no rows", rowCount, equalTo(0)) - this.previewColumnHeadings().apply { + this.previewColumnHeadings(CardsOrNotes.NOTES).apply { assertTrue("no sample values") { allColumns.all { it.sampleValue == null } } val (displayed, _) = this assertThat( @@ -892,7 +915,7 @@ class CardBrowserViewModelTest : JvmTest() { @Test fun `preview - cards`() { runViewModelTest(notes = 1) { - for (preview in previewColumnHeadings().allColumns) { + for (preview in previewColumnHeadings(CardsOrNotes.CARDS).allColumns) { val (expectedLabel, expectedValue) = when (preview.columnType) { SFLD -> Pair("Sort Field", "Front") @@ -925,7 +948,7 @@ class CardBrowserViewModelTest : JvmTest() { @Test fun `preview - notes`() { runViewModelNotesTest(notes = 1) { - for (preview in previewColumnHeadings().allColumns) { + for (preview in previewColumnHeadings(CardsOrNotes.NOTES).allColumns) { val (expectedLabel, expectedValue) = when (preview.columnType) { SFLD -> Pair("Sort Field", "Front") @@ -1131,10 +1154,11 @@ val CardBrowserViewModel.column2 fun CardBrowserViewModel.setColumn( index: Int, column: CardBrowserColumn, + cardsOrNotes: CardsOrNotes = this.cardsOrNotes, ): Boolean { val newColumns = availableColumns.toMutableList() newColumns[index] = column - return updateColumns(newColumns) + return updateColumns(newColumns, cardsOrNotes) } val Pair, List>.allColumns