Skip to content

Commit 5f5a90e

Browse files
LukasPaczosLukasPaczos
authored
Optimize bookmarks filtering and sorting (#6357)
Task/Issue URL: https://app.asana.com/1/137249556945/project/414730916066338/task/1210718637089838?focus=true ### Description 1. Adds a new dev setting to populate saved sites repo with random bookmarks. 2. Optimizes bookmark search by skipping relative position recalculations. When filtering, item order remains unchanged, allowing us to bypass an extra round of equality checks. 3. Moves bookmark update diff computation to a worker thread to avoid UI thread contention and ANRs. 4. Decouples bookmark items to display from the rest of the state to avoid unnecessary computations and UI updates. Previously, updating the search query triggered a view state change (due to the persisted `ViewState#searchQuery`), which caused the `RecyclerView` to recalculate before the actual filtering occurred. As a result, the `RecyclerView` was recalculated and updated twice per query, which was also contributing to resource contention. ### Steps to test this PR - [x] With bookmarks populated, open "Menu -> Bookmarks". - [x] Verify that both "manual" and "by name" options work correctly. - [x] Verify that searching for tabs works correctly. - [x] Try with an extreme number of tabs (500+) and verify that there are no ANRs or main thread hangs when sorting/filtering. - [x] (optional) Try the new dev setting under "Developer Settings -> Tabs and Saved Sites"
1 parent b62a002 commit 5f5a90e

File tree

12 files changed

+240
-76
lines changed

12 files changed

+240
-76
lines changed

‎app/src/internal/java/com/duckduckgo/app/dev/settings/DevSettingsActivity.kt

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,6 @@ import com.duckduckgo.app.dev.settings.DevSettingsViewModel.Command.CustomTabs
3939
import com.duckduckgo.app.dev.settings.DevSettingsViewModel.Command.Notifications
4040
import com.duckduckgo.app.dev.settings.DevSettingsViewModel.Command.OpenUASelector
4141
import com.duckduckgo.app.dev.settings.DevSettingsViewModel.Command.SendTdsIntent
42-
import com.duckduckgo.app.dev.settings.DevSettingsViewModel.Command.ShowSavedSitesClearedConfirmation
4342
import com.duckduckgo.app.dev.settings.DevSettingsViewModel.Command.Tabs
4443
import com.duckduckgo.app.dev.settings.customtabs.CustomTabsInternalSettingsActivity
4544
import com.duckduckgo.app.dev.settings.db.UAOverride
@@ -101,9 +100,6 @@ class DevSettingsActivity : DuckDuckGoActivity() {
101100
Thread.sleep(10000)
102101
}
103102
}
104-
binding.clearSavedSites.setOnClickListener {
105-
viewModel.clearSavedSites()
106-
}
107103
binding.overrideUserAgentSelector.setOnClickListener { viewModel.onUserAgentSelectorClicked() }
108104
binding.overridePrivacyRemoteConfigUrl.setOnClickListener { viewModel.onRemotePrivacyUrlClicked() }
109105
binding.customTabs.setOnClickListener { viewModel.customTabsClicked() }
@@ -135,7 +131,6 @@ class DevSettingsActivity : DuckDuckGoActivity() {
135131
when (it) {
136132
is SendTdsIntent -> sendTdsIntent()
137133
is OpenUASelector -> showUASelector()
138-
is ShowSavedSitesClearedConfirmation -> showSavedSitesClearedConfirmation()
139134
is ChangePrivacyConfigUrl -> showChangePrivacyUrl()
140135
is CustomTabs -> showCustomTabs()
141136
Notifications -> showNotifications()
@@ -167,10 +162,6 @@ class DevSettingsActivity : DuckDuckGoActivity() {
167162
popup.show(binding.root, binding.overrideUserAgentSelector)
168163
}
169164

170-
private fun showSavedSitesClearedConfirmation() {
171-
Toast.makeText(this, getString(R.string.devSettingsClearSavedSitesConfirmation), Toast.LENGTH_SHORT).show()
172-
}
173-
174165
private fun showChangePrivacyUrl() {
175166
val options = ActivityOptions.makeSceneTransitionAnimation(this).toBundle()
176167
startActivity(PrivacyConfigInternalSettingsActivity.intent(this), options)

‎app/src/internal/java/com/duckduckgo/app/dev/settings/DevSettingsViewModel.kt

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@ import com.duckduckgo.app.survey.api.SurveyEndpointDataStore
2525
import com.duckduckgo.app.tabs.store.TabSwitcherPrefsDataStore
2626
import com.duckduckgo.common.utils.DispatcherProvider
2727
import com.duckduckgo.di.scopes.ActivityScope
28-
import com.duckduckgo.savedsites.api.SavedSitesRepository
2928
import com.duckduckgo.traces.api.StartupTraces
3029
import com.duckduckgo.user.agent.api.UserAgentProvider
3130
import javax.inject.Inject
@@ -44,7 +43,6 @@ class DevSettingsViewModel @Inject constructor(
4443
private val devSettingsDataStore: DevSettingsDataStore,
4544
private val startupTraces: StartupTraces,
4645
private val userAgentProvider: UserAgentProvider,
47-
private val savedSitesRepository: SavedSitesRepository,
4846
private val dispatcherProvider: DispatcherProvider,
4947
private val surveyEndpointDataStore: SurveyEndpointDataStore,
5048
private val tabSwitcherPrefsDataStore: TabSwitcherPrefsDataStore,
@@ -60,7 +58,6 @@ class DevSettingsViewModel @Inject constructor(
6058
sealed class Command {
6159
object SendTdsIntent : Command()
6260
object OpenUASelector : Command()
63-
object ShowSavedSitesClearedConfirmation : Command()
6461
object ChangePrivacyConfigUrl : Command()
6562
object CustomTabs : Command()
6663
data object Notifications : Command()
@@ -137,13 +134,6 @@ class DevSettingsViewModel @Inject constructor(
137134
}
138135
}
139136

140-
fun clearSavedSites() {
141-
viewModelScope.launch(dispatcherProvider.io()) {
142-
savedSitesRepository.deleteAll()
143-
command.send(Command.ShowSavedSitesClearedConfirmation)
144-
}
145-
}
146-
147137
fun notificationsClicked() {
148138
viewModelScope.launch { command.send(Command.Notifications) }
149139
}

‎app/src/internal/java/com/duckduckgo/app/dev/settings/tabs/DevTabsActivity.kt

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,13 +51,21 @@ class DevTabsActivity : DuckDuckGoActivity() {
5151
setupToolbar(binding.includeToolbar.toolbar)
5252

5353
binding.addTabsButton.setOnClickListener {
54-
viewModel.addTabs(binding.tabCount.text.toString().toInt())
54+
viewModel.addTabs(binding.tabCount.text.toInt())
5555
}
5656

5757
binding.clearTabsButton.setOnClickListener {
5858
viewModel.clearTabs()
5959
}
6060

61+
binding.addBookmarksButton.setOnClickListener {
62+
viewModel.addBookmarks(binding.bookmarksCount.text.toInt())
63+
}
64+
65+
binding.clearBookmarksButton.setOnClickListener {
66+
viewModel.clearBookmarks()
67+
}
68+
6169
observeViewState()
6270
}
6371

@@ -68,6 +76,7 @@ class DevTabsActivity : DuckDuckGoActivity() {
6876

6977
private fun render(viewState: ViewState) {
7078
binding.tabCountHeader.text = getString(R.string.devSettingsTabsScreenHeader, viewState.tabCount)
79+
binding.bookmarksCountHeader.text = getString(R.string.devSettingsTabsBookmarksScreenHeader, viewState.bookmarkCount)
7180
}
7281

7382
companion object {

‎app/src/internal/java/com/duckduckgo/app/dev/settings/tabs/DevTabsViewModel.kt

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import com.duckduckgo.anvil.annotations.ContributesViewModel
2222
import com.duckduckgo.app.tabs.model.TabDataRepository
2323
import com.duckduckgo.common.utils.DispatcherProvider
2424
import com.duckduckgo.di.scopes.ActivityScope
25+
import com.duckduckgo.savedsites.api.SavedSitesRepository
2526
import javax.inject.Inject
2627
import kotlinx.coroutines.flow.MutableStateFlow
2728
import kotlinx.coroutines.flow.asStateFlow
@@ -58,10 +59,12 @@ private val randomUrls = listOf(
5859
class DevTabsViewModel @Inject constructor(
5960
private val dispatcher: DispatcherProvider,
6061
private val tabDataRepository: TabDataRepository,
62+
private val savedSitesRepository: SavedSitesRepository,
6163
) : ViewModel() {
6264

6365
data class ViewState(
6466
val tabCount: Int = 0,
67+
val bookmarkCount: Int = 0,
6568
)
6669

6770
private val _viewState = MutableStateFlow(ViewState())
@@ -74,6 +77,13 @@ class DevTabsViewModel @Inject constructor(
7477
}
7578
.flowOn(dispatcher.io())
7679
.launchIn(viewModelScope)
80+
81+
savedSitesRepository.getBookmarks()
82+
.onEach { bookmarks ->
83+
_viewState.update { it.copy(bookmarkCount = bookmarks.count()) }
84+
}
85+
.flowOn(dispatcher.io())
86+
.launchIn(viewModelScope)
7787
}
7888

7989
fun addTabs(count: Int) {
@@ -92,4 +102,22 @@ class DevTabsViewModel @Inject constructor(
92102
tabDataRepository.deleteAll()
93103
}
94104
}
105+
106+
fun addBookmarks(count: Int) {
107+
viewModelScope.launch(dispatcher.io()) {
108+
repeat(count) {
109+
val randomIndex = randomUrls.indices.random()
110+
savedSitesRepository.insertBookmark(
111+
title = "",
112+
url = randomUrls[randomIndex],
113+
)
114+
}
115+
}
116+
}
117+
118+
fun clearBookmarks() {
119+
viewModelScope.launch(dispatcher.io()) {
120+
savedSitesRepository.deleteAll()
121+
}
122+
}
95123
}

‎app/src/internal/res/layout/activity_dev_settings.xml

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -73,13 +73,6 @@
7373
app:primaryText="@string/devSettingsTriggerAnr"
7474
app:secondaryText="@string/devSettingsTriggerAnrSubtitle" />
7575

76-
<com.duckduckgo.common.ui.view.listitem.TwoLineListItem
77-
android:id="@+id/clearSavedSites"
78-
android:layout_width="match_parent"
79-
android:layout_height="wrap_content"
80-
app:primaryText="@string/devSettingsClearSavedSites"
81-
app:secondaryText="@string/devSettingsClearSavedSitesSubtitle" />
82-
8376
<com.duckduckgo.common.ui.view.listitem.TwoLineListItem
8477
android:id="@+id/customTabs"
8578
android:layout_width="match_parent"

‎app/src/internal/res/layout/activity_dev_tabs.xml

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,5 +100,78 @@
100100

101101
</androidx.constraintlayout.widget.ConstraintLayout>
102102

103+
<Space
104+
android:layout_width="match_parent"
105+
android:layout_height="8dp" />
106+
107+
<com.duckduckgo.common.ui.view.text.DaxTextView
108+
android:id="@+id/bookmarksCountHeader"
109+
android:layout_width="match_parent"
110+
android:layout_height="48dp"
111+
android:gravity="center"
112+
android:text="@string/devSettingsTabsBookmarksScreenHeader"
113+
app:typography="caption_allCaps" />
114+
115+
<Space
116+
android:layout_width="match_parent"
117+
android:layout_height="8dp" />
118+
119+
<androidx.constraintlayout.widget.ConstraintLayout
120+
android:layout_width="match_parent"
121+
android:layout_height="wrap_content">
122+
123+
<com.duckduckgo.common.ui.view.listitem.OneLineListItem
124+
android:id="@+id/generateBookmarksItem"
125+
android:layout_width="0dp"
126+
android:layout_height="wrap_content"
127+
app:layout_constraintEnd_toStartOf="@id/bookmarksCount"
128+
app:layout_constraintStart_toStartOf="parent"
129+
app:layout_constraintTop_toTopOf="parent"
130+
app:primaryText="@string/devSettingsTabsScreenBookmarksToAdd" />
131+
132+
<com.duckduckgo.common.ui.view.text.DaxTextInput
133+
android:id="@+id/bookmarksCount"
134+
style="@style/Widget.DuckDuckGo.TextInput"
135+
android:layout_width="72dp"
136+
android:layout_height="wrap_content"
137+
android:layout_marginEnd="@dimen/keyline_4"
138+
android:gravity="center"
139+
android:inputType="number"
140+
android:text="25"
141+
app:layout_constraintBottom_toBottomOf="@id/generateBookmarksItem"
142+
app:layout_constraintEnd_toEndOf="parent"
143+
app:layout_constraintStart_toEndOf="@id/generateBookmarksItem"
144+
app:layout_constraintTop_toTopOf="@id/generateBookmarksItem"
145+
app:type="single_line"
146+
tools:ignore="HardcodedText" />
147+
148+
<com.duckduckgo.common.ui.view.button.DaxButtonPrimary
149+
android:id="@+id/addBookmarksButton"
150+
android:layout_width="wrap_content"
151+
android:layout_height="wrap_content"
152+
android:layout_marginTop="@dimen/keyline_4"
153+
android:text="@string/devSettingsTabsScreenAddBookmarksButtonText"
154+
app:daxButtonSize="large" />
155+
156+
<com.duckduckgo.common.ui.view.button.DaxButtonPrimary
157+
android:id="@+id/clearBookmarksButton"
158+
android:layout_width="wrap_content"
159+
android:layout_height="wrap_content"
160+
android:layout_marginTop="@dimen/keyline_4"
161+
android:text="@string/devSettingsTabsScreenClearBookmarksButtonText"
162+
app:daxButtonSize="large" />
163+
164+
<androidx.constraintlayout.helper.widget.Flow
165+
android:id="@+id/bookmarksFlowView"
166+
android:layout_width="match_parent"
167+
android:layout_height="0dp"
168+
android:layout_marginTop="@dimen/keyline_4"
169+
app:constraint_referenced_ids="addBookmarksButton,clearBookmarksButton"
170+
app:flow_horizontalStyle="spread"
171+
app:flow_wrapMode="chain"
172+
app:layout_constraintTop_toBottomOf="@id/generateBookmarksItem" />
173+
174+
</androidx.constraintlayout.widget.ConstraintLayout>
175+
103176
</LinearLayout>
104177
</LinearLayout>

‎app/src/internal/res/values/donottranslate.xml

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -27,9 +27,6 @@
2727
<string name="devStartupTracingByline">Enable/disable start-up tracing</string>
2828
<string name="devSettingsTriggerAnr">Trigger ANR</string>
2929
<string name="devSettingsTriggerAnrSubtitle">Click here to trigger an ANR in the app</string>
30-
<string name="devSettingsClearSavedSites">Clear saved sites</string>
31-
<string name="devSettingsClearSavedSitesSubtitle">Click here to clear all bookmarks, folders and favorites</string>
32-
<string name="devSettingsClearSavedSitesConfirmation">Saved sites cleared</string>
3330
<string name="devSettingsUseSandBoxSurvey">Use Sandbox Survey</string>
3431
<string name="devEnableWebContentDebuggingTitle">Web Content Debugging</string>
3532
<string name="devEnableWebContentDebuggingByline">Enable WebView debugging in Chrome DevTools</string>
@@ -49,13 +46,17 @@
4946
<string name="devSettingsShowTabSwitcherAnimatedTile">Reset TabSwitcher Animated Tile</string>
5047
<string name="devSettingsShowTabSwitcherAnimatedSubtitle">Click here to reset the animated tile if you have previously dismissed it</string>
5148

52-
<!-- Tabs -->
53-
<string name="devSettingsScreenTabs">Tabs</string>
54-
<string name="devSettingsScreenTabsSubtitle">Create tabs for testing</string>
49+
<!-- Tabs and Saved Sites -->
50+
<string name="devSettingsScreenTabs">Tabs and Saved Sites</string>
51+
<string name="devSettingsScreenTabsSubtitle">Create tabs or bookmarks for testing</string>
5552
<string name="devSettingsTabsScreenHeader" instruction="Not translated">Current tab count: %1$d</string>
5653
<string name="devSettingsTabsScreenTabsToAdd">Tabs to add</string>
5754
<string name="devSettingsTabsScreenAddTabsButtonText">Add Tabs</string>
5855
<string name="devSettingsTabsScreenClearTabsButtonText">Clear Tabs</string>
56+
<string name="devSettingsTabsBookmarksScreenHeader" instruction="Not translated">Current bookmarks count: %1$d</string>
57+
<string name="devSettingsTabsScreenBookmarksToAdd">Bookmarks to add</string>
58+
<string name="devSettingsTabsScreenAddBookmarksButtonText">Add Bookmarks</string>
59+
<string name="devSettingsTabsScreenClearBookmarksButtonText">Clear Bookmarks</string>
5960

6061
<!-- Privacy Audit settings -->
6162
<string name="auditSettingsTitle">Audit Settings</string>

‎saved-sites/saved-sites-impl/src/main/java/com/duckduckgo/savedsites/impl/bookmarks/BookmarksActivity.kt

Lines changed: 18 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,8 @@ import java.util.Date
9797
import java.util.Locale
9898
import java.util.TimeZone
9999
import javax.inject.Inject
100+
import kotlinx.coroutines.flow.launchIn
101+
import kotlinx.coroutines.flow.onEach
100102
import kotlinx.coroutines.launch
101103

102104
@InjectWith(ActivityScope::class)
@@ -173,6 +175,7 @@ class BookmarksActivity : DuckDuckGoActivity(), BookmarksScreenPromotionPlugin.C
173175

174176
setupBookmarksRecycler()
175177
observeViewModel()
178+
observeItemsToDisplay()
176179
initializeSearchBar()
177180

178181
viewModel.fetchBookmarksAndFolders(getParentFolderId())
@@ -323,7 +326,8 @@ class BookmarksActivity : DuckDuckGoActivity(), BookmarksScreenPromotionPlugin.C
323326
private fun setupBookmarksRecycler() {
324327
bookmarksAdapter = BookmarksAdapter(
325328
viewModel,
326-
this,
329+
lifecycleOwner = this,
330+
dispatcherProvider = dispatchers,
327331
faviconManager,
328332
onBookmarkClick = { bookmark ->
329333
viewModel.onSelected(bookmark)
@@ -354,8 +358,6 @@ class BookmarksActivity : DuckDuckGoActivity(), BookmarksScreenPromotionPlugin.C
354358
private fun observeViewModel() {
355359
viewModel.viewState.observe(this) { viewState ->
356360
viewState?.let { state ->
357-
val items = state.sortedItems
358-
359361
if (state.sortingMode == MANUAL) {
360362
bookmarksAdapter.isReorderingEnabled = true
361363
itemTouchHelper.attachToRecyclerView(contentBookmarksBinding.recycler)
@@ -364,14 +366,8 @@ class BookmarksActivity : DuckDuckGoActivity(), BookmarksScreenPromotionPlugin.C
364366
itemTouchHelper.attachToRecyclerView(null)
365367
}
366368

367-
bookmarksAdapter.setItems(
368-
items,
369-
items.isEmpty() && getParentFolderId() == SavedSitesNames.BOOKMARKS_ROOT,
370-
false,
371-
)
372369
binding.searchMenu.isVisible =
373370
viewModel.viewState.value?.enableSearch == true || getParentFolderId() != SavedSitesNames.BOOKMARKS_ROOT
374-
exportMenuItem?.isEnabled = items.isNotEmpty()
375371
configurePromotionsContainer()
376372
}
377373
}
@@ -400,6 +396,18 @@ class BookmarksActivity : DuckDuckGoActivity(), BookmarksScreenPromotionPlugin.C
400396
}
401397
}
402398

399+
private fun observeItemsToDisplay() {
400+
viewModel.itemsToDisplay.onEach { items ->
401+
bookmarksAdapter.setItems(
402+
items,
403+
showEmptyHint = items.isEmpty() && getParentFolderId() == SavedSitesNames.BOOKMARKS_ROOT,
404+
showEmptySearchHint = false,
405+
detectMoves = true,
406+
)
407+
exportMenuItem?.isEnabled = items.isNotEmpty()
408+
}.launchIn(lifecycleScope)
409+
}
410+
403411
private fun launchSyncSettings() {
404412
val intent = globalActivityStarter.startIntent(this, SyncActivityWithEmptyParams)
405413
syncActivityLauncher.launch(intent)
@@ -490,7 +498,7 @@ class BookmarksActivity : DuckDuckGoActivity(), BookmarksScreenPromotionPlugin.C
490498
}
491499

492500
private fun initializeSearchBar() {
493-
searchListener = BookmarksQueryListener(viewModel, bookmarksAdapter)
501+
searchListener = BookmarksQueryListener(viewModel, bookmarksAdapter, dispatchers)
494502
if (bookmarksSortingFeature.self().isEnabled()) {
495503
binding.searchMenu.setOnClickListener {
496504
showSearchBar()

0 commit comments

Comments
 (0)