10000 Feature/automatic data clearing home screen shortcut fix (#413) · rzoro/Android@d3a6962 · GitHub
[go: up one dir, main page]
Skip to content

Commit d3a6962

Browse files
CDRussellCDRussell
authored
Feature/automatic data clearing home screen shortcut fix (duckduckgo#413)
* Move from IO scheduler to single-threaded executor for DB work This is to address the situation where we might have concurrent requests to open a new tab (one the blank, default tab and the other the one the user really wants to see). With the IO scheduler, there is the possibility of adding both, and then selecting the default tab as the one to show the user, resulting in the desired tab being relegated to the background. Moving to a single-threaded executor should let us better control the DB-related actions like adding and selecting a tab. There are likely even better ways to do this in future, but hopefully this will suffice for now. * Add RxJavaPlugin hook for single scheduler * Change web content to be hidden when ViewState is first initialized This should always have been the case. Leaving it init to showing the web view content can cause timing issues whereby an incoming intent could be processed and consumed too early, and thus leaves the intent liable to get "lost"; if the incoming intent is to load a URL and that load begins, but then shortly afterwards the data clearing kills all tabs, the URL the user wanted will not be shown to them.
1 parent ab4fbdf commit d3a6962

File tree

4 files changed

+27
-6
lines changed

4 files changed

+27
-6
lines changed

‎app/src/androidTest/java/com/duckduckgo/app/InstantSchedulersRule.kt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ class InstantSchedulersRule : TestRule {
4040
RxJavaPlugins.setIoSchedulerHandler { Schedulers.trampoline() }
4141
RxJavaPlugins.setComputationSchedulerHandler { Schedulers.trampoline() }
4242
RxJavaPlugins.setNewThreadSchedulerHandler { Schedulers.trampoline() }
43+
RxJavaPlugins.setSingleSchedulerHandler { Schedulers.trampoline() }
4344
RxAndroidPlugins.setMainThreadSchedulerHandler { Schedulers.trampoline() }
4445

4546
base.evaluate()

‎app/src/androidTest/java/com/duckduckgo/app/browser/BrowserViewModelTest.kt

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ import com.duckduckgo.app.tabs.model.TabRepository
2828
import com.nhaarman.mockitokotlin2.*
2929
import org.junit.After
3030
import org.junit.Assert.assertEquals
31+
import org.junit.Assert.assertTrue
3132
import org.junit.Before
3233
import org.junit.Rule
3334
import org.junit.Test
@@ -119,6 +120,11 @@ class BrowserViewModelTest {
119120
assertEquals(DisplayMessage(R.string.fireDataCleared), commandCaptor.lastValue)
120121
}
121122

123+
@Test
124+
fun whenViewStateCreatedThenWebViewContentShouldBeHidden() {
125+
assertTrue(testee.viewState.value!!.hideWebContent)
126+
}
127+
122128
companion object {
123129
const val TAB_ID = "TAB_ID"
124130
}

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ class BrowserViewModel(
3939
) : ViewModel() {
4040

4141
data class ViewState(
42-
val hideWebContent: Boolean = false
42+
val hideWebContent: Boolean = true
4343
)
4444

4545
sealed class Command {
@@ -81,6 +81,7 @@ class BrowserViewModel(
8181

8282
fun onTabsUpdated(tabs: List<TabEntity>?) {
8383
if (tabs == null || tabs.isEmpty()) {
84+
Timber.i("Tabs list is null or empty; adding default tab")
8485
tabRepository.add(isDefaultTab = true)
8586
return
8687
}

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

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import androidx.lifecycle.MutableLiveData
2323
import com.duckduckgo.app.global.model.Site
2424
import com.duckduckgo.app.global.model.SiteFactory
2525
import com.duckduckgo.app.tabs.db.TabsDao
26+
import io.reactivex.Scheduler
2627
import io.reactivex.schedulers.Schedulers
2728
import timber.log.Timber
2829
import java.util.*
@@ -57,7 +58,7 @@ class TabDataRepository @Inject constructor(private val tabsDao: TabsDao, privat
5758

5859
override fun add(tabId: String, data: MutableLiveData<Site>, isDefaultTab: Boolean) {
5960
siteData[tabId] = data
60-
Schedulers.io().scheduleDirect {
61+
databaseExecutor().scheduleDirect {
6162

6263
Timber.i("Trying to add tab, is default? $isDefaultTab, current tabs count: ${tabsDao.tabs().size}")
6364

@@ -73,7 +74,7 @@ class TabDataRepository @Inject constructor(private val tabsDao: TabsDao, privat
7374
}
7475

7576
override fun addNewTabAfterExistingTab(url: String?, tabId: String) {
76-
Schedulers.io().scheduleDirect {
77+
databaseExecutor().scheduleDirect {
7778
val position = tabsDao.tab(tabId)?.position ?: -1
7879
val uri = Uri.parse(url)
7980
val title = uri.host?.removePrefix("www.") ?: url
@@ -83,7 +84,7 @@ class TabDataRepository @Inject constructor(private val tabsDao: TabsDao, privat
8384
}
8485

8586
override fun update(tabId: String, site: Site?) {
86-
Schedulers.io().scheduleDirect {
87+
databaseExecutor().scheduleDirect {
8788
val position = tabsDao.tab(tabId)?.position ?: 0
8889
val tab = TabEntity(tabId, site?.url, site?.title, true, position)
8990
tabsDao.updateTab(tab)
@@ -102,7 +103,7 @@ class TabDataRepository @Inject constructor(private val tabsDao: TabsDao, privat
102103
}
103104

104105
override fun delete(tab: TabEntity) {
105-
Schedulers.io().scheduleDirect {
106+
databaseExecutor().scheduleDirect {
106107
tabsDao.deleteTabAndUpdateSelection(tab)
107108
}
108109
siteData.remove(tab.tabId)
@@ -115,9 +116,21 @@ class TabDataRepository @Inject constructor(private val tabsDao: TabsDao, privat
115116
}
116117

117118
override fun select(tabId: String) {
118-
Schedulers.io().scheduleDirect {
119+
databaseExecutor().scheduleDirect {
119120
val selection = TabSelectionEntity(tabId = tabId)
120121
tabsDao.insertTabSelection(selection)
121122
}
122123
}
124+
125+
/**
126+
* In this class we typically delegate DB work to a scheduler
127+
* Historically, this was the IO scheduler, which can use multiple threads to do the work
128+
* However, this presented the possibility of race conditions. One such case was when the blank tab was shown to the user and the URL
129+
* they actually wanted was opened in a background tab.
130+
*
131+
* While there are likely even better ways of doing this, moving to a single-threaded executor is likely good enough to fix this for now
132+
*/
133+
private fun databaseExecutor(): Scheduler {
134+
return Schedulers.single()
135+
}
123136
}

0 commit comments

Comments
 (0)
0