-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Fix opening downloads and streams on external players on Android 11+ and refactor external intents #9833
Conversation
- Don't check the availability of a default package due to package visibility restrictions on Android 11+, use try-catches blocks instead. HTTP default browser test has been removed; - Always show an app chooser on Android 10+ if there is only one action to handle the original intent, so the user can use actions in the chooser, such as copying a text to clipboard in a share sheet for ACTION_SEND intents; - Remove openUrlInBrowser delegate method and use directly the remaining method in the previous usages of the delegate. This change clarifies whether a chooser will be shown for the ACTION_VIEW intent which will be created from the given URL; - Make ShareUtils.openAppChooser public, so functions and methods which need to use a system chooser don't have to write a similar code. It also requires a new string argument, chooserTitle, which is the title which will be set to the chooser for all intents except the ACTION_SEND ones on Android 9+, as the system ignores it on these Android versions. This method is now used in MissionAdapter to share a downloaded file.
As we only use now standard intents, we do not need to query packages for http, https and market schemes anymore, so we can remove the query declaration in Android manifest: see https://web.archive.org/web/20200817164113/https://developer.android.com/ preview/privacy/package-visibility#use-cases-not-affected for more details.
As ShareUtils.openIntentInApp is used directly with showNoAppToast set to true in MissionAdapter.viewWithFileProvider, the string resource "No app installed to play this file" (toast_no_player) is not used anymore and has been replaced by a similar one already used for other intents, "No app on your device can open this" (no_app_to_open_intent). That's the reason why it is removed with this commit.
Kudos, SonarCloud Quality Gate passed! |
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.
Code looks good to me, I just have some suggestions on how to improve the ShareUtils
methods structure. Thanks!
The behavior requested in #9403 seems now to be NewPipe's behavior. So when opening in browser, the official youtube app would show up instead. Only if I disable the official app's "Open by default" system setting, then the browser shows up. And if I then set NewPipe to open supported links, opening in browser results in the NewPipe share dialog coming up, which is surely not wanted. Note that this happens both if I set useChooser
to false
and true
, with the only difference that in the first case no chooser shows up. Is this the way Android is supposed to work?
* app or browser (the default app associated with this link or the default | ||
* browser if there is no default app will be only shown in the system | ||
* chooser on Android 12 and higher) |
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.
* app or browser (the default app associated with this link or the default | |
* browser if there is no default app will be only shown in the system | |
* chooser on Android 12 and higher) | |
* app or browser (only the default app associated with this link, or the default | |
* browser if there is no default app, will be shown in the system | |
* chooser on Android 12 and higher) |
* web URL with false for the boolean param. | ||
* It uses {@link #openIntentInApp(Context, Intent, boolean)} to open the market scheme | ||
* and {@link #openUrlInBrowser(Context, String, boolean)} to open Google Play Store's | ||
* web URL with false for the boolean parameter. |
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.
* web URL with false for the boolean parameter. | |
* web URL without an app chooser. |
return resolveInfo.activityInfo.packageName; | ||
public static boolean openIntentInApp(@NonNull final Context context, | ||
@NonNull final Intent intent, | ||
final boolean showNoAppToOpenToast) { |
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.
What about creating two methods and removing this boolean
parameter? I would call the two methods:
tryOpenIntentInApp
which just doestry{...}catch{return false}; return true
openIntentInApp
which doesif (!tryOpenIntentInApp()) showToast
and has return typevoid
*/ | ||
public static boolean openUrlInBrowser(@NonNull final Context context, | ||
final String url, | ||
final boolean useChooser) { |
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.
Also here. I think it would be better to create two different methods, so that it is clear what's happening just by reading the code (no need to look up parameter documentation).
openUrlInDefaultBrowser
openUrlInBrowserWithChooser
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.
Not a bad idea, but I would use openUrlInDefaultApp
and openUrlWithAppChooser
, as we do not look at a browser anymore.
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.
Right
Yes, it is, starting with Android 12. Another solution would be to reintroduce query packages permission for |
Mmmh, then I guess this is the right thing to do, let's hope it does not create issues for users. I don't think we should implement non-standard things: users have the ability to copy the url e.g. from the share sheet (including older android versions if there is a "Copy to clipboard" app installed). Then I think the "Open in browser" button should become "Open in..." with this icon (or similar) |
Closing in favor of #9850, which aims to keep the current behavior as much as possible and embed the requested changes. |
What is it?
Description of the changes in your PR
This PR fixes opening streams on external players and downloads on Android 11 and higher and refactors how external intents are handled.
It does more precisely the following changes:
try
catch
blocks. As a result, the HTTP default browser test for web links has been removed;ACTION_SEND
intents;ShareUtils
,openAppChooser
, has been madepublic
and is now used when sharing a download inMissionAdapter
, in order to avoid code duplication;ShareUtils
have been also made: see the commits messages and the code changes for more details;queries
element in the Android manifest has been removed.toast_no_player
string resource has been removed, as it was only used inMissionAdapter
when trying to a play a download, and has been replaced by the generic oneNo app on your device can open this
with the usage of the methodopenIntentInApp
inShareUtils
instead of starting manually the intent.Fixes the following issue(s)
APK testing
The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR.
Please test that the changes work fine with this build, by using the following locations where external chooser and view intents are used:
I personally tested the changes on an Android 14 Developer Preview 1 emulator and my Honor 9X device, running EMUI 10.0 with Android 10, and everything about external intents I have been able to test was working as excepted.
Due diligence