-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
Changes from all commits
89f823f
18f4f3e
78cad07
070673f
7e51878
03a9f64
afa91e0
4fedc38
09bebf0
98cf219
87c4898
e3fbbb9
dbf5a71
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,11 +30,10 @@ 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 } 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 { 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'; | ||
|
@@ -46,6 +45,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); | ||
|
@@ -94,6 +94,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; | ||
|
||
|
@@ -106,7 +108,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); | ||
|
||
|
@@ -267,7 +270,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), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @wprater careful, this is being overridden later in a |
||
onType: (value: string) => { /* ignore, handle later */ }, | ||
onShow: () => this.handleOnShow(true), | ||
onHide: (reason) => this.handleOnHide(true, reason) | ||
|
@@ -479,6 +482,15 @@ export class QuickOpenController extends WorkbenchComponent implements IQuickOpe | |
|
||
this.previousValue = prefix; | ||
|
||
// Track active editor before navigation | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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); | ||
}); | ||
|
@@ -499,7 +511,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), | ||
|
@@ -549,6 +561,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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @wprater shouldn't this be There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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... There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Im not entirely clear how the goto variants are used? I appear to have abstracted a bunch of the |
||
|
||
// restore the previous preview editor | ||
if (this.previousPreviewEditorInput) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.) easy, just close the preview editor that was opened Also you have to be careful that you do this from the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'll handle this in There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
@@ -1048,9 +1087,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; | ||
|
||
|
@@ -1103,19 +1143,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); | ||
} | ||
} | ||
|
@@ -1158,4 +1185,4 @@ export class RemoveFromEditorHistoryAction extends Action { | |
} | ||
}); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'; | ||
|
||
|
@@ -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, | ||
protected _configurationService: IConfigurationService | ||
) { | ||
super(); | ||
} | ||
|
||
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @wprater any particular reason you added the extra There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Did you look at this in context of the whole |
||
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; | ||
} | ||
} | ||
|
@@ -435,4 +459,4 @@ export class QuickOpenAction extends Action { | |
|
||
return TPromise.as(null); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
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 :-/
There was a problem hiding this comment.
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)?