Skip to content
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

fix not updating the nav view when add/removing profiles #15162

Merged
merged 11 commits into from
Apr 17, 2023
Merged

fix not updating the nav view when add/removing profiles #15162

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
1ecbc18
fix not updating the nav view when add/removing profiles
zadjii-msft Apr 11, 2023
a28a6aa
add guards to menuitemsource.try_as
zadjii-msft Apr 11, 2023
eb8c196
Merge branch 'main' into dev/migire/b/15140-settings-add-remove
zadjii-msft Apr 12, 2023
66520dd
WHAT THE HECK IS GOING ON
zadjii-msft Apr 13, 2023
1720e85
yep this works
zadjii-msft Apr 13, 2023
e8c0224
cleanup
zadjii-msft Apr 13, 2023
ab4ba7c
a bit of dead code
zadjii-msft Apr 13, 2023
b8b61bd
Merge branch 'main' into dev/migire/b/15140-settings-add-remove
zadjii-msft Apr 17, 2023
6d8c1b0
Merge remote-tracking branch 'origin/main' into dev/migire/b/15140-se…
zadjii-msft Apr 17, 2023
249ccb0
cleanup
zadjii-msft Apr 17, 2023
31b8e9d
correct, this was redundant
zadjii-msft Apr 17, 2023
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
121 changes: 47 additions & 74 deletions src/cascadia/TerminalSettingsEditor/MainPage.cpp
View file
Open in desktop
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
Open in desktop
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