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

commandPalette keybinding should close the palette when it's open #6679

Closed
zadjii-msft opened this issue Jun 25, 2020 · 1 comment · Fixed by #8044 or #8586
Closed

commandPalette keybinding should close the palette when it's open #6679

zadjii-msft opened this issue Jun 25, 2020 · 1 comment · Fixed by #8044 or #8586
Labels
Area-CmdPal Command Palette issues and features Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Task It's a feature request, but it doesn't really need a major design. Priority-2 A description (P2) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
Terminal v2.0

Comments

@zadjii-msft
Copy link
Member

This is being moved from discussion in #6635.

We wanted to enable the commandPalette keybinding to also work within the palette itself.

carlos-zamora

I know I suggested this, but it unveiled a huge problem. When paste is bound to ctrl+v, we paste to the underlying terminal twice. A similarly weird problem occurs with copy.

Maybe we should only detect the commandPalette action and use that as an additional way to dismiss it.

DHowett

Don't we just need to mark it Handled here? lol

zadjii-msft

This is shockingly bad actually.

  • If we just mark it as Handled(true), then the following KeyUp will still land in the TermControl, resulting in ^VPasted Text being written to the Terminal.
  • If we change the keybinding handler to on KeyUp, then it's likely that the first keyup it'll see is the keybinding for toggle the palette, which will immediately close it.

I'm going to move this to a follow-up issue, to get at the least the toggle the command palette action back, and maybe the full keybindings

[3:29 PM] Mike Griese
I'll file a follow-up - maybe we could do something where the action is selected on the keydown, but then only dispatched on the keyup?
​> [3:30 PM] Mike Griese
so we'll only try a keybinding on a keyup after a non-modifier keydown
@ghost ghost 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 Jun 25, 2020
@zadjii-msft zadjii-msft added Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Jun 25, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jun 25, 2020
@zadjii-msft zadjii-msft added the Priority-2 A description (P2) label Jun 25, 2020
@zadjii-msft zadjii-msft added this to the Terminal v2.0 milestone Jun 25, 2020
This was referenced Jun 25, 2020
Open
Merged
@DHowett DHowett removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jun 25, 2020
@zadjii-msft zadjii-msft mentioned this issue Oct 13, 2020
Closed
@zadjii-msft zadjii-msft added the Area-CmdPal Command Palette issues and features label Dec 1, 2020
This was referenced Dec 2, 2020
Closed
Merged
@ghost ghost added the In-PR This issue has a related PR label Dec 3, 2020
@zadjii-msft zadjii-msft mentioned this issue Dec 3, 2020
Closed
@ghost ghost closed this as completed in #8044 Dec 14, 2020
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Dec 14, 2020
ghost pushed a commit that referenced this issue Dec 14, 2020
@kaihugo
Enable shortcut while CommandPalette is open (#8044)
1965fb5 
<!-- 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
It is maybe not the best way since I had to get all the cases for key handling so I just created for each of them. As a result the code get longer(not optimized). Most difficult thing was Next tab and Previous tab I just could not solve it.

### 9 commands that couldn't enabled > <
     1. Increase font size               -> could not find VirtualKey for "-"
     2. Decrease font size             -> could not find VirtualKey for "+"
     3. Split pane, split:horizontal -> could not find VirtualKey for "-"
     4. Split pane, split:vertical     -> could not find VirtualKey for "+"
     5. Open default settings        -> could not find VirtualKey for ","
     6. Open settings file               -> could not find VirtualKey for ","
     7. Open new tab dropdown    -> could not resolve keybindings
     8. Next tab                               -> could not resolve keybindings
     9. Previous tab                        -> could not resolve keybindings
 
<!-- Other than the issue solved, is this relevant to any other issues/existing PRs? --> 
## References

<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [x] Closes #6679
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [ ] Tests added/passed
* [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx
* [ ] Schema updated.
* [ ] 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: #xxx

<!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here -->
## Detailed Description of the Pull Request / Additional comments

<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed
@DHowett DHowett reopened this Dec 14, 2020
@DHowett
Copy link
Member

DHowett commented Dec 14, 2020

Reopened -- reverted PR

@ghost ghost mentioned this issue Dec 14, 2020
Merged
@ghost ghost added In-PR This issue has a related PR and removed Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. labels Dec 14, 2020
@ghost ghost closed this as completed in #8586 Dec 18, 2020
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Dec 18, 2020
ghost pushed a commit that referenced this issue Dec 18, 2020
@kaihugo
Enable shortcut while CommandPalette is open (#8586)
01a0490 
This commit introduces direct shortcut dispatch to TerminalPage, which
allows it to respond to key bindings before the command palette.

This allows the user to use shortcuts from the command palette while
it's open.

Closes #6679
@zadjii-msft zadjii-msft mentioned this issue Jan 3, 2021
Closed
mpela81 pushed a commit to mpela81/terminal that referenced this issue Jan 28, 2021
@kaihugo @mpela81
Enable shortcut while CommandPalette is open (microsoft#8044)
4c82a7c 
<!-- 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
It is maybe not the best way since I had to get all the cases for key handling so I just created for each of them. As a result the code get longer(not optimized). Most difficult thing was Next tab and Previous tab I just could not solve it.

### 9 commands that couldn't enabled > <
     1. Increase font size               -> could not find VirtualKey for "-"
     2. Decrease font size             -> could not find VirtualKey for "+"
     3. Split pane, split:horizontal -> could not find VirtualKey for "-"
     4. Split pane, split:vertical     -> could not find VirtualKey for "+"
     5. Open default settings        -> could not find VirtualKey for ","
     6. Open settings file               -> could not find VirtualKey for ","
     7. Open new tab dropdown    -> could not resolve keybindings
     8. Next tab                               -> could not resolve keybindings
     9. Previous tab                        -> could not resolve keybindings
 
<!-- Other than the issue solved, is this relevant to any other issues/existing PRs? --> 
## References

<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [x] Closes microsoft#6679
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [ ] Tests added/passed
* [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx
* [ ] Schema updated.
* [ ] 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: #xxx

<!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here -->
## Detailed Description of the Pull Request / Additional comments

<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed
mpela81 pushed a commit to mpela81/terminal that referenced this issue Jan 28, 2021
@kaihugo @mpela81
Enable shortcut while CommandPalette is open (microsoft#8586)
65db5b1 
This commit introduces direct shortcut dispatch to TerminalPage, which
allows it to respond to key bindings before the command palette.

This allows the user to use shortcuts from the command palette while
it's open.

Closes microsoft#6679
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area-CmdPal Command Palette issues and features Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Task It's a feature request, but it doesn't really need a major design. Priority-2 A description (P2) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
Terminal v2.0
Development

Successfully merging a pull request may close this issue.

Enable shortcut while CommandPalette is open
Enable shortcut while CommandPalette is open
2 participants
@DHowett @zadjii-msft