From 9fed14a95e9a3cbf437740983e21c725cf7bde6e Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Tue, 19 Jan 2021 14:14:07 -0800 Subject: [PATCH] Fix refresh-related crashes in Settings UI (#8773) 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 #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 #8747 and #8748. Closes #8747 Closes #8748 --- .../TerminalSettingsEditor/MainPage.cpp | 108 +++++++++++------- .../TerminalSettingsEditor/MainPage.h | 1 - 2 files changed, 68 insertions(+), 41 deletions(-) diff --git a/src/cascadia/TerminalSettingsEditor/MainPage.cpp b/src/cascadia/TerminalSettingsEditor/MainPage.cpp index ae765856a57..5cb19741a61 100644 --- a/src/cascadia/TerminalSettingsEditor/MainPage.cpp +++ b/src/cascadia/TerminalSettingsEditor/MainPage.cpp @@ -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() }) + if (const auto& navViewItem{ selectedItem.try_as() }) { - 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 menuItemsSTL; + for (const auto& item : menuItems) + { + if (const auto& navViewItem{ item.try_as() }) + { + if (const auto& tag{ navViewItem.Tag() }) { if (tag.try_as()) { - // 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() }) + else if (const auto& stringTag{ tag.try_as() }) { 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().Tag() }) + const auto& selectedItemStringTag{ selectedItemTag.try_as() }; + const auto& selectedItemProfileTag{ selectedItemTag.try_as() }; + for (const auto& item : menuItems) { - if (const auto oldProfile{ tag.try_as() }) + if (const auto& menuItem{ item.try_as() }) { - // 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() }) + { + 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() }) + { + 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() }) - { - // 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().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().Tag() }) { _Navigate(unbox_value(tag)); } @@ -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()) { if (navString == addProfileTag) @@ -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() diff --git a/src/cascadia/TerminalSettingsEditor/MainPage.h b/src/cascadia/TerminalSettingsEditor/MainPage.h index a89ea9a333a..d977b6320ea 100644 --- a/src/cascadia/TerminalSettingsEditor/MainPage.h +++ b/src/cascadia/TerminalSettingsEditor/MainPage.h @@ -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 };