Anki-Android icon indicating copy to clipboard operation
Anki-Android copied to clipboard

Entry points for review reminders

Open ericli3690 opened this issue 7 months ago • 10 comments

Purpose / Description

  • Created the ScheduleReminders fragment in a new reviewreminders package
  • Added a new string resource for ScheduleReminders to 12-dont-translate.xml
  • Added buttons to launch the ScheduleReminders activity from DeckPicker, StudyOptionsFragment, and HeaderFragment
  • Created ScheduleRemindersDestination to comply with new DeckPickerViewModel code
  • Made HeaderFragment index the notifications and review reminders options conditionally
  • Updated a pre-existing DeckPickerTest test to also test the ScheduleReminders button
  • Made toggling the review reminders developer option reload the settings fragment

image image

Fixes

  • Part of GSoC 2025: Review Reminders

Approach

  • Currently starting by creating the space within which the new system will function.
  • Created a new ScheduleReminders activity and defined the deck-specific and global editing scopes. This deviates slightly from my original GSoC proposal as per discussions on Discord: [1] [2]
  • This is updated from the originally planned name, EditNotifications.
  • The next steps will be to fill in the RecyclerView within ScheduleReminders and to begin trying to create/read/update/delete notification data.

How Has This Been Tested?

  • DeckPickerTest executes successfully.
  • Opening ScheduleReminders through all created routes (long pressing a deck in the DeckPicker screen, clicking the toolbar bell icon in StudyOptionsFragment, and clicking the option in Settings) works and displays expected information.
  • When the developer option is disabled, no new changes are accessible.
  • Tested on a physical Samsung S23, API 34.

Learning

  • This took me a while longer than I expected because I suddenly ran into some problems first with building and then with executing tests within DeckPickerTest. I think Android Studio didn't like running the parameterized tests for some reason and I had to manually edit the build configuration of each individual test before hitting run.
  • I initially was trying to figure out how to import a new bell icon and even asked around the Discord server for the process for importing svgs before realizing there was already a bell icon in the resources folder. Whoops.
  • Decided to remove the cumbersome three editing scopes and just focus on two to improve the UX experience.

Checklist

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

ericli3690 avatar May 16 '25 05:05 ericli3690

Apologies for the repeated force pushes, my code is failing a few tests. Running the tests on my own machine is really annoying because a lot of them are flaky (in particular I'm running up against this known bug here: #14796). Does anyone know if there's a fix for that issue? Discussion in that thread on fixing it seems to have stalled out.

ericli3690 avatar May 16 '25 06:05 ericli3690

For the future, in your PR and commit message, please uses backticks instead of square brackets. This way, github shows this as code.

Square bracket is great in kotlin documentation, because that's the way javadoc works and javadoc links to other part of the code. But it's not something that github or most git tools know how to interprets. So instead we should use markdown and not javadoc syntax

I'm not asking you to change the current commits or PR. I don't care enough about this issue to ask you to do extra work here

Arthur-Milchior avatar May 17 '25 14:05 Arthur-Milchior

Apologies for the square brackets. I've corrected them to backticks where possible and will continue to do so in the future.

As per discussions on Discord, I've removed basically all edits to 12-dont-translate.xml and am now hardcoding all strings as literals within the code itself. I'll pull them out into a string resource file in August once the feature is stable. The exception is "Edit notifications", which I need to keep as a string resource so that DeckPickerContextMenu can accept it as an argument in an enum declaration (it doesn't work if I just pass in the string literal), and so that it can be used as a title in study_options_fragment.xml (the linter gets mad if I don't). For now, I've kept this single resource in 12-dont-translate.xml. Let me know if I should move it somewhere else.

ericli3690 avatar May 21 '25 02:05 ericli3690

Ah, something I've just thought of. Should I rename EditNotifications to EditReviewReminders? I've noticed that I sometimes substitute the word "notifications" for "review reminders" in my code which is probably not good. It might be best to standardize the name of this feature. Is "Review Reminders" the final official English name for this feature? It was the original name of this idea on the GSoC ideas list.

ericli3690 avatar May 22 '25 03:05 ericli3690

As per @lukstbit's recommendation, I've rebased and squashed all my commits together. I've done a major refactoring to switch EditNotifications into ScheduleReminders and am now using a fragment within SingleFragmentActivity rather than creating a whole new activity. The UI has changed marginally (the top bar is now white instead of blue) in keeping with how most other SingleFragmentActivity fragments are styled within AnkiDroid. Updated images are in the PR description.

ericli3690 avatar May 28 '25 05:05 ericli3690

Please do not review this PR. I'm actively refactoring it as I've realized I can streamline the UX. Sorry for asking for reviews and then going back and changing things again, I really do appreciate the constructive criticism of my code and am sorry for pulling it out of the merge pipeline when it's so close to the end.

ericli3690 avatar Jun 09 '25 04:06 ericli3690

Refactored. UI images in the PR description have been updated. Looking forward to reviews!

ericli3690 avatar Jun 14 '25 18:06 ericli3690

Thanks Ashish and Sanjay! Updated.

ericli3690 avatar Jun 14 '25 23:06 ericli3690

  • Replaced launchCatchingIO with viewModelScope.launch

ericli3690 avatar Jun 15 '25 16:06 ericli3690

Cool! Thanks for the review. Since there are two approvals, I can mark this as Pending Merge, right?

ericli3690 avatar Jun 18 '25 01:06 ericli3690

I've incorporated the above suggestions into my code and will open up a PR for the updates once https://github.com/ankidroid/Anki-Android/pull/18364 is merged.

ericli3690 avatar Jun 21 '25 04:06 ericli3690