Eagerly preview files in quickopen widgets #13909

wprater · 2016-10-17T21:53:52Z

mention-bot · 2016-10-17T21:53:54Z

@wprater, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bpasero, @chrmarti and @egamma to be potential reviewers.

msftclas · 2016-10-17T21:53:56Z

Hi @wprater, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

msftclas · 2016-10-17T22:01:12Z

@wprater, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

wprater · 2016-10-17T22:23:10Z

per my comment #8939 (comment) I would l prefer we closed a newly opened preview editor upon a cancellation request.

please review and let me know what you think.

wprater · 2016-10-18T04:32:29Z

e7ed1c6 adds support for openSymbolHandler. however, while the previews are occurring, the lines are not being selected.

edit: fixed. was clobbering the selection prop in the input resource options :)

wprater · 2016-10-19T01:26:27Z

src/vs/workbench/browser/parts/editor/editorPicker.ts

@@ -267,4 +267,4 @@ export class AllEditorsPicker extends BaseEditorPicker {

 		return super.getAutoFocus(searchValue);
 	}
-}


@bpasero sorry about the newlines. I can rebase them out. should we add "files.insertFinalNewline": false to workspace settings or .editorconfig ?

bpasero · 2016-10-19T05:01:14Z

@wprater the "active editor" is not enough to check, you need to check for any opened editor that is a preview editor inside a group. See https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/common/editor.ts#L814

One thing I just realized is that we need to introduce a new editor option to force it to open as preview because if a user has preview disabled ("workbench.editor.enablePreview": false) he would not get happy...

wprater · 2016-10-19T17:31:09Z

One thing I just realized is that we need to introduce a new editor option to force it to open as preview because if a user has preview disabled ("workbench.editor.enablePreview": false) he would not get happy...

thanks for the tip ;) I'll look into this today.

wprater

IEditorGroupService.getStacksModel() gives you complete access about what is going on in the editor stacks world (1 stack contains 3 groups side by side of N editor tabs). I think the best is to just store the current preview editor before doing the magic and then restore either the previous one on cancel or close the opened one if there was no preview editor before.

@bpasero per your comment above, I have some pseudo code to discover any previous preview editors so I can close any new ones. However, Im not certain the best way to get access to editorGroupService from within quickopencontroller. Thoughts?

// Determine if there was a preview editor already open
this.previousPreviewEditorInput = null;

// how to get the activeGroup?
const activeGroup = this.historyService.editorGroupService.getStacksModel().activeGroup;

const visiblePreviewEditors = activeGroup.editors.filter((input: EditorInput) => {
    return activeGroup.isPreview(input);
});
if (visiblePreviewEditors.length) {
    this.previousPreviewEditorInput = visiblePreviewEditors[0].input;
}

the best I have so far is to add getStacksModel(): IEditorStacksModel to the IEditorPart IWorkbenchEditorService interfaces, so I can get the stacks model from the editorService.

wprater · 2016-10-19T22:19:10Z

src/vs/workbench/browser/parts/quickopen/quickOpenController.ts

-		}
-
-		return false;
+		return super.run(mode, context);


I moved all this out per 0618752

having it in EditorQuickOpenEntry allows us to keep this eager preview and open logic in one place for all the other quick open widgets (not sure of nomenclature here), with the exception of EditorPickerEntry. It extends on QuickOpenEntryGroup.

@wprater EditorQuickOpenEntry has some behaviour that we do not want in EditorPickerEntry (e.g. opening to the side) but we do want the preview especially in the EditorPickerEntry as well. I fear for now we have to duplicate this if it is not too much code...

wprater · 2016-10-19T22:19:47Z

src/vs/workbench/browser/parts/quickopen/quickOpenController.ts

-			if (this.input instanceof EditorInput) {
-				this.editorService.openEditor(this.input, { pinned }, sideBySide).done(null, errors.onUnexpectedError);
-			} else {
-				this.editorService.openEditor({ resource: (this.input as IResourceInput).resource, options: { pinned: true } }, sideBySide);


can you tell me why pinned was forced to true for a IResourceInput here?

@wprater thanks, a bad bug that I introduced and will push a fix to master. it should be { pinned } as the other one.

wprater · 2016-10-19T22:23:02Z

src/vs/workbench/browser/parts/editor/editorPart.ts

-		const pinned = !this.previewEditors || (options && (options.pinned || typeof options.index === 'number')) || input.isDirty();
+		let pinned = !this.previewEditors || (options && (options.pinned || typeof options.index === 'number')) || input.isDirty();
+		if (this.previewEditors) {
+			pinned = !this.configurationService.getConfiguration<IWorkbenchEditorConfiguration>().workbench.editor.enablePreviewFromQuickOpen;


using the config service here vs. EditorHistoryEntry. I may have preferred to put the config service call in the getOptions of EditorQuickOpenEntry, but there is no configurationService directly available there.

@wprater you can use most service from most classes by just adding it to the constructor as I was showing it for quickOpenController. We use constructor service injection and you typically do not have to deal with it.

I am not yet sure how we can get this working without introducing a new editor option forcePreview (or something like that) to open editors non-pinned when navigating in quick open.

I see your idea with using enablePreviewFromQuickOpen setting but I think this is different: it controls if a file opened from quick open shows up pinned or as preview. In our case we always want preview no matter what this setting does.

@wprater you can use most service from most classes by just adding it to the constructor as I was showing it for quickOpenController. We use constructor service injection and you typically do not have to deal with it.

good to know. in this case, I have to explicitly pass it down to the EditorQuickOpenEntry during the super call for all the abstract classes that extend it. not really a good approach.

I would rather put all this stuff in the abstract classes getOptions.

I see your idea with using enablePreviewFromQuickOpen setting but I think this is different: it controls if a file opened from quick open shows up pinned or as preview. In our case we always want preview no matter what this setting does.

yeah, I did add a option for that, but called it eagerPreview. I will rename it forcePreview.

It is fine to push down services like that but if you have a more elegant solution, also fine.

So you are talking about introducing a new global setting to control previewing from quick open? I honestly thing we do not need that because in other quick opens we already behave like that (e.g. quick outline, goto line, change theme). We can just have it on by default 👍

bpasero · 2016-10-20T04:51:47Z

@wprater just add @IEditorGroupService private editorGroupService: IEditorGroupService to the constructor of quickOpenController and add the import:
import { IEditorGroupService} from 'vs/workbench/services/group/common/groupService';

This service is being created before quick open so you can get it injected.

wprater · 2016-10-21T00:28:28Z

that worked perfect!

wprater · 2016-10-21T02:05:06Z

@bpasero added support for history blocking while tabs are being eagerly previewed! but need a better way to test it.

wprater · 2016-10-21T04:43:37Z

I need to fix the restoration of tabs and then we can re-review.

bpasero · 2016-10-21T06:30:07Z

Ok ping me then.

bpasero · 2016-10-21T07:02:27Z

@wprater since I just merged your other PR there are merge conflicts now.

wprater · 2016-10-21T07:04:59Z

@wprater since I just merged your other PR there are merge conflicts now.

@bpasero yeah, fixing them now.

wprater · 2016-10-25T23:26:32Z

The duplication of the code was driving me crazy and looking really ugly. So I refactored some of the EditorQuickOpenEntry stuff. Now the blocking and pinned option support is all in one place for the openSymbol and quick open widget. The editorPicker still blocks on its own.

ugly example..

wprater

start a new review? or should I pull out the editor restoring bit? I cleanup these commits when review is done.

wprater · 2016-10-25T23:37:55Z

If you want I can look into the restoring of editors after we have merged this in. We can leave it out from this PR to make the change easier to digest.

Let's do that. I want to get this merged in and I like your idea of the freeze/restore.

bpasero · 2016-10-26T04:46:22Z

We are in end game currently and I will only have time next week again. I suggest you bring this into a good shape without the editor restoring thingy (I can add it later) and then I merge it next week.

…uickopen

bpasero · 2016-11-05T07:32:13Z

src/vs/workbench/common/editor.ts

@@ -440,6 +440,7 @@ export class EditorOptions implements IEditorOptions {
 		options.forceOpen = settings.forceOpen;
 		options.revealIfVisible = settings.revealIfVisible;
 		options.pinned = settings.pinned;
+		options.forcePreview = settings.forcePreview;


@wprater can you quickly check where else in this file options are handled, I think you missed to add this new flag e.g. to TextDiffEditorOptions

I did not add it because TextDiffEditorOptions would ever be used in a preview content, correct?

bpasero · 2016-11-05T07:33:24Z

src/vs/workbench/browser/quickopen.ts

-			if (mode === Mode.OPEN_IN_BACKGROUND) {
-				openInBackgroundOptions = { pinned: true, preserveFocus: true };
-			}
+			sideBySide = !context.quickNavigateConfiguration && context.keymods.indexOf(KeyMod.CtrlCmd) >= 0;


@wprater any particular reason you added the extra !context.quickNavigateConfiguration check here?

What do you mean by "extra"?

When I removed the sideBySide and pinned options from https://github.com/Microsoft/vscode/pull/13909/files/87c4898204236054020596a89a459e77ebfefd04#diff-efeb3f5a180f00bd9db3f96c43f67f5cL1107 I wanted to keep the logic being used.

bpasero · 2016-11-05T07:34:37Z

src/vs/workbench/browser/quickopen.ts

-				} else if (openInBackgroundOptions) {
-					opts = EditorOptions.create(openInBackgroundOptions);
-				}
+		let pinned;


@wprater why do we need pinned here? first of all it does not seem to be used when the mode is Mode.OPEN. And then I think the editor part itself is already dealing with the setting workbench.editor.enablePreviewFromQuickOpen?

Im a bit confused by your comment. Also, I don't see where the editorPart is handling this. It used to be in the controller, but was moved when I refactored that out here.

It's used in OPEN_IN_BACKGROUND and OPEN modes and gets it's value from the workbench.editor.enablePreviewFromQuickOpen setting while in OPEN mode. Some of this logic was taken when I removed this logic from the controller.

Did you look at this in context of the whole run method here?

bpasero · 2016-11-05T07:36:05Z

src/vs/workbench/browser/parts/editor/editorPart.ts

@@ -239,7 +239,7 @@ export class EditorPart extends Part implements IEditorPart, IEditorGroupService
 		// while the UI is not yet ready. Clients have to deal with this fact and we have to make sure that the
 		// stacks model gets updated if any of the UI updating fails with an error.
 		const group = this.ensureGroup(position, !options || !options.preserveFocus);
-		const pinned = !this.previewEditors || (options && (options.pinned || typeof options.index === 'number')) || input.isDirty();
+		const pinned = (!this.previewEditors && !(options && options.forcePreview)) || (options && (options.pinned || typeof options.index === 'number')) || input.isDirty();


This is actually interesting: conceptually we cannot have a dirty file open as preview. So if you have many dirty files and you navigate over them in quick open they will open as pinned editors :-/

When you say quick open, do you mean the control+tab or Open anywhere? Sorry, Im not up to speed on all the nomenclature.

Won't dirty files already be opened an not previewed, so when they get selected, the editor will just be opened (switched to foreground)?

bpasero · 2016-11-05T07:39:37Z

src/vs/workbench/browser/parts/quickopen/quickOpenController.ts

@@ -30,11 +30,12 @@ import { IResourceInput, IEditorInput } from 'vs/platform/editor/common/editor';
 import { IModeService } from 'vs/editor/common/services/modeService';
 import { getIconClasses } from 'vs/workbench/browser/labels';
 import { IModelService } from 'vs/editor/common/services/modelService';
-import { EditorInput, getUntitledOrFileResource, IWorkbenchEditorConfiguration } from 'vs/workbench/common/editor';
+// import { EditorInput, getUntitledOrFileResource, IWorkbenchEditorConfiguration } from 'vs/workbench/common/editor';


@wprater why commented out?

I was still working on this, sorry, I never intend to commit commented out code. It was meant to be removed.

bpasero · 2016-11-05T07:41:05Z

src/vs/workbench/parts/quickopen/browser/gotoLineHandler.ts

@@ -184,7 +192,11 @@ export class GotoLineHandler extends QuickOpenHandler {
 			this.lastKnownEditorViewState = (<IEditor>editor.getControl()).saveViewState();
 		}

-		return TPromise.as(new QuickOpenModel([new GotoLineEntry(searchValue, this.editorService, this)]));
+		const entry = this.instantiationService.createInstance(


@wprater you do not need to pass in the services, just do: this.instantiationService.createInstance(GotoLineEntry, searchValue)

All services will be passed in automatically.

The constructor's signature requires a handler as well as those services. Everything will get passed in while using the instantiationService?

constructor(line: string, editorService: IWorkbenchEditorService, historyService: IHistoryService, configurationService: IConfigurationService, handler: GotoLineHandler)

bpasero · 2016-11-05T07:42:40Z

@wprater added more feedback. I did not look closely at quickOpenController.ts as the file seemed to have work in progress code (commented out imports et al?). If that file is about restoring the previous editor state, maybe you remove your changes and I can look into that particular thing later.

Btw I noticed that dirty editors cannot open as preview editors (conceptually currently). That is a pretty bad limitation for this feature I fear :-(

wprater · 2016-11-05T18:16:08Z

@wprater added more feedback. I did not look closely at quickOpenController.ts as the file seemed to have work in progress code (commented out imports et al?). If that file is about restoring the previous editor state, maybe you remove your changes and I can look into that particular thing later.

Commented on this, but I did not intend to commit the commented out code. It's ready to look at, the code in run was refactored out into EditorQuickOpenEntry

wprater · 2016-11-05T18:17:34Z

Btw I noticed that dirty editors cannot open as preview editors (conceptually currently). That is a pretty bad limitation for this feature I fear :-(

I don't see to understand why they would ever be attempted to open as a preview? They are always already open, correct? So those editors would become active during the select (preview).

Was caught up on a ton of project work these last weeks.

bpasero · 2016-11-06T06:58:27Z

@wprater yeah good point on the fact that dirty files are always opened. however, you might have opened a dirty file in another group (there can be up to 3 groups) and so you might be in group 1 doing your quick open preview business when a dirty file is opened in group 2 or 3. This leads to opening the dirty file in to group 1 while you preview over, UNLESS we would actually reveal the file in the other group, which would make the whole preview business more complicated.

I find it very interesting what sublime does and they do not have this problem because they seem to have a special preview editor that is not even opening a new tab in the tab well:

This goes back to your original idea of having a custom editor for preview but goes beyond that: We do currently not have such a preview editor that does NOT open a new tab.

Sublime also leverages this when you have no file open:

wprater · 2016-11-06T18:01:40Z

UNLESS we would actually reveal the file in the other group, which would make the whole preview business more complicated

Yeah, Im not sure that's the best experience? Think you've always been keen to keep the focus within the focused group. Especially if we follow up with this issue #14142

Did you want to chat offline (or in another ticket) about how we could create such a temporary (preview) editor? I think we have most of the mechanics done already. Just a matter of rendering an editor on top of the current editor's group and sending events for tabs revealing or what not.

Otherwise, I need to figure out how deal with these dirty editors.

bpasero · 2016-11-07T12:23:57Z

The more I think about this problem the more I believe we need to introduce a new concept in the editor space which is a truly preview-editor that does not show up anywhere in the UI besides showing the contents of a file. This editor should (when open) not cause a change in the tab well, will be ignored by any editor listeners (such as the history - making the blocking of history obsolete) and should not cause any UI change anywhere else (e.g. opened editors view). Once any other editor opens (or even the same editor), this preview-editor is thrown away and replaced with the real editor that shows with a tab. If the same editor is opened that was previously previewed, it should keep the contents of the editor and just update the tab well on top.

I fear this is not such a simple change because it drags through many layers and needs some more thinking. But I thought I send out this idea already.

wprater · 2016-11-09T07:15:05Z

Yeah, I agree that we should go this route, after going through all these save/restore/preview states I was always thinking a simpler overlay/temp editor could help simplify things.

Let me know how I can help.

bpasero · 2016-11-09T08:07:59Z

Unfortunately it needs to be a full new concept that goes beyond a temp editor. Appreciate your help, I need to think about this for a while. Some other P1 work is requiring my full attention right now.

drKnoxy · 2016-12-19T15:27:16Z

so is this PR dead?

bpasero · 2016-12-19T15:58:55Z

I think it is ongoing, but from my end I did not have enough time to check back. I think the critical piece missing is a way of opening an editor as preview without interfering with any tabs model or emitting events otherwise. This is a bit outside of the scope of this PR to be fair but I think the preview cannot be done properly without such functionality on the core workbench editor layer.

wprater · 2016-12-19T23:58:26Z

Im still very keen to help get this landed. We were close, but as @bpasero said, there was a condition where an editor could open in a pane that was only meant to be a preview.

A temporary view which would overlay on top of the current editor part, would be one solution. I dont know enough of the internal mechanics to take a good stab at this, however.

More than happy to brainstorm.. or review code ideas.

peabnuts123 · 2017-01-18T02:38:15Z

Poke / +1 as a drive to get this developed properly / merged as this would make a pretty big difference to UX in vscode

bpasero · 2017-01-19T08:08:11Z

Unfortunately this needs a lot more thinking to get right, closing until we revisit this.

nicooprat · 2017-08-29T13:05:14Z

Is this still something planned? No more activity there too: #12892

Would love to see this happening! 👍

msftclas added the cla-required label Oct 17, 2016

msftclas added cla-signed and removed cla-required labels Oct 17, 2016

wprater mentioned this pull request Oct 17, 2016

Show preview editors while navigating through quick open results #12892

Closed

bpasero self-assigned this Oct 18, 2016

wprater force-pushed the eagerly-preview-quickopen branch 2 times, most recently from 1bf54e9 to da200f3 Compare October 19, 2016 01:24

wprater commented Oct 19, 2016

View reviewed changes

wprater force-pushed the eagerly-preview-quickopen branch 2 times, most recently from be7b5d9 to 0618752 Compare October 19, 2016 21:03

wprater commented Oct 19, 2016

View reviewed changes

wprater force-pushed the eagerly-preview-quickopen branch from 0618752 to 6f94d8a Compare October 20, 2016 22:08

wprater force-pushed the eagerly-preview-quickopen branch from aae016a to 3a8af47 Compare October 21, 2016 02:04

wprater force-pushed the eagerly-preview-quickopen branch 2 times, most recently from 4107685 to bf01703 Compare October 21, 2016 04:42

wprater force-pushed the eagerly-preview-quickopen branch from c243f5b to e68b30d Compare October 21, 2016 07:27

wprater added 2 commits October 25, 2016 16:18

refactored into super

09bebf0

handle history blocking in EditorQuickOpenEntry

98cf219

wprater force-pushed the eagerly-preview-quickopen branch from 7ceadd3 to 98cf219 Compare October 25, 2016 23:19

wprater commented Oct 25, 2016

View reviewed changes

bpasero mentioned this pull request Oct 26, 2016

Open a previewEditor while navigating files through QuickOpen #13236

Closed

bpasero added 2 commits November 5, 2016 08:29

Merge remote-tracking branch 'upstream/master' into eagerly-preview-q…

87c4898

…uickopen

fix compile error

e3fbbb9

bpasero requested changes Nov 5, 2016

View reviewed changes

remove unsued imports

dbf5a71

bpasero removed their assignment Jan 18, 2017

bpasero closed this Jan 19, 2017

shreeve mentioned this pull request Feb 16, 2019

Preview files in quick open and when highlighting in explorer shreeve/vscode#1

Open

github-actions bot locked and limited conversation to collaborators Mar 27, 2020

Eagerly preview files in quickopen widgets #13909

Eagerly preview files in quickopen widgets #13909

Conversation

wprater commented Oct 17, 2016 • edited Loading

mention-bot commented Oct 17, 2016

msftclas commented Oct 17, 2016

msftclas commented Oct 17, 2016

wprater commented Oct 17, 2016

wprater commented Oct 18, 2016 • edited Loading

Choose a reason for hiding this comment

bpasero commented Oct 19, 2016 • edited Loading

wprater commented Oct 19, 2016

wprater left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpasero Oct 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpasero commented Oct 20, 2016 • edited Loading

wprater commented Oct 21, 2016

wprater commented Oct 21, 2016 • edited Loading

wprater commented Oct 21, 2016

bpasero commented Oct 21, 2016

bpasero commented Oct 21, 2016

wprater commented Oct 21, 2016

wprater commented Oct 25, 2016 • edited Loading

wprater left a comment

Choose a reason for hiding this comment

wprater commented Oct 25, 2016

bpasero commented Oct 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpasero commented Nov 5, 2016

wprater commented Nov 5, 2016

wprater commented Nov 5, 2016

bpasero commented Nov 6, 2016

wprater commented Nov 6, 2016 • edited Loading

bpasero commented Nov 7, 2016 • edited Loading

wprater commented Nov 9, 2016

bpasero commented Nov 9, 2016

drKnoxy commented Dec 19, 2016

bpasero commented Dec 19, 2016

wprater commented Dec 19, 2016

peabnuts123 commented Jan 18, 2017

bpasero commented Jan 19, 2017

nicooprat commented Aug 29, 2017

wprater commented Oct 17, 2016 •

edited

Loading

wprater commented Oct 18, 2016 •

edited

Loading

bpasero commented Oct 19, 2016 •

edited

Loading

wprater left a comment •

edited

Loading

bpasero Oct 20, 2016 •

edited

Loading

bpasero commented Oct 20, 2016 •

edited

Loading

wprater commented Oct 21, 2016 •

edited

Loading

wprater commented Oct 25, 2016 •

edited

Loading

wprater commented Nov 6, 2016 •

edited

Loading

bpasero commented Nov 7, 2016 •

edited

Loading