Fix fragmented DeckPicker menu ownership and sw600dp split detection by sanjaysargam · Pull Request #21620 · ankidroid/Anki-Android

sanjaysargam · 2026-08-27T17:32:51Z

Note

Assisted-by: GPT-5.4 Mini

Purpose / Description

This moves split pane study options menu handling from StudyOptionsFragment into DeckPicker, aligns the fragmented-layout check with the real sw600dp breakpoint, and removes screenSize from DeckPicker config handling so foldable and resizable window changes recreate the correct UI state

Fixes

Fixes Deck option appears on a non-split Deck Picker #21555

Approach

Centralize fragmented menu delegation in DeckPicker

How Has This Been Tested?

Pixel 10 Pro Foldable

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 · 2026-08-27T17:42:29Z

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

PreferencesScreenshotTest: 1 change
ReviewRemindersScreenshotTest: 16 changes

All 17 changed screenshots

PreferencesScreenshotTest

ScheduleRemindersFragment_compare.png

ReviewRemindersScreenshotTest

settingsHostTablet_scheduleReminders_compare.png
settingsHostTablet_troubleshooting_compare.png
settingsHost_landscapeCutout_collapsed_compare.png
settingsHost_landscapeCutout_compare.png
settingsHost_landscapeCutout_rtl_compare.png
settingsHost_scheduleReminders_compare.png
settingsHost_scheduleReminders_scrolled_compare.png
settingsHost_troubleshooting_compare.png
standaloneActivityHost_scheduleReminders_compare.png
standaloneActivityHost_systemBars_compare.png
standaloneActivityHost_systemBars_scrolledToEnd_compare.png
standaloneActivityHost_troubleshooting_compare.png
studyOptionsFragmentHost_scheduleReminders_compare.png
studyOptionsFragmentHost_troubleshooting_compare.png
studyOptionsFrameHost_scheduleReminders_compare.png
studyOptionsFrameHost_troubleshooting_compare.png

david-allison · 2026-08-27T18:47:36Z

@ericli3690 Could you opine on the menu changes.

And probably add an issue for a unit test to ossify the decision on the displayed menu items.

david-allison

Sorry... I could have sworn I asked, could you split out the two functional changes here into separate commits

ericli3690

Oops, this somehow slipped my notification radar somehow, apologies for the late reply.

ericli3690 · 2026-09-02T22:04:28Z

+    private inline fun <T> withStudyOptionsFragment(block: StudyOptionsFragment.() -> T): T? =
+        if (fragmented) {
+            fragment?.block()
+        } else {
+            null
+        }


This feels... kinda hacky. Are we sure this is the intent of the TODO comment you've deleted? I was under the impression that the TODO is instructing a future developer to move the logic from the Fragment into the Activity, not just to hotwire calls to the Fragment code in.

Checking the history of the TODO comment, it's a suggestion from @david-allison (#16425), perhaps he could clarify what he initially wanted.

I know a lot more about menus now. I'll change my opinion

Fragments should define their own menus (MenuProvider)

The Menu host (typically the activity) either coordinates, or provides the state to the fragments for them to hide their items

The menu host implicitly defines ordering, consider orderInCategory if you want finer grained control

Edit: I'll stand by my original point that one fragment should not own the menus of its siblings

david-allison · 2026-09-06T07:01:45Z

Tests are failing

david-allison · 2026-09-06T22:17:50Z

    override var fragmented: Boolean
        get() =
-            resources.configuration.screenLayout and Configuration.SCREENLAYOUT_SIZE_MASK ==
-                Configuration.SCREENLAYOUT_SIZE_XLARGE
+            resources.configuration.smallestScreenWidthDp >= 600
        set(_) = throw UnsupportedOperationException()


I feel this continues to cause problems. Let's use

override var fragmented: Boolean get() = binding.studyoptionsFragment != null set(_) = throw UnsupportedOperationException()

We've discussed (maybe in Discord) that users are unhappy in 2.24 regarding both the Deck Picker and the Note Editor split screen changes, especially the modifications in the 'fragmented' threshold, and what acceptable values are for foldables.

I don't think we've done sufficient research/testing [proposed work in the linked issue] to come to a consensus on what this value should be.

[Tablet] Reconsider design of Note Editor Previewer #21010

Agreed. This also means you'd be able to align line 517:

