Skip to content

Improved Review forgotten cards dialog#21379

Open
defkorean wants to merge 1 commit into
ankidroid:mainfrom
defkorean:review-forgotten-cards-dialog
Open

Improved Review forgotten cards dialog#21379
defkorean wants to merge 1 commit into
ankidroid:mainfrom
defkorean:review-forgotten-cards-dialog

Conversation

@defkorean

Copy link
Copy Markdown
Contributor

Purpose / Description

This improves on the Review forgotten cards dialog option in Custom study. It solves most of the tasks listed in the issue below but leaves room for reusable code as I wait on #20678. I'm opening up this pull request to show progress and welcome any feedback for changes and if I can move on with wrapping up the last task, then I will proceed and update here.

Fixes

Approach

It reuses code from SetDueDateDialog.kt and implements the added file from #20520, NonLeadingZeroInputFilter.kt which prevents additional 0s to be added.

Attached below is a screen recording of the emulator showing these changes:

Review.forgotten.cards.mp4

How Has This Been Tested?

Emulated on a Pixel 9, API 36.0

Learning (optional, can help others)

Describe the research stage
What helped me write parts of the solution for this option in custom study dialog that could be helpful for the other options is referencing SetDueDateDialog.kt

Checklist

Please, go through these checks before submitting the PR.

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • UI Changes: You have tested your change using the Google Accessibility Scanner

@github-actions

Copy link
Copy Markdown
Contributor

Important

Maintainers: This PR contains Strings changes

  1. Sync Translations before merging this PR and wait for the action to complete
  2. Review and merge the auto-generated PR in order to sync all user-submitted translations
  3. Sync Translations again and merge the PR so the huge automated string changes caused by merging this PR are by themselves and easy to review

@sanjaysargam sanjaysargam left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When there is no input or 0, then the suffix is showing days
Is this intentional ?

filters += NonLeadingZeroInputFilter

setText(defaultValue)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

revert this line

R.plurals.set_due_date_label_suffix,
defaultValue.toIntOrNull() ?: 0,
)
doOnTextChanged { text, _, _, _ ->

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This might be slightly inefficient
consider doAfterTextChanged which fires once per text change

@sanjaysargam sanjaysargam added Needs Author Reply Waiting for a reply from the original author and removed Needs Review labels Jul 18, 2026
@sanjaysargam

Copy link
Copy Markdown
Member

I see test is failing

TranslationTest > translatable strings do not duplicate GeneratedTranslations FAILED
    org.opentest4j.AssertionFailedError: 1 XML string(s) duplicate a backend translation.
    If caused by a backend update, add to BASELINE_DUPLICATES:
    "Review forgotten cards", // R.string.custom_study_forgotten | TR.customStudyReviewForgottenCards()

@defkorean

Copy link
Copy Markdown
Contributor Author

When there is no input or 0, then the suffix is showing days Is this intentional ?

The former is intentional by design as I reused one of the strings from 03-dialogs.xml which changes from days to day when there's 1 but days for the rest (as seen in the parent issue here).

@defkorean

Copy link
Copy Markdown
Contributor Author

I see test is failing

TranslationTest > translatable strings do not duplicate GeneratedTranslations FAILED
    org.opentest4j.AssertionFailedError: 1 XML string(s) duplicate a backend translation.
    If caused by a backend update, add to BASELINE_DUPLICATES:
    "Review forgotten cards", // R.string.custom_study_forgotten | TR.customStudyReviewForgottenCards()

Actually I was wondering about this with how the translations would work when changing the string in line 38 of the xml file:
<string name="custom_study_forgotten">Review forgotten cards</string>

After doing some reading of the Development Guide, do I need to make a separate pull request for this change to appear?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs Author Reply Waiting for a reply from the original author Strings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve the Review forgotten cards dialog

2 participants