FileSavePicker.SuggestedSaveFilePath

[DinahK-2SO]

Description

This PR changes the property of FileSavePicker.SuggestedSaveFile into a light-weight string property FileSavePicker.SuggestedSaveFilePath, which is the implementation of design spec: #5538

This PR also adds contract for the storage pickers, and excludes the unnecessary property (FolderPicker.FileTypeFilter) from the contract.

Code Details

This pull request introduces several changes to the StoragePickers module, focusing on enhancing the FileSavePicker functionality by replacing SuggestedSaveFile with SuggestedSaveFilePath, removing the FileTypeFilter from FolderPicker, and adding new API contracts for better versioning. Additionally, it includes updates to test cases and internal configurations for improved functionality and reliability.

Enhancements to FileSavePicker:

• Replaced SuggestedSaveFile with SuggestedSaveFilePath in FileSavePicker to allow specifying file paths directly, including updates to both the implementation (FileSavePicker.cpp) and header (FileSavePicker.h). [1] [2]
• Added logic to PickerParameters::ConfigureFileSaveDialog to handle SuggestedSaveFilePath for setting the starting directory and file name in the save dialog, with fallback to SuggestedFileName.
• Updated the .idl file to reflect the SuggestedSaveFilePath change in the FileSavePicker runtime class.

Simplifications to FolderPicker:

• Removed the FileTypeFilter property from FolderPicker, simplifying its API and associated logic in both the implementation (FolderPicker.cpp) and header (FolderPicker.h). [1] [2]

API Versioning and Contracts:

• Introduced a new StoragePickersContract (version 1.8) in the .idl file to enable better API versioning and feature gating for StoragePickers.

Test Suite Updates:

• Added new test cases for PickerParameters::ConfigureFileSaveDialog to verify behavior when SuggestedSaveFilePath is set and its precedence over SuggestedFileName.
• Updated tests for FileSavePicker to validate the SuggestedSaveFilePath property.

Internal Configuration Updates:

• Enhanced PickerCommon.cpp with additional includes (shellapi.h, shobjidl_core.h, etc.) to support new functionality for file path handling.

[DinahK-2SO]

Good morning @jonwis and @asklar , thanks for reviewing the design spec #5538.

This is a implementation PR, it contains the changes raised during the design spec review and discussion.

Please also review this PR when you're available.

[Copilot]

Pull Request Overview

This pull request refactors the FileSavePicker implementation to use a lightweight string property for the suggested save file path, updates the related API contracts, and introduces enhanced dialog configuration along with comprehensive tests.

• Replaces StorageFile SuggestedSaveFile with hstring SuggestedSaveFilePath
• Introduces the ConfigureFileSaveDialog method in PickerCommon
• Updates tests to verify the new behavior in both FileSavePicker and PickerCommon

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
test/StoragePickersTests/StoragePickersTests.cpp	Adds tests for the new SuggestedSaveFilePath property.
test/StoragePickersTests/PickerCommonTests.cpp	Provides unit tests covering dialog configuration and precedence of SuggestedSaveFilePath over SuggestedFileName.
dev/Interop/StoragePickers/PickerCommon.h	Updates internal structure and adds declaration for ConfigureFileSaveDialog.
dev/Interop/StoragePickers/PickerCommon.cpp	Implements ConfigureFileSaveDialog using std::filesystem to set the starting directory and file name based on the new property.
dev/Interop/StoragePickers/Microsoft.Windows.Storage.Pickers.idl	Reflects the API changes by replacing SuggestedSaveFile with SuggestedSaveFilePath and adding API contract definitions.
dev/Interop/StoragePickers/FileSavePicker.h	Updates the public interface to use hstring for suggested save file paths.
dev/Interop/StoragePickers/FileSavePicker.cpp	Refactors implementation and integrates the new ConfigureFileSaveDialog for a cleaner dialog setup.

[codendone]

Just the remaining question about how to handle failure from SHCreateItemFromParsingName(). I'm not sure it is good to just ignore the failure. I expect at least some failures would have previously been caught in app code, such as if the string doesn't have valid path syntax. Would the old model have also required that the path exist?

I wonder if we should throw an invalid argument error (with an error string to explain which arg is invalid). What does the old syntax or the UWP picker do with an invalid path syntax or missing path?

[DinahK-2SO]

Just the remaining question about how to handle failure from SHCreateItemFromParsingName(). I'm not sure it is good to just ignore the failure. I expect at least some failures would have previously been caught in app code, such as if the string doesn't have valid path syntax. Would the old model have also required that the path exist?

I wonder if we should throw an invalid argument error (with an error string to explain which arg is invalid). What does the old syntax or the UWP picker do with an invalid path syntax or missing path?

Hi @codendone , Thanks for raising this!

In most cases, the old model (the UWP FileSavePicker) wouldn't encounter this issue, because the UWP picker APIs typically take a StorageFile object to set this value. When a StorageFile is created, the system already ensures that this file and its folder exists - so the UWP FileSavePicker doesn't need to do any extra validation.

That said, it is possible to simulate this scenario in UWP with a specifically designed code and steps:
Use the code 2 launch 2 open pickers, and one save picker:

1. At the first FileOpenPicker, manually pick a StorageFile.
2. On launching the second picker, manually delete the folder containing the picked file from step 1.
3. Then, before the launching a FileSavePicker, setting the previously picked StorageFile as SuggestedSaveFile in code.

In this case, the save file dialog will still launch successfully because the original StorageFile object remains valid for setting the folder - even though the actual folder on file system no longer exists.

After having launched, the dialog will show an error message : "Location is not available", because the folder that previous existed is gone. Please note that this message box doesn't block the user from continuing to use the save dialog. The user can dismiss the message box and proceed. @asklar , in summary a similar messagebox is shown in the UWP pickers.

And the SuggestedSaveFile's file name is still automatically filled in the file name input box.

In contrast, the new picker model uses a string path to set SuggestedSaveFilePath, and SaveFileDialog requires a valid ShellItem to call SetFolder. So we must create the shell item from the folder string and validate whether the folder item has been successfully created before setting it to the dialog.

Even if the folder item creation fails (e.g., the folder doesn't exist), as a suggested value, it should not block users from using the dialog - that's the reason why we initially chose to ignore the error and only to set the folder when it exists.

On second thought, we could show a message box before launching the dialog - similar to how UWP shows an error after launch. This would inform the user about the non-existing but expected folder, without blocking user from using the dialog

1. But the timing differs slightly from UWP's behavior.
2. I'm displaying the message box with warning icon (instead of the error icon of UWP picker), as it won't block user from using the dialog.
3. What's more, this message doesn't have localization yet (pending on a PR for localization Adding Localization To Foundation #5495 , we can add its localization in future releases)

How do you like this update? Here's a demo of the latest iteration:

[DinahK-2SO]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[codendone]

Follow-up is still happening on the handling of invalid/missing path, but if you'd like to open a separate issue for follow-up on that, I'm okay with completing this PR and handling that follow-up before the API goes stable.

[DinahK-2SO]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[DinahK-2SO]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).