feat: handle virtual spaces when inserting code to cursor position #675
Conversation
ege0zcan
force-pushed
the
virtualSpaces
branch
3 times, most recently
from
50cbdd1 to
c45743b
Compare
| } | ||
|
|
||
| let textWithIndent = '' | ||
| params.code.split('\n').forEach((line, index) => { | ||
| if (index === 0) { | ||
| if (hasVirtualSpace) { | ||
| textWithIndent += indent |
There was a problem hiding this comment.
Similar to Line 233, if the first line is blank, it shouldn't receive indentations. (Can this also be added as a test case please?)
There was a problem hiding this comment.
I was under the impression that in the case of a virtual space we actually need this extra indent.
For a regular insertion (no virtual spaces) we don't add any indentation on the first line since the expectation is to insert the code block at the current cursor position and adding an indentation to the insertion would break the expectation
However, in the case of virtual spaces, the cursor might appear to the user at a certain position due to virtual spaces (e.g., 4 virtual spaces on a line, with the server receiving the cursor position as character 4). In this situation, the server must convert the virtual indentation into a real indentation when inserting the content into the file.
This is what the line in question achieves. To ensure the indentation is applied correctly, the server adjusts the cursor position to character 0 on line 222, so that the real spaces are inserted at the beginning of the line.
There was a problem hiding this comment.
Here is an arbitrary example for illustrative purposes. My comment above refers to line 81 in the sample image, but I have provided a complete representation of the desired outcome. It's a corner case where the first line is blank - generally this should be uncommon.
There was a problem hiding this comment.
Good catch and a nice demonstration, this makes sense to me. I will update the code and continue on adding the test cases
ege0zcan
force-pushed
the
virtualSpaces
branch
from
c45743b to
7391990
Compare
| }) | ||
| }) | ||
|
|
||
| it('handles virtual spaces correctly when code starts with empty line', async () => { |
There was a problem hiding this comment.
We should also have test case for line with code but no indent and virtual spaces set.
|
|
||
| const indentRange = { | ||
| start: { line: cursorPosition.line, character: 0 }, | ||
| end: cursorPosition, | ||
| } | ||
| const documentContent = await this.#features.workspace.getTextDocument(params.textDocument.uri) | ||
| let indent = documentContent?.getText(indentRange) | ||
|
|
||
| let hasVirtualSpace = false | ||
| if (indent && indent.trim().length !== 0) { | ||
| indent = ' '.repeat(indent.length - indent.trimStart().length) |
There was a problem hiding this comment.
Not in scope of your change, but I think it will not calculate indent correctly in some corner cases, e.g. if cursor is somewhere within the real indent not after.
There was a problem hiding this comment.
nit: I would also define different variables for cursorIntent and calculatedIntent - it would be more readable and we would not need else below.
| this.#log('Indent is nullish and the cursor position is greater than zero while inserting code') | ||
| indent = ' '.repeat(cursorPosition.character) | ||
| hasVirtualSpace = true | ||
| cursorPosition.character = 0 | ||
| } | ||
|
|
||
| let textWithIndent = '' | ||
| params.code.split('\n').forEach((line, index) => { | ||
| if (index === 0) { |
There was a problem hiding this comment.
nit: this code will trim the first newline in case the code starts with /n/n/n
There was a problem hiding this comment.
In general, this code becomes difficult to read with nested conditions. I would think about how to flatten it out (like, move if(!textWithIndent) {textWithIndent+='\n' + indent} to top) or split to functions for different pre-conditions (like, with virtual spaces and without).
Desription
Adds the ability to handle virtual spaces to chat server
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.