From 20b46d1d14eace4820ccabe3f0edc54769dbad73 Mon Sep 17 00:00:00 2001 From: Victor Andreasson Date: Sat, 22 Jun 2024 17:23:35 +0200 Subject: [PATCH] Allow selecting multiple notebooks In this solution, there are some differences in the top "app bar" menu choices when one vs many notebooks are selected: - The "export" and "rename" choices are removed when multiple notebooks are selected. - When setting the repository link of multiple notebooks, no repository is pre-selected, even if all notebooks are currently linked to the same repository. The rationale is that I found no good way of showing existing links to multiple repos, so it seemed better to me to never make a pre-selection. - When deleting a single notebook, its repository URL is shown in the dialog prompting whether to delete the linked repository file. However, wwhen deleting multiple notebooks, no repository URLs are shown. The rationale is that I found no intuitive way of presenting multiple URLs in the dialog along with their corresponding notebooks. --- .../orgzly/android/espresso/BooksTest.java | 109 ++++++++++++++++++ .../orgzly/android/espresso/SyncingTest.java | 13 +-- .../android/espresso/util/EspressoUtils.java | 12 ++ .../orgzly/android/ui/books/BooksFragment.kt | 68 +++++++---- .../orgzly/android/ui/books/BooksViewModel.kt | 67 ++++++----- .../main/res/layout/dialog_book_delete.xml | 2 +- app/src/main/res/values/strings.xml | 10 +- 7 files changed, 210 insertions(+), 71 deletions(-) diff --git a/app/src/androidTest/java/com/orgzly/android/espresso/BooksTest.java b/app/src/androidTest/java/com/orgzly/android/espresso/BooksTest.java index 04d1b75b6..d23d4dd42 100644 --- a/app/src/androidTest/java/com/orgzly/android/espresso/BooksTest.java +++ b/app/src/androidTest/java/com/orgzly/android/espresso/BooksTest.java @@ -11,8 +11,12 @@ import static androidx.test.espresso.intent.Intents.intending; import static androidx.test.espresso.intent.matcher.IntentMatchers.hasAction; import static androidx.test.espresso.intent.matcher.IntentMatchers.hasExtra; +import static androidx.test.espresso.matcher.ViewMatchers.hasChildCount; +import static androidx.test.espresso.matcher.ViewMatchers.isChecked; import static androidx.test.espresso.matcher.ViewMatchers.isDisplayed; import static androidx.test.espresso.matcher.ViewMatchers.isEnabled; +import static androidx.test.espresso.matcher.ViewMatchers.isNotChecked; +import static androidx.test.espresso.matcher.ViewMatchers.withClassName; import static androidx.test.espresso.matcher.ViewMatchers.withId; import static androidx.test.espresso.matcher.ViewMatchers.withText; import static com.orgzly.android.espresso.util.EspressoUtils.contextualToolbarOverflowMenu; @@ -21,7 +25,9 @@ import static com.orgzly.android.espresso.util.EspressoUtils.onNoteInBook; import static com.orgzly.android.espresso.util.EspressoUtils.onSnackbar; import static com.orgzly.android.espresso.util.EspressoUtils.replaceTextCloseKeyboard; +import static com.orgzly.android.espresso.util.EspressoUtils.sync; import static org.hamcrest.Matchers.allOf; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.endsWith; import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.startsWith; @@ -41,6 +47,7 @@ import com.orgzly.R; import com.orgzly.android.BookFormat; import com.orgzly.android.OrgzlyTest; +import com.orgzly.android.repos.RepoType; import com.orgzly.android.ui.main.MainActivity; import org.junit.Before; @@ -299,4 +306,106 @@ public void testBackPressClosesSelectionMenu() { // Make sure we're still in the app onBook(0, R.id.item_book_title).check(matches(withText("book-1"))); } + + @Test + public void testSetLinkOnSingleBookCurrentRepoIsSelected() { + testUtils.setupRepo(RepoType.MOCK, "mock://repo"); + sync(); + onBook(0, R.id.item_book_link_repo).check(matches(withText("mock://repo"))); + onBook(0).perform(longClick()); + contextualToolbarOverflowMenu().perform(click()); + onView(withText(R.string.books_context_menu_item_set_link)).perform(click()); + onView(withText("mock://repo")).check(matches(isChecked())); + } + + /** + * When setting the link of multiple books, no repo should be pre-selected, + * no matter how many repos there are, and no matter whether the books + * already have a link or not. The reason for this is that we have no + * intuitive way of displaying links to multiple repos. + */ + @Test + public void testSetLinkOnMultipleBooksNoRepoIsSelected() { + testUtils.setupRepo(RepoType.MOCK, "mock://repo"); + sync(); + onBook(0, R.id.item_book_link_repo).check(matches(withText("mock://repo"))); + onBook(1, R.id.item_book_link_repo).check(matches(withText("mock://repo"))); + onBook(0).perform(longClick()); + onBook(1).perform(click()); + contextualToolbarOverflowMenu().perform(click()); + onView(withText(R.string.books_context_menu_item_set_link)).perform(click()); + onView(withText("mock://repo")).check(matches(isNotChecked())); + } + + @Test + public void testDeleteSingleBookLinkedUrlIsShown() { + testUtils.setupRepo(RepoType.MOCK, "mock://repo"); + sync(); + onBook(0, R.id.item_book_link_repo).check(matches(withText("mock://repo"))); + onBook(0).perform(longClick()); + contextualToolbarOverflowMenu().perform(click()); + onView(withText(R.string.delete)).perform(click()); + onView(withText(R.string.also_delete_linked_book)).check(matches(isDisplayed())); + onView(withId(R.id.delete_linked_url)).check(matches(withText("mock://repo/book-1.org"))); + } + + @Test + public void testDeleteMultipleBooksLinkedUrlIsNotShown() { + testUtils.setupRepo(RepoType.MOCK, "mock://repo"); + sync(); + onBook(0, R.id.item_book_link_repo).check(matches(withText("mock://repo"))); + onBook(1, R.id.item_book_link_repo).check(matches(withText("mock://repo"))); + onBook(0).perform(longClick()); + onBook(1).perform(click()); + contextualToolbarOverflowMenu().perform(click()); + onView(withText(R.string.delete)).perform(click()); + onView(withText(R.string.also_delete_linked_books)).check(matches(isDisplayed())); + onView(withId(R.id.delete_linked_url)).check(matches(withText(""))); + } + + @Test + public void testDeleteMultipleBooksWithNoLinks() { + onBook(0).perform(longClick()); + onBook(1).perform(click()); + contextualToolbarOverflowMenu().perform(click()); + onView(withText(R.string.delete)).perform(click()); + onView(withText(R.string.delete)).perform(click()); + assert dataRepository.getBooks().size() == 1; + } + + @Test + public void testDeleteMultipleBooksAndRooks() { + testUtils.setupRepo(RepoType.MOCK, "mock://repo"); + sync(); + onBook(0, R.id.item_book_link_repo).check(matches(withText("mock://repo"))); + onBook(1, R.id.item_book_link_repo).check(matches(withText("mock://repo"))); + onBook(0).perform(longClick()); + onBook(1).perform(click()); + contextualToolbarOverflowMenu().perform(click()); + onView(withText(R.string.delete)).perform(click()); + onView(withId(R.id.delete_linked_checkbox)).perform(click()); + onView(withText(R.string.delete)).perform(click()); + assert dataRepository.getBooks().size() == 1; + } + + /** + * When multiple books are selected, the "rename" and "export" actions should be removed from + * the context menu. By also testing that only the expected number of actions are shown, we + * protect against someone later adding actions to the menu without fully considering the support for + * multiple selected books. When such support is added, this test will need to be updated. + */ + @Test + public void testMultipleBooksSelectedContextMenuShowsSupportedActionsOnly() { + onBook(0).perform(longClick()); + contextualToolbarOverflowMenu().perform(click()); + onView(withText(R.string.rename)).check(matches(isDisplayed())); + onView(withText(R.string.export)).check(matches(isDisplayed())); + onView(withClassName(containsString("MenuDropDownListView"))).check(matches(hasChildCount(4))); + pressBack(); + onBook(1).perform(click()); + contextualToolbarOverflowMenu().perform(click()); + onView(withText(R.string.rename)).check(doesNotExist()); + onView(withText(R.string.export)).check(doesNotExist()); + onView(withClassName(containsString("MenuDropDownListView"))).check(matches(hasChildCount(2))); + } } diff --git a/app/src/androidTest/java/com/orgzly/android/espresso/SyncingTest.java b/app/src/androidTest/java/com/orgzly/android/espresso/SyncingTest.java index eecbd1d24..d8ad9b7db 100644 --- a/app/src/androidTest/java/com/orgzly/android/espresso/SyncingTest.java +++ b/app/src/androidTest/java/com/orgzly/android/espresso/SyncingTest.java @@ -5,7 +5,6 @@ import static androidx.test.espresso.action.ViewActions.click; import static androidx.test.espresso.action.ViewActions.longClick; import static androidx.test.espresso.assertion.ViewAssertions.matches; -import static androidx.test.espresso.contrib.DrawerActions.close; import static androidx.test.espresso.contrib.DrawerActions.open; import static androidx.test.espresso.matcher.ViewMatchers.isDescendantOfA; import static androidx.test.espresso.matcher.ViewMatchers.isDisplayed; @@ -14,7 +13,6 @@ import static androidx.test.espresso.matcher.ViewMatchers.withText; import static com.orgzly.android.espresso.util.EspressoUtils.clickSetting; import static com.orgzly.android.espresso.util.EspressoUtils.contextualToolbarOverflowMenu; -import static com.orgzly.android.espresso.util.EspressoUtils.grantAlarmsAndRemindersPermission; import static com.orgzly.android.espresso.util.EspressoUtils.onActionItemClick; import static com.orgzly.android.espresso.util.EspressoUtils.onBook; import static com.orgzly.android.espresso.util.EspressoUtils.onListItem; @@ -24,6 +22,7 @@ import static com.orgzly.android.espresso.util.EspressoUtils.recyclerViewItemCount; import static com.orgzly.android.espresso.util.EspressoUtils.replaceTextCloseKeyboard; import static com.orgzly.android.espresso.util.EspressoUtils.settingsSetTodoKeywords; +import static com.orgzly.android.espresso.util.EspressoUtils.sync; import static org.hamcrest.Matchers.allOf; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.endsWith; @@ -69,16 +68,6 @@ public void tearDown() throws Exception { } } - /** - * Utility method for starting sync using drawer button. - */ - private void sync() { - grantAlarmsAndRemindersPermission(); - onView(withId(R.id.drawer_layout)).perform(open()); - onView(withId(R.id.sync_button_container)).perform(click()); - onView(withId(R.id.drawer_layout)).perform(close()); - } - @Test public void testRunSync() { scenario = ActivityScenario.launch(MainActivity.class); diff --git a/app/src/androidTest/java/com/orgzly/android/espresso/util/EspressoUtils.java b/app/src/androidTest/java/com/orgzly/android/espresso/util/EspressoUtils.java index 66141e453..cc401eaba 100644 --- a/app/src/androidTest/java/com/orgzly/android/espresso/util/EspressoUtils.java +++ b/app/src/androidTest/java/com/orgzly/android/espresso/util/EspressoUtils.java @@ -7,6 +7,8 @@ import static androidx.test.espresso.action.ViewActions.click; import static androidx.test.espresso.action.ViewActions.pressKey; import static androidx.test.espresso.action.ViewActions.replaceText; +import static androidx.test.espresso.contrib.DrawerActions.close; +import static androidx.test.espresso.contrib.DrawerActions.open; import static androidx.test.espresso.matcher.ViewMatchers.hasDescendant; import static androidx.test.espresso.matcher.ViewMatchers.isAssignableFrom; import static androidx.test.espresso.matcher.ViewMatchers.isDisplayed; @@ -512,4 +514,14 @@ public static void grantAlarmsAndRemindersPermission() { getInstrumentation().getUiAutomation().executeShellCommand(shellCmd); } } + + /** + * Utility method for starting sync using drawer button. + */ + public static void sync() { + grantAlarmsAndRemindersPermission(); + onView(withId(R.id.drawer_layout)).perform(open()); + onView(withId(R.id.sync_button_container)).perform(click()); + onView(withId(R.id.drawer_layout)).perform(close()); + } } diff --git a/app/src/main/java/com/orgzly/android/ui/books/BooksFragment.kt b/app/src/main/java/com/orgzly/android/ui/books/BooksFragment.kt index c10abc9ff..b025568dd 100644 --- a/app/src/main/java/com/orgzly/android/ui/books/BooksFragment.kt +++ b/app/src/main/java/com/orgzly/android/ui/books/BooksFragment.kt @@ -10,6 +10,7 @@ import android.text.TextUtils import android.text.TextWatcher import android.util.Log import android.view.LayoutInflater +import android.view.Menu import android.view.View import android.view.ViewGroup import androidx.activity.OnBackPressedCallback @@ -155,7 +156,7 @@ class BooksFragment : CommonFragment(), DrawerItem, OnViewHolderClickListener - val bookId = viewAdapter.getSelection().getOnly() + val bookIds = viewAdapter.getSelection().getIds() - if (bookId == null) { + if (bookIds.isEmpty()) { Log.e(TAG, "Cannot handle action when there are no items selected") return@setOnMenuItemClickListener true } when (menuItem.itemId) { R.id.books_context_menu_rename -> { - viewModel.renameBookRequest(bookId) + // N.B. Menu item is hidden when multiple books are selected + viewModel.renameBookRequest(bookIds.first()) } R.id.books_context_menu_set_link -> { - viewModel.setBookLinkRequest(bookId) + viewModel.setBookLinksRequest(bookIds) } R.id.books_context_menu_force_save -> { @@ -254,7 +257,7 @@ class BooksFragment : CommonFragment(), DrawerItem, OnViewHolderClickListener - viewModel.forceSaveBookRequest(bookId) + viewModel.forceSaveBookRequest(bookIds) } .setNegativeButton(R.string.cancel, null) .show() @@ -265,18 +268,19 @@ class BooksFragment : CommonFragment(), DrawerItem, OnViewHolderClickListener - viewModel.forceLoadBookRequest(bookId) + viewModel.forceLoadBookRequest(bookIds) } .setNegativeButton(R.string.cancel, null) .show() } R.id.books_context_menu_export -> { - viewModel.exportBookRequest(bookId, BookFormat.ORG) + // N.B. Menu item is hidden when multiple books are selected + viewModel.exportBookRequest(bookIds.first(), BookFormat.ORG) } R.id.books_context_menu_delete -> { - viewModel.deleteBookRequest(bookId) + viewModel.deleteBooksRequest(bookIds) } } @@ -291,6 +295,13 @@ class BooksFragment : CommonFragment(), DrawerItem, OnViewHolderClickListener if (uri != null) { @@ -305,8 +316,17 @@ class BooksFragment : CommonFragment(), DrawerItem, OnViewHolderClickListener) { val dialogBinding = DialogBookDeleteBinding.inflate(LayoutInflater.from(context)) + val dialogTitle: String + val book: BookView? + if (books.size == 1) { + book = books.first() + dialogTitle = getString(R.string.delete_with_quoted_argument, book.book.name) + } else { + book = null + dialogTitle = getString(R.string.delete_amount_of_books, books.size) + } dialogBinding.deleteLinkedCheckbox.setOnCheckedChangeListener { _, isChecked -> dialogBinding.deleteLinkedUrl.isEnabled = isChecked @@ -316,20 +336,23 @@ class BooksFragment : CommonFragment(), DrawerItem, OnViewHolderClickListener { val deleteLinked = dialogBinding.deleteLinkedCheckbox.isChecked - viewModel.deleteBook(book.book.id, deleteLinked) + for (book in books) { + viewModel.deleteBook(book.book.id, deleteLinked) + } } } } val builder = MaterialAlertDialogBuilder(requireContext()) - .setTitle(getString(R.string.delete_with_quoted_argument, book.book.name)) + .setTitle(dialogTitle) .setPositiveButton(R.string.delete, dialogClickListener) .setNegativeButton(R.string.cancel, dialogClickListener) - if (book.syncedTo != null) { + if (book?.syncedTo != null) { dialogBinding.deleteLinkedUrl.text = book.syncedTo.uri.toString() - builder.setView(dialogBinding.root) + dialogBinding.deleteLinkedCheckbox.text = getString(R.string.also_delete_linked_book) } + builder.setView(dialogBinding.root) dialog = builder.show() } @@ -421,9 +444,9 @@ class BooksFragment : CommonFragment(), DrawerItem, OnViewHolderClickListener - if (bookView != null) { - deleteBookDialog(bookView) + viewModel.booksToDeleteEvent.observeSingle(viewLifecycleOwner, Observer { bookViews -> + if (bookViews.isNotEmpty()) { + deleteBooksDialog(bookViews) } }) @@ -445,7 +468,7 @@ class BooksFragment : CommonFragment(), DrawerItem, OnViewHolderClickListener + viewModel.setBookLinkRequestEvent.observeSingle(this) { (bookIds, links, urls, checked) -> if (links.isEmpty()) { activity?.showSnackbar(getString(R.string.no_repos), R.string.repositories) { activity?.let { @@ -454,7 +477,6 @@ class BooksFragment : CommonFragment(), DrawerItem, OnViewHolderClickListener - viewModel.setBookLink(book.id, links[which]) + viewModel.setBookLinks(bookIds, links[which]) dialog?.dismiss() dialog = null } - .setNeutralButton(R.string.remove_notebook_link) { dialog, which -> - viewModel.setBookLink(book.id) + .setNeutralButton(R.string.remove_notebook_link) { _, _ -> + viewModel.setBookLinks(bookIds) } .setNegativeButton(R.string.cancel, null) .show() diff --git a/app/src/main/java/com/orgzly/android/ui/books/BooksViewModel.kt b/app/src/main/java/com/orgzly/android/ui/books/BooksViewModel.kt index 7a45cc552..3046c4d3d 100644 --- a/app/src/main/java/com/orgzly/android/ui/books/BooksViewModel.kt +++ b/app/src/main/java/com/orgzly/android/ui/books/BooksViewModel.kt @@ -15,7 +15,6 @@ import com.orgzly.android.ui.CommonViewModel import com.orgzly.android.ui.SingleLiveEvent import com.orgzly.android.usecase.* import com.orgzly.android.util.LogUtils -import java.io.File class BooksViewModel(private val dataRepository: DataRepository) : CommonViewModel() { @@ -24,7 +23,7 @@ class BooksViewModel(private val dataRepository: DataRepository) : CommonViewMod // Book being operated on (deleted, renamed, etc.) private var lastBook = MutableLiveData>() - val bookToDeleteEvent: SingleLiveEvent = SingleLiveEvent() + val booksToDeleteEvent: SingleLiveEvent> = SingleLiveEvent() val bookDeletedEvent: SingleLiveEvent = SingleLiveEvent() val bookToRenameEvent: SingleLiveEvent = SingleLiveEvent() val bookToExportEvent: SingleLiveEvent> = SingleLiveEvent() @@ -64,9 +63,10 @@ class BooksViewModel(private val dataRepository: DataRepository) : CommonViewMod } } - fun deleteBookRequest(bookId: Long) { + fun deleteBooksRequest(bookIds: Set) { + val bookViews = bookIds.map { requireNotNull(dataRepository.getBookView(it)) }.toSet() App.EXECUTORS.diskIO().execute { - bookToDeleteEvent.postValue(dataRepository.getBookView(bookId)) + booksToDeleteEvent.postValue(bookViews) } } @@ -94,29 +94,28 @@ class BooksViewModel(private val dataRepository: DataRepository) : CommonViewMod } data class BookLinkOptions( - val book: Book, val links: List, val urls: List, val selected: Int) + val bookIds: Set, val links: List, val urls: List, val selected: Int) - fun setBookLinkRequest(bookId: Long) { + fun setBookLinksRequest(bookIds: Set) { App.EXECUTORS.diskIO().execute { - val bookView = dataRepository.getBookView(bookId) - - if (bookView == null) { - errorEvent.postValue(Throwable("Book not found")) - + if (bookIds.isEmpty()) { + errorEvent.postValue(Throwable("No books found")) } else { val repos = dataRepository.getRepos() val options = if (repos.isEmpty()) { - BookLinkOptions(bookView.book, emptyList(), emptyList(), -1) - + BookLinkOptions(bookIds, emptyList(), emptyList(), -1) } else { - val currentLink = bookView.linkRepo - - val selectedLink = repos.indexOfFirst { - it.url == currentLink?.url + if (bookIds.size == 1) { + val bookView = dataRepository.getBookView(bookIds.first()) + val currentLink = bookView?.linkRepo + val selectedLink = repos.indexOfFirst { + it.url == currentLink?.url + } + BookLinkOptions(bookIds, repos, repos.map { it.url }, selectedLink) + } else { + BookLinkOptions(bookIds, repos, repos.map { it.url }, -1) } - - BookLinkOptions(bookView.book, repos, repos.map { it.url }, selectedLink) } setBookLinkRequestEvent.postValue(options) @@ -124,26 +123,32 @@ class BooksViewModel(private val dataRepository: DataRepository) : CommonViewMod } } - fun setBookLink(bookId: Long, repo: Repo? = null) { - App.EXECUTORS.diskIO().execute { - catchAndPostError { - UseCaseRunner.run(BookLinkUpdate(bookId, repo)) + fun setBookLinks(bookIds: Set, repo: Repo? = null) { + for (bookId in bookIds) { + App.EXECUTORS.diskIO().execute { + catchAndPostError { + UseCaseRunner.run(BookLinkUpdate(bookId, repo)) + } } } } - fun forceSaveBookRequest(bookId: Long) { - App.EXECUTORS.diskIO().execute { - catchAndPostError { - UseCaseRunner.run(BookForceSave(bookId)) + fun forceSaveBookRequest(bookIds: Set) { + for (bookId in bookIds) { + App.EXECUTORS.diskIO().execute { + catchAndPostError { + UseCaseRunner.run(BookForceSave(bookId)) + } } } } - fun forceLoadBookRequest(bookId: Long) { - App.EXECUTORS.diskIO().execute { - catchAndPostError { - UseCaseRunner.run(BookForceLoad(bookId)) + fun forceLoadBookRequest(bookIds: Set) { + for (bookId in bookIds) { + App.EXECUTORS.diskIO().execute { + catchAndPostError { + UseCaseRunner.run(BookForceLoad(bookId)) + } } } } diff --git a/app/src/main/res/layout/dialog_book_delete.xml b/app/src/main/res/layout/dialog_book_delete.xml index db9a77214..d5e9acbcd 100644 --- a/app/src/main/res/layout/dialog_book_delete.xml +++ b/app/src/main/res/layout/dialog_book_delete.xml @@ -14,7 +14,7 @@ android:layout_height="wrap_content" android:layout_gravity="start|center_vertical" android:gravity="start|center_vertical" - android:text="@string/also_delete_linked_book" + android:text="@string/also_delete_linked_books" android:paddingStart="8dp" android:paddingEnd="8dp" /> diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 8e66dfab4..eaf761f93 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -24,6 +24,7 @@ Edit note Delete Delete “%s” + Delete %d notebooks Name Query Done @@ -290,8 +291,9 @@ Renamed from “%s” Delete linked remote notebook - Deleting notebook failed: %s - Notebook deleted + Delete any linked remote notebooks + Deleting notebook(s) failed: %s + Notebook(s) deleted Priority %s Default priority @@ -614,8 +616,8 @@ Notifications Sound, vibrate, notification dot Hide metadata - Overwrite local notebook? - Overwrite remote notebook? + Overwrite local notebook(s)? + Overwrite remote notebook(s)? Drawers folded Fold drawers by default Log to drawer on time shift