Managed back stack for parent fragments in the app settings · Pull Request #18057 · ankidroid/Anki-Android

ghost · 2025-03-05T00:44:47Z

Purpose / Description

Issue: The Settings screen closes when searching and the Back button or the Navigation icon is pressed, causing the user to restart their search.
Why it Matters: This issue creates a significant usability barrier, hindering users' ability to effectively use the settings. Addressing it will make the settings experience more efficient and less frustrating.

Fixes

Fixes Settings' search should be cancellable #18051 (comment)

Approach

Before: Child fragments were effectively managed within the handleOnBackPressed() method. However, this method did not manage the back stack for the parent fragment, which is directly hosted by the activity.
After the changes: Now, the method also manages the parent fragment's back stack, ensuring it remains on the screen when the user is searching and presses the back button or navigation icon, instead of navigating directly to the home screen.

How Has This Been Tested?

This issue has been tested on a physical device running on Android 15, as well as on Android Emulators (API 35 and API 34), with all possible navigation states.

This video clip shows how it is working now:
(Please avoid the playback speed, make it normal, due to the length of the video it is edited to 2x)
https://github.com/user-attachments/assets/dd1b8d11-c4b0-4fce-9f57-52ad939e8f82

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

welcome · 2025-03-05T00:44:49Z

First PR! 🚀 We sincerely appreciate that you have taken the time to propose a change to AnkiDroid! Please have patience with us as we are all volunteers - we will get to this as soon as possible.

lukstbit

Hey, thank you for contributing. Pleas format the code otherwise our code style action below is going to fail.

lukstbit · 2025-03-05T05:49:33Z

AnkiDroid/src/main/java/com/ichi2/anki/preferences/Preferences.kt

            .setNavigationOnClickListener { onBackPressedCallback.handleOnBackPressed() }

-        requireActivity().onBackPressedDispatcher.addCallback(viewLifecycleOwner, onBackPressedCallback)
+        requireActivity().onBackPressedDispatcher.addCallback( onBackPressedCallback)


Why is this change needed?

Thank you for your feedback! I have refactored the code and added back viewLifecycleOwner to ensure proper lifecycle management. Please let me if you have any other suggestions that needs to be done.

@iamits-07 I assume you're a gsoc candidate. If so, you're really expected to answer questions if you want to be considered.

Either there was no reason, which is a bad sign as I don't see how we can trust code you wrote (for example if you just followed a LLM advice without understanding them). Or there was a reason and then answering would allow us to evaluate how good a fit you're

Arthur-Milchior

Thanks for the PR.
It works great on phone. Sadly, on tablet, it breaks the "return" button when you have selected multiple settings submenu

Arthur-Milchior · 2025-03-06T03:55:13Z

AnkiDroid/src/main/java/com/ichi2/anki/preferences/Preferences.kt

            .setNavigationOnClickListener { onBackPressedCallback.handleOnBackPressed() }

-        requireActivity().onBackPressedDispatcher.addCallback(viewLifecycleOwner, onBackPressedCallback)
+        requireActivity().onBackPressedDispatcher.addCallback( onBackPressedCallback)


@iamits-07 I assume you're a gsoc candidate. If so, you're really expected to answer questions if you want to be considered.

Either there was no reason, which is a bad sign as I don't see how we can trust code you wrote (for example if you just followed a LLM advice without understanding them). Or there was a reason and then answering would allow us to evaluate how good a fit you're

Arthur-Milchior · 2025-03-06T04:55:17Z

