Skip to content

Fix FileTypeChoices in Storage Pickers to preserve insertion order #5948

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
Open

Fix FileTypeChoices in Storage Pickers to preserve insertion order #5948

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
e3eadf9
FileTypeChoices should respect user defined order
DinahK-2SO Oct 28, 2025
58e8476
O(1) index
DinahK-2SO Oct 28, 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
161 changes: 151 additions & 10 deletions dev/Interop/StoragePickers/FileTypeChoicesMap.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,34 @@
#include "FileTypeChoicesMap.h"
#include "FileTypeFilterVector.h"
#include "TerminalVelocityFeatures-StoragePickers2.h"
#include <algorithm>
#include <unordered_map>

namespace winrt::Microsoft::Windows::Storage::Pickers::implementation
{
FileTypeChoicesMap::FileTypeChoicesMap()
{
// Pre-allocate capacity for typical file type choices (usually 2-8 items)
m_orderedMap.reserve(8);
m_keyToIndex.reserve(8);
}

size_t FileTypeChoicesMap::FindKeyIndex(hstring const& key) const
Copy link
Member

@Scottj1s Scottj1s Oct 29, 2025

Choose a reason for hiding this comment

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

lots of these methods can be made noexcept

{
auto it = m_keyToIndex.find(std::wstring(key.c_str()));
return (it != m_keyToIndex.end()) ? it->second : SIZE_MAX;
Copy link
Member

@Scottj1s Scottj1s Oct 29, 2025

Choose a reason for hiding this comment

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

minor clarification suggestion:
constexpr size_t element_not_found{ SIZE_MAX };

}

auto FileTypeChoicesMap::FindKey(hstring const& key) const
Copy link
Member

@DrusTheAxe DrusTheAxe Oct 30, 2025

Choose a reason for hiding this comment

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

auto return type for a method is cute but harder to reader/grok. That's a long-term maintainability tax which is the wrong vector (pardon the pun) -- generally best for code to be easily maintainable, not hard to maintain.

SUGGESTION: Specify actual return type

same comment applies throughout

same comment

{
size_t index = FindKeyIndex(key);
return (index != SIZE_MAX) ? m_orderedMap.begin() + index : m_orderedMap.end();
}

auto FileTypeChoicesMap::FindKey(hstring const& key)
{
size_t index = FindKeyIndex(key);
return (index != SIZE_MAX) ? m_orderedMap.begin() + index : m_orderedMap.end();
}

bool FileTypeChoicesMap::Insert(hstring const& key, winrt::Windows::Foundation::Collections::IVector<hstring> const& value)
Expand All @@ -21,7 +44,7 @@ namespace winrt::Microsoft::Windows::Storage::Pickers::implementation

// Create a new FileTypeFilterVector and copy all values from the input vector
auto validatingVector = make<FileTypeFilterVector>();

if (value)
{
// Each Append call will validate the extension
Expand All @@ -30,42 +53,160 @@ namespace winrt::Microsoft::Windows::Storage::Pickers::implementation
validatingVector.as<winrt::Windows::Foundation::Collections::IVector<hstring>>().Append(item);
}
}

return m_innerMap.Insert(key, validatingVector);

// Check if key already exists using O(1) lookup
size_t index = FindKeyIndex(key);
if (index != SIZE_MAX)
{
// Key exists, update the value
m_orderedMap[index].second = validatingVector;
return false; // Indicates replacement, not insertion
}
else
{
// Key doesn't exist, insert new pair at the end to maintain insertion order
size_t newIndex = m_orderedMap.size();
m_orderedMap.emplace_back(key, validatingVector);
m_keyToIndex[std::wstring(key.c_str())] = newIndex;
return true; // Indicates new insertion
}
}

winrt::Windows::Foundation::Collections::IVector<hstring> FileTypeChoicesMap::Lookup(hstring const& key) const
{
return m_innerMap.Lookup(key);
auto it = FindKey(key);
if (it != m_orderedMap.end())
{
return it->second;
}
throw winrt::hresult_out_of_bounds(L"Key not found");
}

uint32_t FileTypeChoicesMap::Size() const
{
return m_innerMap.Size();
return static_cast<uint32_t>(m_orderedMap.size());
}

bool FileTypeChoicesMap::HasKey(hstring const& key) const
{
return m_innerMap.HasKey(key);
return FindKey(key) != m_orderedMap.end();
}

winrt::Windows::Foundation::Collections::IMapView<hstring, winrt::Windows::Foundation::Collections::IVector<hstring>> FileTypeChoicesMap::GetView() const
{
return m_innerMap.GetView();
return make<OrderedMapView>(m_orderedMap);
Copy link
Member

@Scottj1s Scottj1s Oct 29, 2025

Choose a reason for hiding this comment

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

If caller holds GetView return value past FileTypeChoicesMap destruction, you'll have a dangling pointer crash. Consider std::make_shared on m_orderedMap, winrt::make_weak on FileTypeChoicesMap, etc.

}

void FileTypeChoicesMap::Remove(hstring const& key)
{
m_innerMap.Remove(key);
size_t index = FindKeyIndex(key);
Copy link
Member

@Scottj1s Scottj1s Oct 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Non-atomic reading and/or writing of m_orderedMap & m_keyToIndex - consider a mutex. Same for nearly all other methods.

Copy link
Collaborator

@codendone codendone Oct 29, 2025

Choose a reason for hiding this comment

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

Or have just one std::vector with a pair of the key and its value. Lookup may theoretically be slower, but the expected count is so low that it is unlikely to matter.

Copy link
Member

@Scottj1s Scottj1s Oct 29, 2025

Choose a reason for hiding this comment

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

Or have just one std::vector with a pair of the key and its value. Lookup may theoretically be slower, but the expected count is so low that it is unlikely to matter.

that would resolve some scenarios but not all (TOCTOU)

Copy link
Member

@DrusTheAxe DrusTheAxe Oct 30, 2025

Choose a reason for hiding this comment

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

Is this API an inproc or OOP WinRT object?

If inproc then why should it internally protect instances against concurrent access? The method's not static / no global variables, instances are not typically expected to be accessed concurrently, and typically if a developer does concurrently access an instance of an object from 2 threads managing that concurrent access is their responsbility.

If OOP then yes, every method exposed by every interface on the runtimeclass is potentially access concurrently and needs to protect itself.

if (index != SIZE_MAX)
{
// Remove from vector
m_orderedMap.erase(m_orderedMap.begin() + index);

// Remove from hash map
m_keyToIndex.erase(std::wstring(key.c_str()));

// Update indices for all elements after the removed one
for (auto& kvp : m_keyToIndex)
{
if (kvp.second > index)
{
kvp.second--;
}
}
}
}

void FileTypeChoicesMap::Clear()
{
m_innerMap.Clear();
m_orderedMap.clear();
m_keyToIndex.clear();
}

winrt::Windows::Foundation::Collections::IIterator<winrt::Windows::Foundation::Collections::IKeyValuePair<hstring, winrt::Windows::Foundation::Collections::IVector<hstring>>> FileTypeChoicesMap::First() const
{
return m_innerMap.First();
return make<OrderedMapIterator>(m_orderedMap);
}

// OrderedMapIterator implementation
winrt::Windows::Foundation::Collections::IKeyValuePair<hstring, winrt::Windows::Foundation::Collections::IVector<hstring>> OrderedMapIterator::Current() const
{
if (m_current >= m_map.size())
{
throw winrt::hresult_out_of_bounds();
}
// Create a simple key-value pair
Copy link
Member

@DrusTheAxe DrusTheAxe Oct 30, 2025

Choose a reason for hiding this comment

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

NIT: Add blank line 139.5

auto tempMap = single_threaded_map<hstring, winrt::Windows::Foundation::Collections::IVector<hstring>>();
Copy link
Member

@DrusTheAxe DrusTheAxe Oct 30, 2025

Choose a reason for hiding this comment

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

NIT: auto tempMap{ ... };

ES.23: Prefer the {}-initializer syntax

same comment applies throughout

tempMap.Insert(m_map[m_current].first, m_map[m_current].second);
return tempMap.First().Current();
}

bool OrderedMapIterator::HasCurrent() const
{
return m_current < m_map.size();
}

bool OrderedMapIterator::MoveNext()
{
if (m_current < m_map.size())
{
++m_current;
return m_current < m_map.size();
}
return false;
}

uint32_t OrderedMapIterator::GetMany(array_view<winrt::Windows::Foundation::Collections::IKeyValuePair<hstring, winrt::Windows::Foundation::Collections::IVector<hstring>>> items)
{
uint32_t copied = 0;
Copy link
Member

@DrusTheAxe DrusTheAxe Oct 30, 2025

Choose a reason for hiding this comment

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

NIT: uint32_t copied{};

while (copied < items.size() && HasCurrent())
{
items[copied] = Current();
MoveNext();
++copied;
}
return copied;
}

// OrderedMapView implementation
auto OrderedMapView::FindKey(hstring const& key) const -> vector_type::const_iterator
Copy link
Member

@DrusTheAxe DrusTheAxe Oct 30, 2025

Choose a reason for hiding this comment

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

Why auto OrderedMapView::FindKey(hstring const& key) const -> vector_type::const_iterator
and not vector_type::const_iterator OrderedMapView::FindKey(hstring const& key) const
?

{
return std::find_if(m_map.begin(), m_map.end(),
[&key](const auto& pair) { return pair.first == key; });
}

winrt::Windows::Foundation::Collections::IVector<hstring> OrderedMapView::Lookup(hstring const& key) const
{
auto it = FindKey(key);
if (it != m_map.end())
{
return it->second;
}
throw winrt::hresult_out_of_bounds(L"Key not found");
}

uint32_t OrderedMapView::Size() const
Copy link
Member

@Scottj1s Scottj1s Oct 29, 2025

Choose a reason for hiding this comment

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

noexcept

{
return static_cast<uint32_t>(m_map.size());
}

bool OrderedMapView::HasKey(hstring const& key) const
{
return FindKey(key) != m_map.end();
}

void OrderedMapView::Split(winrt::Windows::Foundation::Collections::IMapView<hstring, winrt::Windows::Foundation::Collections::IVector<hstring>>& first,
winrt::Windows::Foundation::Collections::IMapView<hstring, winrt::Windows::Foundation::Collections::IVector<hstring>>& second) const
Copy link
Member

@Scottj1s Scottj1s Oct 29, 2025

Choose a reason for hiding this comment

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

noexcept

{
// For simplicity, not implementing split
first = nullptr;
second = nullptr;
}

winrt::Windows::Foundation::Collections::IIterator<winrt::Windows::Foundation::Collections::IKeyValuePair<hstring, winrt::Windows::Foundation::Collections::IVector<hstring>>> OrderedMapView::First() const
{
return make<OrderedMapIterator>(m_map);
Copy link
Member

@Scottj1s Scottj1s Oct 29, 2025

Choose a reason for hiding this comment

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

another dangling pointer concern

}
}
58 changes: 56 additions & 2 deletions dev/Interop/StoragePickers/FileTypeChoicesMap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,16 @@

#pragma once
#include <winrt/Windows.Foundation.Collections.h>
#include <vector>
#include <utility>
#include "FileTypeFilterVector.h"

namespace winrt::Microsoft::Windows::Storage::Pickers::implementation
{
// Forward declarations
struct OrderedMapView;
struct OrderedMapIterator;

struct FileTypeChoicesMap : implements<FileTypeChoicesMap,
winrt::Windows::Foundation::Collections::IMap<hstring, winrt::Windows::Foundation::Collections::IVector<hstring>>,
winrt::Windows::Foundation::Collections::IIterable<winrt::Windows::Foundation::Collections::IKeyValuePair<hstring, winrt::Windows::Foundation::Collections::IVector<hstring>>>>
Expand All @@ -28,7 +34,55 @@ namespace winrt::Microsoft::Windows::Storage::Pickers::implementation
winrt::Windows::Foundation::Collections::IIterator<winrt::Windows::Foundation::Collections::IKeyValuePair<hstring, winrt::Windows::Foundation::Collections::IVector<hstring>>> First() const;

private:
winrt::Windows::Foundation::Collections::IMap<hstring, winrt::Windows::Foundation::Collections::IVector<hstring>> m_innerMap{
single_threaded_map<hstring, winrt::Windows::Foundation::Collections::IVector<hstring>>() };
// Use vector to maintain insertion order + hash map for O(1) lookup
std::vector<std::pair<hstring, winrt::Windows::Foundation::Collections::IVector<hstring>>> m_orderedMap;
std::unordered_map<std::wstring, size_t> m_keyToIndex; // Maps key to index in m_orderedMap

// Helper methods
auto FindKey(hstring const& key) const;
auto FindKey(hstring const& key);
Copy link
Collaborator

@codendone codendone Oct 29, 2025

Choose a reason for hiding this comment

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

Why not only have the const version?

Copy link
Member

@DrusTheAxe DrusTheAxe Oct 30, 2025

Choose a reason for hiding this comment

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

+1

Usually only need const + non-const for mutating methods. Read-only behavior only needs const method

size_t FindKeyIndex(hstring const& key) const;
};

