-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[file_selector] Add parameter to control whether the new folders button is visible in the file dialog #9965
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[file_selector] Add parameter to control whether the new folders button is visible in the file dialog #9965
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution!
| /// | ||
| /// [canCreateDirectories] controls whether the user is allowed to create new | ||
| /// directories in the dialog (if supported on the platform). | ||
| /// Currently only supported on Linux and macOS. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The app-facing package has no control over this, so it's subject to becoming stale (and misleading about unendorsed implementations). It should just say that it may not be supported on all platforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. I changed the doc so that it infers less about the implementations
Let me know what you think
| ## NEXT | ||
| ## 1.1.0 | ||
|
|
||
| * Adds `canCreateDirectories` parameter to `FileDialogOptions` to control the visibility of the New Folder button in file dialogs on supported platforms. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FileDialogOptions shouldn't be mentioned at this level, since it's not the public API here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes total sense.
Fixed manually all changelogs
| const String initialDirectory = '/home/flutteruser'; | ||
| const String confirmButtonText = 'Use this profile picture'; | ||
| const String suggestedName = 'suggested_name'; | ||
| const bool canCreateDirectories = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no need for this to be stateful in the tests; values should be local.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file shouldn't be changed; it's for legacy compat, and legacy implementations won't support new features.
...tor/file_selector_platform_interface/lib/src/platform_interface/file_selector_interface.dart
Show resolved
Hide resolved
packages/file_selector/file_selector_linux/linux/test/file_selector_plugin_test.cc
Show resolved
Hide resolved
...elector_macos/macos/file_selector_macos/Sources/file_selector_macos/FileSelectorPlugin.swift
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay in getting back to this. A few more comments on the cross-platform parts.
| expect(location?.path, expectedSavePath); | ||
| }); | ||
|
|
||
| test('sets to disable the creation of new directories', () async { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'sets the directory creation control flag'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
| expect(directoryPath, expectedDirectoryPath); | ||
| }); | ||
|
|
||
| test('sets to enable de creation of new directories', () async { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
| ); | ||
| expect(directoryPaths, expectedDirectoryPaths); | ||
| }); | ||
| test('sets to enable de creation of new directories', () async { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
| ## NEXT | ||
| ## 1.1.0 | ||
|
|
||
| * Adds `canCreateDirectories` param to `getDirectoryPath` and `getDirectoryPaths` to control the visibility of the New Folder button in file dialogs on supported platforms. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be describing an exact UI element, because this layer has no control over what the specific UI is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I get it now. It makes sense.
I will describe the behavior instead.
| }); | ||
|
|
||
| group('getDirectoryPathWithOptions', () { | ||
| test('Should throw unimplemented exception', () async { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and below: that's not what we want to test for the new methods. The implementations of the new methods should not throw UnimplementedError, because that would break backward compatibility when the app-facing package change lands. They must call the deprecated methods, and that's what the test should be testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see.
I didn't even realize the getSaveLocation test was doing exactly what you described.
I'll update the mentioned tests to check if the deprecated methods were called.
cb4e65c to
0b3beb5
Compare
0b3beb5 to
397d15f
Compare
I appreciate your patience |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iOS side LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks good; please go ahead and split out the platform interface initial PR now and we can get that reviewed and landed. (Normally we'd wait for all the platform reviews for that, but I've looked over all of them, and they are straightforward enough that we don't need to block the interface PR on that.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
android portion lgtm
Adds a new parameter
canCreateDirectoriestoFileDialogOptionsto control the visibility of the New Folder button in file dialogs on supported platforms.Currently only configurable on Linux and macOS
This is the "Combined PR"
Let me know if we are okay with this approach so I can open a new PR covering the interface changes
Fixes: flutter/flutter#141339
Pre-Review Checklist
[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under1.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under1.///).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-assistbot 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
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