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

The "Overwrite key binding" warning is very broken #17754

Closed
zadjii-msft opened this issue Aug 20, 2024 · 6 comments · Fixed by #17763
Closed

The "Overwrite key binding" warning is very broken #17754

zadjii-msft opened this issue Aug 20, 2024 · 6 comments · Fixed by #17763
Assignees
@PankajBhojwani
Labels
Area-Settings UI Anything specific to the SUI In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal.
Milestone
Terminal v1.22

Comments

@zadjii-msft
Copy link
Member

(from bug bash 1.22)

Action Editor
🛑 "Overwrite key binding" warning is very broken

image
(thats right, I had to take a screenshot of the image in Loop, thanks Loop)

I can't say "no". Dismissing it is like saying "yes". Also it scrolls horizontally.
@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Aug 20, 2024
@zadjii-msft
Copy link
Member Author

/cc @PankajBhojwani since I believe you were the last one to touch Actions

@zadjii-msft zadjii-msft added this to the Terminal v1.22 milestone Aug 20, 2024
@zadjii-msft zadjii-msft added the Area-Settings UI Anything specific to the SUI label Aug 20, 2024
@PankajBhojwani
Copy link
Contributor

I'm somewhat certain this is the way it worked before my changes as well (I see this same behaviour in Stable)

@DHowett
Copy link
Member

DHowett commented Aug 21, 2024

I see this same behaviour in Stable

I do not. Dismissing the flyout does not "confirm" and overwrite the binding in Stable.

@zadjii-msft
Copy link
Member Author

Dismissing the flyout does confirm for me in 1.21. So maybe it regressed in 20->21?

@zadjii-msft
Copy link
Member Author

I wonder if it's something more subtle in XAML somewhere. Cause like, on 1.20, when you hit the checkbox button, the action stays in the "editing" state, with the textbox and the buttons. But in 1.21, the button immediately exits the editing state, then pops the "are you sure"?

@DHowett
Copy link
Member

DHowett commented Aug 21, 2024

Much more likely that it was from #14292, which rewrote the action view model (and it should be the viewmodel that keeps track of states like this.)

@PankajBhojwani PankajBhojwani mentioned this issue Aug 21, 2024
Merged
4 tasks
@microsoft-github-policy-service microsoft-github-policy-service bot added the In-PR This issue has a related PR label Aug 21, 2024
@carlos-zamora carlos-zamora added Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Aug 21, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label Aug 21, 2024
DHowett pushed a commit that referenced this issue Aug 21, 2024
@PankajBhojwani @DHowett
Fix overwrite key binding warning in the SUI (#17763)
cb8a3f6 
Fixes a regression from the actions MVVM change in #14292 - attempting
to overwrite a keybinding was displaying a warning but propagating the
change before the user acknowledged it.

The overwrite key binding warning in the SUI works like before

Closes #17754

(cherry picked from commit ce92b18)
Service-Card-Id: PVTI_lADOAF3p4s4AmhmszgSF01Y
Service-Version: 1.21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@PankajBhojwani PankajBhojwani

Labels
Area-Settings UI Anything specific to the SUI In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal.
Projects
1.21 Servicing Pipeline
Status: Shipped
Milestone
Terminal v1.22
Development

Successfully merging a pull request may close this issue.

Fix overwrite key binding warning in the SUI microsoft/terminal
4 participants
@DHowett @carlos-zamora @zadjii-msft @PankajBhojwani