Nx Cloud Report

CI is running for commit e2d9154.

📂 Click to track the progress, see the status, the terminal output, and the build insights.

Sent with 💌 from NxCloud.

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign the CLA at https://www.nativescript.org/cla.
CLA has not been signed by users: @prabudevarrajan1.
After signing the CLA, you can ask me to recheck this PR by posting @cla-bot check as a comment to the PR.

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign the CLA at https://www.nativescript.org/cla.
CLA has not been signed by users: @prabudevarrajan1.
After signing the CLA, you can ask me to recheck this PR by posting @cla-bot check as a comment to the PR.

@NathanWalker Updated as per your comments. Please review and merge accordingly

@cla-bot check

The cla-bot has been summoned, and re-checked this pull request!

I'm quite against merging this one. We duplicate part of code that already resides in openUrl.
I believe we should enhance method openUrl to actually check for the existence of an activity that supports schema using package manager. Then, we could return the boolean result of openUrl itself in shouldOverrideUrl (which should fix your reported problem).

@prabudevarrajan I had plans to work on this one but I'm okay with your PR too if you pay more attention to my requested changes. I could even create a PR tonight and see if you agree to my changes.

Note: Permission QUERY_ALL_PACKAGES is quite dangerous. Everything is fine as long as apps contain queries for the schemas they have support for.

I'm quite against merging this one. We duplicate part of code that already resides in openUrl. I believe we should enhance method openUrl to actually check for the existence of an activity that supports schema using package manager. Then, we could return the boolean result of openUrl itself in shouldOverrideUrl (which should fix your reported problem).

@prabudevarrajan I had plans to work on this one but I'm okay with your PR too if you pay more attention to my requested changes. I could even create a PR tonight and see if you agree to my changes.

Note: Permission QUERY_ALL_PACKAGES is quite dangerous. Everything is fine as long as apps contain queries for the schemas they have support for.

@CatchABus As per your comments, I have updated the logic accordingly.

Regarding QUERY_ALL_PACKAGES, it's not recommended unless the user is aware of what he is doing. I have updated the trace log accordingly.

I dont think QUERY_ALL_PACKAGES is necessary here or should even be mentioned. As @CatchABus said it is a dangerous perm in the sense that google is likely to refuse your app. The trace error should remove mention of it. It would wrongly induce users into thinking they need that permission when they dont

I dont think QUERY_ALL_PACKAGES is necessary here or should even be mentioned. As @CatchABus said it is a dangerous perm in the sense that google is likely to refuse your app. The trace error should remove mention of it. It would wrongly induce users into thinking they need that permission when they dont

Yup agreed. I have updated the trace error message accordingly.

@prabudevarrajan Can you take a look at this one? #10148 (comment)

I think we're done if setComponent gets added and works.

For the record, actual fix resides in shouldOverrideUrlLoading.

Changes in openUrl helper do not alter behaviour and are improvement changes.
Before this change, method handled the concept of unsupported schema with try-catch clause when startActivity call threw exception.

Nice job @prabudevarrajan !