Enable deck option target selection in study screen#19963
Enable deck option target selection in study screen#19963lukstbit wants to merge 1 commit intoankidroid:mainfrom
Conversation
|
Awesome! Something feels like it's missing visually, but I don't know what What happens when a deck in the list is long and truncation occurs? Does What if there's a lot of entries in the list? I'd be tempted to add the card count as a subtitle and see how this looks Can you colour the filtered deck options the same colour as in the deck picker? Does cancel need to be there? Back or tapping outside should be sufficient |
|
I might not be seeing clearly, but the dialog title doesn't feel prominent enough. Also, the Material 2 guidelines agree with david-allison. |
354ef23 to
87279d8
Compare
|
I updated the images.
Just wraps around and makes the row bigger. I think this is better than truncating.
At most there could be 3 entries based on the implementation.
I don't think this is a good idea: it would complicate the implementation and I'm not sure what value offers to the user(besides desktop doesn't show this).
Should've done this from the start.
No and I removed it. I added it initially to follow the desktop ui. I updated the dialog appearance by using MaterialAlertDialogBuilder. It has the same surface/container colors issue but this will be fixed later. The contrast for dynamic color on black theme is not great but I'm tempted to leave it for now and wait for the surface colors to be addressed. Note on the previous design: I just used the normal dialog. Having the title so long probably feels like a message text and not a title. |
AnkiDroid/src/main/java/com/ichi2/anki/pages/DeckOptionsDestination.kt
Outdated
Show resolved
Hide resolved
david-allison
left a comment
There was a problem hiding this comment.
Looks good! A couple questions
Also could do with @NeedsTest
AnkiDroid/src/main/java/com/ichi2/anki/ui/windows/reviewer/ReviewerFragment.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/ui/windows/reviewer/ReviewerViewModel.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/ui/windows/reviewer/ReviewerFragment.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/ui/windows/reviewer/ReviewerViewModel.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/ui/windows/reviewer/ReviewerViewModel.kt
Outdated
Show resolved
Hide resolved
|
|
||
| viewModel.destinationFlow.collectIn(lifecycleScope) { destination -> | ||
| if (destination is DeckOptionsDestination && destination.haMultipleOptions) { | ||
| if (destination.options.any { it.name == null }) { |
There was a problem hiding this comment.
Is there any other way to handle a 'bad' deck as a non-representable state
for example: making the option itself null, rather than relying on the implicit assumption of name == null => invalid'.
There was a problem hiding this comment.
making the option itself null, rather than relying on the implicit assumption of name == null => invalid'.
I modified the code to filter out the options that have a null deck name.
AnkiDroid/src/main/java/com/ichi2/anki/ui/windows/reviewer/ReviewerFragment.kt
Outdated
Show resolved
Hide resolved
87279d8 to
3f64421
Compare
The implementation is inserted around the current code so other usages outside of the new study screen work as before. See https://github.com/lukstbit/anki/blob/d24d2e33943af2361b5a9880572b30887efcf3ee/qt/aqt/deckoptions.py#L83-L100
3f64421 to
df78cbb
Compare
|
I think I fixed all raised issues. |
| ) | ||
| startActivity(updatedDestination.toIntent(requireContext())) | ||
| } | ||
| return@collectIn |
There was a problem hiding this comment.
Is there a need for this return?
| val card = currentCard.await() | ||
| val options = getDeckOptionsTargets(deckId, card) | ||
| val isFiltered = options.first { it.deckId == deckId }.isFiltered | ||
| val destination = DeckOptionsDestination(deckId, isFiltered, options) |
There was a problem hiding this comment.
nit: this logic could be inside either: DeckOptionsDestination.create(deckId) or DeckOptionsDestination.from(deckId, options), one doing the isFiltered check, one extracting the logic
This feels like a general utility, rather than a responsibility of the study screen
|
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
Added an implementation for this feature around the current code of DeckOptionsDestination. I'm open for suggestions on UI, I just added a simple dialog:
Normal decks(and with long names) + filtered decks
Notice: the contrast isn't great for filtered deck color and black background.
Fixes
How Has This Been Tested?
Opened a lot of decks options screens, verified other usages of DeckOptionsDestination, ran tests.
Checklist