Skip to content

Conversation

@danferreira
Copy link
Contributor

@danferreira danferreira commented Nov 16, 2025

Updates macos and linux file_selector platforms to implement the new canCreateDirectories parameter from platform interface

Until now, only macos and linux are able to override this parameter

This is the "platform implementations" step for #9965

Part of: flutter/flutter#141339

Pre-Review Checklist

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

Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the gemini-code-assist bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.

Footnotes

  1. Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. 2 3

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces the canCreateDirectories option across various platforms for the file_selector package. While the implementations for Linux and macOS seem complete with corresponding native changes and tests, there are some areas for improvement. Specifically, the Android and Web platforms are missing an implementation for getDirectoryPathsWithOptions, which could lead to runtime errors. On Android and Windows, the canCreateDirectories option is ignored in directory selection methods without documentation, which could be confusing. I've also noted some opportunities to reduce code duplication in the native tests for Linux and macOS to improve maintainability. My detailed feedback is in the comments below.

Comment on lines +373 to +376
let panelController = TestPanelController()
let plugin = FileSelectorPlugin(
viewProvider: TestViewProvider(),
panelController: panelController)

Choose a reason for hiding this comment

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

medium

There is a lot of repeated setup code across your test cases (e.g., initializing TestPanelController and FileSelectorPlugin). You could reduce this duplication by moving the common setup into an override func setUp() method for the ExampleTests class. This would make the tests cleaner and easier to maintain.

@danferreira danferreira force-pushed the file_selector-plataform-implementations branch from 5eb0740 to b39655a Compare November 16, 2025 19:02
@danferreira danferreira changed the title [file_selector] Implement canCreateDirectories across all platforms [file_selector] Implement canCreateDirectories on macos and linux Nov 16, 2025
Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

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

LGTM

@robert-ancell and @vashworth will do secondary review approvals for the two platforms.

/// Nullable because it does not apply to the "save" action.
final bool? selectMultiple;

/// Whether to allow new folders creation.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: folder

@stuartmorgan-g stuartmorgan-g added triage-macos Should be looked at in macOS triage triage-linux Should be looked at in Linux triage labels Nov 18, 2025
Copy link
Contributor

@robert-ancell robert-ancell left a comment

Choose a reason for hiding this comment

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

Change looks good on Linux, thanks!

@stuartmorgan-g stuartmorgan-g removed the triage-linux Should be looked at in Linux triage label Nov 19, 2025
Copy link
Contributor

@vashworth vashworth left a comment

Choose a reason for hiding this comment

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

macOS side LGTM

@vashworth
Copy link
Contributor

Linux repo_checks test is failing due to formatting:

These files are not formatted correctly (see diff below):
   packages/file_selector/file_selector_android/example/lib/main.dart
   packages/file_selector/file_selector_linux/example/lib/get_multiple_directories_page.dart
   packages/file_selector/file_selector_linux/example/lib/home_page.dart
   packages/file_selector/file_selector_linux/example/lib/main.dart
   packages/file_selector/file_selector_linux/example/lib/open_multiple_images_page.dart
 To fix run the repository tooling `format` command: https://github.com/flutter/packages/blob/main/script/tool/README.md#format-code
 or copy-paste this command into your terminal:
 patch -p1 <<DONE
 diff --git a/packages/file_selector/file_selector_android/example/lib/main.dart b/packages/file_selector/file_selector_android/example/lib/main.dart
 index 1a2362dc1..b69b4c1e4 100644
 --- a/packages/file_selector/file_selector_android/example/lib/main.dart
 +++ b/packages/file_selector/file_selector_android/example/lib/main.dart
 @@ -40,8 +40,8 @@ class MyApp extends StatelessWidget {
        home: const HomePage(),
        routes: <String, WidgetBuilder>{
          '/open/image': (BuildContext context) => const OpenImagePage(),
 -        '/open/images':
 -            (BuildContext context) => const OpenMultipleImagesPage(),
 +        '/open/images': (BuildContext context) =>
 +            const OpenMultipleImagesPage(),
          '/open/text': (BuildContext context) => const OpenTextPage(),
        },
      );
 diff --git a/packages/file_selector/file_selector_linux/example/lib/get_multiple_directories_page.dart b/packages/file_selector/file_selector_linux/example/lib/get_multiple_directories_page.dart
 index 0c5297313..fe7fd3d24 100644
 --- a/packages/file_selector/file_selector_linux/example/lib/get_multiple_directories_page.dart
 +++ b/packages/file_selector/file_selector_linux/example/lib/get_multiple_directories_page.dart
 @@ -22,8 +22,8 @@ class GetMultipleDirectoriesPage extends StatelessWidget {
      if (context.mounted) {
        await showDialog<void>(
          context: context,
 -        builder:
 -            (BuildContext context) => TextDisplay(directoryPaths.join('\n')),
 +        builder: (BuildContext context) =>
 +            TextDisplay(directoryPaths.join('\n')),
        );
      }
    }
 diff --git a/packages/file_selector/file_selector_linux/example/lib/home_page.dart b/packages/file_selector/file_selector_linux/example/lib/home_page.dart
 index a4e6f8288..b49884852 100644
 --- a/packages/file_selector/file_selector_linux/example/lib/home_page.dart
 +++ b/packages/file_selector/file_selector_linux/example/lib/home_page.dart
 @@ -54,8 +54,8 @@ class HomePage extends StatelessWidget {
              ElevatedButton(
                style: style,
                child: const Text('Open a get directories dialog'),
 -              onPressed:
 -                  () => Navigator.pushNamed(context, '/multi-directories'),
 +              onPressed: () =>
 +                  Navigator.pushNamed(context, '/multi-directories'),
              ),
            ],
          ),
 diff --git a/packages/file_selector/file_selector_linux/example/lib/main.dart b/packages/file_selector/file_selector_linux/example/lib/main.dart
 index f1d207839..28a342002 100644
 --- a/packages/file_selector/file_selector_linux/example/lib/main.dart
 +++ b/packages/file_selector/file_selector_linux/example/lib/main.dart
 @@ -32,13 +32,13 @@ class MyApp extends StatelessWidget {
        home: const HomePage(),
        routes: <String, WidgetBuilder>{
          '/open/image': (BuildContext context) => const OpenImagePage(),
 -        '/open/images':
 -            (BuildContext context) => const OpenMultipleImagesPage(),
 +        '/open/images': (BuildContext context) =>
 +            const OpenMultipleImagesPage(),
          '/open/text': (BuildContext context) => const OpenTextPage(),
          '/save/text': (BuildContext context) => SaveTextPage(),
          '/directory': (BuildContext context) => const GetDirectoryPage(),
 -        '/multi-directories':
 -            (BuildContext context) => const GetMultipleDirectoriesPage(),
 +        '/multi-directories': (BuildContext context) =>
 +            const GetMultipleDirectoriesPage(),
        },
      );
    }
 diff --git a/packages/file_selector/file_selector_linux/example/lib/open_multiple_images_page.dart b/packages/file_selector/file_selector_linux/example/lib/open_multiple_images_page.dart
 index 1b11e4939..0b9fe3525 100644
 --- a/packages/file_selector/file_selector_linux/example/lib/open_multiple_images_page.dart
 +++ b/packages/file_selector/file_selector_linux/example/lib/open_multiple_images_page.dart
 @@ -80,10 +80,9 @@ class MultipleImagesDisplay extends StatelessWidget {
            children: <Widget>[
              ...files.map(
                (XFile file) => Flexible(
 -                child:
 -                    kIsWeb
 -                        ? Image.network(file.path)
 -                        : Image.file(File(file.path)),
 +                child: kIsWeb
 +                    ? Image.network(file.path)
 +                    : Image.file(File(file.path)),
                ),
              ),
            ],
 DONE

@danferreira
Copy link
Contributor Author

Linux repo_checks test is failing due to formatting:

These files are not formatted correctly (see diff below):
   packages/file_selector/file_selector_android/example/lib/main.dart
   packages/file_selector/file_selector_linux/example/lib/get_multiple_directories_page.dart
   packages/file_selector/file_selector_linux/example/lib/home_page.dart
   packages/file_selector/file_selector_linux/example/lib/main.dart
   packages/file_selector/file_selector_linux/example/lib/open_multiple_images_page.dart
 To fix run the repository tooling `format` command: https://github.com/flutter/packages/blob/main/script/tool/README.md#format-code
 or copy-paste this command into your terminal:
 patch -p1 <<DONE
 diff --git a/packages/file_selector/file_selector_android/example/lib/main.dart b/packages/file_selector/file_selector_android/example/lib/main.dart
 ....
 DONE

Weird, for some reason dart format keeps applying those changes locally.
I fixed applying the suggested patch script instead

@stuartmorgan-g
Copy link
Collaborator

If you are running dart format directly instead of using the repo tooling, you probably need to run flutter pub get. Dart's formatter has behavioral changes that are gated on the min SDK version as recorded in a file in .dart_tool/, which is updated by pub get. If the file is stale, the formatter can apply the wrong version of the format.

@stuartmorgan-g stuartmorgan-g added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 21, 2025
@auto-submit auto-submit bot merged commit e67b6be into flutter:main Nov 21, 2025
80 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 21, 2025
github-merge-queue bot pushed a commit to flutter/flutter that referenced this pull request Nov 21, 2025
flutter/packages@b1e2fb0...e67b6be

2025-11-21 [email protected] [file_selector] Implement
canCreateDirectories on macos and linux (flutter/packages#10443)
2025-11-21 [email protected] [various] Replace deprecated
Color.value in SVG packages (flutter/packages#10482)
2025-11-20 [email protected] [pigeon]
Fixes compilation error with unbounded type parameter for
InstanceManager (flutter/packages#10483)
2025-11-20 [email protected] [webview_flutter] Replace deprecated
Color.value (flutter/packages#10480)
2025-11-20 [email protected] [google_maps_flutter] Replace
deprecated color APIs in platform interface (flutter/packages#10477)
2025-11-20 [email protected] Roll Flutter (stable) from
b45fa18 to f5a8537 (12 revisions) (flutter/packages#10478)
2025-11-20 [email protected] Roll Flutter from
de4be4f to 9f383e0 (21 revisions) (flutter/packages#10481)

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 [email protected] 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
@danferreira danferreira deleted the file_selector-plataform-implementations branch November 21, 2025 16:24
okorohelijah pushed a commit to okorohelijah/flutter that referenced this pull request Nov 21, 2025
…r#178927)

flutter/packages@b1e2fb0...e67b6be

2025-11-21 [email protected] [file_selector] Implement
canCreateDirectories on macos and linux (flutter/packages#10443)
2025-11-21 [email protected] [various] Replace deprecated
Color.value in SVG packages (flutter/packages#10482)
2025-11-20 [email protected] [pigeon]
Fixes compilation error with unbounded type parameter for
InstanceManager (flutter/packages#10483)
2025-11-20 [email protected] [webview_flutter] Replace deprecated
Color.value (flutter/packages#10480)
2025-11-20 [email protected] [google_maps_flutter] Replace
deprecated color APIs in platform interface (flutter/packages#10477)
2025-11-20 [email protected] Roll Flutter (stable) from
b45fa18 to f5a8537 (12 revisions) (flutter/packages#10478)
2025-11-20 [email protected] Roll Flutter from
de4be4f to 9f383e0 (21 revisions) (flutter/packages#10481)

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 [email protected] 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App p: file_selector platform-linux platform-macos triage-macos Should be looked at in macOS triage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants