-
Notifications
You must be signed in to change notification settings - Fork 391
New properties in Storage.Pickers - SuggestedDefaultFolder, SuggestedFolder, FileTypeChoices #5772
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
New properties in Storage.Pickers - SuggestedDefaultFolder, SuggestedFolder, FileTypeChoices #5772
Conversation
9a2af4e
to
52e0974
Compare
} | ||
|
||
void ValidateSuggestedFolder(winrt::hstring const& path) | ||
void ValidateFolderPath(winrt::hstring const& path, std::string const& propertyName) |
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.
In ValidateFolderPath, how does the implementation handle edge cases like relative paths (e.g., .\folder, ..\parent), standard UNC paths (e.g., \server\share), and non-existent folders (e.g., C:\NonExistent)? The code doesn’t seem to reject relative paths or check folder existence, which could lead to runtime errors?
Can you clarify the intended behavior for these cases and share how they were tested!
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 @Saharsh979 for joining the review.
All path strings in the storage pickers are full path.
The relative path are not supported yet.
{ | ||
FileTypeChoicesMap(); | ||
FileTypeChoicesMap(bool forSavePicker = 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.
Curiosity - Can you explain the rationale behind this update?
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.
Yes, this is for velocity. The FileSavePicker already has FileTypeChoices, so it doesn't need an velocity check.
This PR is adding FileTypeChoices property to the FileOpenPicker, meaning that we need a velocity check when this customized map type is used for in the FIleOpenPicker.
In future when we remove this velocity, we can remove the forSavePicker flag too.
Note that this is an internal class, it should not impact the public API contract.
{ | ||
return m_fileTypeFilter; | ||
} | ||
winrt::hstring FileOpenPicker::SuggestedFolder() |
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.
Curiosity: Maybe it might be worthwhile to test performance as well? Could complex or large FileTypeChoices lists slow down picker init?
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.
Should be ok. It is the same property logic existing in FileSavePicker.
hstring SuggestedFolder(); | ||
void SuggestedFolder(hstring const& value); | ||
|
||
hstring SuggestedStartFolder(); |
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.
Nit - Is there a possibility to move common logic to a base class?
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, this is a good point. We might do some refactoring in future.
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.
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
550d668
to
0618e5b
Compare
Thanks @codendone and @Saharsh979 for reviewing this PR. The pipeline build failed last night - caused by the missing head file of velocity. So I pushed a fix in the last iteration. Please review the updates when you have a moment. |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
…lder, FileTypeChoices (#5771) This is a design spec for adding new functionality to the Microsoft.Windows.Storage.Pickers APIs, including 2 main changes: - the SuggestedFolder and SuggestedStartFolder properties for all three picker types, - the new FileTypeChoices property for FileOpenPicker. Code implementation details can be found in (#5772)
An implementation of
This pull request introduces new experimental features to the Windows Storage Pickers API, specifically enabling support for suggested folders and enhanced file type choices. These changes are gated behind a new feature flag,
Feature_StoragePickers2
, and are only available in the experimental channel. The updates affect the FileOpenPicker, FileSavePicker, and FolderPicker classes, as well as related internal data structures and the public API contract.Feature flag and API contract updates:
Feature_StoragePickers2
, including its XML specification and auto-generated header (TerminalVelocityFeatures-StoragePickers2.xml
,TerminalVelocityFeatures-StoragePickers2.h
). The feature is always enabled in the experimental channel and disabled in preview/stable channels. [1] [2]SuggestedFolder
,SuggestedStartFolder
,FileTypeChoices
) to the pickers, gated by the new feature flag. [1] [2] [3] [4]New properties and API surface:
SuggestedFolder
andSuggestedStartFolder
properties toFileOpenPicker
,FileSavePicker
, andFolderPicker
, with validation and feature gating. Also added theFileTypeChoices
property toFileOpenPicker
. [1] [2] [3] [4] [5] [6] [7] [8] [9]Internal data structure changes:
FileTypeChoicesMap
andFileTypeFilterVector
to support feature gating and picker-specific logic. [1] [2] [3] [4]Parameter handling and validation:
Miscellaneous: