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.

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

Fix alt+space opening system menu and sending keys to terminal #10988

Merged
5 commits merged into from
Aug 24, 2021

Conversation

PankajBhojwani
Copy link
Contributor

@PankajBhojwani PankajBhojwani commented Aug 19, 2021

If both of the following are true

  1. alt+space is not explicitly unbound
  2. alt+space is not bound to a command

Then the window procedure will handle the alt+space to open up the context menu.
In this case, we need to make sure we don't send the keys to terminal.

Closes #10935

@ghost ghost added Area-Input Related to input processing (key presses, mouse, etc.) Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Severity-Blocking We won't ship a release like this! No-siree. labels Aug 19, 2021
@lhecker
Copy link
Member

lhecker commented Aug 19, 2021

Isn't this equal to having a default key binding for Alt+Space which is bound to a no-op action?

@DHowett
Copy link
Member

DHowett commented Aug 20, 2021

Here's a really silly thought. What if we just add a new "openSystemMenu" binding? We could be done with this whole issue and allow people to re-bind it to whatever they want! All we'd need to do is move _OpenSystemMenu to IslandWindow, hook up some events from AppHost/AppLogic/TerminalPage (ugh hopefully not all of them) and... we're off to the races! We can also do that work later and mark this part with a big TODO that links to a workitem that says what I just said above^

@DHowett
Copy link
Member

DHowett commented Aug 20, 2021

Perhaps there's a nod to #5833 somewhere in there... but that is a CONSOLE workitem. Hm.

@zadjii-msft
Copy link
Member

What if we just add a new "openSystemMenu" binding? We could be done with this whole issue and allow people to re-bind it to whatever they want!

that's a good idea

@PankajBhojwani
Copy link
Contributor Author

Here's a really silly thought. What if we just add a new "openSystemMenu" binding? We could be done with this whole issue and allow people to re-bind it to whatever they want! All we'd need to do is move _OpenSystemMenu to IslandWindow, hook up some events from AppHost/AppLogic/TerminalPage (ugh hopefully not all of them) and... we're off to the races! We can also do that work later and mark this part with a big TODO that links to a workitem that says what I just said above^

Sounds good to me! Added a work item for it for 1.12

@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Aug 23, 2021
@ghost
Copy link

ghost commented Aug 23, 2021

Hello @DHowett!

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.

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am okay with this solution -- it's targeted and specific, but it does rely on explicit unbinding. I don't love that, but it's better than the alternative.

Piping input directly into the terminal using one of the direct input listeners is always going to be fraught with peril.

@PankajBhojwani, alt+space works for the menu when you do this? Excellent.

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay yea lets do this for now, but openSystemMenu is definitely the better solution here long-term

@ghost ghost merged commit b113126 into main Aug 24, 2021
@ghost ghost deleted the dev/pabhoj/alt_space_fix branch August 24, 2021 14:07
ghost pushed a commit that referenced this pull request Sep 10, 2021
## Summary of the Pull Request
Basically undoes #10988  in favour of implementing it as described in #11018 

## PR Checklist
* [x] Closes #11018 
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [X] Tests added/passed
* [X] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx
* [X] Schema updated.
* [x] I work here

## Validation Steps Performed

- alt+space opens the system menu by default
- when alt+space is bound, the keys do not get send to terminal
- right-click on the tab bar didn't break (still opens system menu at the location of the cursor)
zadjii-msft pushed a commit that referenced this pull request Dec 1, 2021
This fixes a regression introduced in #10988 for the 1.11 release branch.
For later branches this issue was fixed in #11086.

## PR Checklist
* [x] Closes #11649
* [x] I work here
* [x] Tests added/passed

## Validation Steps Performed
* AltGr+Space produces "_" with the bépo keyboard layout
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Input Related to input processing (key presses, mouse, etc.) Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Severity-Blocking We won't ship a release like this! No-siree.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

alt+space displays system menu *and* sends key binding to Terminal
4 participants