force preview editor on quick search navigation

[andreamah]

Fixes #205136
Related to #204853
cc @bpasero

[rzhao271]

I'm worried that if a user already specified that they want preview off, that they might actually want all the results to remain. There are also several enable preview settings, so I wonder whether using or creating a setting like that could work instead.

[andreamah]

That's true. Thoughts @bpasero @TylerLeonhardt ? I know there's a setting for quick open- I wonder if we could follow that or create a new setting? But at the same time, many settings might get confusing.

[lramos15]

I've requested Ben's review on this just to prevent a random review from occurring. Adding something to the core editor type is always a big change. I'm surprised there's not an editor option that can accomplish what we're looking for here.

[andreamah]

After talking to Logan about this, he suggested that we just change pinned: boolean in the editor options to pinned: boolean | 'forced' instead of adding a forcePreview option. Not sure which choice is preferred.

[bpasero]

Yeah I like the idea of forced if we can do it easily. Also please consider to add tests for this new behaviour.

One thing to keep in mind is that if a user has disabled preview editors, we should make sure that no preview editors stick around after leaving quick search.

[andreamah]

One thing to keep in mind is that if a user has disabled preview editors, we should make sure that no preview editors stick around after leaving quick search.

I already made sure that there were no preview editors on esc from the caller. I can also make sure that closing the pick by focusing on the editor will also make it a non-preview editor. Do you think that that's sufficient, or would there need to be more on the callee (OpenEditor) side?

[bpasero]

Yeah I think if the picker closes its a good moment to reset any preview state. But that is also the time to restore the previous UI state, so I guess its all part of it anyway.

[andreamah]

I made the argument pinned?: boolean | 'forcedDisable';, since forced seems like it's forcing it to pin, whereas it's actually forcing it to be unpinned. There was a little more making sure that pinned converted correctly when using other types, so you can take a look and see if that's what you prefer or whether we should try something else. I also added some fairly basic tests.

Also, I created #205236 to track making sure that all preview editors are closed after using quick search.

[bpasero]

I just had an idea how to do this alternatively: we allow to temporarily override user settings via IEditorGroupService.mainPart.enforcePartOptions. It returns a IDisposable to clean up when done and allows you to enforce options such as enablePreview: true. I think it would even take care of removing any preview state once you dispose it. Can you see if that works? Sorry I only figured this out now...

[andreamah]

I just had an idea how to do this alternatively: we allow to temporarily override user settings via IEditorGroupService.mainPart.enforcePartOptions. It returns a IDisposable to clean up when done and allows you to enforce options such as enablePreview: true. I think it would even take care of removing any preview state once you dispose it. Can you see if that works? Sorry I only figured this out now...

I don't think that that would work in my case, because once I dispose the enforcePartOptions IDisposable, all preview editors become non-preview. Since you can use a picker without it being in focus (workbench.quickOpen.closeOnFocusLost), we can't just leave enablePreview on the whole time while the picker is open. Therefore, I would be doing

> enablePreview: true
> await openEditor
> dispose such that enablePreview: false

every time I want to open an editor, but this turns into

> enablePreview: true
> await openEditor
> dispose such that enablePreview: false
> **all preview editors become non-preview, removing all of the work done to create a preview editor**

[bpasero]

Are we able to detect this case and dispose the enforced editor part options? And then are we able to detect that focus goes back to the picker to enforce it again? If so, I am not too worried about that behaviour. You could argue that if the user leaves the picker open, the user might want to actually use the editor that was opened as preview and then making it non-preview seems OK to me.

I am a bit sceptical about the current approach here forcing everyone that deals with editor options to use the new utility method, so I was hoping for a simpler change.

[andreamah]

I have created a 'layered' disposable that where a 'local' disposable will drive turning on/off enablePreview and the global on/off will actually stop the trigger of "onDidChangeEditorPartOptions" when enabled.

So then the process looks more like

start preview session (globalDisposable initialize)
actually force enablePreview to be true (localDisposable initialize)
await openEditor(<FooEditor>)
localDisposable dispose

**things are still in preview mode if they were opened when localDisposable was active, therefore, FooEditor is still in preview

globalDisposable dispose

**everything back to normal

[andreamah]

Closing in favour of: #205530