Issue #753 - Downloads that open in a new tab leave user on an empty tab

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

@@ -121,16 +121,18 @@ class BrowserViewModelTest {
 
     @Test
     fun whenNewTabRequestedThenTabAddedToRepository() = runBlocking<Unit> {
+        whenever(mockTabRepository.liveSelectedTab).doReturn(MutableLiveData())
         testee.onNewTabRequested()
-        verify(mockTabRepository).add()
+        verify(mockTabRepository).addWithSource()
     }
 
     @Test
     fun whenOpenInNewTabRequestedThenTabAddedToRepository() = runBlocking<Unit> {
         val url = "http://example.com"
         whenever(mockOmnibarEntryConverter.convertQueryToUrl(url)).thenReturn(url)
+        whenever(mockTabRepository.liveSelectedTab).doReturn(MutableLiveData())
         testee.onOpenInNewTabRequested(url)
-        verify(mockTabRepository).add(url)
+        verify(mockTabRepository).addWithSource(url)
     }
 
     @Test

app/src/androidTest/java/com/duckduckgo/app/global/db/AppDatabaseTest.kt

@@ -281,6 +281,11 @@ class AppDatabaseTest {
         return stage
     }
 
+    @Test
+    fun whenMigratingFromVersion23To24ThenValidationSucceeds() {
+        createDatabaseAndMigrate(23, 24, migrationsProvider.MIGRATION_23_TO_24)
+    }
+
     private fun createDatabase(version: Int) {
         testHelper.createDatabase(TEST_DB_NAME, version).close()
     }

app/src/androidTest/java/com/duckduckgo/app/tabs/model/TabDataRepositoryTest.kt

@@ -297,6 +297,39 @@ class TabDataRepositoryTest {
         db.close()
     }
 
+    @Test
+    fun whenAddWithSourceEnsureTabEntryContainsExpectedSourceId() = runBlocking<Unit> {
+        val db = createDatabase()
+        val dao = db.tabsDao()
+        val sourceTab = TabEntity(tabId = "sourceId", url = "http://www.example.com", position = 0)
+        dao.addAndSelectTab(sourceTab)
+
+        testee = TabDataRepository(dao, SiteFactory(mockPrivacyPractices, mockEntityLookup), mockWebViewPreviewPersister, useOurAppDetector)
+
+        val addedTabId = testee.addWithSource("http://www.example.com", skipHome = false, isDefaultTab = false)
+        val addedTab = testee.liveSelectedTab.blockingObserve()
+        assertEquals(addedTabId, addedTab?.tabId)
+        assertEquals(addedTab?.sourceTabId, sourceTab.tabId)
+    }
+
+    @Test
+    fun whenDeleteCurrentTabAndSelectSourceLiveSelectedTabReturnsToSourceTab() = runBlocking<Unit> {
+        val db = createDatabase()
+        val dao = db.tabsDao()
+        val sourceTab = TabEntity(tabId = "sourceId", url = "http://www.example.com", position = 0)
+        val tabToDelete = TabEntity(tabId = "tabToDeleteId", url = "http://www.example.com", position = 1, sourceTabId = "sourceId")
+        dao.addAndSelectTab(sourceTab)
+        dao.addAndSelectTab(tabToDelete)
+
+        testee = TabDataRepository(dao, SiteFactory(mockPrivacyPractices, mockEntityLookup), mockWebViewPreviewPersister, useOurAppDetector)
+
+        var currentSelectedTabId = testee.liveSelectedTab.blockingObserve()?.tabId
+        assertEquals(currentSelectedTabId, tabToDelete.tabId)
+        testee.deleteCurrentTabAndSelectSource()
+        currentSelectedTabId = testee.liveSelectedTab.blockingObserve()?.tabId
+        assertEquals(currentSelectedTabId, sourceTab.tabId)
+    }
+
     private fun createDatabase(): AppDatabase {
         return Room.inMemoryDatabaseBuilder(InstrumentationRegistry.getInstrumentation().targetContext, AppDatabase::class.java)
             .allowMainThreadQueries()

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

@@ -1107,10 +1107,19 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope, DaxDialogLi
         }
     }
 
