Skip to content

Multi-selection: Bookmarking #5716

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
Parent: Multi-selection: Tab switcher
merged 10 commits into from
Mar 17, 2025
Merged

Multi-selection: Bookmarking #5716

merged 10 commits into from
Mar 17, 2025

Conversation

@0nko
Copy link
Member

@0nko 0nko commented Feb 28, 2025 •
edited by mikescamell
Loading

Task/Issue URL: https://app.asana.com/0/72649045549333/1209118961868653

Description

This PR adds the bookmarking functionality to the tab switcher.

Steps to test this PR

Bookmarking selected tabs

  • Make sure you have at least 3 distinct sites/tabs
  • Make sure no bookmarks exist
  • Go to the tab switcher
  • Tap on the more menu button
  • Tap on Select Tab
  • Tap on the more menu button
  • Verify only Select All menu is availabe
  • Select 2 tabs
  • Tap on the bookmark toolbar button
  • Verify that Bookmark 2 Tabs? dialog appears
  • Tap on Bookmark
  • Verify that this snackbar appears: 2 bookmarks added
  • Navigate to Bookmarks and verify the bookmarks were really created

Undo bookmark action

  • Make sure you have at least 1 tab
  • Make sure no bookmarks exist
  • Go to the tab switcher -> Select a tab -> Tap on the bookmark menu
  • Verify that this snackbar appears: 1 bookmarks added with the Undo action
  • Tap on the Undo action
  • Navigate to Bookmarks
  • Verify the bookmark is not listed

Bookmarking unique and non-NTP tabs

  • Make sure you have 4 distinct sites/tabs, including 1 NTP
  • Make sure one of them is already bookmarked
  • Go to the tab switcher
  • Tap on the more menu button
  • Tap on Select Tabs
  • Tap on Select All
  • Verify that Bookmark 4 Tabs? dialog appears
  • Tap on Bookmark
  • Verify that this snackbar appears: 2 bookmarks added (NTP and the existing bookmark skipped)
  • Tap on the more menu and bookmark the selected tabs again
  • Verify that this snackbar appears: 0 bookmarks added, now without the Undo action
  • Navigate to Bookmarks and verify the 2 bookmarks added previously were really created

This was referenced Feb 28, 2025
Merged
Merged
Merged
Merged
Merged
@0nko Graphite App
Copy link
Member Author

0nko commented Feb 28, 2025 •
edited
Loading

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@0nko 0nko changed the title Add the bookmarking functionality logic Multi-selection: Bookmarking Feb 28, 2025
@0nko 0nko marked this pull request as ready for review February 28, 2025 17:45
@0nko 0nko force-pushed the feature/ondrej/tab-multi-selection-bookmarking branch from bb287ba to a5f3410 Compare March 3, 2025 10:37
@0nko 0nko force-pushed the feature/ondrej/tab-multi-selection-link-sharing branch from 353ba9a to d955785 Compare March 3, 2025 10:37
@0nko 0nko force-pushed the feature/ondrej/tab-multi-selection-bookmarking branch from a5f3410 to 173c9d6 Compare March 3, 2025 10:41
@0nko 0nko requested a review from mikescamell March 3, 2025 16:51
@0nko 0nko force-pushed the feature/ondrej/tab-multi-selection-bookmarking branch from 41850ad to aaa0d58 Compare March 3, 2025 20:28
@0nko 0nko force-pushed the feature/ondrej/tab-multi-selection-link-sharing branch from 16b8b26 to 8b87fde Compare March 3, 2025 20:28
@0nko 0nko mentioned this pull request Mar 3, 2025
Merged
41 tasks
@0nko 0nko force-pushed the feature/ondrej/tab-multi-selection-link-sharing branch from 8b87fde to f8eb8b9 Compare March 3, 2025 20:39
@0nko 0nko force-pushed the feature/ondrej/tab-multi-selection-bookmarking branch from aaa0d58 to 0e745d2 Compare March 3, 2025 20:39
@0nko 0nko force-pushed the feature/ondrej/tab-multi-selection-link-sharing branch from f8eb8b9 to 9dd1c3b Compare March 4, 2025 11:00
@0nko 0nko force-pushed the feature/ondrej/tab-multi-selection-bookmarking branch 2 times, most recently from 3db6a74 to c80ecea Compare March 4, 2025 12:52
@0nko 0nko force-pushed the feature/ondrej/tab-multi-selection-link-sharing branch from 9dd1c3b to d984ebe Compare March 4, 2025 12:52
@0nko 0nko force-pushed the feature/ondrej/tab-multi-selection-bookmarking branch from c80ecea to 2cc684b Compare March 5, 2025 10:02
@0nko 0nko force-pushed the feature/ondrej/tab-multi-selection-link-sharing branch 2 times, most recently from 19f70d4 to d33cb74 Compare March 5, 2025 16:56
@0nko 0nko force-pushed the feature/ondrej/tab-multi-selection-bookmarking branch from 2cc684b to cf6b2c2 Compare March 5, 2025 16:56
@0nko 0nko force-pushed the feature/ondrej/tab-multi-selection-link-sharing branch from d33cb74 to bd9aa77 Compare March 6, 2025 14:12
@0nko 0nko force-pushed the feature/ondrej/tab-multi-selection-bookmarking branch 2 times, most recently from 3412b1c to fc005e5 Compare March 10, 2025 10:07
@0nko 0nko force-pushed the feature/ondrej/tab-multi-selection-link-sharing branch from 6f05751 to 38b575a Compare March 10, 2025 10:58
@0nko 0nko force-pushed the feature/ondrej/tab-multi-selection-bookmarking branch from fc005e5 to cf68075 Compare March 10, 2025 10:58
@0nko 0nko force-pushed the feature/ondrej/tab-multi-selection-link-sharing branch from a7c1150 to d365cf5 Compare March 11, 2025 11:47
@0nko 0nko force-pushed the feature/ondrej/tab-multi-selection-bookmarking branch from 77f1d22 to 73fef25 Compare March 12, 2025 17:50
@0nko 0nko force-pushed the feature/ondrej/tab-multi-selection-link-sharing branch 2 times, most recently from 723af33 to 31011ee Compare March 13, 2025 13:08
@0nko 0nko force-pushed the feature/ondrej/tab-multi-selection-bookmarking branch from 73fef25 to 099b529 Compare March 13, 2025 13:08
@0nko 0nko changed the base branch from feature/ondrej/tab-multi-selection-link-sharing to graphite-base/5716 March 13, 2025 13:11
@0nko 0nko force-pushed the feature/ondrej/tab-multi-selection-bookmarking branch from 099b529 to f232f7e Compare March 13, 2025 14:56
@0nko 0nko changed the base branch from graphite-base/5716 to feature/ondrej/tab-multi-selection-link-sharing March 13, 2025 14:56
@0nko 0nko force-pushed the feature/ondrej/tab-multi-selection-bookmarking branch from 5a8c063 to 4b2cfaa Compare March 14, 2025 09:41
@0nko 0nko force-pushed the feature/ondrej/tab-multi-selection-link-sharing branch from 9b68851 to 3d875be Compare March 14, 2025 09:41
Base automatically changed from feature/ondrej/tab-multi-selection-link-sharing to feature/ondrej/tab-multi-selection March 14, 2025 09:56
@0nko 0nko force-pushed the feature/ondrej/tab-multi-selection branch from fee4dba to 3d875be Compare March 14, 2025 09:58
mikescamell
mikescamell approved these changes Mar 14, 2025
View reviewed changes
Copy link
Contributor

