-
Notifications
You must be signed in to change notification settings - Fork 92
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
OpenRPC for cursors/ranges rstudioapi shims #2582
Conversation
async modifyEditorLocations(locations: Range[], values: string[]): Promise<null> { | ||
const editor = this.editors.getActiveTextEditor(); | ||
if (!editor) { | ||
return null; |
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.
Should this throw an error?
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 don't think so (???) because similarly to here we don't know that there is an active text editor currently. Maybe when we address #2571 we should also look at what gets returned when there is no active editor.
If you have no active editor in RStudio, it uses the console, which are not handling at all yet. Dealing with this for ranges is analogous to us eventually handling id
here.
I think we probably have some more work to do here, around multi-byte characters. Here's my manual test:
We don't pass this test right now if, for example, the selection contains a emoji. RStudio does pass this test (although some of the numbers you see for positions are different). I think we want this sort of self-consistency, i.e. that we handle positions symmetrically for reading and setting. My hunch is that PR needs to gain this sort of maneuver, but going the other way: positron/src/vs/workbench/api/common/positron/extHostMethods.ts Lines 137 to 168 in 7a3d14b
Here's the basis of my manual test:
It's a little fiddly to get setup but here's what I do:
|
Intent
Addresses #1312 with support for:
insertText()
/modifyRange()
setCursorPosition()
/setSelectionRanges()
Related to posit-dev/ark#275
Approach
Chugging along with more rstudioapi shims, as before 🚂
QA Notes
Make a new R file with this content:
Now run the rstudioapi code, line by line.
Notice that we handle wonky text quite well, with the possible exception, depending on your perspective, of an Emoji ZWJ Sequence. Like in #2132, to do any better, the whole toolchain would need an awareness it does have not of unicode normalization. I say this is good enough for the time being.