diff --git a/app/src/androidTest/java/org/mozilla/fenix/ui/BookmarksTest.kt b/app/src/androidTest/java/org/mozilla/fenix/ui/BookmarksTest.kt index 97ea6d04f..e161c1703 100644 --- a/app/src/androidTest/java/org/mozilla/fenix/ui/BookmarksTest.kt +++ b/app/src/androidTest/java/org/mozilla/fenix/ui/BookmarksTest.kt @@ -445,8 +445,14 @@ class BookmarksTest { getInstrumentation().waitForIdleSync() }.openThreeDotMenu("1") { }.clickDelete { + verifyDeleteFolderConfirmationMessage() + confirmFolderDeletion() + verifyDeleteSnackBarText() }.openThreeDotMenu("2") { }.clickDelete { + verifyDeleteFolderConfirmationMessage() + confirmFolderDeletion() + verifyDeleteSnackBarText() verifyFolderTitle("3") }.goBack { } diff --git a/app/src/androidTest/java/org/mozilla/fenix/ui/robots/BookmarksRobot.kt b/app/src/androidTest/java/org/mozilla/fenix/ui/robots/BookmarksRobot.kt index 9f8d250c7..d01d76933 100644 --- a/app/src/androidTest/java/org/mozilla/fenix/ui/robots/BookmarksRobot.kt +++ b/app/src/androidTest/java/org/mozilla/fenix/ui/robots/BookmarksRobot.kt @@ -13,6 +13,7 @@ import androidx.test.espresso.action.ViewActions.longClick import androidx.test.espresso.action.ViewActions.typeText import androidx.test.espresso.assertion.ViewAssertions.doesNotExist import androidx.test.espresso.assertion.ViewAssertions.matches +import androidx.test.espresso.matcher.RootMatchers import androidx.test.espresso.matcher.ViewMatchers import androidx.test.espresso.matcher.ViewMatchers.isDisplayed import androidx.test.espresso.matcher.ViewMatchers.withChild @@ -113,6 +114,8 @@ class BookmarksRobot { fun verifySignInToSyncButton() = signInToSyncButton().check(matches(isDisplayed())) + fun verifyDeleteFolderConfirmationMessage() = assertDeleteFolderConfirmationMessage() + fun createFolder(name: String) { clickAddFolderButton() addNewFolderName(name) @@ -169,6 +172,13 @@ class BookmarksRobot { fun longTapDesktopFolder(title: String) = onView(withText(title)).perform(longClick()) + fun confirmFolderDeletion() { + onView(withText(R.string.delete_browsing_data_prompt_allow)) + .inRoot(RootMatchers.isDialog()) + .check(matches(isDisplayed())) + .click() + } + class Transition { fun goBack(interact: HomeScreenRobot.() -> Unit): HomeScreenRobot.Transition { goBackButton().click() @@ -335,3 +345,8 @@ private fun assertShareBookmarkFavicon() = private fun assertShareBookmarkUrl() = onView(withId(R.id.share_tab_url)).check(matches(isDisplayed())) + +private fun assertDeleteFolderConfirmationMessage() = + onView(withText(R.string.bookmark_delete_folder_confirmation_dialog)) + .inRoot(RootMatchers.isDialog()) + .check(matches(isDisplayed())) diff --git a/app/src/main/java/org/mozilla/fenix/browser/BaseBrowserFragment.kt b/app/src/main/java/org/mozilla/fenix/browser/BaseBrowserFragment.kt index f03e2ba62..d9802b7e4 100644 --- a/app/src/main/java/org/mozilla/fenix/browser/BaseBrowserFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/browser/BaseBrowserFragment.kt @@ -759,7 +759,7 @@ abstract class BaseBrowserFragment : Fragment(), UserInteractionHandler, Session withContext(Main) { nav( R.id.browserFragment, - BrowserFragmentDirections.actionGlobalBookmarkEditFragment(existing.guid) + BrowserFragmentDirections.actionGlobalBookmarkEditFragment(existing.guid, true) ) } } else { @@ -784,7 +784,7 @@ abstract class BaseBrowserFragment : Fragment(), UserInteractionHandler, Session .setAction(getString(R.string.edit_bookmark_snackbar_action)) { nav( R.id.browserFragment, - BrowserFragmentDirections.actionGlobalBookmarkEditFragment(guid) + BrowserFragmentDirections.actionGlobalBookmarkEditFragment(guid, true) ) } .show() 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 2df8de1b5..5a52f148c 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,6 +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 handleBackPressed() } @@ -44,6 +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 invokePendingDeletion: () -> Unit ) : BookmarkController { @@ -92,6 +94,10 @@ class DefaultBookmarkController( deleteBookmarkNodes(nodes, eventType) } + override fun handleBookmarkFolderDeletion(node: BookmarkNode) { + deleteBookmarkFolder(node) + } + override fun handleBackPressed() { invokePendingDeletion.invoke() navController.popBackStack() 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 573a9ac95..d031ab11e 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 @@ -4,6 +4,7 @@ package org.mozilla.fenix.library.bookmarks +import android.content.DialogInterface import android.os.Bundle import android.view.LayoutInflater import android.view.Menu @@ -11,6 +12,7 @@ import android.view.MenuInflater import android.view.MenuItem import android.view.View import android.view.ViewGroup +import androidx.appcompat.app.AlertDialog import androidx.fragment.app.activityViewModels import androidx.lifecycle.ViewModelProvider import androidx.lifecycle.lifecycleScope @@ -42,8 +44,8 @@ import org.mozilla.fenix.ext.bookmarkStorage import org.mozilla.fenix.ext.components import org.mozilla.fenix.ext.minus import org.mozilla.fenix.ext.nav -import org.mozilla.fenix.ext.toShortUrl import org.mozilla.fenix.ext.requireComponents +import org.mozilla.fenix.ext.toShortUrl import org.mozilla.fenix.library.LibraryPageFragment import org.mozilla.fenix.utils.allowUndo @@ -85,6 +87,7 @@ class BookmarkFragment : LibraryPageFragment(), UserInteractionHan navController = findNavController(), showSnackbar = ::showSnackBarWithText, deleteBookmarkNodes = ::deleteMulti, + deleteBookmarkFolder = ::showRemoveFolderDialog, invokePendingDeletion = ::invokePendingDeletion ), metrics = metrics!! @@ -261,24 +264,13 @@ class BookmarkFragment : LibraryPageFragment(), UserInteractionHan } private fun deleteMulti(selected: Set, eventType: Event = Event.RemoveBookmarks) { - pendingBookmarksToDelete.addAll(selected) + updatePendingBookmarksToDelete(selected) - val bookmarkTree = sharedViewModel.selectedFolder!! - pendingBookmarksToDelete - bookmarkInteractor.onBookmarksChanged(bookmarkTree) - - val deleteOperation: (suspend () -> Unit) = { - deleteSelectedBookmarks(pendingBookmarksToDelete) - pendingBookmarkDeletionJob = null - // Since this runs in a coroutine, we can't depend upon the fragment still being attached - metrics?.track(Event.RemoveBookmarks) - refreshBookmarks() - } - - pendingBookmarkDeletionJob = deleteOperation + pendingBookmarkDeletionJob = getDeleteOperation(eventType) val message = when (eventType) { is Event.RemoveBookmarks -> { - getString(R.string.bookmark_deletion_multiple_snackbar_message) + getRemoveBookmarksSnackBarMessage(selected) } is Event.RemoveBookmarkFolder, is Event.RemoveBookmark -> { @@ -294,18 +286,87 @@ class BookmarkFragment : LibraryPageFragment(), UserInteractionHan viewLifecycleOwner.lifecycleScope.allowUndo( view!!, message, getString(R.string.bookmark_undo_deletion), { - pendingBookmarksToDelete.removeAll(selected) - pendingBookmarkDeletionJob = null - refreshBookmarks() - }, operation = deleteOperation + undoPendingDeletion(selected) + }, operation = getDeleteOperation(eventType) ) } + private fun getRemoveBookmarksSnackBarMessage(selected: Set): String { + return if (selected.size > 1) { + getString(R.string.bookmark_deletion_multiple_snackbar_message) + } else { + val bookmarkNode = selected.first() + getString( + R.string.bookmark_deletion_snackbar_message, + bookmarkNode.url?.toShortUrl(context!!.components.publicSuffixList) + ?: bookmarkNode.title + ) + } + } + override fun onDestroyView() { super.onDestroyView() _bookmarkInteractor = null } + private fun showRemoveFolderDialog(selected: BookmarkNode) { + activity?.let { activity -> + AlertDialog.Builder(activity).apply { + setMessage(R.string.bookmark_delete_folder_confirmation_dialog) + 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)) + pendingBookmarkDeletionJob = getDeleteOperation(Event.RemoveBookmarkFolder) + dialog.dismiss() + val message = getDeleteDialogString(selected) + lifecycleScope.allowUndo( + view!!, + message, + getString(R.string.bookmark_undo_deletion), + { + undoPendingDeletion(setOf(selected)) + }, + operation = getDeleteOperation(Event.RemoveBookmarkFolder) + ) + } + create() + } + .show() + } + } + + private fun updatePendingBookmarksToDelete(selected: Set) { + pendingBookmarksToDelete.addAll(selected) + val bookmarkTree = sharedViewModel.selectedFolder!! - pendingBookmarksToDelete + 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 + refreshBookmarks() + } + + private fun getDeleteOperation(event: Event): (suspend () -> Unit) { + return { + deleteSelectedBookmarks(pendingBookmarksToDelete) + pendingBookmarkDeletionJob = null + // Since this runs in a coroutine, we can't depend upon the fragment still being attached + metrics?.track(event) + refreshBookmarks() + } + } + private fun invokePendingDeletion() { pendingBookmarkDeletionJob?.let { viewLifecycleOwner.lifecycleScope.launch { 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 f6676f23c..e8fac64b4 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 @@ -89,7 +89,11 @@ class BookmarkFragmentInteractor( BookmarkNodeType.FOLDER -> Event.RemoveBookmarkFolder null -> Event.RemoveBookmarks } - bookmarksController.handleBookmarkDeletion(nodes, eventType) + if (eventType == Event.RemoveBookmarkFolder) { + bookmarksController.handleBookmarkFolderDeletion(nodes.first()) + } else { + bookmarksController.handleBookmarkDeletion(nodes, eventType) + } } override fun onBackPressed() { diff --git a/app/src/main/java/org/mozilla/fenix/library/bookmarks/edit/EditBookmarkFragment.kt b/app/src/main/java/org/mozilla/fenix/library/bookmarks/edit/EditBookmarkFragment.kt index 6df216315..494887489 100644 --- a/app/src/main/java/org/mozilla/fenix/library/bookmarks/edit/EditBookmarkFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/library/bookmarks/edit/EditBookmarkFragment.kt @@ -166,7 +166,7 @@ class EditBookmarkFragment : Fragment(R.layout.fragment_edit_bookmark) { bookmarkNode?.let { bookmark -> FenixSnackbar.make( view = activity.getRootView()!!, - isDisplayedWithBrowserToolbar = true + isDisplayedWithBrowserToolbar = args.requiresSnackbarPaddingForToolbar ) .setText( getString( 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 83c806818..760c195c4 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 @@ -28,6 +28,7 @@ import mozilla.components.support.base.feature.UserInteractionHandler import org.mozilla.fenix.BrowserDirection 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 @@ -38,6 +39,7 @@ import org.mozilla.fenix.ext.components 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 @SuppressWarnings("TooManyFunctions", "LargeClass") @@ -106,6 +108,7 @@ class HistoryFragment : LibraryPageFragment(), UserInteractionHandl } private fun deleteHistoryItems(items: Set) { + val message = getMultiSelectSnackBarMessage(items) viewLifecycleOwner.lifecycleScope.launch { context?.components?.run { for (item in items) { @@ -114,6 +117,7 @@ class HistoryFragment : LibraryPageFragment(), UserInteractionHandl } } viewModel.invalidate() + showSnackBar(view!!, message) historyStore.dispatch(HistoryFragmentAction.ExitDeletionMode) } } @@ -155,10 +159,12 @@ 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) } true } @@ -193,6 +199,17 @@ class HistoryFragment : LibraryPageFragment(), UserInteractionHandl else -> super.onOptionsItemSelected(item) } + private fun getMultiSelectSnackBarMessage(historyItems: Set): String { + return if (historyItems.size > 1) { + getString(R.string.history_delete_multiple_items_snackbar) + } else { + getString( + R.string.history_delete_single_item_snackbar, + historyItems.first().url.toShortUrl(requireComponents.publicSuffixList) + ) + } + } + override fun onBackPressed(): Boolean = historyView.onBackPressed() private fun openItem(item: HistoryItem, mode: BrowsingMode? = null) { @@ -222,6 +239,7 @@ class HistoryFragment : LibraryPageFragment(), UserInteractionHandl launch(Main) { viewModel.invalidate() historyStore.dispatch(HistoryFragmentAction.ExitDeletionMode) + showSnackBar(requireView(), getString(R.string.preferences_delete_browsing_data_snackbar)) } } diff --git a/app/src/main/res/navigation/nav_graph.xml b/app/src/main/res/navigation/nav_graph.xml index 7384d02d9..8fc791344 100644 --- a/app/src/main/res/navigation/nav_graph.xml +++ b/app/src/main/res/navigation/nav_graph.xml @@ -241,6 +241,10 @@ + Delete history Are you sure you want to clear your history? + + History Deleted + + Deleted %1$s Clear @@ -520,6 +524,10 @@ Edit bookmark Select folder + + Are you sure you want to delete this folder? + + Deleted %1$s Add folder @@ -573,7 +581,7 @@ The first parameter is the host part of the URL of the bookmark deleted, if any --> Deleted %1$s - Deleting selected bookmarks + Bookmarks deleted UNDO diff --git a/app/src/test/java/org/mozilla/fenix/crashes/CrashReporterControllerTest.kt b/app/src/test/java/org/mozilla/fenix/crashes/CrashReporterControllerTest.kt index 7d3c78b11..d3ac958f6 100644 --- a/app/src/test/java/org/mozilla/fenix/crashes/CrashReporterControllerTest.kt +++ b/app/src/test/java/org/mozilla/fenix/crashes/CrashReporterControllerTest.kt @@ -58,7 +58,7 @@ class CrashReporterControllerTest { } @Test - fun `"handle close and remove tab`() { + fun `handle close and remove tab`() { val controller = CrashReporterController(crash, session, navContoller, components, settings) controller.handleCloseAndRemove(sendCrash = false)?.joinBlocking() 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 f79bc853c..71cf57e16 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,6 +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 invokePendingDeletion: () -> Unit = mockk(relaxed = true) private val homeActivity: HomeActivity = mockk(relaxed = true) @@ -92,6 +93,7 @@ class BookmarkControllerTest { navController = navController, showSnackbar = showSnackbar, deleteBookmarkNodes = deleteBookmarkNodes, + deleteBookmarkFolder = deleteBookmarkFolder, invokePendingDeletion = invokePendingDeletion ) } @@ -237,11 +239,11 @@ class BookmarkControllerTest { } @Test - fun `handleBookmarkDeletion for a folder should properly call a passed in delegate`() { - controller.handleBookmarkDeletion(setOf(subfolder), Event.RemoveBookmarkFolder) + fun `handleBookmarkDeletion for a folder should properly call the delete folder delegate`() { + controller.handleBookmarkFolderDeletion(subfolder) verify { - deleteBookmarkNodes(setOf(subfolder), Event.RemoveBookmarkFolder) + deleteBookmarkFolder(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 b389f74f2..c0129e961 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.handleBookmarkDeletion(setOf(subfolder), Event.RemoveBookmarkFolder) + bookmarkController.handleBookmarkFolderDeletion(subfolder) } }