-
Notifications
You must be signed in to change notification settings - Fork 45
in VE use Positron's statementRangeProvider to execute statements
#825
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
statementRangeProvider in Positron to execute statements at cursor in VEstatementRangeProvider in Positron to execute statements at cursor
statementRangeProvider in Positron to execute statements at cursorstatementRangeProvider to execute statements at cursor
statementRangeProvider to execute statements at cursorstatementRangeProvider to execute statements
|
I think it's ok to prioritize Positron fixes and leave the VSC fix open for a community PR if there's an approach there that would work. |
|
This is really exciting to see! 🎉 I think there is a bit of improvement to do in terms of where the cursor goes in the visual editor after using these. I see that you said there is some difficulty about setting the cursor position in the visual editor 😬 so it's possible this is a big challenge. Take a look at what I mean:
I also wanted to ask about Python, because I don't see the Python statement range provider being applied, I think? Here is an example to look at:
|
I should be able to set the cursor in the visual editor! The cursor placement problem I was having wasn't visual editor specific, it was virtual doc specific. I will try to improve where the cursor goes after running ctrl+enter. Aside, to elaborate on the virtual doc + cursor problem I was facing: it would be nice to unify execution in the visual editor and the source editor by creating a virtual doc for the current code block, placing a cursor in the virtual doc, and running the relevent execution-at-cursor command in that virtual doc. Unfortunately, I think virtual docs are just files and are not "open" in an editor, so they don't have a cursor associated with them. |
Heard and seen! Thanks for the great example. Indeed the python statement range provider does not seem to be being applied. Also looking at this. |
|
Q: In the source editor, when you A: @sharon-wang found it for me: https://github.com/posit-dev/positron/blob/main/src/vs/workbench/contrib/positronConsole/browser/positronConsoleActions.ts#L428 in the I will copy this logic into the Quarto LSP. Maybe ideally Positron would provide a command like "vscode.executeStatementRangeProviderAndGetNextStatementPosition" that returns both a |
|
I think this was only applying to R documents because of this here:
What happens in the source editor is something like this:
quarto/apps/vscode/src/host/hooks.ts Line 97 in 012d457
quarto/apps/vscode/src/host/hooks.ts Line 161 in 012d457
quarto/apps/vscode/src/host/hooks.ts Line 186 in 012d457
So it goes back and forth a fair amount! I think you will want to set up request forwarding in the You can see what LSP functionality the source editor has with all the "providers" in here: quarto/apps/vscode/src/lsp/client.ts Line 72 in 012d457
|
3e39dce to
ff907e3
Compare
|
Statement execution (with cursor advancement to the next statement) should now be working for both R and Python cells in the Visual Editor in Positron. A caveat:
|
I think this is a safe assumption. Someone might one day want to run code cells with extra braces, like |
| } | ||
|
|
||
| export type CodeViewSelectionAction = "nextline" | "nextblock" | "prevblock"; | ||
| export type CodeViewSelectionAction = "nextline" | "nextblock" | "prevblock" | { line: number, character: number }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if you want to add a field to that object that you can explicitly use as the tag for a tagged union.
If you needed to expand the type today, then you've accepted that migrations are necessary. But you've now made migration a little bit harder in the case where a new value also has line: number, character: number.
export type CodeViewSelectionAction = "nextline" | "nextblock" | "prevblock" | { line: number, character: number }
| {some_other_field: string, line: number, character: number} // testing for this is not annoying.
cscheid
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor comment on type design, but otherwise LGTM.
juliasilge
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| } | ||
|
|
||
| // if in Positron | ||
| if (hasHooks()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (hasHooks()) { | |
| if (isPositron) { |
Instead of adding a new use of hasHooks(), can we use the better support in the positron npm package, like this:
quarto/apps/vscode/src/core/quarto.ts
Line 20 in 681e682
| import { tryAcquirePositronApi } from "@posit-dev/positron"; |
And then something like:
const isPositron = tryAcquirePositronApi();We don't need to do a wholesale refactor of the extension, but let's not add new uses of the old, worse approach.
|
Thank you so much for your persistence in working on this tough problem! 🙌 This implementation has unfortunately broken how the statement range provider works in source mode in Python. Take a file like this: Put your cursor in the Python code and use Cmd+Enter to step through the statements. With the current release build of the Quarto extension, you'll step through the statements one by one, including the multiline statement, moving forward to the next statement each time. With this current PR, the statements are not identified correctly and you do not move forward correctly I still feel surprised about the approach being taken here, with putting really specific logic into the command vs. piping through the LSP request to the visual editor. Can you outline for my understanding why we are taking this approach? From my previous experience in this area,
I would love to understand more about why that type of approach hasn't played out. I know you have really focused on this area, and I definitely trust that you have learned a lot and we can figure out a good way forward! |
|
Hey @juliasilge, I am working on writing up a proper response to those questions.
edit: okay, I am fairly confident now that the latest commit on edit 2: I tracked down the problem: Embarrassingly, I did not properly fix my small refactor after being called out on it being breaking. I failed to properly inline edit 3: I have gone ahead and merged a one line PR to fix the source mode statement execution. In that PR I describe where the issue came from. edit 4: I have rebased this PR on |
I don't think my changes to `codeViewSetBlockSelection` work. It seems that `navigateToPos` does not work inside codeMirror? That must be why only "nextline" works in the command - it doesn't use `navigateToPos` it uses a special codeView thing.
ff907e3 to
e944e72
Compare

New in Positron: pressing ctrl+enter with your cursor in a multiline R or Python statement executes the whole statement.
Before it would only execute the current line.
Note: this fix relies on Positron's
vscode.executeStatementRangeProvidercommand and does not work in VSCode. I was attempting to get this working in VSCode by creating a virtual doc for the code block, setting your cursor in the virtual doc, and running a cell executor on the virtual doc. That did not work out because I don't know how to set your cursor's position in a virtual doc and cell execution commands don't take positions as arguments.addresses posit-dev/positron#7709