Fix macios filepicker #2981
Fix macios filepicker #2981
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in the FileSaver implementation for macOS and iOS by correctly configuring the UIDocumentPickerViewController to treat the operation as saving a new file rather than copying an existing one.
- Updates the
UIDocumentPickerViewControllerconstructor call to include theasCopy: trueparameter - Ensures the OS properly handles moving temporary files to user-selected locations during save operations
| var tcs = taskCompetedSource = new(cancellationToken); | ||
|
|
||
| documentPickerViewController = new([fileUrl]) | ||
| documentPickerViewController = new([fileUrl], true) |
There was a problem hiding this comment.
Should we delete temp file in this case if we copy it instead of move?
There was a problem hiding this comment.
I think so. I started refactoring this, I wanted to see if I could make it a more logical flow, and added a Cleanup method that handled that and the dispose. But I didn't want to introduce too many changes.
There was a problem hiding this comment.
Ideally if we don't use the temp file at all and write directly to the target location.
To make it simple for now you can wrap the code in try/finally block and delete temp file in finally section if it exists. It should not add complexity and many changes.
There was a problem hiding this comment.
Ideally if we don't use the temp file at all and write directly to the target location.
I've spent a bit of time looking into this and I don't think it's practical. The only way to write directly outside the app's sandbox is using a security scoped URL and something like NSFileCoordinator. The problem is, this requires the user to grant access to it first, so it's not something we can do with a new file.
Additionally, we would have to track that the user has granted access and have safety checks that fall back to the temp file approach if not. And the problem with the first part is that it requires app-wide tracking and coordination. It's not impossible (far from it) but it's outside the scope of the vertical slice of this API.
The current approach (creating a temp file, then moving) is the canonical approach with Apple's API.
To make it simple for now you can wrap the code in try/finally block and delete temp file in finally section if it exists. It should not add complexity and many changes.
Agreed 🙂 Done in 47ddfe5. Please let me know if this is ok, or if you want me to make any other changes.
There was a problem hiding this comment.
I made a tiny change to ensure the temp file is always deleted. Please let me know if it looks good
Ensures temporary directory used by the file picker is always removed after the file selection is made, regardless of success or failure. Also improves error handling when a view controller cannot be retrieved. Fixes CommunityToolkit#2460
src/CommunityToolkit.Maui.Core/Essentials/FileSaver/FileSaverImplementation.macios.cs
Outdated
Show resolved
Hide resolved
src/CommunityToolkit.Maui.Core/Essentials/FileSaver/FileSaverImplementation.macios.cs
Outdated
Show resolved
Hide resolved
Ensures temporary directory used by the file picker is correctly removed on iOS. Moves the directory removal to the completion handler of the view controller presentation, ensuring it's executed after the picker is dismissed. Handles exceptions during file removal by re-throwing them after attempting to remove the temporary directory.
| currentViewController.PresentViewController(documentPickerViewController, true, () => | ||
| { | ||
| fileManager.Remove(tempDirectoryPath, out _); | ||
| }); |
There was a problem hiding this comment.
The temporary directory cleanup in the completion callback may execute prematurely before the file picker operations complete. The completion callback of PresentViewController is invoked immediately after the presentation animation finishes, not after the user completes their interaction with the document picker.
This means the temp directory (and the file within it) could be deleted while the UIDocumentPickerViewController is still displaying and before the user has saved the file to their chosen location, which would cause the save operation to fail.
Consider removing the cleanup from the completion callback and instead perform the cleanup in the event handlers (DocumentPickerViewControllerOnDidPickDocumentAtUrls and DocumentPickerViewControllerOnWasCancelled) where the actual file picker interaction is complete.
There was a problem hiding this comment.
@VladislavAntonyuk what are your thoughts on this? I had the cleanup method, which could be called in a finally or as part of InternalDispose, that will always check for the file and remove it if it exists. If we restore that, it's always safe to call, and can be called from any catch or finally block, from dispose, or from the event handlers suggested here.
src/CommunityToolkit.Maui.Core/Essentials/FileSaver/FileSaverImplementation.macios.cs
Outdated
Show resolved
Hide resolved
…mplementation.macios.cs Co-authored-by: Copilot <[email protected]>
| catch | ||
| { | ||
| throw new FileSaveException("Unable to get a window where to present the file saver UI."); | ||
| fileManager.Remove(tempDirectoryPath, out _); |
There was a problem hiding this comment.
The catch block doesn't handle cleanup of event handlers or dispose the document picker. If an exception occurs after the event handlers are attached (lines 44-45), these handlers will remain attached and could cause memory leaks or unexpected behavior in subsequent operations.
Consider calling InternalDispose() in the catch block to ensure proper cleanup of all resources.
| fileManager.Remove(tempDirectoryPath, out _); | |
| fileManager.Remove(tempDirectoryPath, out _); | |
| InternalDispose(); |
4 participants
Description of Change
Updates the
FileSavermaciOS implementation to use the newasCopyargument in theUIDocumentPickerViewControllerto tell the OS that you're saving a new file (not copying) when moving the temp file out of the sandobx to the users' desired location.Linked Issues
PR Checklist
approved(bug) orChampioned(feature/proposal)mainat time of PRAdditional information
Only affects macOS and iOS. Screenshots show the resultant change on a physical iOS device: