Set context keys for document language #608

juliasilge · 2024-11-22T03:47:20Z

This is a revamp of the work in #607.

Unfortunately after I started trying to use this in Positron, it just wasn't early enough or quick enough to set these context keys in languageAtPosition(). I believe that only gets called if we are trying to do one of these:

quarto/apps/vscode/src/vdoc/vdoc.ts

Lines 114 to 121 in d9d1515

    
           export type VirtualDocAction = 
        
             "completion" | 
        
             "hover" | 
        
             "signature" | 
        
             "definition" | 
        
             "format" | 
        
             "statementRange" | 
        
             "helpTopic";

It's just not enough, especially if you are switching back and forth between different files; it's very easy to get in a state where you end up with R shortcuts in Python chunks and there are lots of kinds of edits I could make that did not trigger one of these.

New idea! This PR tries out setting these context keys when we paint the background. In my experience so far, this feels a lot better, and we already are identifying all the tokens so not much extra work.

juliasilge · 2024-11-22T03:59:54Z

Related to posit-dev/positron#5451

DavisVaughan · 2024-11-26T14:33:58Z

apps/vscode/src/providers/background.ts

@@ -162,6 +163,10 @@ async function setEditorHighlightDecorations(
      blockRanges.push(vscRange(block.range));


What if highlightingConfig isn't enabled? See the if-block above.

(See broader comment about using a different approach)

DavisVaughan · 2024-11-26T14:46:05Z

apps/vscode/src/providers/background.ts

+    // expose cell language for use in keybindings, etc
+    const language = mainLanguage(tokens);
+    vscode.commands.executeCommand('setContext', 'quartoLangId', language?.ids[0]);
+


I do not feel it is appropriate to set the context here

Currently this function is quite pure. It is just about setting highlight decorations and is very easy to understand.

If we take a look at why it works to put this here, its because this is called from

triggerUpdateAllEditorsDecorations(), which is called during the onDidChangeVisibleTextEditors() hook

triggerUpdateActiveEditorDecorations(), which is called during the onDidOpenTextDocument() hook

That hook registration is part of activateBackgroundHighlighter(), which is called in activateCommon().

I think what we really want is:

In activateCommon(),

Add a new activateLanguageContextSetter()

Which registers onDidOpenTextDocument() and onDidChangeVisibleTextEditors() hooks of its own, and those hooks set the quartoMainLanguageId of the main language for the document.

Yes you have to reparse the document, but I think that is worth it over muddying the waters about what setEditorHighlightDecorations() is about, plus it gives you finer control over when the context is updated.

This is a great idea! Implemented in c6f28f8 and working really well in posit-dev/positron#5451.

DavisVaughan · 2024-11-26T14:48:57Z

apps/vscode/src/providers/background.ts

@@ -162,6 +163,10 @@ async function setEditorHighlightDecorations(
      blockRanges.push(vscRange(block.range));
    }

+    // expose cell language for use in keybindings, etc
+    const language = mainLanguage(tokens);
+    vscode.commands.executeCommand('setContext', 'quartoLangId', language?.ids[0]);


I'd consider using a non-abbreviated and very descriptive name

quarto.document.mainLanguageId quarto.documentMainLanguageId quartoDocumentMainLanguageId

See "quarto.assistView.isEnabled" as a nice example

It's a little tricky because we have a mix of styles right now for context key naming. Sometimes we do quarto. sometimes we dont https://github.com/search?q=repo%3Aquarto-dev%2Fquarto%20setContext&type=code

I tend to like using the . as a way to namespace it to quarto

(also including main in the name is a way for us to guard against the possibility of us eventually actually getting a per-cell context key right)

Why not just quarto.document.languageId vs (eventually) something like quarto.cell.languageId?

document vs cell is an evocative distinction. main wants to contrast with secondary, and that's not what you're setting up to do in the future.

Works great for me!

DavisVaughan · 2024-11-26T14:51:57Z

apps/vscode/src/vdoc/vdoc.ts

@@ -157,12 +157,8 @@ export async function virtualDocUri(
 export function languageAtPosition(tokens: Token[], position: Position) {
  const block = languageBlockAtPosition(tokens, position);
  if (block) {


IIUC, previously it attempted to update the context key dynamically on each cell "activation", right? That does seem hard to get 100% right.

And now it tries to update the context key 1 time for whatever the main language in the document is.

I suppose that could be mildly confusing. If you have 1 python cell in a mostly R document, then clicking in the python cell and running a keybinding will run the R version. That's kinda confusing, but I'm not sure what we can do better.

I did hope for us to get this working really nicely for reticulate Quarto files (per cell), but when I tried to use the approach with languageAtPosition() in practice, it felt quite awful. So yep, I think for now this approach with the "main language" is the best option.

DavisVaughan

Looks great, just one main comment left about exactly when we should set the context key

DavisVaughan · 2024-11-26T19:16:51Z

apps/vscode/src/providers/context-keys.ts

+function triggerUpdateContextKeys(engine: MarkdownEngine) {
+  for (const editor of vscode.window.visibleTextEditors) {
+    setContextKeys(editor, engine);
+  }
+}


I think this is looking great!

My main comment is about this, and any other usage of visibleTextEditors

IIUC, the context key is "global". i.e. there is not a context key per document, there is exactly 1 context key set globally.

If that is the case, then the context key should probably only be set for the active text editor, not for all visible text editors (because only 1 wins!).

For example, if I open 2 qmds side by side in split panes, one that is mostly R and one that is mostly Python, then I'm not entirely sure which is going to win here because both are "visible". However, only one at a time is ever "active", and I feel like that is probably what we should be targeting with this context key.

Is it possible all we really need to do is having a single hook for onDidChangeActiveTextEditor()?

Does that make any sense?

Makes total sense, yep. Changed in 8d00a51.

I added the hook for onDidChangeTextDocument() (with some debounce) for the case where someone changes their document from being Python to R or vice versa. That is certainly rare, but that case does feel fairly broken if we don't include it.

You are correct that we don't need onDidOpenTextDocument() if we have onDidChangeActiveTextEditor(). 👍

I added the hook for onDidChangeTextDocument()

Makes sense because you also dont have a "main" language at the start of a document until you actually start typing and adding some chunks (or maybe there is a default, but it likely isnt right)

PR looks great nice job!

…ctiveTextEditor()`

juliasilge · 2024-11-26T21:06:28Z

I installed the final .vsix from this PR and ran through all my normal Quarto moves, both with a current release build of Positron, dev build from posit-dev/positron#5451, and VS Code. Behaves as expected and looks good!

Addresses #1955 together with quarto-dev/quarto#608 The Quarto PR provides new context keys for the main language of a Quarto document (for example, R or Python) and we can consume those to provide keyboard shortcuts. There are really only a couple of R ones that we want right now which means that there is no real change here in an R `.qmd` in behavior compared to what is in the RStudio Keymap. 🙈 However, I think this is still worth getting in since we've gone to the trouble of figuring it out, it lets us offer these without the RStudio Keymap being on, we can offer them _only_ in R `.qmd` files (right now, these also show up in Python `.qmd` files if you have the RStudio Keymap on), and we can use this infrastructure in the future. I did remove these from the RStudio Keymap just to clean things up, but it wouldn't hurt _a lot_ to keep them, if there is some reason I am not thinking of? If we do want to remove them, we'll need to do a Quarto release and update the bundled Quarto VS Code extension version in this PR to keep these keybindings functional. ### QA Notes With the new version of the Quarto extension (updated here), you can use the keyboard shortcuts for the pipe and assignment operator in R Quarto documents but not Python ones: - <kbd>Alt</kbd>+<kbd>-</kbd> to get you `<-` - <kbd>Cmd/Ctrl</kbd>+<kbd>Shift</kbd>+<kbd>M</kbd> to get you `|>`

juliasilge added 2 commits November 21, 2024 20:41

Set context keys for cell language when background is painted

11d1830

Update changelog

35ad78a

juliasilge mentioned this pull request Nov 22, 2024

Use new Quarto context keys in keyboard shortcuts posit-dev/positron#5451

Merged

juliasilge requested a review from cscheid November 22, 2024 03:58

juliasilge requested a review from DavisVaughan November 24, 2024 22:47

DavisVaughan requested changes Nov 26, 2024

View reviewed changes

juliasilge added 3 commits November 26, 2024 10:53

Merge branch 'main' into revamp-quarto-lang-context-keys

f1aa3fb

Do not set context keys along with painting background

4a34c1f

Add new activateContextKeySetter()

c6f28f8

juliasilge requested a review from DavisVaughan November 26, 2024 19:01

juliasilge changed the title ~~Set context keys for cell language when background is painted~~ Set context keys for document language Nov 26, 2024

DavisVaughan approved these changes Nov 26, 2024

View reviewed changes

juliasilge added 2 commits November 26, 2024 13:47

Context keys only apply to *active* documents

8d00a51

We don't need onDidOpenTextDocument() because we have `onDidChangeA…

a3535e0

…ctiveTextEditor()`

juliasilge merged commit dca4735 into main Nov 26, 2024
1 check passed

juliasilge deleted the revamp-quarto-lang-context-keys branch November 26, 2024 21:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set context keys for document language #608

Set context keys for document language #608

juliasilge commented Nov 22, 2024

juliasilge commented Nov 22, 2024

DavisVaughan Nov 26, 2024 •

edited

Loading

DavisVaughan Nov 26, 2024

juliasilge Nov 26, 2024

DavisVaughan Nov 26, 2024 •

edited

Loading

cscheid Nov 26, 2024

DavisVaughan Nov 26, 2024

DavisVaughan Nov 26, 2024

juliasilge Nov 26, 2024

DavisVaughan left a comment

DavisVaughan Nov 26, 2024

juliasilge Nov 26, 2024

juliasilge Nov 26, 2024 •

edited

Loading

juliasilge Nov 26, 2024

DavisVaughan Nov 26, 2024

juliasilge commented Nov 26, 2024

	export type VirtualDocAction =
	"completion" \|
	"hover" \|
	"signature" \|
	"definition" \|
	"format" \|
	"statementRange" \|
	"helpTopic";

		@@ -162,6 +163,10 @@ async function setEditorHighlightDecorations(
		blockRanges.push(vscRange(block.range));

Set context keys for document language #608

Set context keys for document language #608

Conversation

juliasilge commented Nov 22, 2024

juliasilge commented Nov 22, 2024

DavisVaughan Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavisVaughan Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavisVaughan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliasilge Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliasilge commented Nov 26, 2024

DavisVaughan Nov 26, 2024 •

edited

Loading

DavisVaughan Nov 26, 2024 •

edited

Loading

juliasilge Nov 26, 2024 •

edited

Loading