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

Removed the Material Dialog Box and Added Alert Box from CardBrowserMySearchesDialog

Open neeldoshii opened this issue 1 year ago • 4 comments

Fixes

  • Related #13315

Approach

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

Video

https://github.com/ankidroid/Anki-Android/assets/60827173/e94a9360-9d12-462e-9b32-d75a9dcbeed5

Few questions :

~~1. Are we okay with removing the divider lines in My searches which were there in Material Dialog? As in our extension method of Alert Dialog we don't have it. 2. Padding seems uneven for (-)in my searches compared to Material dialog not sure its it good to go or we need to set padding for (-) explicitly .~~

  1. On closer look when we open save search the keyboard doesn't seems to work it needs a tap at the input and the input opens agains and works fine. I am not able to fix this issue. Need help in this.
  2. Can we add prefill for save search? As the user who search a particular thing is probably gonna save that new thing. Correct me if I am wrong. I didn't added it yet as it was not there previously.
input(hint = getString(R.string.card_browser_list_my_searches_new_name), **prefill = currentSearchTerms**, allowEmpty = false, displayKeyboard = true, waitForPositiveButton = true)

Checklist

Please, go through these checks before submitting the PR.

  • [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 03 '24 22:02 neeldoshii

Split out the 'easy' dialog into another PR

Fixing the second dialog will take longer. Padding definitely needs to be fixed

Also what about the divider decoration?

neeldoshii avatar Feb 07 '24 09:02 neeldoshii

Edit : I fixed it. Edit: Updated the PR description and added the commit. @david-allison ~~I have not tested it yet but I am thinking what if I create an extension function and apply it so we would be having same ui as before (will also have decoration divider, padding).~~ There is no need for padding. I just made the layout width match parent in card_browser_item_my_searches_dialog.xml and it works fine and gives us the desired result.

This is what I came up with it for extension method in AlertDialogFacade for creating the decoration divider which would make the code follow DRY approach for later.

/**
 * Adds a RecyclerView with a custom adapter and decoration to the AlertDialog.
 * @param adapter The adapter for the RecyclerView.
 * @param context The context used to access resources and LayoutInflater.
 */
fun AlertDialog.Builder.customListAdapterWithDecoration(adapter: RecyclerView.Adapter<*>, context: Context) {
    val recyclerView = LayoutInflater.from(context).inflate(R.layout.dialog_generic_recycler_view, null, false) as RecyclerView
    recyclerView.adapter = adapter
    recyclerView.layoutManager = LinearLayoutManager(context)
    val dividerItemDecoration = DividerItemDecoration(recyclerView.context, LinearLayoutManager.VERTICAL)
    recyclerView.addItemDecoration(dividerItemDecoration)
    this.setView(recyclerView)
}

neeldoshii avatar Feb 07 '24 11:02 neeldoshii

The vertical padding on that screenshot looks off, and the touch targets look too small

Try adding 8-12dp top & bottom and see if it looks good

david-allison avatar Feb 07 '24 15:02 david-allison

The vertical padding on that screenshot looks off, and the touch targets look too small

Try adding 8-12dp top & bottom and see if it looks good

image

I simplified the layout instead of nested layout(relative and linear, increasing size of imageview was not making the textview vertical center aligned properly) I made it simplified and clean.

few things can be again added as per accessibility suggestion by Android Studio

This item may not have a label readable by screen readers.

Adding a content description would fix this. I am not sure which string to add for it and not sure from localization. I am not adding it also its of low priority. Just adding it for reference of issue.

neeldoshii avatar Feb 07 '24 16:02 neeldoshii

@neeldoshii Check the patch below for specific changes. Note that I didn't verify the dialog on the emulator.

diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/dialogs/CardBrowserMySearchesDialog.kt b/AnkiDroid/src/main/java/com/ichi2/anki/dialogs/CardBrowserMySearchesDialog.kt
index b068b3d804..78bd03204c 100644
--- a/AnkiDroid/src/main/java/com/ichi2/anki/dialogs/CardBrowserMySearchesDialog.kt
+++ b/AnkiDroid/src/main/java/com/ichi2/anki/dialogs/CardBrowserMySearchesDialog.kt
@@ -5,20 +5,17 @@ import android.annotation.SuppressLint
 import android.app.Dialog
 import android.os.Bundle
 import androidx.appcompat.app.AlertDialog
-import androidx.recyclerview.widget.DividerItemDecoration
-import androidx.recyclerview.widget.LinearLayoutManager
-import com.afollestad.materialdialogs.MaterialDialog
-import com.afollestad.materialdialogs.input.input
-import com.afollestad.materialdialogs.list.customListAdapter
-import com.afollestad.materialdialogs.list.getRecyclerView
 import com.ichi2.anki.R
 import com.ichi2.anki.analytics.AnalyticsDialogFragment
 import com.ichi2.compat.CompatHelper.Companion.getSerializableCompat
 import com.ichi2.ui.ButtonItemAdapter
+import com.ichi2.utils.createAndApply
+import com.ichi2.utils.input
 import com.ichi2.utils.message
 import com.ichi2.utils.negativeButton
 import com.ichi2.utils.positiveButton
 import com.ichi2.utils.show
+import com.ichi2.utils.title
 import timber.log.Timber
 
 // TODO: Add different classes for the two different dialogs
@@ -37,7 +34,7 @@ class CardBrowserMySearchesDialog : AnalyticsDialogFragment() {
     @SuppressLint("CheckResult")
     override fun onCreateDialog(savedInstanceState: Bundle?): Dialog {
         super.onCreate(savedInstanceState)
-        val dialog = MaterialDialog(requireActivity())
+        val dialog = AlertDialog.Builder(requireActivity())
         val type = requireArguments().getInt("type")
         if (type == CARD_BROWSER_MY_SEARCHES_TYPE_LIST) {
             savedFilters = requireArguments().getSerializableCompat("savedFilters")
@@ -51,7 +48,7 @@ class CardBrowserMySearchesDialog : AnalyticsDialogFragment() {
                 itemCallback = { searchName ->
                     Timber.d("item clicked: %s", searchName)
                     mySearchesDialogListener!!.onSelection(searchName)
-                    dialog.dismiss()
+                    dismiss()
                 },
                 buttonCallback = { searchName ->
                     Timber.d("button clicked: %s", searchName)
@@ -60,27 +57,24 @@ class CardBrowserMySearchesDialog : AnalyticsDialogFragment() {
             ).apply {
                 notifyAdapterDataSetChanged() // so the values are sorted.
                 dialog.title(text = resources.getString(R.string.card_browser_list_my_searches_title))
-                    .customListAdapter(this, null)
+                    .customListAdapterWithDecoration(this, requireActivity())
             }
         } else if (type == CARD_BROWSER_MY_SEARCHES_TYPE_SAVE) {
             val currentSearchTerms = requireArguments().getString("currentSearchTerms")
-            dialog.title(text = getString(R.string.card_browser_list_my_searches_save))
-                .positiveButton(android.R.string.ok)
-                .negativeButton(R.string.dialog_cancel)
-                .input(hintRes = R.string.card_browser_list_my_searches_new_name) { _: MaterialDialog?, text: CharSequence ->
-                    Timber.d("Saving search with title/terms: %s/%s", text, currentSearchTerms)
-                    mySearchesDialogListener!!.onSaveSearch(text.toString(), currentSearchTerms)
+            return dialog.show {
+                    title(text = getString(R.string.card_browser_list_my_searches_save))
+                    positiveButton(android.R.string.ok)
+                    negativeButton(R.string.dialog_cancel)
+                    setView(R.layout.dialog_generic_text_input)
+                }.apply {
+                    input(hint = getString(R.string.card_browser_list_my_searches_new_name)) { dialog, text ->
+                            Timber.d("Saving search with title/terms: %s/%s", text, currentSearchTerms)
+                            mySearchesDialogListener!!.onSaveSearch(text.toString(), currentSearchTerms)
+                            dialog.dismiss()
+                    }
                 }
         }
-        runCatching { dialog.getRecyclerView() }.onSuccess { recyclerView ->
-            val layoutManager = recyclerView.layoutManager as LinearLayoutManager
-            val dividerItemDecoration = DividerItemDecoration(recyclerView.context, layoutManager.orientation)
-            val scale = resources.displayMetrics.density
-            val dpAsPixels = (5 * scale + 0.5f).toInt()
-            dialog.view.setPadding(dpAsPixels, 0, dpAsPixels, dpAsPixels)
-            recyclerView.addItemDecoration(dividerItemDecoration)
-        }
-        return dialog
+        return dialog.create()
     }
 
     private fun removeSearch(searchName: String) {

lukstbit avatar Feb 26 '24 06:02 lukstbit

@neeldoshii Check the patch below for specific changes. Note that I didn't verify the dialog on the emulator.

diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/dialogs/CardBrowserMySearchesDialog.kt b/AnkiDroid/src/main/java/com/ichi2/anki/dialogs/CardBrowserMySearchesDialog.kt
index b068b3d804..78bd03204c 100644
--- a/AnkiDroid/src/main/java/com/ichi2/anki/dialogs/CardBrowserMySearchesDialog.kt
+++ b/AnkiDroid/src/main/java/com/ichi2/anki/dialogs/CardBrowserMySearchesDialog.kt
@@ -5,20 +5,17 @@ import android.annotation.SuppressLint
 import android.app.Dialog
 import android.os.Bundle
 import androidx.appcompat.app.AlertDialog
-import androidx.recyclerview.widget.DividerItemDecoration
-import androidx.recyclerview.widget.LinearLayoutManager
-import com.afollestad.materialdialogs.MaterialDialog
-import com.afollestad.materialdialogs.input.input
-import com.afollestad.materialdialogs.list.customListAdapter
-import com.afollestad.materialdialogs.list.getRecyclerView
 import com.ichi2.anki.R
 import com.ichi2.anki.analytics.AnalyticsDialogFragment
 import com.ichi2.compat.CompatHelper.Companion.getSerializableCompat
 import com.ichi2.ui.ButtonItemAdapter
+import com.ichi2.utils.createAndApply
+import com.ichi2.utils.input
 import com.ichi2.utils.message
 import com.ichi2.utils.negativeButton
 import com.ichi2.utils.positiveButton
 import com.ichi2.utils.show
+import com.ichi2.utils.title
 import timber.log.Timber
 
 // TODO: Add different classes for the two different dialogs
@@ -37,7 +34,7 @@ class CardBrowserMySearchesDialog : AnalyticsDialogFragment() {
     @SuppressLint("CheckResult")
     override fun onCreateDialog(savedInstanceState: Bundle?): Dialog {
         super.onCreate(savedInstanceState)
-        val dialog = MaterialDialog(requireActivity())
+        val dialog = AlertDialog.Builder(requireActivity())
         val type = requireArguments().getInt("type")
         if (type == CARD_BROWSER_MY_SEARCHES_TYPE_LIST) {
             savedFilters = requireArguments().getSerializableCompat("savedFilters")
@@ -51,7 +48,7 @@ class CardBrowserMySearchesDialog : AnalyticsDialogFragment() {
                 itemCallback = { searchName ->
                     Timber.d("item clicked: %s", searchName)
                     mySearchesDialogListener!!.onSelection(searchName)
-                    dialog.dismiss()
+                    dismiss()
                 },
                 buttonCallback = { searchName ->
                     Timber.d("button clicked: %s", searchName)
@@ -60,27 +57,24 @@ class CardBrowserMySearchesDialog : AnalyticsDialogFragment() {
             ).apply {
                 notifyAdapterDataSetChanged() // so the values are sorted.
                 dialog.title(text = resources.getString(R.string.card_browser_list_my_searches_title))
-                    .customListAdapter(this, null)
+                    .customListAdapterWithDecoration(this, requireActivity())
             }
         } else if (type == CARD_BROWSER_MY_SEARCHES_TYPE_SAVE) {
             val currentSearchTerms = requireArguments().getString("currentSearchTerms")
-            dialog.title(text = getString(R.string.card_browser_list_my_searches_save))
-                .positiveButton(android.R.string.ok)
-                .negativeButton(R.string.dialog_cancel)
-                .input(hintRes = R.string.card_browser_list_my_searches_new_name) { _: MaterialDialog?, text: CharSequence ->
-                    Timber.d("Saving search with title/terms: %s/%s", text, currentSearchTerms)
-                    mySearchesDialogListener!!.onSaveSearch(text.toString(), currentSearchTerms)
+            return dialog.show {
+                    title(text = getString(R.string.card_browser_list_my_searches_save))
+                    positiveButton(android.R.string.ok)
+                    negativeButton(R.string.dialog_cancel)
+                    setView(R.layout.dialog_generic_text_input)
+                }.apply {
+                    input(hint = getString(R.string.card_browser_list_my_searches_new_name)) { dialog, text ->
+                            Timber.d("Saving search with title/terms: %s/%s", text, currentSearchTerms)
+                            mySearchesDialogListener!!.onSaveSearch(text.toString(), currentSearchTerms)
+                            dialog.dismiss()
+                    }
                 }
         }
-        runCatching { dialog.getRecyclerView() }.onSuccess { recyclerView ->
-            val layoutManager = recyclerView.layoutManager as LinearLayoutManager
-            val dividerItemDecoration = DividerItemDecoration(recyclerView.context, layoutManager.orientation)
-            val scale = resources.displayMetrics.density
-            val dpAsPixels = (5 * scale + 0.5f).toInt()
-            dialog.view.setPadding(dpAsPixels, 0, dpAsPixels, dpAsPixels)
-            recyclerView.addItemDecoration(dividerItemDecoration)
-        }
-        return dialog
+        return dialog.create()
     }
 
     private fun removeSearch(searchName: String) {

tested and fixed the issue in the current commit. Thank you @lukstbit

neeldoshii avatar Feb 26 '24 11:02 neeldoshii

It still feels like the padding is too large on the right hand side

Implementer's choice, it looks better than the previous screen at this point

Thanks!!!

Should I update the code to match the padding like this for reducing padding right?

  1. Can we add prefill for save search? As the user who search a particular thing is probably gonna save that new thing. Correct me if I am wrong. I didn't added it yet as it was not there previously.
input(hint = getString(R.string.card_browser_list_my_searches_new_name), **prefill = currentSearchTerms**, allowEmpty = false, displayKeyboard = true, waitForPositiveButton = true)

I have one more question from the PR description which could be helpful. Should we add this?

neeldoshii avatar Feb 26 '24 11:02 neeldoshii

Related to your questions, leave the code/UI as it is for now.

lukstbit avatar Feb 29 '24 05:02 lukstbit

squashed + force pushed

david-allison avatar Feb 29 '24 06:02 david-allison