Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[android_intent] add flags option #2000

Merged
mklim merged 4 commits into from
Aug 28, 2019
Merged

[android_intent] add flags option #2000

mklim merged 4 commits into from
Aug 28, 2019

Conversation

@thanhdatvo
Copy link
Contributor

@thanhdatvo thanhdatvo commented Aug 22, 2019

Description

we are launching to a different package we need to set the FLAG_ACTIVITY_NEW_TASK flag

Similar to react native Link.openUrl() : https://github.com/facebook/react-native/blob/3b6f6ca4d5fcee6f1bc6d6242e3e2ef136e4d546/ReactAndroid/src/main/java/com/facebook/react/modules/intent/IntentModule.java#L97

Related Issues

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the CLA.
  • I am willing to follow-up on review comments in a timely manner.

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 a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

@thanhdatvo thanhdatvo requested a review from mklim as a code owner August 22, 2019 11:23
@thanhdatvo thanhdatvo mentioned this pull request Aug 22, 2019
Closed
13 tasks
ened
ened reviewed Aug 22, 2019
View reviewed changes
Copy link
Contributor

@ened ened left a comment

Choose a reason for hiding this comment

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

Thank you for adding a test, I think this is a great addition.

Did you increase the version in pubspec.yaml?

packages/android_intent/test/android_intent_test.dart Outdated
});
group('AndroidIntent', () {
test('pass right params', () async {
if (Platform.isIOS) {
Copy link
Contributor

@ened ened Aug 22, 2019

Choose a reason for hiding this comment

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

I would suggest to split this into two tests

tests invalid platform (you need to supply a MockPlatform for iOS)
and
pass right params (just with mock platform for Android) and remove the if/else.

MockPlatform details can be seen in the accompanying test to https://github.com/flutter/plugins/blob/master/packages/firebase_messaging/lib/firebase_messaging.dart.

packages/android_intent/test/android_intent_test.dart Outdated
}
});
test('pass wrong params', () async {
if (Platform.isIOS) {
Copy link
Contributor

@ened ened Aug 22, 2019

Choose a reason for hiding this comment

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

The if/else could be omitted if implemented as noted above

@thanhdatvo thanhdatvo force-pushed the feature/android_intent_add_flag branch from b08f38f to f083a21 Compare August 26, 2019 03:44
@thanhdatvo
Copy link
Contributor Author

thanhdatvo commented Aug 26, 2019 •
edited
Loading

Hi @ened , sorry to make you wait. I have:

  • pumped version to 0.3.3
  • added mock platform to test
  • updated syntax => const AndroidIntent

@ened
Copy link
Contributor

ened commented Aug 28, 2019

I think this is a great PR!

@collinjackson, @mklim WDYT - ready to merge?

@mklim mklim self-assigned this Aug 28, 2019
mklim
mklim approved these changes Aug 28, 2019
View reviewed changes
Copy link
Contributor

@mklim mklim left a comment

Choose a reason for hiding this comment

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

@datvt9312 thank you for the contribution and for adding tests!

@ened thanks for reviewing this! It was really important to help land this.

Here's the LGTM you need from a Flutter team member. :)

@mklim mklim added submit queue The Flutter team is in the process of landing this PR. and removed in review labels Aug 28, 2019
@mklim mklim merged commit b76fe5d into flutter:master Aug 28, 2019
mormih pushed a commit to mormih/plugins that referenced this pull request Nov 17, 2019
@thanhdatvo @mormih
[android_intent] add flags option (flutter#2000)
9563255 
Added "flags" option to call intent.addFlags(int) in native.
sungmin-park pushed a commit to sungmin-park/flutter-plugins that referenced this pull request Dec 17, 2019
@thanhdatvo
[android_intent] add flags option (flutter#2000)
0ed0aaa 
Added "flags" option to call intent.addFlags(int) in native.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@cyanglaz cyanglaz Awaiting requested review from cyanglaz

@iskakaushik iskakaushik Awaiting requested review from iskakaushik

2 more reviewers

@ened ened ened left review comments

@mklim mklim mklim approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@mklim mklim

Labels

cla: yes submit queue The Flutter team is in the process of landing this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@thanhdatvo @ened @mklim @googlebot