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

Settings UI: Navigating across profiles drags focused text setting #8836

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

Settings UI: Navigating across profiles drags focused text setting #8836

carlos-zamora opened this issue Jan 21, 2021 · 0 comments · Fixed by #8838
Assignees
@carlos-zamora
Labels
Area-Settings UI 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-Blocking We won't ship a release like this! No-siree.
Milestone
Terminal v1.6

Comments

@carlos-zamora
Copy link
Member

Steps to reproduce

  1. In Settings UI, navigate to a profile (Profile A)
  2. (assuming it's populated) click on the Icon text box
  3. Navigate to a different profile (Profile B) with a different icon

Expected behavior

Profile B should be unchanged.

Actual behavior

Profile B now has the same icon from Profile A.

Additional Comments

This kind of behavior occurs if you replace step 2 with clicking on any TextBox represented setting (i.e. tabTitle, startingDirectory, etc...).

This is a particularly weird bug. It's possible that this bug has been around for a while and we haven't noticed it.

CC @DHowett
@carlos-zamora carlos-zamora added Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Severity-Blocking We won't ship a release like this! No-siree. Priority-0 Bugs that we consider release-blocking/recall-class (P0) Area-Settings UI Anything specific to the SUI labels Jan 21, 2021
@carlos-zamora carlos-zamora added this to the Terminal v1.6 milestone Jan 21, 2021
@carlos-zamora carlos-zamora self-assigned this Jan 21, 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 21, 2021
@carlos-zamora carlos-zamora removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jan 21, 2021
@carlos-zamora carlos-zamora mentioned this issue Jan 21, 2021
Merged
@ghost ghost added the In-PR This issue has a related PR label Jan 21, 2021
@ghost ghost closed this as completed in #8838 Jan 21, 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 21, 2021
ghost pushed a commit that referenced this issue Jan 21, 2021
@carlos-zamora
Revert "Maintain current Pivot selection... (#8803)" (#8838)
b6f5f2a 
This reverts commit a7d7362 introduced by #8803.

Reverting this commit fixes #8836 at the expense of the profile page
remembering the last Pivot selection. The 

## References
#6800 - Settings UI Epic

#8803 maintained a `ProfilePageNavigationState` in `MainPage` to remember
the pivot position. However, the two-way binding on the TextBoxes
now seem to happen too late (after the navigation occurs),
resulting in the text being applied to the wrong profile.  In other
words, the sequence of events probably looks something like this:

1. user types text (_state.profile = old profile)
2. user moves to new profile
3. navigation completes (_state.profile = new profile)
4. textbox two-way binding fires, setting _state.profile.WHATEVER = value

## Validation Steps Performed
Performed repro sets from #8836. Bug no longer occurs. 

Reopens #8769
Closes #8836
mpela81 pushed a commit to mpela81/terminal that referenced this issue Jan 28, 2021
@carlos-zamora @mpela81
Revert "Maintain current Pivot selection... (microsoft#8803)" (micros…
e262e0c 
…oft#8838)

This reverts commit a7d7362 introduced by microsoft#8803.

Reverting this commit fixes microsoft#8836 at the expense of the profile page
remembering the last Pivot selection. The 

## References
microsoft#6800 - Settings UI Epic

microsoft#8803 maintained a `ProfilePageNavigationState` in `MainPage` to remember
the pivot position. However, the two-way binding on the TextBoxes
now seem to happen too late (after the navigation occurs),
resulting in the text being applied to the wrong profile.  In other
words, the sequence of events probably looks something like this:

1. user types text (_state.profile = old profile)
2. user moves to new profile
3. navigation completes (_state.profile = new profile)
4. textbox two-way binding fires, setting _state.profile.WHATEVER = value

## Validation Steps Performed
Performed repro sets from microsoft#8836. Bug no longer occurs. 

Reopens microsoft#8769
Closes microsoft#8836
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-Settings UI 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-Blocking We won't ship a release like this! No-siree.
Projects
None yet
Milestone
Terminal v1.6
Development

Successfully merging a pull request may close this issue.

Revert "Maintain current Pivot selection... (#8803)" microsoft/terminal
1 participant
@carlos-zamora