Fix second filter state save in filtered deck screen

Replaces the cards state AutoCompleteTextView(thanks Material theme)
with a Spinner.

Fixes a bug where the second filter state was not saved because the
code considered it null when the deck being loaded didn't already
had the second filter enabled.

Author: lukstbit

AnkiDroid/src/main/java/com/ichi2/anki/filtered/FilteredDeckOptionsFragment.kt

@@ -25,6 +25,7 @@ import android.view.View
 import android.widget.AdapterView
 import android.widget.ArrayAdapter
 import android.widget.CheckBox
+import android.widget.Spinner
 import androidx.appcompat.app.AlertDialog
 import androidx.core.os.bundleOf
 import androidx.core.view.isVisible
@@ -35,7 +36,6 @@ import androidx.lifecycle.Lifecycle
 import androidx.lifecycle.lifecycleScope
 import androidx.lifecycle.repeatOnLifecycle
 import com.google.android.material.materialswitch.MaterialSwitch
-import com.google.android.material.textfield.MaterialAutoCompleteTextView
 import com.google.android.material.textfield.TextInputEditText
 import com.google.android.material.textfield.TextInputLayout
 import com.ichi2.anki.CardBrowser
@@ -52,6 +52,8 @@ import com.ichi2.utils.show
 import com.ichi2.utils.title
 import dev.androidbroadcast.vbpd.viewBinding
 import kotlinx.coroutines.launch
