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

Jump to bottom

Crash when deleting a profile you just added #8747

Closed
carlos-zamora opened this issue Jan 12, 2021 · 0 comments · Fixed by #8773
Closed

Crash when deleting a profile you just added #8747

carlos-zamora opened this issue Jan 12, 2021 · 0 comments · Fixed by #8773
Assignees
@carlos-zamora
Labels
Area-SettingsUI Anything specific to the SUI 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. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. Severity-Crash Crashes are real bad news.
Milestone
Terminal v1.6

Comments

@carlos-zamora
Copy link
Member

Steps to reproduce

  1. Create a new profile - apply.
  2. Delete the profile without changing the selected profile
  • Here, the terminal you deleted the profile in hangs, before you even have a chance to apply the changes
  • The crash seems like its in XAML so not sure what happened

Expected behavior

The selected profile is deleted and another profile is selected.

Actual behavior

Crash
@carlos-zamora carlos-zamora added Issue-Bug It either shouldn't be doing this or needs an investigation. Severity-Crash Crashes are real bad news. Product-Terminal The new Windows Terminal. Priority-0 Bugs that we consider release-blocking/recall-class (P0) Area-SettingsUI Anything specific to the SUI labels Jan 12, 2021
@carlos-zamora carlos-zamora added this to the Terminal v1.6 milestone Jan 12, 2021
@ghost ghost added the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jan 12, 2021
@carlos-zamora carlos-zamora mentioned this issue Jan 12, 2021
Open
25 tasks
@zadjii-msft zadjii-msft removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jan 12, 2021
@carlos-zamora carlos-zamora self-assigned this Jan 12, 2021
@carlos-zamora carlos-zamora mentioned this issue Jan 13, 2021
Merged
@ghost ghost added the In-PR This issue has a related PR label Jan 13, 2021
@ghost ghost closed this as completed in #8773 Jan 19, 2021
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Jan 19, 2021
ghost pushed a commit that referenced this issue Jan 19, 2021
@carlos-zamora
Fix refresh-related crashes in Settings UI (#8773)
9fed14a 
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
mpela81 pushed a commit to mpela81/terminal that referenced this issue Jan 28, 2021
@carlos-zamora @mpela81
Fix refresh-related crashes in Settings UI (microsoft#8773)
cf76bb0 
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
@zadjii-msft zadjii-msft mentioned this issue May 5, 2022
Closed
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@carlos-zamora carlos-zamora

Labels
Area-SettingsUI Anything specific to the SUI 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. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. Severity-Crash Crashes are real bad news.
Projects
None yet
Milestone
Terminal v1.6
Development

Successfully merging a pull request may close this issue.

Fix refresh-related crashes in Settings UI microsoft/terminal
2 participants
@carlos-zamora @zadjii-msft