Add new filtered deck options implementation#19669
Add new filtered deck options implementation#19669lukstbit wants to merge 5 commits intoankidroid:mainfrom
Conversation
|
Important Maintainers: This PR contains Strings changes
|
|
Thank you so much!!! From the video (no code viewed):
|
|
If I may give my feedback:
|
bd785df to
07b3bb3
Compare
|
Feedback considered:
It should now.
I already added a help menu item linking to the docs webpage. Adding extra TLDR text in the view feels verbose. Helpful for beginners the very first times when creating filtered decks, useless and just taking space for anyone else.
Implemented it but note that I removed the Options for part of the title, there simply isn't enough space for the full original title: Options for Filtered deck 15:15. I updated the images and video at the top.
Yes I didn't add any styling to them. They should now be more consistent, I also added extra padding to their related content, it provides better structuring IMO.
Yes, that's how that widget works. Not enthusiastic about it either but I'm fine with it. The user can either use the dropdown to bring the popup or enter text to filter the values.
Yes, thanks. I was looking too much at the desktop ui. |
07b3bb3 to
7026df5
Compare
|
I think the remaining checkboxes should also be switches. Also, it would be better if all switches were below "Options", the second filter/delays were above it, and the switches font were normal weight. If you're fine with deviating slightly from Anki Desktopthe "Name" box is unnecessary and the "Build" button could be replaced with a checkmark icon. |
AnkiDroid/src/main/java/com/ichi2/anki/filtered/FilteredDeckOptionsFragment.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/filtered/FilteredDeckOptionsFragment.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/filtered/FilteredDeckOptionsFragment.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/filtered/FilteredDeckOptionsViewModel.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/filtered/FilteredDeckOptionsViewModel.kt
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/filtered/FilteredDeckOptionsViewModel.kt
Show resolved
Hide resolved
AnkiDroid/src/test/java/com/ichi2/anki/filtered/FilteredDeckOptionsViewModelTest.kt
Outdated
Show resolved
Hide resolved
0b62561 to
017c38d
Compare
|
I updated the images.
Tried it and like it enough to implement it, thanks.
Did this on purpose.
Probably but it's also consistent with all the other inputs so I'm keeping it.
Doesn't work, we have two cases Build and Rebuild and the text is important.
Implemented both requests, but note that I used the backend string for seconds. |
AnkiDroid/src/main/java/com/ichi2/anki/filtered/FilteredDeckOptionsFragment.kt
Outdated
Show resolved
Hide resolved
017c38d to
8d788ed
Compare
|
Note that I marked the test: building a filtered deck with no cards fails unless allow empty is checked as flaky on windows. Passed on linux every time I ran the test but in Windows CI it sometimes fails. |
|
reviewer note: I'm expecting to review this and hopefully approve shortly but - I'd like to wait to merge it for just about a week - until 20251217 Goal is to preserve the ability to sync strings via cherry-pick to the release branch for just a little bit after stable goes live and the initial burst of translations comes in when people notice there are new strings in the app This PR deletes strings and that would crash the app if I cherry-pick strings sync and those removed strings were accessed by the stable branch code that still exists. |
8d788ed to
6c7eb7c
Compare
To be used in filtered decks options screen.
Uses the implementation of SingleFragmentActivity and delegates the configuration changes back to the system. Handling configuration changes on our own is not something useful for most fragments(excluding page fragment which are mostly a WebView) and has the potential to introduce quite a few bugs.
Note: this doesn't implement completely the desktop functionality for this screen, everything missing will be added in a future PR.
6c7eb7c to
68a26d0
Compare
david-allison
left a comment
There was a problem hiding this comment.
All optional/nitpicks. Looks GREAT!!!
| showThemedToast( | ||
| context = requireContext(), | ||
| textResource = R.string.something_wrong, | ||
| shortLength = false, |
There was a problem hiding this comment.
This doesn't seem necessary after an error dialog has appeared
| binding.filterLimitInputLayout.error = | ||
| if (filter1State.limit.toIntOrNull() == null) TR.errorsInvalidInputEmpty() else null | ||
| binding.filterCardsInput.setAdapterIfChanged(state.cardOptions, filter1State.index) | ||
| // rescheduling(done here because in filter 2 setup we might exit early) |
There was a problem hiding this comment.
| // rescheduling(done here because in filter 2 setup we might exit early) | |
| // rescheduling (done here because in filter 2 setup we might exit early) |
| /** | ||
| * Starts a [ConfigAwareSingleFragmentActivity] containing this fragment. If [search] or | ||
| * [search2] are provided, they will be used as the default search text. | ||
| * @param did the [DeckId] of a filtered deck. If it's non-zero, load and modify its settings |
There was a problem hiding this comment.
nit: consider different methods for 'create' and 'edit'
| private val did: DeckId | ||
| get() = savedStateHandle.get<Long>(ARG_DECK_ID) ?: 0L |
There was a problem hiding this comment.
document the meaning of 0
consider .require<DeckId> - the value should always be sent to the fragment
| _state.update { Initializing(throwable = backendQueryResult.exceptionOrNull()) } | ||
| return@launch | ||
| } | ||
| val (filteredDeckData, cardsOptions) = backendQueryResult.getOrThrow() |
There was a problem hiding this comment.
nit
This pattern seems unusual:
- check
isFailure- return if
true
- return if
- Then call
getOrThrow
Is there a partition method, or would getOrElse be cleaner?
| val firstFilter = config.getSearchTerms(0) | ||
| val secondFilter = if (config.searchTermsCount > 1) config.getSearchTerms(1) else null | ||
| return FilteredDeckOptions( | ||
| // backend uses 0 for creating a new filtered deck but we use null instead |
There was a problem hiding this comment.
nit; We use a mix, probably should standardize
| Deck.Filtered.SearchTerm | ||
| .newBuilder() | ||
| .setSearch(filter2State.search) | ||
| .setLimit(filter2State.limit.toIntOrNull() ?: 0) | ||
| .setOrder( | ||
| Deck.Filtered.SearchTerm.Order | ||
| .forNumber(filter2State.index), | ||
| ), | ||
| ) |
There was a problem hiding this comment.
searchTerms got better, IMO
Deck.Filtered may be cleaner as a builder instead of using dsl
Subject: [PATCH] a
---
Index: AnkiDroid/src/main/java/com/ichi2/anki/filtered/FilteredDeckOptionsViewModel.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/filtered/FilteredDeckOptionsViewModel.kt b/AnkiDroid/src/main/java/com/ichi2/anki/filtered/FilteredDeckOptionsViewModel.kt
--- a/AnkiDroid/src/main/java/com/ichi2/anki/filtered/FilteredDeckOptionsViewModel.kt (revision 6b787c2253308cb9858ed7137b9dcf103f6822fb)
+++ b/AnkiDroid/src/main/java/com/ichi2/anki/filtered/FilteredDeckOptionsViewModel.kt (date 1768596709543)
@@ -21,7 +21,10 @@
import androidx.lifecycle.viewModelScope
import anki.collection.OpChangesWithId
import anki.decks.Deck
+import anki.decks.DeckKt.FilteredKt.searchTerm
+import anki.decks.DeckKt.filtered
import anki.decks.FilteredDeckForUpdate
+import anki.decks.copy
import anki.decks.filteredDeckForUpdate
import com.ichi2.anki.CollectionManager.withCol
import com.ichi2.anki.filtered.FilteredDeckOptionsFragment.Companion.ARG_DECK_ID
@@ -284,37 +287,32 @@
id = current.id ?: 0
name = current.name
allowEmpty = current.allowEmpty
- val configBuilder =
- Deck.Filtered
- .newBuilder()
- .setReschedule(current.shouldReschedule)
- .setPreviewAgainSecs(current.delayAgain.toIntOrNull() ?: 60)
- .setPreviewHardSecs(current.delayHard.toIntOrNull() ?: 600)
- .setPreviewGoodSecs(current.delayGood.toIntOrNull() ?: 0)
- .addSearchTerms(
- Deck.Filtered.SearchTerm
- .newBuilder()
- .setSearch(current.filter1State.search)
- .setLimit(current.filter1State.limit.toIntOrNull() ?: 0)
- .setOrder(
- Deck.Filtered.SearchTerm.Order
- .forNumber(current.filter1State.index),
- ),
+ val config =
+ filtered {
+ reschedule = current.shouldReschedule
+ previewAgainSecs = current.delayAgain.toIntOrNull() ?: 60
+ previewHardSecs = current.delayHard.toIntOrNull() ?: 600
+ previewGoodSecs = current.delayGood.toIntOrNull() ?: 0
+ searchTerms.add(
+ searchTerm {
+ search = current.filter1State.search
+ limit = current.filter1State.limit.toIntOrNull() ?: 0
+ order = Deck.Filtered.SearchTerm.Order.forNumber(current.filter1State.index)
+ }
)
+ }
val filter2State = current.filter2State
if (current.isSecondFilterEnabled && filter2State != null) {
- configBuilder.addSearchTerms(
- Deck.Filtered.SearchTerm
- .newBuilder()
- .setSearch(filter2State.search)
- .setLimit(filter2State.limit.toIntOrNull() ?: 0)
- .setOrder(
- Deck.Filtered.SearchTerm.Order
- .forNumber(filter2State.index),
- ),
- )
+ config.copy {
+ searchTerms.add(searchTerm {
+ search = filter2State.search
+ limit = filter2State.limit.toIntOrNull() ?: 0
+ order = Deck.Filtered.SearchTerm.Order
+ .forNumber(filter2State.index)
+ })
+ }
}
- config = configBuilder.build()
+ this.config = config
}
companion object {|
If the loading takes some time, the empty button at the top is weird. Screen_recording_20260116_171119.webmAlso, it should not do anything while loading. At the moment, it waits the page to load and instantly build a deck Screen_recording_20260116_171404.webmOne option is to use a simple check icon button instead of using text Tested with Subject: [PATCH] a
---
Index: AnkiDroid/src/main/java/com/ichi2/anki/filtered/FilteredDeckOptionsFragment.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/filtered/FilteredDeckOptionsFragment.kt b/AnkiDroid/src/main/java/com/ichi2/anki/filtered/FilteredDeckOptionsFragment.kt
--- a/AnkiDroid/src/main/java/com/ichi2/anki/filtered/FilteredDeckOptionsFragment.kt (revision 6b787c2253308cb9858ed7137b9dcf103f6822fb)
+++ b/AnkiDroid/src/main/java/com/ichi2/anki/filtered/FilteredDeckOptionsFragment.kt (date 1768593964386)
@@ -49,6 +49,7 @@
import com.ichi2.utils.show
import com.ichi2.utils.title
import dev.androidbroadcast.vbpd.viewBinding
+import kotlinx.coroutines.delay
import kotlinx.coroutines.launch
/**
@@ -117,6 +118,7 @@
}
is FilteredDeckOptions -> {
+ delay(4000)
bindState(state)
// show any errors we might have
if (state.throwable != null) {
|
BrayanDSO
left a comment
There was a problem hiding this comment.
Currently, the only blocking thing for me is the input boxes misalignment
| Intent(context, ConfigAwareSingleFragmentActivity::class.java).apply { | ||
| putExtra(FRAGMENT_NAME_EXTRA, fragmentClass.jvmName) | ||
| putExtra(FRAGMENT_ARGS_EXTRA, arguments) | ||
| val t: SingleFragmentActivity |
| import kotlinx.coroutines.launch | ||
|
|
||
| /** | ||
| * Represents the screen where a filtered deck can be built or rebuilt after updating it's properties. |
There was a problem hiding this comment.
| * Represents the screen where a filtered deck can be built or rebuilt after updating it's properties. | |
| * Represents the screen where a filtered deck can be built or rebuilt after updating its properties. |
|
|
||
| /** | ||
| * Represents the screen where a filtered deck can be built or rebuilt after updating it's properties. | ||
| * |
There was a problem hiding this comment.
Could you please document the behavior of the two loading indicators?
I had to check in the code what each of them do.
| } | ||
|
|
||
| /** Queries the backend for cards search options and [FilteredDeckForUpdate] guarding against any exception. */ | ||
| private fun Collection.safeBackendDataQuery(did: DeckId): Result<Pair<FilteredDeckForUpdate, List<String>>> = |
There was a problem hiding this comment.
Document filtered decks backend data query exceptions
safeBackendDataQuery() and safeAddOrUpdateFilteredDeck() guard against errors while building a filtered deck.
The errors that can be thrown should be documented so other developers know what they should expect
text to peel to a new issue
| Deck.Filtered.SearchTerm | ||
| .newBuilder() | ||
| .setSearch(filter2State.search) | ||
| .setLimit(filter2State.limit.toIntOrNull() ?: 0) | ||
| .setOrder( | ||
| Deck.Filtered.SearchTerm.Order | ||
| .forNumber(filter2State.index), | ||
| ), | ||
| ) |
There was a problem hiding this comment.
searchTerms got better, IMO
Deck.Filtered may be cleaner as a builder instead of using dsl
Subject: [PATCH] a
---
Index: AnkiDroid/src/main/java/com/ichi2/anki/filtered/FilteredDeckOptionsViewModel.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/filtered/FilteredDeckOptionsViewModel.kt b/AnkiDroid/src/main/java/com/ichi2/anki/filtered/FilteredDeckOptionsViewModel.kt
--- a/AnkiDroid/src/main/java/com/ichi2/anki/filtered/FilteredDeckOptionsViewModel.kt (revision 6b787c2253308cb9858ed7137b9dcf103f6822fb)
+++ b/AnkiDroid/src/main/java/com/ichi2/anki/filtered/FilteredDeckOptionsViewModel.kt (date 1768596709543)
@@ -21,7 +21,10 @@
import androidx.lifecycle.viewModelScope
import anki.collection.OpChangesWithId
import anki.decks.Deck
+import anki.decks.DeckKt.FilteredKt.searchTerm
+import anki.decks.DeckKt.filtered
import anki.decks.FilteredDeckForUpdate
+import anki.decks.copy
import anki.decks.filteredDeckForUpdate
import com.ichi2.anki.CollectionManager.withCol
import com.ichi2.anki.filtered.FilteredDeckOptionsFragment.Companion.ARG_DECK_ID
@@ -284,37 +287,32 @@
id = current.id ?: 0
name = current.name
allowEmpty = current.allowEmpty
- val configBuilder =
- Deck.Filtered
- .newBuilder()
- .setReschedule(current.shouldReschedule)
- .setPreviewAgainSecs(current.delayAgain.toIntOrNull() ?: 60)
- .setPreviewHardSecs(current.delayHard.toIntOrNull() ?: 600)
- .setPreviewGoodSecs(current.delayGood.toIntOrNull() ?: 0)
- .addSearchTerms(
- Deck.Filtered.SearchTerm
- .newBuilder()
- .setSearch(current.filter1State.search)
- .setLimit(current.filter1State.limit.toIntOrNull() ?: 0)
- .setOrder(
- Deck.Filtered.SearchTerm.Order
- .forNumber(current.filter1State.index),
- ),
+ val config =
+ filtered {
+ reschedule = current.shouldReschedule
+ previewAgainSecs = current.delayAgain.toIntOrNull() ?: 60
+ previewHardSecs = current.delayHard.toIntOrNull() ?: 600
+ previewGoodSecs = current.delayGood.toIntOrNull() ?: 0
+ searchTerms.add(
+ searchTerm {
+ search = current.filter1State.search
+ limit = current.filter1State.limit.toIntOrNull() ?: 0
+ order = Deck.Filtered.SearchTerm.Order.forNumber(current.filter1State.index)
+ }
)
+ }
val filter2State = current.filter2State
if (current.isSecondFilterEnabled && filter2State != null) {
- configBuilder.addSearchTerms(
- Deck.Filtered.SearchTerm
- .newBuilder()
- .setSearch(filter2State.search)
- .setLimit(filter2State.limit.toIntOrNull() ?: 0)
- .setOrder(
- Deck.Filtered.SearchTerm.Order
- .forNumber(filter2State.index),
- ),
- )
+ config.copy {
+ searchTerms.add(searchTerm {
+ search = filter2State.search
+ limit = filter2State.limit.toIntOrNull() ?: 0
+ order = Deck.Filtered.SearchTerm.Order
+ .forNumber(filter2State.index)
+ })
+ }
}
- config = configBuilder.build()
+ this.config = config
}
companion object {| * Note: do NOT add any configuration changes in the manifest for this activity. Either use [SingleFragmentActivity] | ||
| * or declare your own copy. | ||
| */ | ||
| class ConfigAwareSingleFragmentActivity : SingleFragmentActivity() { |
There was a problem hiding this comment.
For discussion (for a separate issue):
I think that SingleFragmentActivity should be the one NOT handling configuration changes, and implementers should deal with the configuration changes themselves.
Our configuration handling is mostly due to the use of WebViews, so we could have some kind of class named PageActivity, WebViewActivity or even AnkiActivity (and rename the old AnkiActivity to BaseActivity).
Or we could just keep its managed configuration changes as a minimal set and keep things simple.
This comment was marked as outdated.
This comment was marked as outdated.
|
Hello 👋, this PR has had no activity for more than 2 weeks and needs a reply from the author. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically |



Purpose / Description
Initially, while working on #18875, I implemented the filtered deck related part outside of the (old)FilteredDeckOptions like CreateDeckDialog was doing. After thinking about this I decided that it is a better idea to implement a new screen for filtered decks following desktop behavior.
Changes:
The line count is big but two thirds of it is the layout, tests and views setup.
Video with general behavior(Don't keep activities is on):
Screen_recording_20251202_092039.webm
Images with how it looks with different themes
Fixes
DeckSpinnerSelectionand recodeDeckSelectionDialog#18875How Has This Been Tested?
Manually checked the behavior against desktop behavior, ran tests.
Checklist