From 399d3284990d0b54a5d5532647633d0f39a53492 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Gonz=C3=A1lez?= Date: Thu, 16 Jan 2025 16:33:41 +0100 Subject: [PATCH] Bookmarks: New toolbar for sorting feature flag (#5452) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Task/Issue URL: https://app.asana.com/0/1174433894299346/1209107275709085/f ### Description This PR adds the new toolbar for the Bookmarks screen under the feature flag Note that the select sort icon doesn’t react to changes yet ### Steps to test this PR _Toolbar functionality_ - [x] Enable “bookmarksSorting” feature flag - [x] Add a 4 bookmarks and open the bookmarks screen - [x] Tap on the add folder icon - [x] Verify Add folder screen opens - [x] Tap on the overflow menu - [x] Verify the new menu appear (Sort options available) - [x] Go back and add another bookmark - [x] Open bookmarks again - [x] Verify Search icon appears - [x] Tap on search icon - [x] Run some queries verify filter still works ### UI changes | New | Old | | ------ | ----- | ![Screenshot_20250110_135355](https://github.com/user-attachments/assets/14adc6e7-1b01-42df-8449-b80f57c73ec1)|![Screenshot_20250110_135419](https://github.com/user-attachments/assets/ed828856-d103-4a48-94d8-f400c7cf9141)| --- .../main/res/drawable/ic_folder_add_24.xml | 29 -- .../common/ui/view/PopupMenuItemView.kt | 42 +++ .../common/ui/view/listitem/DaxListItem.kt | 20 +- .../res/layout/component_popup_menu_item.xml | 6 + .../main/res/layout/view_popup_menu_item.xml | 27 +- .../src/main/res/values/attrs-menu-item.xml | 1 + .../main/res/values/design-system-theming.xml | 1 + .../common-ui/src/main/res/values/widgets.xml | 7 +- saved-sites/saved-sites-impl/build.gradle | 1 + .../BookmarkItemTouchHelperCallback.kt | 4 +- .../bookmarks/BookmarkScreenViewHolders.kt | 119 +++--- .../impl/bookmarks/BookmarksActivity.kt | 343 ++++++++++++++---- .../impl/bookmarks/BookmarksAdapter.kt | 31 +- .../BookmarksNameSortingComparator.kt | 71 ++++ .../impl/bookmarks/BookmarksQueryListener.kt | 2 +- .../impl/bookmarks/BookmarksViewModel.kt | 48 +++ .../impl/store/BookmarksDataStore.kt | 73 ++++ .../impl/store/BookmarksDataStoreModule.kt | 43 +++ .../main/res/drawable/ic_folder_add_24.xml | 17 +- .../main/res/layout/activity_bookmarks.xml | 111 ++++++ .../main/res/layout/popup_bookmarks_menu.xml | 52 +++ .../res/menu/bookmark_folder_popup_menu.xml | 12 +- .../src/main/res/menu/bookmark_popup_menu.xml | 27 ++ .../src/main/res/values/donottranslate.xml | 6 + .../main/res/values/styles-saved-sites.xml | 1 - .../impl/bookmarks/BookmarksViewModelTest.kt | 91 +++++ 26 files changed, 975 insertions(+), 210 deletions(-) delete mode 100644 app/src/main/res/drawable/ic_folder_add_24.xml create mode 100644 saved-sites/saved-sites-impl/src/main/java/com/duckduckgo/savedsites/impl/bookmarks/BookmarksNameSortingComparator.kt create mode 100644 saved-sites/saved-sites-impl/src/main/java/com/duckduckgo/savedsites/impl/store/BookmarksDataStore.kt create mode 100644 saved-sites/saved-sites-impl/src/main/java/com/duckduckgo/savedsites/impl/store/BookmarksDataStoreModule.kt create mode 100644 saved-sites/saved-sites-impl/src/main/res/layout/popup_bookmarks_menu.xml rename app/src/main/res/menu/bookmark_folders_activity_menu.xml => saved-sites/saved-sites-impl/src/main/res/menu/bookmark_folder_popup_menu.xml (75%) create mode 100644 saved-sites/saved-sites-impl/src/main/res/menu/bookmark_popup_menu.xml diff --git a/app/src/main/res/drawable/ic_folder_add_24.xml b/app/src/main/res/drawable/ic_folder_add_24.xml deleted file mode 100644 index 8ca42fe9e288..000000000000 --- a/app/src/main/res/drawable/ic_folder_add_24.xml +++ /dev/null @@ -1,29 +0,0 @@ - - - - - - diff --git a/common/common-ui/src/main/java/com/duckduckgo/common/ui/view/PopupMenuItemView.kt b/common/common-ui/src/main/java/com/duckduckgo/common/ui/view/PopupMenuItemView.kt index 2dff2fa7f0e7..85286f3bbe1e 100644 --- a/common/common-ui/src/main/java/com/duckduckgo/common/ui/view/PopupMenuItemView.kt +++ b/common/common-ui/src/main/java/com/duckduckgo/common/ui/view/PopupMenuItemView.kt @@ -17,8 +17,12 @@ package com.duckduckgo.common.ui.view import android.content.Context +import android.graphics.drawable.Drawable +import android.text.SpannableString +import android.text.style.ForegroundColorSpan import android.util.AttributeSet import android.widget.LinearLayout +import androidx.annotation.DrawableRes import androidx.core.content.ContextCompat import com.duckduckgo.common.ui.view.PopupMenuItemView.PopupMenuItemType.DESTRUCTIVE import com.duckduckgo.common.ui.view.PopupMenuItemView.PopupMenuItemType.PRIMARY @@ -55,6 +59,13 @@ constructor( } setPrimaryTextType(primaryTextType) } + + if (hasValue(R.styleable.PopupMenuItemView_trailingIcon)) { + setTrailingIconDrawable(getDrawable(R.styleable.PopupMenuItemView_trailingIcon)!!) + } else { + setTrailingIconVisibility(false) + } + binding.label.text = getString(R.styleable.PopupMenuItemView_primaryText) ?: "" updateContentDescription() recycle() @@ -66,6 +77,10 @@ constructor( updateContentDescription() } + fun setPrimaryText(label: CharSequence) { + binding.label.text = label + } + fun setPrimaryText(label: () -> String) { binding.label.text = label() updateContentDescription() @@ -82,6 +97,33 @@ constructor( binding.root.contentDescription = binding.label.text } + fun setDisabled() { + val textColorAttr = R.attr.daxColorTextDisabled + val spannable = SpannableString(binding.label.text) + spannable.setSpan(ForegroundColorSpan(binding.root.context.getColorFromAttr(textColorAttr)), 0, spannable.length, 0) + setPrimaryText(spannable) + isEnabled = false + } + + fun setTrailingIconDrawable(drawable: Drawable) { + binding.trailingIcon.setImageDrawable(drawable) + setTrailingIconVisibility(true) + } + + fun setTrailingIconResource(@DrawableRes resource: Int) { + binding.trailingIcon.setImageResource(resource) + setTrailingIconVisibility(true) + } + + /** Sets the leading icon image visibility */ + fun setTrailingIconVisibility(visible: Boolean) { + if (visible) { + binding.trailingIcon.show() + } else { + binding.trailingIcon.gone() + } + } + enum class PopupMenuItemType { PRIMARY, DESTRUCTIVE, diff --git a/common/common-ui/src/main/java/com/duckduckgo/common/ui/view/listitem/DaxListItem.kt b/common/common-ui/src/main/java/com/duckduckgo/common/ui/view/listitem/DaxListItem.kt index af66b996a5c9..5d7fed00ab50 100644 --- a/common/common-ui/src/main/java/com/duckduckgo/common/ui/view/listitem/DaxListItem.kt +++ b/common/common-ui/src/main/java/com/duckduckgo/common/ui/view/listitem/DaxListItem.kt @@ -60,6 +60,21 @@ abstract class DaxListItem( itemContainer.setOnClickListener { onClick() } } + /** Sets the item long click listener */ + fun setLongClickListener(onClick: () -> Unit) { + itemContainer.setOnLongClickListener { + onClick() + true + } + } + + /** Sets the item click listener */ + fun removeLongClickListener() { + itemContainer.setOnLongClickListener { + false + } + } + /** Sets the primary text title */ fun setPrimaryText(title: CharSequence?) { primaryText.text = title @@ -146,7 +161,10 @@ abstract class DaxListItem( * depends on the size of the image */ - fun setLeadingIconSize(imageSize: IconSize, type: ImageBackground) { + fun setLeadingIconSize( + imageSize: IconSize, + type: ImageBackground, + ) { val iconSize = resources.getDimensionPixelSize(IconSize.dimension(imageSize)) val backgroundSize = if (type == ImageBackground.None) { iconSize diff --git a/common/common-ui/src/main/res/layout/component_popup_menu_item.xml b/common/common-ui/src/main/res/layout/component_popup_menu_item.xml index 3809cbc52faa..dc99fa75e10c 100644 --- a/common/common-ui/src/main/res/layout/component_popup_menu_item.xml +++ b/common/common-ui/src/main/res/layout/component_popup_menu_item.xml @@ -38,4 +38,10 @@ app:primaryText="Destructive Type" app:primaryTextType="destructive" /> + + \ No newline at end of file diff --git a/common/common-ui/src/main/res/layout/view_popup_menu_item.xml b/common/common-ui/src/main/res/layout/view_popup_menu_item.xml index d0cbdc363640..13ef28274ad7 100644 --- a/common/common-ui/src/main/res/layout/view_popup_menu_item.xml +++ b/common/common-ui/src/main/res/layout/view_popup_menu_item.xml @@ -14,21 +14,38 @@ ~ limitations under the License. --> - + android:background="?attr/selectableItemBackground" + android:layout_width="match_parent" + android:layout_height="wrap_content"> - \ No newline at end of file + + + \ No newline at end of file diff --git a/common/common-ui/src/main/res/values/attrs-menu-item.xml b/common/common-ui/src/main/res/values/attrs-menu-item.xml index 8367ec7b7fbd..2ae6c195bf55 100644 --- a/common/common-ui/src/main/res/values/attrs-menu-item.xml +++ b/common/common-ui/src/main/res/values/attrs-menu-item.xml @@ -27,6 +27,7 @@ + \ No newline at end of file diff --git a/common/common-ui/src/main/res/values/design-system-theming.xml b/common/common-ui/src/main/res/values/design-system-theming.xml index f0d4885bc9f7..87113291fa92 100644 --- a/common/common-ui/src/main/res/values/design-system-theming.xml +++ b/common/common-ui/src/main/res/values/design-system-theming.xml @@ -50,6 +50,7 @@ @style/Widget.DuckDuckGo.PopUpOverflowMenu @style/Widget.DuckDuckGo.OverflowButton + @style/Widget.DuckDuckGo.PopupMenu @style/Widget.DuckDuckGo.BottomSheetDialog @style/Widget.DuckDuckGo.TabLayout @style/Widget.DuckDuckGo.RadioButton diff --git a/common/common-ui/src/main/res/values/widgets.xml b/common/common-ui/src/main/res/values/widgets.xml index 8a3bafd38969..cbbaee121f60 100644 --- a/common/common-ui/src/main/res/values/widgets.xml +++ b/common/common-ui/src/main/res/values/widgets.xml @@ -77,7 +77,12 @@ - \ No newline at end of file diff --git a/saved-sites/saved-sites-impl/src/test/java/com/duckduckgo/savedsites/impl/bookmarks/BookmarksViewModelTest.kt b/saved-sites/saved-sites-impl/src/test/java/com/duckduckgo/savedsites/impl/bookmarks/BookmarksViewModelTest.kt index dd2d163248a6..00294b6de78b 100644 --- a/saved-sites/saved-sites-impl/src/test/java/com/duckduckgo/savedsites/impl/bookmarks/BookmarksViewModelTest.kt +++ b/saved-sites/saved-sites-impl/src/test/java/com/duckduckgo/savedsites/impl/bookmarks/BookmarksViewModelTest.kt @@ -30,8 +30,14 @@ import com.duckduckgo.savedsites.api.models.SavedSite.Bookmark import com.duckduckgo.savedsites.api.models.SavedSite.Favorite import com.duckduckgo.savedsites.api.models.SavedSites import com.duckduckgo.savedsites.api.models.SavedSitesNames +import com.duckduckgo.savedsites.api.models.SavedSitesNames.BOOKMARKS_ROOT import com.duckduckgo.savedsites.api.service.SavedSitesManager import com.duckduckgo.savedsites.impl.SavedSitesPixelName +import com.duckduckgo.savedsites.impl.bookmarks.BookmarksAdapter.BookmarkItem +import com.duckduckgo.savedsites.impl.bookmarks.BookmarksAdapter.BookmarksItemTypes +import com.duckduckgo.savedsites.impl.store.BookmarksDataStore +import com.duckduckgo.savedsites.impl.store.SortingMode.MANUAL +import com.duckduckgo.savedsites.impl.store.SortingMode.NAME import com.duckduckgo.sync.api.engine.SyncEngine import com.duckduckgo.sync.api.favicons.FaviconsFetchingPrompt import kotlinx.coroutines.flow.flowOf @@ -70,6 +76,7 @@ class BookmarksViewModelTest { private val syncEngine: SyncEngine = mock() private val pixel: Pixel = mock() private val faviconsFetchingPrompt: FaviconsFetchingPrompt = mock() + private val bookmarksDataStore: BookmarksDataStore = mock() private val bookmark = Bookmark( @@ -92,6 +99,7 @@ class BookmarksViewModelTest { pixel, syncEngine, faviconsFetchingPrompt, + bookmarksDataStore, coroutineRule.testDispatcherProvider, coroutineRule.testScope, ) @@ -113,6 +121,7 @@ class BookmarksViewModelTest { ), ) + whenever(bookmarksDataStore.getSortingMode()).thenReturn(NAME) testee.fetchBookmarksAndFolders(SavedSitesNames.BOOKMARKS_ROOT) } @@ -453,4 +462,86 @@ class BookmarksViewModelTest { verify(pixel).fire(SavedSitesPixelName.EDIT_BOOKMARK_DELETE_BOOKMARK_CLICKED) } + + @Test + fun whenSortingModeSelectedThenDataStored() = runTest { + testee.onSortingModeSelected(NAME) + verify(bookmarksDataStore).setSortingMode(NAME) + } + + @Test + fun whenSortingByNameSelectedThenListIsSorted() { + val items = mutableListOf() + val folderNews = BookmarkFolder(id = "folderA", name = "News", parentId = SavedSitesNames.BOOKMARKS_ROOT, 0, 0, "timestamp") + val folderSports = BookmarkFolder(id = "folderB", name = "Sports", parentId = SavedSitesNames.BOOKMARKS_ROOT, 0, 0, "timestamp") + val bookmarkAs = Bookmark(id = "bookmarkA", title = "As", url = "www.example.com", parentId = SavedSitesNames.BOOKMARKS_ROOT, "timestamp") + val bookmarkCnn = Bookmark(id = "bookmarCnn", title = "Cnn", url = "www.example.com", parentId = SavedSitesNames.BOOKMARKS_ROOT, "timestamp") + val bookmarkReddit = Bookmark( + id = "bookmarReddit", + title = "Reddit", + url = "www.example.com", + parentId = SavedSitesNames.BOOKMARKS_ROOT, + "timestamp", + ) + val bookmarkTheGuardian = Bookmark( + id = "bookmarT", + title = "The Guardian", + url = "www.example.com", + parentId = SavedSitesNames.BOOKMARKS_ROOT, + "timestamp", + ) + + items.add(BookmarkItem(bookmarkAs)) + items.add(BookmarkItem(bookmarkReddit)) + items.add(BookmarkItem(bookmarkCnn)) + items.add(BookmarkItem(bookmarkTheGuardian)) + items.add(BookmarksAdapter.BookmarkFolderItem(folderSports)) + items.add(BookmarksAdapter.BookmarkFolderItem(folderNews)) + + val sortedElements = testee.sortElements(items, NAME) + assertEquals((sortedElements[0] as BookmarkItem).bookmark, bookmarkAs) + assertEquals((sortedElements[1] as BookmarkItem).bookmark, bookmarkCnn) + assertEquals((sortedElements[2] as BookmarksAdapter.BookmarkFolderItem).bookmarkFolder, folderNews) + assertEquals((sortedElements[3] as BookmarkItem).bookmark, bookmarkReddit) + assertEquals((sortedElements[4] as BookmarksAdapter.BookmarkFolderItem).bookmarkFolder, folderSports) + assertEquals((sortedElements[5] as BookmarkItem).bookmark, bookmarkTheGuardian) + } + + @Test + fun whenBrowserMenuPressedAndBookmarksEmptyThenCommandSent() { + whenever(savedSitesRepository.getSavedSites(anyString())).thenReturn( + flowOf(SavedSites(emptyList(), emptyList())), + ) + + testee.fetchBookmarksAndFolders(BOOKMARKS_ROOT) + + testee.onBrowserMenuPressed() + + verify(commandObserver).onChanged(commandCaptor.capture()) + assertEquals(NAME, (commandCaptor.lastValue as BookmarksViewModel.Command.ShowBrowserMenu).sortingMode) + assertEquals(true, (commandCaptor.lastValue as BookmarksViewModel.Command.ShowBrowserMenu).buttonsDisabled) + } + + @Test + fun whenBrowserMenuPressedAndBookmarksNotEmptyThenCommandSent() { + testee.fetchBookmarksAndFolders(BOOKMARKS_ROOT) + + testee.onBrowserMenuPressed() + + verify(commandObserver).onChanged(commandCaptor.capture()) + assertEquals(NAME, (commandCaptor.lastValue as BookmarksViewModel.Command.ShowBrowserMenu).sortingMode) + assertEquals(false, (commandCaptor.lastValue as BookmarksViewModel.Command.ShowBrowserMenu).buttonsDisabled) + } + + @Test + fun whenBrowserMenuPressedAndManualSortingModeThenCommandSent() { + whenever(bookmarksDataStore.getSortingMode()).thenReturn(MANUAL) + testee.fetchBookmarksAndFolders(BOOKMARKS_ROOT) + + testee.onBrowserMenuPressed() + + verify(commandObserver).onChanged(commandCaptor.capture()) + assertEquals(MANUAL, (commandCaptor.lastValue as BookmarksViewModel.Command.ShowBrowserMenu).sortingMode) + assertEquals(false, (commandCaptor.lastValue as BookmarksViewModel.Command.ShowBrowserMenu).buttonsDisabled) + } }