Skip to content

Skip tag selection when no tags are available in the deck #18562

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
+35 −7
Open

Skip tag selection when no tags are available in the deck #18562

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c108ada
Skip tag selection when no tags are available in the deck
Amardurai Jun 18, 2025
ce72558
Update AnkiDroid/src/main/java/com/ichi2/anki/dialogs/customstudy/Cus…
Amardurai Jun 19, 2025
60fb6ce
Fix lint check failures
Amardurai Jun 19, 2025
e76f59a
Add requiresDefaults flag to ContextMenuOption and update assertion f…
Amardurai Jun 19, 2025
340f1df
Pass correct CramKind when tags are not available in CustomStudyDialog
Amardurai Jun 19, 2025
57f1c4b
Merge branch 'main' into 18479-fix-custom-study-navigation
Amardurai Jun 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 35 additions & 7 deletions AnkiDroid/src/main/java/com/ichi2/anki/dialogs/customstudy/CustomStudyDialog.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,8 @@ class CustomStudyDialog : AnalyticsDialogFragment() {
*/
@NeedsTest("17757: fragment not dismissed before result is output")
private fun buildInputDialog(contextMenuOption: ContextMenuOption): AlertDialog {
require(deferredDefaults.isCompleted || selectedSubDialog!!.checkAvailability == null)
require(deferredDefaults.isCompleted || selectedSubDialog?.requiresDefaults == false)

/*
TODO: Try to change to a standard input dialog (currently the thing holding us back is having the extra
TODO: hint line for the number of cards available, and having the pre-filled text selected by default)
Expand All @@ -311,6 +312,12 @@ class CustomStudyDialog : AnalyticsDialogFragment() {
setDropDownViewResource(android.R.layout.simple_spinner_dropdown_item)
}
}

val canChooseTags =
Copy link
Member

@david-allison david-allison Jun 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should block - requiring deferredDefaults to be completed before the screen is launched.

This is currently done:

Anki-Android/AnkiDroid/src/main/java/com/ichi2/anki/dialogs/customstudy/CustomStudyDialog.kt

Lines 188 to 196 in 60fb6ce

// on a slow phone, 'extend limits' may be clicked before we know there's no new/review cards
// show 'no cards due' if this occurs
if (item.checkAvailability != null) {
val defaults = withProgress { deferredDefaults.await() }
if (!item.checkAvailability(defaults)) {
showSnackbar(getString((R.string.studyoptions_no_cards_due)))
return
}
}

Since this no longer just means checkAvailability, we should also have a requiresDefaults property on ContextMenuOption, and modify the above assertion:

require(deferredDefaults.isCompleted || selectedSubDialog!!.checkAvailability == null)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood, Added the new requiresDefaults field to ContextMenuOption and updated the assertion!

deferredDefaults.isCompleted &&
deferredDefaults.getCompleted().tags.isNotEmpty() &&
contextMenuOption == STUDY_TAGS

val editText =
v.findViewById<EditText>(R.id.custom_study_details_edittext2).apply {
setText(defaultValue)
Expand All @@ -323,7 +330,7 @@ class CustomStudyDialog : AnalyticsDialogFragment() {
}
}
val positiveBtnLabel =
if (contextMenuOption == STUDY_TAGS) {
if (canChooseTags) {
TR.customStudyChooseTags().toSentenceCase(requireContext(), R.string.sentence_choose_tags)
} else {
getString(R.string.dialog_ok)
Expand Down Expand Up @@ -365,15 +372,24 @@ class CustomStudyDialog : AnalyticsDialogFragment() {
allowSubmit = true
return@setOnClickListener
}
if (contextMenuOption == STUDY_TAGS) {
if (canChooseTags) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ This doesn't seem to pass in the CramKind selection

Anki-Android/AnkiDroid/src/main/java/com/ichi2/anki/dialogs/customstudy/CustomStudyDialog.kt

Lines 153 to 164 in 60fb6ce

if (selectedStatePosition == AdapterView.INVALID_POSITION) return@setFragmentResultListener
val kind = CustomStudyCardState.entries[selectedStatePosition].kind
val cardsAmount = userInputValue ?: 100 // the default value
requireActivity().launchCatchingTask {
withProgress {
try {
customStudy(option, cardsAmount, kind, tagsToInclude, tagsToExclude)
} finally {
requireActivity().dismissAllDialogFragments()
}
}
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're already passing the CramKind as the kind parameter here:

val kind = CustomStudyCardState.entries[selectedStatePosition].kind
...
customStudy(option, cardsAmount, kind, tagsToInclude, tagsToExclude)

So the CramKind is already being handled. Let me know if I missed anything!

Copy link
Member

@david-allison david-allison Jun 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If canChooseTags is false AND contextMenuOption == STUDY_TAGS: A user has selected a CramKind

The following code is then executed, which does not use the CramKind: customStudy(contextMenuOption, n)

customStudy is defined as the following, which uses a default value of cramKind instead.

Given that this bug was flagged in a TODO, it may be a good idea to create a sealed class which blocks this from happening again

    // TODO cram kind and the included/excluded tags lists are only relevant for STUDY_TAGS and
    //  should be included in the option to not leak in the method's api
    private suspend fun customStudy(
        contextMenuOption: ContextMenuOption,
        userEntry: Int,
        cramKind: CramKind = CramKind.CRAM_KIND_NEW,

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarification!

I've updated the code to explicitly pass the CramKind, even when canChooseTags is false, like this:

requireActivity().launchCatchingTask {
    withProgress {
        try {
            val kind = CustomStudyCardState.entries[selectedStatePosition].kind
            customStudy(contextMenuOption, n, kind)
        } finally {
            requireActivity().dismissAllDialogFragments()
        }
    }
}

Looks good?

Copy link
Member

@david-allison david-allison Jun 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you test it?

EDIT: It feels like I'm talking with an LLM right now... I can't see how that would work given the definition:

Anki-Android/AnkiDroid/src/main/java/com/ichi2/anki/dialogs/customstudy/CustomStudyDialog.kt

Lines 136 to 140 in 60fb6ce

private val selectedStatePosition: Int
get() =
dialog
?.findViewById<Spinner>(R.id.cards_state_selector)
?.selectedItemPosition ?: AdapterView.INVALID_POSITION

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I’ve tested with all four options, and it’s working as expected.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did you test it, it doesn't compile.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2025-06-19 at 11 28 00 PM

// mark allowSubmit as true because, if the user cancels TagLimitFragment, when
// we come back we wouldn't be able to trigger again TagLimitFragment
allowSubmit = true
val tagsSelectionDialog = TagLimitFragment.newInstance(dialogDeckId)
tagsSelectionDialog.show(parentFragmentManager, TagLimitFragment.TAG)
return@setOnClickListener
}
launchCustomStudy(contextMenuOption, n)
requireActivity().launchCatchingTask {
withProgress {
try {
val kind = CustomStudyCardState.entries[selectedStatePosition].kind
customStudy(contextMenuOption, n, kind, emptyList(), emptyList())
} finally {
requireActivity().dismissAllDialogFragments()
}
}
}
}
}

Expand Down Expand Up @@ -546,12 +562,21 @@ class CustomStudyDialog : AnalyticsDialogFragment() {
enum class ContextMenuOption(
val getTitle: Resources.() -> String,
val checkAvailability: ((CustomStudyDefaults) -> Boolean)? = null,
val requiresDefaults: Boolean = false,
) {
/** Increase today's new card limit */
EXTEND_NEW({ TR.customStudyIncreaseTodaysNewCardLimit() }, checkAvailability = { it.extendNew.isUsable }),
EXTEND_NEW(
getTitle = { TR.customStudyIncreaseTodaysNewCardLimit() },
checkAvailability = { it.extendNew.isUsable },
requiresDefaults = true,
),

/** Increase today's review card limit */
EXTEND_REV({ TR.customStudyIncreaseTodaysReviewCardLimit() }, checkAvailability = { it.extendReview.isUsable }),
EXTEND_REV(
getTitle = { TR.customStudyIncreaseTodaysReviewCardLimit() },
checkAvailability = { it.extendReview.isUsable },
requiresDefaults = true,
),

/** Review forgotten cards */
STUDY_FORGOT({ TR.customStudyReviewForgottenCards() }),
Expand All @@ -563,7 +588,10 @@ class CustomStudyDialog : AnalyticsDialogFragment() {
STUDY_PREVIEW({ TR.customStudyPreviewNewCards() }),

/** Limit to particular tags */
STUDY_TAGS({ TR.customStudyStudyByCardStateOrTag() }),
STUDY_TAGS(
getTitle = { TR.customStudyStudyByCardStateOrTag() },
requiresDefaults = true,
),
}

@Parcelize
Expand Down