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

Make whitespace trimming in block selection configurable #9807

Merged
4 commits merged into from
Apr 23, 2021
Merged

Make whitespace trimming in block selection configurable #9807

4 commits merged into from
Apr 23, 2021

Conversation

Don-Vito
Copy link
Contributor

@Don-Vito Don-Vito commented Apr 14, 2021 •
edited
Loading

PR Checklist

Detailed Description of the Pull Request / Additional comments

Added global flag named trimBlockSelection set to false by default.
The setting was added to Interactions menu of the SUI.

@ghost ghost 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.) Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Apr 14, 2021
@github-actions

This comment has been minimized.

Sign in to view
@Don-Vito
Copy link
Contributor Author

image

miniksa
miniksa approved these changes Apr 14, 2021
View reviewed changes
Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

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

Looks good to me but...

Documentation updated - not yet

We definitely need to file the doc bug or make sure @cinnamon-msft is ready for this.

@Don-Vito
Copy link
Contributor Author

to file the doc bug or m

Thanks!. Issuing it now - waited for initial approval.

@Don-Vito Don-Vito mentioned this pull request Apr 14, 2021
Merged
@Don-Vito
Copy link
Contributor Author

Looks good to me but...

Documentation updated - not yet

We definitely need to file the doc bug or make sure @cinnamon-msft is ready for this.

The documentation was added here: MicrosoftDocs/terminal#313

carlos-zamora
carlos-zamora approved these changes Apr 14, 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.

Looks great! Thanks!

src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw Outdated Show resolved Hide resolved
src/cascadia/TerminalCore/TerminalSelection.cpp
Comment on lines +261 to +265
// GH#9706: Trimming of trailing white-spaces in block selection is configurable.
const auto includeCRLF = !singleLine || _blockSelection;
const auto trimTrailingWhitespace = !singleLine && (!_blockSelection || _trimBlockSelection);
const auto formatWrappedRows = _blockSelection;
return _buffer->GetText(includeCRLF, trimTrailingWhitespace, selectionRects, GetAttributeColors, formatWrappedRows);
Copy link
Member

Choose a reason for hiding this comment

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

THIS LOOKS SO MUCH BETTER. THANK YOU

@carlos-zamora carlos-zamora added the AutoMerge Marked for automatic merge by the bot when requirements are met label Apr 14, 2021
@ghost
Copy link

ghost commented Apr 14, 2021

Hello @carlos-zamora!

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.

Do note that I've been instructed to only help merge pull requests of this repository that have been opened for at least 8 hours, a condition that will be fulfilled in about 1 hour 51 minutes. No worries though, I will be back when the time is right! 😉

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.

DHowett
DHowett requested changes Apr 14, 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.

Blocking so that we have space for a discussion about where the setting belongs (which level), naming, phrasing, etc.

2 signoffs is dangerously close to "it merges without us agreeing on stuff" ;P

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 14, 2021
@DHowett DHowett removed the AutoMerge Marked for automatic merge by the bot when requirements are met label Apr 14, 2021
@Don-Vito Don-Vito requested a review from DHowett April 14, 2021 18:03
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 14, 2021
@Don-Vito
Copy link
Contributor Author

@DHowett - requested review just to ensure that the bot removes "Needs-Author-Feedback".

@carlos-zamora
Copy link
Member

I'll kick off the discussion then 😊

where the setting belongs (which level)

I think this makes sense as a global. It's next to copyOnSelect, so the user can expect a consistent selection interaction across all of their sessions.

naming

trimBlockSelection: <bool>

I actually really like this name. It's short. It's clear that it only impacts block selection.

My only complaint could be that the trimming only happens when we copy, not when we select. But I think that's me just grasping at straws to find something wrong with it haha. And I honestly think people won't think of the setting in that way; docs, the schema description, and the SUI header should be more than enough to remove that ambiguity.

phrasing, etc.

"Remove trailing white-spaces when copying text in rectangular selection to clipboard"

Maybe change "rectangular" to "block". I'm mildly irritated by how long it is, but I'd rather see it be descriptive than not. And I think this is ok to be "negative", because "Keep trailing white-spaces..." isn't clear that the alternative is to remove them. But yeah, definitely want more input on this one.

Miscellaneous design thoughts

We could change this setting to trimCopiedText, but I really don't like that because I like that we're already handling trimming text pretty smart. Line selection doesn't trim, whereas block selection is up to the user. From this comment, it seems like there's already consensus on block selection being configurable in this way so this seems like a solid win for me.

@zadjii-msft zadjii-msft self-assigned this Apr 15, 2021
@miniksa
Copy link
Member

miniksa commented Apr 15, 2021

I concur with all of @carlos-zamora's points and don't have anything further to add.

@Don-Vito
Copy link
Contributor Author

It's been a week. So bump.

zadjii-msft
zadjii-msft approved these changes Apr 23, 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.

Yea this is super straightforward, I have no problems with this. /discussion

@zadjii-msft zadjii-msft assigned DHowett and unassigned zadjii-msft Apr 23, 2021
DHowett
DHowett approved these changes Apr 23, 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.

Shopping around the new wording, then we'll merge. Thanks for the patience!

@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Apr 23, 2021
@ghost ghost merged commit 3d09c7d into microsoft:main Apr 23, 2021
@ghost
Copy link

ghost commented May 25, 2021

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

Handy links:

@ghost ghost mentioned this pull request May 25, 2021
Closed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@DHowett DHowett DHowett approved these changes

@miniksa miniksa miniksa approved these changes

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

Assignees

@DHowett DHowett

Labels
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.) 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
Milestone
No milestone
Development

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