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

Prevent copying an empty line by accident #59193

Merged
merged 3 commits into from
Nov 12, 2018
Merged

Prevent copying an empty line by accident #59193

merged 3 commits into from
Nov 12, 2018

Conversation

usernamehw
Copy link
Contributor

Fixes #55243

@usernamehw usernamehw changed the title Prevent copying an empty string by accident Prevent copying an empty line by accident Sep 23, 2018
@rebornix rebornix added this to the October 2018 milestone Sep 26, 2018
@@ -142,6 +142,13 @@ class ExecCommandCopyAction extends ExecCommandAction {
if (!emptySelectionClipboard && editor.getSelection().isEmpty()) {
return;
}
// prevent copying an empty string by accident
if (editor.getSelections().length === 1 && editor.getSelection().isEmpty()) {
Copy link
Member

Choose a reason for hiding this comment

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

You may want to compare between editor.getModel().getLineFirstNonWhitespaceColumn and editor.getModel().getLineMaxColumn instead of doing a substring.

Copy link
Member

@rebornix rebornix left a comment

Choose a reason for hiding this comment

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

The approach looks good to me. The only comment I have is doing less costly comparison.

@usernamehw
Copy link
Contributor Author

cc @rebornix

@joaomoreno joaomoreno modified the milestones: October 2018, November 2018 Nov 8, 2018
Copy link
Member

@rebornix rebornix left a comment

Choose a reason for hiding this comment

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

It looks good to me.

@rebornix rebornix merged commit 0a1eb06 into microsoft:master Nov 12, 2018
@usernamehw usernamehw deleted the copy_empty branch November 12, 2018 20:15
@dkelosky
Copy link

dkelosky commented Apr 1, 2019

Is this setting configurable? I preferred the old behavior, copy whatever line I press 'Ctrl +c' on regardless of its content.

This PR assumes a user didn't want to copy a blank line which I usually do.

It seems the real fix is to copy whatever line the user used the keyboard shortcut on and recommend clipboard managers if you copy something that you didn't intend.

@dkelosky
Copy link

dkelosky commented Apr 1, 2019

FYI, I opened #69903 for this. Ctrl + C (or similar keyboard mappings on other editors like notepad++ or even online editors - like firebase rules editor) make no assumption on intention. Ctrl + C just copies the line.

I'm not opposed to inference, but in this case, it seems like it should be configurable and not assumed that copy of a blank line was in error. 😄

@zhanxiaoge
Copy link

I do not think this is a mistake, on the contrary I think he can improve my efficiency, please return to the original.

PKRoma pushed a commit to PKRoma/vscode that referenced this pull request Jul 25, 2019
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ctrl + C on blank lines
5 participants