Skip to content

Supplements Missing Functionalities for FileOpenPicker.FileTypeFilter and FileSavePicker.FilterTypeChoices #5358

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+48 −26
Open

Supplements Missing Functionalities for FileOpenPicker.FileTypeFilter and FileSavePicker.FilterTypeChoices #5358

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a2c1adc
fix the filetypefileters and choices
DinahK-2SO Apr 23, 2025
886a38f
select All Files on launch; FOS_STRICTFILETYPES; display All Files wh…
DinahK-2SO Apr 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion dev/Interop/StoragePickers/FileOpenPicker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ namespace winrt::Microsoft::Windows::Storage::Pickers::implementation
auto dialog = create_instance<IFileOpenDialog>(CLSID_FileOpenDialog, CONTEXT_ALL);

parameters.ConfigureDialog(dialog);
check_hresult(dialog->SetFileTypeIndex(parameters.FileTypeFilterPara.size()));

{
auto hr = dialog->Show(parameters.HWnd);
Expand Down Expand Up @@ -142,7 +143,7 @@ namespace winrt::Microsoft::Windows::Storage::Pickers::implementation
auto dialog = create_instance<IFileOpenDialog>(CLSID_FileOpenDialog, CONTEXT_ALL);

parameters.ConfigureDialog(dialog);

check_hresult(dialog->SetFileTypeIndex(parameters.FileTypeFilterPara.size()));
check_hresult(dialog->SetOptions(FOS_ALLOWMULTISELECT));

{
Expand Down
24 changes: 9 additions & 15 deletions dev/Interop/StoragePickers/FileSavePicker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,9 +118,17 @@ namespace winrt::Microsoft::Windows::Storage::Pickers::implementation

if (!PickerCommon::IsHStringNullOrEmpty(defaultFileExtension))
{
check_hresult(dialog->SetDefaultExtension(defaultFileExtension.c_str()));
// the default extension has a ".", like ".txt". remove the "." (a.k.a "txt") then set it to the dialog
// otherwise, the diaplayed file name would have two "."
check_hresult(dialog->SetDefaultExtension(defaultFileExtension.c_str() + 1));
}
else
{
check_hresult(dialog->SetDefaultExtension(L""));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a behavior difference between calling dialog->SetDefaultExtension(L"") and not calling SetDefaultExtension?

}

check_hresult(dialog->SetOptions(FOS_STRICTFILETYPES));
Copy link
Collaborator

Choose a reason for hiding this comment

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

The UWP pickers didn't use FOS_STRICTFILETYPES, did they? I'm concerned that forcing strict on all apps may be undesirable. (For example, maybe the app likes ".jpg" but the user prefers ".jpeg" and wants to force that even if the app doesn't list that extension.)


if (!PickerCommon::IsHStringNullOrEmpty(suggestedFileName))
{
check_hresult(dialog->SetFileName(suggestedFileName.c_str()));
Expand Down Expand Up @@ -154,20 +162,6 @@ namespace winrt::Microsoft::Windows::Storage::Pickers::implementation
check_hresult(shellItem->GetDisplayName(SIGDN_NORMALDISPLAY, fileName.put()));
std::wstring fileNameStr(fileName.get());

// Check if the file name has an extension
if ((fileNameStr.find_last_of(L".") == std::wstring::npos) && (fileTypeChoices.Size() > 0))
{
// If the user defined file name doesn't have an extension,
// automatically use the first extension from the first category in fileTypeChoices.
auto firstCategory = fileTypeChoices.First().Current();
auto value = firstCategory.Value();
if (value.Size() > 0)
{
auto firstExtension = value.GetAt(0);
pathStr = pathStr + firstExtension;
}
}

// Create a file. If the file already exists,
// since common item dialog prompts to let user select cancel or override, thus we can safely truncate here.
// Due to our design spec to align with UWP pickers, we need ensure existance of picked file.
Expand Down
41 changes: 33 additions & 8 deletions dev/Interop/StoragePickers/PickerCommon.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ namespace PickerCommon {
}

/// <summary>
/// Capture and processing pickers filter inputs and convert them into Common Item Dialog's accepting type
/// Capture and processing pickers filter inputs and convert them into Common Item Dialog's accepting type, for FileOpenPicker
/// </summary>
/// <param name="buffer">temp buffer to hold dynamically transformed strings</param>
/// <param name="filters">winrt style filters</param>
Expand All @@ -152,29 +152,54 @@ namespace PickerCommon {
/// </returns>
std::vector<COMDLG_FILTERSPEC> CaptureFilterSpec(std::vector<winrt::hstring>& buffer, winrt::Windows::Foundation::Collections::IVectorView<winrt::hstring> filters)
Copy link
Member

Choose a reason for hiding this comment

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

buffer is a rather nondescript name.

SUGGESTION: formattedFilters? filtersForCommonDialog?

{
std::vector<COMDLG_FILTERSPEC> result(filters.Size());
int resultSize = filters.Size() + 1;
buffer.clear();
buffer.reserve(filters.Size() * (size_t)2);
buffer.reserve(resultSize * (size_t)2);
Copy link
Member

Choose a reason for hiding this comment

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

Why the (size_t) cast? And if you do need a cast, you shouldn't use C style casts but rather C++ new style cast notation e.g. static_cast<size_t>(2)

SUGGESTION:

buffer.clear();`
buffer.reserve((filters.Size() + 1) * 2);

If the compiler gives you warning about mixing unsigned values from .Size() and signed values from the constants you can make the constants unsigned e.g. buffer.reserve((filters.Size() + 1u) * 2u);


std::wstring allFilesExtensionList;
for (const auto& filter : filters)
{
auto ext = FormatExtensionWithWildcard(filter);
buffer.push_back(filter);
buffer.push_back(ext);
allFilesExtensionList += ext + L";";
}
Copy link
Member

Choose a reason for hiding this comment

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

You worry about growing buffer above with more capacity than you immediately need (size = add 1 and double that) but not here for allFilesExtensionList? This line will cause extra allocation amounting to

wstring temp ctor = ext.operator+(L";")
allFilesExtensionList += 
temp dtor

SUGGESTION:

allFilesExtensionList.reserve(ext.length() + 1);
allFilesExtensionList += ext;
allFilesExtensionList += L";";

String+String creating a temporary's not perfectly efficient but not the worst thing in the world. Doing it in a loop gets more interesting as it magnifies the impact. And since you're already being somewhat meticulous elsewhere to efficiently minimize memory allocation (and deallocation) and copying data it seems worthy to do here too.

for (size_t i = 0; i < filters.Size(); i++)

if (!allFilesExtensionList.empty())
{
result.at(i) = { buffer.at(i * 2).c_str(), buffer.at(i * 2 + 1).c_str() };
allFilesExtensionList.pop_back(); // Remove the last semicolon
}

if (result.size() == 0)
if (filters.Size() == 0)
{
result.push_back({ L"All Files", L"*.*" });
// when filters not defined, set filter to All Files *.*
buffer.push_back(L"All Files");
Copy link
Collaborator

Choose a reason for hiding this comment

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

"All Files" needs to come from a localized string resource.

buffer.push_back(L"*.*");
}
else if (filters.Size() == 1 && allFilesExtensionList == L"*")
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the app chose "" as the extension, it seems wrong for us to override that the be ".*" with the "All Files" name we chose.

{
// when there're only one filter "*", set filter to All Files *.* (override the values pushed above)
buffer[0] = L"All Files";
buffer[1] = L"*.*";
resultSize = 1;
}
else
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this new automatic "All Files" behavior something the UWP pickers do? Conceptually, the automatic behavior is nice, but I worry we might be forcing something on apps which don't want it or might have issues in some cases. Two potential problems I'm thinking of:

  1. What if the app wants "All Images" rather than "All Files"? The app can add that themselves, but then we override the default with this additional option.
  2. What if the app has its own categories, such as "Word Documents (.doc, .docx)", "Excel Documents (.xls, .xlsx)", (category:) "Office Documents (.doc, .docx, .xls, .xlsx)", where extensions are duplicated? Does "All Files" end up duplicating the extensions from the "Office Documents" category?

My feeling is that to handling (1) we might need an explicit API for the app to say if it wants an automatic "All Files", perhaps with an optional string if they want a different string used.

Handling (2) likely requires extra de-duplicate code.

buffer.push_back(L"All Files");
buffer.push_back(allFilesExtensionList.c_str());
}

std::vector<COMDLG_FILTERSPEC> result(resultSize);
Copy link
Member

Choose a reason for hiding this comment

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

NIT: ...result{ resultSize };

ES.23: Prefer the {}-initializer syntax

Same comment applies throughout

for (size_t i = 0; i < resultSize; i++)
{
result.at(i) = { buffer.at(i * 2).c_str(), buffer.at(i * 2 + 1).c_str() };
}

return result;
}

/// <summary>
/// Capture and processing pickers filter inputs and convert them into Common Item Dialog's accepting type
/// Capture and processing pickers filter inputs and convert them into Common Item Dialog's accepting type, for FileSavePicker
/// </summary>
/// <param name="buffer">temp buffer to hold dynamically transformed strings</param>
/// <param name="filters">winrt style filters</param>
Expand Down
6 changes: 4 additions & 2 deletions specs/StoragePickers/FileSavePicker.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,8 @@ var savePicker = new FileSavePicker(this.AppWindow.Id)
// (Optional) specify the text displayed on commit button. If not specified, use system default.
CommitButtonText = "Save Document",

// (Optional) categorized extensions types. If not specified, use system default: All Files (*.*)
// (Optional) categorized extensions types. If not specified, allow All Files (*.*)
// Note that when allow All Files (*.*), end users can save a file without extension.
FileTypeChoices = {
{ "Documents", new List<string> { ".txt", ".doc", ".docx" } }
},
Expand All @@ -76,7 +77,8 @@ savePicker.SuggestedStartLocation(PickerLocationId::DocumentsLibrary);
// (Optional) specify the default file name. If not specified, use system default.
savePicker.SuggestedFileName(L"NewDocument");

// (Optional) categorized extensions types. If not specified, use system default: All Files (*.*)
// (Optional) categorized extensions types. If not specified, allow All Files (*.*)
// Note that when allow All Files (*.*), end users can save a file without extension.
savePicker.FileTypeChoices().Insert(L"Text", winrt::single_threaded_vector<winrt::hstring>({ L".txt" }));

// (Optional) specify the default file extension (will be appended after the default file name).
Expand Down