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

Conversation

@jddeep
Copy link
Contributor

@jddeep jddeep commented Feb 24, 2020

Description

updated _launchUniversalLinkIos to launch the url passed. It could also be done that we remove the url argument passed and open the hardcoded url. Either of the case needs to be done.

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.

updated _launchUniversalLinkIos  to launch the url passed. It could also be done that we remove the url argument passed and open the hardcoded url. Either of the case needs to be done.
@franciscojma86
Copy link
Contributor

LGTM but not sure if there might be an initial reason for having that hardcoded link. @cyanglaz added that snippet so might be the best person to approve.

If approved, please also update https://github.com/flutter/plugins/blob/master/packages/url_launcher/url_launcher_macos/example/lib/main.dart, which is a copy of that example.

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.

Thank you for the contribution! The bugfix itself LG.

This PR also needs tests before it can be merged, see Tree Hygiene #5. Can we possibly add some kind of driver test that checks that the link launching works? If not a unit test would also be OK.

It should also update the pubspec.yaml and CHANGELOG.md version so that we can publish the fixed version.

@jddeep
Copy link
Contributor Author

jddeep commented Feb 25, 2020

@mklim There is already a test written for universal links here - https://github.com/flutter/plugins/blob/master/packages/url_launcher/url_launcher/test/url_launcher_test.dart#L105 and it passes. I don't think we need to change this test anything drastically. Should I add anything more?:thought_balloon:
And sure, I will update the pubspec.yaml and CHANGELOG.md version for publishing.

@jddeep jddeep requested review from blasten and mklim February 25, 2020 09:39
@franciscojma86
Copy link
Contributor

@jddeep please change the macOS example app as well. I know it's another package, and a duplicated example is not optimal, but until we have a better solution we should try to keep them as equal as possible.
https://github.com/flutter/plugins/blob/master/packages/url_launcher/url_launcher_macos/example/lib/main.dart.

@jddeep
Copy link
Contributor Author

jddeep commented Feb 26, 2020

@franciscojma86 Done. Thanks.

@jddeep jddeep changed the title fix: url_launcher - updated _launchUniversalLinkIos [url_launcher] fix: url_launcher - updated _launchUniversalLinkIos Feb 26, 2020
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.

nit: The url_launcher_macos package also needs its pubspec.yaml and CHANGELOG updated too now.

For testing: that unit test only covers the plugin API itself, and not the behavior of the example app. If it had been covering this behavior it would have been broken before this PR fixed it. What we'd want for this patch is a little different, ideally we'd want a test that checks that the example app itself is launching the URLs correctly. Maybe something with a mock platform channel instance like that unit test you linked, but a widget test?

Thanks again for contributing.

@jddeep
Copy link
Contributor Author

jddeep commented Feb 26, 2020

@mklim So here is a widget test I was writing, but there were some doubts. How would we test whether the url is launchable or not once the button is tapped and is directed to a browser of OS? Also but the line await tester.tap(browserlaunchBtn); throws an error while running the test Failed assertion: boolean expression must not be null. What do you think?

    await tester.pumpWidget(MaterialApp(home:MyHomePage(
      title: 'Url launcher Widget Test',
    )));
      Finder browserlaunchBtn = find.widgetWithText(RaisedButton, 'Launch in browser');
      expect(browserlaunchBtn, findsOneWidget);
      await tester.tap(browserlaunchBtn);
  }); 

@jddeep jddeep requested a review from mklim February 26, 2020 14:51
@mklim
Copy link
Contributor

mklim commented Feb 27, 2020

@jddeep that's pretty close. So the missing piece there is that when you click on the button, it's still trying to actually call through the plugin to the platform code to launch the link. That error message you're seeing is the Flutter app expecting the plugin to return a bool and then panicking when it doesn't. For our purposes here a widget test is basically the same as a unit test. There is no underlying platform, so the plugin can't "really" work and really launch any links. So the thing to do here is to test that the button works is to mock out the Dart part of the plugin that would normally send the calls down to Java/ObjC to actually launch the link, and then verify that the mock Dart plugin interface received a call from the example app asking it to launch the link. I uploaded a working test doing this to mklim/plugins@6a9d4387cb200943043a67e7fce46d862e364650, feel free to patch it into your PR.

@jddeep
Copy link
Contributor Author

jddeep commented Feb 27, 2020

@mklim Thanks for the help. I see now. I have updated my test as well. Do we want tests for each button here or testing for one button to check if the url is launchable is cool? Do let me know or we can merge this up.

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.

LGTM! Thanks again for contributing and working on this. Just the test for the change in this PR is enough here.

@jddeep jddeep changed the title [url_launcher] fix: url_launcher - updated _launchUniversalLinkIos [url_launcher] fix: url_launcher - updated _launchUniversalLinkIos and Added Tests Feb 27, 2020
@mklim mklim merged commit 20ff19e into flutter:master Feb 28, 2020
EdwinRomelta pushed a commit to EdwinRomelta/plugins that referenced this pull request Jun 11, 2020
…d Added Tests (flutter#2552)

updated _launchUniversalLinkIos to launch the url passed. It could also be done that we remove the url argument passed and open the hardcoded url. Either of the case needs to be done.
FlutterSu pushed a commit to FlutterSu/flutter-plugins that referenced this pull request Nov 20, 2020
…d Added Tests (flutter#2552)

updated _launchUniversalLinkIos to launch the url passed. It could also be done that we remove the url argument passed and open the hardcoded url. Either of the case needs to be done.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants