[file_selector] Update file_selector_platform_interface by adding the new property 'uniformTypeIdentifiers' to the XTypeGroup. #6433

juandausa · 2022-09-15T20:00:48Z

Update file_selector_platform_interface by adding the new property 'uniformTypeIdentifiers' to the XTypeGroup.

Update file_selector_platform_interface by adding the new property 'uniformTypeIdentifiers'.

List which issues are fixed by this PR. You must list at least one issue.

Rename XTypeGroup's macUTIs #103743

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

stuartmorgan-g

Thanks for the submission!

stuartmorgan-g · 2022-09-19T18:10:27Z

.../file_selector/file_selector_platform_interface/lib/src/types/x_type_group/x_type_group.dart

@@ -26,7 +26,7 @@ class XTypeGroup {
  final List<String>? mimeTypes;

  /// The UTIs for this group
-  final List<String>? macUTIs;
+  List<String>? macUTIs;


This should stay final; there shouldn't be a setter for it. I forgot this was a conceptually immutable class when I wrote that example of forwarding constructs in the issue.

(Filed flutter/flutter#111906 since this change shouldn't have been possible in the first place.)

stuartmorgan-g · 2022-09-19T18:13:47Z

.../file_selector/file_selector_platform_interface/lib/src/types/x_type_group/x_type_group.dart

@@ -50,6 +50,14 @@ class XTypeGroup {
        (webWildCards?.isEmpty ?? true);
  }

+  /// Returns the list of Uniform Type Identifiers for this group.
+  List<String>? get uniformTypeIdentifiers => macUTIs;


The field should be renamed, and the aliasing be done in the other direction, because the alias will be deprecated later, not the field.

Relatedly, the constructor needs to be updated to allow passing uniformTypeIdentifiers.

The behavior needs to be that we assert that they aren't both non-null, and then the field is set to uniformTypeIdentifiers ?? macUTIs.

Hi Stuart, currently the constructor allows null values for the macUTIs field. Do you mean that if both macUTIs and uniformTypeIdentifiers are null, we should just throw an exception? Wouldn't it be a breaking change?

I've been thinking about this, and now I understand that you meant if both are not null we should throw an exception, shouldn't we?

Yes, that's correct.

stuartmorgan-g · 2022-09-19T18:15:21Z

packages/file_selector/file_selector_platform_interface/test/x_type_group_test.dart

-      final List<String> extensions = <String>['txt', 'jpg'];
-      final List<String> mimeTypes = <String>['text/plain'];
-      final List<String> macUTIs = <String>['public.plain-text'];
-      final List<String> webWildCards = <String>['image/*'];


Please revert this change, and just inline whatever values you need in the new tests. Tests should generally be as localized as possible, to make them easy to understand and change in isolation.

packages/file_selector/file_selector_platform_interface/test/x_type_group_test.dart

stuartmorgan-g · 2022-09-19T18:29:49Z

packages/file_selector/file_selector_platform_interface/CHANGELOG.md

@@ -1,5 +1,6 @@
-## NEXT
+## 2.1.1


Oh, and this should actually be 2.2.0 since it's new API.

adpinola · 2022-10-11T15:28:10Z

Hi @stuartmorgan we introduced the changes you requested, can you take a look again? Thanks.

stuartmorgan-g

Minor wording nits, and the test needs to be auto-formatted, but otherwise looks good!

stuartmorgan-g · 2022-10-11T17:33:14Z

packages/file_selector/file_selector_platform_interface/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 2.3.0
+
+* Adds the `uniformTypeIdentifiers` property to the `XTypeGroup` that relies on `macUTIs`. The last will be deprecated for future releases.


I think this would be clearer as "Replaces macUTIs with uniformTypeIdentifiers. macUTIs is available as an alias, but will be deprecated in a future release."

This makes it clear that this is conceptually a replacement rather than a new property.

stuartmorgan-g · 2022-10-11T17:34:25Z

.../file_selector/file_selector_platform_interface/lib/src/types/x_type_group/x_type_group.dart

-  }) : _extensions = extensions;
+  })  : _extensions = extensions,
+        assert(uniformTypeIdentifiers == null || macUTIs == null,
+            'It is only allowed to specify either macUTIs or uniformTypeIdentifiers'),


"Only one of uniformTypeIdentifiers or macUTIs can be non-null"

adpinola · 2022-10-11T18:06:17Z

Feedback applied.
Moving forward to replacing the old macUTIs, we will @Deprecate it, and then we should replace it with the new uniformTypeIdentifiers.
Can this be done in a single PR for all platforms, or we should create one to mark the property as deprecated and then replace it in one PR for each platform?

Thanks.

stuartmorgan-g

LGTM, @ditman for second review.

stuartmorgan-g · 2022-10-11T18:59:24Z

Moving forward to replacing the old macUTIs, we will @Deprecate it, and then we should replace it with the new uniformTypeIdentifiers. Can this be done in a single PR for all platforms, or we should create one to mark the property as deprecated and then replace it in one PR for each platform?

The order is:

publish the new API
update all of the code in flutter/plugins to use the new API instead of the old one
deprecate the old API

The last step can't be done before the previous one because it will break our CI analysis.

…niformTypeIdentifiers' to the XTypeGroup.

ditman

Thanks for the change, let's land this! My comment is totally optional, I normally don't asssert error messages; admittedly this implementation is more strict :)

ditman · 2022-10-18T18:09:22Z

packages/file_selector/file_selector_platform_interface/test/x_type_group_test.dart

+          throwsA(predicate((Object? e) =>
+              e is AssertionError &&
+              e.message ==
+                  'Only one of uniformTypeIdentifiers or macUTIs can be non-null')));


I'm not a fan of asserting the text of the assertion error, I'd just do a expect(closure, throwsAssertionError) (or throwsA(isA<AssertionError>()))

…adding the new property 'uniformTypeIdentifiers' to the XTypeGroup. (flutter/plugins#6433)

… new property 'uniformTypeIdentifiers' to the XTypeGroup. (flutter#6433)

juandausa requested a review from stuartmorgan-g as a code owner September 15, 2022 20:00

github-actions bot added the p: file_selector label Sep 15, 2022

juandausa force-pushed the 103743-rename-property-macUTIs branch 2 times, most recently from 2a73760 to 6777bfb Compare September 16, 2022 11:38

stuartmorgan-g suggested changes Sep 19, 2022

View reviewed changes

stuartmorgan-g reviewed Sep 19, 2022

View reviewed changes

juandausa force-pushed the 103743-rename-property-macUTIs branch 2 times, most recently from 9f3c42b to ea39f35 Compare September 26, 2022 20:18

baezfacundo force-pushed the 103743-rename-property-macUTIs branch from ea39f35 to f71fad5 Compare September 29, 2022 15:56

eugerossetto force-pushed the 103743-rename-property-macUTIs branch from f71fad5 to 99441f6 Compare October 11, 2022 13:29

stuartmorgan-g suggested changes Oct 11, 2022

View reviewed changes

adpinola force-pushed the 103743-rename-property-macUTIs branch from 41e4a5c to 9c12e31 Compare October 11, 2022 17:54

stuartmorgan-g approved these changes Oct 11, 2022

View reviewed changes

stuartmorgan-g requested a review from ditman October 11, 2022 18:57

adpinola force-pushed the 103743-rename-property-macUTIs branch 2 times, most recently from abfd0e3 to 0ec98e7 Compare October 14, 2022 13:20

Juan Angel Dausa and others added 4 commits October 14, 2022 17:37

Update file_selector_platform_interface by adding the new property 'u…

5fd9f85

…niformTypeIdentifiers' to the XTypeGroup.

Version pump

6f997f0

change test description

4810d52

apply feedback

875851c

adpinola force-pushed the 103743-rename-property-macUTIs branch from 0ec98e7 to 875851c Compare October 14, 2022 20:38

ditman approved these changes Oct 18, 2022

View reviewed changes

ditman added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 18, 2022

auto-submit bot merged commit fd3ee38 into flutter:main Oct 18, 2022

engine-flutter-autoroll mentioned this pull request Oct 18, 2022

Roll Plugins from 3d44753ad9ba to fd3ee3885c8b (3 revisions) flutter/flutter#113623

Merged

adam-harwood pushed a commit to adam-harwood/flutter_plugins that referenced this pull request Nov 3, 2022

[file_selector] Update file_selector_platform_interface by adding the…

bc9726b

… new property 'uniformTypeIdentifiers' to the XTypeGroup. (flutter#6433)

This was referenced Nov 17, 2022

[webview_flutter] Added Android implementation of PlatformWebViewController #6674

Merged

Pull latest main branch changes into v4_webview #6713

Merged

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

[file_selector] Update file_selector_platform_interface by adding the…

c57943b

… new property 'uniformTypeIdentifiers' to the XTypeGroup. (flutter#6433)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[file_selector] Update file_selector_platform_interface by adding the new property 'uniformTypeIdentifiers' to the XTypeGroup. #6433

[file_selector] Update file_selector_platform_interface by adding the new property 'uniformTypeIdentifiers' to the XTypeGroup. #6433

juandausa commented Sep 15, 2022

stuartmorgan-g left a comment

stuartmorgan-g Sep 19, 2022

stuartmorgan-g Sep 19, 2022

stuartmorgan-g Sep 19, 2022

stuartmorgan-g Sep 19, 2022

juandausa Sep 20, 2022

juandausa Sep 20, 2022

stuartmorgan-g Sep 20, 2022

stuartmorgan-g Sep 19, 2022

stuartmorgan-g Sep 19, 2022

adpinola commented Oct 11, 2022

stuartmorgan-g left a comment

stuartmorgan-g Oct 11, 2022

stuartmorgan-g Oct 11, 2022

adpinola commented Oct 11, 2022

stuartmorgan-g left a comment

stuartmorgan-g commented Oct 11, 2022

ditman left a comment

ditman Oct 18, 2022

		@@ -1,3 +1,7 @@
		## 2.3.0

		* Adds the `uniformTypeIdentifiers` property to the `XTypeGroup` that relies on `macUTIs`. The last will be deprecated for future releases.

[file_selector] Update file_selector_platform_interface by adding the new property 'uniformTypeIdentifiers' to the XTypeGroup. #6433

[file_selector] Update file_selector_platform_interface by adding the new property 'uniformTypeIdentifiers' to the XTypeGroup. #6433

Conversation

juandausa commented Sep 15, 2022

Update file_selector_platform_interface by adding the new property 'uniformTypeIdentifiers' to the XTypeGroup.

Pre-launch Checklist

stuartmorgan-g 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

adpinola commented Oct 11, 2022

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adpinola commented Oct 11, 2022

stuartmorgan-g left a comment

Choose a reason for hiding this comment

stuartmorgan-g commented Oct 11, 2022

ditman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment