Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Eagerly preview files in quickopen widgets #13909

Closed
wants to merge 13 commits into from
8 changes: 7 additions & 1 deletion src/vs/platform/editor/common/editor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,12 @@ export interface IEditorOptions {
*/
pinned?: boolean;

/**
* Editor that is being shown with an `forcePreview` will override the `enablePreview` setting
* of the workspace configuration to allow the editor to be shown as a preview editor.
*/
forcePreview?: boolean;

/**
* The index in the document stack where to insert the editor into when opening.
*/
Expand All @@ -197,4 +203,4 @@ export interface ITextEditorOptions extends IEditorOptions {
endLineNumber?: number;
endColumn?: number;
};
}
}
4 changes: 2 additions & 2 deletions src/vs/workbench/browser/parts/editor/editorPart.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :-/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

const active = (group.count === 0) || !options || !options.inactive;
group.openEditor(input, { active, pinned, index: options && options.index });

Expand Down Expand Up @@ -1380,4 +1380,4 @@ export class EditorPart extends Part implements IEditorPart, IEditorGroupService
private hasGroup(position: Position): boolean {
return !!this.stacks.groupAt(position);
}
}
}
28 changes: 21 additions & 7 deletions src/vs/workbench/browser/parts/editor/editorPicker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,18 @@ import { IAutoFocus, Mode, IEntryRunContext, IQuickNavigateConfiguration } from
import { QuickOpenModel, QuickOpenEntry, QuickOpenEntryGroup } from 'vs/base/parts/quickopen/browser/quickOpenModel';
import scorer = require('vs/base/common/scorer');
import { IModeService } from 'vs/editor/common/services/modeService';
import { IHistoryService } from 'vs/workbench/services/history/common/history';
import { getIconClasses } from 'vs/workbench/browser/labels';
import { IModelService } from 'vs/editor/common/services/modelService';
import { QuickOpenHandler } from 'vs/workbench/browser/quickopen';
import { Position } from 'vs/platform/editor/common/editor';
import { IEditorOptions, ITextEditorOptions, Position } from 'vs/platform/editor/common/editor';
import { IEditorGroupService } from 'vs/workbench/services/group/common/groupService';
import { IWorkbenchEditorService } from 'vs/workbench/services/editor/common/editorService';
import { IInstantiationService } from 'vs/platform/instantiation/common/instantiation';
import { IWorkspaceContextService } from 'vs/platform/workspace/common/workspace';
import { EditorInput, asFileEditorInput, IEditorGroup, IEditorStacksModel } from 'vs/workbench/common/editor';


