deleted editorOpenWith #116494
deleted editorOpenWith #116494
Conversation
| @@ -631,6 +631,13 @@ export class TestEditorGroupsService implements IEditorGroupsService { | |||
| export class TestEditorGroupView implements IEditorGroupView { | |||
|
|
|||
| constructor(public id: number) { } | |||
| preferredHeight?: number | undefined; | |||
There was a problem hiding this comment.
[nit] Do you need to add these properties? They look unrelated
| @@ -472,6 +472,14 @@ export interface IEditorGroup { | |||
| */ | |||
| openEditor(editor: IEditorInput, options?: IEditorOptions | ITextEditorOptions, context?: OpenEditorContext): Promise<IEditorPane | null>; | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
[nit] Move the JSDoc for the parameters to this interface
| /** | ||
| * Get the group to open the editor in by looking at the pressed keys from the picker. | ||
| */ | ||
| export function getTargetGroup( |
There was a problem hiding this comment.
[nit] I think this function is only used in one spot so let's move it to that file instead
| /** | ||
| * Get a list of all available editors, including the default text editor. | ||
| */ | ||
| export function getAllAvailableEditors( |
There was a problem hiding this comment.
[nit] same for this function. I think it is only used in openEditorWith
There was a problem hiding this comment.
Given a high level review feedback here before drilling into the code itself: I think openEditorWith should not be inside editorGroupView but rather be something I can pass to IEditorService#openEditor as an option. We already have the override property that I can pass to indicate which editor to use:
vscode/src/vs/workbench/common/editor.ts
Line 1012 in 833bae4
In my mental model, an editor group should really not be involved with figuring out which editor to use for a given input. The editor group already deals with typed EditorInput that have an exact mapping to 1 editor. I would have rather expected the editor service to convert to the correct EditorInput (from a { resource } untyped input) based on:
override- user settings (editor associations)
- extensions
| @@ -631,6 +631,13 @@ export class TestEditorGroupsService implements IEditorGroupsService { | |||
| export class TestEditorGroupView implements IEditorGroupView { | |||
|
|
|||
| constructor(public id: number) { } | |||
| preferredHeight?: number | undefined; | |||
|
Closing in favor of #116856 as I basically needed to restart so it was easier. |
This PR fixes #116272.
I have moved the openWith function into the IEditorGroup and all the extra functions and helpers into editor.ts. I still need to add the unsaved file dialog.
@bpasero I'm a bit confused as I feel parts of #116272 contradict #100821, specifically the portion about passing in an override option that shows the picker "we allow to pass in a special override option that will bring up a picker to pick a custom editor (this essentially moves the picker logic from openEditorWith into IEditorService". Would then openWith in the IEditorGroup not have any picker logic and instead be used just for opening a specific editor given an ID? Or would we say if there is no ID we pass that into the editorService to show the picker instead of the picker logic living within that function.