fix: Settings highlight does not follow 'back button' #17447

jainv4156 · 2024-11-15T12:06:38Z

Purpose / Description

Approach

When the back button is pressed, the current fragment is popped from the back stack, and the previous fragment is displayed.
The onResume method of the newly visible fragment is triggered.
In the onResume method, the current fragment passes its unique key to the associated Preferences activity.
The Preferences activity locates the HeaderFragment using the fragment manager and passes the received key to it.
The HeaderFragment uses the key to identify the appropriate header and highlights it.

Description of Changes

HeaderFragment.kt

Added a New Function: handleHighlightHeaderPreferenceOnBack
- Purpose: Highlights a specific header preference when the back button is pressed.

Preferences.kt

Introduced a New Function: handleHighlightPreferenceOnBack
- Details:
- Fetches the HeaderFragment instance.
- Calls the handleHighlightHeaderPreferenceOnBack method on the HeaderFragment.

SettingsFragment.kt

Overridden the onResume Method:
- Purpose: Ensures the highlighting logic is triggered when the fragment resumes.
- Details:
  - Calls the handleHighlightPreferenceOnBack method from Preferences to handle the highlighting logic.

Fixes

Fixes [BUG]: Tablet: Settings highlight does not follow 'back button' #17414

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

Screen_recording_20241115_173821.webm

Aditya13s · 2024-11-17T16:37:39Z

@jainv4156 , can you complete all the details in the PR template? Thanks!

jainv4156 · 2024-11-18T11:54:38Z

@jainv4156 , can you complete all the details in the PR template? Thanks!

just to be sure ,do you mean testing with accessibility scanner.

Aditya13s · 2024-11-18T12:02:10Z

just to be sure ,do you mean testing with accessibility scanner.

It's about the description and the approach you used, present in the template

mikehardy

seems reasonable - tiny nitpicks just a typo

mikehardy · 2024-11-26T12:43:52Z

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

