From 834ba9b9b0ed90291a1a252d290e17beba8b0bed Mon Sep 17 00:00:00 2001 From: Benjamin Pasero Date: Mon, 11 Dec 2017 12:03:28 +0100 Subject: [PATCH] debt - clean up some todos and migration code --- .../parts/quickopen/browser/quickOpenModel.ts | 11 ++-- src/vs/code/electron-main/window.ts | 2 +- src/vs/code/electron-main/windows.ts | 47 ++------------ .../platform/extensions/common/extensions.ts | 1 - .../electron-main/historyMainService.ts | 8 +-- .../telemetry/common/telemetryUtils.ts | 1 - src/vs/platform/windows/common/windows.ts | 1 - .../parts/quickopen/quickOpenController.ts | 2 +- .../common/editor/resourceEditorInput.ts | 3 +- .../parts/backup/common/backupRestorer.ts | 2 - .../browser/editors/fileEditorTracker.ts | 3 +- .../files/common/editors/fileEditorInput.ts | 2 +- .../parts/search/browser/openFileHandler.ts | 4 +- .../media/update.contribution.css | 2 +- .../textfile/common/textFileEditorModel.ts | 3 +- .../workspace/node/workspaceEditingService.ts | 62 ++----------------- 16 files changed, 27 insertions(+), 127 deletions(-) diff --git a/src/vs/base/parts/quickopen/browser/quickOpenModel.ts b/src/vs/base/parts/quickopen/browser/quickOpenModel.ts index 773a4b79b4d..914878e5454 100644 --- a/src/vs/base/parts/quickopen/browser/quickOpenModel.ts +++ b/src/vs/base/parts/quickopen/browser/quickOpenModel.ts @@ -171,8 +171,13 @@ export class QuickOpenEntry { return false; } - public isFile(): boolean { - return false; // TODO@Ben debt with editor history merging + /** + * Determines if this quick open entry should merge with the editor history in quick open. If set to true + * and the resource of this entry is the same as the resource for an editor history, it will not show up + * because it is considered to be a duplicate of an editor history. + */ + public mergeWithEditorHistory(): boolean { + return false; } } @@ -412,8 +417,6 @@ class Renderer implements IRenderer { data.actionBar.context = entry; // make sure the context is the current element this.actionProvider.getActions(null, entry).then((actions) => { - // TODO@Ben this will not work anymore as soon as quick open has more actions - // but as long as there is only one are ok if (data.actionBar.isEmpty() && actions && actions.length > 0) { data.actionBar.push(actions, { icon: true, label: false }); } else if (!data.actionBar.isEmpty() && (!actions || actions.length === 0)) { diff --git a/src/vs/code/electron-main/window.ts b/src/vs/code/electron-main/window.ts index 6f0871deb7a..fbb29ab568c 100644 --- a/src/vs/code/electron-main/window.ts +++ b/src/vs/code/electron-main/window.ts @@ -194,7 +194,7 @@ export class CodeWindow implements ICodeWindow { this._win = new BrowserWindow(options); this._id = this._win.id; - // TODO@Ben Bug in Electron (https://github.com/electron/electron/issues/10862). On multi-monitor setups, + // Bug in Electron (https://github.com/electron/electron/issues/10862). On multi-monitor setups, // it can happen that the position we set to the window is not the correct one on the display. // To workaround, we ask the window for its position and set it again if not matching. // This only applies if the window is not fullscreen or maximized and multiple monitors are used. diff --git a/src/vs/code/electron-main/windows.ts b/src/vs/code/electron-main/windows.ts index 1561c4bb792..2dab6f94941 100644 --- a/src/vs/code/electron-main/windows.ts +++ b/src/vs/code/electron-main/windows.ts @@ -45,10 +45,6 @@ interface INewWindowState extends ISingleWindowState { hasDefaultState?: boolean; } -interface ILegacyWindowState extends IWindowState { - workspacePath?: string; -} - interface IWindowState { workspace?: IWorkspaceIdentifier; folderPath?: string; @@ -56,10 +52,6 @@ interface IWindowState { uiState: ISingleWindowState; } -interface ILegacyWindowsState extends IWindowsState { - openedFolders?: IWindowState[]; -} - interface IWindowsState { lastActiveWindow?: IWindowState; lastPluginDevelopmentHostWindow?: IWindowState; @@ -151,39 +143,12 @@ export class WindowsManager implements IWindowsMainService { @IInstantiationService private instantiationService: IInstantiationService ) { this.windowsState = this.stateService.getItem(WindowsManager.windowsStateStorageKey) || { openedWindows: [] }; + if (!Array.isArray(this.windowsState.openedWindows)) { + this.windowsState.openedWindows = []; + } this.fileDialog = new FileDialog(environmentService, telemetryService, stateService, this); this.workspacesManager = new WorkspacesManager(workspacesMainService, backupMainService, environmentService, this); - - this.migrateLegacyWindowState(); - } - - private migrateLegacyWindowState(): void { - const state: ILegacyWindowsState = this.windowsState; - - // TODO@Ben migration from previous openedFolders to new openedWindows property - if (Array.isArray(state.openedFolders) && state.openedFolders.length > 0) { - state.openedWindows = state.openedFolders; - state.openedFolders = void 0; - } else if (!state.openedWindows) { - state.openedWindows = []; - } - - // TODO@Ben migration from previous workspacePath in window state to folderPath - const states: ILegacyWindowState[] = []; - states.push(state.lastActiveWindow); - states.push(state.lastPluginDevelopmentHostWindow); - states.push(...state.openedWindows); - states.forEach(state => { - if (!state) { - return; - } - - if (typeof state.workspacePath === 'string') { - state.folderPath = state.workspacePath; - state.workspacePath = void 0; - } - }); } public ready(initialUserEnv: IProcessEnvironment): void { @@ -277,7 +242,7 @@ export class WindowsManager implements IWindowsMainService { // - closeAll(2): onBeforeWindowClose(2, false), onBeforeWindowClose(2, false), onBeforeQuit(0) // private onBeforeQuit(): void { - const currentWindowsState: ILegacyWindowsState = { + const currentWindowsState: IWindowsState = { openedWindows: [], lastPluginDevelopmentHostWindow: this.windowsState.lastPluginDevelopmentHostWindow, lastActiveWindow: this.lastClosedWindowState @@ -940,10 +905,6 @@ export class WindowsManager implements IWindowsMainService { const windowConfig = this.configurationService.getValue('window'); restoreWindows = ((windowConfig && windowConfig.restoreWindows) || 'one') as RestoreWindowsSetting; - if (restoreWindows === 'one' /* default */ && windowConfig && windowConfig.reopenFolders) { - restoreWindows = windowConfig.reopenFolders; // TODO@Ben migration from deprecated window.reopenFolders setting - } - if (['all', 'folders', 'one', 'none'].indexOf(restoreWindows) === -1) { restoreWindows = 'one'; } diff --git a/src/vs/platform/extensions/common/extensions.ts b/src/vs/platform/extensions/common/extensions.ts index 7c78e26544b..fe757879b07 100644 --- a/src/vs/platform/extensions/common/extensions.ts +++ b/src/vs/platform/extensions/common/extensions.ts @@ -127,7 +127,6 @@ export interface IExtensionService { _serviceBrand: any; /** - * TODO@Ben: Delete this and use `whenInstalledExtensionsRegistered` * An event emitted when extensions are registered after their extension points got handled. * * This event will also fire on startup to signal the installed extensions. diff --git a/src/vs/platform/history/electron-main/historyMainService.ts b/src/vs/platform/history/electron-main/historyMainService.ts index 7e20f516ecf..761b5c05265 100644 --- a/src/vs/platform/history/electron-main/historyMainService.ts +++ b/src/vs/platform/history/electron-main/historyMainService.ts @@ -22,10 +22,6 @@ import { IEnvironmentService } from 'vs/platform/environment/common/environment' import { isEqual } from 'vs/base/common/paths'; import { RunOnceScheduler } from 'vs/base/common/async'; -export interface ILegacyRecentlyOpened extends IRecentlyOpened { - folders: string[]; // TODO@Ben migration -} - export class HistoryMainService implements IHistoryMainService { private static readonly MAX_TOTAL_RECENT_ENTRIES = 100; @@ -179,9 +175,9 @@ export class HistoryMainService implements IHistoryMainService { let files: string[]; // Get from storage - const storedRecents = this.stateService.getItem(HistoryMainService.recentlyOpenedStorageKey) as ILegacyRecentlyOpened; + const storedRecents = this.stateService.getItem(HistoryMainService.recentlyOpenedStorageKey); if (storedRecents) { - workspaces = storedRecents.workspaces || storedRecents.folders || []; + workspaces = storedRecents.workspaces || []; files = storedRecents.files || []; } else { workspaces = []; diff --git a/src/vs/platform/telemetry/common/telemetryUtils.ts b/src/vs/platform/telemetry/common/telemetryUtils.ts index 4890d1d609a..41c3db8d63f 100644 --- a/src/vs/platform/telemetry/common/telemetryUtils.ts +++ b/src/vs/platform/telemetry/common/telemetryUtils.ts @@ -134,7 +134,6 @@ const configurationValueWhitelist = [ 'workbench.sideBar.location', 'window.openFilesInNewWindow', 'javascript.validate.enable', - 'window.reopenFolders', 'window.restoreWindows', 'extensions.autoUpdate', 'files.eol', diff --git a/src/vs/platform/windows/common/windows.ts b/src/vs/platform/windows/common/windows.ts index 29af4d3a449..ca7ec0fcb2e 100644 --- a/src/vs/platform/windows/common/windows.ts +++ b/src/vs/platform/windows/common/windows.ts @@ -207,7 +207,6 @@ export interface IWindowSettings { openFilesInNewWindow: 'on' | 'off' | 'default'; openFoldersInNewWindow: 'on' | 'off' | 'default'; restoreWindows: 'all' | 'folders' | 'one' | 'none'; - reopenFolders: 'all' | 'one' | 'none'; // TODO@Ben deprecated restoreFullscreen: boolean; zoomLevel: number; titleBarStyle: 'native' | 'custom'; diff --git a/src/vs/workbench/browser/parts/quickopen/quickOpenController.ts b/src/vs/workbench/browser/parts/quickopen/quickOpenController.ts index b594ff1ea96..c5e59196388 100644 --- a/src/vs/workbench/browser/parts/quickopen/quickOpenController.ts +++ b/src/vs/workbench/browser/parts/quickopen/quickOpenController.ts @@ -857,7 +857,7 @@ export class QuickOpenController extends Component implements IQuickOpenService const result = handlerResults[i]; const resource = result.getResource(); - if (!result.isFile() || !resource || !mapEntryToResource[resource.toString()]) { + if (!result.mergeWithEditorHistory() || !resource || !mapEntryToResource[resource.toString()]) { additionalHandlerResults.push(result); } } diff --git a/src/vs/workbench/common/editor/resourceEditorInput.ts b/src/vs/workbench/common/editor/resourceEditorInput.ts index 4f9a57314ec..850154f02bc 100644 --- a/src/vs/workbench/common/editor/resourceEditorInput.ts +++ b/src/vs/workbench/common/editor/resourceEditorInput.ts @@ -93,7 +93,8 @@ export class ResourceEditorInput extends EditorInput { if (!(model instanceof ResourceEditorModel)) { ref.dispose(); this.modelReference = null; - return TPromise.wrapError(new Error(`Unexpected model for ResourceInput: ${this.resource}`)); // TODO@Ben eventually also files should be supported, but we guard due to the dangerous dispose of the model in dispose() + + return TPromise.wrapError(new Error(`Unexpected model for ResourceInput: ${this.resource}`)); } return model; diff --git a/src/vs/workbench/parts/backup/common/backupRestorer.ts b/src/vs/workbench/parts/backup/common/backupRestorer.ts index d1b4010f586..6e1d90e3e7e 100644 --- a/src/vs/workbench/parts/backup/common/backupRestorer.ts +++ b/src/vs/workbench/parts/backup/common/backupRestorer.ts @@ -93,8 +93,6 @@ export class BackupRestorer implements IWorkbenchContribution { const options = { pinned: true, preserveFocus: true, inactive: index > 0 || hasOpenedEditors }; if (resource.scheme === UNTITLED_SCHEMA && !BackupRestorer.UNTITLED_REGEX.test(resource.fsPath)) { - // TODO@Ben debt: instead of guessing if an untitled file has an associated file path or not - // this information should be provided by the backup service and stored as meta data within return { filePath: resource.fsPath, options }; } diff --git a/src/vs/workbench/parts/files/browser/editors/fileEditorTracker.ts b/src/vs/workbench/parts/files/browser/editors/fileEditorTracker.ts index 1baf91a02d9..a4b89ff6e86 100644 --- a/src/vs/workbench/parts/files/browser/editors/fileEditorTracker.ts +++ b/src/vs/workbench/parts/files/browser/editors/fileEditorTracker.ts @@ -142,8 +142,7 @@ export class FileEditorTracker implements IWorkbenchContribution { // We have received reports of users seeing delete events even though the file still // exists (network shares issue: https://github.com/Microsoft/vscode/issues/13665). // Since we do not want to close an editor without reason, we have to check if the - // file is really gone and not just a faulty file event (TODO@Ben revisit when we - // have a more stable file watcher in place for this scenario). + // file is really gone and not just a faulty file event. // This only applies to external file events, so we need to check for the isExternal // flag. let checkExists: TPromise; diff --git a/src/vs/workbench/parts/files/common/editors/fileEditorInput.ts b/src/vs/workbench/parts/files/common/editors/fileEditorInput.ts index 0b7c07c9f74..f7994c2ca25 100644 --- a/src/vs/workbench/parts/files/common/editors/fileEditorInput.ts +++ b/src/vs/workbench/parts/files/common/editors/fileEditorInput.ts @@ -240,7 +240,7 @@ export class FileEditorInput extends EditorInput implements IFileEditorInput { // Resolve as text return this.textFileService.models.loadOrCreate(this.resource, { encoding: this.preferredEncoding, reload: refresh }).then(model => { - // TODO@Ben this is a bit ugly, because we first resolve the model and then resolve a model reference. the reason being that binary + // This is a bit ugly, because we first resolve the model and then resolve a model reference. the reason being that binary // or very large files do not resolve to a text file model but should be opened as binary files without text. First calling into // loadOrCreate ensures we are not creating model references for these kind of resources. // In addition we have a bit of payload to take into account (encoding, reload) that the text resolver does not handle yet. diff --git a/src/vs/workbench/parts/search/browser/openFileHandler.ts b/src/vs/workbench/parts/search/browser/openFileHandler.ts index 3a85e381b07..04e8cad5150 100644 --- a/src/vs/workbench/parts/search/browser/openFileHandler.ts +++ b/src/vs/workbench/parts/search/browser/openFileHandler.ts @@ -88,8 +88,8 @@ export class FileEntry extends EditorQuickOpenEntry { this.range = range; } - public isFile(): boolean { - return true; // TODO@Ben debt with editor history merging + public mergeWithEditorHistory(): boolean { + return true; } public getInput(): IResourceInput | EditorInput { diff --git a/src/vs/workbench/parts/update/electron-browser/media/update.contribution.css b/src/vs/workbench/parts/update/electron-browser/media/update.contribution.css index b0a88187489..4e602f932a2 100644 --- a/src/vs/workbench/parts/update/electron-browser/media/update.contribution.css +++ b/src/vs/workbench/parts/update/electron-browser/media/update.contribution.css @@ -8,7 +8,7 @@ -webkit-mask-size: 22px; } -/* HACK @bpasero @ben */ +/* TODO@Ben this is a hack to overwrite the icon for release notes eitor */ .file-icons-enabled .show-file-icons .release-notes-ext-file-icon.file-icon::before { content: ' '; background-image: url('code-icon.svg'); diff --git a/src/vs/workbench/services/textfile/common/textFileEditorModel.ts b/src/vs/workbench/services/textfile/common/textFileEditorModel.ts index ee7360e86b3..4905dff62b3 100644 --- a/src/vs/workbench/services/textfile/common/textFileEditorModel.ts +++ b/src/vs/workbench/services/textfile/common/textFileEditorModel.ts @@ -145,8 +145,7 @@ export class TextFileEditorModel extends BaseTextEditorModel implements ITextFil // We have received reports of users seeing delete events even though the file still // exists (network shares issue: https://github.com/Microsoft/vscode/issues/13665). // Since we do not want to mark the model as orphaned, we have to check if the - // file is really gone and not just a faulty file event (TODO@Ben revisit when we - // have a more stable file watcher in place for this scenario). + // file is really gone and not just a faulty file event. checkOrphanedPromise = TPromise.timeout(100).then(() => { if (this.disposed) { return true; diff --git a/src/vs/workbench/services/workspace/node/workspaceEditingService.ts b/src/vs/workbench/services/workspace/node/workspaceEditingService.ts index 5803459ade3..7bb21530f42 100644 --- a/src/vs/workbench/services/workspace/node/workspaceEditingService.ts +++ b/src/vs/workbench/services/workspace/node/workspaceEditingService.ts @@ -16,7 +16,7 @@ import { IWorkspaceIdentifier, IWorkspaceFolderCreationData } from 'vs/platform/ import { IWorkspaceConfigurationService } from 'vs/workbench/services/configuration/common/configuration'; import { WorkspaceService } from 'vs/workbench/services/configuration/node/configurationService'; import { migrateStorageToMultiRootWorkspace } from 'vs/platform/storage/common/migration'; -import { IStorageService, StorageScope } from 'vs/platform/storage/common/storage'; +import { IStorageService } from 'vs/platform/storage/common/storage'; import { StorageService } from 'vs/platform/storage/common/storageService'; import { ConfigurationScope, IConfigurationRegistry, Extensions as ConfigurationExtensions } from 'vs/platform/configuration/common/configurationRegistry'; import { Registry } from 'vs/platform/registry/common/platform'; @@ -28,15 +28,11 @@ import { ICommandService } from 'vs/platform/commands/common/commands'; import { distinct } from 'vs/base/common/arrays'; import { isLinux } from 'vs/base/common/platform'; import { isEqual } from 'vs/base/common/resources'; -import { Action } from 'vs/base/common/actions'; -import product from 'vs/platform/node/product'; export class WorkspaceEditingService implements IWorkspaceEditingService { public _serviceBrand: any; - private static readonly INFO_MESSAGE_KEY = 'enterWorkspace.message'; - constructor( @IJSONEditingService private jsonEditingService: IJSONEditingService, @IWorkspaceContextService private contextService: WorkspaceService, @@ -145,17 +141,14 @@ export class WorkspaceEditingService implements IWorkspaceEditingService { if (result) { return this.migrate(result.workspace).then(() => { - // Show message to user (once) if entering workspace state - if (this.contextService.getWorkbenchState() !== WorkbenchState.WORKSPACE) { - this.informUserOnce(); // TODO@Ben remove me after a couple of releases - } + // TODO@Ben TODO@Sandeep the following requires ugly casts and should probably have a service interface // Reinitialize backup service - const backupFileService = this.backupFileService as BackupFileService; // TODO@Ben ugly cast + const backupFileService = this.backupFileService as BackupFileService; backupFileService.initialize(result.backupPath); // Reinitialize configuration service - const workspaceImpl = this.contextService as WorkspaceService; // TODO@Ben TODO@Sandeep ugly cast + const workspaceImpl = this.contextService as WorkspaceService; return workspaceImpl.initialize(result.workspace); }); } @@ -168,53 +161,6 @@ export class WorkspaceEditingService implements IWorkspaceEditingService { }); } - private informUserOnce(): void { - if (product.quality !== 'stable') { - return; // only for stable - } - - if (this.storageService.getBoolean(WorkspaceEditingService.INFO_MESSAGE_KEY)) { - return; // user does not want to see it again - } - - const closeAction = new Action( - 'enterWorkspace.close', - nls.localize('enterWorkspace.close', "Close"), - null, - true, - () => TPromise.as(true) - ); - - const dontShowAgainAction = new Action( - 'enterWorkspace.dontShowAgain', - nls.localize('enterWorkspace.dontShowAgain', "Don't Show Again"), - null, - true, - () => { - this.storageService.store(WorkspaceEditingService.INFO_MESSAGE_KEY, true, StorageScope.GLOBAL); - - return TPromise.as(true); - } - ); - const moreInfoAction = new Action( - 'enterWorkspace.moreInfo', - nls.localize('enterWorkspace.moreInfo', "More Information"), - null, - true, - () => { - const uri = URI.parse('https://go.microsoft.com/fwlink/?linkid=861970'); - window.open(uri.toString(true)); - - return TPromise.as(true); - } - ); - - this.messageService.show(Severity.Info, { - message: nls.localize('enterWorkspace.prompt', "Learn more about working with multiple folders in VS Code."), - actions: [moreInfoAction, dontShowAgainAction, closeAction] - }); - } - private migrate(toWorkspace: IWorkspaceIdentifier): TPromise { // Storage (UI State) migration -- GitLab