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

Add an openSystemMenu keybinding #11086

Merged
4 commits merged into from
Sep 10, 2021
Merged

Add an openSystemMenu keybinding #11086

4 commits merged into from
Sep 10, 2021

Conversation

PankajBhojwani
Copy link
Contributor

@PankajBhojwani PankajBhojwani commented Aug 30, 2021 •
edited by carlos-zamora
Loading

Summary of the Pull Request

Basically undoes #10988 in favour of implementing it as described in #11018

PR Checklist

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)

@ghost ghost added Area-Input Related to input processing (key presses, mouse, etc.) Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Aug 30, 2021
zadjii-msft
zadjii-msft approved these changes Sep 8, 2021
View reviewed changes
src/cascadia/WindowsTerminal/main.cpp
Comment on lines +192 to +193
host.OnDirectKeyEvent(VK_SPACE, LOBYTE(HIWORD(message.lParam)), true);
continue;
Copy link
Member

Choose a reason for hiding this comment

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

huh, why don't we need the if() here anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We want to always consume the alt+space since this is now a default keybinding (so if the host did not deal with it, it means it was explcitly unbound and we still don't want system XAML to get it)

@zadjii-msft zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Sep 9, 2021
carlos-zamora
carlos-zamora requested changes Sep 10, 2021
View reviewed changes
Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

Just update the schema and the docs and we're good.

Also, just wanna double check, if you unbind alt+space, it gets sent straight to the terminal right? The system menu doesn't pop up?

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 10, 2021
@PankajBhojwani PankajBhojwani mentioned this pull request Sep 10, 2021
Merged
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 10, 2021
@PankajBhojwani
Copy link
Contributor Author

Just update the schema and the docs and we're good.

thanks for the reminder, done!

Also, just wanna double check, if you unbind alt+space, it gets sent straight to the terminal right? The system menu doesn't pop up?

Yep! The menu doesn't pop up and the terminal gets the alt+space (so the end result is it looks like you just typed space)

carlos-zamora
carlos-zamora approved these changes Sep 10, 2021
View reviewed changes
Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

Thanks!

@carlos-zamora carlos-zamora added the AutoMerge Marked for automatic merge by the bot when requirements are met label Sep 10, 2021
@ghost
Copy link

ghost commented Sep 10, 2021

Hello @carlos-zamora!

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.

@ghost ghost merged commit 97722d3 into main Sep 10, 2021
@ghost ghost deleted the dev/pabhoj/alt_space branch September 10, 2021 18:25
@ghost
Copy link

ghost commented Oct 20, 2021

🎉Windows Terminal Preview v1.12.2922.0 has been released which incorporates this pull request.:tada:

Handy links:

@ghost ghost mentioned this pull request Oct 20, 2021
Closed
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

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

@carlos-zamora carlos-zamora carlos-zamora approved these changes

@miniksa miniksa Awaiting requested review from miniksa

@DHowett DHowett Awaiting requested review from DHowett

@leonMSFT leonMSFT Awaiting requested review from leonMSFT

@lhecker lhecker Awaiting requested review from lhecker

Assignees
No one assigned
Labels
Area-Input Related to input processing (key presses, mouse, etc.) AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Second It's a PR that needs another sign-off Product-Terminal The new Windows Terminal.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add an "openSystemMenu" keybinding
3 participants
@PankajBhojwani @carlos-zamora @zadjii-msft