Skip to content

Commit f04c7af

Browse files
0nko0nko
authored
Tab swiping: Improve state handling in the pager adapter (#6057)
Task/Issue URL: https://app.asana.com/1/137249556945/project/1207418217763355/task/1210226097518220?focus=true ### Description This PR updates the `TabPagerAdapter` to keep all data required for tab fragment creation within the adapter and avoid DB lookups. Whenever tabs are updated in the DB, the adapter data is updated. It requires to keep some extra data besides the tab ID but eliminates the chance for a race condition when the adapter requests a new fragment and a tab is deleted before it has a chance get data. ### Steps to test this PR Smoke testing tab swiping should be enough, as the race condition is not easy to reproduce. --- - To see the specific tasks where the Asana app for GitHub is being used, see below: - https://app.asana.com/0/0/1208271988740971
1 parent 930fa35 commit f04c7af

File tree

6 files changed

+50
-170
lines changed

6 files changed

+50
-170
lines changed

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

Lines changed: 6 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@ import com.duckduckgo.app.browser.omnibar.model.OmnibarPosition.BOTTOM
5555
import com.duckduckgo.app.browser.omnibar.model.OmnibarPosition.TOP
5656
import com.duckduckgo.app.browser.shortcut.ShortcutBuilder
5757
import com.duckduckgo.app.browser.tabs.TabManager
58+
import com.duckduckgo.app.browser.tabs.TabManager.TabModel
5859
import com.duckduckgo.app.browser.tabs.adapter.TabPagerAdapter
5960
import com.duckduckgo.app.browser.webview.RealMaliciousSiteBlockerWebViewIntegration
6061
import com.duckduckgo.app.di.AppCoroutineScope
@@ -105,7 +106,6 @@ import kotlinx.coroutines.Job
105106
import kotlinx.coroutines.delay
106107
import kotlinx.coroutines.flow.collectLatest
107108
import kotlinx.coroutines.launch
108-
import kotlinx.coroutines.runBlocking
109109
import timber.log.Timber
110110

111111
// open class so that we can test BrowserApplicationStateInfo
@@ -178,7 +178,10 @@ open class BrowserActivity : DuckDuckGoActivity() {
178178
private var currentTab: BrowserTabFragment?
179179
get() {
180180
return if (swipingTabsFeature.isEnabled) {
181-
tabPagerAdapter.currentFragment
181+
val selectedTabId = tabManager.getSelectedTabId()
182+
supportFragmentManager.fragments
183+
.filterIsInstance<BrowserTabFragment>()
184+
.firstOrNull { it.tabId == selectedTabId }
182185
} else {
183186
_currentTab
184187
}
@@ -206,9 +209,6 @@ open class BrowserActivity : DuckDuckGoActivity() {
206209
fragmentManager = supportFragmentManager,
207210
lifecycleOwner = this,
208211
activityIntent = intent,
209-
getSelectedTabId = tabManager::getSelectedTabId,
210-
getTabById = ::getTabById,
211-
requestAndWaitForNewTab = ::requestAndWaitForNewTab,
212212
swipingTabsFeature = swipingTabsFeature,
213213
)
214214
}
@@ -1002,18 +1002,10 @@ open class BrowserActivity : DuckDuckGoActivity() {
10021002
}
10031003
}
10041004

1005-
private fun onTabsUpdated(updatedTabIds: List<String>) {
1005+
private fun onTabsUpdated(updatedTabIds: List<TabModel>) {
10061006
tabPagerAdapter.onTabsUpdated(updatedTabIds)
10071007
}
10081008

1009-
private fun getTabById(tabId: String): TabEntity? = runBlocking {
1010-
return@runBlocking tabManager.getTabById(tabId)
1011-
}
1012-
1013-
private fun requestAndWaitForNewTab(): TabEntity = runBlocking {
1014-
return@runBlocking tabManager.requestAndWaitForNewTab()
1015-
}
1016-
10171009
fun launchNewTab(query: String? = null, sourceTabId: String? = null, skipHome: Boolean = false) {
10181010
lifecycleScope.launch {
10191011
if (swipingTabsFeature.isEnabled) {

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

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ import com.duckduckgo.app.browser.defaultbrowsing.prompts.DefaultBrowserPromptsE
3434
import com.duckduckgo.app.browser.defaultbrowsing.prompts.DefaultBrowserPromptsExperiment.Command.OpenSystemDefaultBrowserDialog
3535
import com.duckduckgo.app.browser.defaultbrowsing.prompts.DefaultBrowserPromptsExperiment.SetAsDefaultActionTrigger
3636
import com.duckduckgo.app.browser.omnibar.OmnibarEntryConverter
37+
import com.duckduckgo.app.browser.tabs.TabManager.TabModel
3738
import com.duckduckgo.app.fire.DataClearer
3839
import com.duckduckgo.app.generalsettings.showonapplaunch.ShowOnAppLaunchFeature
3940
import com.duckduckgo.app.generalsettings.showonapplaunch.ShowOnAppLaunchOptionHandler
@@ -145,12 +146,12 @@ class BrowserViewModel @Inject constructor(
145146
.distinctUntilChanged()
146147
.debounce(100)
147148

148-
val tabsFlow: Flow<List<String>> = tabRepository.flowTabs
149-
.map { tabs -> tabs.map { tab -> tab.tabId } }
149+
val tabsFlow: Flow<List<TabModel>> = tabRepository.flowTabs
150+
.map { tabs -> tabs.map { tab -> TabModel(tab.tabId, tab.url, tab.skipHome) } }
150151
.distinctUntilChanged()
151152

152153
val selectedTabIndex: Flow<Int> = combine(tabsFlow, selectedTabFlow) { tabs, selectedTab ->
153-
tabs.indexOf(selectedTab)
154+
tabs.indexOfFirst { it.tabId == selectedTab }
154155
}.filterNot { it == -1 }
155156

156157
private var dataClearingObserver = Observer<ApplicationClearDataState> { state ->

‎app/src/main/java/com/duckduckgo/app/browser/tabs/DefaultTabManager.kt

Lines changed: 12 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -18,20 +18,13 @@ package com.duckduckgo.app.browser.tabs
1818

1919
import com.duckduckgo.app.browser.SkipUrlConversionOnNewTabFeature
2020
import com.duckduckgo.app.browser.omnibar.OmnibarEntryConverter
21-
import com.duckduckgo.app.browser.tabs.TabManager.Companion.NEW_TAB_CREATION_TIMEOUT_LIMIT
21+
import com.duckduckgo.app.browser.tabs.TabManager.TabModel
2222
import com.duckduckgo.app.tabs.model.TabEntity
2323
import com.duckduckgo.app.tabs.model.TabRepository
2424
import com.duckduckgo.common.utils.DispatcherProvider
2525
import com.duckduckgo.di.scopes.ActivityScope
2626
import com.squareup.anvil.annotations.ContributesBinding
2727
import javax.inject.Inject
28-
import kotlin.time.Duration.Companion.seconds
29-
import kotlinx.coroutines.FlowPreview
30-
import kotlinx.coroutines.TimeoutCancellationException
31-
import kotlinx.coroutines.flow.catch
32-
import kotlinx.coroutines.flow.firstOrNull
33-
import kotlinx.coroutines.flow.timeout
34-
import kotlinx.coroutines.flow.transformWhile
3528
import kotlinx.coroutines.withContext
3629
import timber.log.Timber
3730

@@ -41,15 +34,20 @@ interface TabManager {
4134
const val NEW_TAB_CREATION_TIMEOUT_LIMIT = 2 // seconds
4235
}
4336

44-
fun registerCallbacks(onTabsUpdated: (List<String>) -> Unit)
37+
fun registerCallbacks(onTabsUpdated: (List<TabModel>) -> Unit)
4538
fun getSelectedTabId(): String?
4639
fun onSelectedTabChanged(tabId: String)
4740

48-
suspend fun onTabsChanged(updatedTabIds: List<String>)
41+
suspend fun onTabsChanged(updatedTabIds: List<TabModel>)
4942
suspend fun switchToTab(tabId: String)
50-
suspend fun requestAndWaitForNewTab(): TabEntity
5143
suspend fun openNewTab(query: String? = null, sourceTabId: String? = null, skipHome: Boolean = false): String
5244
suspend fun getTabById(tabId: String): TabEntity?
45+
46+
data class TabModel(
47+
val tabId: String,
48+
val url: String?,
49+
val skipHome: Boolean,
50+
)
5351
}
5452

5553
@ContributesBinding(ActivityScope::class)
@@ -59,10 +57,10 @@ class DefaultTabManager @Inject constructor(
5957
private val queryUrlConverter: OmnibarEntryConverter,
6058
private val skipUrlConversionOnNewTabFeature: SkipUrlConversionOnNewTabFeature,
6159
) : TabManager {
62-
private lateinit var onTabsUpdated: (List<String>) -> Unit
60+
private lateinit var onTabsUpdated: (List<TabModel>) -> Unit
6361
private var selectedTabId: String? = null
6462

65-
override fun registerCallbacks(onTabsUpdated: (List<String>) -> Unit) {
63+
override fun registerCallbacks(onTabsUpdated: (List<TabModel>) -> Unit) {
6664
this.onTabsUpdated = onTabsUpdated
6765
}
6866

@@ -72,7 +70,7 @@ class DefaultTabManager @Inject constructor(
7270
selectedTabId = tabId
7371
}
7472

75-
override suspend fun onTabsChanged(updatedTabIds: List<String>) {
73+
override suspend fun onTabsChanged(updatedTabIds: List<TabModel>) {
7674
onTabsUpdated(updatedTabIds)
7775

7876
if (updatedTabIds.isEmpty()) {
@@ -83,29 +81,6 @@ class DefaultTabManager @Inject constructor(
8381
}
8482
}
8583

86-
@OptIn(FlowPreview::class)
87-
override suspend fun requestAndWaitForNewTab(): TabEntity = withContext(dispatchers.io()) {
88-
val tabId = openNewTab()
89-
return@withContext tabRepository.flowTabs
90-
.transformWhile { result ->
91-
result.firstOrNull { it.tabId == tabId }?.let { entity ->
92-
emit(entity)
93-
return@transformWhile false // stop after finding the tab
94-
}
95-
return@transformWhile true // continue looking if not found
96-
}
97-
.timeout(NEW_TAB_CREATION_TIMEOUT_LIMIT.seconds)
98-
.catch { e ->
99-
if (e is TimeoutCancellationException) {
100-
// timeout expired and the new tab was not found
101-
throw IllegalStateException("A new tab failed to be created within $NEW_TAB_CREATION_TIMEOUT_LIMIT second")
102-
} else {
103-
throw e
104-
}
105-
}
106-
.firstOrNull() ?: throw IllegalStateException("Tabs flow completed before finding the new tab")
107-
}
108-
10984
override suspend fun switchToTab(tabId: String) = withContext(dispatchers.io()) {
11085
if (tabId != tabRepository.getSelectedTab()?.tabId) {
11186
tabRepository.select(tabId)

‎app/src/main/java/com/duckduckgo/app/browser/tabs/adapter/FragmentStateAdapter.java

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -535,7 +535,6 @@ private void removeFragment(long itemId) {
535535

536536
if (!fragment.isAdded()) {
537537
mFragments.remove(itemId);
538-
Timber.d("$$$ Fragment (not added) removed: %s", itemId);
539538
return;
540539
}
541540

@@ -557,7 +556,6 @@ private void removeFragment(long itemId) {
557556
try {
558557
mFragmentManager.beginTransaction().remove(fragment).commitNow();
559558
mFragments.remove(itemId);
560-
Timber.d("$$$ Fragment removed (after transaction): %s", itemId);
561559
} finally {
562560
mFragmentEventDispatcher.dispatchPostEvents(onPost);
563561
}

‎app/src/main/java/com/duckduckgo/app/browser/tabs/adapter/TabPagerAdapter.kt

Lines changed: 25 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -26,40 +26,32 @@ import androidx.recyclerview.widget.DiffUtil
2626
import androidx.recyclerview.widget.RecyclerView
2727
import com.duckduckgo.app.browser.BrowserActivity
2828
import com.duckduckgo.app.browser.BrowserTabFragment
29-
import com.duckduckgo.app.tabs.model.TabEntity
29+
import com.duckduckgo.app.browser.tabs.TabManager.TabModel
3030
import com.duckduckgo.common.ui.tabs.SwipingTabsFeatureProvider
3131

3232
class TabPagerAdapter(
3333
lifecycleOwner: LifecycleOwner,
34-
private val fragmentManager: FragmentManager,
34+
fragmentManager: FragmentManager,
3535
private val activityIntent: Intent?,
36-
private val getTabById: (String) -> TabEntity?,
37-
private val requestAndWaitForNewTab: () -> TabEntity,
38-
private val getSelectedTabId: () -> String?,
3936
swipingTabsFeature: SwipingTabsFeatureProvider,
4037
) : FragmentStateAdapter(fragmentManager, lifecycleOwner.lifecycle, swipingTabsFeature) {
41-
private val tabIds = mutableListOf<String>()
38+
private val tabs = mutableListOf<TabModel>()
4239
private var messageForNewFragment: Message? = null
4340

44-
override fun getItemCount() = tabIds.size
41+
override fun getItemCount() = tabs.size
4542

4643
override fun getItemId(position: Int): Long {
47-
return if (position >= 0 && position < tabIds.size) {
48-
tabIds[position].hashCode().toLong()
44+
return if (position >= 0 && position < tabs.size) {
45+
tabs[position].tabId.hashCode().toLong()
4946
} else {
5047
RecyclerView.NO_ID
5148
}
5249
}
5350

54-
override fun containsItem(itemId: Long) = tabIds.any { it.hashCode().toLong() == itemId }
55-
56-
val currentFragment: BrowserTabFragment?
57-
get() = fragmentManager.fragments
58-
.filterIsInstance<BrowserTabFragment>()
59-
.firstOrNull { it.tabId == getSelectedTabId() }
51+
override fun containsItem(itemId: Long) = tabs.any { it.tabId.hashCode().toLong() == itemId }
6052

6153
override fun createFragment(position: Int): Fragment {
62-
val tab = getTabById(tabIds[position]) ?: requestAndWaitForNewTab()
54+
val tab = tabs[position]
6355
val isExternal = activityIntent?.getBooleanExtra(BrowserActivity.LAUNCH_FROM_EXTERNAL_EXTRA, false) == true
6456

6557
return if (messageForNewFragment != null) {
@@ -78,31 +70,38 @@ class TabPagerAdapter(
7870
}
7971

8072
@SuppressLint("NotifyDataSetChanged")
81-
fun onTabsUpdated(newTabs: List<String>) {
82-
val diff = DiffUtil.calculateDiff(PagerDiffUtil(tabIds, newTabs))
83-
diff.dispatchUpdatesTo(this)
84-
tabIds.clear()
85-
tabIds.addAll(newTabs)
73+
fun onTabsUpdated(newTabs: List<TabModel>) {
74+
if (tabs.map { it.tabId } != newTabs.map { it.tabId }) {
75+
// we only want to notify the adapter if the tab IDs change
76+
val diff = DiffUtil.calculateDiff(PagerDiffUtil(tabs, newTabs))
77+
tabs.clear()
78+
tabs.addAll(newTabs)
79+
diff.dispatchUpdatesTo(this)
80+
} else {
81+
// the state of tabs is managed separately, so we don't need to notify the adapter, but we need URL and skipHome to create new fragments
82+
tabs.clear()
83+
tabs.addAll(newTabs)
84+
}
8685
}
8786

8887
fun getTabIdAtPosition(position: Int): String? {
89-
return if (position < tabIds.size) {
90-
tabIds[position]
88+
return if (position < tabs.size) {
89+
tabs[position].tabId
9190
} else {
9291
null
9392
}
9493
}
9594

9695
inner class PagerDiffUtil(
97-
private val oldList: List<String>,
98-
private val newList: List<String>,
96+
private val oldList: List<TabModel>,
97+
private val newList: List<TabModel>,
9998
) : DiffUtil.Callback() {
10099
override fun getOldListSize() = oldList.size
101100

102101
override fun getNewListSize() = newList.size
103102

104103
override fun areItemsTheSame(oldItemPosition: Int, newItemPosition: Int): Boolean {
105-
return oldList[oldItemPosition] == newList[newItemPosition]
104+
return oldList[oldItemPosition].tabId == newList[newItemPosition].tabId
106105
}
107106

108107
override fun areContentsTheSame(oldItemPosition: Int, newItemPosition: Int): Boolean {

0 commit comments

Comments
 (0)