Explore moving quick input dependency

[lramos15]

Fixes #173691

Please take a look at the TODO @bpasero @TylerLeonhardt it appears that we might be blocked on what to do when two editors with the same defaults conflict. We used to show the picker and have the user select which one they wanted, but that isn't possible. I tried to open the settings editor so they can manually pick, but this isn't possible either. While this case doesn't happen often I would like a nice flow. Should we just tell them to manually go to settings?

[bpasero]

The editor pane has some limited functionality to show a placeholder, we use this for errors for example:

So for that case we could actually show something meaningful to the user, we do not have to use a notification.

In order to do so, throw an IEditorOpenError that you can create from:

vscode/src/vs/workbench/common/editor.ts

Line 1535 in e1967e9

export function createEditorOpenError(messageOrError: string | Error, actions: IAction[], options?: IEditorOpenErrorOptions): IEditorOpenError {

The error needs to be thrown from the setInput call, but maybe we can expand this to work in the resolving case as well. Since the editor resolver is part of the editor component, that probably makes sense.

[lramos15]

But do we want to throw a big in your face error when we can open the file, it's just we're picking the editor type at random until the user configures what they want their default to be

[bpasero]

So how would it work then, you click on a file and a notification opens?

[lramos15]

So how would it work then, you click on a file and a notification opens?

That is correct, that's how it works currently if you have two default editors for the same file type. Dismissing the notification or selecting keep default would assume you like the choice we made. The case I'm stuck on is configuring a different choice if you don't like the case we picked.

[bpasero]

I think my immediate reaction would be: if I click on a file, I would expect an editor to open, and if I have to make a choice, I do it from there. The reason we added the large placeholder was partially also to prevent the flow of clicking in the explorer and having to hunt down the notifiction when no editor opens.

[lramos15]

Yeah I guess the difference here though is that the editor is fully functional and you could go about your work without ever seeing the notification. It might even be the editor you want, it's more how do we tell the user "Hey you have two defaults installed, that's a bit strange you should pick one". Maybe we do nothing and try not to hand hold in this case. Instead just show an unactionable warning and let the user figure out how to fix this? I don't think this case is super often in general, so maybe it's best not to overthink it.

[bpasero]

Oh sorry, now I get it: we do open an editor and additionally inform that there are other choices. In that case a notification makes good sense that opens settings on a button click 👍

[lramos15]

sense that opens settings on a button click 👍

But we cannot open settings due to a cyclical dependency with the preferences service. Before we used the quick input to show the editors you could pick and then we would use the config service to write it to your settings. To remove the IQuickInputService, I attempted to shift to the IPreferencesService to open the settings UI, but that also has a cyclical dependency. So I wonder how we can work around this.

[bpasero]

Yeah I see. Maybe we have to go down the path of executing a command here. But then we could as well have a command that shows quick pick...

[lramos15]

Yeah a command is exactly what @TylerLeonhardt is already basically doing to get around the cyclical dependency so it feels like it's just hiding the problem. I'm surprised this isn't a more common problem of a core service trying to prompt and react to a user change.

[bpasero]

I am also fine to not show any notification at all. There is the "Reopen with" flow that guides the user, so I think that is an option the user will always have.

[bpasero]

Only minor feedback, feel free to merge then.

[bpasero]

It's still a bit fishy that a service would expose a method where you have to pass in another service. If we want to go down this path then maybe you should rather do a instantiationService.invoke() call within and get to the needed service and add a TODO to extract that method to a new service such as IEditorResolverPickerService.

In other words, I think this approach just hides the dependency behind debt.

[lramos15]

TODO to extract that method to a new service such as IEditorResolverPickerService.

To me what is weird is how do other services normally handle reacting to state? This seems like a common pattern, but I guess a core service like the Editor or EditorResolverService is so deeply intertwined into the core of VS Code that it's dependencies need to be minor.

[bpasero]

I think there are different ways:

• build a new service that wraps around the functionality needed that is more service-heavy to be able to remove the heavy services from the core one
• just have a core model for the work that a UI component then can pick up to work with [1]

[1] for example notification service and dialog service work that way: you can super eager create notifications or dialogs and they will go into a non-UI queue only to be later processed by a UI component that is more service heavy.

[TylerLeonhardt]

@bpasero you thinking we could have a quickinput queue service? And internal services use that while the actual UI service relies on OpenerService?

[bpasero]

It is probably overkill here for this particular problem. Here I would probably consider having a new service specifically to bring up the editor picker.

[TylerLeonhardt]

@bpasero doesn't that have the same problem just with a longer change of dependencies?:

• IQuickInputService
• IOpenerService
• ICodeEditorService
• IEditorService
• IEditorResolverService
• IEditorResolverPickerService <-- NEW ✨
• IQuickInputService

[bpasero]

Hm no, the editor resolver service cannot depend on the new service. The new service breaks the connection from editor resolver to quick input service.

[vivodi]

Is this still relevant?