From 3feab90b19ed6bd2b373bb915e04294aa189830a Mon Sep 17 00:00:00 2001 From: David Walsh Date: Wed, 29 Apr 2020 10:21:11 -0500 Subject: [PATCH] For issue #9949 - Bookmarks/History deletion inconsistencies - 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 #8648. --- .../java/org/mozilla/fenix/ui/HistoryTest.kt | 2 + .../mozilla/fenix/ui/robots/HistoryRobot.kt | 5 + .../library/bookmarks/BookmarkController.kt | 8 +- .../library/bookmarks/BookmarkFragment.kt | 46 ++++--- .../bookmarks/BookmarkFragmentInteractor.kt | 2 +- .../fenix/library/history/HistoryAdapter.kt | 32 ++++- .../fenix/library/history/HistoryFragment.kt | 129 ++++++++++++------ .../library/history/HistoryFragmentStore.kt | 22 ++- .../fenix/library/history/HistoryView.kt | 8 +- .../viewholders/HistoryListItemViewHolder.kt | 11 +- app/src/main/res/values/strings.xml | 6 +- .../bookmarks/BookmarkControllerTest.kt | 6 +- .../BookmarkFragmentInteractorTest.kt | 2 +- .../history/HistoryFragmentStoreTest.kt | 16 ++- 14 files changed, 206 insertions(+), 89 deletions(-) diff --git a/app/src/androidTest/java/org/mozilla/fenix/ui/HistoryTest.kt b/app/src/androidTest/java/org/mozilla/fenix/ui/HistoryTest.kt index bf0dd4e7b..c9a8bc177 100644 --- a/app/src/androidTest/java/org/mozilla/fenix/ui/HistoryTest.kt +++ b/app/src/androidTest/java/org/mozilla/fenix/ui/HistoryTest.kt @@ -158,6 +158,7 @@ class HistoryTest { }.openHistory { }.openThreeDotMenu { }.clickDelete { + verifyDeleteSnackbarText("Deleted") verifyEmptyHistoryView() } } @@ -174,6 +175,7 @@ class HistoryTest { clickDeleteHistoryButton() verifyDeleteConfirmationMessage() confirmDeleteAllHistory() + verifyDeleteSnackbarText("Browsing data deleted") verifyEmptyHistoryView() } } diff --git a/app/src/androidTest/java/org/mozilla/fenix/ui/robots/HistoryRobot.kt b/app/src/androidTest/java/org/mozilla/fenix/ui/robots/HistoryRobot.kt index d88b6f277..61f5e4d9b 100644 --- a/app/src/androidTest/java/org/mozilla/fenix/ui/robots/HistoryRobot.kt +++ b/app/src/androidTest/java/org/mozilla/fenix/ui/robots/HistoryRobot.kt @@ -83,6 +83,8 @@ class HistoryRobot { .click() } + fun verifyDeleteSnackbarText(text: String) = assertSnackBarText(text) + class Transition { fun goBack(interact: HistoryRobot.() -> Unit): Transition { goBackButton().click() @@ -152,3 +154,6 @@ private fun assertDeleteConfirmationMessage() = .check(matches(isDisplayed())) private fun assertCopySnackBarText() = snackBarText().check(matches(withText("URL copied"))) + +private fun assertSnackBarText(text: String) = + snackBarText().check(matches(withText(Matchers.containsString(text)))) diff --git a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkController.kt b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkController.kt index 5a52f148c..19c64df51 100644 --- a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkController.kt +++ b/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, eventType: Event) - fun handleBookmarkFolderDeletion(node: BookmarkNode) + fun handleBookmarkFolderDeletion(nodes: Set) fun handleBackPressed() } @@ -45,7 +45,7 @@ class DefaultBookmarkController( private val navController: NavController, private val showSnackbar: (String) -> Unit, private val deleteBookmarkNodes: (Set, Event) -> Unit, - private val deleteBookmarkFolder: (BookmarkNode) -> Unit, + private val deleteBookmarkFolder: (Set) -> 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) { + deleteBookmarkFolder(nodes) } override fun handleBackPressed() { diff --git a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragment.kt b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragment.kt index c6b26e770..21262394b 100644 --- a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragment.kt @@ -274,13 +274,17 @@ class BookmarkFragment : LibraryPageFragment(), UserInteractionHan } private fun deleteMulti(selected: Set, 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, containsFolders = false) } is Event.RemoveBookmarkFolder, is Event.RemoveBookmark -> { @@ -301,9 +305,16 @@ class BookmarkFragment : LibraryPageFragment(), UserInteractionHan ) } - private fun getRemoveBookmarksSnackBarMessage(selected: Set): String { + private fun getRemoveBookmarksSnackBarMessage( + selected: Set, + 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( @@ -314,29 +325,38 @@ class BookmarkFragment : LibraryPageFragment(), UserInteractionHan } } + private fun getDialogConfirmationMessage(selected: Set): 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) { 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, containsFolders = true) viewLifecycleOwner.lifecycleScope.allowUndo( requireView(), - message, + snackbarMessage, getString(R.string.bookmark_undo_deletion), { - undoPendingDeletion(setOf(selected)) + undoPendingDeletion(selected) }, operation = getDeleteOperation(Event.RemoveBookmarkFolder) ) @@ -353,14 +373,6 @@ class BookmarkFragment : LibraryPageFragment(), 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) { pendingBookmarksToDelete.removeAll(selected) pendingBookmarkDeletionJob = null diff --git a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragmentInteractor.kt b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragmentInteractor.kt index e8fac64b4..395c4f106 100644 --- a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragmentInteractor.kt +++ b/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) } diff --git a/app/src/main/java/org/mozilla/fenix/library/history/HistoryAdapter.kt b/app/src/main/java/org/mozilla/fenix/library/history/HistoryAdapter.kt index 5f970334a..234082b74 100644 --- a/app/src/main/java/org/mozilla/fenix/library/history/HistoryAdapter.kt +++ b/app/src/main/java/org/mozilla/fenix/library/history/HistoryAdapter.kt @@ -33,6 +33,8 @@ class HistoryAdapter( private var mode: HistoryFragmentState.Mode = HistoryFragmentState.Mode.Normal override val selectedItems get() = mode.selectedItems + var pendingDeletionIds = emptySet() + private val itemsWithHeaders: MutableMap = mutableMapOf() override fun getItemViewType(position: Int): Int = HistoryListItemViewHolder.LAYOUT_ID @@ -48,13 +50,33 @@ class HistoryAdapter( } override fun onBindViewHolder(holder: HistoryListItemViewHolder, position: Int) { - val previous = if (position == 0) null else getItem(position - 1) val current = getItem(position) ?: return + val headerForCurrentItem = timeGroupForHistoryItem(current) + val isPendingDeletion = pendingDeletionIds.contains(current.visitedAt) + var timeGroup: HistoryItemTimeGroup? = null - val previousHeader = previous?.let(::timeGroupForHistoryItem) - val currentHeader = timeGroupForHistoryItem(current) - val timeGroup = if (currentHeader != previousHeader) currentHeader else null - holder.bind(current, timeGroup, position == 0, mode) + // Add or remove the header and position to the map depending on it's deletion status + if (itemsWithHeaders.containsKey(headerForCurrentItem)) { + if (isPendingDeletion && itemsWithHeaders[headerForCurrentItem] == position) { + itemsWithHeaders.remove(headerForCurrentItem) + } else if (isPendingDeletion && itemsWithHeaders[headerForCurrentItem] != position) { + // do nothing + } else { + if (position <= itemsWithHeaders[headerForCurrentItem] as Int) { + itemsWithHeaders[headerForCurrentItem] = position + timeGroup = headerForCurrentItem + } + } + } else if (!isPendingDeletion) { + itemsWithHeaders[headerForCurrentItem] = position + timeGroup = headerForCurrentItem + } + + holder.bind(current, timeGroup, position == 0, mode, isPendingDeletion) + } + + fun updatePendingDeletionIds(pendingDeletionIds: Set) { + this.pendingDeletionIds = pendingDeletionIds } companion object { diff --git a/app/src/main/java/org/mozilla/fenix/library/history/HistoryFragment.kt b/app/src/main/java/org/mozilla/fenix/library/history/HistoryFragment.kt index ffc327cec..35f1a1281 100644 --- a/app/src/main/java/org/mozilla/fenix/library/history/HistoryFragment.kt +++ b/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 @@ -31,17 +34,18 @@ import org.mozilla.fenix.HomeActivity import org.mozilla.fenix.R import org.mozilla.fenix.addons.showSnackBar import org.mozilla.fenix.browser.browsingmode.BrowsingMode -import org.mozilla.fenix.components.Components import org.mozilla.fenix.components.FenixSnackbar 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(), UserInteractionHandler { @@ -49,6 +53,8 @@ class HistoryFragment : LibraryPageFragment(), UserInteractionHandl private lateinit var historyView: HistoryView private lateinit var historyInteractor: HistoryInteractor private lateinit var viewModel: HistoryViewModel + private var undoScope: CoroutineScope? = null + private var pendingHistoryDeletionJob: (suspend () -> Unit)? = null override fun onCreateView( inflater: LayoutInflater, @@ -59,7 +65,10 @@ class HistoryFragment : LibraryPageFragment(), UserInteractionHandl historyStore = StoreProvider.get(this) { HistoryFragmentStore( HistoryFragmentState( - items = listOf(), mode = HistoryFragmentState.Mode.Normal + items = listOf(), + mode = HistoryFragmentState.Mode.Normal, + pendingDeletionIds = emptySet(), + isDeletingItems = false ) ) } @@ -111,18 +120,18 @@ class HistoryFragment : LibraryPageFragment(), UserInteractionHandl } private fun deleteHistoryItems(items: Set) { - 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) + undoScope = CoroutineScope(IO) + undoScope?.allowUndo( + requireView(), + getMultiSelectSnackBarMessage(items), + getString(R.string.bookmark_undo_deletion), + { + undoPendingDeletion(items) + }, + getDeleteHistoryItemsOperation(items) + ) } @ExperimentalCoroutinesApi @@ -146,8 +155,8 @@ class HistoryFragment : LibraryPageFragment(), UserInteractionHandl override fun onCreateOptionsMenu(menu: Menu, inflater: MenuInflater) { val menuRes = when (historyStore.state.mode) { HistoryFragmentState.Mode.Normal -> R.menu.library_menu + is HistoryFragmentState.Mode.Syncing -> R.menu.library_menu is HistoryFragmentState.Mode.Editing -> R.menu.history_select_multi - else -> return } inflater.inflate(menuRes, menu) @@ -162,13 +171,8 @@ class HistoryFragment : LibraryPageFragment(), 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) + historyStore.dispatch(HistoryFragmentAction.ExitEditMode) true } R.id.open_history_in_new_tabs_multi_select -> { @@ -177,10 +181,7 @@ class HistoryFragment : LibraryPageFragment(), UserInteractionHandl selectedItem.url } - nav( - R.id.historyFragment, - HistoryFragmentDirections.actionGlobalHome() - ) + navigate(HistoryFragmentDirections.actionGlobalHome()) true } R.id.open_history_in_private_tabs_multi_select -> { @@ -193,10 +194,7 @@ class HistoryFragment : LibraryPageFragment(), UserInteractionHandl browsingModeManager.mode = BrowsingMode.Private supportActionBar?.hide() } - nav( - R.id.historyFragment, - HistoryFragmentDirections.actionGlobalHome() - ) + navigate(HistoryFragmentDirections.actionGlobalHome()) true } else -> super.onOptionsItemSelected(item) @@ -206,14 +204,22 @@ class HistoryFragment : LibraryPageFragment(), 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) @@ -253,23 +259,58 @@ class HistoryFragment : LibraryPageFragment(), UserInteractionHandl } } - private suspend fun deleteSelectedHistory( - selected: Set, - components: Components = requireComponents - ) { - requireComponents.analytics.metrics.track(Event.HistoryItemRemoved) - val storage = components.core.historyStorage - for (item in selected) { - storage.deleteVisit(item.url, item.visitedAt) - } - } - private fun share(data: List) { requireComponents.analytics.metrics.track(Event.HistoryItemShared) 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(items: Set): (suspend () -> Unit) { + return { + CoroutineScope(IO).launch { + historyStore.dispatch(HistoryFragmentAction.EnterDeletionMode) + context?.components?.run { + for (item in items) { + analytics.metrics.track(Event.HistoryItemRemoved) + core.historyStorage.deleteVisit(item.url, item.visitedAt) + } + } + historyStore.dispatch(HistoryFragmentAction.ExitDeletionMode) + pendingHistoryDeletionJob = null + } + } + } + + private fun updatePendingHistoryToDelete(items: Set) { + pendingHistoryDeletionJob = getDeleteHistoryItemsOperation(items) + val ids = items.map { item -> item.visitedAt }.toSet() + historyStore.dispatch(HistoryFragmentAction.AddPendingDeletionSet(ids)) + } + + private fun undoPendingDeletion(items: Set) { + pendingHistoryDeletionJob = null + val ids = items.map { item -> item.visitedAt }.toSet() + historyStore.dispatch(HistoryFragmentAction.UndoRemovePendingDeletionSet(ids)) + } + + private fun invokePendingDeletion() { + pendingHistoryDeletionJob?.let { + viewLifecycleOwner.lifecycleScope.launch { + it.invoke() + }.invokeOnCompletion { + pendingHistoryDeletionJob = null + } + } } private suspend fun syncHistory() { diff --git a/app/src/main/java/org/mozilla/fenix/library/history/HistoryFragmentStore.kt b/app/src/main/java/org/mozilla/fenix/library/history/HistoryFragmentStore.kt index 5b16b200c..cd50df173 100644 --- a/app/src/main/java/org/mozilla/fenix/library/history/HistoryFragmentStore.kt +++ b/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 AddPendingDeletionSet(val itemIds: Set) : HistoryFragmentAction() + data class UndoRemovePendingDeletionSet(val itemIds: Set) : HistoryFragmentAction() object EnterDeletionMode : HistoryFragmentAction() object ExitDeletionMode : HistoryFragmentAction() object StartSync : HistoryFragmentAction() @@ -41,12 +43,16 @@ sealed class HistoryFragmentAction : Action { * @property items List of HistoryItem to display * @property mode Current Mode of History */ -data class HistoryFragmentState(val items: List, val mode: Mode) : State { +data class HistoryFragmentState( + val items: List, + val mode: Mode, + val pendingDeletionIds: Set, + val isDeletingItems: Boolean +) : State { sealed class Mode { open val selectedItems = emptySet() object Normal : Mode() - object Deleting : Mode() object Syncing : Mode() data class Editing(override val selectedItems: Set) : Mode() } @@ -73,9 +79,17 @@ 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.EnterDeletionMode -> state.copy(isDeletingItems = true) + is HistoryFragmentAction.ExitDeletionMode -> state.copy(isDeletingItems = false) is HistoryFragmentAction.StartSync -> state.copy(mode = HistoryFragmentState.Mode.Syncing) is HistoryFragmentAction.FinishSync -> state.copy(mode = HistoryFragmentState.Mode.Normal) + is HistoryFragmentAction.AddPendingDeletionSet -> + state.copy( + pendingDeletionIds = state.pendingDeletionIds + action.itemIds + ) + is HistoryFragmentAction.UndoRemovePendingDeletionSet -> + state.copy( + pendingDeletionIds = state.pendingDeletionIds - action.itemIds + ) } } diff --git a/app/src/main/java/org/mozilla/fenix/library/history/HistoryView.kt b/app/src/main/java/org/mozilla/fenix/library/history/HistoryView.kt index b26f3ad99..8e461b54a 100644 --- a/app/src/main/java/org/mozilla/fenix/library/history/HistoryView.kt +++ b/app/src/main/java/org/mozilla/fenix/library/history/HistoryView.kt @@ -90,7 +90,6 @@ class HistoryView( val view: View = LayoutInflater.from(container.context) .inflate(R.layout.component_history, container, true) - private var items: List = listOf() var mode: HistoryFragmentState.Mode = HistoryFragmentState.Mode.Normal private set @@ -116,13 +115,16 @@ class HistoryView( fun update(state: HistoryFragmentState) { val oldMode = mode - view.progress_bar.isVisible = state.mode === HistoryFragmentState.Mode.Deleting + view.progress_bar.isVisible = state.isDeletingItems view.swipe_refresh.isRefreshing = state.mode === HistoryFragmentState.Mode.Syncing view.swipe_refresh.isEnabled = state.mode === HistoryFragmentState.Mode.Normal || state.mode === HistoryFragmentState.Mode.Syncing - items = state.items mode = state.mode + historyAdapter.updatePendingDeletionIds(state.pendingDeletionIds) + + updateEmptyState(state.pendingDeletionIds.size != historyAdapter.currentList?.size) + historyAdapter.updateMode(state.mode) val first = layoutManager.findFirstVisibleItemPosition() val last = layoutManager.findLastVisibleItemPosition() + 1 diff --git a/app/src/main/java/org/mozilla/fenix/library/history/viewholders/HistoryListItemViewHolder.kt b/app/src/main/java/org/mozilla/fenix/library/history/viewholders/HistoryListItemViewHolder.kt index 685081167..c14be966b 100644 --- a/app/src/main/java/org/mozilla/fenix/library/history/viewholders/HistoryListItemViewHolder.kt +++ b/app/src/main/java/org/mozilla/fenix/library/history/viewholders/HistoryListItemViewHolder.kt @@ -11,13 +11,13 @@ import kotlinx.android.synthetic.main.library_site_item.view.* import org.mozilla.fenix.R import org.mozilla.fenix.ext.hideAndDisable import org.mozilla.fenix.ext.showAndEnable -import org.mozilla.fenix.utils.Do import org.mozilla.fenix.library.SelectionHolder import org.mozilla.fenix.library.history.HistoryFragmentState import org.mozilla.fenix.library.history.HistoryInteractor import org.mozilla.fenix.library.history.HistoryItem import org.mozilla.fenix.library.history.HistoryItemMenu import org.mozilla.fenix.library.history.HistoryItemTimeGroup +import org.mozilla.fenix.utils.Do class HistoryListItemViewHolder( view: View, @@ -44,8 +44,15 @@ class HistoryListItemViewHolder( item: HistoryItem, timeGroup: HistoryItemTimeGroup?, showDeleteButton: Boolean, - mode: HistoryFragmentState.Mode + mode: HistoryFragmentState.Mode, + isPendingDeletion: Boolean = false ) { + if (isPendingDeletion) { + itemView.history_layout.visibility = View.GONE + } else { + itemView.history_layout.visibility = View.VISIBLE + } + itemView.history_layout.titleView.text = item.title itemView.history_layout.urlView.text = item.url diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 9b72708d4..077fb2e70 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -566,6 +566,8 @@ Select folder Are you sure you want to delete this folder? + + %s will delete the selected items. Deleted %1$s @@ -620,8 +622,10 @@ Deleted %1$s - + Bookmarks deleted + + Deleting selected folders UNDO diff --git a/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkControllerTest.kt b/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkControllerTest.kt index 71cf57e16..8faf7e314 100644 --- a/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkControllerTest.kt +++ b/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkControllerTest.kt @@ -42,7 +42,7 @@ class BookmarkControllerTest { private val navController: NavController = mockk(relaxed = true) private val showSnackbar: (String) -> Unit = mockk(relaxed = true) private val deleteBookmarkNodes: (Set, Event) -> Unit = mockk(relaxed = true) - private val deleteBookmarkFolder: (BookmarkNode) -> Unit = mockk(relaxed = true) + private val deleteBookmarkFolder: (Set) -> Unit = mockk(relaxed = true) private val invokePendingDeletion: () -> Unit = mockk(relaxed = true) private val homeActivity: HomeActivity = mockk(relaxed = true) @@ -240,10 +240,10 @@ class BookmarkControllerTest { @Test fun `handleBookmarkDeletion for a folder should properly call the delete folder delegate`() { - controller.handleBookmarkFolderDeletion(subfolder) + controller.handleBookmarkFolderDeletion(setOf(subfolder)) verify { - deleteBookmarkFolder(subfolder) + deleteBookmarkFolder(setOf(subfolder)) } } diff --git a/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkFragmentInteractorTest.kt b/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkFragmentInteractorTest.kt index c0129e961..2ffbe3f4b 100644 --- a/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkFragmentInteractorTest.kt +++ b/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkFragmentInteractorTest.kt @@ -196,7 +196,7 @@ class BookmarkFragmentInteractorTest { interactor.onDelete(setOf(subfolder)) verify { - bookmarkController.handleBookmarkFolderDeletion(subfolder) + bookmarkController.handleBookmarkFolderDeletion(setOf(subfolder)) } } diff --git a/app/src/test/java/org/mozilla/fenix/library/history/HistoryFragmentStoreTest.kt b/app/src/test/java/org/mozilla/fenix/library/history/HistoryFragmentStoreTest.kt index ee68f7b6c..a7b9470d5 100644 --- a/app/src/test/java/org/mozilla/fenix/library/history/HistoryFragmentStoreTest.kt +++ b/app/src/test/java/org/mozilla/fenix/library/history/HistoryFragmentStoreTest.kt @@ -60,7 +60,9 @@ class HistoryFragmentStoreTest { fun finishSync() = runBlocking { val initialState = HistoryFragmentState( items = listOf(), - mode = HistoryFragmentState.Mode.Syncing + mode = HistoryFragmentState.Mode.Syncing, + pendingDeletionIds = emptySet(), + isDeletingItems = false ) val store = HistoryFragmentStore(initialState) @@ -71,16 +73,22 @@ class HistoryFragmentStoreTest { private fun emptyDefaultState(): HistoryFragmentState = HistoryFragmentState( items = listOf(), - mode = HistoryFragmentState.Mode.Normal + mode = HistoryFragmentState.Mode.Normal, + pendingDeletionIds = emptySet(), + isDeletingItems = false ) private fun oneItemEditState(): HistoryFragmentState = HistoryFragmentState( items = listOf(), - mode = HistoryFragmentState.Mode.Editing(setOf(historyItem)) + mode = HistoryFragmentState.Mode.Editing(setOf(historyItem)), + pendingDeletionIds = emptySet(), + isDeletingItems = false ) private fun twoItemEditState(): HistoryFragmentState = HistoryFragmentState( items = listOf(), - mode = HistoryFragmentState.Mode.Editing(setOf(historyItem, newHistoryItem)) + mode = HistoryFragmentState.Mode.Editing(setOf(historyItem, newHistoryItem)), + pendingDeletionIds = emptySet(), + isDeletingItems = false ) }