"insert code cell" splits cell if cursor inside - #1086
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
vezwork
changed the title
vezwork
changed the title
juliasilge
left a comment
juliasilge
left a comment
There was a problem hiding this comment.
Thank you so much for working on this! I left five inline comments on some suggested improvements, and two of them ask for behavior that may not match RStudio exactly FWIW. Take a look and see if you agree!
| } else { | ||
| splitStart = clamp(selection.start); | ||
| splitEnd = clamp(selection.end); | ||
| } |
There was a problem hiding this comment.
splitStart and splitEnd come from selection.start and selection.end at exact character offsets. Nothing snaps them to line boundaries, so a partial-line selection splits the code in the middle of a line.
The empty-selection branch just above already normalizes to column 0. Can the selection branch can do the same, taking selection.start.line for the start, and the line after selection.end.line for the end Ignore an end position at column 0, because that position belongs to the previous line.
There was a problem hiding this comment.
You're suggesting making it so it does not split in the middle of a line right?
There was a problem hiding this comment.
Yes, for this case. In other branches, we already handle not splitting in the middle of the line.
| } | ||
|
|
||
| // render the cells (empty cells get a blank line for the cursor to land on) | ||
| const cellText = (body: string) => header + "\n" + body + "\n" + fence; |
There was a problem hiding this comment.
header (line 152) is the whole fence line, and cellText reuses it for every entry in bodies. Chunk labels and cell options are therefore copied onto each new cell.
To reproduce: split a cell whose header is ```{r setup, include=FALSE}. The result is two chunks that both carry the label setup. knitr then stops the render with Duplicate chunk label 'setup'. The include=FALSE option also lands on a cell that the user did not intend it for.
languageNameFromBlock is already imported in this file, so a new cell can get a bare header like this: fence + "{" + languageNameFromBlock(block) + "}".
One cell still needs to keep the original header, so we do have to decide that and I think the first cell is mostly. One rule that I think will work is for the first cell that has a body keeps the original header, and every other cell gets the bare header.
| const cellBody = (range: Range) => { | ||
| const text = doc | ||
| .getText(range) | ||
| .replace(/^([ \t]*\n)+/, "") |
There was a problem hiding this comment.
We do sometimes get reports from Windows users dealing with line ending issues. Both trim patterns match LF only, and the joins below hard-code "\n". A CRLF document therefore keeps a blank line it should lose, gains a stray carriage return, and ends up with mixed line endings.
doc.eol gives the line ending of the document. Could we use it for both joins, and make the two trim patterns \r?\n aware? EndOfLine is not in the import list at the top of the file yet.
| const applied = await editor.edit((edit) => edit.replace(replaceRange, newText)); | ||
| if (applied) { | ||
| const cursor = new Position(cursorLine, 0); | ||
| editor.selection = new Selection(cursor, cursor); | ||
| editor.revealRange(new Range(cursor, cursor)); | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
This returns true even when editor.edit rejects the edit, and line 57 returns as soon as it sees true. A rejected edit therefore looks the same as a successful split, with no text changes, no fallback, and no message to the user.
This isn't a huge deal as the window is small and the result is a no-op rather than damaged text. Do you think it's worth a line because the no-op is silent?
| "scope": "window", | ||
| "type": "integer", | ||
| "default": 250, | ||
| "default": 50, |
There was a problem hiding this comment.
The delay default drops from 250 ms to 50 ms, so the two throttles it feeds can fire five times as often. Most of that extra work looks avoidable, so what do you think about caching this? I'm not necessarily making an argument for keeping 250 ms.
engine.parse is already cheap on repeat calls. markdownitParser wraps cachingParser (packages/quarto-core/src/markdown/parser.ts:23), which memoizes by uri and version. A tick with no edit does not reparse.
The part that has no cache right now is the inline code scan in background.ts:166-176. It calls lineAt and matchAll for every line of the document, on every tick. The document highlight provider registered at background.ts:95 fires as the cursor moves, so this whole-document scan can now run 20 times per second while the user only moves the cursor around.
If blockRanges and inlineRanges were cached by document version, the way div-brackets.ts:56 already caches its tokens, those ticks would cost almost nothing. Then 50 ms is a fine default.
3 participants
Fixes #382
Kapture.2026-08-12.at.14.52.30.mp4
Improves the "insert code cell" functionality to split cell when cursor is inside, or insert cell above when cursor is at top. Also splits three ways when there is a selection (not shown in video).
Tested in both VSCode and Positron.
Design considerations
An LLM looked up the RStudio source code for "insert code cell" as the reference implementation for this feature. The functionality should match RStudio.
I lowered the background highlighting delay ms in this PR from 250ms to 50ms because 250s looked disorientingly jarring when splitting code cells. I don't know how reasonable this is to do, but it seems much nicer in general if people's computers can handle it. RStudio has very fast background highlighting so its nice if the experience in VSCode and Positron can try to match that.