feat(reminders): streamline review reminders UI#21140
Conversation
|
Snapshot diff report vs
All 16 changed screenshotsAddEditReminderDialogScreenshotTest
PreferencesScreenshotTest
ReviewRemindersScreenshotTest
|
There was a problem hiding this comment.
Pull request overview
This PR refactors how the “Review reminders” screens are routed and displayed across Settings, Study Options, and DeckPicker, moving toward fragment-based navigation with more flexible toolbar handling.
Changes:
- Route “Review reminders” as a
ScheduleRemindersFragmentdestination (Settings header, preference search, Study Options, DeckPicker side panel). - Rework the Schedule Reminders UI to support multiple host contexts (external activity toolbar vs internal collapsible/non-collapsible toolbars).
- Add a
FragmentManager.showDialogFragment()helper and scope reminder dialogs tochildFragmentManager.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| AnkiDroid/src/main/res/xml/preferences_review_reminders.xml | Adds a placeholder XML to provide a stable @XmlRes routing key for preference search. |
| AnkiDroid/src/main/res/xml/preference_headers.xml | Routes “Review reminders” header directly to ScheduleRemindersFragment. |
| AnkiDroid/src/main/res/values/styles.xml | Adds a TopAppBar subtitle text appearance for the collapsible toolbar. |
| AnkiDroid/src/main/res/layout/fragment_schedule_reminders.xml | Reworks layout to include an AppBar + (optional) collapsing toolbar patterns. |
| AnkiDroid/src/main/res/layout/fragment_reminder_troubleshooting.xml | Updates toolbar id/structure and system window fitting. |
| AnkiDroid/src/main/res/layout/activity_study_options.xml | Enables fitsSystemWindows on the root layout. |
| AnkiDroid/src/main/java/com/ichi2/anki/utils/ext/Fragment.kt | Makes onWindowFocusChanged cleanup safer against dead ViewTreeObserver. |
| AnkiDroid/src/main/java/com/ichi2/anki/utils/Dialog.kt | Adds FragmentManager.showDialogFragment() convenience wrapper. |
| AnkiDroid/src/main/java/com/ichi2/anki/StudyOptionsFragment.kt | Emits a fragment result to request opening review reminders instead of starting an activity. |
| AnkiDroid/src/main/java/com/ichi2/anki/StudyOptionsActivity.kt | Listens for the fragment result and opens ScheduleRemindersFragment in-place with back stack support. |
| AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ScheduleRemindersFragment.kt | Introduces host-dependent toolbar strategy, new fragment factory, and dialog scoping changes. |
| AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReminderTroubleshootingFragment.kt | Adds support for using an external activity toolbar when embedded in certain hosts. |
| AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/AddEditReminderDialog.kt | Adds rotation workaround for MaterialTimePicker callbacks and adjusts fragment result listening. |
| AnkiDroid/src/main/java/com/ichi2/anki/preferences/Preferences.kt | Special-cases preference search routing to open ScheduleRemindersFragment. |
| AnkiDroid/src/main/java/com/ichi2/anki/preferences/HeaderFragment.kt | Updates preference search indexing/routing and fragment-to-header mapping for reminders. |
| AnkiDroid/src/main/java/com/ichi2/anki/NoteEditorFragment.kt | Migrates toolbar API usage to setToolbarText. |
| AnkiDroid/src/main/java/com/ichi2/anki/multimedia/MultimediaFragment.kt | Migrates toolbar API usage to setToolbarText. |
| AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt | Opens schedule reminders either in the side panel (large screens) or via the existing destination flow. |
| AnkiDroid/src/main/java/com/ichi2/anki/AnkiActivity.kt | Replaces setToolbarTitle with setToolbarText (title + optional subtitle). |
Comments suppressed due to low confidence (1)
AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ScheduleRemindersFragment.kt:265
- Toolbar title is set via hardcoded string literal ("Review reminders") when using the activity toolbar. This is user-visible text; consider using a string resource (and keep it consistent with the Settings header title).
retrieveSubtitle { subtitle ->
setToolbarText("Review reminders", subtitle)
}
invalidateMenu()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
59c7f71 to
3774c60
Compare
| .withTitle("Review reminders") | ||
| .withResId(R.xml.preference_headers) | ||
| .withResId(R.xml.preferences_review_reminders) | ||
| .withSummary("Notifications") |
There was a problem hiding this comment.
If the user searches for "notif" or "notification" in the search bar, I want them to be able to find this preference. Hence this withSummary.
| * TODO: Split SingleFragmentActivity into two activities | ||
| * One with `android:configChanges="orientation"` for hosting WebView fragments | ||
| * and one without for hosting non-WebView fragments like this one. |
There was a problem hiding this comment.
Should I make an issue for this?
There was a problem hiding this comment.
ConfigAwareSingleFragmentActivity
| (arguments ?: bundleOf()).getParcelableCompat<ReviewReminderScope>(ARGS_SCOPE) | ||
| ?: ReviewReminderScope.Global |
There was a problem hiding this comment.
This "soft-failure" defaulting is actually a used codepath, as it's tricky to pass arguments to ScheduleRemindersFragment when the Preference is clicked in Settings. See preference_headers.xml:
<com.ichi2.preferences.HeaderPreference
android:fragment="com.ichi2.anki.reviewreminders.ScheduleRemindersFragment"There was a problem hiding this comment.
Comment that it's not dead code.
Is there a reason you didn't set a click handler on the Header?
There was a problem hiding this comment.
Comment added.
Good point; I considered it, but it would mean adding custom logic for exactly one of the HeaderPreferences hosted in the Settings menu. Having a bespoke setOnPreferenceClickListener feels ugly to me; I prefer following the local pattern in Preferences and using this default instead.
| (arguments ?: bundleOf()).getParcelableCompat<FragmentHost>(ARGS_HOST) | ||
| ?: FragmentHost.SETTINGS |
| launchCatchingTask { | ||
| binding.toolbar.subtitle = scope.getDeckName() | ||
| override fun onDestroyView() { | ||
| troubleshootingSnackbar?.dismiss() |
There was a problem hiding this comment.
This is necessary if, for example, the user navigates back from the ScheduleRemindersFragment to the StudyOptionsFragment. Since ScheduleRemindersFragment is no longer its own activity, we need to dismiss the snackbar before relinquishing control.
| * Sets up the troubleshooting snackbar which is shown persistently when checks find a warning/error. | ||
| * Tapping "Fix" opens the full troubleshooting screen. | ||
| */ | ||
| private suspend fun setupTroubleshootingSnackbar() { |
There was a problem hiding this comment.
Nothing was changed inside this method, all I did was extract it out to clean up onViewCreated.
| * TODO: Implement edge-to-edge for both [SingleFragmentActivity] and its child, PreferencesActivity | ||
| * After doing so, this insets-handling logic and the use of fitsSystemWindows in the layout file | ||
| * can be re-evaluated. UI and the collapsing toolbar should be tested on both small and wide screens | ||
| * with all app display themes from all possible locations this fragment can be opened from. |
There was a problem hiding this comment.
Should I create an issue for this?
| dismissAllDialogFragments() | ||
| openScheduleReminders(deckId) |
There was a problem hiding this comment.
The order of these needed to be reversed or else the opened scheduled reminders screen would be immediately popped by the dialog fragment dismissal, which pops every fragment added between it and the dialog fragment, inclusive.
|
|
||
| private fun tryShowScheduleRemindersPanel(deckId: DeckId): Boolean { | ||
| val sidePanel = binding.studyoptionsFragment ?: return false | ||
| if (!sidePanel.isVisible) return false |
There was a problem hiding this comment.
This early return is necessary in the scenario where the collection is empty and the side panel is hidden.
| if (!fragmented) { | ||
| requireAnkiActivity().setToolbarText(R.string.empty_string) | ||
| } |
There was a problem hiding this comment.
Necessary because otherwise on narrow screens, the ScheduleRemindersFragment text will persist once we press back and return to StudyOptionsFragment if we are hosted within StudyOptionsActivity.
Should not be done on wide screens because otherwise the DeckPicker title gets deleted.
|
|
||
| <com.google.android.material.appbar.MaterialToolbar | ||
| android:id="@+id/toolbar" | ||
| android:id="@+id/troubleshooting_toolbar" |
There was a problem hiding this comment.
Done because of a quirk where the Settings screen manually hides all toolbars on wide screens for stylistic purposes. However, it's better UX to have it for the nested troubleshooting screen. The simplest quick fix is to just change the id.
| android:layout_width="match_parent" | ||
| android:layout_height="match_parent"> | ||
| android:layout_height="?collapsingToolbarLayoutMediumSize" | ||
| app:expandedTitleMarginTop="80dp" |
There was a problem hiding this comment.
expandedTitleMarginTop is required to give the deck name subtitle a bit more room to breathe.
|
I partially edited the description to bring Brayan's message into the issue description. Discord may not be long-term stable |
| * TODO: Split SingleFragmentActivity into two activities | ||
| * One with `android:configChanges="orientation"` for hosting WebView fragments | ||
| * and one without for hosting non-WebView fragments like this one. |
There was a problem hiding this comment.
ConfigAwareSingleFragmentActivity
david-allison
left a comment
There was a problem hiding this comment.
Wow, LGTM, extremely well-designed
| */ | ||
| private val scheduleRemindersScope: ReviewReminderScope by lazy { | ||
| requireArguments().getParcelableCompat<ReviewReminderScope>(ARGS_SCOPE) | ||
| (arguments ?: bundleOf()).getParcelableCompat<ReviewReminderScope>(ARGS_SCOPE) |
There was a problem hiding this comment.
bundleOf is deprecated, use Bundle()
| * TODO: Split SingleFragmentActivity into two activities | ||
| * One with `android:configChanges="orientation"` for hosting WebView fragments | ||
| * and one without for hosting non-WebView fragments like this one. |
There was a problem hiding this comment.
ConfigAwareSingleFragmentActivity
| (arguments ?: bundleOf()).getParcelableCompat<ReviewReminderScope>(ARGS_SCOPE) | ||
| ?: ReviewReminderScope.Global |
There was a problem hiding this comment.
Comment that it's not dead code.
Is there a reason you didn't set a click handler on the Header?
| ~ This file exists solely to provide a stable @XmlRes resource ID used as a routing key | ||
| ~ in the preference search infrastructure (HeaderFragment + getFragmentFromXmlRes). | ||
| ~ ScheduleRemindersFragment is not a PreferenceFragmentCompat, so there are no | ||
| ~ preferences to index here. |
There was a problem hiding this comment.
I feel the intent: "exists for search" is a little lost in the explanation.
| override fun onCreateOptionsMenu(menu: Menu): Boolean { | ||
| menuInflater.inflate(R.menu.activity_study_options, menu) | ||
| val undoMenuItem = menu.findItem(R.id.action_undo) | ||
| val undoActionProvider = MenuItemCompat.getActionProvider(undoMenuItem) as? RtlCompliantActionProvider | ||
| // Set the proper click target for the undo button's ActionProvider | ||
| undoActionProvider?.clickHandler = { _, menuItem -> onOptionsItemSelected(menuItem) } | ||
| undoMenuItem.isVisible = undoState.hasAction | ||
| undoMenuItem.isVisible = undoState.hasAction && (currentFragment is StudyOptionsFragment) | ||
| undoMenuItem.title = undoState.label | ||
| return true | ||
| } |
There was a problem hiding this comment.
Optional/for a TODO/ignore this
Take a look at CardBrowser + CardBrowserFragment's use of MenuProvider
Would this simplify the logic? Make one provider for each state: fragmented and non-fragmented, and the fragments control their menus
Anki-Android/AnkiDroid/src/main/java/com/ichi2/anki/CardBrowser.kt
Lines 361 to 381 in 25d4bc1
There was a problem hiding this comment.
Working on this in a separate PR since it's sort of separate; will push shortly
| parentFragmentManager.setFragmentResult( | ||
| REQUEST_STUDY_OPTIONS_REVIEW_REMINDERS, | ||
| bundleOf( | ||
| KEY_REVIEW_REMINDERS_DECK_ID to col!!.decks.current().id, | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Optional: consider using this pattern for strongly typed listeners
| * Tapping "Fix" opens the full troubleshooting screen. | ||
| */ | ||
| private suspend fun setupTroubleshootingSnackbar() { | ||
| troubleshootingViewModel.state |
There was a problem hiding this comment.
Optional: launchCollectionInLifecycleScope simplifies this a little
| this@launchCollectionInLifecycleScope.collect { | ||
| if (isRobolectric) { | ||
| HandlerUtils.postOnNewHandler { runBlocking { block(it) } } | ||
| val lifecycle = fragment.lifecycle |
There was a problem hiding this comment.
If this is a test fix, it should be squashed into when the test failure started, otherwise we have commits which may cause git bisect to produce a bad result
There was a problem hiding this comment.
Following up on David's note this DESTROYED-guard is a Robolectric-only test fix but it's bundled into the DeckPicker/StudyOptions feature commit. Can it be squashed into the commit that introduced the test failure so git bisect stays clean?
There was a problem hiding this comment.
Yep, I did a git bisect and found that the DeckPicker/StudyOptions feature commit is the one that caused the test to fail. Before it, the test passes. To be honest, I did this test fix via AI because it was frustrating me and based on its analysis it seems to just be an issue with Robolectric and the test suite, not with the code itself, so I was happy with just a quick test-only fix gated behind isRobolectric. If you guys really want, I can pin down the exact root cause, but I personally think that's not the best use of my time.
f826e56 to
6a753ae
Compare
6a753ae to
3bd2dbe
Compare
david-allison
left a comment
There was a problem hiding this comment.
I'm happy, all nitpicks. Implementer's choice on them, I'd recommend TODOs for anything which isn't trivial to fix and unambiguously positive.
| super.onViewCreated(view, savedInstanceState) | ||
|
|
||
| requireAnkiActivity().setToolbarTitle(title) | ||
| requireAnkiActivity().setToolbarText(title) |
| subtitle: String? = null, | ||
| ) { | ||
| supportActionBar?.title = title | ||
| supportActionBar?.subtitle = subtitle |
There was a problem hiding this comment.
If a subtitle was previously set, calling setToolbarText("foo") would remove it. Is this intentional?
There was a problem hiding this comment.
Yep. I was also considering making the subtitle a mandatory argument to force callers to acknowledge that the subtitle will be cleared, but I didn't think it was worth the extra verbosity.
| super.onViewCreated(view, savedInstanceState) | ||
| requireActivity().addMenuProvider(this, viewLifecycleOwner, Lifecycle.State.RESUMED) | ||
| if (!fragmented) { | ||
| requireAnkiActivity().setToolbarText(R.string.empty_string) |
There was a problem hiding this comment.
I'd be happy with "", your call.
There was a problem hiding this comment.
Yeah, good point, "" is better. Fixed!
| private val host: ScheduleRemindersFragment.FragmentHost by lazy { | ||
| requireNotNull( | ||
| requireArguments().getParcelableCompat<ScheduleRemindersFragment.FragmentHost>(ARGS_HOST), | ||
| ) { | ||
| "Host cannot be null" | ||
| } | ||
| } |
There was a problem hiding this comment.
nit:
would it be worthwhile making this a delegate function, similar to requireParcelable in implementation (or just using requireParcelable)
A custom delegate example:
There was a problem hiding this comment.
I'll handle this in a separate PR as this is not the only place this happens. I think a custom delegate is a bit excessive, but I think using requireParcelable everywhere could be nice.
| val lifecycle = fragment.lifecycle | ||
| HandlerUtils.postOnNewHandler { | ||
| if (lifecycle.currentState != Lifecycle.State.DESTROYED) { | ||
| runBlocking { block(it) } | ||
| } | ||
| } |
There was a problem hiding this comment.
consider an extension to reduce duplication
| /** | ||
| * Bundle key for the deck ID whose study options are being displayed, used when the review reminders screen is being opened. | ||
| */ | ||
| private const val KEY_REVIEW_REMINDERS_DECK_ID = "review_reminders_deck_id" |
There was a problem hiding this comment.
See the following for naming
There was a problem hiding this comment.
Will handle this in a separate PR as I use KEY_ in a lot of other places, and also because I realized I'm non-compliant with ARG_ (the review reminder files use ARGS_).
After the review reminders UI refactoring changes that come after this commit, there is a chance that when the troubleshooting screen is rotated twice in quick succession from within the settings screen that it crashes due to removeOnWindowFocusChangeListener trying to run on a detached viewTreeObserver. This change fixes this by only removing the listener if it is existent and alive.
Previously, there was a horizontal divider between the toolbar and review reminders list / troubleshooting menu, along with horizontal dividers between each individual review reminder listing. I think this should be removed to bring the screen more in line stylistically with how the rest of the settings menu looks (there are no dividers between Preferences, nor between the toolbar and primary content).
The AddEditReminderDialog's TimePicker breaks and fails to save information if rotated in some scenarios after the upcoming commits which rejiggle the review reminders UI are implemented. This is due to latent issues in the TimePicker component. See the TODO comment and docstrings for more details.
A future change will involve setting the subtitle of StudyOptionsActivity from a fragment nested inside it (ScheduleRemindersFragment, ReminderTroubleshootingFragment). Hence, we modify the contract of the pre-existing setToolbarText methods in AnkiActivity, which is inherited by StudyOptionsActivity.
…Activity StudyOptionsActivity was given explicit handling for orientation changes wayyyy back in 2012 to solve a bug: ankidroid@905fd21 But it's no longer needed today, and in fact keeping it causes issues with when StudyOptionsActivity hosts review reminder UI in an upcoming commit. In particular, notice that the onConfigurationChanged method in the linked commit is no longer present in StudyOptionsActivity.
…n host ScheduleReminders can be hosted in multiple places, each of which presents their own unique UI challenges: 1. Long-pressing a deck from DeckPicker: On small screens, there is no suitable activity currently available, and so we should launch one to host ScheduleRemindersFragment in the form of SingleFragmentActivity. On large screens, there MAY be a right-side panel available (it is hidden in the edge case where the collection is empty); the UI looks cleanest if it is used. 2. Clicking on the "bell" icon from StudyOptionsFragment: On small screens, StudyOptionsActivity is currently active, and so it should be used as the host rather than launching a new activity. However, it manages an external toolbar and so using an internal toolbar would not work. We need to latch onto the external support action bar and modify it. On large screens, the right-side panel is available similar to option 1 if the collection is non-empty. 3. The screen may be launched from Settings, in which case the user is inspecting all reminders across all decks. Here, it should match the style of the other Settings Preferences screens, i.e. a collapsing toolbar, and should align whether on a small screen or large screen. In my opinion, the cleanest way to handle this is to enumerate the possible hosts ScheduleReminders can have via FragmentHost, define what the ToolbarType should be for each host, and then modify the toolbar and styling appropriately. This keeps all logic centralized within ScheduleRemindersFragment and maintains a single source of truth. Additionally, due to an issue with `fitsSystemWindows`, we sometimes need to set the upper padding programmatically. See the TODO comment in this commit. Also, as edge-to-edge is gradually implemented, fitsSystemWindows will stop being necessary and some of the UI code here will be not needed. This is the case, for instance, in DeckPicker. Hence we must also record whether a host has edge-to-edge enabled. See the TODO comment. Since ReminderTroubleshootingFragment is launched from ScheduleRemindersFragment, it also has similar UI issues and also needs to be aware of its host. Proper launching of ReminderTroubleshootingFragment is also implemented. A subtitle style is added.
Adds a FragmentManager.showDialogFragment extension function. The normal showDialogFragment function is an extension on FragmentActivity and attaches the dialog to the supportFragmentManager, which is activity-scoped. This: 1. Causes issues when ScheduleRemindersFragment is hosted within Settings, as Preferences.kt launches its children within its childFragmentManager, meaning that AddEditReminderDialog's setFragmentResultListener needs to be changed to `requireActivity().supportFragmentManager.setFragmentResultListener`. 2. However, even though the above is implementable and works (I tried it), it is kind of bad because we've attached the listener to the entire activity. If the ScheduleRemindersFragment dies and thus there is no "correct" place for AddEditReminderDialog to return, the listener is technically still alive on the activity. This feels wrong. A better solution in my opinion is to attach the AddEditReminderDialog result listeners to the childFragmentManager of ScheduleRemindersFragment. While this can be done via a direct call to showDialogFragmentImpl, I thought calling a function with the word "Impl" in it was a bit ugly and so created an extension function.
Deletes old "search" override code in the Settings screen. Adds a new preferences XML file solely for indexing purposes, but directs the HeaderFragment to simply open ScheduleRemindersFragment on click.
… StudyOptionsFragment Adds logic for opening ScheduleRemindersFragment in the correct fragment container hosts in DeckPicker and StudyOptionsFragment. Mirrors `openStudyOptions` and `tryShowStudyOptionsPanel` by adding `openScheduleReminders` and `tryShowScheduleRemindersPanel`. Reloads the StudyOptionsFragment and removes ScheduleRemindersFragment if an appbar menu button is clicked while the fragment is open on large screens to prevent stale state from showing. Assisted-by: Composer 2.5 Fast [only for debugging test failure, Flow.kt]
Yanks the troubleshooting snackbar setup code into a helper function to declutter `onViewCreated` a bit.
Screenshot tests for the two key review reminder UI interfaces. In particular, making sure the ScheduleRemindersFragment looks good no matter where it is opened from. As edits to the UI accumulate over the coming years, we don't want ScheduleRemindersFragment to silently regress in one of the four hosts. Instrumented tests coming soon. Assisted-by: Claude Sonnet 4.6 Medium [Initial test drafts]
b1ab121 to
b027df7
Compare
Note
Assisted-by: Claude Sonnet 4.5, Composer 2.5 (initial rough drafts for tests)
Purpose / Description
This change was originally proposed by @BrayanDSO. Thanks Brayan!
This took a long time to get right and required a lot of iteration. I had to scrap a lot of approaches. I also ran into a lot of issues that I consider out of scope with regards to this PR. I've added TODO comments and might create some issues.
ScheduleReminders can be hosted in multiple places, each of which presents their own unique UI challenges:
phone.mp4
tablet.mp4
Fixes
Approach
Previously:
Now:
How Has This Been Tested?
Permission requesting dialog works as before.
The permission requesting Snackbar auto-dismisses upon navigating away from the fragment.
Searching for "Notifications" or "Review Reminders" in settings locates the setting.
Rotation of all screens works.
Visuals on all screens have no visual glitches.
Creating, updating, viewing, and deleting reminders works.
All four app themes work.
When the collection is empty and the right-hand study options fragment panel is hidden on a tablet, long-clicking to open schedule reminders opens it in SingleFragmentActivity.
Tested on a emulated Google Pixel, API 25.
Tested on a physical Lenovo Tab M11, API 35.
Tested on a physical Samsung S23, API 36.
Learning
Checklist