Skip to content

Keep deleting lines when triggering deleteAllLeft on column 1 - #28392

Merged
Peng Lyu (rebornix) merged 13 commits into
microsoft:masterfrom
alfondotnet:feature/cmd-delete
Jun 19, 2018
Merged

Peng Lyu (rebornix) merged 13 commits into
microsoft:masterfrom
alfondotnet:feature/cmd-delete

Conversation

@alfondotnet

@alfondotnet Alfonso Embid-Desmet (alfondotnet) commented Jun 9, 2017 •

Copy link
Copy Markdown
Contributor

Related issue: #17143,

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

@alfondotnet

Copy link
Copy Markdown
Contributor Author

Peng Lyu (@rebornix) any concerns with this change? thanks!

@rebornix

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor

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.

@alfondotnet

Copy link
Copy Markdown
Contributor Author

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

@msftclas

Microsoft Contribution License Agreements (msftclas) commented Oct 16, 2017 •

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.

Comment thread build/dependencies.js Outdated

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'] });

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

my project didn't build with this spread here

let endCursorState = this._getEndCursorState(primaryCursor, effectiveRanges);

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

@alfondotnet

Copy link
Copy Markdown
Contributor Author

Peng Lyu (@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

Copy link
Copy Markdown

any updates on this?

@rebornix
Peng Lyu (rebornix) self-requested a review June 18, 2018 15:40
@rebornix
Peng Lyu (rebornix) merged commit 344f4d3 into microsoft:master Jun 19, 2018
@rebornix

Copy link
Copy Markdown
Contributor

alfonsoperez thanks for your contribution ❤️

@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.

5 participants