@mikescamell mikescamell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! Works as expected 🎉

I think the last testing step should read

"Navigate to Bookmarks and verify the 2 3 bookmarks added previously were really created"

correct?

app/src/main/java/com/duckduckgo/app/tabs/ui/TabSwitcherSnackbar.kt Outdated
}

private val snackbar = Snackbar.make(anchorView, message, Snackbar.LENGTH_LONG)
.setDuration(SNACKBAR_DISPLAY_TIME_MS) // 3.5 seconds
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍Maybe we can remove the comment here as we're using a constant that might change

app/src/main/java/com/duckduckgo/app/tabs/ui/TabSwitcherSnackbar.kt
import com.google.android.material.snackbar.BaseTransientBottomBar
import com.google.android.material.snackbar.Snackbar

class TabSwitcherSnackbar(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

app/src/main/java/com/duckduckgo/app/tabs/ui/TabSwitcherViewModel.kt Outdated

fun undoBookmarkAction() {
recentlySavedBookmarks.forEach { bookmark ->
viewModelScope.launch(dispatcherProvider.io()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡Would it make more sense to move the launch outside the forEach? This is only going to get called if we're actually undoing something so we definitely will deleting something, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I just thought I'd parallelize the calls but I guess it doesn't really matter here.

app/src/main/java/com/duckduckgo/app/tabs/ui/TabSwitcherViewModel.kt
}
}

private suspend fun saveSiteBookmark(tabId: String) = withContext(dispatcherProvider.io()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍I find this function a little difficult to read, there's quite a bit going on here and lots of null checks

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hopefully this change will make it a bit more readable.

@0nko
Copy link
Member Author

0nko commented Mar 17, 2025

Thanks @mikescamell!

I think the last testing step should read "Navigate to Bookmarks and verify the 2 3 bookmarks added previously were really created" correct?

No. Because of the step Make sure one of them is already bookmarked, one bookmark should be skipped since only unique bookmarks are added.

mikescamell
mikescamell reviewed Mar 17, 2025
View reviewed changes
app/src/main/java/com/duckduckgo/app/tabs/ui/TabSwitcherSnackbar.kt

private val snackbar = Snackbar.make(anchorView, message, Snackbar.LENGTH_LONG)
.setDuration(SNACKBAR_DISPLAY_TIME_MS) // 3.5 seconds
.setDuration(SNACKBAR_DISPLAY_TIME_MS)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

mikescamell
mikescamell reviewed Mar 17, 2025
View reviewed changes
app/src/main/java/com/duckduckgo/app/tabs/ui/TabSwitcherViewModel.kt
fun undoBookmarkAction() {
recentlySavedBookmarks.forEach { bookmark ->
viewModelScope.launch(dispatcherProvider.io()) {
viewModelScope.launch(dispatcherProvider.io()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

@0nko 0nko force-pushed the feature/ondrej/tab-multi-selection-bookmarking branch from ed2738d to 57d8234 Compare March 17, 2025 17:45
@0nko 0nko force-pushed the feature/ondrej/tab-multi-selection branch from 3d875be to 5849087 Compare March 17, 2025 17:45
@0nko 0nko merged commit 9aa5957 into feature/ondrej/tab-multi-selection Mar 17, 2025
4 of 5 checks passed
@0nko 0nko deleted the feature/ondrej/tab-multi-selection-bookmarking branch March 17, 2025 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mikescamell mikescamell mikescamell approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@0nko @mikescamell