Multi-selection: Ship review change requests #5841

0nko · 2025-03-31T17:25:35Z

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

Description

This PR implements the following changes:

Implement undo snackbar after tab deletion in the browser screen
Add the AI chat back to the normal mode menu
Reverse the order of share/bookmark tabs in selection mode
Revert the tab switcher title back to "Tabs"

Steps to test this PR

Undo snackbar

Go to the tab switcher
Make sure you have multiple tabs
Tap on the more menu -> Close all tabs
Verify a snackbar with the Undo button appears in the browser
Tap on the Undo button
Verify the closed tabs are all restored
Verify the NTP is at the last position

AI chat

Go to the tab switcher
Verify the AI chat menu item is visible and works

Selection mode menu

Tab switcher screen title

Go to the tab switcher
Verify the title says "Tabs"

0nko · 2025-03-31T17:25:52Z

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

Multi-selection: Ship review change requests #5841 👈 (View in Graphite)
Multi-selection: Tab switcher #5464 : 9 other dependent PRs (#5467 , #5588 , #5623 and 6 others)
develop

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

mikescamell

Looks good to me! Just a few questions and comments but nothing blocking.

Nice work! 🙌

mikescamell · 2025-04-01T08:25:14Z

app/src/main/java/com/duckduckgo/app/tabs/model/TabDataRepository.kt

-        if (tabManagerFeatureFlags.multiSelection().isEnabled()) {
-            clearAllSiteData(getDeletableTabIds())
-        }
+        clearAllSiteData(getDeletableTabIds())


❓Why is this unflagged now?

mikescamell · 2025-04-01T08:33:20Z

