Skip to content
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

Reimplement file picker on macOS with method channels #1690

Conversation

orestesgaolin
Copy link
Contributor

@orestesgaolin orestesgaolin commented Jan 29, 2025

This PR:

It is still somewhat prone to #1680 - if the picked file contains / then it's replaced by :

I was wondering if I should address #1582 but figured it would be better to migrate both iOS and macOS later. I'll try if I find some time.

Now we can also address #1105 but that would require public API change.

Can someone confirm if this addresses #1685?

Here's a demo of this plugin on macOS 15.1.1 (24B91)

file-picker-demo.mp4

Feel free to add the note below to https://github.com/miguelpruivo/flutter_file_picker/wiki/Setup

On macOS it's necessary to enable "Read" User Selected File entitlement for selecting files and "Read/Write" for saving.

screenshot_20250129_103407

Copy link
Collaborator

@navaronbracke navaronbracke left a comment

Choose a reason for hiding this comment

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

Overall this looks great! Just some minor feedback around deprecations & docs.

example/macos/RunnerTests/RunnerTests.swift Outdated Show resolved Hide resolved
example/macos/Flutter/GeneratedPluginRegistrant.swift Outdated Show resolved Hide resolved
lib/src/file_picker_macos.dart Outdated Show resolved Hide resolved
macos/file_picker.podspec Outdated Show resolved Hide resolved
lib/src/file_picker.dart Outdated Show resolved Hide resolved
macos/Classes/FilePickerPlugin.swift Outdated Show resolved Hide resolved
macos/Classes/FilePickerPlugin.swift Outdated Show resolved Hide resolved
macos/Classes/FilePickerPlugin.swift Show resolved Hide resolved
macos/Classes/FilePickerPlugin.swift Show resolved Hide resolved
macos/Classes/FilePickerPlugin.swift Outdated Show resolved Hide resolved
@navaronbracke
Copy link
Collaborator

@orestesgaolin We can add SPM support for MacOS/iOS as a follow-up PR.

@navaronbracke
Copy link
Collaborator

I will check if this fixes #1685 with symbolic links.
If not, then we should probably do the resolution internally.

@navaronbracke
Copy link
Collaborator

navaronbracke commented Jan 29, 2025

@orestesgaolin For #1105 we could add a new method, that allows picking both files and directories?

But we also need to pay attention to the feedback in #1469

orestesgaolin and others added 2 commits January 29, 2025 13:29
Co-authored-by: Navaron Bracke <[email protected]>

Update lib/src/file_picker.dart

Co-authored-by: Navaron Bracke <[email protected]>

Update lib/src/file_picker.dart

Co-authored-by: Navaron Bracke <[email protected]>

Update lib/src/file_picker.dart

Co-authored-by: Navaron Bracke <[email protected]>

Update lib/src/file_picker.dart

Co-authored-by: Navaron Bracke <[email protected]>

Update lib/src/file_picker.dart

Co-authored-by: Navaron Bracke <[email protected]>

Update lib/src/file_picker.dart

Co-authored-by: Navaron Bracke <[email protected]>
@orestesgaolin orestesgaolin force-pushed the implement-on-macos-method-channel branch from 6c7e5db to 90c4513 Compare January 29, 2025 12:30
@orestesgaolin orestesgaolin force-pushed the implement-on-macos-method-channel branch from c405fd1 to ec43673 Compare January 29, 2025 12:59
@orestesgaolin
Copy link
Contributor Author

@navaronbracke I applied all suggestions and responded to comments, marked them as resolved -- hope you don't mind. Feel free to unresolve any pending issues or questions.

@orestesgaolin
Copy link
Contributor Author

Btw I can bump the version and update the changelog here too, should I bump it to 8.2.0 or 8.1.8?

Copy link
Collaborator

@navaronbracke navaronbracke left a comment

Choose a reason for hiding this comment

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

LGTM. Can you update the version to 8.2.0 and update the changelog? Then this can be released when the PR is merged :)

@navaronbracke
Copy link
Collaborator

I still need to check if this handles symlinks, so I'll probably do that before we merge this.

@navaronbracke
Copy link
Collaborator

The analysis failure is unrelated to this PR. I'll look into that as well.

@navaronbracke
Copy link
Collaborator

Btw I can bump the version and update the changelog here too, should I bump it to 8.2.0 or 8.1.8?

Version 8.2.0 is fine.

The analysis failure is unrelated to this PR. I'll look into that as well.

