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.

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

feat(android_intent_plus): support for intent as URI #2970

Merged
merged 12 commits into from
May 31, 2024

Conversation

jaumard
Copy link
Contributor

@jaumard jaumard commented May 30, 2024

Description

add support for parsing URI intent into android_intent_plus

Related Issues

Fix #2164

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I titled the PR using Conventional Commits.
  • I did not modify the CHANGELOG.md nor the plugin version in pubspec.yaml files.
  • All existing and new tests are passing.
  • The analyzer (flutter analyze) does not report any problems on my PR.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate that with a ! in the title as explained in Conventional Commits).
  • No, this is not a breaking change.

@jaumard jaumard changed the title Fix(android_intent_plus): support for intent as URI fix(android_intent_plus): support for intent as URI May 30, 2024
@miquelbeltran
Copy link
Member

Please follow the contribution guidelines.

  1. Don't modify the changelog.
  2. Don't modify the pubspec.
  3. Provide a description of the changes.
'intent://payment#Intent;action=ch.twint.action.TWINT_PAYMENT;scheme=twint;S.code=98815;S.startingOrigin=EXTERNAL_WEB_BROWSER;S.browser_fallback_url=;end';

This also looks suspicious, why include an intent to a payments' app with some obscure code.

@jaumard
Copy link
Contributor Author

jaumard commented May 30, 2024

Sorry I read it too fast and miss the NOT in I did not modify the CHANGELOG.md nor the plugin version in pubspec.yaml files. I can revert those.

This also looks suspicious, why include an intent to a payments' app with some obscure code.

I just took the one I had for my use case, it come from safer pay payment provider we uses. This code is internal to TWINT Swiss app usage, they use this code to verify a payment. I don't have other example to put in the example app, I can remove it completely if this is necessary but I needed that to test that it was actually working.

Why closing the PR instead of asking for changes? Should I bother make another one or just not use this package?

@miquelbeltran miquelbeltran reopened this May 30, 2024
Copy link
Member

@miquelbeltran miquelbeltran left a comment

Choose a reason for hiding this comment

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

Let's keep it in this PR.

I did a quick overview and marked some stuff to be changed and improved.

I will have to do an actual test on my device before approving.

packages/android_intent_plus/CHANGELOG.md Outdated Show resolved Hide resolved
packages/android_intent_plus/pubspec.yaml Outdated Show resolved Hide resolved
packages/android_intent_plus/example/lib/main.dart Outdated Show resolved Hide resolved
packages/android_intent_plus/example/pubspec.yaml Outdated Show resolved Hide resolved
@jaumard jaumard requested a review from miquelbeltran May 30, 2024 11:55
@jaumard
Copy link
Contributor Author

jaumard commented May 30, 2024

I have reverted pub spec of example project but now the CI is broken saying the method I added doesn't exist. I don't have that issue locally using Melos bootstrap command... not sure what happen

@miquelbeltran
Copy link
Member

No worries, this is another issue that should be solved on the CI side.

I pushed some changes. The happy path works, but I am unable to get a parsing exception no matter what strings I send.

I want to take a look at the native tests, I am not even sure if these run on CI or not, in case we want to add a test here.

@jaumard
Copy link
Contributor Author

jaumard commented May 30, 2024

@miquelbeltran to test the error case I used 'intent:#Intent;;end', with this I was able to see the error in the logs

@miquelbeltran
Copy link
Member

Thanks, that worked!

E/flutter (11114): [ERROR:flutter/runtime/dart_vm_initializer.cc(41)] Unhandled Exception: PlatformException(parse_error, Failed to parse URI, illegal Intent URI format at index 15: intent:#Intent;;end, null)
E/flutter (11114): #0      StandardMethodCodec.decodeEnvelope (package:flutter/src/services/message_codecs.dart:648:7)
E/flutter (11114): #1      MethodChannel._invokeMethod (package:flutter/src/services/platform_channel.dart:334:18)
E/flutter (11114): <asynchronous suspension>
E/flutter (11114): #2      AndroidIntent.parseAndLaunch (package:android_intent_plus/android_intent.dart:162:5)
E/flutter (11114): <asynchronous suspension>
E/flutter (11114): 

Copy link
Member

@miquelbeltran miquelbeltran left a comment

Choose a reason for hiding this comment

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

I think it looks good now, but I need to fix first the build scripts on main branch, so I will come back to merge this once that is fixed.

@miquelbeltran miquelbeltran changed the title fix(android_intent_plus): support for intent as URI feat(android_intent_plus): support for intent as URI May 30, 2024
@jaumard
Copy link
Contributor Author

jaumard commented May 30, 2024

No problem thanks for your help @miquelbeltran !

@miquelbeltran
Copy link
Member

Thanks to you!

@miquelbeltran miquelbeltran merged commit e453087 into fluttercommunity:main May 31, 2024
11 checks passed
@miquelbeltran
Copy link
Member

miquelbeltran commented May 31, 2024

Thanks again @jaumard for the contribution!

Merged this, it will probably take a while until we release this, but you can use git-ref in pubspec meanwhile: https://dart.dev/tools/pub/dependencies#git-packages

@jaumard
Copy link
Contributor Author

jaumard commented May 31, 2024

Thanks for looking at it quickly, I'll use the git dep until it's on pub. What is the strategy for your release?

@miquelbeltran
Copy link
Member

I think we should prepare a release batch sometime soon, but I need to talk with the other maintainers if there are any blockers

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.

[Bug]: The URI_INTENT_SCHEME of Android is not supported
2 participants