Refactor ForgetCardsDialog and potentially fix a bug #17176
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mikehardy
approved these changes
Oct 1, 2024
mikehardy
added
Needs Second Approval
david-allison
previously requested changes
Oct 2, 2024
AnkiDroid/src/main/java/com/ichi2/anki/scheduling/ForgetCardsDialog.kt
Outdated
Show resolved
Hide resolved
david-allison
added
the
Needs Author Reply
The refactoring mainly removes the associated ViewModel as it only had two simple properties and a method to call an activity method to do its work. ForgetCardsDialog was refactored: - simplify the dialog customization - simplify the dialog itself by removing the need to pass the target cards ids. With these changes the dialog becomes a simple UI panel from which the user to select his desired options. The cards ids are fetched by the activity itself when they are actually needed - use modern communication(FragmentResult) between the dialog fragment and the holder activity doing the work - create utility methods so activities using the dialog can easily do so
The call scheduler.forgetCards() produces OpChanges for: browser_table: true card: true mtime: true study_queues: true
lukstbit
force-pushed
the
refactor_forgetCardsDialog
branch
from
b6d3f0d
to
07303e9
Compare
lukstbit
added
Pending Merge
lukstbit
dismissed
david-allison’s stale review
Review request was handled, used undoableOp
github-actions
bot
removed
the
Pending Merge
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose / Description
First commit is a refactoring for the code around ForgetCardsDialog, looked verbose for what it was doing.
Second commit is an attempt to fix the linked issue. With that change when the current card's progress is reset the reviewer removes the card and changes the counts. This seems to be the behavior on desktop, at least from what I saw. I'm not sure about this, but I also don't see any issues with having that call in production.
There are no UI changes but just to be transparent:
Fixes
How Has This Been Tested?
Ran the tests, everything seems to work.
Checklist