export class EditorPickerEntry extends QuickOpenEntryGroup {
private stacks: IEditorStacksModel;

Expand All @@ -35,6 +37,7 @@ export class EditorPickerEntry extends QuickOpenEntryGroup {
@IWorkbenchEditorService private editorService: IWorkbenchEditorService,
@IModeService private modeService: IModeService,
@IModelService private modelService: IModelService,
@IHistoryService protected historyService: IHistoryService,
@IEditorGroupService editorGroupService: IEditorGroupService
) {
super();
Expand Down Expand Up @@ -77,16 +80,27 @@ export class EditorPickerEntry extends QuickOpenEntryGroup {

public run(mode: Mode, context: IEntryRunContext): boolean {
if (mode === Mode.OPEN) {
return this.runOpen(context);
this.runOpen(context);

return true;
} else if (mode === Mode.PREVIEW) {
this.runOpen(context, { forcePreview: true, pinned: false, revealIfVisible: true, preserveFocus: true });

return false;
}

return super.run(mode, context);
}

private runOpen(context: IEntryRunContext): boolean {
this.editorService.openEditor(this.editor, null, this.stacks.positionOfGroup(this.group)).done(null, errors.onUnexpectedError);

return true;
private runOpen(context: IEntryRunContext, options?: IEditorOptions | ITextEditorOptions) {
if (options.forcePreview) {
this.historyService.block(true);
}
this.editorService.openEditor(this.editor, options, this.stacks.positionOfGroup(this.group))
.done(() => this.historyService.block(false), err => {
this.historyService.block(false);
errors.onUnexpectedError(err);
});
}
}

Expand Down Expand Up @@ -267,4 +281,4 @@ export class AllEditorsPicker extends BaseEditorPicker {

return super.getAutoFocus(searchValue);
}
}
}
69 changes: 49 additions & 20 deletions src/vs/workbench/browser/parts/quickopen/quickOpenController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wprater why commented out?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

import { EditorInput, getUntitledOrFileResource } from 'vs/workbench/common/editor';
import { WorkbenchComponent } from 'vs/workbench/common/component';
import Event, { Emitter } from 'vs/base/common/event';
import { IPartService } from 'vs/workbench/services/part/common/partService';
import { KeyMod } from 'vs/base/common/keyCodes';
// import { KeyMod } from 'vs/base/common/keyCodes';
import { QuickOpenHandler, QuickOpenHandlerDescriptor, IQuickOpenRegistry, Extensions, EditorQuickOpenEntry } from 'vs/workbench/browser/quickopen';
import errors = require('vs/base/common/errors');
import { IWorkbenchEditorService } from 'vs/workbench/services/editor/common/editorService';
Expand All @@ -46,6 +47,7 @@ import { ITelemetryService } from 'vs/platform/telemetry/common/telemetry';
import { IWorkspaceContextService } from 'vs/platform/workspace/common/workspace';
import { IContextKeyService, RawContextKey, IContextKey } from 'vs/platform/contextkey/common/contextkey';
import { IHistoryService } from 'vs/workbench/services/history/common/history';
import { IEditorGroupService } from 'vs/workbench/services/group/common/groupService';

const HELP_PREFIX = '?';
const QUICK_OPEN_MODE = new RawContextKey<boolean>('inQuickOpen', false);
Expand Down Expand Up @@ -94,6 +96,8 @@ export class QuickOpenController extends WorkbenchComponent implements IQuickOpe
private previousActiveHandlerDescriptor: QuickOpenHandlerDescriptor;
private actionProvider = new ContributableActionProvider();
private previousValue = '';
private previousActiveEditorInput: IEditorInput;
private previousPreviewEditorInput: IEditorInput;
private visibilityChangeTimeoutHandle: number;
private closeOnFocusLost: boolean;

Expand All @@ -106,7 +110,8 @@ export class QuickOpenController extends WorkbenchComponent implements IQuickOpe
@IConfigurationService private configurationService: IConfigurationService,
@IHistoryService private historyService: IHistoryService,
@IInstantiationService private instantiationService: IInstantiationService,
@IPartService private partService: IPartService
@IPartService private partService: IPartService,
@IEditorGroupService private editorGroupService: IEditorGroupService
) {
super(QuickOpenController.ID);

Expand Down Expand Up @@ -267,7 +272,7 @@ export class QuickOpenController extends WorkbenchComponent implements IQuickOpe
withElementById(this.partService.getWorkbenchElementId()).getHTMLElement(),
{
onOk: () => { /* ignore, handle later */ },
onCancel: () => { /* ignore, handle later */ },
onCancel: () => this.handleOnCancel(true),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wprater careful, this is being overridden later in a setCallbacks() call, so you have to do it there for the picker.

onType: (value: string) => { /* ignore, handle later */ },
onShow: () => this.handleOnShow(true),
onHide: (reason) => this.handleOnHide(true, reason)
Expand Down Expand Up @@ -479,6 +484,15 @@ export class QuickOpenController extends WorkbenchComponent implements IQuickOpe

this.previousValue = prefix;

// Track active editor before navigation
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wprater what about the picker? I think you need to extract this into a reusable method so that you can also use it when the picker opens.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the show method is called in the picker as well. what am I missing here?

Copy link
Member

@bpasero bpasero Oct 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is debt: we have two different quick open widgets for the picker (e.g. the one that shows when you want to change a UI theme, or change encoding of a file) and for the quick open main widget (the one that allows to do most things like searching for files). I am actually not so sure anymore we would want this functionality for the picker so maybe we remove the code that handles the cancellation from the picker and only run it from the quick open widget.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the only part we need to remember for the picker is which editor was active before it was opened. this way we're able to openEditor on that Editor upon a cancel.

const activeGroup = this.editorGroupService.getStacksModel().activeGroup;

// Determine if there was a preview editor already open
if (activeGroup) {
this.previousActiveEditorInput = activeGroup.activeEditor;
this.previousPreviewEditorInput = activeGroup.previewEditor;
}

const promiseCompletedOnHide = new TPromise<void>(c => {
this.promisesToCompleteOnHide.push(c);
});
Expand All @@ -499,7 +513,7 @@ export class QuickOpenController extends WorkbenchComponent implements IQuickOpe
withElementById(this.partService.getWorkbenchElementId()).getHTMLElement(),
{
onOk: () => { /* ignore */ },
onCancel: () => { /* ignore */ },
onCancel: () => this.handleOnCancel(false),
onType: (value: string) => this.onType(value || ''),
onShow: () => this.handleOnShow(false),
onHide: (reason) => this.handleOnHide(false, reason),
Expand Down Expand Up @@ -549,6 +563,33 @@ export class QuickOpenController extends WorkbenchComponent implements IQuickOpe
return promiseCompletedOnHide;
}

private handleOnCancel(isPicker: boolean): void {
// restore the editor part state after cancelling
this.historyService.block(true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wprater shouldn't this be block(false)? also I am missing the all to block(true).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the other blocking is done by checking the editor options. if they are set to forcePreview they are ignored. Im blocking here for the restoring process only.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh ok that explains the other method. I find this confusing, and would only block when the history is blocked via this method. I think the forcePreview option and the fact that history is blocked should totally be independent and not be dependent on each other. Someone might want to forcePreview and would be confused if suddenly the history does not work...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true. I'll look for a clean way to implement the block on all the areas that need it while previewing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

without duplicating code in three places handling the openEditor and blocking of history, services, I need to inject or get access to the historyService in EditorHistoryEntry. All the classes that extend it seem to handle the injection fine with the exception of GotoLineEntry. Thoughts on how to handle that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, the goto line handler was causing the history to add stuff when previewing. I think also the gotoSymbolHandler.ts does! For those you can check https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/parts/search/browser/openFileHandler.ts#L119 how we pass in the instantiation service and use that to create the entries. Doing so, you can get hold of any service via the constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started the refactor process a bit, well, I hope it will be made easier. All the abstract classes of EditorQuickOpenEntry now pass in both both history and configuration services. This helps get the opening and options of the widget cleaned up as well as the blocking support we need.

Im not entirely clear how the goto variants are used? I appear to have abstracted a bunch of the gotoSymbolHandler away now with the preview and open options moved into EditorQuickOpenEntry


// restore the previous preview editor
if (this.previousPreviewEditorInput) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wprater unfortunately I think this is more complicated for the restore part. I can see the following states:

  • 1.) no active editor
  • 2.) no preview editor
  • 3.) active editor is preview editor
  • 4.) active editor is not preview editor

