Skip to content

Commit 44f8f44

Browse files
CrisBarreiroCrisBarreiro
authored
Fix tab not being inserted when opening in new tab (#6178)
Task/Issue URL: https://app.asana.com/1/137249556945/project/1203249713006009/task/1210059188286306?focus=true ### Description Fix Duck Player video not being shown because of: 1. In TabsRepository we were using postValue instead of setValue, and therefore in some cases the new value was not emitted in time 2. This X post generates a temporary intermediate tab when clicking the link, that then launches the Duck Player video on a new tab. This intermediate tab has no URL, and therefore when the Duck Player tab is inserted via addAndSelectTab (which removes tabs with null URL as the first step), insertion fails because of a foreign key violation (sourceTabId doesn't exist anymore) ### Steps to test this PR _Feature 1_ - [x] Go to Feature flags inventory and make sure both `tabInsertionFixes` and `swipingTabs` are enabled - [x] Restart the app - [x] Set Duck Player to "Always" - [x] Open this X post: https://x.com/tdinh_me/status/1909223312053657730 - [x] Click on the YouTube link shared in the X post - [x] Check the video is opened in a new tab _Feature 2_ - [x] Go to Feature flags inventory and make sure both `tabInsertionFixes` is enabled and `swipingTabs` is disabled - [x] Restart the app - [x] Set Duck Player to "Always" - [x] Open this X post: https://x.com/tdinh_me/status/1909223312053657730 - [x] Click on the YouTube link shared in the X post - [x] Check the video is opened in a new tab
1 parent 3595341 commit 44f8f44

File tree

6 files changed

+123
-15
lines changed

6 files changed

+123
-15
lines changed

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2970,6 +2970,7 @@ class BrowserTabViewModel @Inject constructor(
29702970
}
29712971

29722972
override fun openMessageInNewTab(message: Message) {
2973+
command.value = GenerateWebViewPreviewImage
29732974
command.value = OpenMessageInNewTab(message, tabId)
29742975
}
29752976

‎app/src/main/java/com/duckduckgo/app/tabs/TabFeatureFlags.kt

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,4 +32,7 @@ interface TabManagerFeatureFlags {
3232

3333
@Toggle.DefaultValue(DefaultFeatureValue.FALSE)
3434
fun multiSelection(): Toggle
35+
36+
@Toggle.DefaultValue(DefaultFeatureValue.TRUE)
37+
fun tabInsertionFixes(): Toggle
3538
}

‎app/src/main/java/com/duckduckgo/app/tabs/db/TabsDao.kt

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ import dagger.SingleInstanceIn
2626
import java.time.LocalDateTime
2727
import kotlin.math.max
2828
import kotlinx.coroutines.flow.Flow
29+
import logcat.logcat
2930

3031
@Dao
3132
@SingleInstanceIn(AppScope::class)
@@ -160,10 +161,24 @@ abstract class TabsDao {
160161
abstract fun incrementPositionStartingAt(position: Int)
161162

162163
@Transaction
163-
open fun addAndSelectTab(tab: TabEntity) {
164-
deleteBlankTabs()
165-
insertTab(tab)
166-
insertTabSelection(TabSelectionEntity(tabId = tab.tabId))
164+
open fun addAndSelectTab(tab: TabEntity, updateIfBlankParent: Boolean = false) {
165+
try {
166+
val parent = tab.sourceTabId?.let { tab(it) }
167+
val newTab = if (updateIfBlankParent && parent != null && parent.url == null) {
168+
/*
169+
* If the parent tab is blank, we need to update the parent tab with the previous
170+
* one. Otherwise, foreign key constrains won't be met
171+
*/
172+
tab.copy(sourceTabId = parent.sourceTabId, position = parent.position)
173+
} else {
174+
tab
175+
}
176+
deleteBlankTabs()
177+
insertTab(newTab)
178+
insertTabSelection(TabSelectionEntity(tabId = newTab.tabId))
179+
} catch (e: Exception) {
180+
logcat { "Error adding and selecting tab: $e" }
181+
}
167182
}
168183

169184
@Transaction

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

Lines changed: 45 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -93,12 +93,21 @@ class TabDataRepository @Inject constructor(
9393

9494
private var purgeDeletableTabsJob = ConflatedJob()
9595

96+
private var tabInsertionFixesFlag: Boolean? = null
97+
9698
override suspend fun add(
9799
url: String?,
98100
skipHome: Boolean,
99101
): String = withContext(dispatchers.io()) {
100102
val tabId = generateTabId()
101-
add(tabId, buildSiteData(url, tabId), skipHome = skipHome, isDefaultTab = false)
103+
val flag = getAndCacheTabInsertionFixesFlag()
104+
val siteData = if (flag) {
105+
buildSiteData(url, tabId)
106+
} else {
107+
buildSiteDataSync(url, tabId)
108+
}
109+
add(tabId, siteData, skipHome = skipHome, isDefaultTab = false, updateIfBlankParent = flag)
110+
102111
return@withContext tabId
103112
}
104113

@@ -108,34 +117,52 @@ class TabDataRepository @Inject constructor(
108117
sourceTabId: String,
109118
): String = withContext(dispatchers.io()) {
110119
val tabId = generateTabId()
111-
120+
val flag = getAndCacheTabInsertionFixesFlag()
121+
val siteData = if (flag) {
122+
buildSiteData(url, tabId)
123+
} else {
124+
buildSiteDataSync(url, tabId)
125+
}
112126
add(
113127
tabId = tabId,
114-
data = buildSiteData(url, tabId),
128+
data = siteData,
115129
skipHome = skipHome,
116130
isDefaultTab = false,
117131
sourceTabId = sourceTabId,
132+
updateIfBlankParent = flag,
118133
)
119134

120135
return@withContext tabId
121136
}
122137

123138
override suspend fun addDefaultTab(): String = withContext(dispatchers.io()) {
124139
val tabId = generateTabId()
125-
140+
val flag = getAndCacheTabInsertionFixesFlag()
141+
val siteData = if (flag) {
142+
buildSiteData(url = null, tabId = tabId)
143+
} else {
144+
buildSiteDataSync(url = null, tabId)
145+
}
126146
add(
127147
tabId = tabId,
128-
data = buildSiteData(url = null, tabId = tabId),
148+
data = siteData,
129149
skipHome = false,
130150
isDefaultTab = true,
151+
updateIfBlankParent = flag,
131152
)
132153

133154
return@withContext tabId
134155
}
135156

157+
private suspend fun getAndCacheTabInsertionFixesFlag(): Boolean {
158+
return tabInsertionFixesFlag ?: withContext(dispatchers.io()) {
159+
tabManagerFeatureFlags.tabInsertionFixes().isEnabled()
160+
}.also { tabInsertionFixesFlag = it }
161+
}
162+
136163
private fun generateTabId() = UUID.randomUUID().toString()
137164

138-
private fun buildSiteData(url: String?, tabId: String): MutableLiveData<Site> {
165+
private fun buildSiteDataSync(url: String?, tabId: String): MutableLiveData<Site> {
139166
val data = MutableLiveData<Site>()
140167
url?.let {
141168
val siteMonitor = siteFactory.buildSite(url = it, tabId = tabId)
@@ -144,12 +171,23 @@ class TabDataRepository @Inject constructor(
144171
return data
145172
}
146173

174+
private suspend fun buildSiteData(url: String?, tabId: String): MutableLiveData<Site> {
175+
val data = MutableLiveData<Site>()
176+
url ?: return data
177+
val siteMonitor = siteFactory.buildSite(url = url, tabId = tabId)
178+
withContext(dispatchers.main()) {
179+
data.value = siteMonitor
180+
}
181+
return data
182+
}
183+
147184
private fun add(
148185
tabId: String,
149186
data: MutableLiveData<Site>,
150187
skipHome: Boolean,
151188
isDefaultTab: Boolean,
152189
sourceTabId: String? = null,
190+
updateIfBlankParent: Boolean = false,
153191
) {
154192
siteData[tabId] = data
155193
databaseExecutor().scheduleDirect {
@@ -178,6 +216,7 @@ class TabDataRepository @Inject constructor(
178216
position = position,
179217
sourceTabId = sourceTabId,
180218
),
219+
updateIfBlankParent = updateIfBlankParent,
181220
)
182221
}
183222
}

‎app/src/test/java/com/duckduckgo/tabs/db/TabsDaoTest.kt

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -465,4 +465,36 @@ class TabsDaoTest {
465465
assertNotNull(testee.tab(tab3.tabId))
466466
assertEquals(tab3, testee.selectedTab())
467467
}
468+
469+
@Test
470+
fun whenAddAndSelectWithBlankParentAndUpdateIfBlankParentTrueThenUpdateTabBeforeInserting() {
471+
val zero = TabEntity(
472+
"TAB_ID0",
473+
position = 0,
474+
sourceTabId = null,
475+
url = "http://example.com",
476+
)
477+
val first = TabEntity(
478+
"TAB_ID1",
479+
position = 1,
480+
sourceTabId = "TAB_ID0",
481+
url = null,
482+
)
483+
val second = TabEntity(
484+
"TAB_ID2",
485+
position = 2,
486+
sourceTabId = "TAB_ID1",
487+
url = "http://example.com",
488+
)
489+
490+
testee.insertTab(zero)
491+
testee.insertTab(first)
492+
testee.addAndSelectTab(second, updateIfBlankParent = true)
493+
494+
assertFalse(testee.tabs().contains(first))
495+
val storedTab = testee.tab("TAB_ID2")
496+
assertEquals("TAB_ID0", storedTab?.sourceTabId)
497+
assertEquals(1, storedTab?.position)
498+
assertEquals(storedTab, testee.selectedTab())
499+
}
468500
}

‎app/src/test/java/com/duckduckgo/tabs/model/TabDataRepositoryTest.kt

Lines changed: 23 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -123,10 +123,28 @@ class TabDataRepositoryTest {
123123
testee.add("http://www.example.com")
124124

125125
val captor = argumentCaptor<TabEntity>()
126-
verify(mockDao).addAndSelectTab(captor.capture())
126+
verify(mockDao).addAndSelectTab(captor.capture(), any())
127127
assertTrue(captor.firstValue.viewed)
128128
}
129129

130+
@Test
131+
fun whenTabAddAndTabInsertionFixesOnThenAddAndSelectIsCalledWithUpdateIfBlankParent() = runTest {
132+
val testee = tabDataRepository()
133+
tabManagerFeatureFlags.tabInsertionFixes().setRawStoredState(State(enable = true))
134+
testee.add("http://www.example.com")
135+
136+
verify(mockDao).addAndSelectTab(any(), eq(true))
137+
}
138+
139+
@Test
140+
fun whenTabAddAndTabInsertionFixesOffThenAddAndSelectIsCalledWithUpdateIfBlankParentFalse() = runTest {
141+
val testee = tabDataRepository()
142+
tabManagerFeatureFlags.tabInsertionFixes().setRawStoredState(State(enable = false))
143+
testee.add("http://www.example.com")
144+
145+
verify(mockDao).addAndSelectTab(any(), eq(false))
146+
}
147+
130148
@Test
131149
fun whenTabUpdatedAfterOpenInBackgroundThenViewedIsTrue() = runTest {
132150
val testee = tabDataRepository()
@@ -170,7 +188,7 @@ class TabDataRepositoryTest {
170188
fun whenAddCalledThenTabAddedAndSelectedAndBlankSiteDataCreated() = runTest {
171189
val testee = tabDataRepository()
172190
val createdId = testee.add()
173-
verify(mockDao).addAndSelectTab(any())
191+
verify(mockDao).addAndSelectTab(any(), any())
174192
assertNotNull(testee.retrieveSiteData(createdId))
175193
}
176194

@@ -179,7 +197,7 @@ class TabDataRepositoryTest {
179197
val testee = tabDataRepository()
180198
val url = "http://example.com"
181199
val createdId = testee.add(url)
182-
verify(mockDao).addAndSelectTab(any())
200+
verify(mockDao).addAndSelectTab(any(), any())
183201
assertNotNull(testee.retrieveSiteData(createdId))
184202
assertEquals(url, testee.retrieveSiteData(createdId).value!!.url)
185203
}
@@ -229,7 +247,7 @@ class TabDataRepositoryTest {
229247
testee.add("http://www.example.com")
230248

231249
val captor = argumentCaptor<TabEntity>()
232-
verify(mockDao).addAndSelectTab(captor.capture())
250+
verify(mockDao).addAndSelectTab(captor.capture(), any())
233251
assertTrue(captor.firstValue.position == 0)
234252
}
235253

@@ -245,7 +263,7 @@ class TabDataRepositoryTest {
245263
testee.add("http://www.example.com")
246264

247265
val captor = argumentCaptor<TabEntity>()
248-
verify(mockDao).addAndSelectTab(captor.capture())
266+
verify(mockDao).addAndSelectTab(captor.capture(), any())
249267
assertTrue(captor.firstValue.position == 1)
250268
}
251269

0 commit comments

Comments
 (0)