For issue mozilla-mobile#9949 - Inconsistencies between deleting book…

…mark and history items

 - Added the undo action for deleting individual history items by creating a new field to the history state containing the id's of the history items that are pending for deletion; This field is used inside the update function from the view to show/hide the items.

 - Added a new check inside the "deleteMulti" method from BookmarkFragment that calls the showRemoveFoldersDialog to prevent the user from being able to delete one or more bookmark folders without being asked for confirmation, as in mozilla-mobile#8648.

app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkController.kt

@@ -35,7 +35,7 @@ interface BookmarkController {
     fun handleBookmarkSharing(item: BookmarkNode)
     fun handleOpeningBookmark(item: BookmarkNode, mode: BrowsingMode)
     fun handleBookmarkDeletion(nodes: Set<BookmarkNode>, eventType: Event)
-    fun handleBookmarkFolderDeletion(node: BookmarkNode)
+    fun handleBookmarkFolderDeletion(nodes: Set<BookmarkNode>)
     fun handleBackPressed()
 }
 
@@ -45,7 +45,7 @@ class DefaultBookmarkController(
     private val navController: NavController,
     private val showSnackbar: (String) -> Unit,
     private val deleteBookmarkNodes: (Set<BookmarkNode>, Event) -> Unit,
-    private val deleteBookmarkFolder: (BookmarkNode) -> Unit,
+    private val deleteBookmarkFolder: (Set<BookmarkNode>) -> Unit,
     private val invokePendingDeletion: () -> Unit
 ) : BookmarkController {
 
@@ -94,8 +94,8 @@ class DefaultBookmarkController(
         deleteBookmarkNodes(nodes, eventType)
     }
 
-    override fun handleBookmarkFolderDeletion(node: BookmarkNode) {
-        deleteBookmarkFolder(node)
+    override fun handleBookmarkFolderDeletion(nodes: Set<BookmarkNode>) {
+        deleteBookmarkFolder(nodes)
     }
 
     override fun handleBackPressed() {

app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragment.kt

@@ -269,13 +269,17 @@ class BookmarkFragment : LibraryPageFragment<BookmarkNode>(), UserInteractionHan
     }
 
     private fun deleteMulti(selected: Set<BookmarkNode>, eventType: Event = Event.RemoveBookmarks) {
+        selected.forEach { if (it.type == BookmarkNodeType.FOLDER) {
+            showRemoveFolderDialog(selected)
+            return
+        } }
         updatePendingBookmarksToDelete(selected)
 
         pendingBookmarkDeletionJob = getDeleteOperation(eventType)
 
         val message = when (eventType) {
             is Event.RemoveBookmarks -> {
-                getRemoveBookmarksSnackBarMessage(selected)
+                getRemoveBookmarksSnackBarMessage(selected, false)
             }
             is Event.RemoveBookmarkFolder,
             is Event.RemoveBookmark -> {
@@ -296,9 +300,16 @@ class BookmarkFragment : LibraryPageFragment<BookmarkNode>(), UserInteractionHan
         )
     }
 
-    private fun getRemoveBookmarksSnackBarMessage(selected: Set<BookmarkNode>): String {
+    private fun getRemoveBookmarksSnackBarMessage(
+        selected: Set<BookmarkNode>,
+        containsFolders: Boolean
+    ): String {
         return if (selected.size > 1) {
-            getString(R.string.bookmark_deletion_multiple_snackbar_message_2)
+            return if (containsFolders) {
+                getString(R.string.bookmark_deletion_multiple_snackbar_message_3)
+            } else {
+                getString(R.string.bookmark_deletion_multiple_snackbar_message_2)
+            }
         } else {
             val bookmarkNode = selected.first()
             getString(
@@ -309,29 +320,38 @@ class BookmarkFragment : LibraryPageFragment<BookmarkNode>(), UserInteractionHan
         }
     }
 
+    private fun getDialogConfirmationMessage(selected: Set<BookmarkNode>): String {
+        return if (selected.size > 1) {
+            getString(R.string.bookmark_delete_multiple_folders_confirmation_dialog, getString(R.string.app_name))
+        } else {
+            getString(R.string.bookmark_delete_folder_confirmation_dialog)
+        }
+    }
+
     override fun onDestroyView() {
         super.onDestroyView()
         _bookmarkInteractor = null
     }
 
-    private fun showRemoveFolderDialog(selected: BookmarkNode) {
+    private fun showRemoveFolderDialog(selected: Set<BookmarkNode>) {
         activity?.let { activity ->
             AlertDialog.Builder(activity).apply {
-                setMessage(R.string.bookmark_delete_folder_confirmation_dialog)
+                val dialogConfirmationMessage = getDialogConfirmationMessage(selected)
+                setMessage(dialogConfirmationMessage)
                 setNegativeButton(R.string.delete_browsing_data_prompt_cancel) { dialog: DialogInterface, _ ->
                     dialog.cancel()
                 }
                 setPositiveButton(R.string.delete_browsing_data_prompt_allow) { dialog: DialogInterface, _ ->
-                    updatePendingBookmarksToDelete(setOf(selected))
+                    updatePendingBookmarksToDelete(selected)
                     pendingBookmarkDeletionJob = getDeleteOperation(Event.RemoveBookmarkFolder)
                     dialog.dismiss()
-                    val message = getDeleteDialogString(selected)
+                    val snackbarMessage = getRemoveBookmarksSnackBarMessage(selected, true)
                     viewLifecycleOwner.lifecycleScope.allowUndo(
                         requireView(),
-                        message,
+                        snackbarMessage,
                         getString(R.string.bookmark_undo_deletion),
                         {
-                            undoPendingDeletion(setOf(selected))
+                            undoPendingDeletion(selected)
                         },
                         operation = getDeleteOperation(Event.RemoveBookmarkFolder)
                     )
@@ -348,14 +368,6 @@ class BookmarkFragment : LibraryPageFragment<BookmarkNode>(), UserInteractionHan
         bookmarkInteractor.onBookmarksChanged(bookmarkTree)
     }
 
-    private fun getDeleteDialogString(selected: BookmarkNode): String {
-        return getString(
-            R.string.bookmark_deletion_snackbar_message,
-            context?.components?.publicSuffixList?.let { selected.url?.toShortUrl(it) }
-                ?: selected.title
-        )
-    }
-
     private suspend fun undoPendingDeletion(selected: Set<BookmarkNode>) {
         pendingBookmarksToDelete.removeAll(selected)
         pendingBookmarkDeletionJob = null

app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragmentInteractor.kt

@@ -90,7 +90,7 @@ class BookmarkFragmentInteractor(
             null -> Event.RemoveBookmarks
         }
         if (eventType == Event.RemoveBookmarkFolder) {
-            bookmarksController.handleBookmarkFolderDeletion(nodes.first())
+            bookmarksController.handleBookmarkFolderDeletion(nodes)
         } else {
             bookmarksController.handleBookmarkDeletion(nodes, eventType)
         }

app/src/main/java/org/mozilla/fenix/library/history/HistoryAdapter.kt

@@ -33,6 +33,7 @@ class HistoryAdapter(
 
     private var mode: HistoryFragmentState.Mode = HistoryFragmentState.Mode.Normal
     override val selectedItems get() = mode.selectedItems
+    var pendingDeletionIds = emptySet<Int>()
 
     override fun getItemViewType(position: Int): Int = HistoryListItemViewHolder.LAYOUT_ID
 
@@ -54,7 +55,11 @@ class HistoryAdapter(
         val previousHeader = previous?.let(::timeGroupForHistoryItem)
         val currentHeader = timeGroupForHistoryItem(current)
         val timeGroup = if (currentHeader != previousHeader) currentHeader else null
-        holder.bind(current, timeGroup, position == 0, mode)
+        holder.bind(current, timeGroup, position == 0, mode, pendingDeletionIds.contains(current.id))
+    }
+
+    fun updatePendingDeletionIds(pendingDeletionIds: Set<Int>) {
+        this.pendingDeletionIds = pendingDeletionIds
     }
 
     companion object {

app/src/main/java/org/mozilla/fenix/library/history/HistoryFragment.kt

@@ -17,8 +17,11 @@ import android.view.ViewGroup
 import androidx.appcompat.app.AlertDialog
 import androidx.lifecycle.Observer
 import androidx.lifecycle.lifecycleScope
+import androidx.navigation.NavDirections
 import androidx.navigation.fragment.findNavController
 import kotlinx.android.synthetic.main.fragment_history.view.*
+import kotlinx.coroutines.CoroutineScope
+import kotlinx.coroutines.Dispatchers.IO
 import kotlinx.coroutines.Dispatchers.Main
 import kotlinx.coroutines.ExperimentalCoroutinesApi
 import kotlinx.coroutines.launch
@@ -36,11 +39,13 @@ import org.mozilla.fenix.components.StoreProvider
 import org.mozilla.fenix.components.history.createSynchronousPagedHistoryProvider
 import org.mozilla.fenix.components.metrics.Event
 import org.mozilla.fenix.ext.components
+import org.mozilla.fenix.ext.getStringWithArgSafe
 import org.mozilla.fenix.ext.nav
 import org.mozilla.fenix.ext.requireComponents
 import org.mozilla.fenix.ext.showToolbar
 import org.mozilla.fenix.ext.toShortUrl
 import org.mozilla.fenix.library.LibraryPageFragment
+import org.mozilla.fenix.utils.allowUndo
 
 @SuppressWarnings("TooManyFunctions", "LargeClass")
 class HistoryFragment : LibraryPageFragment<HistoryItem>(), UserInteractionHandler {
@@ -49,6 +54,9 @@ class HistoryFragment : LibraryPageFragment<HistoryItem>(), UserInteractionHandl
     private lateinit var historyInteractor: HistoryInteractor
     private lateinit var viewModel: HistoryViewModel
 
+    private var pendingHistoryDeletionJob: (suspend () -> Unit)? = null
+    private var pendingHistoryToDelete: MutableSet<HistoryItem> = mutableSetOf()
+
     override fun onCreateView(
         inflater: LayoutInflater,
         container: ViewGroup?,
@@ -108,18 +116,16 @@ class HistoryFragment : LibraryPageFragment<HistoryItem>(), UserInteractionHandl
     }
 
     private fun deleteHistoryItems(items: Set<HistoryItem>) {
-        val message = getMultiSelectSnackBarMessage(items)
-        viewLifecycleOwner.lifecycleScope.launch {
-            context?.components?.run {
-                for (item in items) {
-                    analytics.metrics.track(Event.HistoryItemRemoved)
-                    core.historyStorage.deleteVisit(item.url, item.visitedAt)
-                }
-            }
-            viewModel.invalidate()
-            showSnackBar(requireView(), message)
-            historyStore.dispatch(HistoryFragmentAction.ExitDeletionMode)
-        }
+        updatePendingHistoryToDelete(items)
+            CoroutineScope(IO).allowUndo(
+            requireView(),
+            getMultiSelectSnackBarMessage(items),
+            getString(R.string.bookmark_undo_deletion),
+            {
+                undoPendingDeletion(items)
+            },
+            getDeleteHistoryItemsOperation()
+        )
     }
 
     @ExperimentalCoroutinesApi
@@ -159,13 +165,7 @@ class HistoryFragment : LibraryPageFragment<HistoryItem>(), UserInteractionHandl
             true
         }
         R.id.delete_history_multi_select -> {
-            val message = getMultiSelectSnackBarMessage(selectedItems)
-            viewLifecycleOwner.lifecycleScope.launch(Main) {
-                deleteSelectedHistory(historyStore.state.mode.selectedItems, requireComponents)
-                viewModel.invalidate()
-                historyStore.dispatch(HistoryFragmentAction.ExitDeletionMode)
-                showSnackBar(requireView(), message)
-            }
+            deleteHistoryItems(historyStore.state.mode.selectedItems)
             true
         }
         R.id.open_history_in_new_tabs_multi_select -> {
@@ -174,10 +174,7 @@ class HistoryFragment : LibraryPageFragment<HistoryItem>(), UserInteractionHandl
                 selectedItem.url
             }
 
-            nav(
-                R.id.historyFragment,
-                HistoryFragmentDirections.actionGlobalHome()
-            )
+            navigate(HistoryFragmentDirections.actionGlobalHome())
             true
         }
         R.id.open_history_in_private_tabs_multi_select -> {
@@ -190,10 +187,7 @@ class HistoryFragment : LibraryPageFragment<HistoryItem>(), UserInteractionHandl
                 browsingModeManager.mode = BrowsingMode.Private
                 supportActionBar?.hide()
             }
-            nav(
-                R.id.historyFragment,
-                HistoryFragmentDirections.actionGlobalHome()
-            )
+            navigate(HistoryFragmentDirections.actionGlobalHome())
             true
         }
         else -> super.onOptionsItemSelected(item)
@@ -203,14 +197,22 @@ class HistoryFragment : LibraryPageFragment<HistoryItem>(), UserInteractionHandl
         return if (historyItems.size > 1) {
             getString(R.string.history_delete_multiple_items_snackbar)
         } else {
-            getString(
+            requireContext().getStringWithArgSafe(
                 R.string.history_delete_single_item_snackbar,
                 historyItems.first().url.toShortUrl(requireComponents.publicSuffixList)
             )
         }
     }
 
-    override fun onBackPressed(): Boolean = historyView.onBackPressed()
+    override fun onPause() {
+        invokePendingDeletion()
+        super.onPause()
+    }
+
+    override fun onBackPressed(): Boolean {
+        invokePendingDeletion()
+        return historyView.onBackPressed()
+    }
 
     private fun openItem(item: HistoryItem, mode: BrowsingMode? = null) {
         requireComponents.analytics.metrics.track(Event.HistoryItemOpened)
@@ -266,6 +268,58 @@ class HistoryFragment : LibraryPageFragment<HistoryItem>(), UserInteractionHandl
         val directions = HistoryFragmentDirections.actionGlobalShareFragment(
             data = data.toTypedArray()
         )
-        nav(R.id.historyFragment, directions)
+        navigate(directions)
+    }
+
+    private fun navigate(directions: NavDirections) {
+        invokePendingDeletion()
+        findNavController().nav(
+            R.id.historyFragment,
+            directions
+        )
+    }
+
+    private fun getDeleteHistoryItemsOperation(): (suspend () -> Unit) {
+        return {
+            CoroutineScope(IO).launch {
+                historyStore.dispatch(HistoryFragmentAction.EnterDeletionMode)
+                context?.components?.run {
+                    for (item in pendingHistoryToDelete) {
+                        analytics.metrics.track(Event.HistoryItemRemoved)
+                        core.historyStorage.deleteVisit(item.url, item.visitedAt)
+                    }
+                }
+                historyStore.dispatch(HistoryFragmentAction.ExitDeletionMode)
+                viewModel.invalidate()
+            }
+            val pendingDeletionIds: Set<Int> = pendingHistoryToDelete.map { item -> item.id }.toSet()
+            historyStore.dispatch(HistoryFragmentAction.RemovePendingDeletionIds(pendingDeletionIds))
+            pendingHistoryToDelete.clear()
+            pendingHistoryDeletionJob = null
+        }
+    }
+
+    private fun updatePendingHistoryToDelete(items: Set<HistoryItem>) {
+        pendingHistoryToDelete.addAll(items)
+        pendingHistoryDeletionJob = getDeleteHistoryItemsOperation()
+        val pendingDeletionIds: Set<Int> = items.map { item -> item.id }.toSet()
+        historyStore.dispatch(HistoryFragmentAction.AddPendingDeletionIds(pendingDeletionIds))
+    }
+
+    private fun undoPendingDeletion(items: Set<HistoryItem>) {
+        pendingHistoryToDelete.removeAll(items)
+        pendingHistoryDeletionJob = null
+        val pendingDeletionIds: Set<Int> = items.map { item -> item.id }.toSet()
+        historyStore.dispatch(HistoryFragmentAction.RemovePendingDeletionIds(pendingDeletionIds))
+    }
+
+    private fun invokePendingDeletion() {
+        pendingHistoryDeletionJob?.let {
+            viewLifecycleOwner.lifecycleScope.launch {
+                it.invoke()
+            }.invokeOnCompletion {
+                pendingHistoryDeletionJob = null
+            }
+        }
     }
 }

app/src/main/java/org/mozilla/fenix/library/history/HistoryFragmentStore.kt

@@ -30,6 +30,8 @@ sealed class HistoryFragmentAction : Action {
     object ExitEditMode : HistoryFragmentAction()
     data class AddItemForRemoval(val item: HistoryItem) : HistoryFragmentAction()
     data class RemoveItemForRemoval(val item: HistoryItem) : HistoryFragmentAction()
+    data class AddPendingDeletionIds(val itemIds: Set<Int>) : HistoryFragmentAction()
+    data class RemovePendingDeletionIds(val itemIds: Set<Int>) : HistoryFragmentAction()
     object EnterDeletionMode : HistoryFragmentAction()
     object ExitDeletionMode : HistoryFragmentAction()
 }
@@ -39,7 +41,11 @@ sealed class HistoryFragmentAction : Action {
  * @property items List of HistoryItem to display
  * @property mode Current Mode of History
  */
-data class HistoryFragmentState(val items: List<HistoryItem>, val mode: Mode) : State {
+data class HistoryFragmentState(
+    val items: List<HistoryItem>,
+    val mode: Mode,
+    val pendingDeletionIds: Set<Int> = emptySet()
+) : State {
     sealed class Mode {
         open val selectedItems = emptySet<HistoryItem>()
 
@@ -72,5 +78,12 @@ private fun historyStateReducer(
         is HistoryFragmentAction.ExitEditMode -> state.copy(mode = HistoryFragmentState.Mode.Normal)
         is HistoryFragmentAction.EnterDeletionMode -> state.copy(mode = HistoryFragmentState.Mode.Deleting)
         is HistoryFragmentAction.ExitDeletionMode -> state.copy(mode = HistoryFragmentState.Mode.Normal)
+        is HistoryFragmentAction.AddPendingDeletionIds ->
+            state.copy(
+                pendingDeletionIds = state.pendingDeletionIds + action.itemIds,
+                mode = HistoryFragmentState.Mode.Normal
+            )
+        is HistoryFragmentAction.RemovePendingDeletionIds ->
+            state.copy(pendingDeletionIds = state.pendingDeletionIds - action.itemIds)
     }
 }

app/src/main/java/org/mozilla/fenix/library/history/HistoryView.kt

@@ -106,6 +106,8 @@ class HistoryView(
         items = state.items
         mode = state.mode
 
+        historyAdapter.updatePendingDeletionIds(state.pendingDeletionIds)
+
         historyAdapter.updateMode(state.mode)
         val first = layoutManager.findFirstVisibleItemPosition()
         val last = layoutManager.findLastVisibleItemPosition() + 1