1.) easy, just close the preview editor that was opened
2.) also easy, just close the preview editor
3.) restore the previous active editor by opening it
4.) restore the previous editor in the preview slot but open it in the background because it was not the active one. also open the previous active editor

Also you have to be careful that you do this from the onCancel() callback. Because as far as I remember this method is not called when you actually pick an entry from quick open. I would expect some additional logic when an element is picked.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also you have to be careful that you do this from the onCancel() callback. Because as far as I remember this method is not called when you actually pick an entry from quick open. I would expect some additional logic when an element is picked.

I'll handle this in handleOnHide and use the reason arg to apply logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's do as you suggest and handle the reset in another commit. I have most of it working, but I like your idea of the freeze/restore in the editor service.

this.editorService.openEditor(this.previousPreviewEditorInput, { preserveFocus: true });
}
// otherwise close the preview editor that was created with eager preview
else {
const activeGroup = this.editorGroupService.getStacksModel().activeGroup;
const groupPosition = this.editorGroupService.getStacksModel().positionOfGroup(activeGroup);
if (activeGroup && activeGroup.previewEditor) {
this.editorService.closeEditor(groupPosition, activeGroup.previewEditor);
}
}

// restore the prevously active tab
this.editorService.openEditor(this.previousActiveEditorInput).done(
() => this.historyService.block(false),
err => {
this.historyService.block(false);
errors.onUnexpectedError(err);
}
);
}

private handleOnShow(isPicker: boolean): void {
if (isPicker && this.quickOpenWidget) {
this.quickOpenWidget.hide(HideReason.FOCUS_LOST);
Expand Down Expand Up @@ -1048,9 +1089,10 @@ export class EditorHistoryEntry extends EditorQuickOpenEntry {
@IModelService private modelService: IModelService,
@ITextFileService private textFileService: ITextFileService,
@IWorkspaceContextService contextService: IWorkspaceContextService,
@IHistoryService private historyService: IHistoryService,
@IConfigurationService private configurationService: IConfigurationService
) {
super(editorService);
super(editorService, historyService, configurationService);

this.input = input;

Expand Down Expand Up @@ -1103,19 +1145,6 @@ export class EditorHistoryEntry extends EditorQuickOpenEntry {
}

public run(mode: Mode, context: IEntryRunContext): boolean {
if (mode === Mode.OPEN) {
const sideBySide = !context.quickNavigateConfiguration && context.keymods.indexOf(KeyMod.CtrlCmd) >= 0;
const pinned = !this.configurationService.getConfiguration<IWorkbenchEditorConfiguration>().workbench.editor.enablePreviewFromQuickOpen;

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 } }, sideBySide);
}

return true;
}

return super.run(mode, context);
}
}
Expand Down Expand Up @@ -1158,4 +1187,4 @@ export class RemoveFromEditorHistoryAction extends Action {
}
});
}
}
}
74 changes: 49 additions & 25 deletions src/vs/workbench/browser/quickopen.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,11 @@ import { Action } from 'vs/base/common/actions';
import { KeyMod } from 'vs/base/common/keyCodes';
import { Mode, IEntryRunContext, IAutoFocus, IModel, IQuickNavigateConfiguration } from 'vs/base/parts/quickopen/common/quickOpen';
import { QuickOpenEntry, IHighlight, QuickOpenEntryGroup, QuickOpenModel } from 'vs/base/parts/quickopen/browser/quickOpenModel';
import { EditorOptions, EditorInput } from 'vs/workbench/common/editor';
import { IResourceInput, IEditorInput, IEditorOptions } from 'vs/platform/editor/common/editor';
import { EditorOptions, EditorInput, IWorkbenchEditorConfiguration } from 'vs/workbench/common/editor';
import { IEditor, IResourceInput, IEditorInput, IEditorOptions } from 'vs/platform/editor/common/editor';
import { IConfigurationService } from 'vs/platform/configuration/common/configuration';
import { IWorkbenchEditorService } from 'vs/workbench/services/editor/common/editorService';
import { IHistoryService } from 'vs/workbench/services/history/common/history';
import { IQuickOpenService } from 'vs/workbench/services/quickopen/common/quickOpenService';
import { AsyncDescriptor } from 'vs/platform/instantiation/common/descriptors';

Expand Down Expand Up @@ -231,7 +233,11 @@ export interface IEditorQuickOpenEntry {
*/
export class EditorQuickOpenEntry extends QuickOpenEntry implements IEditorQuickOpenEntry {

constructor(private _editorService: IWorkbenchEditorService) {
constructor(
private _editorService: IWorkbenchEditorService,
private _historyService: IHistoryService,
private _configurationService: IConfigurationService
) {
super();
}

Expand All @@ -250,35 +256,53 @@ export class EditorQuickOpenEntry extends QuickOpenEntry implements IEditorQuick
public run(mode: Mode, context: IEntryRunContext): boolean {
const hideWidget = (mode === Mode.OPEN);

let sideBySide;
if (mode === Mode.OPEN || mode === Mode.OPEN_IN_BACKGROUND) {
let sideBySide = context.keymods.indexOf(KeyMod.CtrlCmd) >= 0;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

let input = this.getInput();
if (input instanceof EditorInput) {
let opts = this.getOptions();
if (opts) {
opts = objects.mixin(opts, openInBackgroundOptions, true);
} else if (openInBackgroundOptions) {
opts = EditorOptions.create(openInBackgroundOptions);
}
let pinned;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

let modeOverrideOptions: IEditorOptions;
if (mode === Mode.OPEN) {
pinned = !this._configurationService.getConfiguration<IWorkbenchEditorConfiguration>().workbench.editor.enablePreviewFromQuickOpen;
} else if (mode === Mode.PREVIEW) {
pinned = false;
modeOverrideOptions = { forcePreview: true, pinned, revealIfVisible: true, preserveFocus: true };

this._historyService.block(true);
} else if (mode === Mode.OPEN_IN_BACKGROUND) {
pinned = true;
modeOverrideOptions = { pinned, preserveFocus: true };
}

this.editorService.openEditor(input, opts, sideBySide).done(null, errors.onUnexpectedError);
} else {
const resourceInput = <IResourceInput>input;
let openEditorPromise: TPromise<IEditor>;
let input = this.getInput();
if (input instanceof EditorInput) {
let opts = this.getOptions();
if (opts) {
opts = objects.mixin(opts, modeOverrideOptions, true);
} else if (modeOverrideOptions) {
opts = EditorOptions.create(modeOverrideOptions);
}

if (openInBackgroundOptions) {
resourceInput.options = objects.assign(resourceInput.options || Object.create(null), openInBackgroundOptions);
}
openEditorPromise = this.editorService.openEditor(input, opts, sideBySide);
} else {
const resourceInput = <IResourceInput>input;

this.editorService.openEditor(resourceInput, sideBySide).done(null, errors.onUnexpectedError);
if (modeOverrideOptions) {
resourceInput.options = objects.assign(resourceInput.options || Object.create(null), modeOverrideOptions);
}

openEditorPromise = this.editorService.openEditor(resourceInput, sideBySide);
}

openEditorPromise.done(
() => this._historyService.block(false),
err => {
this._historyService.block(false);
errors.onUnexpectedError(err);
});

return hideWidget;
}
}
Expand Down Expand Up @@ -435,4 +459,4 @@ export class QuickOpenAction extends Action {

return TPromise.as(null);
}
}
}
Loading