app/src/main/java/com/duckduckgo/app/tabs/model/TabDataRepository.kt

    private fun clearAllSiteData(tabIds: List<String>) {
        tabIds.forEach { tabId ->
+            webViewSessionStorage.deleteSession(tabId)
+            adClickManager.clearTabId(tabId)


❓This feels off to me. I don't think it should be the responsibility of the TabRepository to cleanup webViewSessionStorage and adClickManager. I don't think we have the concept of UseCases but this is where I would see one useful, to coordinate all the cleanup needed. But I see we were also doing that for PreviewImages and Favicon here already.

What do you think? And it's beyond the scope of this project.

I noticed that we were already doing the cleanup for other things, like you mentioned. I figured it'd make sense to put it all in a single place where any code deleting a tab would execute this -- hence the tab repository. The reason for this is that we would miss the cleanup for the previews and favicons when the tabs were purged, so it made sense to me to unify it here.

mikescamell · 2025-04-01T08:38:36Z

app/src/main/java/com/duckduckgo/app/browser/BrowserViewModel.kt

+        data object LaunchPlayStore : Command()
+        data object LaunchFeedbackView : Command()
+        data object LaunchTabSwitcherAfterTabsUndeleted : Command()
        data class ShowAppEnjoymentPrompt(val promptCount: PromptCount) : Command()


💡I don't think we need to specify why in this command name, if in the end all we do is launch the TabSwitcher. I would just call it LaunchTabSwitcher

mikescamell · 2025-04-01T08:46:02Z

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

    fun undoDeletableTabs(tabIds: List<String>) {
-        viewModelScope.launch(dispatcherProvider.io()) {
+        viewModelScope.launch {
            tabRepository.undoDeletable(tabIds)


❓Why did you remove the use of the io dispatcher?

They were unnecessary, the operation is scheduled on an IO thread in the repository using databaseExecutor().scheduleDirect().

mikescamell · 2025-04-01T09:07:09Z

app/src/main/java/com/duckduckgo/app/browser/BrowserActivity.kt

+    private fun configureFlowCollectors() {
+        if (swipingTabsFeature.isEnabled) {
+            lifecycleScope.launch {
+                viewModel.tabsFlow.flowWithLifecycle(lifecycle).collectLatest {


💡Use repeatOnLifecycle and launch all flow collections under one lifecycleScope.launch

mikescamell · 2025-04-01T09:07:35Z

app/src/main/java/com/duckduckgo/app/browser/BrowserActivity.kt

-        } else {
+        if (!swipingTabsFeature.isEnabled) {
+            // when swiping is enabled, the state is controlled be flows initialized in configureFlowCollectors()
            viewModel.selectedTab.observe(this) {


mikescamell · 2025-04-01T09:08:46Z

app/src/main/java/com/duckduckgo/app/browser/BrowserViewModel.kt

    }.filterNot { it == -1 }

+    val deletableTabsFlow = tabRepository.flowDeletableTabs
+        .map { tabs -> tabs.map { tab -> tab.tabId } }


mikescamell · 2025-04-01T09:09:26Z

app/src/main/java/com/duckduckgo/app/browser/BrowserViewModel.kt

+        .filter { it.isNotEmpty() }
+        .distinctUntilChanged()
+        .debounce(100.milliseconds)
+        .conflate()


❓Why do we need the debounce? Is it a similar situation to selectedTabFlow?

Yeah, I've observed this weird edge case when deleting multiple tabs in succession and sometimes the snackbar didn't work correctly. This fixed it.

mikescamell · 2025-04-01T09:11:18Z

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

-        tabRepository.deleteTabs(tabIds)
+    suspend fun onUndoDeleteSnackbarDismissed(tabIds: List<String>) {
+        // delete only recently deleted tabs, because others may need to be preserved for undeleting
+        deleteTabs(tabIds)


💡"undeleting" -> "restoring"

mikescamell · 2025-04-01T09:16:11Z

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

-            webViewSessionStorage.deleteSession(tabId)
-        }
+    private suspend fun deleteTabs(tabIds: List<String>) {
+        tabRepository.deleteTabs(tabIds)


❓Why did we remove the io dispatcher?

Because the operation is scheduled on an IO thread in the repository using databaseExecutor().scheduleDirect().

…creen

… collections

This was referenced Mar 31, 2025

Multi-selection: Tests #5799

Merged

Multi-selection: Tab closing #5725

Merged

This was referenced Mar 31, 2025

Multi-selection: Pixels #5788

Merged

Multi-selection: Translations #5840

Merged

0nko changed the title ~~Perform all tab-related data clearing in the repository~~ Multi-selection: Ship review Mar 31, 2025

0nko changed the title ~~Multi-selection: Ship review~~ Multi-selection: Ship review change requests Mar 31, 2025

0nko marked this pull request as ready for review March 31, 2025 17:38

0nko requested a review from mikescamell March 31, 2025 17:57

mikescamell self-assigned this Apr 1, 2025

mikescamell approved these changes Apr 1, 2025

View reviewed changes

0nko force-pushed the feature/ondrej/tab-multi-selection-ship-review-changes branch from d15dea2 to c727db9 Compare April 1, 2025 17:27

0nko force-pushed the feature/ondrej/tab-multi-selection-translation branch from 0e54338 to 0b534bd Compare April 1, 2025 17:27

0nko force-pushed the feature/ondrej/tab-multi-selection-ship-review-changes branch from c727db9 to 9f11cf0 Compare April 2, 2025 10:09

0nko force-pushed the feature/ondrej/tab-multi-selection-translation branch from 13d87b2 to 725c456 Compare April 2, 2025 10:09

0nko force-pushed the feature/ondrej/tab-multi-selection-ship-review-changes branch from 9f11cf0 to 7f2b510 Compare April 2, 2025 11:05

0nko force-pushed the feature/ondrej/tab-multi-selection-translation branch from 725c456 to 12ac334 Compare April 2, 2025 11:05

0nko force-pushed the feature/ondrej/tab-multi-selection-ship-review-changes branch from 7f2b510 to 032bcb3 Compare April 2, 2025 16:19

0nko force-pushed the feature/ondrej/tab-multi-selection-translation branch from bda4f31 to b744a85 Compare April 2, 2025 16:19

0nko added 11 commits April 3, 2025 11:50

Perform all tab-related data clearing in the repository

8f1a5b8

Add undo snackbar functionality to browser screen

8dbd69e

Skip deleted tab purging when leaving tab switcher

086226a

Use a 200ms delay before checking for deletable tabs in the browser s…

b1cf8b7

…creen

Revert the tab switcher title in normal mode

c61727f

Add duck AI menu item back

341333c

Reverse the order of the share and bookmark menu items

ec25ab2

Show undeleted tabs in front of the current tab

e55dc43

Fix the active tab position when tabs undeleted

abd327f

Move the flow collector initialization to onCreate to avoid duplicate…

4fe0e11

… collections

Fix the deletable tabs observation in the browser

49295fd

0nko added 11 commits April 3, 2025 11:50

Fix the TabSwichertViewModelTest

774c613

Update TabsDaoTest

9120f07

Fix TabDataRepositoryTest

ddb821d

Update the fake repository method signature

41a80fa

Optimize imports

dfa1722

Fix a comment

dea2918

Simplify the flow collection

9dc8315

Rename the command to a have a simpler name

83d8fb7

Add the feature flag back

3d98fed

Fix the build

b6723fb

Fix the unit tests

40202e1

0nko force-pushed the feature/ondrej/tab-multi-selection-ship-review-changes branch from d480d5e to 40202e1 Compare April 3, 2025 09:50

Base automatically changed from feature/ondrej/tab-multi-selection-translation to feature/ondrej/tab-multi-selection April 3, 2025 09:59

0nko force-pushed the feature/ondrej/tab-multi-selection branch from 8e56d79 to 052eab9 Compare April 3, 2025 10:00

0nko merged commit ec8e0fb into feature/ondrej/tab-multi-selection Apr 3, 2025
3 of 5 checks passed

0nko deleted the feature/ondrej/tab-multi-selection-ship-review-changes branch April 3, 2025 10:10

Uh oh!

Multi-selection: Ship review change requests #5841

Multi-selection: Ship review change requests #5841

Uh oh!

Conversation

0nko commented Mar 31, 2025 • edited by mikescamell Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Steps to test this PR

Uh oh!

0nko commented Mar 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikescamell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

0nko commented Mar 31, 2025 •

edited by mikescamell

Loading

0nko commented Mar 31, 2025 •

edited

Loading