Remove editorOpenWith #116856

lramos15 · 2021-02-17T15:05:19Z

This PR outlines part of the solution to #116272.

This removes the use of openWith and adds additional functionality to openEditor.

The next steps are to: (@bpasero feel free to correct me if I'm wrong 😄 )

Make the editor service aware of editor associations
Update custom editor inputs to no longer be async and add custom editor input functionality to createEditorInput
Remove the editorGroups' understanding of what an override is and just expect the correct input to be passed in.
Adopt this for other "custom editors" such as the search editor.

bpasero

I glanced over the changes and left some feedback. I think large chunks of code such as IEditorService#openEditorWith would simplify much once we have a way to return custom editor inputs from the createEditorInput flow. In my mind, the only logic editor service should have is to know which editor input to create based on overrides and picks. But that is a follow up step to consider as we have already discussed.

Besides, there are some types and interfaces I fail to find a good place for. I don't really understand the IConfigurationNode for custom editors, nor why schemas have to change. All of this stuff should really not be inside the editor service but in some registry outside. Same goes for constants defined in editor.ts.

I think my biggest issue in this area now is that I see code and concepts that I have not seen before (because I did not author them), making it hard for me the reason about them.

Ideally we can clean this up with existing patterns we have 👍

src/vs/workbench/contrib/files/browser/fileCommands.ts

src/vs/platform/editor/common/editor.ts

src/vs/workbench/common/editor.ts

src/vs/workbench/services/editor/browser/editorService.ts

Co-authored-by: Benjamin Pasero <benjamin.pasero@gmail.com>

- OverrideOptions => EditorOverride - It is safe to destructure (...) undefined - Ensure when destructuring, override is always winning - Not a big fan of shared builtinProviderDisplayName import

This moves the relevant code out of the editor service.

bpasero · 2021-02-21T10:43:03Z

src/vs/workbench/services/editor/browser/editorService.ts

@@ -570,7 +570,7 @@ export class EditorService extends Disposable implements EditorServiceImpl {
 						}

 						const fileEditorInput = this.createEditorInput({ resource, forceFile: true });
-						const textOptions: IEditorOptions | ITextEditorOptions = options ? { ...options, override: OverrideOptions.DISABLED } : { override: OverrideOptions.DISABLED };
+						const textOptions: IEditorOptions | ITextEditorOptions = { ...options, override: EditorOverride.DISABLED };


@lramos15 fyi destructuring even undefined is fine

bpasero · 2021-02-21T10:50:21Z

@lramos15 @mjbvz ok, I went over all the code and pushed changes to align with existing patterns. Please do a thorough review and testing of the following changes:

293664b moves all of the previous IEditorService#registerCustomEditorViewTypesHandler into a static IEditorAssociationsRegistry. I did not fully understand why the editor service would be responsible for managing and updating the workbench.editorAssociations configuration, imho that is a very static concept that should behave like registering an editor to a registry. I also removed the notion of viewType from there. What is a viewType in the context of an editor? I renamed to editorType
f7b7f94 rewrites entirely the resolution of an editor override if one has to be picked. It clearly distinguishes between picking an override and finding one from a string that was passed in. There was additional complexity due to the fact that the provided group and options can change depending on how the user interacts with the picker
8da2cec makes sure that the editor delegate still works, even with overrides

I will file follow up debt issues for related things I noticed that are beyond the scope of this PR.

bpasero · 2021-02-21T10:52:35Z

src/vs/workbench/services/editor/browser/editorService.ts

+		}
+
+		// Collect all overrides for resource
+		const allEditorOverrides = this.getEditorOverrides(resource, undefined, undefined);


Should this call forward the options and group from the openEditor call or why is undefined explicitly passed in?

I haven no idea, it didn't seem to change anything for me. Maybe @mjbvz knows

bpasero · 2021-02-22T07:56:51Z

Commenting on your proposed next steps:

Make the editor service aware of editor associations

Yeah possibly. However in #117207 I noticed that actually only custom editors seem to use workbench.editorAssociations so I think we should have a dedicated discussion where this ends up being moved to. Maybe we end up moving workbench.editorAssociations out to custom editors only and leave a very simply solution in the editor service that converts a resource to an editor input.

Update custom editor inputs to no longer be async and add custom editor input functionality to createEditorInput

I reported #117208 to look into that. If we have the concept of letting the user pick a custom editor as part of the open editor flow, we probably have to make createEditorInput async, at least for those cases. As such, maybe createEditorInput could be sync in most cases but async in some, based on editor options passed in.

Remove the editorGroups' understanding of what an override is and just expect the correct input to be passed in.

Yes, 💯 , and that is #100281

Adopt this for other "custom editors" such as the search editor.

Yes, maybe also settings editor.

lramos15 · 2021-02-22T17:01:18Z

This looks a lot cleaner for me. Thank you to @bpasero for putting the pieces together that I couldn't quite figure out. I will utilize debt week to improve on this more based on the above comments.

mjbvz

Looks good overall to me. Nice work!

src/vs/platform/editor/common/editor.ts

mjbvz · 2021-02-17T17:53:11Z