if (binding.studyoptionsFragment == null) {

To instead be if (!fragmented) {

Then update unit tests to match

ericli3690 · 2026-09-06T23:27:08Z

    override var fragmented: Boolean
        get() =
-            resources.configuration.screenLayout and Configuration.SCREENLAYOUT_SIZE_MASK ==
-                Configuration.SCREENLAYOUT_SIZE_XLARGE
+            resources.configuration.smallestScreenWidthDp >= 600
        set(_) = throw UnsupportedOperationException()


Agreed. This also means you'd be able to align line 517:

if (binding.studyoptionsFragment == null) {

To instead be if (!fragmented) {

Then update unit tests to match

ericli3690 · 2026-09-07T00:02:20Z

            android:theme="@style/Theme_Dark.Launcher"
            android:exported="false"
-            android:configChanges="keyboardHidden|orientation|screenSize"
+            android:configChanges="keyboardHidden|orientation"


Hm, my LLM claims that it doesn't make sense to do only orientation; double check, but apparently if you remove screenSize you should be safe to remove orientation too

david-allison · 2026-09-13T13:55:17Z

            android:theme="@style/Theme_Dark.Launcher"
            android:exported="false"
-            android:configChanges="keyboardHidden|orientation|screenSize"
+            android:configChanges="keyboardHidden"


Drop this, it's out of scope, (would need its own PR to discuss)

david-allison · 2026-09-13T13:56:53Z

    @Test
    fun checkDisplayOfStudyOptionsOnTablet() {
-        assumeTrue("We are running on a tablet", qualifiers!!.contains("xlarge"))
+        assumeTrue("We are running on a tablet", resources.configuration.smallestScreenWidthDp >= 600)


I still feel this is out of scope for the PR, we shouldn't be mixing redefining the layout widths in with this PR, and the prerequistite research hasn't been performed to determine the qualifier we want to use for a split screen.

`fragmented` checked `SCREENLAYOUT_SIZE_XLARGE`, which does not match the `layout-sw600dp` resource boundary that the split-pane layout actually uses, so the flag could disagree with the inflated layout. Derive it from the layout itself: fragmented iff the layout provides the side panel container, so the flag and the UI can never disagree regardless of device geometry

fragments define their own menus via MenuProvider, and the menu host dispatches creation, preparation and selection to them; one fragment must not own or drive the menus of its siblings.

github-actions Bot added the Needs Review label Aug 27, 2026

david-allison added the Needs reviewer reply Waiting for a reply from another reviewer label Aug 27, 2026

david-allison reviewed Sep 2, 2026

View reviewed changes

ericli3690 reviewed Sep 2, 2026

View reviewed changes

ericli3690 added Needs Author Reply Waiting for a reply from the original author and removed Needs reviewer reply Waiting for a reply from another reviewer labels Sep 2, 2026

sanjaysargam force-pushed the todo/menu-options branch from 509fa58 to c2cb3a7 Compare September 6, 2026 06:17

sanjaysargam force-pushed the todo/menu-options branch from c2cb3a7 to 21a32af Compare September 6, 2026 12:56

sanjaysargam requested review from david-allison and ericli3690 September 6, 2026 13:42

david-allison removed the Needs Author Reply Waiting for a reply from the original author label Sep 6, 2026

david-allison reviewed Sep 6, 2026

View reviewed changes

david-allison requested changes Sep 6, 2026

View reviewed changes

david-allison added the Needs Author Reply Waiting for a reply from the original author label Sep 6, 2026

ericli3690 reviewed Sep 7, 2026

View reviewed changes

sanjaysargam force-pushed the todo/menu-options branch from 1856a68 to 73e55f6 Compare September 13, 2026 12:57

david-allison reviewed Sep 13, 2026

View reviewed changes

david-allison requested changes Sep 13, 2026

View reviewed changes

sanjaysargam added 2 commits September 15, 2026 12:48

refactor(menu): fragment-owned study options menu

b695711

fragments define their own menus via MenuProvider, and the menu host dispatches creation, preparation and selection to them; one fragment must not own or drive the menus of its siblings.

sanjaysargam force-pushed the todo/menu-options branch from 73e55f6 to b695711 Compare September 15, 2026 07:18

sanjaysargam removed the Needs Author Reply Waiting for a reply from the original author label Sep 15, 2026

Uh oh!

Uh oh!

Conversation

sanjaysargam commented Aug 27, 2026

Purpose / Description

Fixes

Approach

How Has This Been Tested?

Checklist

Uh oh!

github-actions Bot commented Aug 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

david-allison commented Aug 27, 2026

Uh oh!

david-allison left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericli3690 left a comment

Choose a reason for hiding this comment

Uh oh!

ericli3690 Sep 2, 2026

Choose a reason for hiding this comment

Uh oh!

david-allison Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-allison commented Sep 6, 2026

Uh oh!

david-allison Sep 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericli3690 Sep 6, 2026

Choose a reason for hiding this comment

Uh oh!

ericli3690 Sep 6, 2026

Choose a reason for hiding this comment

Uh oh!

ericli3690 Sep 7, 2026

Choose a reason for hiding this comment

Uh oh!

sanjaysargam Sep 13, 2026

Choose a reason for hiding this comment

Uh oh!

david-allison Sep 13, 2026

Choose a reason for hiding this comment

Uh oh!

david-allison Sep 13, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions Bot commented Aug 27, 2026 •

edited

Loading

david-allison left a comment •

edited

Loading

david-allison Sep 2, 2026 •

edited

Loading

david-allison Sep 6, 2026 •

edited

Loading