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

Do not dismiss selection if the Windows keys is pressed as a key-combination #9163

Merged

Conversation

imaginary-person
Copy link
Contributor

@imaginary-person imaginary-person commented Feb 14, 2021

Aims to fix #8791.

Summary of the Pull Request

Prior to this PR, if the Windows key was pressed as a part of a key combination, then selection was being dismissed. For example, when a user pressed Windows + Shift + S keys to invoke the Capture & Annotate tool.
This PR adds an exception for not clearing selection when either of the two Windows keys are pressed as part of a key combination.
It was tested manually by trying to reproduce the issue.

PR Checklist

Validation Steps Performed

  1. Build Terminal.
  2. Write anything & make a selection.
  3. Press Windows+ Shift + S keys.
  4. The Capture & Annotate tool appears but the selection made in step 2 isn't dismissed (doesn't disappear).

Get changes from main repo
microsoft#8791. Don't dismiss selection if Windows key was also pressed.
Remove trailing whitespace
@msftbot msftbot bot added 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-3 A description (P3) Product-Terminal The new Windows Terminal. labels Feb 14, 2021
Clear whitespace in empty line
Revise code for better abstraction & readability
@imaginary-person imaginary-person changed the title Win button do not dismiss selection Do not dismiss selection if the Windows keys is pressed as a key-combination Feb 14, 2021
Copy link
Contributor

@Don-Vito Don-Vito left a comment

Choose a reason for hiding this comment

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

Not an expert, but besides a nit looks good to me! 😊

Probably, not as a part of this PR, we should also prevent the winkey combinations from scrolling (inside Terminal::SendKeyEvent)

src/cascadia/TerminalControl/TermControl.cpp Outdated Show resolved Hide resolved
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 AutoMerge Marked for automatic merge by the bot when requirements are met zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. zStable-Service-Queued-1.12 A floating label that tracks the current Stable version for servicing purposes. labels Feb 17, 2021
@msftbot
Copy link
Contributor

msftbot bot commented Feb 17, 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.

@msftbot msftbot bot merged commit 7d37ba2 into microsoft:main Feb 17, 2021
@DHowett DHowett removed zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. zStable-Service-Queued-1.12 A floating label that tracks the current Stable version for servicing purposes. labels Feb 23, 2021
DHowett pushed a commit that referenced this pull request Feb 23, 2021
…ination (#9163)

Aims to fix #8791.

<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? -->
## Summary of the Pull Request
Prior to this PR, if the Windows key was pressed as a part of a key combination, then selection was being dismissed. For example,  when a user pressed `Windows` + `Shift` + `S` keys to invoke the _Capture & Annotate_ tool.
This PR adds an exception for not clearing selection when either of the two Windows keys are pressed as part of a key combination.
It was tested manually by trying to reproduce the issue.

<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [ ] Closes #8791
* [x ] CLA signed.
* [x ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #8791

<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed
1. Build Terminal.
2. Write anything & make a selection.
3. Press `Windows`+ `Shift` + `S` keys.
4. The _Capture & Annotate_ tool appears but the selection made in step 2 isn't dismissed (doesn't disappear).

(cherry picked from commit 7d37ba2)
@msftbot
Copy link
Contributor

msftbot bot commented Mar 1, 2021

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

Handy links:

@msftbot
Copy link
Contributor

msftbot bot commented Mar 1, 2021

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

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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-3 A description (P3) Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Win+Shift+S dismisses the selection
6 participants