+    private fun closeAndReturnToSourceIfBlankTab() {
+        if (viewModel.url == null) {
+            launch {
+                viewModel.closeAndSelectSourceTab()
+            }
+        }
+    }
+
     private fun createDownloadListener(): FileDownloadListener {
         return object : FileDownloadListener {
             override fun downloadStarted() {
                 fileDownloadNotificationManager.showDownloadInProgressNotification()
+                closeAndReturnToSourceIfBlankTab()
             }
 
             override fun downloadFinished(file: File, mimeType: String?) {
@@ -1134,6 +1143,14 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope, DaxDialogLi
                 snackbar.show()
             }
 
+            override fun downloadCancelled() {
+                closeAndReturnToSourceIfBlankTab()
+            }
+
+            override fun downloadOpened() {
+                closeAndReturnToSourceIfBlankTab()
+            }
+
             private fun showDownloadManagerAppSettings() {
                 try {
                     val intent = Intent(Settings.ACTION_APPLICATION_DETAILS_SETTINGS)

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

@@ -505,6 +505,14 @@ class BrowserTabViewModel(
         viewModelScope.launch { removeCurrentTabFromRepository() }
     }
 
+    override fun closeAndSelectSourceTab() {
+        viewModelScope.launch { removeAndSelectTabFromRepository() }
+    }
+
+    private suspend fun removeAndSelectTabFromRepository() {
+        tabRepository.deleteCurrentTabAndSelectSource()
+    }
+
     fun onUserPressedForward() {
         navigationAwareLoginDetector.onEvent(NavigationEvent.UserAction.NavigateForward)
         if (!currentBrowserViewState().browserShowing) {

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

@@ -127,11 +127,15 @@ class BrowserViewModel(
     }
 
     suspend fun onNewTabRequested(isDefaultTab: Boolean = false): String {
-        return tabRepository.add(isDefaultTab = isDefaultTab)
+        return tabRepository.addWithSource(isDefaultTab = isDefaultTab)
     }
 
     suspend fun onOpenInNewTabRequested(query: String, skipHome: Boolean = false): String {
-        return tabRepository.add(queryUrlConverter.convertQueryToUrl(query), skipHome, isDefaultTab = false)
+        return tabRepository.addWithSource(
+                queryUrlConverter.convertQueryToUrl(query),
+                skipHome,
+                isDefaultTab = false
+        )
     }
 
     suspend fun onTabsUpdated(tabs: List<TabEntity>?) {

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

@@ -49,7 +49,7 @@ class DownloadConfirmationFragment : BottomSheetDialogFragment() {
     lateinit var downloadListener: FileDownloadListener
 
     private val pendingDownload: PendingFileDownload by lazy {
-        arguments!![PENDING_DOWNLOAD_BUNDLE_KEY] as PendingFileDownload
+        requireArguments()[PENDING_DOWNLOAD_BUNDLE_KEY] as PendingFileDownload
     }
 
     private var file: File? = null
@@ -87,6 +87,7 @@ class DownloadConfirmationFragment : BottomSheetDialogFragment() {
         }
         view.cancel.setOnClickListener {
             Timber.i("Cancelled download for url ${pendingDownload.url}")
+            downloadListener.downloadCancelled()
             dismiss()
         }
 
@@ -126,6 +127,7 @@ class DownloadConfirmationFragment : BottomSheetDialogFragment() {
                 Timber.e("No suitable activity found")
                 Toast.makeText(activity, R.string.downloadConfirmationUnableToOpenFileText, Toast.LENGTH_SHORT).show()
             }
+            downloadListener.downloadOpened()
         }
     }
 

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

@@ -47,6 +47,7 @@ interface WebViewClientListener {
     fun recoverFromRenderProcessGone()
     fun requiresAuthentication(request: BasicAuthenticationRequest)
     fun closeCurrentTab()
+    fun closeAndSelectSourceTab()
     fun upgradedToHttps()
     fun surrogateDetected(surrogate: SurrogateResponse)
 

app/src/main/java/com/duckduckgo/app/browser/downloader/FileDownloader.kt

@@ -52,6 +52,8 @@ class FileDownloader @Inject constructor(
         fun downloadStarted()
         fun downloadFinished(file: File, mimeType: String?)
         fun downloadFailed(message: String, downloadFailReason: DownloadFailReason)
+        fun downloadCancelled()
+        fun downloadOpened()
     }
 }
 

app/src/main/java/com/duckduckgo/app/browser/downloader/NetworkFileDownloader.kt

@@ -46,6 +46,7 @@ class NetworkFileDownloader @Inject constructor(private val context: Context) {
         }
         val manager = context.getSystemService(Context.DOWNLOAD_SERVICE) as DownloadManager?
         manager?.enqueue(request)
+        callback.downloadStarted()
     }
 
     private fun downloadManagerAvailable(): Boolean {

app/src/main/java/com/duckduckgo/app/global/db/AppDatabase.kt

@@ -63,7 +63,7 @@ import com.duckduckgo.app.usage.search.SearchCountDao
 import com.duckduckgo.app.usage.search.SearchCountEntity
 
 @Database(
-    exportSchema = true, version = 23, entities = [
+    exportSchema = true, version = 24, entities = [
         TdsTracker::class,
         TdsEntity::class,
         TdsDomainEntity::class,
@@ -316,6 +316,12 @@ class MigrationsProvider(
         }
     }
 
+    val MIGRATION_23_TO_24: Migration = object : Migration(23, 24) {
+        override fun migrate(database: SupportSQLiteDatabase) {
+            database.execSQL("ALTER TABLE `tabs` ADD COLUMN `sourceTabId` TEXT")
+        }
+    }
+
     val ALL_MIGRATIONS: List<Migration>
         get() = listOf(
             MIGRATION_1_TO_2,
@@ -339,7 +345,8 @@ class MigrationsProvider(
             MIGRATION_19_TO_20,
             MIGRATION_20_TO_21,
             MIGRATION_21_TO_22,
-            MIGRATION_22_TO_23
+            MIGRATION_22_TO_23,
+            MIGRATION_23_TO_24
         )
 
     @Deprecated(

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

@@ -85,6 +85,24 @@ abstract class TabsDao {
         }
     }
 
+    @Transaction
+    open fun deleteTabAndUpdateSelection(tab: TabEntity, newSelectedTab: TabEntity? = null) {
+        deleteTab(tab)
+
+        if (newSelectedTab != null) {
+            insertTabSelection(TabSelectionEntity(tabId = newSelectedTab.tabId))
+            return
+        }
+
+        if (selectedTab() != null) {
+            return
+        }
+
+        firstTab()?.let {
+            insertTabSelection(TabSelectionEntity(tabId = it.tabId))
+        }
+    }
+
     @Insert(onConflict = OnConflictStrategy.REPLACE)
     abstract fun insertTabSelection(tabSelectionEntity: TabSelectionEntity)
 

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

@@ -26,8 +26,10 @@ import com.duckduckgo.app.global.useourapp.UseOurAppDetector
 import com.duckduckgo.app.tabs.db.TabsDao
 import io.reactivex.Scheduler
 import io.reactivex.schedulers.Schedulers
+import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.GlobalScope
 import kotlinx.coroutines.launch
+import kotlinx.coroutines.withContext
 import timber.log.Timber
 import java.util.*
 import javax.inject.Inject
@@ -53,6 +55,23 @@ class TabDataRepository @Inject constructor(
         return tabId
     }
 
+    override suspend fun addWithSource(url: String?, skipHome: Boolean, isDefaultTab: Boolean): String {
+        val tabId = generateTabId()
+        val sourceTabId = withContext(Dispatchers.IO) {
+            tabsDao.selectedTab()?.tabId
+        }
+
+        add(
+            tabId,
+            buildSiteData(url),
+            skipHome = skipHome,
+            isDefaultTab = isDefaultTab,
+            sourceTabId = sourceTabId
+        )
+
+        return tabId
+    }
+
     private fun generateTabId() = UUID.randomUUID().toString()
 
     private fun buildSiteData(url: String?): MutableLiveData<Site> {
@@ -64,7 +83,7 @@ class TabDataRepository @Inject constructor(
         return data
     }
 
-    override suspend fun add(tabId: String, data: MutableLiveData<Site>, skipHome: Boolean, isDefaultTab: Boolean) {
+    override suspend fun add(tabId: String, data: MutableLiveData<Site>, skipHome: Boolean, isDefaultTab: Boolean, sourceTabId: String?) {
         siteData[tabId] = data
         databaseExecutor().scheduleDirect {
 
@@ -83,7 +102,15 @@ class TabDataRepository @Inject constructor(
             }
             Timber.i("About to add a new tab, isDefaultTab: $isDefaultTab. $tabId, position: $position")
 
-            tabsDao.addAndSelectTab(TabEntity(tabId, data.value?.url, data.value?.title, skipHome, true, position))
+            tabsDao.addAndSelectTab(TabEntity(
+                    tabId = tabId,
+                    url = data.value?.url,
+                    title = data.value?.title,
+                    skipHome = skipHome,
+                    viewed = true,
+                    position = position,
+                    sourceTabId = sourceTabId
+            ))
         }
     }
 
@@ -145,6 +172,21 @@ class TabDataRepository @Inject constructor(
         siteData.remove(tab.tabId)
     }
 
+    override suspend fun deleteCurrentTabAndSelectSource() {
+        databaseExecutor().scheduleDirect {
+            val tabToDelete = tabsDao.selectedTab() ?: return@scheduleDirect
+
+            deleteOldPreviewImages(tabToDelete.tabId)
+            val tabToSelect = tabToDelete.sourceTabId
+                    .takeUnless { it.isNullOrBlank() }
+                    ?.let {
+                        tabsDao.tab(it)
+                    }
+            tabsDao.deleteTabAndUpdateSelection(tabToDelete, tabToSelect)
+            siteData.remove(tabToDelete.tabId)
+        }
+    }
+
     override fun deleteAll() {
         Timber.i("Deleting tabs right now")
         tabsDao.deleteAllTabs()

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

@@ -33,7 +33,8 @@ data class TabEntity(
     var skipHome: Boolean = false,
     var viewed: Boolean = true,
     var position: Int,
-    var tabPreviewFile: String? = null
+    var tabPreviewFile: String? = null,
+    var sourceTabId: String? = null
 )
 
 val TabEntity.isBlank: Boolean

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

@@ -31,7 +31,9 @@ interface TabRepository {
      */
     suspend fun add(url: String? = null, skipHome: Boolean = false, isDefaultTab: Boolean = false): String
 
-    suspend fun add(tabId: String, data: MutableLiveData<Site>, skipHome: Boolean = false, isDefaultTab: Boolean = false)
+    suspend fun add(tabId: String, data: MutableLiveData<Site>, skipHome: Boolean = false, isDefaultTab: Boolean = false, sourceTabId: String? = null)
+
+    suspend fun addWithSource(url: String? = null, skipHome: Boolean = false, isDefaultTab: Boolean = false): String
 
     suspend fun addNewTabAfterExistingTab(url: String? = null, tabId: String)
 
@@ -44,6 +46,8 @@ interface TabRepository {
 
     suspend fun delete(tab: TabEntity)
 
+    suspend fun deleteCurrentTabAndSelectSource()
+
     fun deleteAll()
 
     suspend fun select(tabId: String)