Advance cursor to next code chunk after executing last line - #1127
jeroenjanssens wants to merge 1 commit into
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. |
When running code line-by-line with Cmd+Enter, executing the last line of a code chunk now moves the cursor to the first line of the next chunk instead of falling into prose. Related: quarto-dev#704
8fad34a to
f76fcb3
Compare
juliasilge
left a comment
There was a problem hiding this comment.
Thank you for this contribution! I think it's defensible that the behavior you describe is a good one to have as the default, but I will highlight that this is the opposite of what is requested in #704 so we'll eventually need to think through how we can make this opt out (or in, I suppose).
My biggest concern is that we need to get this under test, because we're not quite there yet in terms of the behavior. There's at least one regression, and I also think the two visual editor changes never run.
The blocker: the cursor gets stuck on a #| line
navigateToBlock moves the cursor to block.range.start.line + 1 (commands.ts:780). That is the first line inside the chunk. If the next chunk starts with cell options, the cursor lands on the #| line.
With a document like this, the cursor moves from a previous cell to line 13:
12 ```{r}
13 #| label: two
14 b_first <- 10
15 ```
In Positron I then press Cmd+Enter again and nothing happens. No code reaches the console and the cursor does not move. Every later Cmd+Enter also does nothing. The only way out is to move the cursor by hand.
This is worse than main where the cursor never stops on an option line, because it moves down one line at a time. This branch puts the cursor there on purpose, so a very common keystroke now leads to a dead end. navigateToBlock is shared with quarto.goToNextCell, so this landing position problem does already exist on main but this PR makes it much more frequent, because it moves the problem from an explicit navigation command to every Cmd+Enter at a chunk boundary.
There is a smaller related problem where the line-by-line path sends the raw document line to the interpreter (commands.ts:318). It does not strip cell options, while the run-cell path uses codeWithoutOptionsFromBlock (commands.ts:301), so this path can submit #| label: potato to the runtime.
The two visual editor branches are unreachable
rmdChunk appends a trailing newline to the chunk code (packages/editor/src/api/rmd.ts:178). So lines(activeBlock.code) always ends with an empty string. codeBlockPositionToViewPos has a comment about this at packages/editor/src/api/codeview.ts:198, and it slices that empty line off before it uses the line number.
For a chunk with three code lines, codeLines.length is 4 and the largest cursor line index is 2. So:
context.selection.start.line >= codeLines.length - 1(commands.ts:395 and commands.ts:420) is>= 3. The cursor never reaches 3.adjustedPos.line >= codeLines.length(commands.ts:390) is>= 4, which has the same problem.
IIUC both branches are dead and only the source editor path changes behavior today. I built this and it does seem to do nothing in the visual editor.
There is also a question about whether the visual editor needs this at all. Each chunk is a separate CodeMirror instance. The existing "nextline" action runs if (cursorLineDown(cmView)) { cursorLineStart(cmView); } (packages/editor-codemirror/src/behaviors/trackselection.ts:61). At the last line cursorLineDown returns false, so the cursor stays inside the chunk. The problem in #704 is that the cursor falls into prose, and that does not happen in visual mode.
Suggested way forward
1. Fix the landing position. Make the advance land on the first executable line, not on the first line. isYamlHashOption (providers/cell/executors.ts:126) already has the test you need. This would work well as its own small PR, because it also improves quarto.goToNextCell on main!
2. Narrow this PR to the source editor. Remove the two visual editor changes. They do not run today, and the visual editor does not have the problem they correct.
3. Keep the visual editor work for later. Advance to the next chunk in visual mode could still be a good improvement in the future. It needs its own change and its own test.
Tests
The stuck cursor and the dead branches are both cheap to test, and a test for each one would fail on this branch today.
Landing position, in the main vscode suite. cellCommands(host, engine) is exported and Command.execute() is public, so a test can build the command with a fake host rather than go through the registered command:
- The fake host returns
{ execute: async () => {} }fromcellExecutorForLanguage. It has noexecuteSelection, so the manual selection branch runs. - Open the document in memory with
vscode.workspace.openTextDocument({ language: "quarto", content }).cell-features.test.tsdoes this to keep.vdoc.*files out of the workspace. - Put the cursor on the last line of the first chunk, call
execute(), then assert oneditor.selection.active.line.
The case that matters most: the next chunk starts with #|, and the cursor must land on the code line below it.
Other cases worth covering: a middle line moves down one line, the last chunk in the document does not move, and the next chunk is {ojs} with no executor. That last one matters because you call nextBlock with requireExecutor = false (commands.ts:343), while RunNextCellCommand uses requireEvaluated = true (commands.ts:201). Is that difference deliberate? I don't think I understand it.
The line arithmetic, as a pure function. If we keep any of the visual editor work, the decision is worth extracting into a small function such as shouldAdvanceToNextBlock(cursorLine, lastCodeLineIndex). packages/core already runs node --import tsx --test test/*.test.ts, so these tests are fast and need no VS Code. Add one test that pins the input fact as well:
assert.equal(lines(rmdChunk("{r}\nx <- 1\ny <- 2")!.code).length, 3);Without that test, a unit test can repeat the same wrong assumption about line counts.
Positron, as a follow up. In Positron, our src/test/positron/execute-cell.test.ts already has the harness: a spy over positron.runtime, an in-memory document, a cursor position, and quarto.runCurrentCell. An assertion on the editor selection after the run covers the branch where executeSelectionInteractive returns true. That is the path where Positron moves the cursor itself, and it is the path where I saw the stuck state. If we do end up making some of these changes, I am happy to take that on to make sure we don't regress there.
Summary
When running code line-by-line with Cmd+Enter, executing the last line of a code chunk causes the cursor to "fall out" of the chunk into prose. This is disruptive because the next chunk could be dozens of lines away and the user has to navigate back manually. Worse, if the user keeps pressing Cmd+Enter, the interpreter will try to execute the closing fence or prose text, leading to errors.
With this change, executing the last line of a code chunk moves the cursor to the first line of the next code chunk (if one exists). If there's no next chunk, the cursor lands on the closing fence as before.
This covers three code paths in
RunCurrentCommand:executeAtPositionpath)The "run entire cell" paths (plain VS Code with Python/R, non-knitr visual editor) are unaffected since they don't do line-by-line execution.
Related
#704 describes the same trigger (Cmd+Enter on the last line exits the chunk), though the author there preferred the cursor to stay put rather than advance. Both behaviors could coexist: this PR handles advancing to the next chunk, while Positron's Alt+Enter (posit-dev/positron#2778) already supports executing without moving the cursor.