src/vs/workbench/contrib/customEditor/common/contributedCustomEditors.ts

 export const defaultCustomEditor = new CustomEditorInfo({
-	id: DEFAULT_EDITOR_ID,
+	id: 'default',


[nit] Why does this constant need to be inlined?

It breaks layering, as everywhere else this is used is in browser

src/vs/workbench/services/editor/browser/editorService.ts

mjbvz · 2021-02-22T20:29:43Z

src/vs/workbench/contrib/customEditor/common/contributedCustomEditors.ts

 	displayName: nls.localize('promptOpenWith.defaultEditor.displayName', "Text Editor"),
-	providerDisplayName: builtinProviderDisplayName,
+	providerDisplayName: nls.localize('builtinProviderDisplayName', "Built-in"),


[nit] Try extracting this string since it it used in two places

That is probably on me, I wasn't sure about the benefit of sharing NLS strings here. If we have a concept of built-in custom editors, shouldn't we rather have a flag to indicate as such and then maybe put the label in one central place?

lramos15 added 2 commits February 17, 2021 09:26

Move editorOpenWith to the editorService

f1024f8

Remove custom editor input handling

773cb0f

lramos15 requested review from bpasero and mjbvz February 17, 2021 15:05

lramos15 mentioned this pull request Feb 17, 2021

deleted editorOpenWith #116494

Closed

lramos15 self-assigned this Feb 17, 2021

lramos15 added the debt Code quality issues label Feb 17, 2021

lramos15 and others added 4 commits February 17, 2021 16:00

Merged changes

6cae5f3

Fix open with API command

60889c5

Merge remote-tracking branch 'upstream/main' into editorServiceOpenWith

eac3a48

adopt master properly

6138c86

bpasero requested changes Feb 18, 2021

View reviewed changes

lramos15 and others added 9 commits February 18, 2021 10:18

Remove dnagling comment

399b72e

Address some feedback

bf01999

Cleanup region comment.

5b0dc61

Co-authored-by: Benjamin Pasero <benjamin.pasero@gmail.com>

Cleanup region comment.

1cdc476

Co-authored-by: Benjamin Pasero <benjamin.pasero@gmail.com>

Update override typings

3cc1560

Merge branch 'main' into editorServiceOpenWith

b6aad40

Fix missing picker

88bd032

Fix bug with reopen with

e3cbe87

Remove duplicate import

bea6f08

bpasero self-requested a review February 20, 2021 09:25

bpasero added 7 commits February 20, 2021 10:28

Merge remote-tracking branch 'upstream/main' into editorServiceOpenWith

60bb8ba

First round of feedback via changes

e8b820b

- OverrideOptions => EditorOverride - It is safe to destructure (...) undefined - Ensure when destructuring, override is always winning - Not a big fan of shared builtinProviderDisplayName import

editorservice - static DEFAULT_EDITOR_OVERRIDE_ID

32360ff

editors - introduce a editor associations registry

293664b

This moves the relevant code out of the editor service.

cleanup editor picking

f7b7f94

cleanup editor delegate

8da2cec

final 💄

8694848

bpasero reviewed Feb 21, 2021

View reviewed changes

bpasero self-requested a review February 21, 2021 10:51

bpasero requested changes Feb 21, 2021

View reviewed changes

This was referenced Feb 22, 2021

Duplicate open with logic #117206

Closed

Workbench editor associations should be a generic editor concept or move to custom editors entirely #117207

Closed

Revisit custom editor input factory #117208

Closed

Merge remote-tracking branch 'upstream/main' into editorServiceOpenWith

78c5aff

lramos15 mentioned this pull request Feb 22, 2021

Test: Editor open and editor open with regression testing #117279

Closed

3 tasks

mjbvz approved these changes Feb 22, 2021

View reviewed changes

lramos15 merged commit 413963c into microsoft:main Feb 22, 2021

github-actions bot locked and limited conversation to collaborators Apr 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove editorOpenWith #116856

Remove editorOpenWith #116856

lramos15 commented Feb 17, 2021

bpasero left a comment •

edited

Loading

bpasero Feb 21, 2021

bpasero commented Feb 21, 2021

bpasero Feb 21, 2021

lramos15 Feb 22, 2021 •

edited

Loading

bpasero commented Feb 22, 2021

lramos15 commented Feb 22, 2021

mjbvz left a comment

mjbvz Feb 17, 2021

lramos15 Feb 22, 2021 •

edited

Loading

mjbvz Feb 22, 2021

bpasero Feb 23, 2021

Remove editorOpenWith #116856

Remove editorOpenWith #116856

Conversation

lramos15 commented Feb 17, 2021

bpasero left a comment • edited Loading

Choose a reason for hiding this comment

bpasero Feb 21, 2021

Choose a reason for hiding this comment

bpasero commented Feb 21, 2021

bpasero Feb 21, 2021

Choose a reason for hiding this comment

lramos15 Feb 22, 2021 • edited Loading

Choose a reason for hiding this comment

bpasero commented Feb 22, 2021

lramos15 commented Feb 22, 2021

mjbvz left a comment

Choose a reason for hiding this comment

mjbvz Feb 17, 2021

Choose a reason for hiding this comment

lramos15 Feb 22, 2021 • edited Loading

Choose a reason for hiding this comment

mjbvz Feb 22, 2021

Choose a reason for hiding this comment

bpasero Feb 23, 2021

Choose a reason for hiding this comment

bpasero left a comment •

edited

Loading

lramos15 Feb 22, 2021 •

edited

Loading

lramos15 Feb 22, 2021 •

edited

Loading