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

Implement Keyboard Selection #10824

Merged
merged 10 commits into from
Sep 23, 2021
Merged

Conversation

carlos-zamora
Copy link
Member

@carlos-zamora carlos-zamora commented Jul 29, 2021

Summary of the Pull Request

Implements the following keyboard selection non-configurable key bindings:

  • shift+arrow --> move endpoint by character
  • ctrl+shift+left/right --> move endpoint by word
  • shift+home/end --> move to beginning/end of line
  • ctrl+shift+home/end --> move to beginning/end of buffer

This was purposefully done in the ControlCore layer to make keyboard selection an innate part of how the terminal functions (aka a shared component across terminal consumers).

References

#715 - Keyboard Selection
#2840 - Spec

Detailed Description of the Pull Request / Additional comments

The most relevant section is TerminalSelection.cpp, where we define how each movement operates. It's basically a giant embedded switch-case statement. We leverage a lot of the work done in a11y to perform the movements.

Validation Steps Performed

  • General cases:
    • test all of the key bindings added
  • Corner cases:
    • char: wide glyph support
    • word: move towards, away, and across the selection pivot
    • automatically scroll viewport
    • ESC (and other key combos) are still clearing the selection properly

@carlos-zamora
Copy link
Member Author

carlos-zamora commented Jul 29, 2021

Discussion

  • settings names:
    • "mode" --> "expansionMode"? More descriptive but might not be necessary
    • "updateSelection" --> "moveSelectionAnchor"? Sounds too descriptive imo
    • "view" --> "viewport"? Unsure if "view" is clear, but "viewport" is too in-the-know
    • "buffer" --> ???
    • "cell" --> "char"? We support wide glyphs, so this might be more correct? Resolved by 5b4dd33
  • behavior:
    • word selection currently operates like mouse selection (select by runs of same delimiter class, not "words" [i.e. "abc " is "abc" and " "]). Text editors behave differently by (un)selecting the whitespace next to the word. Should we consider this a backlog follow-up item? Do we want selection to work this way?
    • buffer selection moves to the end of the full buffer. Something we can't see and that isn't very helpful. Should we set the "end of the buffer" to something more useful/reasonable? Like the bottom of the mutable viewport? Resolved by Implement Keyboard Selection #10824 (comment)
  • known issues (unrelated to this PR)

@carlos-zamora carlos-zamora force-pushed the dev/cazamor/keyboard-selection branch 2 times, most recently from 1df76e6 to 841cd2a Compare July 31, 2021 00:13
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.

I guess I'm blocking only for the GenerateName thing? And update the docs? But I suppose the second can wait for the naming discussions to be finalized. Code looks great though.

Already looking forward to 1.12!

src/cascadia/TerminalSettingsModel/ActionArgs.cpp Outdated Show resolved Hide resolved
{ "command": {"action": "updateSelection", "direction": "down", "mode": "view" }, "keys": "shift+pgdn" },
{ "command": {"action": "updateSelection", "direction": "up", "mode": "buffer" }, "keys": "ctrl+shift+home" },
{ "command": {"action": "updateSelection", "direction": "down", "mode": "buffer" }, "keys": "ctrl+shift+end" },

Copy link
Member

Choose a reason for hiding this comment

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

ah ohkay, we'll need a separate action for selectAll. That's fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that can be a separate PR too

