fix(controls): improve post-assignment dialog

[Sanjeev-Narang]

Purpose / Description

The 'Only answer' option is supposed to remove the binding from 'Show answer' but it was not doing that. The dialog's appearance was not consistent with the previous subsequent dialogs in controls. Also need to implement option 3 mentioned in the issue.

Fixes

• Fixes Improve post-assignment dialog for Answer actions in Controls #20759

Approach

1. Update the preference resource file Strings according to the option 3.
2. In Fragment, replace the items list and logic, with a question message and consistent buttons
3. Correct the 'No' ( previously 'only answer') logic.

Before:

After:

How Has This Been Tested?

Unit test, API36

Learning (optional, can help others)

Learnt about controls settings, testing and the issue related codebase of AnkiDroid

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]

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.

[github-actions]

Important

Maintainers: This PR contains Strings changes

1. Sync Translations before merging this PR and wait for the action to complete
2. Review and merge the auto-generated PR in order to sync all user-submitted translations
3. Sync Translations again and merge the PR so the huge automated string changes caused by merging this PR are by themselves and easy to review

[david-allison]

Handle the case where the binding is already assigned to 'Show answer': - 'No' will remove it without confirmation.

I'd question using yes/no over more descriptive strings. Material design used to discourage yes/no.

@manocormen FYI - I'll defer to your thoughts here

[david-allison]

nit: inline this, we're no longer calling setItems(items)

[manocormen]

Material 3 seem to still discourage yes/no, so instead we could use:

• Positive case: "Assign it".
• Negative case: "Don't assign it", and when it's already assigned, "Unassign it", to make the removal explicit.

[Sanjeev-Narang]

@manocormen since we are discussing a more descriptive alert dialog, perhaps you could reconsider my earlier implementation:
#20759 (comment)

@david-allison Happy to update the PR once changes are decided.

[manocormen]

@Sanjeev-Narang In your earlier design, I liked the concision of "Keep" / "Remove", but I found the dialog at tad confusing in context: we've just assigned a gesture to action1, and now we're told "The gesture is already assigned to action2": to me, it makes it sound like a conflict, so I'd favor Zorn's simpler design (but with descriptive buttons).

@david-allison I'm not sure this is on the table, but we could also simplify: remove the secondary dialog altogether.

[Sanjeev-Narang]

@manocormen It's been few days, should we wait for David's response or implement the strings the way you suggested? ("assign it", "don't assign it" and "unassign it")

[david-allison]

I'll likely be able to get to this over the weekend, please go with @manocormen's guidance on this one

[manocormen]

@Sanjeev-Narang Thanks for bearing with us. We had a chat internally. Please implement the following behavior:

Also assign to 'Show answer'? Skip / Assign

If the gesture is already assigned to Show answer, don't show the dialog at all. This keeps the shortcut focused on intuitive positive assignments and sidesteps potentially confusing unassignment language.

[Sanjeev-Narang]

@manocormen sounds good. I'll implement this in next 1-2 days. In the meantime, i'm marking this PR as draft

[github-actions]

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

• PreferencesScreenshotTest: 7 changes

All 7 changed screenshots

PreferencesScreenshotTest

• AdvancedSettingsFragment_compare.png
• ControlsSettingsFragment_compare.png
• CustomButtonsSettingsFragment_compare.png
• DeveloperOptionsFragment_compare.png
• GeneralSettingsFragment_compare.png
• HeaderFragment_compare.png
• ReviewerOptionsFragment_compare.png

[manocormen]

Unless I'm missing something, skipping should simply assign the binding on the answer side.

[Sanjeev-Narang]

i'm really sorry. i forgot to remove that code. gonna fix this in couple of minutes.

[Sanjeev-Narang]

@manocormen I've updated the code.

[manocormen]

Thank you, LGTM. A maintainer will review this when possible; please give them time.

[david-allison]

Looks great!!!

Please add this test, rebase on main to fix the conflicts, then it should be good to go

See: https://github.com/ankidroid/Anki-Android/wiki/Development-Guide#applying-a-patch

Please squash the change in and force push. Let me know if you need help with this

Index: AnkiDroid/src/test/java/com/ichi2/anki/preferences/ControlsSettingsFragmentTest.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/AnkiDroid/src/test/java/com/ichi2/anki/preferences/ControlsSettingsFragmentTest.kt b/AnkiDroid/src/test/java/com/ichi2/anki/preferences/ControlsSettingsFragmentTest.kt
--- a/AnkiDroid/src/test/java/com/ichi2/anki/preferences/ControlsSettingsFragmentTest.kt	(revision 8d04499f8a0877c6e438641f84be7637235f495d)
+++ b/AnkiDroid/src/test/java/com/ichi2/anki/preferences/ControlsSettingsFragmentTest.kt	(date 1788818943645)
@@ -33,6 +33,8 @@
 import org.hamcrest.Matchers.not
 import org.junit.Test
 import org.junit.runner.RunWith
+import org.robolectric.shadows.ShadowDialog
+import kotlin.test.assertNull
 
 @RunWith(AndroidJUnit4::class)
 class ControlsSettingsFragmentTest : RobolectricTest() {
@@ -84,6 +86,22 @@
             assertThat("binding not assigned to answer", answerBindings, hasItem(equalTo(Pair(binding, CardSide.ANSWER))))
         }
     }
+
+    @Test
+    fun `no dialog if binding is already assigned to show answer`() {
+        withControlsSettings {
+            val binding = Binding.GestureInput(Gesture.SWIPE_UP)
+            showAnswerPref.addBinding(binding, CardSide.QUESTION)
+
+            answerPref.setBinding(binding)
+
+            assertNull(ShadowDialog.getLatestDialog(), "no dialog should be shown")
+            val showAnswerBindings = showAnswerPref.getMappableBindings().map { Pair(it.binding, it.side) }
+            val answerBindings = answerPref.getMappableBindings().map { Pair(it.binding, it.side) }
+            assertThat("binding not kept on Show answer", showAnswerBindings, hasItem(Pair(binding, CardSide.QUESTION)))
+            assertThat("binding not assigned to answer", answerBindings, hasItem(Pair(binding, CardSide.ANSWER)))
+        }
+    }
 }
 
 private val ControlsSettingsFragment.showAnswerPref

[david-allison]

Really nice

[Sanjeev-Narang]

@david-allison I added the patch. Please review

[david-allison]

Cheers! No need to ping, it's on my queue as soon as you push

[david-allison]

LGTM!

I have force pushed to get this in with a couple of nitpicks:

• strings are renamed to have a dialog_ prefix
• a typo in a test name was fixed

Awaiting a strings sync, and merge. On us

Thank you so much for your time and effort here! 🫡