Skip to content

Commit

Permalink
fix not updating the nav view when add/removing profiles (#15162)
Browse files Browse the repository at this point in the history 
* make the list of MenuItems observable, so the nav view can actually
listen for changes to it
* Use the MenuItemsSource to find the index to add at, rather than the
MenuItems (which isn't accurate anymore)
* Stash a single observable vector as the menuitemsource, and modify
that whenever we need to do modifications.
* I attempted to create a new vector, then copy into the new one, then
replace the MenuItemsSource with the new vector, but that _refused_ to
work. So let's just... not.

Regressed in #14630
Closes #15140

Manually validated that this and #13673 are still fixed
  • Loading branch information
@zadjii-msft
zadjii-msft authored Apr 17, 2023
1 parent e106c09 commit 1825ca1
Show file tree
Hide file tree
Showing 2 changed files with 50 additions and 74 deletions.
121 changes: 47 additions & 74 deletions src/cascadia/TerminalSettingsEditor/MainPage.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,54 +117,18 @@ 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.
// --> 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);

// We want to refresh the list of profiles in the NavigationView.
// In order to add profiles we can use _InitializeProfilesList();
// But before we can do that we have to remove existing profiles first of course.
// This "erase-remove" idiom will achieve just that.
menuItemsSTL.erase(
std::remove_if(
menuItemsSTL.begin(),
menuItemsSTL.end(),
[](const auto& item) -> bool {
if (const auto& navViewItem{ item.try_as<MUX::Controls::NavigationViewItem>() })
{
if (const auto& tag{ navViewItem.Tag() })
{
if (tag.try_as<Editor::ProfileViewModel>())
{
// remove NavViewItem pointing to a Profile
return true;
}
if (const auto& stringTag{ tag.try_as<hstring>() })
{
if (stringTag == addProfileTag)
{
// remove the "Add Profile" item
return true;
}
}
}
}
return false;
}),
menuItemsSTL.end());

// 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.
// Collect only the first items out of the menu item source, the static
// ones that we don't want to regenerate.
//
// 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);
// By manipulating a MenuItemsSource this way, rather than manipulating the
// MenuItems directly, we avoid a crash in WinUI.
//
// By making the vector only _originalNumItems big to start, GetMany
// will only fill that number of elements out of the current source.
std::vector<IInspectable> menuItemsSTL(_originalNumItems, nullptr);
_menuItemSource.GetMany(0, menuItemsSTL);
// now, just stick them back in.
_menuItemSource.ReplaceAll(menuItemsSTL);

// Repopulate profile-related menu items
_InitializeProfilesList();
Expand All @@ -177,7 +141,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
// refresh the current page using the breadcrumb data we collected before the refresh
if (const auto& crumb{ lastBreadcrumb.try_as<Breadcrumb>() })
{
for (const auto& item : menuItems)
for (const auto& item : _menuItemSource)
{
if (const auto& menuItem{ item.try_as<MUX::Controls::NavigationViewItem>() })
{
Expand Down Expand Up @@ -217,7 +181,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{ SettingsNav().MenuItems().GetAt(0).as<MUX::Controls::NavigationViewItem>() };
const auto& firstItem{ _menuItemSource.GetAt(0).as<MUX::Controls::NavigationViewItem>() };
SettingsNav().SelectedItem(firstItem);
_Navigate(unbox_value<hstring>(firstItem.Tag()), BreadcrumbSubPage::None);
}
Expand Down Expand Up @@ -251,8 +215,10 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
{
uint32_t insertIndex;
auto selectedItem{ SettingsNav().SelectedItem() };
auto menuItems{ SettingsNav().MenuItems() };
menuItems.IndexOf(selectedItem, insertIndex);
if (_menuItemSource)
{
_menuItemSource.IndexOf(selectedItem, insertIndex);
}
if (profileGuid != winrt::guid{})
{
// if we were given a non-empty guid, we want to duplicate the corresponding profile
Expand Down Expand Up @@ -545,7 +511,6 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation

_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
Expand All @@ -558,7 +523,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
auto profileVM = _viewModelForProfile(profile, _settingsClone);
profileVM.SetupAppearances(_colorSchemesPageVM.AllColorSchemes());
auto navItem = _CreateProfileNavViewItem(profileVM);
menuItems.Append(navItem);
_menuItemSource.Append(navItem);
}
}

Expand All @@ -572,7 +537,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
icon.Glyph(L"\xE710");
addProfileItem.Icon(icon);

menuItems.Append(addProfileItem);
_menuItemSource.Append(addProfileItem);
}

// BODGY
Expand All @@ -592,18 +557,20 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
}

auto menuItems{ SettingsNav().MenuItems() };
_originalNumItems = menuItems.Size();
// 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 original = std::vector<IInspectable>{ _originalNumItems, nullptr };
menuItems.GetMany(0, original);

auto newSource = winrt::single_threaded_vector<IInspectable>(std::move(menuItemsSTL));
SettingsNav().MenuItemsSource(newSource);
_menuItemSource = winrt::single_threaded_observable_vector<IInspectable>(std::move(original));

SettingsNav().MenuItemsSource(_menuItemSource);
}

void MainPage::_CreateAndNavigateToNewProfile(const uint32_t index, const Model::Profile& profile)
Expand All @@ -612,7 +579,11 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
const auto profileViewModel{ _viewModelForProfile(newProfile, _settingsClone) };
profileViewModel.SetupAppearances(_colorSchemesPageVM.AllColorSchemes());
const auto navItem{ _CreateProfileNavViewItem(profileViewModel) };
SettingsNav().MenuItems().InsertAt(index, navItem);

if (_menuItemSource)
{
_menuItemSource.InsertAt(index, navItem);
}

// Select and navigate to the new profile
SettingsNav().SelectedItem(navItem);
Expand Down Expand Up @@ -666,22 +637,24 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
// remove selected item
uint32_t index;
auto selectedItem{ SettingsNav().SelectedItem() };
auto menuItems{ SettingsNav().MenuItems() };
menuItems.IndexOf(selectedItem, index);
menuItems.RemoveAt(index);

// navigate to the profile next to this one
const auto newSelectedItem{ menuItems.GetAt(index < menuItems.Size() - 1 ? index : index - 1) };
SettingsNav().SelectedItem(newSelectedItem);
const auto newTag = newSelectedItem.as<MUX::Controls::NavigationViewItem>().Tag();
if (const auto profileViewModel = newTag.try_as<ProfileViewModel>())
if (_menuItemSource)
{
profileViewModel->FocusDeleteButton(true);
_Navigate(*profileViewModel, BreadcrumbSubPage::None);
}
else
{
_Navigate(newTag.as<hstring>(), BreadcrumbSubPage::None);
_menuItemSource.IndexOf(selectedItem, index);
_menuItemSource.RemoveAt(index);

// navigate to the profile next to this one
const auto newSelectedItem{ _menuItemSource.GetAt(index < _menuItemSource.Size() - 1 ? index : index - 1) };
SettingsNav().SelectedItem(newSelectedItem);
const auto newTag = newSelectedItem.as<MUX::Controls::NavigationViewItem>().Tag();
if (const auto profileViewModel = newTag.try_as<ProfileViewModel>())
{
profileViewModel->FocusDeleteButton(true);
_Navigate(*profileViewModel, BreadcrumbSubPage::None);
}
else
{
_Navigate(newTag.as<hstring>(), BreadcrumbSubPage::None);
}
}
}

Expand Down
3 changes: 3 additions & 0 deletions src/cascadia/TerminalSettingsEditor/MainPage.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,9 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation

private:
Windows::Foundation::Collections::IObservableVector<IInspectable> _breadcrumbs;
Windows::Foundation::Collections::IObservableVector<IInspectable> _menuItemSource;
size_t _originalNumItems = 0u;

Model::CascadiaSettings _settingsSource;
Model::CascadiaSettings _settingsClone;

Expand Down

0 comments on commit 1825ca1

Please sign in to comment.