Automatically open QMD files in visual/source editor if specified #577

isabelizimm · 2024-10-18T21:24:46Z

as part of posit-dev/positron#2933

Should have the following behavior:

if editor: XXXX is specified in yaml at top of file, use that
if no editor in file yaml, but editor: XXXX is specified in _quarto.yml/_quarto.yaml, use that
otherwise, use configurable default editor

DavisVaughan

Some drive by thoughts

apps/vscode/src/providers/editor/toggle.ts

apps/vscode/src/main.ts

DavisVaughan · 2024-10-22T18:03:34Z

apps/vscode/src/providers/editor/toggle.ts

+  const viewType = config === 'visual' ? VisualEditorProvider.viewType : 'textEditor';
+  vscode.workspace.getConfiguration('workbench').update('editor.defaultView', viewType, true);
+  await vscode.commands.executeCommand("workbench.action.setDefaultEditor",
+    vscode.Uri.file('filename.qmd'),


i think this could be replaced as "*.qmd" if we instead expose updateUserAssociations from the Positron side

Actually I wonder if we could expose updateUserAssociations() with the slightly better name of setEditorAssociations(), but then expose it from positron.d.ts rather than as a command (either in the top level 'positron' namespace, or under a new 'workbench' sub namespace, mirroring what VS Code has. That would work like our existing 'language' and 'runtime' sub namespaces).

That would probably be nice because it would force you to look at Quarto's hasHooks() and hooksApi() helpers to determine whether or not you are in positron (we use that in a few other places too)

(FWIW saveAllTitled had to be a command to be able to call it from R, but this feels more like an Extension API feature that can be a "real" function exported from positron.d.ts)

apps/vscode/src/main.ts

apps/vscode/package.json

apps/vscode/src/providers/editor/toggle.ts

DavisVaughan · 2024-10-22T20:22:58Z

apps/vscode/src/providers/editor/toggle.ts

+  const viewType = config === 'visual' ? VisualEditorProvider.viewType : 'textEditor';
+  vscode.workspace.getConfiguration('workbench').update('editor.defaultView', viewType, true);
+  await vscode.commands.executeCommand("workbench.action.setDefaultEditor",
+    vscode.Uri.file('filename.qmd'),


Actually I wonder if we could expose updateUserAssociations() with the slightly better name of setEditorAssociations(), but then expose it from positron.d.ts rather than as a command (either in the top level 'positron' namespace, or under a new 'workbench' sub namespace, mirroring what VS Code has. That would work like our existing 'language' and 'runtime' sub namespaces).

That would probably be nice because it would force you to look at Quarto's hasHooks() and hooksApi() helpers to determine whether or not you are in positron (we use that in a few other places too)

(FWIW saveAllTitled had to be a command to be able to call it from R, but this feels more like an Extension API feature that can be a "real" function exported from positron.d.ts)

apps/vscode/src/providers/editor/toggle.ts

DavisVaughan · 2024-10-22T20:40:49Z

apps/vscode/src/main.ts

+
+  const documentOpenHandler = vscode.window.onDidChangeActiveTextEditor(async () => {
+    // Check if the document language is "quarto"
+


It seems like we don't have things quite right in this changing active text editor listener

A diff editor view of a quarto document immediately closes

Screen.Recording.2024-10-22.at.4.37.39.PM.mov

I can't seem to go from Visual Mode to Source Mode explicitly

Screen.Recording.2024-10-22.at.4.37.57.PM.mov

Co-authored-by: Davis Vaughan <davis@posit.co>

DavisVaughan · 2024-10-28T14:22:04Z

apps/vscode/package.json

+        "quarto.defaultEditor": {
+          "order": 11,
+          "type": "string",
+          "markdownDescription": "Default editor. Only applicable in Positron.",
+          "enum": [
+            "source",
+            "visual"
+          ],
+          "default": "source"
+        },


Can remove now?

apps/vscode/package.json

apps/vscode/src/providers/editor/configurations.ts

apps/vscode/src/main.ts

apps/vscode/src/providers/editor/editor.ts

juliasilge · 2024-10-29T20:54:49Z

~~I got the .vsix from here to test out the behavior and unfortunately 😬 I see:~~

~~command 'quarto.editInVisualMode' not found~~

~~Might something have gone wrong with how the command is registered?~~

Never mind! Needed to restart the extension host.

DavisVaughan · 2024-10-29T16:09:59Z

apps/vscode/src/providers/editor/editor.ts

+      if (t.closed.length > 0) { return; }
+      const tabs = t.changed.length > 0 ? t.changed : t.opened;


I'm not sure this is quite right, because I don't think changed, closed, and opened are mutually exclusive.

i.e. i think we can get a tab change event with both newly opened and newly changed tabs in it

So maybe these two lines really need to just be const tabs = [t.changed, t.opened] (or whatever the syntax is to combine two arrays of Tabs together. We'd just ignore closed tabs i guess.

Hmmm... I think we can just target opened tabs, and ignore tab changes. We will presume that the editor should stay the same as the user leaves it, eg, if you open something in source mode, go to another tab, update a _quarto.yml to have a default as visual, then going back to the original file should still be source (unless they close and reopen it).

opened tabs are never considered active, but I think if we're only looking at newly opened tabs, we can always treat them as if they are. That should eliminate the switching error you're seeing as well.

I played around with this for awhile and i think the problem is going to be that at open time the tabs aren't in the window.tabGroup array yet. You have to wait for them to become isActive first, which would come in a change event.

apps/vscode/src/providers/editor/editor.ts

DavisVaughan · 2024-10-29T16:20:23Z

apps/vscode/src/providers/editor/editor.ts

+            const fileContent = Buffer.from(fileData).toString('utf8');
+            const editorMode = determineMode(fileContent, uri);
+            let isSwitch = this.visualEditorPendingSwitchToSource.has(uri.toString()) || this.editorPendingSwitchToVisual.has(uri.toString());
+            this.editorPendingSwitchToVisual.delete(uri.toString());


Gosh, it would be super nice if we also managed this.visualEditorLastSourcePos.delete(uri); here. It's a bit strange that we only .delete one of the two here.

It is currently managed by onDidChangeActiveTextEditor() down below, but I'm wondering if we just didn't know about the Tabs API at the time that hook was written.

I think it would be pretty straightforward to remove the onDidChangeActiveTextEditor() hook and put its code here, we are already doing a lot of the same stuff!

We know:

That we are on the active document

That we are in a quarto document

Whether or not we are switching or not

So really after this branch:

if (editorMode && editorMode != viewType && !isSwitch) {

it looks like we could just add another branch of

if (isSwitchToSource) { // you'd need to split out isSwitch into isSwitchToSource and isSwitchToVisual

and that would handle all the pos related code you see below about putting the cursor in the right place when we switch into source mode from visual mode (where we were tracking the editor position for this exact purpose).

And then you get to remove onDidChangeActiveTextEditor entirely

I'm not sure if we would be able to move everything super cleanly; Tabs don't have access to a TextEditor AFAICT, which is how the user is navigated to the location

ah good point

apps/vscode/src/providers/editor/editor.ts

apps/vscode/src/main.ts

apps/vscode/src/providers/editor/editor.ts

DavisVaughan

A few more small bugs but looks good enough to approve now - the timing of all this stuff is so confusing!

Don't forget a CHANGELOG bullet!

apps/vscode/src/providers/editor/editor.ts

DavisVaughan · 2024-10-30T15:01:52Z

apps/vscode/src/providers/editor/editor.ts

+            }
+
+            if (editorMode && editorMode != viewType && !isSwitch) {
+              const allTabs = window.tabGroups.all?.[0]?.tabs;


I think we need a big meaningful comment here, because this is (at first glance) really weird that we have to do this.

In my experience playing around with this if you set await window.tabGroups.close(tab, true); using the tab you get from the change event, it won't close and you get a "tab does not exist" error in the debug console.

From what I can tell, the tab we get in the change event is not PRECISELY equal to the tabs that exist in window.tabGroups at this point in time. It's possible that during this opened event they are a little out of sync. The tab we care about closing does exist in there, we just have to find it by matching by URI - something we know is stable across change events.

So I think it looks like this:

// Unfortunately we cannot just `await window.tabGroups.close(tab, true);`. // The `tab` we get from the change event seems to not be precisely the same // as the tab in `window.tabGroups`, so if we try and close `tab` we get a "tab // not found" error. The one we care about does exist in `window.tabGroups`, // we just have to manually find it using the URI as a stable field to match on.

DavisVaughan · 2024-10-30T15:03:25Z

apps/vscode/src/providers/editor/editor.ts

+              // find tab to close if swapping editor type. we don't want to close an active
+              // tab since a tab we are opening has not been set as active yet. we also don't
+              // want to close preview tabs since they will automatically be overriden
+              const tabsToClose = allTabs.filter(tab =>


I believe we are expecting exactly 1 tab to match here, so you could use find() instead of filter() and only do the close/open dance when we find() a tab, like

const tabToClose = allTabs.find(stuffhere); if (tabToClose) { await window.tabGroups.close(tabToClose, true); await commands.executeCommand("vscode.openWith", uri, editorMode); }

That might also be slightly safer in some weird edge cases. Like with the "double open" bugs I've reported in this review, we would not have actually opened the visual editor if we programmed it this way, because tabToClose would have been undefined and the entire branch would not have run (it still would have been a bug on our part, just a slightly less bad one).

DavisVaughan · 2024-10-30T15:06:17Z

apps/vscode/src/providers/editor/editor.ts

+    context.subscriptions.push(window.tabGroups.onDidChangeTabs(async (t) => {
+      const tabs = t.opened;
+
+      if (tabs.length > 0) {


can still flatten this if you want

Co-authored-by: Davis Vaughan <davis@posit.co>

auto open in default or specified viewer

8fbcbfc

isabelizimm mentioned this pull request Oct 18, 2024

new command to set default editor opener type posit-dev/positron#5075

Closed

clean up for clarity

ed97a28

DavisVaughan reviewed Oct 22, 2024

View reviewed changes

apps/vscode/src/providers/editor/toggle.ts Outdated Show resolved Hide resolved

apps/vscode/src/providers/editor/toggle.ts Outdated Show resolved Hide resolved

DavisVaughan reviewed Oct 22, 2024

View reviewed changes

apps/vscode/src/providers/editor/toggle.ts Outdated Show resolved Hide resolved

apps/vscode/src/providers/editor/toggle.ts Show resolved Hide resolved

apps/vscode/src/main.ts Outdated Show resolved Hide resolved

DavisVaughan reviewed Oct 22, 2024

View reviewed changes

use onDidChangeActiveTextEditor

cd181b4

isabelizimm marked this pull request as ready for review October 22, 2024 18:53

DavisVaughan reviewed Oct 22, 2024

View reviewed changes

apps/vscode/src/main.ts Outdated Show resolved Hide resolved

apps/vscode/src/main.ts Outdated Show resolved Hide resolved

DavisVaughan reviewed Oct 22, 2024

View reviewed changes

apps/vscode/src/providers/editor/toggle.ts Show resolved Hide resolved

DavisVaughan reviewed Oct 22, 2024

View reviewed changes

isabelizimm and others added 7 commits October 22, 2024 22:20

Apply suggestions from code review

66aaea2

Co-authored-by: Davis Vaughan <davis@posit.co>

changes from review

d2d8bcd

move listener to VisualEditorProvider

5d88ec7

only have one editor listener

88c00c5

check if this is a diff

d9bea7d

track editor associations

6824712

fix typing error

9e27565

DavisVaughan reviewed Oct 28, 2024

View reviewed changes

update from review

a80bf80

juliasilge mentioned this pull request Oct 28, 2024

Update formatting for .ts and .json files in apps/vscode/ #590

Merged

isabelizimm added 3 commits October 28, 2024 18:50

move everything to Tab API

dad050a

clean up unused code

0cf2ea6

linting

d5ec9c9

DavisVaughan reviewed Oct 29, 2024

View reviewed changes

isabelizimm added 2 commits October 29, 2024 18:16

only watch opened tabs

d0a65d7

preserve focus on tab swaps

0660b46

DavisVaughan approved these changes Oct 30, 2024

View reviewed changes

isabelizimm and others added 2 commits October 30, 2024 12:42

Apply suggestions from code review

fed009c

Co-authored-by: Davis Vaughan <davis@posit.co>

add comments

14673be

isabelizimm merged commit 227af78 into main Oct 30, 2024
1 check passed

isabelizimm deleted the save-editor-type branch October 30, 2024 17:48

This was referenced Oct 31, 2024

update CHANGELOG #595

Merged

Quarto: Add an API to set preferred editor (for respecting visual mode preference) posit-dev/positron#2933

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automatically open QMD files in visual/source editor if specified #577

Automatically open QMD files in visual/source editor if specified #577

isabelizimm commented Oct 18, 2024 •

edited

Loading

DavisVaughan left a comment

DavisVaughan Oct 22, 2024

DavisVaughan Oct 22, 2024

DavisVaughan Oct 22, 2024

DavisVaughan Oct 22, 2024

DavisVaughan Oct 28, 2024

juliasilge commented Oct 29, 2024 •

edited

Loading

DavisVaughan Oct 29, 2024

isabelizimm Oct 29, 2024

DavisVaughan Oct 29, 2024

DavisVaughan Oct 29, 2024

isabelizimm Oct 29, 2024

DavisVaughan Oct 29, 2024

DavisVaughan left a comment

DavisVaughan Oct 30, 2024

DavisVaughan Oct 30, 2024

DavisVaughan Oct 30, 2024

DavisVaughan Oct 30, 2024


		const documentOpenHandler = vscode.window.onDidChangeActiveTextEditor(async () => {
		// Check if the document language is "quarto"

		if (t.closed.length > 0) { return; }
		const tabs = t.changed.length > 0 ? t.changed : t.opened;

Automatically open QMD files in visual/source editor if specified #577

Automatically open QMD files in visual/source editor if specified #577

Conversation

isabelizimm commented Oct 18, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliasilge commented Oct 29, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isabelizimm commented Oct 18, 2024 •

edited

Loading

juliasilge commented Oct 29, 2024 •

edited

Loading