[file_selector] Convert iOS to Swift and SPM by stuartmorgan-g · Pull Request #6755 · flutter/packages

stuartmorgan-g · 2024-05-17T16:45:47Z

Converts the plugin and its tests to Swift, and re-adds the SPM support that was reverted due to modulemap issues.

In order to avoid the ugliness and loss of type saftey of using associated objects in the Swift version, this replaces that with a bridge object that serves as a delegate instead of the plugin, and manages its own lifetime in coordination with the plugin.

While this is one PR, the conversion was done in individual steps, each of which is a commit:

Rewrite just the tests in Swift, with no implementation changes, ensuring that everything still passed.
Rewrite the implementation in Swift, changing the tests only as necessary for the structural changes to the implementation due to the removal of associated objects
Re-add SPM

The changes in the generated Dart files are just due to updating to the latest version of Pigeon (to avoid writing the Swift implementation against an older version of the Swift API, and then having to update again later for breaking changes).

Part of flutter/flutter#119015
Fixes flutter/flutter#146903

Pre-launch Checklist

stuartmorgan-g · 2024-05-17T16:47:33Z

This is the easiest way to remove the use of modulemaps, right? 😁

Let me know if you want me to actually break this up into separate PRs. The overall change isn't actually that big so this seemed potentially easier, but I could easily split it along commit lines.

jmagman

@hellohuanlin could you review just the files FileSelectorPlugin.swift ViewPresenter.swift for idiomatic Swift?

LGTM

hellohuanlin · 2024-05-22T20:54:29Z

packages/file_selector/file_selector_ios/example/ios/RunnerTests/FileSelectorTests.swift

+
+@testable import file_selector_ios
+
+class TestViewPresenter: ViewPresenter {


nit: final

hellohuanlin · 2024-05-22T20:56:45Z

...r/file_selector_ios/ios/file_selector_ios/Sources/file_selector_ios/FileSelectorPlugin.swift

+    sendResult(
+      urls.map({ document in
+        document.path
+      }))


nit: sendResult(urls.map { $0.path })

Ah right, I forgot Swift has shorthand for this.

can also use key path urls.map(\.path) which is even shorter. sometimes i forget about it (no need to change tho)

hellohuanlin · 2024-05-22T21:00:53Z

...r/file_selector_ios/ios/file_selector_ios/Sources/file_selector_ios/FileSelectorPlugin.swift

+      ?? UIDocumentPickerViewController(
+        documentTypes: config.utis.map({ uti in
+          // See comment in messages.dart for why this is safe.
+          uti!


can cast directly config.utis as! [TypeOfThis] without the loop

Interesting, I guess I've internalized the Dart model of having to cast elements rather than the collections themselves.

hellohuanlin · 2024-05-22T21:01:16Z

...r/file_selector_ios/ios/file_selector_ios/Sources/file_selector_ios/FileSelectorPlugin.swift

+        documentTypes: config.utis.map({ uti in
+          // See comment in messages.dart for why this is safe.
+          uti!
+        }), in: UIDocumentPickerMode.import)


nit: in: .import

Done. Someday I'll internalize this one.

hellohuanlin · 2024-05-22T21:01:51Z

...r/file_selector_ios/ios/file_selector_ios/Sources/file_selector_ios/FileSelectorPlugin.swift

+    documentPicker.delegate = completionBridge
+
+    let presenter =
+      self.viewPresenterOverride ?? UIApplication.shared.delegate?.window??.rootViewController


is viewPresenterOverride created for testing purpose?

Yes, both to avoid actually having modal UI pop up (which causes issues when running multiple tests since we can't dismiss it) and to assert that it's called with expected values.

…s#6755)

flutter/packages@6525441...1008d9e 2024-05-23 engine-flutter-autoroll@skia.org Roll Flutter from 73bf206 to 8d955cd (24 revisions) (flutter/packages#6786) 2024-05-23 engine-flutter-autoroll@skia.org Roll Flutter (stable) from 5dcb86f to a14f74f (3 revisions) (flutter/packages#6784) 2024-05-23 stuartmorgan@google.com [file_selector] Convert iOS to Swift and SPM (flutter/packages#6755) 2024-05-23 49699333+dependabot[bot]@users.noreply.github.com [webview]: Bump androidx.webkit:webkit from 1.7.0 to 1.10.0 in /packages/webview_flutter/webview_flutter_android/android (flutter/packages#5996) 2024-05-23 engine-flutter-autoroll@skia.org Roll Flutter from d02292d to 73bf206 (31 revisions) (flutter/packages#6780) 2024-05-23 goderbauer@google.com [rfw] Adds support for `DecorationImage.filterQuality`. (flutter/packages#6781) 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

Convert tests to Swift

a8b038b

stuartmorgan-g requested a review from jmagman as a code owner May 17, 2024 16:45

github-actions bot added p: file_selector platform-ios labels May 17, 2024

stuartmorgan-g requested a review from bparrishMines as a code owner May 17, 2024 17:08

stuartmorgan-g force-pushed the file-selector-ios-swift branch from eb6df7f to cb91c01 Compare May 17, 2024 17:09

stuartmorgan-g added 3 commits May 17, 2024 13:10

Convert implementation to Swift

02b7305

Re-add SPM support

2b49872

Version bump

6ce49a3

stuartmorgan-g force-pushed the file-selector-ios-swift branch from cb91c01 to 6ce49a3 Compare May 17, 2024 17:10

stuartmorgan-g removed the request for review from bparrishMines May 17, 2024 17:20

jmagman approved these changes May 22, 2024

View reviewed changes

jmagman requested a review from hellohuanlin May 22, 2024 19:49

hellohuanlin approved these changes May 22, 2024

View reviewed changes

stuartmorgan-g added 2 commits May 22, 2024 20:17

Merge branch 'main' into file-selector-ios-swift

d518add

Review feedback

3dd7d9d

stuartmorgan-g added the autosubmit Merge PR when tree becomes green via auto submit App label May 23, 2024

auto-submit bot merged commit df16d4e into flutter:main May 23, 2024

engine-flutter-autoroll mentioned this pull request May 23, 2024

Roll Packages from 65254411e677 to df16d4e7bde3 (4 revisions) flutter/flutter#148951

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 23, 2024

df16d4e [file_selector] Convert iOS to Swift and SPM (flutter/package…

4c1e10d

…s#6755)

engine-flutter-autoroll mentioned this pull request May 23, 2024

Roll Packages from 65254411e677 to 13d111d92ad5 (5 revisions) flutter/flutter#148962

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 23, 2024

df16d4e [file_selector] Convert iOS to Swift and SPM (flutter/package…

2a0dbed

…s#6755)

engine-flutter-autoroll mentioned this pull request May 23, 2024

Roll Packages from 65254411e677 to 1008d9e9cd07 (6 revisions) flutter/flutter#148980

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 23, 2024

df16d4e [file_selector] Convert iOS to Swift and SPM (flutter/package…

d3b997e

…s#6755)

reidbaker mentioned this pull request Jul 2, 2024

i151188 update espresso dependencies v2 reidbaker/packages#6

Closed

11 tasks


		@testable import file_selector_ios

		class TestViewPresenter: ViewPresenter {

Conversation

stuartmorgan-g commented May 17, 2024

Pre-launch Checklist

Uh oh!

stuartmorgan-g commented May 17, 2024

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants