Fix FileTypeChoices in Storage Pickers to preserve insertion order #5948

DinahK-2SO · 2025-10-28T17:46:20Z

Description

This is a fix for issue:

Order of FileTypeChoices in new pickers is not respected #5827

The existing FileTypeChoices property was built on an unordered map, which does not preserve the user-defined order.

However, in the FileOpenPicker and FileSavePicker, the FileTypeChoices need to be displayed in the order they were inserted, rather than in a random order.

This is important for developers' and end-users' experience because:

Developers expect to see their file type options in the logical order they added them
The first added option is usually the default selection
Maintaining consistent ordering helps end users quickly locate the desired file type

Additionaly, the insertion order is respected in the legacy UWP FileSavePicker.

Fix

This is a backward-compatible fix. The goal of this fix is to maintain the existing Map type API contract and its good performance while ensuring that the display order of FileTypeChoices meets expectations.

This pull request refactors the implementation of the FileTypeChoicesMap to ensure that the insertion order of keys is preserved and provides efficient key lookups. It replaces the previous unordered map-based implementation with a custom ordered map backed by a vector and a hash map for O(1) lookups. The changes also introduce custom iterator and view types to maintain order during iteration and map viewing. Corresponding tests are updated to verify the new behavior.

Code change details:

Core implementation changes:

Refactored FileTypeChoicesMap to use a vector (m_orderedMap) for maintaining insertion order and an unordered map (m_keyToIndex) for fast key lookups, replacing the previous single_threaded_map-based approach. Added helper methods for key lookup and index finding. [1] [2]
Implemented custom OrderedMapIterator and OrderedMapView types to support ordered iteration and map viewing, ensuring the public API returns items in insertion order. [1] [2]

API and behavior updates:

Updated all IMap and IMapView interface methods (Insert, Lookup, Size, HasKey, GetView, Remove, Clear, First) to operate on the new ordered data structures and maintain correct semantics.

Testing and validation:

Modified picker tests to insert file type choices in a specific order (Pictures, Adobe Illustrator, Documents) and updated assertions to verify that the order is preserved in the resulting file type filter parameters. [1] [2] [3] [4]

These changes ensure that the FileTypeChoicesMap behaves as an ordered map, which is important for scenarios where the order of file type choices matters in the UI or API responses.

Scottj1s · 2025-10-29T16:48:35Z

dev/Interop/StoragePickers/FileTypeChoicesMap.cpp

+    size_t FileTypeChoicesMap::FindKeyIndex(hstring const& key) const
+    {
+        auto it = m_keyToIndex.find(std::wstring(key.c_str()));
+        return (it != m_keyToIndex.end()) ? it->second : SIZE_MAX;


minor clarification suggestion:
constexpr size_t element_not_found{ SIZE_MAX };

Scottj1s · 2025-10-29T17:06:08Z

dev/Interop/StoragePickers/FileTypeChoicesMap.cpp

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


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.

Scottj1s · 2025-10-29T17:07:37Z

dev/Interop/StoragePickers/FileTypeChoicesMap.cpp

    void FileTypeChoicesMap::Remove(hstring const& key)
    {
-        m_innerMap.Remove(key);
+        size_t index = FindKeyIndex(key);


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

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.

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)

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.

Scottj1s · 2025-10-29T17:11:22Z

dev/Interop/StoragePickers/FileTypeChoicesMap.cpp

+
+    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);


another dangling pointer concern

Scottj1s · 2025-10-29T17:13:15Z

dev/Interop/StoragePickers/FileTypeChoicesMap.cpp

+    }
+
+    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


Scottj1s · 2025-10-29T17:14:19Z

test/StoragePickersTests/PickerCommonTests.cpp

-                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(


add test case to exercise reuse of existing index

Scottj1s · 2025-10-29T17:16:07Z

dev/Interop/StoragePickers/FileTypeChoicesMap.cpp

+        throw winrt::hresult_out_of_bounds(L"Key not found");
+    }
+
+    uint32_t OrderedMapView::Size() const


Scottj1s · 2025-10-29T17:17:28Z

dev/Interop/StoragePickers/FileTypeChoicesMap.cpp

+        m_keyToIndex.reserve(8);
+    }
+
+    size_t FileTypeChoicesMap::FindKeyIndex(hstring const& key) const


lots of these methods can be made noexcept

