Skip to content

Conversation

@cmonfortep
Copy link
Contributor

@cmonfortep cmonfortep commented Aug 6, 2020

Task/Issue URL: https://app.asana.com/0/414730916066338/1187653972293689/f
Tech Design URL:
CC:

Description:
Tries to fix the following issue:

Current behavior:

  • User clicks on a link from tab A
  • New tab is opened (Tab B)
  • User presses back
  • Home page shown (in Tab B)
  • User presses back again (in Tab B)
  • App is closed

Expected behavior:

  • User clicks on a link from tab A
  • New tab is opened (Tab B)
  • User presses back (Tab B)
  • Tab B closed
  • Tab A opened

Related open issues:
#815
#696

Steps to test this PR:

Test: Open link on a new tab

  1. Open the app
  2. Perform a search or visit any website (Tab A)
  3. long press on a link
  4. open on a new tab (Tab B)
  5. ensure new tab is opened with expected link
  6. press back
  7. ensure Tab B is closed, and user goes back to Tab A

Test: Open link on a new tab (source tab deleted)

  1. Open the app
  2. Perform a search or visit any website (Tab A)
  3. long press on a link
  4. open on a new tab (Tab B)
  5. ensure new tab is opened with expected link
  6. Go to tab switcher, delete Tab A
  7. Go to Tab B
  8. Press back
  9. Ensure the home screen is shown

Test: Open link on a background tab

  1. Open the app
  2. Perform a search or visit any website (Tab A)
  3. long press on a link
  4. Click on "Open in Background Tab" (Tab B)
  5. ensure new tab is created (tabs counter)
  6. Go to that tab (open tabswitcher, and change tab)
  7. press back
  8. ensure Tab B is closed, and user goes back to Tab A

Test: Open link on a background tab (source tab deleted)

  1. Open the app
  2. Perform a search or visit any website (Tab A)
  3. long press on a link
  4. Click on "Open in Background Tab" (Tab B)
  5. ensure new tab is created (tabs counter)
  6. Go to tab switcher, delete Tab A
  7. Go to Tab B
  8. Press back
  9. Ensure the home screen is shown

Test: Open link in new tab (target=_blank)

  1. Open the app
  2. Visit https://www.w3schools.com/code/tryit.asp?filename=GGETHXPDSEMN (Tab A)
  3. Press run and click on "test" or "link in new tab"
  4. ensure new tab is opened (Tab B)
  5. press back
  6. ensure Tab B is closed, and user goes back to Tab A

Test: Open link in new tab (target=_blank / source tab deleted)

  1. Open the app
  2. Visit https://www.w3schools.com/code/tryit.asp?filename=GGETHXPDSEMN (Tab A)
  3. Press run and click on "test" or "link in new tab"
  4. ensure new tab is opened (Tab B)
  5. Go to tab switcher, delete Tab A
  6. Go to Tab B
  7. Press back
  8. Ensure the home screen is shown

Steps to test previous logic is preserved:

Test: Open a New Empty tab

  1. Open the app
  2. create a new tab manually from overflow menu or tabswitcher
  3. Ensure new tab is created
  4. perform a search o visit a website
  5. press back
  6. Ensure home screen is shown

Test: External links

  1. Go to another app where you have a link to click
  2. Open that link with DDG
  3. Ensure link is opened in a new tab
  4. Press back
  5. Ensure previous app is opened again
  6. Open DDG app again via task manager
  7. Press back on the tab you previously opened
  8. Ensure App gets closed

Test: Search widget

  1. Click on our search widget
  2. search something or click any suggestion
  3. Ensure new tab is created with your search term
  4. Press back
  5. Ensure App gets closed

Test: Shortcut

  1. Create a shortcut to any website
  2. Click on the shortcut
  3. Ensure new tab is created with your search term
  4. Press back
  5. Ensure App gets closed

Test database migration:

  1. Install current main branch
  2. Open as many tabs as you want
  3. Install this branch
  4. Open app again
  5. Ensure app doesn't crash

Internal references:

Software Engineering Expectations
Technical Design Template

Cleanup a isDefaultTab as it was never used in some parts of the code.
@cmonfortep cmonfortep marked this pull request as ready for review August 7, 2020 07:51
val MIGRATION_23_TO_24: Migration = object : Migration(23, 24) {
override fun migrate(database: SupportSQLiteDatabase) {
database.execSQL("ALTER TABLE `tabs` ADD COLUMN `sourceTabId` TEXT")
//https://stackoverflow.com/a/57797179/980345
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CDRussell When testing migrations I detected an issue trying to ALTER a table to introduce a FOREIGN key. It seems it's not supported and the workaround is to migrate -> drop -> create a new table.

I have my worries (based on nothing in concrete, just a feeling), don't know if this can be an issue for users with many open tabs...

@cmonfortep cmonfortep requested a review from CDRussell August 7, 2020 08:03
Copy link
Member

@CDRussell CDRussell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Crashes on upgrading DB:

java.lang.IllegalStateException: Room cannot verify the data integrity. Looks like you've changed schema but forgot to update the version number. You can simply fix this by increasing the version number.

Otherwise, excellent stuff! Get that upgrade path sorted and we should be good to go!

@cmonfortep
Copy link
Contributor Author

cmonfortep commented Aug 7, 2020

@CDRussell sorry for not pointing out how to test the database migration, I've added the info in the description (also in this comment)

The current production release has database version 23, develop has version 24 (when we introduced sourceTabId). I wanted to avoid another database migration to just set sourceTabId as Foreign key. That's why I've just updated the migration step without changing the database version. Could you instead use main to validate the migration?

Test database migration:

  1. Install current main branch
  2. Open as many tabs as you want
  3. Install this branch
  4. Open app again
  5. Ensure app doesn't crash (sourceTabId has null for each column, foreign key exists)

@cmonfortep cmonfortep requested a review from CDRussell August 7, 2020 10:26
@CDRussell
Copy link
Member

Could you instead use main to validate the migration

I can do that. Though going forward i'd like to have nightly builds from develop, so once something is in develop we'd want to migrate any changes.

Copy link
Member

@CDRussell CDRussell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm soooo happy to see this finally improved. great stuff!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants