[file_selector] Basic iOS implementation #5448

spesholized · 2022-04-29T19:27:15Z

Initial iOS implementation.

Fixes flutter/flutter#102777

Pre-launch Checklist

packages/file_selector/file_selector_ios/example/ios/Runner.xcodeproj/project.pbxproj

…odeproj/project.pbxproj Co-authored-by: Jenn Magder <[email protected]>

spesholized · 2022-05-12T20:58:56Z

Friendly ping! @jmagman @stuartmorgan

stuartmorgan

Thanks for the submission! See comments inline.

stuartmorgan · 2022-05-13T19:52:54Z

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

+
+- (void)testPlugin {
+  FTLFileSelectorPlugin *plugin = [[FTLFileSelectorPlugin alloc] init];
+  XCTAssertNotNil(plugin);


Tests that look like this are the result of flutter/flutter#83357

New code needs actual unit tests. See local_auth or camera for real examples. (Maybe also file_selector_macos, for a more relevant domain but the wrong language.)

packages/file_selector/file_selector_ios/lib/file_selector_ios.dart

stuartmorgan · 2022-05-13T20:01:31Z

packages/file_selector/file_selector_ios/ios/Classes/FTLFileSelectorPlugin.m

+  if ([@"openFile" isEqualToString:call.method]) {
+    [self openPickerWithCall:call result:result allowMultiSelection:NO];
+  } else if ([@"openFiles" isEqualToString:call.method]) {
+    [self openPickerWithCall:call result:result allowMultiSelection:YES];


The platform channel layer doesn't need to directly mirror the platform interface. While you'll see that in many of our plugins, it's almost entirely for historical reasons, and it's not something we want to do in new plugins when it doesn't actually make sense. It's pretty clear here that a single method call with a boolean argument would be a much better representation; you can do that mapping in the Dart code (which is easier to test/debug in general) instead of native code.

stuartmorgan · 2022-05-13T20:06:47Z

packages/file_selector/file_selector_ios/lib/file_selector_ios.dart

+    for (final XTypeGroup typeGroup in typeGroups) {
+      // If any group allows everything, no filtering should be done.
+      if ((typeGroup.extensions?.isEmpty ?? true) &&
+          (typeGroup.macUTIs?.isEmpty ?? true) &&


This turns out to have been poor naming :( Filed flutter/flutter#103743

(Doesn't need to block this PR, just leaving here for future reference.)

stuartmorgan · 2022-05-13T20:07:44Z

packages/file_selector/file_selector_ios/lib/file_selector_ios.dart

+
+  // Converts the type group list into a flat list of all allowed types, since
+  // iOS doesn't support filter groups.
+  Map<String, List<String>>? _allowedTypeListFromTypeGroups(


It looks like you're only actually using UTI, so you can just make this a List<String> of UTIs, and remove a level of nesting at both ends of the channel.

stuartmorgan · 2022-05-13T20:18:32Z

packages/file_selector/file_selector_ios/lib/file_selector_ios.dart

+    String? suggestedName,
+    String? confirmButtonText,
+  }) async {
+    return _channel.invokeMethod<String>(


Why call a method you know doesn't exist? You can just throw UnimplementedError. Or even just not override getSavePath in the first place, since that's what the superclass does.

Done. Removed unused overrides.

stuartmorgan · 2022-05-13T20:18:36Z

packages/file_selector/file_selector_ios/lib/file_selector_ios.dart

+    String? initialDirectory,
+    String? confirmButtonText,
+  }) async {
+    return _channel.invokeMethod<String>(


stuartmorgan · 2022-05-13T20:19:02Z

packages/file_selector/file_selector_ios/pubspec.yaml

+
+environment:
+  sdk: ">=2.14.4 <3.0.0"
+  flutter: ">=2.0.0"


For a new plugin, use 2.8

stuartmorgan · 2022-05-13T20:19:44Z

packages/file_selector/file_selector_ios/pubspec.yaml

+        pluginClass: FTLFileSelectorPlugin
+
+dependencies:
+  cross_file: ^0.3.1


Do you need this dependency? f_s_p_i should be handling that.

stuartmorgan · 2022-05-13T20:19:53Z

packages/file_selector/file_selector_ios/pubspec.yaml

+dev_dependencies:
+  flutter_lints: ^1.0.0
+  flutter_test:
+    sdk: flutter


Please add the missing newline.

stuartmorgan

Mostly this looks good at a high level; specific comments inline. I didn't really review the Dart unit tests since they need to be rewritten.

stuartmorgan · 2022-06-07T19:39:30Z

packages/file_selector/file_selector_ios/lib/file_selector_ios.dart

+
+import 'src/messages.g.dart';
+
+const MethodChannel _channel =


This is cruft, and can be removed along with the method to access it.

stuartmorgan · 2022-06-07T19:41:21Z

packages/file_selector/file_selector_ios/test/file_selector_ios_test.dart

+
+  final FileSelectorIOS plugin = FileSelectorIOS();
+
+  final List<MethodCall> log = <MethodCall>[];


For an example of what writing new tests Dart tests for a Pigeon implementation should probably look like, see https://github.com/stuartmorgan/plugins/blob/file-selector-windows-pigeon/packages/file_selector/file_selector_windows/test/file_selector_windows_test.dart (which is from a WIP PR to convert Windows file_selector to Pigeon; our currently-landed examples are mostly straight conversions of tests from this style to minimize test changes when re-implementing).

Done. I also removed some of the tests (that were copied from macOS version) because only openFile and openFiles are implemented at the moment.

stuartmorgan · 2022-06-07T19:45:26Z

packages/file_selector/file_selector_ios/example/integration_test/file_selector_test.dart

+import 'package:integration_test/integration_test.dart';
+
+void main() {
+  IntegrationTestWidgetsFlutterBinding.ensureInitialized();


In cases like this where there's no integration test we can run from the Dart side, instead of stub files you should add an entry to script/configs/exclude_integration_ios.yaml with an explanatory comment (which for desktop implementations of file_selector was # Can't use Flutter integration tests due to native modal UI.).

That way our CI output will report (correctly) that it was excluded from tests, rather that reporting a misleading output that all tests passed.

stuartmorgan · 2022-06-07T19:51:51Z

packages/file_selector/file_selector_ios/pigeons/messages.dart

+  objcHeaderOut: 'ios/Classes/messages.g.h',
+  objcSourceOut: 'ios/Classes/messages.g.m',
+  objcOptions: ObjcOptions(
+    prefix: 'FLT',


We're using per-plugin prefixes (F for Flutter plus two letters based on the plugin name) going forward, to reduce the possibility of inter-plugin collisions, so this should be FFS. Same for the plugin files/classes that are named manually.

stuartmorgan · 2022-06-07T19:52:51Z

packages/file_selector/file_selector_ios/pigeons/messages.dart

+))
+class FileSelectorConfig {
+  FileSelectorConfig(this.utis, this.allowMultiSelection);
+  List<String?>? utis;


In Dart we prefer to make collections non-nullable, so that we don't have two ways of representing the same thing (null and empty), and defaulting to empty collections.

stuartmorgan · 2022-06-07T20:27:01Z

packages/file_selector/file_selector_ios/lib/file_selector_ios.dart

+            utis: _allowedUtiListFromTypeGroups(acceptedTypeGroups),
+            allowMultiSelection: false)))
+        ?.cast<String>();
+    return path == null ? null : XFile(path.first);


And this is why we have a general policy against nullable collections: this line only correctly handles one form of "empty" (null), not the other, making it very fragile.

stuartmorgan · 2022-06-07T20:29:12Z

packages/file_selector/file_selector_ios/lib/file_selector_ios.dart

+
+  // Converts the type group list into a list of all allowed UTIs, since
+  // iOS doesn't support filter groups.
+  List<String>? _allowedUtiListFromTypeGroups(List<XTypeGroup>? typeGroups) {


List<String>

packages/file_selector/file_selector_ios/lib/file_selector_ios.dart

stuartmorgan · 2022-06-07T20:30:30Z

packages/file_selector/file_selector_ios/lib/file_selector_ios.dart

+  // iOS doesn't support filter groups.
+  List<String>? _allowedUtiListFromTypeGroups(List<XTypeGroup>? typeGroups) {
+    if (typeGroups == null || typeGroups.isEmpty) {
+      return null;


And then these should return empty collections.

packages/file_selector/file_selector_ios/lib/file_selector_ios.dart

cyanglaz · 2022-06-22T20:46:20Z

@spesholized Friendly ping. Have you gotten a chance to look at Stuart's comment?

spesholized · 2022-06-22T22:22:54Z

Sorry for the delay, I've been busy with something else in the past couple of weeks but I do want to get this change in soon. I should have some time to review and address the comments in a few days.

…e group handling

… document picker callback.

stuartmorgan

I synced in some recent changes from master, and fixed a few minor issues; with that, LTGM!

@jmagman This is ready for a second review.

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

packages/file_selector/file_selector_ios/ios/file_selector_ios.podspec

packages/file_selector/file_selector_ios/ios/Classes/FFSFileSelectorPlugin.m

packages/file_selector/file_selector_ios/ios/Classes/file_selector_ios-umbrella.h

packages/file_selector/file_selector_ios/AUTHORS

stuartmorgan

(I'm adopting this since the author is out for the moment.)

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

packages/file_selector/file_selector_ios/ios/file_selector_ios.podspec

packages/file_selector/file_selector_ios/ios/Classes/FFSFileSelectorPlugin.m

packages/file_selector/file_selector_ios/ios/Classes/file_selector_ios-umbrella.h

packages/file_selector/file_selector_ios/AUTHORS

jmagman

LGTM once the formatter is happy

jmagman · 2022-08-22T17:54:59Z

packages/file_selector/file_selector_ios/ios/Classes/file_selector_ios-umbrella.h

Formatter didn't like this:

These files are not formatted correctly (see diff below): file_selector/file_selector_ios/ios/Classes/file_selector_ios-umbrella.h To fix run "pub global activate flutter_plugin_tools && pub global run flutter_plugin_tools format" or copy-paste this command into your terminal: patch -p1 <<DONE diff --git a/packages/file_selector/file_selector_ios/ios/Classes/file_selector_ios-umbrella.h b/packages/file_selector/file_selector_ios/ios/Classes/file_selector_ios-umbrella.h index 122724e46..d79d3642b 100644 --- a/packages/file_selector/file_selector_ios/ios/Classes/file_selector_ios-umbrella.h +++ b/packages/file_selector/file_selector_ios/ios/Classes/file_selector_ios-umbrella.h @@ -3,5 +3,4 @@ // found in the LICENSE file. #import <Foundation/Foundation.h> - #import <file_selector_ios/FFSFileSelectorPlugin.h> DONE

Hm, clang-format must assume that any framework-style #import should be a single block. Oh well.

)

spesholized and others added 5 commits April 28, 2022 12:22

Basic iOS implementation

6e7f572

Merge branch 'flutter:main' into main

b4e8697

[file_selector] Basic iOS implementation

75a1bdc

Merge branch 'main' of https://github.com/spesholized/plugins

15f2741

Fix formatting and update environment sdk version

62c60cb

github-actions bot added p: file_selector platform-ios labels Apr 29, 2022

spesholized force-pushed the file_selector_ios branch from cbc69f4 to 1166325 Compare April 29, 2022 19:31

Basic native unit test for plugin

696cb78

spesholized force-pushed the file_selector_ios branch from 1166325 to 696cb78 Compare April 29, 2022 20:27

jmagman reviewed Apr 30, 2022

View reviewed changes

packages/file_selector/file_selector_ios/example/ios/Runner.xcodeproj/project.pbxproj Outdated Show resolved Hide resolved

spesholized and others added 2 commits May 1, 2022 23:18

Update packages/file_selector/file_selector_ios/example/ios/Runner.xc…

11504f4

…odeproj/project.pbxproj Co-authored-by: Jenn Magder <[email protected]>

Add placeholder integration test

252e110

spesholized marked this pull request as ready for review May 3, 2022 18:30

stuartmorgan self-requested a review May 4, 2022 01:24

spesholized requested a review from jmagman May 9, 2022 19:06

stuartmorgan suggested changes May 13, 2022

View reviewed changes

spesholized added 3 commits May 25, 2022 10:28

Remove unused overrides in file_selector_ios.dart, update pubspec

cb76a80

Use pigeon, fix Obj-C prefix typo

ecfad9a

Add native unit tests

4ecda0e

spesholized requested a review from stuartmorgan May 27, 2022 00:32

stuartmorgan suggested changes Jun 7, 2022

View reviewed changes

stuartmorgan mentioned this pull request Jun 13, 2022

[camera] Switch to internal method channels #5943

Merged

11 tasks

spesholized added 2 commits August 2, 2022 13:22

Update tests to use pigeon, change prefix to FFS, update wildcard typ…

2a69911

…e group handling

Use non-null properties in messages.dart, use objc associated for the…

38742a3

… document picker callback.

spesholized requested a review from stuartmorgan August 2, 2022 22:00

Merge branch 'main' into file_selector_ios

ec291b6

stuartmorgan added 6 commits August 19, 2022 13:37

Remove local analysis options

f0bbcf1

Resync examples and tweak UTIs

edc2881

Analysis fixes

e521edc

Autoformat

eabcce5

Missing copyright header

d39d953

Supress deprecation warning on legacy codepath

170c260

stuartmorgan approved these changes Aug 19, 2022

View reviewed changes

jmagman reviewed Aug 20, 2022

View reviewed changes

Review comments

129a097

stuartmorgan reviewed Aug 21, 2022

View reviewed changes

jmagman approved these changes Aug 22, 2022

View reviewed changes

Format

0cfa61c

stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 22, 2022

auto-submit bot merged commit 8f3749c into flutter:main Aug 22, 2022

engine-flutter-autoroll mentioned this pull request Aug 23, 2022

Roll Plugins from 89fe2a2c0b09 to 3a5e6f3acdd3 (4 revisions) flutter/flutter#110092

Merged

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

8f3749c36 [file_selector] Basic iOS implementation (flutter/plugins#5448

990864e

)

jmagman mentioned this pull request Aug 23, 2022

[ci] Add file_selector_ios CODEOWNER #6306

Merged

11 tasks

moisefeelin pushed a commit to feelinproject/plugins that referenced this pull request Aug 26, 2022

[file_selector] Basic iOS implementation (flutter#5448)

9f2fb6d

mauricioluz pushed a commit to mauricioluz/plugins that referenced this pull request Jan 26, 2023

[file_selector] Basic iOS implementation (flutter#5448)

e981581


		final FileSelectorIOS plugin = FileSelectorIOS();

		final List<MethodCall> log = <MethodCall>[];

[file_selector] Basic iOS implementation #5448

[file_selector] Basic iOS implementation #5448

Conversation

spesholized commented Apr 29, 2022

Pre-launch Checklist

spesholized commented May 12, 2022

stuartmorgan left a 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

stuartmorgan left a 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

cyanglaz commented Jun 22, 2022

spesholized commented Jun 22, 2022

stuartmorgan left a comment

Choose a reason for hiding this comment

stuartmorgan left a comment

Choose a reason for hiding this comment

jmagman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment