Add missing Sublime/Atom commands #15787
Conversation
|
@rebornix, thanks for your PR! By analyzing the history of the files in this pull request, we identified @alexandrudima, @egamma and @jrieken to be potential reviewers. |
|
is paste and indent a larger feature as part of the formatting package? or something you'll add independently of this? Indenting to the nearest line is not a practical solution, correct? |
rebornix
force-pushed
the
MissingCommands
branch
from
a4d8f7d to
0cfdc93
Compare
|
Add test cases for these commands. Please note that we don't use the name Besides, the formatting of the content is decided by format provider. @wprater . |
rebornix
force-pushed
the
MissingCommands
branch
from
0cfdc93 to
a4ccc41
Compare
There was a problem hiding this comment.
TL;DR I'd remove the formatAndPaste action for now, we can discuss more about it, I would prefer to have the responsibility of the format contribution to format on paste (with an option).
Otherwise, there's a typo that causes bundling errors, and some corner cases.
Nice job overall! :)
| import { SortLinesCommand } from 'vs/editor/contrib/linesOperations/common/sortLinesCommand'; | ||
| import { getDocumentRangeFormattingEdits } from 'vs/editor/contrib/format/common/format'; | ||
| import { EditOperationsCommand } from 'vs/editor/contrib/format/common//formatCommand'; |
There was a problem hiding this comment.
two slashes here -> vs/editor/contrib/format/common//formatCommand
| insertSpace = false; | ||
| } | ||
|
|
||
| if (trimmedLinesContent.charAt(trimmedLinesContent.length - 1) === ' ' || |
There was a problem hiding this comment.
trimmedLinesContent.length could be 0 here, leading to a call of trimmedLinesContent.charAt(-1).
You could use if (insertSpace && ....)
There was a problem hiding this comment.
I relied on charAt(-1) returning empty string but yes it's not a good one.
|
|
||
| let lineTextWithoutIndent = lineText.substr(firstNonWhitespaceIdx - 1); | ||
|
|
||
| if (lineTextWithoutIndent.charAt(0) === ')') { |
There was a problem hiding this comment.
Why are we looking for ) in the line text here ? i.e. this looks weird to me, why not for ]. I suggest to remove this unless it is part of some weird join lines spec.
There was a problem hiding this comment.
The one I followed is Vim http://vimdoc.sourceforge.net/htmldoc/change.html#J but Sublime is doing this for all matching brackets. I think we can do the same, read matching brackets from language config. Right now I'll just remove it.
| } | ||
|
|
||
| @editorAction | ||
| class PasteAndFormatAction extends EditorAction { |
There was a problem hiding this comment.
👎 IMHO on this action.
I would prefer to have an editor.formatOnPaste which in my opinion would align more with our current approach to automatic formatting (same as formatOnType and formatOnSave). @jrieken what do you think?
This one also assumes it knows how paste is implemented in the cursor, but in case of multicursor paste touches multiple ranges, not just one. i.e. pastedContentRange is incorrect in case of multicursor paste.
I suggest to wait with this action and consider implementing it in the format contribution, we are shipping since a long time without it, and there's no reason to rush it in.
| let selection = selections[i]; | ||
| if (selection.isEmpty()) { | ||
| let cursor = selection.getStartPosition(); | ||
| let word = model.getWordAtPosition(cursor); |
| let selection = selections[i]; | ||
| if (selection.isEmpty()) { | ||
| let cursor = selection.getStartPosition(); | ||
| let word = model.getWordAtPosition(cursor); |
There was a problem hiding this comment.
word can be null here. think of empty lines.
| let word = model.getWordAtPosition(cursor); | ||
| let wordRange = new Range(cursor.lineNumber, word.startColumn, cursor.lineNumber, word.endColumn); | ||
|
|
||
| if (wordRange !== undefined) { |
| } | ||
|
|
||
| @editorAction | ||
| export class LowerCaseAction extends EditorAction { |
There was a problem hiding this comment.
I suggest to create an abstract CaseAction that has an abstract method _modifyText or whatever that can be overwritten to use toLocaleUpperCase and toLocalLowerCase to avoid code duplication.
There was a problem hiding this comment.
Refactored. Now they should look similar to abstract class AbstractFormatAction. Thanks for pointing it out. I was too lazy at that moment.
| ); | ||
| }); | ||
|
|
||
| test('toggle case', function () { |
There was a problem hiding this comment.
add more tests, e.g. for empty lines or for the cursor in the middle of whitespace.
| }); | ||
| }); | ||
|
|
||
| test('transpose', function () { |
There was a problem hiding this comment.
does it work for empty lines, or on empty files?
You can use scripts\test.bat --coverage and my awesome LCOV plugin to see your code coverage ;)
There was a problem hiding this comment.
They work well, added test cases for them as well.
|
@alexandrudima removed paste and indent action and addressed other comments. |
|
Nice! LGTM 👍 |
|
Can the shortcut be added to toggle case along with upper and lowercase, as this makes life easier to have just one shortcut to toggle case. Which is what IDEA follows, which starts with upper casing everything, followed by lower casing, if you press the same shortcut again. |
Add missing commands from Sublime/Atom per #3776 .
TODO:
Discussed with Kai and Alex, our
contribpart of code is used as internal extension so it's Okay to put these missing commands into our code base. However while implementing them in core, I found that there is one new issue: key bindings conflicts. Will have a discuss with Alex and @waderyan on Monday.