[url_launcher] migrating objc plugin to swift #4753

chrisdlangham · 2023-08-22T04:12:46Z

This PR converts the iOS portion of the url_launcher plugin from objc to swift.

Fixes flutter/flutter#119102

Pre-launch Checklist

stuartmorgan

Thanks for the contribution! Could you split out a new PR that contains just the migration of the tests to Swift, so we can land that first in a separate PR? Rewriting tests and implementation in the same PR is much, much riskier than doing each one at a time.

hellohuanlin · 2023-09-13T19:56:25Z

@chrisdlangham do we plan to rebase and update this PR?

chrisdlangham · 2023-09-14T01:55:04Z

@hellohuanlin Yes this is ready now

packages/url_launcher/url_launcher_ios/example/ios/RunnerTests/URLLauncherTests.swift

packages/url_launcher/url_launcher_ios/ios/Classes/Launcher.swift

hellohuanlin · 2023-10-13T18:14:01Z

packages/url_launcher/url_launcher_ios/ios/Classes/URLLauncherPlugin.swift

+    * @param url The invalid URL string
+    * @return The error to return
+    */
+  func invalidURLError(for url: String) -> LaunchResultDetails {


should this be private?

hellohuanlin · 2023-10-13T18:14:10Z

packages/url_launcher/url_launcher_ios/ios/Classes/URLLauncherPlugin.swift

+  private var currentSession: URLLaunchSession?
+  private let launcher: Launcher
+
+  var topViewController: UIViewController? {


should this be private?

hmmm, what's the diff between this func, and the one in UIViewController extension below?

chrisdlangham · 2023-10-19T05:11:43Z

I think I have addressed everything so far. Thanks for the feedback.
@stuartmorgan @hellohuanlin

hellohuanlin · 2023-10-23T19:38:11Z

packages/url_launcher/url_launcher_ios/example/ios/RunnerTests/URLLauncherTests.swift

-        XCTAssertEqual(error?.code, "argument_error")
-        XCTAssertEqual(error?.message, "Unable to parse URL")
-        XCTAssertEqual(error?.details as? String, "Provided URL: urls can't have spaces")
+        XCTAssertTrue(details == .failedToLoad || details == .invalidUrl)


which case will it actually be?

It depends on the iOS version the tests are being run on. I think @stuartmorgan can explain it better, but on iOS 17 we should get .failedToLoad, and on older versions, we should get .invalidUrl

Can you do "if iOS 17 else".

Unfortunately Swift apparently has no equivalent to Obj-C SDK preprocessor checks, and that's the determining factor. We don't want to switch on whether it's running on iOS 17, we want to switch on whether it was linked against the iOS 17 SDK, and that's apparently not expressible in Swift. (The best I can find is people suggesting mapping from the SDK to the version of Xcode that introduced that SDK, and then from there to the version of Swift introduced by that version of Xcode, but that's extremely gross since this has nothing to do with Swift language version.)

I realized there's a better option; I've added a utility method in the test file that checks the NSURL behavior directly, and then switches based on that in the tests.

hellohuanlin · 2023-10-23T19:40:01Z

packages/url_launcher/url_launcher_ios/ios/Classes/Launcher.swift

+}
+
+/// Default implementation of Launcher, using UIApplication.
+final class UIApplicationLauncher: Launcher {


can this simply be an empty extension?

extension UIApplication: Launcher {}

Nice! Done.

hellohuanlin · 2023-10-23T19:40:39Z

packages/url_launcher/url_launcher_ios/ios/Classes/URLLauncherPlugin.swift

+  }
+
+  func launchUrl(
+    url: String, universalLinksOnly: Bool,


hellohuanlin · 2023-10-23T19:41:08Z

packages/url_launcher/url_launcher_ios/ios/Classes/URLLauncherPlugin.swift

+  }
+
+  func openUrlInSafariViewController(
+    url: String, completion: @escaping (Result<LaunchResult, Error>) -> Void


line break. please go over all signatures and make sure comma separated items are in separate lines.

I've swift-formated the whole plugin.

hellohuanlin · 2023-10-23T19:41:39Z

packages/url_launcher/url_launcher_ios/ios/Classes/URLLauncherPlugin.swift

+    topViewController?.present(session.safariViewController, animated: true, completion: nil)
+  }
+
+  func closeSafariViewController() throws {


why is this throwing?

stuartmorgan · 2023-10-20T17:37:01Z

packages/url_launcher/url_launcher_ios/pigeons/messages.dart

+  /// The URL could not be launched
+  failedToLoad,
+
+  /// The URL was not launched because it is not invalid URL


"because the URL is invalid."

stuartmorgan · 2023-10-20T17:37:08Z

packages/url_launcher/url_launcher_ios/pigeons/messages.dart

+  /// The URL was successfully launched.
+  success,
+
+  /// The URL could not be launched


This is missing a period.

stuartmorgan · 2023-10-20T17:40:45Z

packages/url_launcher/url_launcher_ios/test/url_launcher_ios_test.dart

+      );
+      expect(
+          await launcher.launch(
+            'invalid://u r l',


Please don't use the URL that's used for invalid URL tests for a case where the return value isn't invalidUrl. (You could just use the same URL in every test now, but if you're going to keep the URLs that used to be special, please be consistent with them.)

stuartmorgan · 2023-10-20T17:42:12Z

packages/url_launcher/url_launcher_ios/lib/url_launcher_ios.dart

+    required String url,
+  }) {
+    // Replace this in https://github.com/flutter/flutter/issues/127665
+    // This is temporary since FlutterError is not a NSError.


This line can be removed; it's not temporary due to FlutterError. What's temporary is throwing a PlatformException at all.

stuartmorgan · 2023-10-20T17:42:20Z

packages/url_launcher/url_launcher_ios/lib/url_launcher_ios.dart

+  }) {
+    // Replace this in https://github.com/flutter/flutter/issues/127665
+    // This is temporary since FlutterError is not a NSError.
+    // The PlatformExceptions thrown here are for compatibility with the


stuartmorgan · 2023-10-20T17:42:56Z

packages/url_launcher/url_launcher_ios/lib/url_launcher_ios.dart

+    // Replace this in https://github.com/flutter/flutter/issues/127665
+    // This is temporary since FlutterError is not a NSError.
+    // The PlatformExceptions thrown here are for compatibility with the
+    // previous Objective-C implementation.


Please remove "Objective-C" since that's not true of the previous implementation.

stuartmorgan · 2023-10-20T17:43:41Z

packages/url_launcher/url_launcher_ios/lib/url_launcher_ios.dart

+    required final LaunchResult results,
+    required String url,
+  }) {
+    // Replace this in https://github.com/flutter/flutter/issues/127665


// TODO(stuartmorgan): Remove this as part of standardizing error handling. See
// flutter/flutter#127665

stuartmorgan · 2023-10-20T17:44:17Z

packages/url_launcher/url_launcher_ios/lib/url_launcher_ios.dart

+    required final LaunchResult results,
+    required String url,
+  }) {
+    // Replace this in https://github.com/flutter/flutter/issues/127665


Please move this entire comment block to be on the throw PlatformException rather than here, since that's the only part it applies to in this version.

chrisdlangham · 2023-10-26T05:05:14Z

I've noticed quite a few merge conflicts. I've been contemplating whether to continue this, and this has helped me make a decision. Unfortunately, I'm now facing a shortage of both time and energy for this. Please feel free to either close this or relocate it as needed.

stuartmorgan · 2023-10-26T14:08:14Z

@chrisdlangham Thanks for letting us know. This is basically done, and I definitely don't want to lose all the great work you've done on this, so I'll resolve the conflicts with my recent PR, and do any last adjustments to get it landed. We really appreciate the contribution!

Taking over PR; dismissing previous review

stuartmorgan · 2023-10-26T16:07:07Z

I think I've addressed the previous comments, so this should be ready for another review @hellohuanlin

hellohuanlin

LGTM!

…es#4753)

flutter/packages@fea24c5...2af6954 2023-10-26 katelovett@google.com [two_dimensional_scrollables] Add TableSpanPadding (flutter/packages#5039) 2023-10-26 engine-flutter-autoroll@skia.org Roll Flutter (stable) from 6c4930c to d211f42 (1 revision) (flutter/packages#5238) 2023-10-26 chrislangham@rightnow.org [url_launcher] migrating objc plugin to swift (flutter/packages#4753) 2023-10-26 ybz975218925@live.com [go_router_builder]Avoid losing NullabilitySuffix for typeArguments (flutter/packages#5215) 2023-10-26 engine-flutter-autoroll@skia.org Roll Flutter from 5dd2a4e to c555599 (14 revisions) (flutter/packages#5237) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC flutter-ecosystem@google.com,rmistry@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

This PR converts the iOS portion of the url_launcher plugin from objc to swift. *List which issues are fixed by this PR. You must list at least one issue.* flutter/flutter#119102

chrisdlangham added 8 commits August 18, 2023 17:07

converting url_launcher from objc to swift

1d5609f

converting tests to swift

d5727dd

converting tests to swift

c4e55fb

fixing issue where pigeon and method channels were not setup right

050d6b3

formatting

5278199

reverting unintentional local changes

55a345b

fixing issues with Ui tests

8356d7a

formating

120b2d4

chrisdlangham requested a review from stuartmorgan as a code owner August 22, 2023 04:12

Merge branch 'main' into coverting-objc-to-swift

1316f3c

github-actions bot added platform-ios p: url_launcher labels Aug 22, 2023

updating version and change log

7bd3553

stuartmorgan requested changes Aug 22, 2023

View reviewed changes

chrisdlangham added 6 commits August 22, 2023 12:16

converting tests to swift

2741de4

converting unit tests to swift

495de26

Merge branch 'main' into converting-url-launcher-ios-tests-to-swift

c898698

updating change log

6a6c8d5

resolving merge conflicts

254b9af

formatting

869f18f

stuartmorgan requested a review from hellohuanlin August 28, 2023 19:24

chrisdlangham added 3 commits August 29, 2023 00:14

making test class final and private

1120baa

resolving merge conflicts

9f591e3

updated tests and formated pigeon file

13cd002

chrisdlangham and others added 3 commits September 13, 2023 17:39

updates changelog

f8f5f0b

Update CHANGELOG.md

827204a

Merge branch 'main' into coverting-objc-to-swift

30568fd

stuartmorgan self-requested a review September 14, 2023 14:41

hellohuanlin reviewed Oct 13, 2023

View reviewed changes

chrisdlangham added 2 commits October 18, 2023 22:44

Merge branch 'main' into coverting-objc-to-swift

7a2fac5

addresses feedback

4d4f1bb

hellohuanlin reviewed Oct 23, 2023

View reviewed changes

stuartmorgan previously requested changes Oct 23, 2023

View reviewed changes

addressing feedback

7b6272f

Merge branch 'main' into coverting-objc-to-swift

b91dca7

stuartmorgan added 4 commits October 26, 2023 10:33

Replace default launcher implementation with conformance extension

e7e011a

swift-format

d955ef8

Rework return enum to have different versions

1297683

Improve invalid URL testing

43966a0

stuartmorgan requested review from stuartmorgan and hellohuanlin October 26, 2023 16:06

hellohuanlin approved these changes Oct 26, 2023

View reviewed changes

stuartmorgan approved these changes Oct 26, 2023

View reviewed changes

stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 26, 2023

auto-submit bot merged commit 3aff029 into flutter:main Oct 26, 2023
79 checks passed

chrisdlangham deleted the coverting-objc-to-swift branch October 26, 2023 21:20

engine-flutter-autoroll mentioned this pull request Oct 27, 2023

Roll Packages from fea24c5b93b4 to 2af69541a214 (5 revisions) flutter/flutter#137421

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 27, 2023

3aff029 [url_launcher] migrating objc plugin to swift (flutter/packag…

39b530d

…es#4753)

reidbaker mentioned this pull request Jan 12, 2024

Dialog Debug #5881

Closed

reidbaker mentioned this pull request Jan 19, 2024

[in_app_purchase] Add play country code api #5941

Merged

12 tasks

reidbaker mentioned this pull request Feb 19, 2024

[in_app_purchase_android] Add UserChoiceBilling mode. #6162

Merged

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[url_launcher] migrating objc plugin to swift #4753

[url_launcher] migrating objc plugin to swift #4753

chrisdlangham commented Aug 22, 2023 •

edited by stuartmorgan

Loading

stuartmorgan left a comment

hellohuanlin commented Sep 13, 2023

chrisdlangham commented Sep 14, 2023

hellohuanlin Oct 13, 2023

hellohuanlin Oct 13, 2023

hellohuanlin Oct 13, 2023

chrisdlangham commented Oct 19, 2023

hellohuanlin Oct 23, 2023

chrisdlangham Oct 26, 2023

hellohuanlin Oct 26, 2023

stuartmorgan Oct 26, 2023

stuartmorgan Oct 26, 2023

hellohuanlin Oct 23, 2023

stuartmorgan Oct 26, 2023

hellohuanlin Oct 23, 2023

hellohuanlin Oct 23, 2023

stuartmorgan Oct 26, 2023

hellohuanlin Oct 23, 2023

stuartmorgan Oct 20, 2023

stuartmorgan Oct 20, 2023

stuartmorgan Oct 20, 2023

stuartmorgan Oct 20, 2023

stuartmorgan Oct 20, 2023

stuartmorgan Oct 20, 2023

stuartmorgan Oct 20, 2023

stuartmorgan Oct 20, 2023

chrisdlangham commented Oct 26, 2023

stuartmorgan commented Oct 26, 2023

stuartmorgan commented Oct 26, 2023

hellohuanlin left a comment

[url_launcher] migrating objc plugin to swift #4753

[url_launcher] migrating objc plugin to swift #4753

Conversation

chrisdlangham commented Aug 22, 2023 • edited by stuartmorgan Loading

Pre-launch Checklist

stuartmorgan left a comment

Choose a reason for hiding this comment

hellohuanlin commented Sep 13, 2023

chrisdlangham commented Sep 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisdlangham commented Oct 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisdlangham commented Oct 26, 2023

stuartmorgan commented Oct 26, 2023

stuartmorgan commented Oct 26, 2023

hellohuanlin left a comment

Choose a reason for hiding this comment

chrisdlangham commented Aug 22, 2023 •

edited by stuartmorgan

Loading