src/cascadia/TerminalCore/TerminalSelection.cpp Outdated Show resolved Hide resolved
// 4. Scroll (if necessary)
if (const auto viewport = _GetVisibleViewport(); !viewport.IsInBounds(targetPos))
{
if (const auto amtAboveView = viewport.Top() - targetPos.Y; amtAboveView > 0)
Copy link
Member

Choose a reason for hiding this comment

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

little surprised that spellbot didn't complain about amt here.

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 11, 2021
@zadjii-msft
Copy link
Member

  • settings names:
    • "cell" --> "char"? We support wide glyphs, so this might be more correct?

this is the only change I feel is necessary. char is better. The rest are fine

  • behavior:
    • ...Text editors behave differently by (un)selecting the whitespace next to the word. Should we consider this a backlog follow-up item? Do we want selection to work this way?

please no god no. IMO, bad text editors behave this way. Code editors generally don't.

  • buffer selection moves to the end of the full buffer. Something we can't see and that isn't very helpful. Should we set the "end of the buffer" to something more useful/reasonable? Like the bottom of the mutable viewport?

Yea, the mutable bottom is more reasonable. The "scroll forward" doesn't exist for all intents and purposes.

@ghost ghost added the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Aug 18, 2021
@ghost

This comment has been minimized.

@zadjii-msft zadjii-msft removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 18, 2021
@ghost ghost removed the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Aug 18, 2021
@zadjii-msft
Copy link
Member

shh bot

@carlos-zamora
Copy link
Member Author

buffer selection moves to the end of the full buffer. Something we can't see and that isn't very helpful. Should we set the "end of the buffer" to something more useful/reasonable? Like the bottom of the mutable viewport?

Yea, the mutable bottom is more reasonable. The "scroll forward" doesn't exist for all intents and purposes.

Updated _MoveByViewport and _MoveByBuffer to clamp to the mutable bottom. It feels really nice!

@carlos-zamora carlos-zamora force-pushed the dev/cazamor/keyboard-selection branch from 5b4dd33 to b2a7a19 Compare August 18, 2021 23:20
@carlos-zamora carlos-zamora marked this pull request as ready for review August 18, 2021 23:20
@carlos-zamora
Copy link
Member Author

It looks like Terminal.hpp can't find SelectionExpansion from ICoreSettings.idl when in CI, but it builds just fine locally. Any ideas?

@zadjii-msft zadjii-msft added this to the Terminal v1.12 milestone Aug 26, 2021
@carlos-zamora carlos-zamora force-pushed the dev/cazamor/keyboard-selection branch from 5297b80 to 0cc9bd0 Compare September 2, 2021 22:04
@zadjii-msft
Copy link
Member

That's so bizarre that this works locally, but not in CI. My guess is related to this bit at the top of Terminal.hpp:

// You have to forward decl the ICoreSettings here, instead of including the header.
// If you include the header, there will be compilation errors with other
//      headers that include Terminal.hpp
namespace winrt::Microsoft::Terminal::Core
{
    struct ICoreSettings;
    struct ICoreAppearance;
}

OH I know why, it's because you're not building PublicTerminalCore locally. That's the one that's failing 😋

@zadjii-msft
Copy link
Member

ad99008 might work to fix this. It certainly did locally. Take it or leave it :P

zadjii-msft
zadjii-msft previously approved these changes Sep 14, 2021
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.

I hate the duplicated enums, but I understand the world we live in

src/cascadia/PublicTerminalCore/HwndTerminal.cpp Outdated Show resolved Hide resolved
@carlos-zamora carlos-zamora removed their assignment Sep 14, 2021
@carlos-zamora

This comment has been minimized.

@carlos-zamora carlos-zamora marked this pull request as draft September 16, 2021 22:19
@carlos-zamora carlos-zamora force-pushed the dev/cazamor/keyboard-selection branch from e498dea to 118d1cd Compare September 17, 2021 02:58
@carlos-zamora carlos-zamora marked this pull request as ready for review September 17, 2021 02:59
@carlos-zamora carlos-zamora dismissed zadjii-msft’s stale review September 17, 2021 03:06

Dismissing Mike's review because this iteration was a major design change and I wanna make sure he's on board.

@carlos-zamora
Copy link
Member Author

Assigning @DHowett because he has some very passionate thoughts on keyboard selection haha

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.

:shipit:

@zadjii-msft zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Sep 22, 2021
@ghost ghost requested review from miniksa, leonMSFT, PankajBhojwani and lhecker September 22, 2021 15:31
// Return Value:
// - pos - The COORD for the last cell of the current glyph (exclusive)
const til::point TextBuffer::GetGlyphEnd(const til::point pos, std::optional<til::point> limitOptional) const
const til::point TextBuffer::GetGlyphEnd(const til::point pos, bool accessibilityMode, std::optional<til::point> limitOptional) const
Copy link
Member

@lhecker lhecker Sep 22, 2021

Choose a reason for hiding this comment

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

(The amount of arguments our TextBuffer member functions have is worrying ngl. 😄)

Why does "accessibility mode" mean "become exclusive" here? Don't you mean "bool exclusivePosition"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeeeeaaaaah. So, a11y and selection have a lot of similarities and very small/important differences. Some highlights include...

Selection A11y
endpoint representation both endpoints are inclusive; start <= end inclusive start; exclusive end; start <= end
definition of a word a run of legible characters run of legible characters + run of whitespace after it (until next word starts)

I just chose to name the parameter accessibilityMode to make it clear what and when we're only doing some extra work for a11y.

src/cascadia/TerminalControl/ControlCore.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalControl/ControlCore.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalCore/TerminalSelection.cpp Outdated Show resolved Hide resolved
// GH#8522, GH#3758 - Only dismiss the selection on key _down_. If we
// dismiss on key up, then there's chance that we'll immediately dismiss
// GH#8522, GH#3758 - Only modify the selection on key _down_. If we
// modify on key up, then there's chance that we'll immediately dismiss
Copy link
Member

Choose a reason for hiding this comment

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

<showerthought>

If we could handle this modification on key up, then could we do something like bind markMode to shift+left, and have the shift+left keydown enter mark mode, and the keyup extend the selection to the left? Essentially getting quickedit-like selection?

This is of course, crazy, but something to think about

Copy link
Member

Choose a reason for hiding this comment

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

HECK IT DOESN"T EVEN MATTER

a user could just bind markmost to shift+left, and that would start a selection at the cursor, and the subsequent keypresses would extend it without hitting the keybinding, which is actually just like conhost. Brilliant 🥂

@zadjii-msft
Copy link
Member

shipit

@carlos-zamora carlos-zamora merged commit c070be1 into main Sep 23, 2021
@carlos-zamora carlos-zamora deleted the dev/cazamor/keyboard-selection branch September 23, 2021 19:24
@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:

DHowett pushed a commit that referenced this pull request Jun 22, 2022
## Summary of the Pull Request
1.14 port of #13318

Introduced in #10824, this fixes a bug where you could use keyboard selection to move below the scroll area. Instead, we now clamp movement to the mutable viewport (aka the scrollable area). Specifically, we only clamp the y-coordinate to make the experience similar to that of mouse selection.

## Validation Steps Performed
✅ (no output) try to move past bottom of viewport
✅ (with output, at bottom of scroll area) try to move past viewport
✅ (with output, NOT at bottom of scroll area) try to move past viewport
✅ try to move past top of viewport
ghost pushed a commit that referenced this pull request Jun 22, 2022
## Summary of the Pull Request
Introduced in #10824, this fixes a bug where you could use keyboard selection to move below the scroll area. Instead, we now clamp movement to the mutable viewport (aka the scrollable area). Specifically, we clamp to the corners (i.e. 0,0 or bottom right cell of scroll area).

## Validation Steps Performed
✅ (no output) try to move past bottom of viewport
✅ (with output, at bottom of scroll area) try to move past viewport
✅ (with output, NOT at bottom of scroll area) try to move past viewport
✅ try to move past top of viewport
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs-Second It's a PR that needs another sign-off
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants