Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Show on App Launch: Open launch option when set #4995

Merged
merged 5 commits into from
Sep 20, 2024
Merged

Show on App Launch: Open launch option when set #4995

merged 5 commits into from
Sep 20, 2024

Conversation

mikescamell
Copy link
Contributor

@mikescamell mikescamell commented Sep 10, 2024 •
edited
Loading

Task/Issue URL: https://app.asana.com/0/1207908166761516/1208167186867417/f

Description

Adds functionality to open the show on app launch option based on what has been set.

If there is no specialised launch happening then we check the show on app launch setting:

  1. If LastOpenedTab is set, we do nothing as this is current behaviour
  2. If NewTabPage is set then we open a new tab
  3. If SpecificPage is set we compare the url to the current selected tab, if it's not the same we open a new tab otherwise we do nothing

Also as part of this PR, we add validation to the URL:

  1. It is blank will set it as a default to duckduckgo.com
  2. If it has no scheme will add http (in case the website does not currently support https)
  3. Otherwise return the url what was passed in

Steps to test this PR

Last Opened Tab

  • Set "Show on App Launch" to Last Opened Tab
  • Exit Settings
  • Remember your current tab and tab count
  • Force close the app via recents
  • Open the app
  • Check the tab is the same as when you closed
  • Ensure the tab count has not increased

New Tab Page

  • Set "Show on App Launch" to New Tab Page
  • Exit Settings
  • Remember your current tab and tab count. If this is the New Tab Page then please open a website e.g. “bbc.com"
  • Force close the app via recents
  • Open the app
  • Check the tab is now the New Tab Page
  • Ensure the tab count has increased by one
  • Force close the app via recents
  • Open the app
  • Check the tab is still the New Tab Page
  • Ensure the tab count has not increased by one

