Rebuild the profile nav via MenuItemsSource; mitigate a crash #14630
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
added
Area-Settings
This comment has been minimized.
This comment has been minimized.
zadjii-msft
changed the title
carlos-zamora
approved these changes
Jan 11, 2023
ghost
requested review from
DHowett
requested changes
Jan 11, 2023
| @@ -527,7 +536,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation | |||
|
|
|||
| void MainPage::_InitializeProfilesList() | |||
| { | |||
| const auto menuItems = SettingsNav().MenuItems(); | |||
| const auto menuItems = SettingsNav().MenuItemsSource().try_as<Windows::Foundation::Collections::IVector<IInspectable>>(); | |||
There was a problem hiding this comment.
This introduces an ordering dependency, right? It seems like there is code that can run before MenuItemsSource is set and code that can only run after. Are we clear on what happens here if somehow it's not been set?
| @@ -555,6 +564,30 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation | |||
| menuItems.Append(addProfileItem); | |||
| } | |||
|
|
|||
| void MainPage::_SetupNavViewItems() | |||
There was a problem hiding this comment.
For clarity, I might recommend we be very specific about what this does.
_moveXAMLParsedNavItemsIntoItemSource
There was a problem hiding this comment.
In fact, we should make it a debug assert that it never fires twice
ghost
added
the
Needs-Author-Feedback
carlos-zamora
added a commit
that referenced
this pull request
Jan 14, 2023
…ce, to mitigate a crash
ghost
removed
the
Needs-Author-Feedback
microsoft-github-policy-service
bot
requested review from
carlos-zamora and
DHowett
lhecker
approved these changes
Mar 9, 2023
microsoft-github-policy-service
bot
removed
the
Needs-Second
DHowett
approved these changes
Mar 9, 2023
DHowett
changed the title
DHowett
pushed a commit
that referenced
this pull request
Mar 31, 2023
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
zadjii-msft
added a commit
that referenced
this pull request
Apr 17, 2023
* 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Directly manipulating the
NavigationView::MenuItemsvector is bad. If you do that, you're gonna get crashes, in WinUI code forMeasure. However, a WinUI PR (below) gave me an idea: ChangingNavigationView::MenuItemsSourcewill 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: