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

JKane10 · 2020-07-04T18:13:05Z

Task/Issue URL: #753

Description:

Actual: When downloading via a link that has target="_blank", the user is left with a blank tab.
Expected: Ensure that this action does not open a new blank tab.
Referenced the work of @AndrewR8 and the comments in PR Handling target=_blank if link points to a file #798

Approach:

Modified TabEntity to maintain a sourceTabId to reference the source tab by its tab id.
When opening a new tab, store the sourceTabId in the TabEntity db entry.
In the event that a download is opened in a new, blank tab (url is blank), callbacks will use tabRepo to select the source tab and delete the blank tab.

Concerns

This still opens a blank tab in which the download options are displayed, but upon the user completing their selections, the blank tab is cleaned up and the source tab is selected and displayed.
TabEntities now maintain a source whether it is needed or not - Could simplify this a bit by just passing the sourceTabId in the Fragment bundle as well?
LeakCanary is identifying a leak, but struggling to track it down. If this approach is valid, could use some assistance on this.

Steps to test this PR:

Navigate to : https://www.w3schools.com/code/tryit.asp?filename=GGETHXPDSEMN
Run and select "Download in new tab".
Make the appropriate selection
New, blank tab should close and return to source tab.

…o navigate back to. Bumps DB version and adds migration sql Adds viewModel method to close and return to source tab. Adds download callbacks to return to source tab if empty tab is opened. Specifically in this event when a download link opens a new blank tab and needs to close after download has been triggered.

…TabId

…leteAndSelectSource

…753-download-in-new-tab-leaves-behind-blank-tab � Conflicts: � app/schemas/com.duckduckgo.app.global.db.AppDatabase/22.json � app/src/androidTest/java/com/duckduckgo/app/global/db/AppDatabaseTest.kt � app/src/main/java/com/duckduckgo/app/global/db/AppDatabase.kt

…tabs

cmonfortep · 2020-07-06T14:44:35Z

Thanks for the contribution, I will review it this week.

cmonfortep · 2020-07-27T17:39:13Z

@JKane10 Sorry for the delay here. I told you I was going to review your PR a few weeks ago and I didn't have time for it + personal time off. I'm back to the party and I will review it ASAP.

cmonfortep

Hey @JKane10! Thanks for this contribution.
I took a look to your approach and I think it's pretty good! Storing the sourceTabId in the database makes possible to close the blank page and go back to the previous tab. Not only for this case but also for future improvements we will introduce to deal with the "back button".

Here you have some comments from my side. I wanted to share with you some thoughts about how to get the current Tab Id, let me know what do you think.

About the leak, I think the one you are saying is not related to your changes (I saw one related to the constraintLayout), so no need for you to focus there.

Looking forward to work with you here.

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

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

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

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

…anup and referencing the liveSelectedTab from inside the repo rather than duplicating the value and passing as a param

cmonfortep

I was taking a quick look to the changes and I had only one comment, but the rest looks fine. I will wait for that change and do a final full review + testing. Thanks!

cmonfortep · 2020-07-29T21:02:00Z

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

+            buildSiteData(url),
+            skipHome = skipHome,
+            isDefaultTab = isDefaultTab,
+            sourceTabId = liveSelectedTab.value?.tabId


Getting the value from liveSelectedTab works because when we call addWithSource (or deleteCurrentTabAndSelectSource) a ViewModel is observing that LiveData externally. But if we end up in a scenario where we call this method without anyone observing the LiveData, this will fail. I think is better to use tabsDao.selectedTab().

Makes sense, let me know how my latest approach looks. A bit new to coroutines still, so not sure if the usage of withContext is ideal, but we'd need to await the value from the DB before proceeding and can't access the DB on the main thread.

cmonfortep · 2020-07-29T21:02:25Z

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

    }

