-
Notifications
You must be signed in to change notification settings - Fork 36
Always use a unique virtual document file path #688
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
Conversation
It is shared with the LSP Client
// create output channel for extension logs and lsp client logs | ||
const outputChannel = vscode.window.createOutputChannel("Quarto", { log: true }); | ||
|
||
outputChannel.info("Activating Quarto extension."); |
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 found it extremely useful to have an outputChannel
to log to while debugging. I plumbed this through to points of interest so I could log.
I have since removed the log calls, but I still think this is quite useful for future us so I've left the actual output channel in to make this easier later on.
It is shared with the LSP client, essentially we provide our own output channel now rather than having the LSP client generate one automatically for us (we do the same thing in positron)
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 have streamlined this function quite a bit. I found that pretty much everything was shared code, and we don't actually need an "early exit" path for the local
case - it's just the normal path tweaked to create the file in a known directory rather than a temp directory.
The diffs are a little large but I hope it is a big improvement overall
(PS, it is much easier to review this file in Split View in the github ui)
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.
Really the main actual change is that the local
branch that used to exist used a static .vdoc.{ext}
as the file path, which was being overwritten in the middle of an lsp request. It now uses .vdoc.{uuid}.{ext}
to avoid this.
* language server request is running (#683). | ||
*/ | ||
function generateVirtualDocFilepath(directory: string, extension: string): string { | ||
return path.join(directory, ".vdoc." + uuid.v4() + "." + extension); |
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.
Worth noting that for non-local
LSP requests this was called .intellisense.{uuid}.{ext}
but I do not think that name mattered. We now always use .vdoc.{uuid}.{extension}
.
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.
Very good fix, LGTM.
The only comment I have is that this fix makes me wonder about other data races, and a code review might be in order...
To get the fix in quarto-dev/quarto#688 that addresses the virtual document problems observed in quarto-dev/quarto#683 (reported here in Positron several times as well)
Closes #683
See full analysis in #683 (comment)
I have managed to reliably (or at least, quickly) reproduce the problem by repeatedly mashing
Cmd + S
while also moving my mouse around over the code in this qmd (i.e. simultaneously invoking many hover requests). I have not been able to reproduce the issue this way with this PR.