@@ -43,7 +42,7 @@ class HeaderFragment : PreferenceFragmentCompat() {
        highlightHeaderPreference(requirePreference<HeaderPreference>(selectedHeaderPreferenceKey))

        requirePreference<HeaderPreference>(R.string.pref_backup_limits_screen_key)
-            .title = CollectionManager.TR.preferencesBackups()
+            .title = TR.preferencesBackups()


this is a reasonable change but is a refactor (along with the removal of collection manager - and I was curious why it was in the same commit - we usually split refactor stuff into separate commits so that review stays focused on the functional changes. No big deal this time though - just something to keep in the mind when developing in the future - think of the person that will review your code and make sure you keep their attention focused on the important bits by not including unimportant bits at the same time

mikehardy · 2024-11-26T12:44:11Z

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

@@ -105,6 +104,10 @@ class HeaderFragment : PreferenceFragmentCompat() {
        requirePreference<Preference>(R.string.pref_dev_options_screen_key).isVisible = isVisible
    }

+    fun handelHighlightHeaderPreferenceOnBack(key: String) {


typo

Suggested change

fun handelHighlightHeaderPreferenceOnBack(key: String) {

fun handleHighlightHeaderPreferenceOnBack(key: String) {

mikehardy · 2024-11-26T12:44:34Z

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

+        val fragmentManager = supportFragmentManager
+        if (key != null) {
+            val headerFragment = (fragmentManager.findFragmentById(R.id.lateral_nav_container) as? HeaderFragment)
+            headerFragment?.handelHighlightHeaderPreferenceOnBack(key)


typo

Suggested change

headerFragment?.handelHighlightHeaderPreferenceOnBack(key)

headerFragment?.handleHighlightHeaderPreferenceOnBack(key)

david-allison · 2024-11-26T15:29:08Z

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

@@ -100,6 +100,15 @@ abstract class SettingsFragment :
        dialogFragment.show(parentFragmentManager, "androidx.preference.PreferenceFragment.DIALOG")
    }

+    override fun onResume() {
+        super.onResume()
+        preferenceScreen?.key?.let {


(requireActivity() as Preferences)

This is going to conflict with

refactor: move most of the Preferences activity into a fragment #17487

Please be sure that this method would work once the fragment is extracted from the activity

david-allison · 2024-11-26T15:29:33Z

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

@@ -100,6 +100,15 @@ abstract class SettingsFragment :
        dialogFragment.show(parentFragmentManager, "androidx.preference.PreferenceFragment.DIALOG")
    }

+    override fun onResume() {
+        super.onResume()
+        preferenceScreen?.key?.let {


This isn't necessary. handleHighlightPreferenceOnBack handles a nullable string

(requireActivity() as Preferences).handleHighlightPreferenceOnBack(preferenceScreen.key) without the let { will work

david-allison · 2024-11-26T15:35:43Z

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

+    fun handleHighlightPreferenceOnBack(key: String?) {
+        val fragmentManager = supportFragmentManager
+        if (key != null) {
+            val headerFragment = (fragmentManager.findFragmentById(R.id.lateral_nav_container) as? HeaderFragment)
+            headerFragment?.handelHighlightHeaderPreferenceOnBack(key)
+        }
+    }


Extract finding the header fragment into a property so the intent of the method is clear:

fun handleHighlightPreferenceOnBack(key: String?) { if (key == null) return lateralNavigationFragment?.handleHighlightHeaderPreferenceOnBack(key) }

jainv4156 · 2024-11-27T10:42:51Z

Thanks, @david-allison , for mentioning this PR #17487 to me. I checked my implementation by bringing these PR changes locally. As this PR introduces a lot of changes to my implementation, would it be okay if I continue working on my PR after this one is merged?

BrayanDSO · 2024-11-27T12:24:02Z

That PR also fixes the issue, and closes this PR. So it won't be necessary to continue working on this PR after that. Sorry for the time spent here.

jainv4156 · 2024-11-27T17:18:27Z

That PR also fixes the issue, and closes this PR. So it won't be necessary to continue working on this PR after that. Sorry for the time spent here.

sure

jainv4156 closed this Nov 15, 2024

jainv4156 reopened this Nov 15, 2024

lukstbit added the Needs Author Reply Waiting for a reply from the original author label Nov 15, 2024

jainv4156 force-pushed the bug-fixes branch from c2baf0c to 34210f9 Compare November 15, 2024 13:36

fixes Settings highlight does not follow 'back button'

dd9f182

jainv4156 force-pushed the bug-fixes branch from 34210f9 to dd9f182 Compare November 17, 2024 09:21

lukstbit added Needs Review and removed Needs Author Reply Waiting for a reply from the original author labels Nov 19, 2024

mikehardy requested changes Nov 26, 2024

View reviewed changes

mikehardy added the Needs Author Reply Waiting for a reply from the original author label Nov 26, 2024

david-allison requested changes Nov 26, 2024

View reviewed changes

david-allison mentioned this pull request Nov 26, 2024

refactor: move most of the Preferences activity into a fragment #17487

Open

5 tasks

david-allison changed the title ~~fixes Settings highlight does not follow 'back button'~~ fix: Settings highlight does not follow 'back button' Nov 26, 2024

mikehardy added this to the 2.19.3 release milestone Nov 26, 2024

jainv4156 closed this Nov 27, 2024

jainv4156 reopened this Nov 27, 2024

jainv4156 closed this Nov 27, 2024

jainv4156 deleted the bug-fixes branch November 27, 2024 17:19

mikehardy removed this from the 2.19.3 release milestone Nov 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Settings highlight does not follow 'back button' #17447

fix: Settings highlight does not follow 'back button' #17447

jainv4156 commented Nov 15, 2024 •

edited

Loading

Aditya13s commented Nov 17, 2024

jainv4156 commented Nov 18, 2024 •

edited

Loading

Aditya13s commented Nov 18, 2024

mikehardy left a comment

mikehardy Nov 26, 2024

mikehardy Nov 26, 2024

mikehardy Nov 26, 2024

david-allison Nov 26, 2024

david-allison Nov 26, 2024

david-allison Nov 26, 2024

jainv4156 commented Nov 27, 2024 •

edited

Loading

BrayanDSO commented Nov 27, 2024

jainv4156 commented Nov 27, 2024

	fun handelHighlightHeaderPreferenceOnBack(key: String) {
	fun handleHighlightHeaderPreferenceOnBack(key: String) {

	headerFragment?.handelHighlightHeaderPreferenceOnBack(key)
	headerFragment?.handleHighlightHeaderPreferenceOnBack(key)

fix: Settings highlight does not follow 'back button' #17447

fix: Settings highlight does not follow 'back button' #17447

Conversation

jainv4156 commented Nov 15, 2024 • edited Loading

Purpose / Description

Approach

Description of Changes

HeaderFragment.kt

Preferences.kt

SettingsFragment.kt

Fixes

Checklist

Aditya13s commented Nov 17, 2024

jainv4156 commented Nov 18, 2024 • edited Loading

Aditya13s commented Nov 18, 2024

mikehardy left a comment

Choose a reason for hiding this comment

mikehardy Nov 26, 2024

Choose a reason for hiding this comment

mikehardy Nov 26, 2024

Choose a reason for hiding this comment

mikehardy Nov 26, 2024

Choose a reason for hiding this comment

david-allison Nov 26, 2024

Choose a reason for hiding this comment

david-allison Nov 26, 2024

Choose a reason for hiding this comment

david-allison Nov 26, 2024

Choose a reason for hiding this comment

jainv4156 commented Nov 27, 2024 • edited Loading

BrayanDSO commented Nov 27, 2024

jainv4156 commented Nov 27, 2024

jainv4156 commented Nov 15, 2024 •

edited

Loading

jainv4156 commented Nov 18, 2024 •

edited

Loading

jainv4156 commented Nov 27, 2024 •

edited

Loading