-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
feat: Make preferences in ControlSettings tabs searchable #19422
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
A test fails |
david-allison
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
- 1 issue of accessibility/ease of use
- 1 issue of maintainability
Let me know if you'd like more detail on either of the comments - I realise I didn't provide an explicit path to resolution
| delay(100) | ||
| fragment.selectTabForPreference(result.key) | ||
| delay(150) | ||
| (fragment as PreferenceFragmentCompat).let { result.highlight(it) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should handle animations being disabled
I don't know what the code for this would be, but this should handle users speeding up animations in the developer options of a phone (I strongly dislike long animations)
|
|
||
| /** | ||
| * Determines if a preference key belongs to the previewer tab. | ||
| * Previewer preferences typically have "previewer_" prefix in their keys. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way we can ensure that this invariant holds in future?
Ideally a test/enum: this is brittle and people will break it
Can you elaborate a bit more |
david-allison
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One 'safe display mode' issue
| result.highlight(fragment as PreferenceFragmentCompat) | ||
| if (isTabSpecificPreference(result.resourceFile)) { | ||
| (fragment as? ControlsSettingsFragment)?.lifecycleScope?.launch { | ||
| val durationScale = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extract to a helper property/method
| 1f // Default to normal speed if unable to read setting | ||
| } | ||
|
|
||
| if (durationScale > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't take 'safe display mode' into account
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm planning to use the same animation check pattern which is used in DeckPickerFloatingActionMenu.kt
private fun areSystemAnimationsEnabled(): Boolean {
val animDuration: Float =
Settings.Global.getFloat(
context.contentResolver,
Settings.Global.ANIMATOR_DURATION_SCALE,
1f,
)
val animTransition: Float =
Settings.Global.getFloat(
context.contentResolver,
Settings.Global.TRANSITION_ANIMATION_SCALE,
1f,
)
val animWindow: Float =
Settings.Global.getFloat(
context.contentResolver,
Settings.Global.WINDOW_ANIMATION_SCALE,
1f,
)
return animDuration != 0f && animTransition != 0f && animWindow != 0f
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about AnkiActivity::animationDisabled which is an in-app setting
AnkiDroid/src/main/java/com/ichi2/anki/preferences/ControlsSettingsFragment.kt
Outdated
Show resolved
Hide resolved
BrayanDSO
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A more maintainable approach:
Subject: [PATCH] test(preferences): controls keys
---
Index: AnkiDroid/src/main/java/com/ichi2/anki/preferences/ControlsSettingsFragment.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/preferences/ControlsSettingsFragment.kt b/AnkiDroid/src/main/java/com/ichi2/anki/preferences/ControlsSettingsFragment.kt
--- a/AnkiDroid/src/main/java/com/ichi2/anki/preferences/ControlsSettingsFragment.kt (revision a268ec898a0706d1802ea5d54576dc52a091504f)
+++ b/AnkiDroid/src/main/java/com/ichi2/anki/preferences/ControlsSettingsFragment.kt (date 1763127894446)
@@ -66,12 +66,11 @@
* This allows search navigation to automatically switch to the correct tab.
*/
fun selectTabForPreference(key: String) {
- val targetTabIndex =
- if (previewerPreferenceKeys.contains(key)) {
- ControlPreferenceScreen.PREVIEWER.ordinal
- } else {
- ControlPreferenceScreen.REVIEWER.ordinal
- }
+ val targetTabIndex = actionToScreenMap[key]?.ordinal
+ if (targetTabIndex == null) {
+ Timber.w("Could not find the preference with the '%s' key", key)
+ return
+ }
view?.post {
val tabLayout =
@@ -182,7 +181,10 @@
}
companion object {
- private val previewerPreferenceKeys = PreviewerAction.entries.map { it.preferenceKey }.toSet()
+ val actionToScreenMap: Map<String, ControlPreferenceScreen> =
+ ControlPreferenceScreen.entries.flatMap { screen ->
+ screen.getActions().map { action -> action.preferenceKey to screen }
+ }.toMap()
val legacyStudyScreenSettings =
listOf(
BrayanDSO
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be easier to review if you put the Safe display mode changes in a separate commit.
AnkiDroid/src/main/java/com/ichi2/anki/preferences/ControlsSettingsFragment.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/preferences/ControlsTabPreference.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/preferences/Preferences.kt
Outdated
Show resolved
Hide resolved
d80448d to
49eb1da
Compare
BrayanDSO
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Index: AnkiDroid/src/main/java/com/ichi2/anki/preferences/Preferences.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/preferences/Preferences.kt b/AnkiDroid/src/main/java/com/ichi2/anki/preferences/Preferences.kt
--- a/AnkiDroid/src/main/java/com/ichi2/anki/preferences/Preferences.kt (revision 49eb1da070b446c9e4617dfab8a342fef8c81e39)
+++ b/AnkiDroid/src/main/java/com/ichi2/anki/preferences/Preferences.kt (date 1763544734216)
@@ -141,8 +141,8 @@
addToBackStack(fragment.javaClass.name)
}
- if (isTabSpecificPreference(result.resourceFile)) {
- (fragment as? ControlsSettingsFragment)?.lifecycleScope?.launch {
+ if (fragment is ControlsSettingsFragment) {
+ fragment.lifecycleScope.launch {
if (areSystemAnimationsEnabled(requireContext())) {
delay(100)
fragment.selectTabForPreference(result.key)
@@ -159,14 +159,6 @@
}
}
- /**
- * Checks if the given resource file represents a tab-specific preference
- * that requires special tab navigation handling.
- */
- private fun isTabSpecificPreference(
- @XmlRes file: Int,
- ) = ControlPreferenceScreen.entries.any { it.xmlRes == file }
-
private fun setupBackCallbacks() {
requireActivity().onBackPressedDispatcher.addCallback(viewLifecycleOwner, childFragmentOnBackPressedCallback)
childFragmentManager.addOnBackStackChangedListener(childBackStackListener)
AnkiDroid/src/main/java/com/ichi2/anki/preferences/Preferences.kt
Outdated
Show resolved
Hide resolved
| * This function returns false if any of the mentioned system animations are disabled (0f), | ||
| * which addresses safe display mode and accessibility concerns. | ||
| * | ||
| * ANIMATION_DURATION_SCALE - controls app switching animation speed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the docs, apparently checking only this one is necessary.
https://developer.android.com/reference/android/provider/Settings.Global#TRANSITION_ANIMATION_SCALE
Scaling factor for Animator-based animations. This affects both the start delay and duration of all such animations. The value is a float. Setting to 0.0f will cause animations to end immediately. The default value is 1.0f.
On my phone (Android 16, Samsung), those scales can be changed only in the phone developer options. The public user facing setting only allows reducing animations in general.
You don't need to take any action based on this comment, but it's something that I think it's worth noting so the method may be simplified or better documented later.
BrayanDSO
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
meant to request changes
49eb1da to
3aa25bd
Compare
david-allison
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question on animations
Feel free to ignore the nitpick, unless something quick can be done
| result.highlight(fragment as PreferenceFragmentCompat) | ||
| if (fragment is ControlsSettingsFragment) { | ||
| fragment.lifecycleScope.launch { | ||
| if (areSystemAnimationsEnabled(requireContext())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this only check system animations, ignoring the in-app 'safe display mode'
Anki-Android/AnkiDroid/src/main/java/com/ichi2/anki/AnkiActivity.kt
Lines 282 to 292 in 1be4ad3
| /** | |
| * Whether animations should not be displayed | |
| * This is used to improve the UX for e-ink devices | |
| * Can be tested via Settings - Advanced - Safe display mode | |
| * | |
| * @see .animationEnabled | |
| */ | |
| fun animationDisabled(): Boolean { | |
| val preferences = this.sharedPrefs() | |
| return preferences.getBoolean("safeDisplay", false) | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I tested with system animations disabled, the tab selection and preference highlighting don't work in the 'Animations disabled - do everything immediately' case. I think we need delays regardless of the
areSystemAnimationsEnabled condition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry this sat, could you provide reproduction steps or a video
Purpose / Description
Fixes
How Has This Been Tested?
WhatsApp.Video.2025-11-03.at.11.17.51.PM.mp4
Checklist
Please, go through these checks before submitting the PR.