Fix refresh-related crashes in Settings UI #8773

carlos-zamora · 2021-01-13T02:06:39Z

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)

Select the menu item
click "Discard changes"
Verify navigated to same page

Also performed repro steps for #8747 and #8748.

Closes #8747
Closes #8748

src/cascadia/TerminalSettingsEditor/MainPage.cpp

DHowett · 2021-01-13T22:43:35Z

src/cascadia/TerminalSettingsEditor/MainPage.cpp

+        const auto& firstItem{ menuItems.GetAt(0) };
        SettingsNav().SelectedItem(firstItem);
-        if (const auto tag{ SettingsNav().SelectedItem().as<MUX::Controls::NavigationViewItem>().Tag() })
+        if (const auto& tag{ SettingsNav().SelectedItem().try_as<MUX::Controls::NavigationViewItem>().Tag() })


don't go too crazy with this -- some of these things generate temporaries and we don't want to capture references to those. Those should not have the & if we can help it.

The ones you create from try_as, and the ones you create from GetAt, probably aren't safe to take references to. The ones you create from iteration over an iterator, however, are safe to take a reference to.

I apologize - i know i asked you to flip it one way and then the other, but... some nuance 😄

Ok. So just to get this straight, try_as and GetAt aren't safe to take refs of because MenuItems might change from underneath us (especially when looking at the selected item stuff). So it's safer to take a copy so that we can use it later. Right?

Ok. So just to get this straight, try_as and GetAt aren't safe to take refs of because MenuItems might change from underneath us (especially when looking at the selected item stuff). So it's safer to take a copy so that we can use it later. Right?

So, it's a little more complicated than that.

If this were just plain C++, that would be why. For a vector, at returns a reference to the memory location inside the vector's storage, which is durable until the vector changes.

For C++/WinRT, the story is a little different. Sorta. I still don't totally know the C++ reasons for why this works the way it does.

GetAt() returns a new object that was constructed to hold the internal WinRT object pointer. try_as does the same thing. Those objects are projected type wrappers, and they are constructed on return from the function. You can't take a reference to them, because they are not referenceable. They don't exist.

HOWEVER, I think I led you astray. I did some reading on Stack Overflow, and I finally understand it. What you had written before was completely legal, and now I understand why. You can safely revert the code that removed all the &s.

Here's the skinny, on Herb Sutter's blog.

C++ deliberately specifies that binding a temporary object to a reference to const on the stack lengthens the lifetime of the temporary to the lifetime of the reference itself, and thus avoids what would otherwise be a common dangling-reference error.

This reverts commit ced92a6.

This reverts commit 9fd1dfb.

DHowett

love it love it

DHowett · 2021-01-14T01:49:45Z

src/cascadia/TerminalSettingsEditor/MainPage.cpp

+                            if (profileTag->Guid() == selectedItemProfileTag->Guid())
+                            {
+                                // found the one that was selected before the refresh
+                                SettingsNav().SelectedItem(item);
+                                _Navigate(*selectedItemProfileTag);
+                                co_return;
+                            }
+                        }


hey how will this work when you have BASE LAYER selected?

Base Layer is unique in that it has a string tag. So that's covered in the other if-statement.

zadjii-msft · 2021-01-19T20:36:25Z

src/cascadia/TerminalSettingsEditor/MainPage.cpp

