Implement keybindings to quick edit selection by carlos-zamora · Pull Request #3758 · microsoft/terminal

Carlos Zamora (carlos-zamora) · 2019-11-28T00:44:20Z

Summary of the Pull Request

Adds the moveSelectionPoint keybinding, which allows you to modify an existing selection using the keyboard. It has the following parameters:

direction: up | down | left | right
expansionMode: cell | word | viewport | buffer

What's next for keyboard selection:

select entire buffer keybinding (easy)
mark mode
- blocked by approval of spec Keyboard Selection Spec #2840

References

Spec: #2840
#715 (does not close, but does most of the work for it)

This is going to work really cool with #2989

PR Checklist

Detailed Description of the Pull Request / Additional comments

The enum Direction got moved from TermApp to TerminalSettings. This exposes the enum everywhere and makes it easier to pass it down to the terminal control.

The design is pretty straightforward, actually. It's just a switch-case statement for the SelectionExpansionMode (expansionMode). That just calls the appropriate function, which is another switch-case statement for the Direction (direction).

The enum SelectionTarget was added in preparation for Mark Mode.

Validation Steps Performed

Currently just did tests on my own. Unit tests for selection will be added.

Add first draft of functionality for updating selection

# Conflicts: # src/cascadia/TerminalApp/ActionArgs.cpp # src/cascadia/TerminalApp/ActionArgs.h # src/cascadia/TerminalApp/ActionArgs.idl # src/cascadia/TerminalApp/AppKeyBindingsSerialization.cpp # src/cascadia/TerminalApp/Pane.h # src/cascadia/TerminalApp/ShortcutActionDispatch.h # src/cascadia/TerminalApp/TerminalPage.h

Mike Griese (zadjii-msft)

I'd approve this, but I think the Direction/SelectionExpansionMode enums could be unified. If it can't be, I'll approve this

Deleted user (ghost) · 2020-01-10T21:06:52Z

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment.

Michael Niksa (miniksa) · 2020-01-10T21:19:55Z

Chill, msftbot. Carlos Zamora (@carlos-zamora) is still out until Monday.

Deleted user (ghost) · 2020-01-23T22:01:12Z

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment.

Carlos Zamora (carlos-zamora) · 2020-01-24T21:09:55Z

I'm away from my dev box at the moment, so no surprise the build is failing. I'll actually fix things when I get back to the office. For now just making progress on the PR fixes and making sure this PR doesn't get auto-closed by the bot.

