Skip to content

Commit

Permalink
Fix refresh-related crashes in Settings UI (microsoft#8773)
Browse files Browse the repository at this point in the history 
Removes the visibility hack in `UpdateSettings` where we were hiding
Profile menu items instead of removing them. This hack was removed using
`ReplaceAll`. For an unknown reason, calling `Remove()` would result in
an out-of-bounds error in XAML code.

The "Discard" button would improperly refresh the Settings UI. Both of
the bugs were caused by holding a reference to a hidden menu item then
trying to set the `SelectedItem` to that menu item. 

Additionally, 9283375 adds a check for the selected item in
`SettingsNav_ItemInvoked()`. This prevents navigation to an already
selected item. This was the heuristic used by the XAML Controls Gallery.

References microsoft#6800 - Settings UI Epic

## Validation Steps Performed
(Repeated for each menu item)
1. Select the menu item
2. click "Discard changes"
3. Verify navigated to same page

Also performed repro steps for microsoft#8747 and microsoft#8748.

Closes microsoft#8747
Closes microsoft#8748
  • Loading branch information
@carlos-zamora
carlos-zamora authored Jan 19, 2021
1 parent c33a979 commit 9fed14a
Show file tree
Hide file tree
Showing 2 changed files with 68 additions and 41 deletions.
108 changes: 68 additions & 40 deletions src/cascadia/TerminalSettingsEditor/MainPage.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,74 +78,96 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation

co_await winrt::resume_foreground(Dispatcher());

// "remove" all profile-related NavViewItems
// LOAD-BEARING: use Visibility here, instead of menuItems.Remove().
// Remove() works fine on NavViewItems with an hstring tag,
// but causes an out-of-bounds error with Profile tagged items.
// The cause of this error is unknown.
// Deduce information about the currently selected item
IInspectable selectedItemTag;
auto menuItems{ SettingsNav().MenuItems() };
for (auto i = menuItems.Size() - 1; i > 0; --i)
if (const auto& selectedItem{ SettingsNav().SelectedItem() })
{
if (const auto navViewItem{ menuItems.GetAt(i).try_as<MUX::Controls::NavigationViewItem>() })
if (const auto& navViewItem{ selectedItem.try_as<MUX::Controls::NavigationViewItem>() })
{
if (const auto tag{ navViewItem.Tag() })
selectedItemTag = navViewItem.Tag();
}
}

// remove all profile-related NavViewItems by populating a std::vector
// with the ones we want to keep.
// NOTE: menuItems.Remove() causes an out-of-bounds crash. Using ReplaceAll()
// gets around this crash.
std::vector<IInspectable> menuItemsSTL;
for (const auto& item : menuItems)
{
if (const auto& navViewItem{ item.try_as<MUX::Controls::NavigationViewItem>() })
{
if (const auto& tag{ navViewItem.Tag() })
{
if (tag.try_as<Editor::ProfileViewModel>())
{
// hide NavViewItem pointing to a Profile
navViewItem.Visibility(Visibility::Collapsed);
// don't add NavViewItem pointing to a Profile
continue;
}
else if (const auto stringTag{ tag.try_as<hstring>() })
else if (const auto& stringTag{ tag.try_as<hstring>() })
{
if (stringTag == addProfileTag)
{
// hide NavViewItem pointing to "Add Profile"
navViewItem.Visibility(Visibility::Collapsed);
// don't add the "Add Profile" item
continue;
}
}
}
}
menuItemsSTL.emplace_back(item);
}
_InitializeProfilesList();
menuItems.ReplaceAll(menuItemsSTL);

// Repopulate profile-related menu items
_InitializeProfilesList();
// Update the Nav State with the new version of the settings
_colorSchemesNavState.Globals(_settingsClone.GlobalSettings());
_profilesNavState.Schemes(_settingsClone.GlobalSettings().ColorSchemes());
// We'll update the profile in the _profilesNavState whenever we actually navigate to one

_RefreshCurrentPage();
}

void MainPage::_RefreshCurrentPage()
{
auto navigationMenu{ SettingsNav() };
if (const auto selectedItem{ navigationMenu.SelectedItem() })
// now that the menuItems are repopulated,
// refresh the current page using the SelectedItem data we collected before the refresh
if (selectedItemTag)
{
if (const auto tag{ selectedItem.as<MUX::Controls::NavigationViewItem>().Tag() })
const auto& selectedItemStringTag{ selectedItemTag.try_as<hstring>() };
const auto& selectedItemProfileTag{ selectedItemTag.try_as<ProfileViewModel>() };
for (const auto& item : menuItems)
{
if (const auto oldProfile{ tag.try_as<Editor::ProfileViewModel>() })
if (const auto& menuItem{ item.try_as<MUX::Controls::NavigationViewItem>() })
{
// check if the profile still exists
if (const auto profile{ _settingsClone.FindProfile(oldProfile.Guid()) })
if (const auto& tag{ menuItem.Tag() })
{
// Navigate to the page with the given profile
_Navigate(_viewModelForProfile(profile));
return;
if (const auto& stringTag{ tag.try_as<hstring>() })
{
if (stringTag == selectedItemStringTag)
{
// found the one that was selected before the refresh
SettingsNav().SelectedItem(item);
_Navigate(*stringTag);
co_return;
}
}
else if (const auto& profileTag{ tag.try_as<ProfileViewModel>() })
{
if (profileTag->Guid() == selectedItemProfileTag->Guid())
{
// found the one that was selected before the refresh
SettingsNav().SelectedItem(item);
_Navigate(*selectedItemProfileTag);
co_return;
}
}
}
}
else if (const auto stringTag{ tag.try_as<hstring>() })
{
// navigate to the page with this tag
_Navigate(*stringTag);
return;
}
}
}

// could not find the page we were on, fallback to first menu item
const auto firstItem{ navigationMenu.MenuItems().GetAt(0) };
navigationMenu.SelectedItem(firstItem);
if (const auto tag{ navigationMenu.SelectedItem().as<MUX::Controls::NavigationViewItem>().Tag() })
// couldn't find the selected item,
// fallback to first menu item
const auto& firstItem{ menuItems.GetAt(0) };
SettingsNav().SelectedItem(firstItem);
if (const auto& tag{ SettingsNav().SelectedItem().try_as<MUX::Controls::NavigationViewItem>().Tag() })
{
_Navigate(unbox_value<hstring>(tag));
}
Expand Down Expand Up @@ -199,6 +221,13 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
{
if (const auto clickedItemContainer = args.InvokedItemContainer())
{
if (clickedItemContainer.IsSelected())
{
// Clicked on the selected item.
// Don't navigate to the same page again.
return;
}

if (const auto navString = clickedItemContainer.Tag().try_as<hstring>())
{
if (navString == addProfileTag)
Expand Down Expand Up @@ -292,8 +321,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation

void MainPage::ResetButton_Click(IInspectable const& /*sender*/, RoutedEventArgs const& /*args*/)
{
_settingsClone = _settingsSource.Copy();
_RefreshCurrentPage();
UpdateSettings(_settingsSource);
}

void MainPage::_InitializeProfilesList()
Expand Down
1 change: 0 additions & 1 deletion src/cascadia/TerminalSettingsEditor/MainPage.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation

void _Navigate(hstring clickedItemTag);
void _Navigate(const Editor::ProfileViewModel& profile);
void _RefreshCurrentPage();

ColorSchemesPageNavigationState _colorSchemesNavState{ nullptr };
ProfilePageNavigationState _profilesNavState{ nullptr };
Expand Down

0 comments on commit 9fed14a

Please sign in to comment.