+import java.util.Locale.getDefault
+import kotlin.text.replaceFirstChar
 
 /**
  * Represents the screen where a filtered deck can be built or rebuilt after updating its properties.
@@ -171,7 +173,7 @@ class FilteredDeckOptionsFragment : Fragment(R.layout.fragment_filtered_deck_opt
                 SearchInputError.NotANumber -> TR.errorsInvalidInputEmpty()
                 null -> null
             }
-        binding.filterCardsInput.setAdapterIfChanged(state.cardOptions, filter1State.index)
+        binding.filterCards.setAdapterIfNeeded(state.cardOptions, filter1State.index)
         // rescheduling (done here because in filter 2 setup we might exit early)
         binding.checkBoxReschedule.setCheckedIfChanged(state.shouldReschedule)
         binding.rescheduleDelayAgainInput.setTextIfChanged(state.delayAgain)
@@ -188,7 +190,8 @@ class FilteredDeckOptionsFragment : Fragment(R.layout.fragment_filtered_deck_opt
         }
         binding.secondFilterSearchContainer.isVisible = state.isSecondFilterEnabled
         binding.secondFilterLimitInputLayout.isVisible = state.isSecondFilterEnabled
-        binding.secondFilterCardsInputLayout.isVisible = state.isSecondFilterEnabled
+        binding.secondFilterCardsLabel.isVisible = state.isSecondFilterEnabled
+        binding.secondFilterCards.isVisible = state.isSecondFilterEnabled
         val filter2State = state.filter2State ?: return
         binding.secondFilterSearchInput.setTextIfChanged(filter2State.search)
         binding.secondFilterLimitInput.setTextIfChanged(filter2State.limit)
@@ -198,7 +201,7 @@ class FilteredDeckOptionsFragment : Fragment(R.layout.fragment_filtered_deck_opt
                 SearchInputError.NotANumber -> TR.errorsInvalidInputEmpty()
                 null -> null
             }
-        binding.secondFilterCardsInput.setAdapterIfChanged(state.cardOptions, filter2State.index)
+        binding.secondFilterCards.setAdapterIfNeeded(state.cardOptions, filter2State.index)
     }
 
     private fun TextInputLayout.setupRescheduleDelay(
@@ -228,10 +231,8 @@ class FilteredDeckOptionsFragment : Fragment(R.layout.fragment_filtered_deck_opt
         binding.filterLimitInput.onTextChanged { text ->
             viewModel.onLimitChange(FilterIndex.First, text)
         }
-        binding.filterCardsInput.onItemClickListener =
-            AdapterView.OnItemClickListener { _, _, position, _ ->
-                viewModel.onCardsOptionsChange(FilterIndex.First, position)
-            }
+        binding.filterCards.bindListener(FilterIndex.First)
+
         // filter#2
         binding.switchSecondFilter.onCheckedChanged(viewModel::onSecondFilterStatusChange)
         binding.secondFilterSearchInput.onTextChanged { text ->
@@ -243,10 +244,8 @@ class FilteredDeckOptionsFragment : Fragment(R.layout.fragment_filtered_deck_opt
         binding.secondFilterLimitInput.onTextChanged { text ->
             viewModel.onLimitChange(FilterIndex.Second, text)
         }
-        binding.secondFilterCardsInput.onItemClickListener =
-            AdapterView.OnItemClickListener { _, _, position, _ ->
-                viewModel.onCardsOptionsChange(FilterIndex.Second, position)
-            }
+        binding.secondFilterCards.bindListener(FilterIndex.Second)
+
         // reschedule
         binding.checkBoxReschedule.onCheckedChanged(viewModel::onRescheduleChange)
         binding.rescheduleDelayAgainInput.onTextChanged { text ->
@@ -289,12 +288,13 @@ class FilteredDeckOptionsFragment : Fragment(R.layout.fragment_filtered_deck_opt
         // first filter
         binding.filterSearchInputLayout.hint = TR.actionsSearch()
         binding.filterLimitInputLayout.hint = TR.decksLimitTo()
-        binding.filterCardsInputLayout.hint = TR.decksCardsSelectedBy()
+        binding.filterCardsLabel.text = TR.decksCardsSelectedBy().capitalize()
         // second filter
         binding.switchSecondFilter.text = TR.decksEnableSecondFilter()
         binding.secondFilterSearchInputLayout.hint = TR.actionsSearch()
         binding.secondFilterLimitInputLayout.hint = TR.decksLimitTo()
-        binding.secondFilterCardsInputLayout.hint = TR.decksCardsSelectedBy()
+        binding.secondFilterCardsLabel.text = TR.decksCardsSelectedBy().capitalize()
+
         // buttons
         binding.btnShowExcludedCards.text = TR.decksUnmovableCards()
         binding.checkBoxReschedule.text = TR.decksRescheduleCardsBasedOnMyAnswers()
@@ -316,10 +316,28 @@ class FilteredDeckOptionsFragment : Fragment(R.layout.fragment_filtered_deck_opt
         if (this.isChecked != newChecked) this.isChecked = newChecked
     }
 
-    /** Sets the adapter and selection for [MaterialAutoCompleteTextView] only if its items are different */
-    private fun MaterialAutoCompleteTextView.setAdapterIfChanged(
+    private fun Spinner.bindListener(index: FilterIndex) {
+        onItemSelectedListener =
+            object : AdapterView.OnItemSelectedListener {
+                override fun onItemSelected(
+                    adapterView: AdapterView<*>?,
+                    view: View?,
+                    position: Int,
+                    id: Long,
+                ) {
+                    viewModel.onCardsOptionsChange(index, position)
+                }
+
+                override fun onNothingSelected(adapterView: AdapterView<*>?) {
+                    // do nothing
+                }
+            }
+    }
+
+    /** Sets the adapter and selection for cards [Spinner] only if options are available */
+    private fun Spinner.setAdapterIfNeeded(
         cardOptions: List<String>,
-        selectedIndex: Int,
+        selectedPosition: Int,
     ) {
         if (cardOptions.isNotEmpty()) {
             setAdapter(
@@ -329,10 +347,17 @@ class FilteredDeckOptionsFragment : Fragment(R.layout.fragment_filtered_deck_opt
                     cardOptions,
                 ),
             )
-            setText(cardOptions[selectedIndex], false)
+            setSelection(selectedPosition)
         }
     }
 
+    /**
+     * Needed because backend returns the string TR.decksCardsSelectedBy starting with a lowercase.
+     * This was the recommended replacement implementation for the now deprecated Kotlin
+     * capitalize() method.
+     */
+    private fun String.capitalize() = replaceFirstChar { if (it.isLowerCase()) it.titlecase(getDefault()) else it.toString() }
+
     companion object {
         const val ARG_DECK_ID = "arg_deck_id"
         const val ARG_SEARCH = "arg_search"

AnkiDroid/src/main/java/com/ichi2/anki/filtered/FilteredDeckOptionsViewModel.kt

@@ -156,7 +156,12 @@ class FilteredDeckOptionsViewModel(
     fun onSecondFilterStatusChange(isEnabled: Boolean) {
         Timber.i("Second filter status is changing to $isEnabled")
         if (currentState().isSecondFilterEnabled == isEnabled) return
-        updateCurrentState { copy(isSecondFilterEnabled = isEnabled) }
+        updateCurrentState {
+            copy(
+                isSecondFilterEnabled = isEnabled,
+                filter2State = filter2State ?: SearchTermState(),
+            )
+        }
     }
 
     fun onRescheduleChange(isEnabled: Boolean) {

AnkiDroid/src/main/res/layout/fragment_filtered_deck_options.xml

@@ -157,23 +157,21 @@
                     />
             </com.google.android.material.textfield.TextInputLayout>
 
-            <com.google.android.material.textfield.TextInputLayout
-                android:id="@+id/filter_cards_input_layout"
-                style="@style/Widget.Material3.TextInputLayout.OutlinedBox.Dense.ExposedDropdownMenu"
+            <TextView
+                android:id="@+id/filter_cards_label"
                 android:layout_width="match_parent"
                 android:layout_height="wrap_content"
-                android:layout_marginTop="4dp"
-                tools:hint="Cards selected by">
+                android:layout_marginTop="8dp"
+                android:textAppearance="@style/TextAppearance.Material3.LabelLarge"
+                tools:text="Cards selected by"
+                />
 
-                <com.google.android.material.textfield.MaterialAutoCompleteTextView
-                    android:id="@+id/filter_cards_input"
-                    android:layout_width="match_parent"
-                    android:layout_height="wrap_content"
-                    android:inputType="textFilter"
-                    app:dropDownBackgroundTint="?android:attr/colorBackground"
-                    tools:text="Random"
-                    />
-            </com.google.android.material.textfield.TextInputLayout>
+            <Spinner
+                android:id="@+id/filter_cards"
+                android:layout_width="match_parent"
+                android:layout_height="wrap_content"
+                android:minHeight="48dp"
+                />
 
             <!-- Material3Fix after colorSurface* are fixed look into using one here -->
             <com.google.android.material.materialswitch.MaterialSwitch
@@ -231,24 +229,23 @@
                     />
             </com.google.android.material.textfield.TextInputLayout>
 
-            <com.google.android.material.textfield.TextInputLayout
-                android:id="@+id/second_filter_cards_input_layout"
-                style="@style/Widget.Material3.TextInputLayout.OutlinedBox.Dense.ExposedDropdownMenu"
+            <TextView
+                android:id="@+id/second_filter_cards_label"
                 android:layout_width="match_parent"
                 android:layout_height="wrap_content"
+                android:layout_marginTop="8dp"
                 android:visibility="gone"
-                android:layout_marginTop="4dp"
-                tools:hint="Cards selected by">
+                android:textAppearance="@style/TextAppearance.Material3.LabelLarge"
+                tools:text="Cards selected by"
+                />
 
-                <com.google.android.material.textfield.MaterialAutoCompleteTextView
-                    android:id="@+id/second_filter_cards_input"
-                    android:layout_width="match_parent"
-                    android:layout_height="wrap_content"
-                    android:inputType="textFilter"
-                    app:dropDownBackgroundTint="?android:attr/colorBackground"
-                    tools:text="Order due"
-                    />
-            </com.google.android.material.textfield.TextInputLayout>
+            <Spinner
+                android:id="@+id/second_filter_cards"
+                android:layout_width="match_parent"
+                android:layout_height="wrap_content"
+                android:visibility="gone"
+                android:minHeight="48dp"
+                />
 
             <com.ichi2.ui.FixedTextView
                 android:id="@+id/options_label"

AnkiDroid/src/test/java/com/ichi2/anki/filtered/FilteredDeckOptionsViewModelTest.kt

@@ -18,6 +18,10 @@ package com.ichi2.anki.filtered
 
 import androidx.lifecycle.SavedStateHandle
 import androidx.test.ext.junit.runners.AndroidJUnit4
+import anki.decks.Deck
+import anki.decks.DeckKt.FilteredKt.searchTerm
+import anki.decks.DeckKt.filtered
+import anki.decks.filteredDeckForUpdate
 import com.ichi2.anki.CollectionManager.withCol
 import com.ichi2.anki.Flag
 import com.ichi2.anki.RobolectricTest
@@ -253,6 +257,38 @@ class FilteredDeckOptionsViewModelTest : RobolectricTest() {
             }
         }
 
+    @Test
+    fun `a deck without a second filter gets a new fresh filter state when second filter is enabled`() =
+        runTest {
+            addDeck("A", true)
+            addNoteToDeckA { flagCardForNote(this, Flag.RED) }
+            addNoteToDeckA { flagCardForNote(this, Flag.GREEN) }
+            // setup only the first filter
+            val data =
+                filteredDeckForUpdate {
+                    id = 0
+                    name = "filtered"
+                    config =
+                        filtered {
+                            searchTerms.add(
+                                searchTerm {
+                                    search = "flag:1"
+                                    limit = 5
+                                    order =
+                                        Deck.Filtered.SearchTerm.Order
+                                            .forNumber(1)
+                                },
+                            )
+                        }
+                }
+            val changes = withCol { sched.addOrUpdateFilteredDeck(data) }
+            withViewModel(changes.id) {
+                assertNull(current.filter2State)
+                onSecondFilterStatusChange(true)
+                assertNotNull(current.filter2State)
+            }
+        }
+
     /** Returns the current state as a [FilteredDeckOptions] or throw otherwise */
     private val FilteredDeckOptionsViewModel.current: FilteredDeckOptions
         get() = state.value as FilteredDeckOptions