Rebuild the profile nav via MenuItemsSource; mitigate a crash (#14630)

Directly manipulating the `NavigationView::MenuItems` vector is bad. If
you do that, you're gonna get crashes, in WinUI code for `Measure`.
However, a WinUI PR (below) gave me an idea: Changing
`NavigationView::MenuItemsSource` will wholly invalidate the entirety of
the nav view items, and that will avoid the crash.

This code does that. It's a wee bit janky, but it works.

Closes #13673

_might_ affect #12333, need to try and repro.

See also:
* #9273
* #10390
* microsoft/microsoft-ui-xaml#6302
* microsoft/microsoft-ui-xaml#3138, which was
the fix for microsoft/microsoft-ui-xaml#2818

(cherry picked from commit 8c17475)
Service-Card-Id: 88428128
Service-Version: 1.17

Author: zadjii-msft

src/cascadia/TerminalSettingsEditor/MainPage.cpp

@@ -117,6 +117,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
             lastBreadcrumb = _breadcrumbs.GetAt(size - 1);
         }
 
+        // Collect all the values out of the old nav view item source
         auto menuItems{ SettingsNav().MenuItems() };
 
         // We'll remove a bunch of items and iterate over it twice.
@@ -156,7 +157,14 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
                 }),
             menuItemsSTL.end());
 
-        menuItems.ReplaceAll(menuItemsSTL);
+        // Now, we've got a list of just the static entries again. Lets take
+        // those and stick them back into a new winrt vector, and set that as
+        // the source again.
+        //
+        // By setting MenuItemsSource in its entirety, rather than manipulating
+        // MenuItems, we avoid a crash in WinUI.
+        auto newSource = winrt::single_threaded_vector<IInspectable>(std::move(menuItemsSTL));
+        SettingsNav().MenuItemsSource(newSource);
 
         // Repopulate profile-related menu items
         _InitializeProfilesList();
@@ -209,7 +217,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
         // Couldn't find the selected item, fallback to first menu item
         // This happens when the selected item was a profile which doesn't exist in the new configuration
         // We can use menuItemsSTL here because the only things they miss are profile entries.
-        const auto& firstItem{ menuItemsSTL.at(0).as<MUX::Controls::NavigationViewItem>() };
+        const auto& firstItem{ SettingsNav().MenuItems().GetAt(0).as<MUX::Controls::NavigationViewItem>() };
         SettingsNav().SelectedItem(firstItem);
         _Navigate(unbox_value<hstring>(firstItem.Tag()), BreadcrumbSubPage::None);
     }
@@ -527,7 +535,17 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
 
     void MainPage::_InitializeProfilesList()
     {
-        const auto menuItems = SettingsNav().MenuItems();
+        const auto& itemSource{ SettingsNav().MenuItemsSource() };
+        if (!itemSource)
+        {
+            // There wasn't a MenuItemsSource set yet? The only way that's
+            // possible is if we haven't used
+            // _MoveXamlParsedNavItemsIntoItemSource to move the hardcoded menu
+            // entries from XAML into our runtime menu item source. Do that now.
+
+            _MoveXamlParsedNavItemsIntoItemSource();
+        }
+        const auto menuItems = SettingsNav().MenuItemsSource().try_as<IVector<IInspectable>>();
 
         // Manually create a NavigationViewItem for each profile
         // and keep a reference to them in a map so that we
@@ -557,6 +575,37 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
         menuItems.Append(addProfileItem);
     }
 
+    // BODGY
+    // Does the very wacky business of moving all our MenuItems that we
+    // hardcoded in XAML into a runtime MenuItemsSource. We'll then use _that_
+    // MenuItemsSource as the source for our nav view entries instead. This
+    // lets us hardcode the initial entries in precompiled XAML, but then adjust
+    // the items at runtime. Without using a MenuItemsSource, the NavView just
+    // crashes when items are removed (see GH#13673)
+    void MainPage::_MoveXamlParsedNavItemsIntoItemSource()
+    {
+        if (SettingsNav().MenuItemsSource())
+        {
+            // We've already copied over the original items to a source. We can
+            // just skip this now.
+            return;
+        }
+
+        auto menuItems{ SettingsNav().MenuItems() };
+        // Remove all the existing items, and move them to a separate vector
+        // that we'll use as a MenuItemsSource. By doing this, we avoid a WinUI
+        // bug (MUX#6302) where modifying the NavView.Items() directly causes a
+        // crash. By leaving these static entries in XAML, we maintain the
+        // benefit of instantiating them from the XBF, rather than at runtime.
+        //
+        // --> Copy it into an STL vector to simplify our code and reduce COM overhead.
+        std::vector<IInspectable> menuItemsSTL(menuItems.Size(), nullptr);
+        menuItems.GetMany(0, menuItemsSTL);
+
+        auto newSource = winrt::single_threaded_vector<IInspectable>(std::move(menuItemsSTL));
+        SettingsNav().MenuItemsSource(newSource);
+    }
+
     void MainPage::_CreateAndNavigateToNewProfile(const uint32_t index, const Model::Profile& profile)
     {
         const auto newProfile{ profile ? profile : _settingsClone.CreateNewProfile() };

src/cascadia/TerminalSettingsEditor/MainPage.h

@@ -68,6 +68,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
         void _Navigate(const Editor::ProfileViewModel& profile, BreadcrumbSubPage subPage);
 
         void _UpdateBackgroundForMica();
+        void _MoveXamlParsedNavItemsIntoItemSource();
 
         winrt::Microsoft::Terminal::Settings::Editor::ColorSchemesPageViewModel _colorSchemesPageVM{ nullptr };