// Custom iterator to maintain insertion order
struct OrderedMapIterator : implements<OrderedMapIterator,
winrt::Windows::Foundation::Collections::IIterator<winrt::Windows::Foundation::Collections::IKeyValuePair<hstring, winrt::Windows::Foundation::Collections::IVector<hstring>>>>
{
using vector_type = std::vector<std::pair<hstring, winrt::Windows::Foundation::Collections::IVector<hstring>>>;

OrderedMapIterator(vector_type const& map) : m_map(map), m_current(0) {}

winrt::Windows::Foundation::Collections::IKeyValuePair<hstring, winrt::Windows::Foundation::Collections::IVector<hstring>> Current() const;
bool HasCurrent() const;
bool MoveNext();
uint32_t GetMany(array_view<winrt::Windows::Foundation::Collections::IKeyValuePair<hstring, winrt::Windows::Foundation::Collections::IVector<hstring>>> items);

private:
vector_type const& m_map;
size_t m_current;
Copy link
Member

@DrusTheAxe DrusTheAxe Oct 30, 2025

Choose a reason for hiding this comment

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

size_t m_current{};

Always initialize variables

};

// Custom view to maintain insertion order
struct OrderedMapView : implements<OrderedMapView,
winrt::Windows::Foundation::Collections::IMapView<hstring, winrt::Windows::Foundation::Collections::IVector<hstring>>,
winrt::Windows::Foundation::Collections::IIterable<winrt::Windows::Foundation::Collections::IKeyValuePair<hstring, winrt::Windows::Foundation::Collections::IVector<hstring>>>>
{
using vector_type = std::vector<std::pair<hstring, winrt::Windows::Foundation::Collections::IVector<hstring>>>;

OrderedMapView(vector_type const& map) : m_map(map) {}
Copy link
Member

@DrusTheAxe DrusTheAxe Oct 30, 2025

Choose a reason for hiding this comment

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

Why's this method inline but not any others?

e.g. Size() is equally trivial or more so but not inline

SUGGEST: Change this to non-inline for consistency with the rest of the code


// IMapView
winrt::Windows::Foundation::Collections::IVector<hstring> Lookup(hstring const& key) const;
uint32_t Size() const;
bool HasKey(hstring const& key) const;
void Split(winrt::Windows::Foundation::Collections::IMapView<hstring, winrt::Windows::Foundation::Collections::IVector<hstring>>& first,
winrt::Windows::Foundation::Collections::IMapView<hstring, winrt::Windows::Foundation::Collections::IVector<hstring>>& second) const;

// IIterable
winrt::Windows::Foundation::Collections::IIterator<winrt::Windows::Foundation::Collections::IKeyValuePair<hstring, winrt::Windows::Foundation::Collections::IVector<hstring>>> First() const;

private:
vector_type const& m_map;
Copy link
Member

@DrusTheAxe DrusTheAxe Oct 30, 2025

Choose a reason for hiding this comment

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

What happens when the underlying map object passed in to the constructor is destroyed and then this object is used?

I think Scott mentioned this earlier (dangling pointer etc)

vector_type::const_iterator FindKey(hstring const& key) const;
};
}
30 changes: 20 additions & 10 deletions test/StoragePickersTests/PickerCommonTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,10 +214,12 @@ namespace Test::PickerCommonTests
winrt::Microsoft::UI::WindowId windowId{};
winrt::Microsoft::Windows::Storage::Pickers::FileOpenPicker picker(windowId);

