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

Keep deleting lines when triggering deleteAllLeft on column 1 #28392

Merged
merged 13 commits into from Jun 19, 2018

Conversation

Projects
None yet
5 participants
@alfonsoperez
Contributor

alfonsoperez commented Jun 9, 2017

Related issue: #17143,

Let me know what you guys think, I would love to have this feature.

@alfonsoperez

This comment has been minimized.

Contributor

alfonsoperez commented Jul 27, 2017

@rebornix any concerns with this change? thanks!

@rebornix

This comment has been minimized.

Member

rebornix commented Jul 27, 2017

@alfonsoperez I really like the idea and the code looks good. The only thing we forget to take into consideration is multi cursor.

Let's say we have below content

image

Note the cursors are before the last character of each line. Then we press cmd+backspace

image

Everything looks pretty good. Now we have a problem with what's the expected result of another cmd+backspace. Do you think it should behave the same as a simple backspace

backspace

image

cmd+backspace
image

The command is called delete all left. While in this case, the cursor is at the beginning of the line, there is nothing on the left, so I would expect another Delete All Left will just delete the line break before the cursor. While right now, the result is correct but the result selections are not the same as a backspace. What do you think on this?

@rebornix

This comment has been minimized.

Member

rebornix commented Jul 27, 2017

BTW, if you play with Sublime Text, you can see that cmd+backspace works the same as backspace in this particular case. Our _getEndCursorState needs to be smarter.

@alfonsoperez

This comment has been minimized.

Contributor

alfonsoperez commented Jul 27, 2017

Good point!, thanks for the detailed review!, I will take a look

@Microsoft Microsoft deleted a comment from msftclas Sep 12, 2017

@msftclas

This comment has been minimized.

msftclas commented Oct 16, 2017

CLA assistant check
All CLA requirements met.

@@ -43,7 +43,7 @@ function asYarnDependency(prefix, tree) {
}
function getYarnProductionDependencies(cwd) {
const raw = cp.execSync('yarn list --json', { cwd, encoding: 'utf8', env: { ...process.env, NODE_ENV: 'production' }, stdio: [null, null, 'ignore'] });
const raw = cp.execSync('yarn list --json', { cwd, encoding: 'utf8', env: Object.assign({}, process.env, { NODE_ENV: 'production' }), stdio: [null, null, 'ignore'] });

This comment has been minimized.

@alfonsoperez

alfonsoperez Mar 15, 2018

Contributor

my project didn't build with this spread here

let edits: IIdentifiedSingleEditOperation[] = effectiveRanges.map(range => {
endCursorState.push(new Selection(range.startLineNumber, range.startColumn, range.startLineNumber, range.startColumn));

This comment has been minimized.

@alfonsoperez

alfonsoperez Mar 15, 2018

Contributor

I didn't understand this line, wouldn't this produce duplicate cursors?

@alfonsoperez

This comment has been minimized.

Contributor

alfonsoperez commented Mar 15, 2018

@rebornix Sorry for the delay, I have been very busy, I had some spare time to look at this, and added 2 commits, let me know what you think!

@antoniobg

This comment has been minimized.

antoniobg commented Apr 12, 2018

any updates on this?

@rebornix rebornix self-requested a review Jun 18, 2018

@rebornix rebornix merged commit 344f4d3 into Microsoft:master Jun 19, 2018

2 checks passed

VSTS: VS Code 20180618.109 succeeded
Details
license/cla All CLA requirements met.
Details
@rebornix

This comment has been minimized.

Member

rebornix commented Jun 19, 2018

@alfonsoperez thanks for your contribution ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment