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

Adding closeWebView feature to url_launcher #924

Merged
merged 24 commits into from
Nov 25, 2018
Merged

Adding closeWebView feature to url_launcher #924

merged 24 commits into from
Nov 25, 2018

Conversation

timtraversy
Copy link
Contributor

This pull request builds on the work of #658, mostly to address the edits suggested by @mehmetf. Started a new PR since I didn't have write access to thosakwe's branch, so I believe we can close that one and work here.

Changes:

  1. Android- Using BroadcastReceiver to communicate close request to WebView.
  2. iOS- Initiating SFSafariViewController during FLTUrlLaunchSession initiation.

And, like the original PR, this adds a test button to the example project that closes the web view after 5 seconds.

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

@timtraversy
Copy link
Contributor Author

@thosakwe looks like you need to confirm that you are okay with the use of your code here 😃

@thosakwe
Copy link

thosakwe commented Nov 23, 2018

@googlebot I am ok / approve / whatever magic word
👍

@mehmetf mehmetf added cla: yes and removed cla: no labels Nov 23, 2018
@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

@jogboms
Copy link

jogboms commented Nov 24, 2018

@timtraversy @thosakwe I'm having a hard time understanding the PR, i can see it allows closing the WebView from Dart code, was wondering how to close the WebView controller from within the controller.

@timtraversy
Copy link
Contributor Author

I don't think I understand the question. Are you wondering how to close the WebView from within the Android code? This plugin is supposed to abstract that so you only interact with Dart code as a developer.

Copy link
Contributor

@mehmetf mehmetf left a comment

Choose a reason for hiding this comment

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

Please update CHANGELOG and rev the pubspec.yaml version as well.

@mehmetf mehmetf added cla: yes and removed cla: no labels Nov 25, 2018
@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

@mehmetf mehmetf merged commit f057a22 into flutter:master Nov 25, 2018
@thosakwe
Copy link

thosakwe commented Nov 25, 2018 via email

@timtraversy timtraversy deleted the closeWebView branch November 26, 2018 05:13
@scimetfoo
Copy link

When can we expect 4.0.2 to be released?

@timtraversy
Copy link
Contributor Author

Pretty sure it’s live right now.

@scimetfoo
Copy link

Weird. I'm seeing this:

pub get failed (1) Because <application_name> depends on url_launcher ^4.0.2 which doesn't match any versions, version solving failed

@mehmetf
Copy link
Contributor

mehmetf commented Nov 26, 2018

Released to pub.

@grcevski
Copy link

I'm honestly not sure what this change is supposed to do. On Android it's a no-op and on iOS is completely broken. The following code:

  • (void)launchURLInVC:(NSString *)urlString result:(FlutterResult)result {
    NSURL *url = [NSURL URLWithString:urlString];
    _currentSession = [[FLTUrlLaunchSession alloc] initWithUrl:url withFlutterResult:result];
    [_viewController presentViewController:_currentSession.safari
    animated:YES
    completion:^void() {
    self->_currentSession = nil;
    }];
    }

Will kill _currentSession as soon as the webview is loaded, therefore when you call closeWebView you will not see a non-nil value unless you really killed it before the webpage loaded.

@timtraversy
Copy link
Contributor Author

Hm. I added that completion handler because I thought it was called when the user dismissed the VC, but now looking at the docs I see it it's called when the VC is finished loading. That completion handler needs to moved to the safariViewControllerDidFinish delegate method, which is called when the VC is dismissed. I will get to this soon, sorry it was missed.

@grcevski
Copy link

Thanks @timtraversy !

@KingIdee
Copy link

This still does not work for me.

I call await closeWebView();

Here is how I launch my app:

const url = 'random_url';
    if (await canLaunch(url)) {
      await launch(url,forceWebView: true,forceSafariVC: true, enableJavaScript: true);
    } else {
      throw 'Could not launch $url';
 }

@timtraversy
Copy link
Contributor Author

The iOS side of this wasn’t built correctly in this PR. I fixed it in #997 but that has to Ben merger before it will work.

@KingIdee
Copy link

Does this in any way make the calling screen destroy its state. I noticed something unusual while testing your solution though just for iOS.

@timtraversy
Copy link
Contributor Author

What occurred? The state was reset?

andreidiaconu pushed a commit to andreidiaconu/plugins that referenced this pull request Feb 17, 2019
andreidiaconu added a commit to andreidiaconu/plugins that referenced this pull request Feb 17, 2019
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.

8 participants