+    override suspend fun deleteCurrentTabAndSelectSource() {
+        val tabToDelete = liveSelectedTab.value


Same here with this LiveData.

… directly from the tabsDao rather than the LiveData object.

cmonfortep

This is looking great. I think we are close to being done 🙌
There's only one more comment from my side, so if you could address it and test that we haven't broken anything we are good.
Also, would you mind to add a few tests in TabDataRepositoryTest for the methods we introduced in this PR? just to cover the new logic.
Awesome! thanks!

cmonfortep · 2020-07-31T09:35:05Z

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

+                siteData.remove(tabToDelete.tabId)
+            }
+
+            tabToDelete?.sourceTabId?.let { sourceTabId ->


Checking twice this method, I was asking myself what will happen if tabToDelete?.sourceTabId is null. It will not crash but we will not select a new tab. It may produce an inconsistent state for users.
Additionally, we are doing a few operations that modify the database (delete + insert), but they are not inside a transaction. If one fails, we could leave the database in a weird state too.

To solve both things I was thinking about modifying tabsDao.deleteTabAndUpdateSelection, where we can have a @Transaction, and also a fallback to the firstTab.

@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)) } }

and the repository method will look like:

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) } }

I haven't checked if that actually works or if it compiles 😅, just wanted to show the idea. What do you think? please double check from your side too in case you like the approach.

Good call, would definitely want to ensure the entire transaction completes rather than risk leaving the DB in an undesired state. The code you provided works as is and looks good to me.

This also accounts for the unlikely situation that a user opens a link in the new tab, declines the dialog, closes the sourceTab, and then refreshes the download tab to reinitialize the download dialog and proceeds. The user will now return to the firstTab where as previously the user would just remain on the blank new tab.

cmonfortep · 2020-07-31T09:39:18Z

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

            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, data.value?.url, data.value?.title, skipHome, true, position, null, sourceTabId))


Not important: I see you added null as previewImage because you wanted to pass sourceTabId which comes next. To avoid that, you could use named params instead, and only send the ones you know its value.

tabsDao.addAndSelectTab( TabEntity( tabId = tabId, url = data.value?.url, title = data.value?.title, skipHome = skipHome, viewed = true, position = position, sourceTabId = sourceTabId ) )

cmonfortep · 2020-07-31T09:46:40Z

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

+        val tabId = generateTabId()
+        var sourceTabId: String? = null
+
+        withContext(Dispatchers.IO) {


👍 you can perfectly use withContext here to change threads.

Because you mentioned that you were a bit new to coroutines, just a small nice tip 🙌.

If you check the signature of withContext:

public suspend fun <T> withContext( context: CoroutineContext, block: suspend CoroutineScope.() -> T ): T

That means withContext can return the value you are querying in the internal block.

You can do this:

val sourceTabId = withContext(Dispatchers.IO) { tabsDao.selectedTab()?.tabId }

Awesome, this is cleaner too!

…te db calls to ensure data integrity.

…eCurrentTabAndSelectSource)

JKane10 · 2020-08-01T03:50:05Z

Made changes related to the latest feedback and added 2 tests for the 2 new TabDataRepo methods. Let me know if the tests look sufficient.

cmonfortep

@JKane10 This looks great! I've also tested the latest revision and works as expected. Good job!

Sadly, I've just noticed we merged today a PR that has updated the database version, so you have some conflicts to solve here. Could you solve them? I will merge the PR after that.

Solving database code conflicts is a bit tricky: you will need to accept the 23.json from develop, update your database versions to 24, and that will generate a new json file for the database changes we introduced here. Also, update the database test cases to cover migration from 23 to 24. If it gets hairy, let me know and I can solve them for you. Happy to help here.

…753-download-in-new-tab-leaves-behind-blank-tab � Conflicts: � app/schemas/com.duckduckgo.app.global.db.AppDatabase/23.json � app/src/androidTest/java/com/duckduckgo/app/tabs/model/TabDataRepositoryTest.kt

JKane10 · 2020-08-03T18:18:20Z

@JKane10 This looks great! I've also tested the latest revision and works as expected. Good job!

Sadly, I've just noticed we merged today a PR that has updated the database version, so you have some conflicts to solve here. Could you solve them? I will merge the PR after that.

Solving database code conflicts is a bit tricky: you will need to accept the 23.json from develop, update your database versions to 24, and that will generate a new json file for the database changes we introduced here. Also, update the database test cases to cover migration from 23 to 24. If it gets hairy, let me know and I can solve them for you. Happy to help here.

The DB implementation is really well done. Updating the migration and test seems straight forward. Let me know if I missed anything.

cmonfortep

@JKane10 Thanks for the contribution!!! Really enjoyed working with you here. We have been really looking forward to fixing this bug, so thanks for the changes. 🙌

JKane10 · 2020-08-05T13:27:44Z

@JKane10 Thanks for the contribution!!! Really enjoyed working with you here. We have been really looking forward to fixing this bug, so thanks for the changes. 🙌

Thanks for working with me on this @cmonfortep. Enjoyed contributing. I'll keep my eye out for other opportunities.

JKane10 added 5 commits July 4, 2020 01:15

Removes need to pass entire TabEntity in favor of just passing source…

d8aa268

…TabId

Code clean up, scope fix, and mirrors TabDataRepo delete closer in de…

cdd5a1c

…leteAndSelectSource

Updates tests to expected addWithSource rather than just add for new …

9001def

…tabs

cmonfortep self-assigned this Jul 6, 2020

cmonfortep reviewed Jul 28, 2020

View reviewed changes

Addresses PR feedback primarily letting fragment lifecycle handle cle…

2c24885

…anup and referencing the liveSelectedTab from inside the repo rather than duplicating the value and passing as a param

JKane10 requested a review from cmonfortep July 29, 2020 14:27

cmonfortep reviewed Jul 29, 2020

View reviewed changes

Addresses PR feedback - info related to the source tab will be pulled…

d4fb51a

… directly from the tabsDao rather than the LiveData object.

cmonfortep reviewed Jul 31, 2020

View reviewed changes

JKane10 added 2 commits July 31, 2020 22:48

Addresses PR feedback - Create and use transaction rather than separa…

edb1592

…te db calls to ensure data integrity.

Adds tests for the 2 new TabDataRepo methods (AddWithSource and Delet…

702a54c

…eCurrentTabAndSelectSource)

JKane10 requested a review from cmonfortep August 1, 2020 03:50

cmonfortep reviewed Aug 3, 2020

View reviewed changes

JKane10 added 2 commits August 3, 2020 13:05

updates db migrations and tests after merge

e2f9f0c

JKane10 requested a review from cmonfortep August 3, 2020 18:16

cmonfortep approved these changes Aug 5, 2020

View reviewed changes

cmonfortep merged commit 408259c into duckduckgo:develop Aug 5, 2020

CDRussell mentioned this pull request Aug 7, 2020

Fix crash that could happen when trying to download a large data URI image #898

Merged

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

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

Uh oh!

Conversation

JKane10 commented Jul 4, 2020

Uh oh!

cmonfortep commented Jul 6, 2020

Uh oh!

cmonfortep commented Jul 27, 2020

Uh oh!

cmonfortep left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cmonfortep left a comment

Choose a reason for hiding this comment

Uh oh!

cmonfortep Jul 29, 2020

Choose a reason for hiding this comment

Uh oh!

JKane10 Jul 31, 2020

Choose a reason for hiding this comment

Uh oh!

cmonfortep Jul 29, 2020

Choose a reason for hiding this comment

Uh oh!

cmonfortep left a comment

Choose a reason for hiding this comment

Uh oh!

cmonfortep Jul 31, 2020

Choose a reason for hiding this comment

Uh oh!

JKane10 Aug 1, 2020

Choose a reason for hiding this comment

Uh oh!

cmonfortep Jul 31, 2020

Choose a reason for hiding this comment

Uh oh!

cmonfortep Jul 31, 2020

Choose a reason for hiding this comment

Uh oh!

JKane10 Aug 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JKane10 commented Aug 1, 2020

Uh oh!

cmonfortep left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JKane10 commented Aug 3, 2020

Uh oh!

cmonfortep left a comment

Choose a reason for hiding this comment

Uh oh!

JKane10 commented Aug 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

JKane10 Aug 1, 2020 •

edited

Loading

cmonfortep left a comment •

edited

Loading