Skip to content

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

Merged
5 commits merged intomainfrom
dev/pabhoj/alt_space_fix
Aug 24, 2021
Merged

Fix alt+space opening system menu and sending keys to terminal#10988
5 commits merged intomainfrom
dev/pabhoj/alt_space_fix

Conversation

@PankajBhojwani
Copy link
Contributor

@PankajBhojwani PankajBhojwani commented Aug 19, 2021 •
edited by DHowett
Loading

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?

lhecker
lhecker reviewed Aug 19, 2021
View reviewed changes
@DHowett
Copy link
Member

DHowett commented Aug 20, 2021 •
edited
Loading

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 •
edited
Loading

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

@zadjii-msft
Copy link
Member

zadjii-msft commented Aug 20, 2021

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 PankajBhojwani mentioned this pull request Aug 23, 2021
Closed
@PankajBhojwani
Copy link
Contributor Author

PankajBhojwani commented Aug 23, 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^

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.

DHowett
DHowett reviewed Aug 23, 2021
View reviewed changes
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.

zadjii-msft
zadjii-msft approved these changes Aug 24, 2021
View reviewed changes
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
@PankajBhojwani PankajBhojwani mentioned this pull request Aug 30, 2021
Merged
6 tasks
ghost pushed a commit that referenced this pull request Sep 10, 2021
@PankajBhojwani
Add an openSystemMenu keybinding (#11086)
97722d3 
## 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)
This was referenced Nov 27, 2021
Merged
Closed
zadjii-msft pushed a commit that referenced this pull request Dec 1, 2021
@lhecker
Fix AltGr+Space not working (#11832)
2bd3768 
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

Reviewers

@DHowett DHowett DHowett left review comments

@lhecker lhecker lhecker approved these changes

@zadjii-msft zadjii-msft zadjii-msft approved these changes

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

4 participants

@PankajBhojwani @lhecker @DHowett @zadjii-msft