Specific Page

  • Set "Show on App Launch" to Specific Page
  • Check the default URL is “https://duckduckgo.com/”
  • Exit Settings
  • Remember your current tab (it should not be “https://duckduckgo.com/”) and tab count
  • Force close the app via recents
  • Open the app
  • Check the tab is now https://duckduckgo.com/
  • Ensure the tab count has increased by one (if your tab was on the new tab page before you force closed it would not increase)
  • Force close the app via recents
  • Open the app
  • Check the tab is still https://duckduckgo.com/
  • Ensure the tab count has not increased by one

Specific Page Lowercasing

  • Set "Show on App Launch" to Specific Page
  • Change the default URL to “BBC.com”
  • Press back
  • Check that under the "Show on App Launch” setting the url is “http://bbc.com”

Specific Page Updated

  • Set "Show on App Launch" to Specific Page
  • Change the default URL to a different url e.g. “cnn.com"
  • Exit Settings
  • Remember your current tab (it should not be the same url you entered earlier) and tab count
  • Force close the app via recents
  • Open the app
  • Check the tab is now the url you set e.g. cnn.com
  • Ensure the tab count has increased by one

Specific Page Not a URL

  • Set "Show on App Launch" to Specific Page
  • Change the default URL to a url that is invalid e.g. “example"
  • Press back
  • Check that under the "Show on App Launch” setting the url is “http://example”
  • Remember your current tab (it should not be the same url you entered earlier) and tab count
  • Force close the app via recents
  • Open the app
  • Check the tab has attempted to load "http://example"
  • Ensure the tab count has increased by one

Specific Page Different Scheme

  • Set "Show on App Launch" to Specific Page
  • Change the default URL to a url with a different scheme e.g. “ftp://file.com"
  • Press back
  • Check that under the "Show on App Launch” setting the url is still “ftp://file.com”

UI changes

N/A

Demo

Screen_recording_20240910_115305.mp4

@mikescamell Graphite App
Copy link
Contributor Author

mikescamell commented Sep 10, 2024 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @mikescamell and the rest of your teammates on Graphite Graphite

@mikescamell mikescamell force-pushed the feature/mike/show-on-app-launch/open-launch-option-when-set branch from f5376a6 to 4b1b49f Compare September 10, 2024 09:53
@mikescamell mikescamell marked this pull request as ready for review September 10, 2024 10:18
@anikiki anikiki self-assigned this Sep 10, 2024
@mikescamell mikescamell force-pushed the feature/mike/show-on-app-launch/translations_l10n branch from 2255144 to 4b18eac Compare September 11, 2024 07:47
@mikescamell mikescamell force-pushed the feature/mike/show-on-app-launch/open-launch-option-when-set branch 2 times, most recently from 0b1a8e0 to 7959fa4 Compare September 11, 2024 11:48
This was referenced Sep 11, 2024
Merged
Merged
Merged
Merged
Merged
Merged
anikiki
anikiki approved these changes Sep 11, 2024
View reviewed changes
Copy link
Contributor

@anikiki anikiki left a comment

Choose a reason for hiding this comment

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

LGTM 👍

I didn't tick Ensure the tab count has *not increased by one as the tab count is increased due to the trailing "/".

@mikescamell mikescamell force-pushed the feature/mike/show-on-app-launch/translations_l10n branch from ff399a0 to 6a40fe1 Compare September 12, 2024 08:16
@mikescamell mikescamell force-pushed the feature/mike/show-on-app-launch/open-launch-option-when-set branch from 7959fa4 to b7df6fb Compare September 12, 2024 08:16
@mikescamell mikescamell force-pushed the feature/mike/show-on-app-launch/translations_l10n branch from 6a40fe1 to 9b0bfe9 Compare September 12, 2024 09:11
@mikescamell mikescamell force-pushed the feature/mike/show-on-app-launch/open-launch-option-when-set branch from b7df6fb to 8a4f4c6 Compare September 12, 2024 09:12
@mikescamell mikescamell force-pushed the feature/mike/show-on-app-launch/translations_l10n branch from 9b0bfe9 to cbfa50f Compare September 12, 2024 09:47
@mikescamell mikescamell force-pushed the feature/mike/show-on-app-launch/open-launch-option-when-set branch from 8a4f4c6 to fd687fc Compare September 12, 2024 09:47
@mikescamell mikescamell force-pushed the feature/mike/show-on-app-launch/translations_l10n branch from cbfa50f to bd1e9f5 Compare September 12, 2024 13:14
@mikescamell mikescamell force-pushed the feature/mike/show-on-app-launch/open-launch-option-when-set branch from fd687fc to 9e45cbc Compare September 12, 2024 13:14
@mikescamell mikescamell force-pushed the feature/mike/show-on-app-launch/translations_l10n branch from bd1e9f5 to 4dba6e6 Compare September 16, 2024 13:36
@mikescamell mikescamell force-pushed the feature/mike/show-on-app-launch/open-launch-option-when-set branch from 9e45cbc to 39ba200 Compare September 16, 2024 13:36
@mikescamell
Copy link
Contributor Author

LGTM 👍

I didn't tick Ensure the tab count has *not increased by one as the tab count is increased due to the trailing "/".

I’ve moved the default url to be https://duckduckgo.com/ seeing as we always append the slash. There’s no good reason for that not to be the default. I updated the test cases to reflect this.

@mikescamell mikescamell force-pushed the feature/mike/show-on-app-launch/translations_l10n branch from 4dba6e6 to f22a708 Compare September 19, 2024 14:31
@mikescamell mikescamell force-pushed the feature/mike/show-on-app-launch/open-launch-option-when-set branch from 39ba200 to bf33919 Compare September 19, 2024 14:31
@mikescamell mikescamell mentioned this pull request Sep 19, 2024
Merged
12 tasks
@mikescamell
add ShowOnAppLaunchUrlConverter
8949392 
This class will take the url that a user has entered and if:

1. It is blank will set it as a default to duckduckgo.com
2. If it has no scheme will add http (in case the website does not currently support https)
3. Other wise return what was passed in
@mikescamell
open show on app launch setting when set
e0e0be4 
The launchNewSearchOrQuery seemed to be the best place to add check if we should launch the show on app setting. It does a lot of checking based on what's passed into the intent as far as I can see.

If there is no specialised launch happening then we check the show on app launch setting

1. If LastOpenedTab is set, we do nothing
2. If NewTabPage is set then we open a new tab
3. If SpecificPage is set we compare the url to the current selected tab, if it's not the same we open a new tab otherwise we do nothing

I had to expose selectedTab from the TabsDao so I could the tab in a synchronous fashion to check. I would have made the tabsDao.selectedTab() suspending but that would have meant changes elsewhere.
@mikescamell
fix test
62a33a9
@mikescamell
update default url
606fe1c 
we always add the slash when loading a website if it doesn't have it so we might as well add it to the default url as well
@mikescamell mikescamell force-pushed the feature/mike/show-on-app-launch/translations_l10n branch from f22a708 to b40783d Compare September 19, 2024 14:48
@mikescamell mikescamell force-pushed the feature/mike/show-on-app-launch/open-launch-option-when-set branch from bf33919 to 606fe1c Compare September 19, 2024 14:48
@mikescamell
Show on App Launch: Add additional tests (#5000)
621d02e 
Task/Issue URL:
https://app.asana.com/0/1207908166761516/1208156273709083/f

### Description

Adds some additional tests for the BrowserViewModel and the
ShowOnAppLaunch store

### Steps to test this PR

N/A

### UI changes

N/A
@mikescamell mikescamell merged commit 94fb762 into feature/mike/show-on-app-launch/translations_l10n Sep 20, 2024
4 of 5 checks passed
@mikescamell mikescamell deleted the feature/mike/show-on-app-launch/open-launch-option-when-set branch September 20, 2024 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anikiki anikiki anikiki approved these changes

@malmstein malmstein Awaiting requested review from malmstein

@nalcalag nalcalag Awaiting requested review from nalcalag

Assignees

@anikiki anikiki

Labels
STACKED PR - DO NOT MERGE
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mikescamell @anikiki