picker.FileTypeChoices().Insert(
L"Documents", winrt::single_threaded_vector<winrt::hstring>({ L".txt", L".doc", L".docx" }));
picker.FileTypeChoices().Insert(
L"Pictures", winrt::single_threaded_vector<winrt::hstring>({ L".png", L".jpg", L".jpeg", L".bmp" }));
picker.FileTypeChoices().Insert(
Copy link
Member

@Scottj1s Scottj1s Oct 29, 2025

Choose a reason for hiding this comment

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

add test case to exercise reuse of existing index

L"Adobe Illustrator", winrt::single_threaded_vector<winrt::hstring>({ L".ai" }));
picker.FileTypeChoices().Insert(
L"Documents", winrt::single_threaded_vector<winrt::hstring>({ L".txt", L".doc", L".docx" }));

// Act.
PickerParameters parameters{};
Expand All @@ -226,14 +228,17 @@ namespace Test::PickerCommonTests
picker.FileTypeChoices().GetView());

// Assert.
VERIFY_ARE_EQUAL(parameters.FileTypeFilterPara.size(), 2);
VERIFY_ARE_EQUAL(parameters.FileTypeFilterPara.size(), 3);

VERIFY_ARE_EQUAL(
std::wstring(parameters.FileTypeFilterPara[0].pszSpec),
L"*.txt;*.doc;*.docx");
L"*.png;*.jpg;*.jpeg;*.bmp");
VERIFY_ARE_EQUAL(
std::wstring(parameters.FileTypeFilterPara[1].pszSpec),
L"*.png;*.jpg;*.jpeg;*.bmp");
L"*.ai");
VERIFY_ARE_EQUAL(
std::wstring(parameters.FileTypeFilterPara[2].pszSpec),
L"*.txt;*.doc;*.docx");
}

TEST_METHOD(VerifyFilters_FileSavePickerWhenFileTypeChoicesDefinedExpectMatchingSpec)
Expand All @@ -242,10 +247,12 @@ namespace Test::PickerCommonTests
winrt::Microsoft::UI::WindowId windowId{};
winrt::Microsoft::Windows::Storage::Pickers::FileSavePicker picker(windowId);

picker.FileTypeChoices().Insert(
L"Documents", winrt::single_threaded_vector<winrt::hstring>({ L".txt", L".doc", L".docx" }));
picker.FileTypeChoices().Insert(
L"Pictures", winrt::single_threaded_vector<winrt::hstring>({ L".png", L".jpg", L".jpeg", L".bmp" }));
picker.FileTypeChoices().Insert(
L"Adobe Illustrator", winrt::single_threaded_vector<winrt::hstring>({ L".ai" }));
picker.FileTypeChoices().Insert(
L"Documents", winrt::single_threaded_vector<winrt::hstring>({ L".txt", L".doc", L".docx" }));

// Act.
PickerParameters parameters{};
Expand All @@ -254,14 +261,17 @@ namespace Test::PickerCommonTests
picker.FileTypeChoices().GetView());

// Assert.
VERIFY_ARE_EQUAL(parameters.FileTypeFilterPara.size(), 2);
VERIFY_ARE_EQUAL(parameters.FileTypeFilterPara.size(), 3);

VERIFY_ARE_EQUAL(
std::wstring(parameters.FileTypeFilterPara[0].pszSpec),
L"*.txt;*.doc;*.docx");
L"*.png;*.jpg;*.jpeg;*.bmp");
VERIFY_ARE_EQUAL(
std::wstring(parameters.FileTypeFilterPara[1].pszSpec),
L"*.png;*.jpg;*.jpeg;*.bmp");
L"*.ai");
VERIFY_ARE_EQUAL(
std::wstring(parameters.FileTypeFilterPara[2].pszSpec),
L"*.txt;*.doc;*.docx");
}

TEST_METHOD(VerifyFilters_FileSavePickerWhenNoFileTypeChoicesDefinedExpectAsteriskSpec)
Expand Down