AnkiDroid/src/main/java/com/ichi2/anki/preferences/Preferences.kt

        object : OnBackPressedCallback(true) {
            override fun handleOnBackPressed() {
-                if (resources.isWindowCompact() && childFragmentManager.backStackEntryCount > 0) {
+                if ( childFragmentManager.backStackEntryCount > 0) {


Why did you remove "isWindowCompact?"
Do you have any idea why it was for? Did you test your PR on both compact and non compact window? Because I just tested on tablet, and now the feature is partially broken.

Why did you remove "isWindowCompact?" Do you have any idea why it was for? Did you test your PR on both compact and non compact window? Because I just tested on tablet, and now the feature is partially broken.

I removed isWindowCompact() because I tested it on the mobile emulators and physical mobile devices only and thought it wasn't necessary for it. However now, I understand that it affects the behavior on tablets.
I have updated the code to solve the problem in devices like tablets or foldables screens and ensures the proper functionality in both compact and non-compact windows.
Thanks for your guidance.

Then I'd advise for a next time to ask why some code is there before removing it or editing it.
There are old code that is actually useless, it occurs. But you need to ensure it'sa ctually the case.
In particular, here, the name of the function should have been a hint that this feature is related to the size of the screen, whether it's compact or not

Arthur-Milchior · 2025-03-07T00:20:50Z

The code indeed seems to do what I want, great. Can you also add tests. Both tests that ensure that ensures that this feature works, and also a test that ensure that, on tablet, when there is no oingoing search and that you opened many submenu "back" close the settings instead of going to the previous submenu. This would ensure that the previous version of your PR would have failed the test, and thus that it would have been catched before needing our review

ghost · 2025-03-07T00:27:35Z

The code indeed seems to do what I want, great. Can you also add tests. Both tests that ensure that ensures that this feature works, and also a test that ensure that, on tablet, when there is no oingoing search and that you opened many submenu "back" close the settings instead of going to the previous submenu. This would ensure that the previous version of your PR would have failed the test, and thus that it would have been catched before needing our review

Thank you for your feedback and review. I will add the tests to ensure the feature works correctly and also verify that on tablets, the back button behavior is handled as expected. I'll update the PR once the tests are added.

ShridharGoel · 2025-03-08T12:05:31Z

AnkiDroid/src/androidTest/java/com/ichi2/anki/preferences/PreferencesTestCompact.kt

+    @get:Rule
+    val permissionRule: GrantPermissionRule =
+        GrantPermissionRule.grant(
+            Manifest.permission.WRITE_EXTERNAL_STORAGE,
+            Manifest.permission.READ_EXTERNAL_STORAGE,
+        )


Why is this needed? Also, why is a new test file needed for this test when we already have PreferencesTest.kt?

I used this rule to grant storage permission automatically during test execution (on Physical device it was creating problem ), ensuring the test doesn't fail due to permission issues.
As for the new test file, it specifically focuses on navigation behavior in compact and non compact modes, which differs from the general preferences-related tests in PreferencesTest.kt .
Would you recommend proceeding with the merge, or do you suggest any improvements?

But why would this need to access storage?

On running the module, the app launces the Introduction Activity, triggering a permission dialog for newly installed apps and I thought, it would be a good option to add that rule for safer side and consistent execution. However, if this permission isn't strictly necessary for the test, I can review and update the code.

Makes sense, you can use the below:

@get:Rule val runtimePermissionRule = grantPermissions(GrantStoragePermission.storagePermission)

Also, you can consider merging these two new files with a name like PreferencesNavigationTest.

Thanks for your suggestions! I'll update the PR after updating the tests.

If you look at the error message you got, I suspect it was

java.lang.ExceptionInInitializerError
at com.ichi2.anki.preferences.PreferencesNavigationTest.(PreferencesNavigationTest.kt:31)
... 33 trimmed
Caused by: java.lang.IllegalStateException: 'All Files' access is required on your emulator/device. Please grant it manually or change Build Variant to 'playDebug' in Android Studio (Build -> Select Build Variant)

Which, if you read it, explains how to solve the isuse.

Yeah, it's not ideal that you've to switch to the play variant manually

AnkiDroid/src/androidTest/java/com/ichi2/anki/preferences/PreferencesTestNonCompact.kt

AnkiDroid/src/androidTest/java/com/ichi2/anki/preferences/PreferencesTestCompact.kt

Arthur-Milchior

I'm very happy with the evolution of this PR, great tests!

I'd also appreciate a test that use the search bar in non compact mode please.

And while we're at it, even if it's not related to the issue you're solving, I guess it'd be great to have a test that ensure that, if you click on a search result and then goes back, you end up in the list of settings category.

AnkiDroid/src/androidTest/java/com/ichi2/anki/preferences/PreferencesNavigationTest.kt

Arthur-Milchior · 2025-03-08T19:37:40Z

AnkiDroid/src/androidTest/java/com/ichi2/anki/preferences/PreferencesNavigationTest.kt

+     */
+    @Test
+    fun testOnCompactMode() {
+        fun isCompactMode(context: Context): Boolean = context.resources.configuration.smallestScreenWidthDp < 600


You can directly use "context.resources.isWindowCompact()"

Arthur-Milchior · 2025-03-08T19:56:42Z

AnkiDroid/src/androidTest/java/com/ichi2/anki/preferences/PreferencesNavigationTest.kt

+    @get:Rule
+    val runtimePermissionRule = grantPermissions(GrantStoragePermission.storagePermission)
+
+    /*


Please use /**
The second star indicates that it's documentation. You're stating to the reader what you're testing here.

We use a single star for comment, when we are trying to indicate how the code works.

Arthur-Milchior · 2025-03-08T19:59:02Z

AnkiDroid/src/androidTest/java/com/ichi2/anki/preferences/PreferencesNavigationTest.kt

+        onView(withId(R.id.drawer_layout)).perform(DrawerActions.open())
+        onView(withId(R.id.nav_settings)).perform(click())
+        onView(withId(R.id.search)).perform(click())
+        pressBack()


I'd appreciate a check between both pressBack. Something that ensures that a preference such as "Notifications" is displayed.

And also a comment explaining what exactly you're testing at this place. I.e.
"The list of settings categories is displayed"

Arthur-Milchior · 2025-03-08T20:01:20Z

AnkiDroid/src/androidTest/java/com/ichi2/anki/preferences/PreferencesTestCompact.kt

+    @get:Rule
+    val permissionRule: GrantPermissionRule =
+        GrantPermissionRule.grant(
+            Manifest.permission.WRITE_EXTERNAL_STORAGE,
+            Manifest.permission.READ_EXTERNAL_STORAGE,
+        )


If you look at the error message you got, I suspect it was

java.lang.ExceptionInInitializerError
at com.ichi2.anki.preferences.PreferencesNavigationTest.(PreferencesNavigationTest.kt:31)
... 33 trimmed
Caused by: java.lang.IllegalStateException: 'All Files' access is required on your emulator/device. Please grant it manually or change Build Variant to 'playDebug' in Android Studio (Build -> Select Build Variant)

Which, if you read it, explains how to solve the isuse.

Yeah, it's not ideal that you've to switch to the play variant manually

Arthur-Milchior · 2025-03-08T20:05:22Z

AnkiDroid/src/androidTest/java/com/ichi2/anki/preferences/PreferencesNavigationTest.kt

+        onView(withId(R.id.get_started)).perform(click())
+        onView(withId(R.id.drawer_layout)).perform(DrawerActions.open())
+        onView(withId(R.id.nav_settings)).perform(click())
+        onView(withText("Sync")).perform(click())


Please don't hard code strings. Otherwise, the test will break in the device is not configured in English.

` onView(withText(R.string.pref_cat_controls)).perform(click())` works

ghost · 2025-03-09T03:59:51Z

Thanks for your feedback and suggestions. I'll update the PR after updating the tests.

Arthur-Milchior · 2025-03-09T14:22:44Z

Thanks for the update.

I'd also appreciate a test that use the search bar in non compact mode please.

This test is still missing.

more importantly, it seems your test is flaky.

com.ichi2.anki.preferences.PreferencesNavigationTest > testOnCompactMode[emulator-5554 - 11] FAILED
androidx.test.espresso.NoMatchingViewException: No views in hierarchy found matching: view.getId() is <2131362429/com.ichi2.anki:id/get_started>
If the target view is not part of the view hierarchy, you may need to use Espresso.onData to load it from one of the following

I actually had the same problem running locally the first time and then could not reproduce.

I've no idea what's going on. I'll ask advice on discord. if we can't land it non flakily, then I suggest we land the non test code first and let the code for a second PR. After all, your code clearly works, and I don't want to block because of the test suite

Arthur-Milchior · 2025-03-09T14:09:27Z

AnkiDroid/src/androidTest/java/com/ichi2/anki/preferences/PreferencesNavigationTest.kt

-    /*
+    /**
     * This test verifies the navigation when search bar is clicked.
     * When user searches for something in the search bar,It should close the search view


I've a hard time understanding how your sentence breaks. Why is there a upper case juste after a comma?
Also, as it's mainly an app for phone and tablet, we generally use "tap" instead of "click". Even if you're right that it could be clicked if the user use a mouse

Arthur-Milchior · 2025-03-09T14:14:32Z

AnkiDroid/src/androidTest/java/com/ichi2/anki/preferences/PreferencesNavigationTest.kt

    @Test
    fun testOnCompactMode() {
-        fun isCompactMode(context: Context): Boolean = context.resources.configuration.smallestScreenWidthDp < 600
+        fun isCompactMode(context: Context): Boolean = context.resources.isWindowCompact()


sorry I should have been more specific. I don't really see the point of the function. I thought you could juste us isWindowCompact in the assumeTrue

Arthur-Milchior · 2025-03-09T14:32:22Z

Got it. You should have used closeGetStartedScreenIfExists to close the started screen. Because it won't open every time if you run many tests instead of a single one

ghost · 2025-03-09T14:38:20Z

Thanks for suggestions, as you said about the problem during the first run of the module I will try to fix it and update the others.

david-allison

Looks great, cheers!

AnkiDroid/src/androidTest/java/com/ichi2/anki/preferences/PreferencesNavigationTest.kt

mikehardy

CI was stuck on this one, and one of the commits conflicted with main but in a way that was altered in further commits on this PR

I squashed all commits to a single one, then rebase to current main was possible

So, LGTM now, and I'm queueing for merge, thanks!

code is formatted now (or at least I think it is - CI is chewing on it)

- added compact and non-compact tests for Preference navigation.

github-actions bot added the New contributor label Mar 5, 2025

lukstbit previously requested changes Mar 5, 2025

View reviewed changes

lukstbit added the Needs Author Reply Waiting for a reply from the original author label Mar 5, 2025

ghost requested a review from lukstbit March 5, 2025 07:02

Arthur-Milchior removed the Needs Author Reply Waiting for a reply from the original author label Mar 6, 2025

Arthur-Milchior reviewed Mar 6, 2025

View reviewed changes

Arthur-Milchior added the Needs Author Reply Waiting for a reply from the original author label Mar 6, 2025

ghost requested a review from Arthur-Milchior March 6, 2025 11:37

Arthur-Milchior approved these changes Mar 7, 2025

View reviewed changes

Arthur-Milchior added the Needs Second Approval Has one approval, one more approval to merge label Mar 7, 2025

ghost requested a review from Arthur-Milchior March 8, 2025 08:40

ShridharGoel reviewed Mar 8, 2025

View reviewed changes

AnkiDroid/src/androidTest/java/com/ichi2/anki/preferences/PreferencesTestNonCompact.kt Outdated Show resolved Hide resolved

ShridharGoel reviewed Mar 8, 2025

View reviewed changes

AnkiDroid/src/androidTest/java/com/ichi2/anki/preferences/PreferencesTestCompact.kt Outdated Show resolved Hide resolved

Arthur-Milchior reviewed Mar 8, 2025

View reviewed changes

ghost requested review from Arthur-Milchior and ShridharGoel March 9, 2025 08:25

Arthur-Milchior reviewed Mar 9, 2025

View reviewed changes

ghost requested a review from Arthur-Milchior March 9, 2025 20:02

This comment was marked as resolved.

Sign in to view

github-actions bot added the Stale label Mar 23, 2025

BrayanDSO removed Needs Author Reply Waiting for a reply from the original author Stale labels Mar 26, 2025

david-allison approved these changes Mar 30, 2025

View reviewed changes

AnkiDroid/src/androidTest/java/com/ichi2/anki/preferences/PreferencesNavigationTest.kt Show resolved Hide resolved

mikehardy approved these changes Apr 5, 2025

View reviewed changes

mikehardy added Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) and removed Needs Second Approval Has one approval, one more approval to merge labels Apr 5, 2025

mikehardy enabled auto-merge April 5, 2025 16:59

fix: managed backstack for parent fragments in app settings

b213eab

- added compact and non-compact tests for Preference navigation.

mikehardy added this pull request to the merge queue Apr 5, 2025

Merged via the queue into ankidroid:main with commit 860e270 Apr 5, 2025
9 checks passed

github-actions bot removed the Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) label Apr 5, 2025

github-actions bot added this to the 2.21 release milestone Apr 5, 2025

Uh oh!

Comments

Conversation

ghost commented Mar 5, 2025 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose / Description

Fixes

Approach

How Has This Been Tested?

Checklist

Uh oh!

welcome bot commented Mar 5, 2025

Uh oh!

lukstbit left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Arthur-Milchior left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Arthur-Milchior commented Mar 7, 2025

Uh oh!

ghost commented Mar 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Mar 8, 2025 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Arthur-Milchior left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Mar 9, 2025

Uh oh!

Arthur-Milchior commented Mar 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Arthur-Milchior commented Mar 9, 2025

ghost commented Mar 5, 2025 •

edited by ghost

Loading

ghost Mar 8, 2025 •

edited by ghost

Loading