Skip to content

Commit

Permalink
Allow selecting multiple notebooks
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@amberin
amberin committed Jul 2, 2024
1 parent ce16fca commit a5708b0
Show file tree
Hide file tree
Showing 7 changed files with 210 additions and 69 deletions.
109 changes: 109 additions & 0 deletions app/src/androidTest/java/com/orgzly/android/espresso/BooksTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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)));
}
}
11 changes: 1 addition & 10 deletions app/src/androidTest/java/com/orgzly/android/espresso/SyncingTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -23,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;
Expand Down Expand Up @@ -69,15 +69,6 @@ public void tearDown() throws Exception {
}
}

/**
* Utility method for starting sync using drawer button.
*/
private void sync() {
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);
Expand Down
12 changes: 12 additions & 0 deletions app/src/androidTest/java/com/orgzly/android/espresso/util/EspressoUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -513,4 +515,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());
}
}
68 changes: 45 additions & 23 deletions app/src/main/java/com/orgzly/android/ui/books/BooksFragment.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -155,7 +156,7 @@ class BooksFragment : CommonFragment(), DrawerItem, OnViewHolderClickListener<Bo

} else {
// There are books selected
viewAdapter.getSelection().toggleSingleSelect(item.book.id)
viewAdapter.getSelection().toggle(item.book.id)
viewAdapter.notifyDataSetChanged() // FIXME

viewModel.appBar.toModeFromSelectionCount(viewAdapter.getSelection().count)
Expand All @@ -168,7 +169,7 @@ class BooksFragment : CommonFragment(), DrawerItem, OnViewHolderClickListener<Bo
return
}

viewAdapter.getSelection().toggleSingleSelect(item.book.id)
viewAdapter.getSelection().toggle(item.book.id)
viewAdapter.notifyDataSetChanged() // FIXME

viewModel.appBar.toModeFromSelectionCount(viewAdapter.getSelection().count)
Expand Down Expand Up @@ -225,6 +226,7 @@ class BooksFragment : CommonFragment(), DrawerItem, OnViewHolderClickListener<Bo
binding.topToolbar.run {
menu.clear()
inflateMenu(R.menu.books_cab)
hideMenuItemsBasedOnSelection(menu)

setNavigationIcon(R.drawable.ic_arrow_back)

Expand All @@ -233,28 +235,29 @@ class BooksFragment : CommonFragment(), DrawerItem, OnViewHolderClickListener<Bo
}

setOnMenuItemClickListener { menuItem ->
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 -> {
dialog = MaterialAlertDialogBuilder(context)
.setTitle(R.string.books_context_menu_item_force_save)
.setMessage(R.string.overwrite_remote_notebook_question)
.setPositiveButton(R.string.overwrite) { _, _ ->
viewModel.forceSaveBookRequest(bookId)
viewModel.forceSaveBookRequest(bookIds)
}
.setNegativeButton(R.string.cancel, null)
.show()
Expand All @@ -265,18 +268,19 @@ class BooksFragment : CommonFragment(), DrawerItem, OnViewHolderClickListener<Bo
.setTitle(R.string.books_context_menu_item_force_load)
.setMessage(R.string.overwrite_local_notebook_question)
.setPositiveButton(R.string.overwrite) { _, _ ->
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)
}
}

Expand All @@ -291,6 +295,13 @@ class BooksFragment : CommonFragment(), DrawerItem, OnViewHolderClickListener<Bo
}
}

private fun hideMenuItemsBasedOnSelection(menu: Menu) {
// Hide choices which are not applicable when multiple books are selected
for (id in listOf(R.id.books_context_menu_rename, R.id.books_context_menu_export)) {
menu.findItem(id)?.isVisible = viewAdapter.getSelection().count == 1
}
}

private val pickFileForBookExport =
registerForActivityResult(ActivityResultContracts.CreateDocument()) { uri ->
if (uri != null) {
Expand All @@ -305,8 +316,17 @@ class BooksFragment : CommonFragment(), DrawerItem, OnViewHolderClickListener<Bo
pickFileForBookExport.launch(defaultFileName)
}

private fun deleteBookDialog(book: BookView) {
private fun deleteBooksDialog(books: Set<BookView>) {
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
Expand All @@ -316,20 +336,23 @@ class BooksFragment : CommonFragment(), DrawerItem, OnViewHolderClickListener<Bo
when (which) {
DialogInterface.BUTTON_POSITIVE -> {
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()
}
Expand Down Expand Up @@ -421,9 +444,9 @@ class BooksFragment : CommonFragment(), DrawerItem, OnViewHolderClickListener<Bo
})


viewModel.bookToDeleteEvent.observeSingle(viewLifecycleOwner, Observer { bookView ->
if (bookView != null) {
deleteBookDialog(bookView)
viewModel.booksToDeleteEvent.observeSingle(viewLifecycleOwner, Observer { bookViews ->
if (bookViews.isNotEmpty()) {
deleteBooksDialog(bookViews)
}
})

Expand All @@ -445,7 +468,7 @@ class BooksFragment : CommonFragment(), DrawerItem, OnViewHolderClickListener<Bo
activity?.showSnackbar(R.string.message_book_deleted)
})

viewModel.setBookLinkRequestEvent.observeSingle(this) { (book, links, urls, checked) ->
viewModel.setBookLinkRequestEvent.observeSingle(this) { (bookIds, links, urls, checked) ->
if (links.isEmpty()) {
activity?.showSnackbar(getString(R.string.no_repos), R.string.repositories) {
activity?.let {
Expand All @@ -454,20 +477,19 @@ class BooksFragment : CommonFragment(), DrawerItem, OnViewHolderClickListener<Bo
ContextCompat.startActivity(it, intent, null)
}
}

} else {
dialog = MaterialAlertDialogBuilder(requireContext())
.setTitle(R.string.book_link)
.setSingleChoiceItems(
urls.toTypedArray(),
checked
) { _: DialogInterface, which: Int ->
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()
Expand Down
Loading

0 comments on commit a5708b0

Please sign in to comment.