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

Fix CommandPalette to prefer inner interactions over bindings #9056

Merged
merged 4 commits into from
Feb 10, 2021
Merged

Fix CommandPalette to prefer inner interactions over bindings #9056

merged 4 commits into from
Feb 10, 2021

Conversation

Don-Vito
Copy link
Contributor

@Don-Vito Don-Vito commented Feb 5, 2021

PR Checklist

Detailed Description of the Pull Request / Additional comments

  • Currently TerminalPage registers on CmdPal key events:
    • To invoke bindings when the palette is open
    • Since some key combinations are not triggered by KeyDown
      it registers for PreviewKeyDown
  • As a result bindings might be preferred over navigation
    (e.g., ctrl+v will paste into Terminal rather than into search box)
  • To fix this, I moved all interactions inside the CmdPal into
    PreviewKeyDown as well
  • In addition, added specific handling for copy/paste
    which now allow to interact with search box even if not focused

@ghost ghost added Area-CmdPal Command Palette issues and features Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Terminal The new Windows Terminal. labels Feb 5, 2021
@DHowett DHowett added the zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. label Feb 5, 2021
@zadjii-msft zadjii-msft self-assigned this Feb 8, 2021
@Don-Vito
Copy link
Contributor Author

Don-Vito commented Feb 9, 2021

I should stop creating conflicts for myself

zadjii-msft
zadjii-msft approved these changes Feb 10, 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.

This seems like it works right. Thanks for helping clean this up!

@zadjii-msft zadjii-msft removed their assignment Feb 10, 2021
@zadjii-msft zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Feb 10, 2021
carlos-zamora
carlos-zamora requested changes Feb 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.

Want to have a discussion on a few things first.

src/cascadia/TerminalApp/CommandPalette.cpp
Comment on lines +339 to 348
else if (key == VirtualKey::C && ctrlDown)
{
_searchBox().CopySelectionToClipboard();
e.Handled(true);
}
else if (key == VirtualKey::V && ctrlDown)
{
_searchBox().PasteFromClipboard();
e.Handled(true);
}
Copy link
Member

Choose a reason for hiding this comment

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

Should we also hardcode some other text box default bindings like ctrl+a, ctrl+left/right, ctrl+x, and home/end? Or what makes ctrl+c and ctrl+v so special here?

Copy link
Member

Choose a reason for hiding this comment

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

Isn't there a way to ask the text box to handle the event first, and if it can't then we handle it? Or how does the order of handling work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Probably. I handled copy and paste explicitly as right now it is the same default binding for pasting in the terminal. Which means that the current behavior upon ctrl+v is to paste in the terminal.

src/cascadia/TerminalApp/CommandPalette.xaml
@@ -423,7 +422,6 @@ the MIT License. See LICENSE in the project root for license information. -->
AllowDrop="False"
IsItemClickEnabled="True"
ItemClick="_listItemClicked"
PreviewKeyDown="_keyDownHandler"
Copy link
Member

Choose a reason for hiding this comment

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

Just double-checking. So if we have focus on a list view item, any key stroke we make will go to the UserControl's PreviewKeyDown handler on line 18? And that'll properly handle navigating the list view with the arrow keys and home/end?

Copy link
Contributor Author

@Don-Vito Don-Vito Feb 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Yes. Actually Preview is going top down... and if the key is handled by the _keyDownHandler the propagation stops. In other words, this line was doing nothing functional (because if the handler handles the event, the propagation would stop on the palette level, and if the handler does not - there is no need to propagate) 😄

@ghost ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Feb 10, 2021
@DHowett
Copy link
Member

DHowett commented Feb 10, 2021

Aaah you conflicted yourself

DHowett
DHowett approved these changes Feb 10, 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 don't love that we need to manually handle these events -- it's only going to get worse, honestly, but... it works.

@Don-Vito
Copy link
Contributor Author

I don't love that we need to manually handle these events -- it's only going to get worse, honestly, but... it works.

I don't like it either. The main blocker is that ctrl+ events do not bubble up with KeyDown. This simply drives me mad.

@Don-Vito
Copy link
Contributor Author

Aaah you conflicted yourself

Story of my life.

@DHowett
Copy link
Member

DHowett commented Feb 10, 2021

do not bubble up

Indeed! They are "accelerators" 😄 which is a separate part of Xaml used to make keyboard shortcuts work.

@Don-Vito
Copy link
Contributor Author

Resolved the conflicts 💪

@DHowett DHowett merged commit ed19301 into microsoft:main Feb 10, 2021
DHowett pushed a commit that referenced this pull request Feb 10, 2021
@Don-Vito @DHowett
Fix CommandPalette to prefer inner interactions over bindings (#9056)
3ab1d72 
* Currently TerminalPage registers on CmdPal key events:
  * To invoke bindings when the palette is open
  * Since some key combinations are not triggered by KeyDown
    it registers for PreviewKeyDown
* As a result bindings might be preferred over navigation
  (e.g., ctrl+v will paste into Terminal rather than into search box)
* To fix this, I moved all interactions inside the CmdPal into
  PreviewKeyDown as well
* In addition, added specific handling for copy/paste
  which now allow to interact with search box even if not focused

Closes #9044

(cherry picked from commit ed19301)
@DHowett DHowett removed the zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. label Feb 10, 2021
@ghost
Copy link

ghost commented Feb 11, 2021

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

Handy links:

@ghost ghost mentioned this pull request Feb 11, 2021
Closed
@zadjii-msft zadjii-msft mentioned this pull request Feb 12, 2021
Closed
@KalleOlaviNiemitalo KalleOlaviNiemitalo mentioned this pull request Mar 17, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DHowett DHowett DHowett approved these changes

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

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

@miniksa miniksa Awaiting requested review from miniksa

@leonMSFT leonMSFT Awaiting requested review from leonMSFT

@PankajBhojwani PankajBhojwani Awaiting requested review from PankajBhojwani

Assignees
No one assigned
Labels
Area-CmdPal Command Palette issues and features Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Second It's a PR that needs another sign-off Priority-2 A description (P2) Product-Terminal The new Windows Terminal.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Don-Vito @DHowett @carlos-zamora @zadjii-msft