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

Add ability to select all text in the buffer #13045

Merged
2 commits merged into from
May 6, 2022
Merged

Conversation

carlos-zamora
Copy link
Member

@carlos-zamora carlos-zamora commented May 5, 2022

Adds the selectAll action which can be used to select all text in the buffer (regardless of whether a selection is present).

References

#3663 - Mark Mode
#4993 - [Scenario] Keyboard selection

PR Checklist

Detailed Description of the Pull Request / Additional comments

I've made it such that selecting the "entire buffer" really just selects up to the mutable viewport. This seems like a nice QOL improvement since there's generally nothing past that.

When the user selects all, the viewport does not move. This is consistent with CMD behavior and is intended to allow the user to not lose context when selecting everything.

A minor change had to be made to the DxRenderer because this uncovered an underflow issue. Basically, the selection rects were handed to the DxEngine relative to the viewport (which means that some had a negative y-value). At some point, those rects were stored into size_ts, resulting in an underflow issue. This caused the renderer to behave strangely when rendering the selection. Generally, these kinds of issues weren't really noticed because selection would always modify a portion of the viewport.

Funny enough, in a way, this satisfies the "mark mode" scenario because the user now has a way to initiate a selection using only the keyboard. Though this isn't ideal, just a fun thing to point out (that's why I'm not closing the mark mode issue).

Validation Steps Performed

  • Verified using DxEngine and AtlasEngine
  • select all --> keyboard selection --> start moving the top-left endpoint (and scroll to there)
  • select all --> do not scroll automatically

@ghost ghost added Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels May 5, 2022
@DHowett
Copy link
Member

DHowett commented May 5, 2022

Clever, with the existing selection being a requirement for selecting all. It might be hard for somebody to discover this, unfortunately.

I somewhat wonder if we should just make the default Ctrl+Shift+A, always select all even if there is no selection, and then enable Ctrl+A in Mark Mode

@zadjii
Copy link

zadjii commented May 6, 2022

I somewhat wonder if we should just make the default Ctrl+Shift+A, always select all even if there is no selection, and then enable Ctrl+A in Mark Mode

Ack. Especially since there's no way to start a selection with the keyboard ATM

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.

As discussed!

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label May 6, 2022
@zadjii-msft zadjii-msft added this to the Terminal v1.15 milestone May 6, 2022
@carlos-zamora
Copy link
Member Author

Ctrl+Shift+A, always select all even if there is no selection

I'm ok with this, but if that's the case, we should make it a configurable keybinding.

My main reasoning for that is that I really want to separate mark mode stuff and keybindings. For now, I don't want mark mode to be configurable. We'd be introducing a ton of keybindings that are conditional on a selection being present, and that's spooky. If selectAll can be an action that ignores if a selection is present, might as well make it configurable.

and then enable Ctrl+A in Mark Mode

works for me, I'll move this logic over to my mark mode branch then.

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label May 6, 2022
@carlos-zamora
Copy link
Member Author

FYI Most of the new changes were just plumbing the action through, so I'm explicitly not re-requestion a review from @lhecker.

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

@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label May 6, 2022
@ghost
Copy link

ghost commented May 6, 2022

Hello @DHowett!

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.

@ghost ghost merged commit 60a4837 into main May 6, 2022
@ghost ghost deleted the dev/cazamor/kbd-sln/select-all branch May 6, 2022 20:06
@carlos-zamora carlos-zamora added the zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. label May 6, 2022
@DHowett DHowett removed the zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. label May 9, 2022
DHowett pushed a commit that referenced this pull request May 11, 2022
Adds the `selectAll` action to the schema...because I forgot to do that in #13045.
@ghost
Copy link

ghost commented May 24, 2022

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

Handy links:

This pull request was closed.
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-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to select/copy all the text in the session
5 participants