Anki-Android icon indicating copy to clipboard operation
Anki-Android copied to clipboard

CustomStudyDialog uses Alert Dialog

Open neeldoshii opened this issue 1 year ago • 2 comments

Fixes

  • Related #13315

Approach

Replaces usages of Deprecated MaterialDialog and adds the Native AlertDialog For CustomStudyDialog.

Images

Material Dialog Alert Dialog
Material Dialog Image Alert Dialog Image
Material Dialog Alert Dialog
Material Dialog Image Alert Dialog Image

Addition Consideration :

.listItems(items = getValuesFromKeys(keyValueMap, listIds).toList().map { it as CharSequence }) { _: MaterialDialog, _: Int, charSequence: CharSequence ->

For this we didn't had extension method I have used manual method for now using .setItems()

Checklist

  • [x] You have a descriptive commit message with a short title (first line, max 50 chars).
  • [x] You have commented your code, particularly in hard-to-understand areas
  • [x] You have performed a self-review of your own code
  • [X] UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • [X] UI Changes: You have tested your change using the Google Accessibility Scanner

neeldoshii avatar Feb 08 '24 18:02 neeldoshii

tests failing

david-allison avatar Feb 08 '24 20:02 david-allison

I am making this PR as draft for now. As I have many PR in queue once its ready for review I will open it again.

Also the code needs improvement for now. It's better first I will create an extension method then use it rather than creating .setItems it will lack DRY approach.

neeldoshii avatar Feb 08 '24 20:02 neeldoshii

I am making this PR as draft for now. As I have many PR in queue once its ready for review I will open it again.

The PR is now ready for review.

neeldoshii avatar Feb 28 '24 16:02 neeldoshii

Tests are failing

Hi @david-allison. The test is getting failed because in CustomStudyDialogTest we have used MaterialDialog so when testing it with AlertDialog its getting failed i guess. CustomStudyDialogTest

CustomStudyDialogTest > increaseNewCardLimitRegressionTest FAILED
java.lang.ClassCastException: class androidx.appcompat.app.AlertDialog cannot be cast to class com.afollestad.materialdialogs.MaterialDialog (androidx.appcompat.app.AlertDialog and com.afollestad.materialdialogs.MaterialDialog are in unnamed module of loader org.robolectric.internal.AndroidSandbox$SdkSandboxClassLoader @376498da)

neeldoshii avatar Feb 28 '24 17:02 neeldoshii

Hello 👋, this PR has had no activity for more than 2 weeks and needs a reply from the author. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically

github-actions[bot] avatar Mar 13 '24 18:03 github-actions[bot]

Subject: [PATCH] fix: videos with only audio stream

---
Index: AnkiDroid/src/test/java/com/ichi2/anki/dialogs/CustomStudyDialogTest.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/AnkiDroid/src/test/java/com/ichi2/anki/dialogs/CustomStudyDialogTest.kt b/AnkiDroid/src/test/java/com/ichi2/anki/dialogs/CustomStudyDialogTest.kt
--- a/AnkiDroid/src/test/java/com/ichi2/anki/dialogs/CustomStudyDialogTest.kt	(revision a6573e61bbaed9a836f46611564488a27631fc6f)
+++ b/AnkiDroid/src/test/java/com/ichi2/anki/dialogs/CustomStudyDialogTest.kt	(date 1710368439209)
@@ -15,8 +15,7 @@
  */
 package com.ichi2.anki.dialogs
 
-import android.app.AlertDialog
-import android.content.DialogInterface
+import androidx.appcompat.app.AlertDialog
 import androidx.fragment.app.testing.FragmentScenario
 import androidx.lifecycle.Lifecycle
 import androidx.test.ext.junit.runners.AndroidJUnit4
@@ -24,6 +23,7 @@
 import com.ichi2.anki.dialogs.customstudy.CustomStudyDialog
 import com.ichi2.anki.dialogs.customstudy.CustomStudyDialog.CustomStudyListener
 import com.ichi2.anki.dialogs.customstudy.CustomStudyDialogFactory
+import com.ichi2.anki.dialogs.utils.performPositiveClick
 import com.ichi2.libanki.Collection
 import com.ichi2.libanki.sched.Scheduler
 import com.ichi2.testutils.ParametersUtils
@@ -40,6 +40,8 @@
 import org.mockito.kotlin.mock
 import org.mockito.kotlin.whenever
 import org.robolectric.annotation.Config
+import kotlin.test.assertNotNull
+
 
 @RunWith(AndroidJUnit4::class)
 class CustomStudyDialogTest : RobolectricTest() {
@@ -62,12 +64,11 @@
             .withArguments(CustomStudyDialog.ContextMenuOption.STUDY_AHEAD, 1)
             .arguments
         val factory = CustomStudyDialogFactory({ this.col }, mockListener)
-        val scenario = FragmentScenario.launch(CustomStudyDialog::class.java, args, factory)
-        scenario.moveToState(Lifecycle.State.STARTED)
+        val scenario = FragmentScenario.launch(CustomStudyDialog::class.java, args, androidx.appcompat.R.style.Theme_AppCompat, factory)
+        scenario.moveToState(Lifecycle.State.RESUMED)
         scenario.onFragment { f: CustomStudyDialog ->
-            val dialog = f.dialog as AlertDialog?
-            MatcherAssert.assertThat(dialog, IsNull.notNullValue())
-            dialog?.getButton(DialogInterface.BUTTON_POSITIVE)?.callOnClick()
+            val dialog = assertNotNull(f.dialog as AlertDialog?)
+            dialog.performPositiveClick()
         }
         val customStudy = col.decks.current()
         MatcherAssert.assertThat("Custom Study should be dynamic", customStudy.isFiltered)
Index: AnkiDroid/src/test/java/com/ichi2/anki/dialogs/utils/AlertDialogUtils.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/AnkiDroid/src/test/java/com/ichi2/anki/dialogs/utils/AlertDialogUtils.kt b/AnkiDroid/src/test/java/com/ichi2/anki/dialogs/utils/AlertDialogUtils.kt
--- a/AnkiDroid/src/test/java/com/ichi2/anki/dialogs/utils/AlertDialogUtils.kt	(revision a6573e61bbaed9a836f46611564488a27631fc6f)
+++ b/AnkiDroid/src/test/java/com/ichi2/anki/dialogs/utils/AlertDialogUtils.kt	(date 1710368645170)
@@ -16,9 +16,15 @@
 
 package com.ichi2.anki.dialogs.utils
 
+import android.content.DialogInterface
 import android.widget.TextView
 import androidx.appcompat.app.AlertDialog
+import androidx.core.view.isVisible
+import org.hamcrest.MatcherAssert.*
+import androidx.test.platform.app.InstrumentationRegistry
+import com.ichi2.utils.HandlerUtils.executeFunctionUsingHandler
 import com.ichi2.utils.getInputField
+import kotlin.test.assertNotNull
 
 var AlertDialog.input
     get() = getInputField().text.toString()
@@ -28,3 +34,12 @@
     get() = requireNotNull(this.findViewById<TextView>(androidx.appcompat.R.id.alertTitle)) {
         "androidx.appcompat.R.id.alertTitle not found"
     }.text.toString()
+
+fun AlertDialog.performPositiveClick() {
+    // This exists as callOnClick did not call the listener
+    val positiveButton = assertNotNull(getButton(DialogInterface.BUTTON_POSITIVE), message =  "positive button")
+    assertThat("button is visible", positiveButton.isVisible)
+    assertThat("button is enalbed", positiveButton.isEnabled)
+    executeFunctionUsingHandler { positiveButton.callOnClick() }
+    InstrumentationRegistry.getInstrumentation().waitForIdleSync()
+}
\ No newline at end of file

david-allison avatar Mar 13 '24 22:03 david-allison

Test case passed!!! Special thanks to David for the patch that fixed the testcase.

neeldoshii avatar Mar 13 '24 22:03 neeldoshii

Hi, should I squash down to two commits now

  • Refactor to Alert Dialog
  • Listitem code refactor

or should I squash down to one commit?

neeldoshii avatar Apr 03 '24 19:04 neeldoshii

Two commits, both of which should compile if you git checkout <hash>

  • Each commit in main should compile
  • Each commit should aim to do one thing, as this makes reviewing, diagnosing bugs and reverting easier

david-allison avatar Apr 04 '24 02:04 david-allison

Squashed the commits.

Did a git checkout for https://github.com/ankidroid/Anki-Android/pull/15459/commits/b48180b3a861e6e7988654d03d844f0b7ad064a6 and it compiles.

neeldoshii avatar Apr 04 '24 05:04 neeldoshii

@lukstbit should I remove this redundant package as this is no longer used in our test after CustomStudyDialog migration? https://github.com/ankidroid/Anki-Android/blob/6354574aee656b96f732928007618286e9e6c239/AnkiDroid/src/test/java/com/ichi2/testutils/DialogUtils.kt#L17-L37

neeldoshii avatar Apr 26 '24 11:04 neeldoshii

can do any further refactors as an easy followup PR (e.g., removing a no-longer-needed package if that's the case)

mikehardy avatar Apr 26 '24 21:04 mikehardy

Hi there @neeldoshii! This is the OpenCollective Notice for PRs merged from 2024-04-01 through 2024-04-30

If you are interested in compensation for this work, the process with details is here:

https://github.com/ankidroid/Anki-Android/wiki/OpenCollective-Payment-Process#how-to-get-paid

We only post one comment per person per month to avoid spamming you, regardless of the number of PRs merged, but this note applies to all PRs merged for this month

Please understand that our monthly budget is never guaranteed to cover all claims - the cap on payments-per-person may be lower, but we try to make our process as fair and transparent as possible, we just need your understanding.

Thanks!

github-actions[bot] avatar May 16 '24 23:05 github-actions[bot]