-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add handling of app links #1267
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.
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
Conversation
malmstein
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job @joshliebe. Left a few comments but feel free to ignore what you don't agree with.
app/src/main/java/com/duckduckgo/app/browser/SpecialUrlDetector.kt
Outdated
Show resolved
Hide resolved
app/src/androidTest/java/com/duckduckgo/app/browser/SpecialUrlDetectorImplTest.kt
Show resolved
Hide resolved
app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/duckduckgo/app/browser/BrowserWebViewClient.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/duckduckgo/app/browser/SpecialUrlDetector.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/duckduckgo/app/browser/SpecialUrlDetector.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/duckduckgo/app/browser/applinks/AppLinksHandler.kt
Outdated
Show resolved
Hide resolved
Thanks for reviewing @malmstein, will address the comments 👍 |
cmonfortep
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @joshliebe. This looks great, I left some minor comments, feel free to skip them.
I've tested this on my device and it works as described 👍
app/src/main/java/com/duckduckgo/app/browser/SpecialUrlDetector.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/duckduckgo/app/browser/SpecialUrlDetector.kt
Outdated
Show resolved
Hide resolved
|
|
||
| override fun handleNonHttpAppLink(isRedirect: Boolean, launchNonHttpAppLink: () -> Unit): Boolean { | ||
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N && isRedirect && appLinkOpenedInBrowser) { | ||
| return true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've noticed handleAppLink and handleNonHttpAppLink both return Boolean, but the same boolean can have different meanings. E.g: here when NonHttpAppLink not handled we return true, but in handleAppLink we return false.
is there any specific reason why return true in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For non-HTTP links, like market:// or intent://, true is returned so that they are ignored. If false is returned for those links, they will open in the browser and the user will see the WebView error page because they can't be handled. false is returned for HTTP app links since the WebView can open those.
app/src/main/java/com/duckduckgo/app/browser/SpecialUrlDetector.kt
Outdated
Show resolved
Hide resolved
app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt
Outdated
Show resolved
Hide resolved
|
Thanks for reviewing @cmonfortep 🙂 |
Task/Issue URL: https://app.asana.com/0/72649045549333/1199533149091655/f
Tech Design URL: https://app.asana.com/0/481882893211075/1200311072033594
CC:
Description:
This PR adds handling of App Links.
When a user taps an app link they will be prompted to open the link in the corresponding app (By tapping the "Yes" button) or view the content in the browser (By tapping the "No" button"). If there are multiple apps that can handle the link, then a chooser is shown.
The feature can be enabled/disabled in settings. It is enabled by default.
Screenshots:
Steps to test this PR:
Internal references:
Software Engineering Expectations
Technical Design Template