codendone · 2025-10-29T00:00:10Z

dev/Interop/StoragePickers/FileTypeChoicesMap.h

+
+        // Helper methods
+        auto FindKey(hstring const& key) const;
+        auto FindKey(hstring const& key);


Why not only have the const version?

+1

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

codendone · 2025-10-29T18:45:42Z

dev/Interop/StoragePickers/FileTypeChoicesMap.cpp

    void FileTypeChoicesMap::Remove(hstring const& key)
    {
-        m_innerMap.Remove(key);
+        size_t index = FindKeyIndex(key);


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.

DrusTheAxe · 2025-10-30T15:46:01Z

dev/Interop/StoragePickers/FileTypeChoicesMap.cpp

+        return (it != m_keyToIndex.end()) ? it->second : SIZE_MAX;
+    }
+
+    auto FileTypeChoicesMap::FindKey(hstring const& key) const


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

DrusTheAxe · 2025-10-30T15:52:07Z

dev/Interop/StoragePickers/FileTypeChoicesMap.cpp

+        {
+            throw winrt::hresult_out_of_bounds();
+        }
+        // Create a simple key-value pair


NIT: Add blank line 139.5

DrusTheAxe · 2025-10-30T15:52:49Z

dev/Interop/StoragePickers/FileTypeChoicesMap.cpp

+            throw winrt::hresult_out_of_bounds();
+        }
+        // Create a simple key-value pair
+        auto tempMap = single_threaded_map<hstring, winrt::Windows::Foundation::Collections::IVector<hstring>>();


NIT: auto tempMap{ ... };

ES.23: Prefer the {}-initializer syntax

same comment applies throughout

DrusTheAxe · 2025-10-30T15:53:21Z

dev/Interop/StoragePickers/FileTypeChoicesMap.cpp

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


NIT: uint32_t copied{};

DrusTheAxe · 2025-10-30T15:54:30Z

dev/Interop/StoragePickers/FileTypeChoicesMap.cpp

+    }
+
+    // OrderedMapView implementation
+    auto OrderedMapView::FindKey(hstring const& key) const -> vector_type::const_iterator


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

DrusTheAxe · 2025-10-30T15:55:45Z

dev/Interop/StoragePickers/FileTypeChoicesMap.h

+
+        // Helper methods
+        auto FindKey(hstring const& key) const;
+        auto FindKey(hstring const& key);


+1

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

DrusTheAxe · 2025-10-30T15:56:55Z

dev/Interop/StoragePickers/FileTypeChoicesMap.h

+
+    private:
+        vector_type const& m_map;
+        size_t m_current;


size_t m_current{};

Always initialize variables

DrusTheAxe · 2025-10-30T15:58:34Z

dev/Interop/StoragePickers/FileTypeChoicesMap.h

+    {
+        using vector_type = std::vector<std::pair<hstring, winrt::Windows::Foundation::Collections::IVector<hstring>>>;
+
+        OrderedMapView(vector_type const& map) : m_map(map) {}


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

DrusTheAxe · 2025-10-30T16:00:04Z

dev/Interop/StoragePickers/FileTypeChoicesMap.h

+        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;


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)

DinahK-2SO added 2 commits October 28, 2025 15:53

FileTypeChoices should respect user defined order

e3eadf9

O(1) index

58e8476

DinahK-2SO requested a review from codendone October 28, 2025 17:46

microsoft-github-policy-service bot added the needs-triage label Oct 28, 2025

DinahK-2SO marked this pull request as ready for review October 28, 2025 17:47

DinahK-2SO added the area-File access label Oct 29, 2025

DinahK-2SO requested review from Hong-Xiang and Scottj1s October 29, 2025 09:17

Scottj1s reviewed Oct 29, 2025

View reviewed changes

codendone reviewed Oct 29, 2025

View reviewed changes

DrusTheAxe reviewed Oct 30, 2025

View reviewed changes

Fix FileTypeChoices in Storage Pickers to preserve insertion order #5948

Are you sure you want to change the base?

Fix FileTypeChoices in Storage Pickers to preserve insertion order #5948

Uh oh!

Conversation

DinahK-2SO commented Oct 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Scottj1s Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

DinahK-2SO commented Oct 28, 2025 •

edited

Loading

Scottj1s Oct 29, 2025 •

edited

Loading