(I'm sure I have more to do but this is a shot in the dark at progress haha)

Carlos Zamora (carlos-zamora) · 2020-01-28T23:40:07Z

What's left:

Update schema properly
Bug:
- Repro:
  - double click on word
  - move selection anchor with keyboard by one cell onto next word
  - next word is selected
- Expected:
  - next cell is selected
- EDIT: This was fixed in one of the refactors leading up to v1.
Terminal Core Testing

Carlos Zamora (carlos-zamora) · 2020-07-01T20:26:57Z

After updating the branch, ran into a huge problem. It's similar to #6431. When a MoveSelectionPoint keybinding comes in, the key down event is handled properly and runs the logic for moving the selection point. The problem is, the key up event is not handled by the keybinding, but it's still sent in Win32 Input Mode (#6309). This results in the selection being cleared on key up...

I'm not too sure on what the right move here is. Maybe we need to keep track of which key down events are handled as keybindings, and suppress the following key up event?

Carlos Zamora (carlos-zamora) · 2020-07-15T20:39:48Z

Offline discussion w/ Mike Griese (@zadjii-msft) to get around the win32 input mode issue:

if the key chord triggered a keybinding action, suppress the key up event
to do this, in TermControl, if _TryHandleKeyBinding was successful, cache the pressed key chord, and suppress the upcoming key up event for this key chord

That'll be done in the upcoming commit

Ofek Lev (ofek) · 2020-12-01T17:58:34Z

Other than a rebase, are there any other blockers for this?

Carlos Zamora (carlos-zamora) · 2020-12-01T18:06:55Z

Other than a rebase, are there any other blockers for this?

Aside from the rebase, the main blocker was that the selection would be cleared on a KeyUp event (obviously something we don't want haha). Since it's been open for so long, this PR could probably use a bit of polish as a part of the rebase? Not sure.

Mike Griese (zadjii-msft) · 2020-12-01T18:08:01Z

I'm pretty sure it's just blocked on the bureaucratic approval of the spec (#2840), and us having enough time to actually loop back on this one. We'd need to double-check that this matches the latest state of the spec. We've mostly been bogged down in other areas of the project. I too wish we had the engineering resources to finish this one off, but alas 🤷‍♂️ beyond just finishing merging this, we'd need to have someone ready to handle issues that might come up during selfhost before release, and we're all-hands-on-other-decks at the moment 😕

@carlos-zamora

This PR is a resurrection of #8522. @Hegunumo has apparently deleted their account, but the contribution was still valuable. I'm just here to get it across the finish line. This PR adds new action for navigating to the next & previous search results. These actions are unbound by default. These actions can be used from directly within the search dialog also, to immediately navigate the results. Furthermore, if you have a search started, and close the search box, then press this keybinding, _it will still perform the search_. So you can just hit <kbd>F3</kbd> repeatedly with the dialog closed to keep searching new results. Neat! If you dispatch the action on the key down, then dismiss a selection on a key up, we'll end up immediately destroying the selection when you release the bound key. That's annoying. It also bothers @carlos-zamora in #3758. However, I _think_ we can just only dismiss the selection on a key up. I _think_ that's fine. It _seems_ fine so far. We've got an entire release cycle to futz with it. ## Validation Steps Performed I've played with it all day and it seems _crisp_. Closes #7695 Co-authored-by: Kiminori Kaburagi <yukawa_hidenori@icloud.com>

Ofek Lev (ofek) · 2021-03-18T18:21:22Z

Just FYI this still needs a rebase, and is becoming increasingly out of sync

Mike Griese (zadjii-msft) · 2021-03-18T19:38:10Z

Yep, it sure is - we're still bogged down with some other priorities. I'll leave it up to Carlos Zamora (@carlos-zamora) if he wants to just leave this open for now, or close and re-open when we come back to this (in a few months probably)

Carlos Zamora (carlos-zamora) · 2021-03-18T20:09:21Z

Yep, it sure is - we're still bogged down with some other priorities. I'll leave it up to Carlos Zamora (@carlos-zamora) if he wants to just leave this open for now, or close and re-open when we come back to this (in a few months probably)

I'm just gonna outright close this. We need to revisit the specs anyways.

Carlos Zamora added 6 commits November 22, 2019 17:00

Move enums from TermApp to TermSettings

2844434

Add first draft of functionality for updating selection

Update selection anchors properly

a839102

Add scroll functionality

f70a36b

merge master

7de8df1

code format

8c2914b

Mike Griese (zadjii-msft) added Area-Settings Issues related to settings and customizability, for console or terminal Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Product-Terminal The new Windows Terminal. labels Dec 16, 2019

Mike Griese (zadjii-msft) requested changes Jan 3, 2020

View reviewed changes

Comment thread src/cascadia/TerminalApp/ActionArgs.h Resolved

Comment thread src/cascadia/TerminalCore/Terminal.hpp Resolved

Comment thread src/cascadia/TerminalControl/TermControl.cpp Resolved

Comment thread src/cascadia/TerminalCore/TerminalSelection.cpp Resolved

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jan 3, 2020

Mike Griese (zadjii-msft) requested review from Dustin L. Howett (MSFT) (DHowett-MSFT), Leon Liang (leonMSFT) and Michael Niksa (miniksa) January 3, 2020 20:53

ghost added the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Jan 10, 2020

Michael Niksa (miniksa) removed the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Jan 10, 2020

Mike Griese (zadjii-msft) added this to the Terminal v0.9 milestone Jan 16, 2020

ghost added the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Jan 23, 2020

Resolve merge conflicts

f7f2bbe

ghost removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. labels Jan 24, 2020

Carlos Zamora (carlos-zamora) and others added 4 commits January 24, 2020 13:44

PR Requests

d29d016

(I'm sure I have more to do but this is a shot in the dark at progress haha)

Update SelectionTarget to SelectionAnchorTarget

69bb47c

let's try that again...

8d21643

Consolidate enums

de8d201

Carlos Zamora added 2 commits July 1, 2020 10:57

Merge branch 'master' into dev/cazamor/keyboard-selection

40ecf3a

de-duplicate settings enums

9454967

Kayla Cinnamon (cinnamon-msft) modified the milestones: Terminal v1.2, Terminal v1.3 Jul 13, 2020

Mike Griese (zadjii-msft) modified the milestones: Terminal v1.3, Terminal v1.4 Aug 25, 2020

Mike Griese (zadjii-msft) assigned Carlos Zamora (carlos-zamora) Aug 25, 2020

Kayla Cinnamon (cinnamon-msft) modified the milestones: Terminal v1.4, Terminal v1.5 Sep 24, 2020

microsoft-github-updates Bot changed the base branch from master to main October 22, 2020 00:28

Mike Griese (zadjii-msft) mentioned this pull request Nov 9, 2020

Terminal: Support keyboard text selection & navigation #715

Closed

Kayla Cinnamon (cinnamon-msft) modified the milestones: Terminal v1.5, Terminal v1.6 Nov 12, 2020

Mike Griese (zadjii-msft) mentioned this pull request Dec 15, 2020

Add Select all shortcut action #8557

Closed

2 of 6 tasks

This was referenced Jan 27, 2021

Add NextSearchMatch and PreviouSearchMatch actions #8522

Closed

Add findNext, findPrev actions #8917

Merged

Kayla Cinnamon (cinnamon-msft) modified the milestones: Terminal v1.6, Terminal v1.7 Jan 28, 2021

Mike Griese (zadjii-msft) modified the milestones: Terminal v1.7, Terminal v2.0 Feb 2, 2021

Carlos Zamora (carlos-zamora) closed this Mar 18, 2021

Conversation

Carlos Zamora (carlos-zamora) commented Nov 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Uh oh!

Mike Griese (zadjii-msft) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Deleted user (ghost) commented Jan 10, 2020

Uh oh!

Michael Niksa (miniksa) commented Jan 10, 2020

Uh oh!

Deleted user (ghost) commented Jan 23, 2020

Uh oh!

Carlos Zamora (carlos-zamora) commented Jan 24, 2020

Uh oh!

Carlos Zamora (carlos-zamora) commented Jan 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Carlos Zamora (carlos-zamora) commented Jul 1, 2020

Uh oh!

Carlos Zamora (carlos-zamora) commented Jul 15, 2020

Uh oh!

Ofek Lev (ofek) commented Dec 1, 2020

Uh oh!

Carlos Zamora (carlos-zamora) commented Dec 1, 2020

Uh oh!

Mike Griese (zadjii-msft) commented Dec 1, 2020

Uh oh!

Ofek Lev (ofek) commented Mar 18, 2021

Uh oh!

Mike Griese (zadjii-msft) commented Mar 18, 2021

Uh oh!

Carlos Zamora (carlos-zamora) commented Mar 18, 2021

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Carlos Zamora (carlos-zamora) commented Nov 28, 2019 •

edited

Loading

Carlos Zamora (carlos-zamora) commented Jan 28, 2020 •

edited

Loading