Show preview editors while navigating through quick open results

[XVincentX]

I think it would be really useful to open a temporary editor while searching with CMD+P and open the first file found in the list so I can have an immediate feedback without having to guess what's inside.

[XVincentX]

@bpasero Would you be interested in an user contribution for this one?

[bpasero]

@XVincentX sure why not. the tricky part is that there are many listeners for when editors change and you do not want to trigger the same actions depending on wether the editor change is from this preview navigation or a real editor opening. Mainly you do not want to push to all history stacks we have (history.ts). In other words, opening editors like this from quick open should not add to history, only when the decision is made and you picked one. I think this needs to be wired in end to end to make it work properly.

[XVincentX]

@bpasero
Sure, it makes totally sense to me.

I'll try to come up with something. I guess openAnythingHandler.ts is my starting point!

[XVincentX]

@bpasero

Hey,
Today I've tried to go into the source code and trying to understand what would be the effort to make this happen.

Basically I've started from openAnythingHandler.ts
From the exploration I made, I can see that

1. It wraps and "joins" results from openSymbolHandler and openFileHandler (even if it's possible to deactivate the former, but I didn't figure out where the preference is evaluated (yet))
2. It adds the range search :41 in extractRange function.

However, the QuickOpenHandler is generally missing any overridable event for onSelectionChange or something like that so if I understood it correctly the plan would be