The analysis reports 3 switch cases where the default clause can be removed, since it is unreachable.
Maybe you can address this, too?

warning • lib/src/linux/kdialog_handler.dart:68:7 • This default clause is covered by the previous cases. Try removing the default clause,
          or restructuring the preceding patterns. • unreachable_switch_default
warning • lib/src/linux/qarma_and_zenity_handler.dart:60:7 • This default clause is covered by the previous cases. Try removing the default
          clause, or restructuring the preceding patterns. • unreachable_switch_default
warning • lib/src/windows/file_picker_windows.dart:230:7 • This default clause is covered by the previous cases. Try removing the default
          clause, or restructuring the preceding patterns. • unreachable_switch_default

3 issues found.

@navaronbracke
Copy link
Collaborator

@orestesgaolin I was wrong about the minimum MacOS version. When I run the example app with your branch, I get

/Users/navaronbracke/Desktop/flutter_file_picker-implement-on-macos-method-channel/example/macos/Pods/Pods.xcodeproj: warning: The macOS deployment target 'MACOSX_DEPLOYMENT_TARGET' is set to 10.11, but the range of supported deployment target versions is 10.13 to 15.2.99. (in target 'file_picker-file_picker_privacy' from project 'Pods')

So the minimum should be 10.13 and not 10.11.

@navaronbracke
Copy link
Collaborator

navaronbracke commented Jan 29, 2025

@orestesgaolin This does seem to fix the symlink bug!

I did the following to test:

  1. cd ~/Desktop && mkdir test_folder && cd test_folder && mkdir test_folder_2 && cd test_folder_2
  2. I then created some file in nested_folder, i.e. Untitled.rtf
  3. Make a symlink: ln -s "/Users/navaronbracke/Desktop/test_folder/nested_folder" /Users/navaronbracke/Desktop/test_folder_2, so that the nested_folder is hidden by the symlink
  4. run the example app
  5. In the file picker dialog, go into the test_folder_2 (has an arrow icon, so it's the symlink folder)
  6. Select the Untitled.rtf file
  7. The result path is /Users/navaronbracke/Desktop/test_folder/nested_folder/Untitled.rtf, so without the symlink (test_folder_2)

Screenshot 2025-01-29 at 14 40 09

Edit: Updated the OP to reflect this.

@orestesgaolin
Copy link
Contributor Author

Updated min version and proposed a changelog. Also tweaked list items in few recent releases to look nicer ;)

@navaronbracke
Copy link
Collaborator

navaronbracke commented Jan 29, 2025

@orestesgaolin Did you manage to look into the analysis failures on the Dart side? (i.e. dart analyze .)
As I said before, these aren't your fault, but since you are making a big contribution, I'd love to get this fixed also :)

You can remove the following default switch cases, as these are now reported as unreachable. Sadly, this isn't yet supported by dart fix, so you'll have to do it manually.

https://github.com/orestesgaolin/flutter_file_picker/blob/implement-on-macos-method-channel/lib/src/linux/kdialog_handler.dart#L68-L69

https://github.com/orestesgaolin/flutter_file_picker/blob/implement-on-macos-method-channel/lib/src/linux/qarma_and_zenity_handler.dart#L60-L61

https://github.com/orestesgaolin/flutter_file_picker/blob/implement-on-macos-method-channel/lib/src/windows/file_picker_windows.dart#L230-L231

These prevent the updated tests to run in CI. I can do this later, but in case you want to address this still.

@navaronbracke
Copy link
Collaborator

@orestesgaolin I went ahead and put out a patch for the lint fixes (on my corp account, but hey)
Once that is done I'll rebase your PR and then this can land!

@orestesgaolin
Copy link
Contributor Author

Sorry, was doing some of my corp stuff too ;) you were quicker!

@navaronbracke
Copy link
Collaborator

No worries! I'll get it sorted real quick!

@navaronbracke navaronbracke merged commit 261fb43 into miguelpruivo:master Jan 29, 2025
3 checks passed
@navaronbracke
Copy link
Collaborator

Tree is green! I'll check in with pub in a moment. Thanks!

@stephane-archer
Copy link
Collaborator

stephane-archer commented Jan 29, 2025

@navaronbracke @orestesgaolin thank you for your amazing work! I can't wait to ship with the new package

@orestesgaolin orestesgaolin deleted the implement-on-macos-method-channel branch January 29, 2025 14:53
@navaronbracke
Copy link
Collaborator

This should be available on Pub now.

@orestesgaolin
Copy link
Contributor Author

🕺

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants