refactor: use MenuProvider in StudyOptionsActivity, remove starting reviewer in undo

[ericli3690]

Warning

• Blocked by, and stacked on top of feat(reminders): streamline review reminders UI #21140

Note

Assisted-by: Claude Sonnet 4.5

Purpose / Description

• Split off from feat(reminders): streamline review reminders UI #21140
• StudyOptionsActivity is currently using onOptionsItemSelected etc. This PR migrates it to the modern MenuProvider interface.
• Also, addresses a TODO comment left a while ago by lukstbit: I think it's safe to remove the call to open the reviewer:

                     // TODO why are we going to the Reviewer from here? Desktop doesn't do this
                    Reviewer
                        .getIntent(this@StudyOptionsActivity)
                        .apply { flags = Intent.FLAG_ACTIVITY_FORWARD_RESULT }
                        .also { startActivity(it) }
                    finish()

Fixes

Originally created as a response to one of David's comments:

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

	// Update the menu for a fragmented state
	// Added last so all changes take priority
	onAllFragmentsLoaded {
	addPrepareMenuProvider { menu ->
	if (!fragmented) return@addPrepareMenuProvider
	/** Return the menu item with a particular identifier or `null` */
	operator fun Menu.get(id: Int): MenuItem? = this.findItem(id)
	// NoteEditorFragment: Remove save/preview note options if there are no notes
	if (viewModel.rowCount == 0) {
	menu[R.id.action_save]?.isVisible = false
	menu[R.id.action_preview]?.isVisible = false
	}
	menu[R.id.action_edit_note]?.isVisible = false
	// TODO: https://github.com/ankidroid/Anki-Android/issues/20206
	// This blocks a user from previewing all cards
	menu[R.id.action_preview_many]?.isVisible = false
	}

See below for discussion of David's concern above.

Approach

• Simple refactor and removal of the reviewer-opening line

How Has This Been Tested?

• No regressions on tablets, wide screens, including DeckPicker back button and undo button
• Back button works on StudyOptionsActivity
• Undo button works on StudyOptionsActivity
• Undo does not start reviewer anymore; if, for instance, the option to be undone is a deck selection, pressing undo navigates to the deck that was previously selected rather than opening the reviewer
• Physical Samsung S23, API 36

Checklist

• 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

[github-actions]

Snapshot diff report vs main. Open screenshot-diff for diffs.

• AddEditReminderDialogScreenshotTest: 4 changes
• PreferencesScreenshotTest: 1 change
• ReviewRemindersScreenshotTest: 11 changes

All 16 changed screenshots

AddEditReminderDialogScreenshotTest

• add_mode_advanced_open_compare.png
• add_mode_compare.png
• edit_mode_advanced_open_compare.png
• edit_mode_compare.png

PreferencesScreenshotTest

• ScheduleRemindersFragment_compare.png

ReviewRemindersScreenshotTest

• settingsHostTablet_scheduleReminders_compare.png
• settingsHostTablet_troubleshooting_compare.png
• settingsHost_scheduleReminders_compare.png
• settingsHost_scheduleReminders_scrolled_compare.png
• settingsHost_troubleshooting_compare.png
• standaloneActivityHost_scheduleReminders_compare.png
• standaloneActivityHost_troubleshooting_compare.png
• studyOptionsFragmentHost_scheduleReminders_compare.png
• studyOptionsFragmentHost_troubleshooting_compare.png
• studyOptionsFrameHost_scheduleReminders_compare.png
• studyOptionsFrameHost_troubleshooting_compare.png

[ericli3690]

@david-allison This is a follow up addressing your comment here: #21140 (comment)

Or at least, that's what this PR was initially supposed to be. I don't think I read your comment carefully enough at first haha and I ended up just doing a little refactor cleanup before realizing that you wanted to separate out the undo button into... the fragments? Rather than the activity? Perhaps I'm misunderstanding you.

Your original comment annotated StudyOptionsActivity and mentions showing the undo button conditionally by checking whether the activity is fragmented rather than by doing && (currentFragment is StudyOptionsFragment). The problem is that StudyOptionsActivity doesn't have a fragmented mode. It only shows up on narrow screens and takes up the full screen. When StudyOptionsFragment is in a side panel, the host activity is DeckPicker. Thus it makes no sense for StudyOptionsActivity to check if it is "fragmented": it's always not fragmented.

Perhaps what you mean instead is that we should have StudyOptionsFragment own the undo button rather than having StudyOptionsActivity own it? But that then causes problems when StudyOptionsFragment is in a right-side panel on wide screens, as its undo button ID will collide with the DeckPicker activity's undo button ID, even if we hide one of them. What we must do is use a completely different ID rather than action_undo for the StudyOptionsFragment undo button and programmatically hide it when StudyOptionsFragment is fragmented and on the right side on a wide screen. Feels a bit clunky to me, but I can do it if you'd like. I would not be able to use onAllFragmentsLoaded as you originally suggested if we implement it this way, because that requires a FragmentActivity receiver.

I think the original reason why the undo button is located in StudyOptionsActivity is because it's mutually exclusive with DeckPicker's undo button (they're different activities) so we don't need any complex fragmentation checks if we just have one undo button in DeckPicker's menu and one undo button in StudyOptionsActivity's menu. If we move it to StudyOptionsFragment, then all of a sudden we need fragmentation checks.

Come to think of it, why does StudyOptionsActivity need an undo button anyways? The only use I see is for undoing custom study choices like increasing the today's new cards... actually, I guess that's useful, nevermind.

Perhaps the best decision is just what the UI streamlining PR already has? I.e.:

undoMenuItem.isVisible = undoState.hasAction && (currentFragment is StudyOptionsFragment)

The above comment is a bit stream-of-consciousness, sorry about that, but would love to hear your thoughts. Even if we do nothing to address the && (currentFragment is StudyOptionsFragment) code smell, this PR is (I believe) a nice little refactor and eliminates some weird behaviour upon pressing undo.

[ericli3690]

This comment was originally added by @lukstbit at dd4c35d. The code to open the reviewer was initially added in 3e7ad6a9e6. I see no reason to keep it. The behaviour of the undo button seems simpler if we just... only perform the undo. The desktop reviewer doesn't open the reviewer when undo is pressed from the study options view, so lukstbit's point is valid.