1. Add a onSelectionChange to QuickOpenHandler (or find a better naming)
2. Figure out where to call this method (see below).
3. Open a temp editor and show it to the user (probably using IEditorService - but figure out how to not add an history entry (which is still dark to me). I can definitely reuse the default run method for EditorQuickOpenEntry but not sure where the history element is pushed

Selection change event

This seems to be the hardest part.
I've gone through the code and these are my findings
quickOpenController.ts should somehow call QuickOpenHandler.onSelectionChange - which, in turn, should receive an event from the HTML element. It seems like onKeyUp and onKeyDown on ViewItem class is already handled by this.context.controller. This are somehow forwarded to quickOpenWidget.ts where there are some configurable callbacks (IQuickOpenCallbacks). Adding these and forwarding the event might do the trick actually

Am I on the right track?
Thanks a lot!

[XVincentX]

It turns out to be a bit more complicated than I was expecting.

It seems like QuickOpenHandler is not supposed to handle such kind of events, but more specific type of items (Files, Symbols) and what to open when user clicks on it.

I'm restarting the thing from QuickOpenWidget and see where the events can finish up.

Complicated, but still interesting stuff here.

[bpasero]

@XVincentX the area you are looking into is unfortunately quite old and definitely needs some cleanup to make it easier to understand. I appreciate you taking the time to dig into.

Luckily there is a way to find out if an entry was selected or not. Every quick open entry can implement the run method which gets called both for opening an entry or selecting it. When you look at the entries from the openFileHandler you will end up in the editor input entry's implementation: https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/browser/quickopen.ts#L249

We need to add code here to support the Mode.PREVIEW.

I think the harder part is to find a way to prevent history etc. from adding this to their lists. I think every listener of onEditorsChanged() and onEditorsOpening() (see https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/browser/parts/editor/editorPart.ts#L174) needs to be checked if it makes sense to react on this preview kind of thing. And then we need a way to transport this nature to the event, or alternatively have a way to disable history for a short while.

[XVincentX]

@bpasero
Thanks a lot for the clarification, that should help me during the journey eventually.

Let's see if I can come up with some other considerations!

[XVincentX]

@bpasero
So, I tried to modify a bit the function in this commit: dead310
where fundamentally I'm relaxing the condition.

Now I see two things:

1. the run method is not invoked if the current entry is from history - I think that's a problem.
2. When I navigate to a file with the keyboard, the quick open entry is closed and the editor is getting the focus. I tried to override this thing setting option.preserveFocus to false, but it seems like the option is being ignored down to the chain.

I'll look into more eventually - let me know if I'm on the right track.

[bpasero]

Yes, preserveFocus must work and should keep quick open visible, otherwise it looks like a bug that should be looked at separately.

[XVincentX]

I am afraid it does not then - as this code is not working as expected.

https://github.com/XVincentX/vscode/commit/a37a7e63996526656c158fdd9c8d19c58b3aa271

[bpasero]

@XVincentX can you file a bug?

[XVincentX]

I'll try to track down the issue - if not I'll file a new one here 👍

It seems like this is exploding 💣

[XVincentX]

Quick update: the flag is working correctly in editorParts.ts:407 - it just does not get passed into the chain for some reason. I'll keep digging.

[bpasero]

@XVincentX it works for me, you need to add the preserveFocus flag to more places because there is 2 different calls to openEditor here: https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/browser/quickopen.ts#L254

If the input is untyped (IResourceInput), the options are part of the input and need to be added to https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/parts/search/browser/openFileHandler.ts#L96

Also the boolean return value of the run method determines if the quick open should be closed or not and thus needs to return false for preview mode.

[XVincentX]

@bpasero
Which is exactly what I was already doing here: master...XVincentX:master

As you can see I'm setting the preserveFocus attribute to false and returning false from the function, just as you suggested.

I continued to track down the error and probably I found something interesting:

Fundamentally if we have a look to the static from function you can see that it's returning an empty object in my case - which might actually be fine.

The thing is that successively the same null object is passed in the Focus checking function which will find an undefined option and focus the editor by default.

So it seems like the options suggest to not preserve the focus by default, while the editorParts is behaving in the opposite way (focusing by default)

I've put up this which should fix the thing: https://github.com/XVincentX/vscode/commit/c855cb2397221a3308d78655853384acbe6ebc5f

Anyway I still do not understand why you're just setting true in some cases and not simply copy or forward the options object directly.

Furthermore, it seems like the logic is wrong here

const focus = !options || !options.preserveFocus;

if preserveFocus is false, I do not want to focus the document, thouse focus should be false, right?
I've fixed this thing here as well: https://github.com/XVincentX/vscode/commit/3b26ce347ef3163e8fb3bb7d4e9dd33b09777a61

Does all this make sense to you?

P.S: I've opened a PR so we can discuss the lines directly there eventually.

[bpasero]

@XVincentX preserveFocus has to be true to "preserve focus".

[XVincentX]

@bpasero Should this feature be enabled for everyone or - should it be behind a configuration entry?

[bpasero]

@XVincentX we had a UX meeting a while ago where we agreed that this should be the default behaviour 👍 . The reasoning is that we already update the UI while navigating the list for these areas:

• icon theme
• color theme
• symbol outline

Btw I would probably also expect this feature to work for the openSymbolHandler which is used to list all the symbols of a project (Cmd+T). I would also expect it to work for the picker that shows for selecting an editor in a tab list (editorPicker.ts via Cmd+Tab). Finally we have a list of recently opened editors whenever you bring up quick open (Cmd+P) that should also behave like that. I think in general any entry in quick open which points to an editor should preview on selection.

[XVincentX]

@bpasero
I totally agree with you. It seems like I selected a monster as task 😱

I've pushed bunch of other commits and I have it almost working - please have a quick look eventually and be bad - unfortunately I'm entirely based on my intuitions and reverse-engineering capabilities, so I might be totally wrong.

I'll keep going!

[bpasero]

Keep pushing, I am offline now and will check back tomorrow 👍

[bpasero]

Added comments to the PR.

[wprater]

I think the harder part is to find a way to prevent history etc. from adding this to their lists. I think every listener of onEditorsChanged() and onEditorsOpening() (see https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/browser/parts/editor/editorPart.ts#L174) needs to be checked if it makes sense to react on this preview kind of thing. And then we need a way to transport this nature to the event, or alternatively have a way to disable history for a short while.

I made a PR to implement this issue. I will circle back to be certain the history events are not stored.

Would prefer to create a temporary buffer for files that can not be revealIfVisible.

refs #13909

[bpasero]

@XVincentX @wprater you two should probably talk, there are now 2 PRs open for the same issue and I would like to look at just one 👍

[wprater]

I had not seen this until after I was mostly done. :(

My PR had the benefit of recognizing the state of your editors when you cancel out of the quick open dialog. @XVincentX let me know if you wanted me to merge anything to mine. I think you're AWK for a week, so Im more than happy to close this out.

[XVincentX]

I'll be out for the entire week. Feel free to step in

[humiaozuzu]

Any update for this PR? This function is so useful!

[XVincentX]

I was working on this but then @wprater claimed to be almost done with it...you guys tell me!

[bpasero]

Sorry, did not mean to close this one (somehow I thought this was the PR). Short story is that there is a core piece missing in the workbench UI which is to be able to open editors as a new kind of "preview" where they would not even cause a tab to open nor events to trigger (e.g. opened editors would not even see it).

Very similar to how in Sublime an untitled file does not turn into a tab before starting to type:

Before:

After:

This is the concept we need for the picker to be able to show a preview without causing too much updating of the underlying tabs world.

Since I do not feel this is something that should come in via "help wanted", I am removing the label.

[humiaozuzu]

Any update for this PR?

[iameli]

I want this. Is there a PR with meaningful progress I could hack on?

If not I'm going to try and do a really really simple version of this where I generate images from everything in the cache and flash that across the screen. It probably won't respect theming and stuff like that, but it'll be better than nothing.

[bpasero]

Duplicate of #8939