@@ -180,6 +203,13 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
    {
        if (const auto clickedItemContainer = args.InvokedItemContainer())
        {
+            if (clickedItemContainer.IsSelected())


zadjii-msft · 2021-01-19T20:36:48Z

src/cascadia/TerminalSettingsEditor/MainPage.cpp

+        // 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));


oh I see, yea this works because the first one is always a string. gotcha.

src/cascadia/TerminalSettingsEditor/MainPage.cpp

ghost · 2021-01-19T21:52:18Z

Hello @carlos-zamora!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

Upon a settings reload, we would select the correct navigation item for a profile, but navigate to the old one. As a result, you would navigate to the old page that points to a dead profile object. This would make it appear like you did not discard/save the changes. This bugfix navigates to the newly created profile, ensuring that your changes are actually applied to the settings model's clone in use. ## References #8773 - Introduced the bug #6800 - Settings UI Epic ## Validation Steps Performed - Navigate to "powershell" profile - edit "tab title" value - discard changes Before: changes would persist unless you discarded changes again Now: changes are discarded Also verified expected behavior occurs when you click "save" instead of "discard"

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

Upon a settings reload, we would select the correct navigation item for a profile, but navigate to the old one. As a result, you would navigate to the old page that points to a dead profile object. This would make it appear like you did not discard/save the changes. This bugfix navigates to the newly created profile, ensuring that your changes are actually applied to the settings model's clone in use. ## References microsoft#8773 - Introduced the bug microsoft#6800 - Settings UI Epic ## Validation Steps Performed - Navigate to "powershell" profile - edit "tab title" value - discard changes Before: changes would persist unless you discarded changes again Now: changes are discarded Also verified expected behavior occurs when you click "save" instead of "discard"

carlos-zamora added 2 commits January 12, 2021 17:35

Fix refresh-related crashes in Settings UI

0021214

Prevent navigation to already selected menu item

9283375

carlos-zamora added the Area-Settings UI Anything specific to the SUI label Jan 13, 2021

carlos-zamora requested a review from DHowett January 13, 2021 02:06

ghost added Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-0 Bugs that we consider release-blocking/recall-class (P0) Product-Terminal The new Windows Terminal. Severity-Crash Crashes are real bad news. labels Jan 13, 2021

DHowett requested changes Jan 13, 2021

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Jan 13, 2021

address PR comments + sprinkle & everywhere

774477e

carlos-zamora assigned DHowett Jan 13, 2021

DHowett requested changes Jan 13, 2021

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jan 13, 2021

remove excess sprinkles! (&)

9fd1dfb

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jan 13, 2021

carlos-zamora added 3 commits January 13, 2021 15:39

missed a few...

ced92a6

Revert "missed a few..."

3233b90

This reverts commit ced92a6.

Revert "remove excess sprinkles! (&)"

0a1d715

This reverts commit 9fd1dfb.

DHowett approved these changes Jan 14, 2021

View reviewed changes

DHowett reviewed Jan 14, 2021

View reviewed changes

carlos-zamora added the Needs-Second It's a PR that needs another sign-off label Jan 15, 2021

ghost requested review from zadjii-msft, miniksa, leonMSFT and PankajBhojwani January 15, 2021 18:19

carlos-zamora unassigned DHowett Jan 19, 2021

zadjii-msft approved these changes Jan 19, 2021

View reviewed changes

Merge branch 'main' into dev/cazamor/sui/bugfix-crash-del-added-prof

b65dede

carlos-zamora added AutoMerge Marked for automatic merge by the bot when requirements are met and removed Needs-Second It's a PR that needs another sign-off labels Jan 19, 2021

ghost merged commit 9fed14a into main Jan 19, 2021

ghost deleted the dev/cazamor/sui/bugfix-crash-del-added-prof branch January 19, 2021 22:14

carlos-zamora mentioned this pull request Jan 20, 2021

Bugfix: navigate to new profile ref on settings reload #8835

Merged

zadjii-msft mentioned this pull request May 5, 2022

Deleting a (last?) profile in the SUI sends the Terminal to a farm upstate #13017

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix refresh-related crashes in Settings UI #8773

Fix refresh-related crashes in Settings UI #8773

carlos-zamora commented Jan 13, 2021 •

edited by DHowett

Loading

DHowett Jan 13, 2021

DHowett Jan 13, 2021

DHowett Jan 13, 2021

carlos-zamora Jan 13, 2021

DHowett Jan 13, 2021

DHowett Jan 13, 2021

DHowett left a comment

DHowett Jan 14, 2021

carlos-zamora Jan 14, 2021

zadjii-msft Jan 19, 2021

zadjii-msft Jan 19, 2021

ghost commented Jan 19, 2021

Fix refresh-related crashes in Settings UI #8773

Fix refresh-related crashes in Settings UI #8773

Conversation

carlos-zamora commented Jan 13, 2021 • edited by DHowett Loading

Validation Steps Performed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jan 19, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

carlos-zamora commented Jan 13, 2021 •

edited by DHowett

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.