From 9b2a6c6cfbde8be1363cbfcc70f772e4f75d46f1 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Wed, 25 Jan 2017 16:40:26 -0800 Subject: [PATCH 1/7] Remove unused imports from DocumentFormatter.ts --- src/features/DocumentFormatter.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/features/DocumentFormatter.ts b/src/features/DocumentFormatter.ts index 3533f218b3..b387cbaf14 100644 --- a/src/features/DocumentFormatter.ts +++ b/src/features/DocumentFormatter.ts @@ -4,7 +4,6 @@ import vscode = require('vscode'); import { - languages, TextDocument, TextEdit, FormattingOptions, @@ -13,7 +12,7 @@ import { DocumentRangeFormattingEditProvider, Range, } from 'vscode'; -import { LanguageClient, RequestType, NotificationType } from 'vscode-languageclient'; +import { LanguageClient, RequestType } from 'vscode-languageclient'; import Window = vscode.window; import { IFeature } from '../feature'; import * as Settings from '../settings'; From 85a80e069176662a86afc555ad2b607e74fd1058 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Wed, 25 Jan 2017 18:47:44 -0800 Subject: [PATCH 2/7] Prevent concurrent formatting requests for a document A user can execute `Format Document` command on a document even if the same document is already being formatted. This sends another, redundant, formatting request to the server for the same file. Another annoying side effect is that a new formatting status for the same file gets created on the status bar. --- src/features/DocumentFormatter.ts | 35 ++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/src/features/DocumentFormatter.ts b/src/features/DocumentFormatter.ts index b387cbaf14..10150120f6 100644 --- a/src/features/DocumentFormatter.ts +++ b/src/features/DocumentFormatter.ts @@ -59,7 +59,7 @@ interface ScriptRegion { interface MarkerCorrection { name: string; - edits: ScriptRegion[] + edits: ScriptRegion[]; } function editComparer(leftOperand: ScriptRegion, rightOperand: ScriptRegion): number { @@ -81,6 +81,7 @@ function editComparer(leftOperand: ScriptRegion, rightOperand: ScriptRegion): nu } class PSDocumentFormattingEditProvider implements DocumentFormattingEditProvider, DocumentRangeFormattingEditProvider { + private static filesBeingFormatted: Object = new Object; private languageClient: LanguageClient; // The order in which the rules will be executed starting from the first element. @@ -111,11 +112,43 @@ class PSDocumentFormattingEditProvider implements DocumentFormattingEditProvider options: FormattingOptions, token: CancellationToken): TextEdit[] | Thenable { + if (this.isDocumentLocked(document)) { + return; + } + + this.lockDocument(document); let textEdits: Thenable = this.executeRulesInOrder(document, range, options, 0); + this.releaseDocument(document, textEdits); AnimatedStatusBar.showAnimatedStatusBarMessage("Formatting PowerShell document", textEdits); return textEdits; } + isDocumentLocked(document: TextDocument): boolean { + if (PSDocumentFormattingEditProvider.filesBeingFormatted.hasOwnProperty(this.getDocumentKey(document))) { + return true; + } + + return false; + } + + lockDocument(document: TextDocument): void { + if (!this.isDocumentLocked(document)) { + PSDocumentFormattingEditProvider.filesBeingFormatted[this.getDocumentKey(document)] = true; + } + } + + releaseDocument(document: TextDocument, releaseWhenDone: Thenable): void { + if (this.isDocumentLocked(document)) { + releaseWhenDone.then(() => { + delete PSDocumentFormattingEditProvider.filesBeingFormatted[this.getDocumentKey(document)]; + }); + } + } + + getDocumentKey(document: TextDocument): string { + return document.uri.toString(); + } + executeRulesInOrder( document: TextDocument, range: Range, From ed9451a16db95e1b3ad78b17f6bc4417d54761bf Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Wed, 25 Jan 2017 19:44:44 -0800 Subject: [PATCH 3/7] Prevent spillover of edits to another editor If formatting is in progress in a given editor and if we change the active editor then the correction edits can get written to the new active editor. --- src/features/DocumentFormatter.ts | 49 +++++++++++++++++++++++-------- 1 file changed, 37 insertions(+), 12 deletions(-) diff --git a/src/features/DocumentFormatter.ts b/src/features/DocumentFormatter.ts index 10150120f6..9b34a82f2d 100644 --- a/src/features/DocumentFormatter.ts +++ b/src/features/DocumentFormatter.ts @@ -2,6 +2,7 @@ * Copyright (C) Microsoft Corporation. All rights reserved. *--------------------------------------------------------*/ +import * as path from "path"; import vscode = require('vscode'); import { TextDocument, @@ -11,6 +12,7 @@ import { DocumentFormattingEditProvider, DocumentRangeFormattingEditProvider, Range, + TextEditor } from 'vscode'; import { LanguageClient, RequestType } from 'vscode-languageclient'; import Window = vscode.window; @@ -95,6 +97,10 @@ class PSDocumentFormattingEditProvider implements DocumentFormattingEditProvider // hence we keep this as an option but set it true by default. private aggregateUndoStop: boolean; + private get emptyPromise(): Promise { + return Promise.resolve(TextEdit[0]); + } + constructor(aggregateUndoStop = true) { this.aggregateUndoStop = aggregateUndoStop; } @@ -112,17 +118,28 @@ class PSDocumentFormattingEditProvider implements DocumentFormattingEditProvider options: FormattingOptions, token: CancellationToken): TextEdit[] | Thenable { + let editor: TextEditor = this.getEditor(document); + if (editor === undefined) { + Window.showWarningMessage(`Something went wrong. Cannot format ${path.basename(document.fileName)}`); + return this.emptyPromise; + } + if (this.isDocumentLocked(document)) { - return; + Window.showWarningMessage(`Formatting ${path.basename(document.fileName)}. Please wait.`); + return this.emptyPromise; } this.lockDocument(document); - let textEdits: Thenable = this.executeRulesInOrder(document, range, options, 0); + let textEdits: Thenable = this.executeRulesInOrder(editor, range, options, 0); this.releaseDocument(document, textEdits); AnimatedStatusBar.showAnimatedStatusBarMessage("Formatting PowerShell document", textEdits); return textEdits; } + getEditor(document: TextDocument): TextEditor { + return Window.visibleTextEditors.find((e, n, obj) => { return e.document === document; }); + } + isDocumentLocked(document: TextDocument): boolean { if (PSDocumentFormattingEditProvider.filesBeingFormatted.hasOwnProperty(this.getDocumentKey(document))) { return true; @@ -150,13 +167,14 @@ class PSDocumentFormattingEditProvider implements DocumentFormattingEditProvider } executeRulesInOrder( - document: TextDocument, + editor: TextEditor, range: Range, options: FormattingOptions, index: number): Thenable { if (this.languageClient !== null && index < this.ruleOrder.length) { - let rule = this.ruleOrder[index]; + let rule: string = this.ruleOrder[index]; let uniqueEdits: ScriptRegion[] = []; + let document: TextDocument = editor.document; let edits: ScriptRegion[]; return this.languageClient.sendRequest( @@ -197,22 +215,29 @@ class PSDocumentFormattingEditProvider implements DocumentFormattingEditProvider // we do not return a valid array because our text edits // need to be executed in a particular order and it is // easier if we perform the edits ourselves - return this.applyEdit(uniqueEdits, range, 0, index); + return this.applyEdit(editor, uniqueEdits, range, 0, index); }) .then(() => { // execute the same rule again if we left out violations // on the same line + let newIndex: number = index + 1; if (uniqueEdits.length !== edits.length) { - return this.executeRulesInOrder(document, range, options, index); + newIndex = index; } - return this.executeRulesInOrder(document, range, options, index + 1); + + return this.executeRulesInOrder(editor, range, options, newIndex); }); } else { - return Promise.resolve(TextEdit[0]); + return this.emptyPromise; } } - applyEdit(edits: ScriptRegion[], range: Range, markerIndex: number, ruleIndex: number): Thenable { + applyEdit( + editor: TextEditor, + edits: ScriptRegion[], + range: Range, + markerIndex: number, + ruleIndex: number): Thenable { if (markerIndex >= edits.length) { return; } @@ -226,7 +251,7 @@ class PSDocumentFormattingEditProvider implements DocumentFormattingEditProvider edit.endLineNumber - 1, edit.endColumnNumber - 1); if (range === null || range.contains(editRange)) { - return Window.activeTextEditor.edit((editBuilder) => { + return editor.edit((editBuilder) => { editBuilder.replace( editRange, edit.text); @@ -235,11 +260,11 @@ class PSDocumentFormattingEditProvider implements DocumentFormattingEditProvider undoStopAfter: undoStopAfter, undoStopBefore: undoStopBefore }).then((isEditApplied) => { - return this.applyEdit(edits, range, markerIndex + 1, ruleIndex); + return this.applyEdit(editor, edits, range, markerIndex + 1, ruleIndex); }); // TODO handle rejection } else { - return this.applyEdit(edits, range, markerIndex + 1, ruleIndex); + return this.applyEdit(editor, edits, range, markerIndex + 1, ruleIndex); } } From 73201bb1766a196e2f00ccdd9be9bdfd4f6ac476 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Thu, 26 Jan 2017 09:23:50 -0800 Subject: [PATCH 4/7] Add inline documentation to DocumentFormatter.ts --- src/features/DocumentFormatter.ts | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/features/DocumentFormatter.ts b/src/features/DocumentFormatter.ts index 9b34a82f2d..2e8aff161f 100644 --- a/src/features/DocumentFormatter.ts +++ b/src/features/DocumentFormatter.ts @@ -120,17 +120,23 @@ class PSDocumentFormattingEditProvider implements DocumentFormattingEditProvider let editor: TextEditor = this.getEditor(document); if (editor === undefined) { - Window.showWarningMessage(`Something went wrong. Cannot format ${path.basename(document.fileName)}`); return this.emptyPromise; } + // Check if the document is already being formatted. + // If so, then ignore the formatting request. if (this.isDocumentLocked(document)) { - Window.showWarningMessage(`Formatting ${path.basename(document.fileName)}. Please wait.`); return this.emptyPromise; } this.lockDocument(document); let textEdits: Thenable = this.executeRulesInOrder(editor, range, options, 0); + + // If the session crashes for any reason during formatting + // then the document won't be released and hence we won't + // be able to format it after restarting the session. + // Similar issue with the status - the bar will keep displaying + // the previous status even after restarting the session. this.releaseDocument(document, textEdits); AnimatedStatusBar.showAnimatedStatusBarMessage("Formatting PowerShell document", textEdits); return textEdits; From 0e9df49f2eff006ff51e52e7953f135144009f40 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Mon, 30 Jan 2017 11:16:54 -0800 Subject: [PATCH 5/7] Move document locking logic to own class --- src/features/DocumentFormatter.ts | 79 +++++++++++++++++++++---------- 1 file changed, 55 insertions(+), 24 deletions(-) diff --git a/src/features/DocumentFormatter.ts b/src/features/DocumentFormatter.ts index 2e8aff161f..2ec9d4b5ff 100644 --- a/src/features/DocumentFormatter.ts +++ b/src/features/DocumentFormatter.ts @@ -82,8 +82,56 @@ function editComparer(leftOperand: ScriptRegion, rightOperand: ScriptRegion): nu } } +class DocumentLocker { + private lockedDocuments: Object; + + constructor() { + this.lockedDocuments = new Object(); + } + + isLocked(document: TextDocument): boolean { + return this.isLockedInternal(this.getKey(document)); + } + + lock(document: TextDocument, unlockWhenDone?: Thenable): void { + this.lockInternal(this.getKey(document), unlockWhenDone); + } + + unlock(document: TextDocument): void { + this.unlockInternal(this.getKey(document)); + } + + unlockAll(): void { + Object.keys(this.lockedDocuments).slice().forEach(documentKey => this.unlockInternal(documentKey)); + } + + private getKey(document: TextDocument): string { + return document.uri.toString(); + } + + private lockInternal(documentKey: string, unlockWhenDone?: Thenable): void { + if (!this.isLockedInternal(documentKey)) { + this.lockedDocuments[documentKey] = true; + } + + if (unlockWhenDone !== undefined) { + unlockWhenDone.then(() => this.unlockInternal(documentKey)); + } + } + + private unlockInternal(documentKey: string): void { + if (this.isLockedInternal(documentKey)) { + delete this.lockedDocuments[documentKey]; + } + } + + private isLockedInternal(documentKey: string): boolean { + return this.lockedDocuments.hasOwnProperty(documentKey); + } +} + class PSDocumentFormattingEditProvider implements DocumentFormattingEditProvider, DocumentRangeFormattingEditProvider { - private static filesBeingFormatted: Object = new Object; + private static documentLocker = new DocumentLocker(); private languageClient: LanguageClient; // The order in which the rules will be executed starting from the first element. @@ -129,15 +177,15 @@ class PSDocumentFormattingEditProvider implements DocumentFormattingEditProvider return this.emptyPromise; } - this.lockDocument(document); let textEdits: Thenable = this.executeRulesInOrder(editor, range, options, 0); + this.lockDocument(document, textEdits); // If the session crashes for any reason during formatting // then the document won't be released and hence we won't // be able to format it after restarting the session. // Similar issue with the status - the bar will keep displaying // the previous status even after restarting the session. - this.releaseDocument(document, textEdits); + // this.releaseDocument(document, textEdits); AnimatedStatusBar.showAnimatedStatusBarMessage("Formatting PowerShell document", textEdits); return textEdits; } @@ -147,29 +195,11 @@ class PSDocumentFormattingEditProvider implements DocumentFormattingEditProvider } isDocumentLocked(document: TextDocument): boolean { - if (PSDocumentFormattingEditProvider.filesBeingFormatted.hasOwnProperty(this.getDocumentKey(document))) { - return true; - } - - return false; + return PSDocumentFormattingEditProvider.documentLocker.isLocked(document); } - lockDocument(document: TextDocument): void { - if (!this.isDocumentLocked(document)) { - PSDocumentFormattingEditProvider.filesBeingFormatted[this.getDocumentKey(document)] = true; - } - } - - releaseDocument(document: TextDocument, releaseWhenDone: Thenable): void { - if (this.isDocumentLocked(document)) { - releaseWhenDone.then(() => { - delete PSDocumentFormattingEditProvider.filesBeingFormatted[this.getDocumentKey(document)]; - }); - } - } - - getDocumentKey(document: TextDocument): string { - return document.uri.toString(); + lockDocument(document: TextDocument, unlockWhenDone: Thenable): void { + PSDocumentFormattingEditProvider.documentLocker.lock(document, unlockWhenDone); } executeRulesInOrder( @@ -285,6 +315,7 @@ class PSDocumentFormattingEditProvider implements DocumentFormattingEditProvider setLanguageClient(languageClient: LanguageClient): void { this.languageClient = languageClient; + PSDocumentFormattingEditProvider.documentLocker.unlockAll(); } getSettings(rule: string): any { From 0d35540df5131a3cfe1ac8d5988b5ba7e68a1905 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Mon, 30 Jan 2017 13:31:45 -0800 Subject: [PATCH 6/7] Clean up format status during session restart --- src/features/DocumentFormatter.ts | 39 +++++++++++++++++++++++++------ 1 file changed, 32 insertions(+), 7 deletions(-) diff --git a/src/features/DocumentFormatter.ts b/src/features/DocumentFormatter.ts index 2ec9d4b5ff..a188b98f02 100644 --- a/src/features/DocumentFormatter.ts +++ b/src/features/DocumentFormatter.ts @@ -132,6 +132,7 @@ class DocumentLocker { class PSDocumentFormattingEditProvider implements DocumentFormattingEditProvider, DocumentRangeFormattingEditProvider { private static documentLocker = new DocumentLocker(); + private static statusBarTracker = new Object(); private languageClient: LanguageClient; // The order in which the rules will be executed starting from the first element. @@ -180,13 +181,11 @@ class PSDocumentFormattingEditProvider implements DocumentFormattingEditProvider let textEdits: Thenable = this.executeRulesInOrder(editor, range, options, 0); this.lockDocument(document, textEdits); - // If the session crashes for any reason during formatting - // then the document won't be released and hence we won't - // be able to format it after restarting the session. - // Similar issue with the status - the bar will keep displaying - // the previous status even after restarting the session. - // this.releaseDocument(document, textEdits); - AnimatedStatusBar.showAnimatedStatusBarMessage("Formatting PowerShell document", textEdits); + // FIXME If the session crashes for any reason during formatting + // then the bar will keep displaying the previous status even after restarting the session. + // A fix is to restart the window + // AnimatedStatusBar.showAnimatedStatusBarMessage("Formatting PowerShell document", textEdits); + PSDocumentFormattingEditProvider.showStatusBar(document, textEdits); return textEdits; } @@ -315,7 +314,12 @@ class PSDocumentFormattingEditProvider implements DocumentFormattingEditProvider setLanguageClient(languageClient: LanguageClient): void { this.languageClient = languageClient; + + // setLanguageClient is called while restarting a session, + // so this makes sure we clean up the document locker and + // any residual status bars PSDocumentFormattingEditProvider.documentLocker.unlockAll(); + PSDocumentFormattingEditProvider.disposeAllStatusBars(); } getSettings(rule: string): any { @@ -341,6 +345,27 @@ class PSDocumentFormattingEditProvider implements DocumentFormattingEditProvider settings[rule] = ruleSettings; return settings; } + + private static showStatusBar(document: TextDocument, hideWhenDone: Thenable): void { + let statusBar = AnimatedStatusBar.showAnimatedStatusBarMessage("Formatting PowerShell document", hideWhenDone); + this.statusBarTracker[document.uri.toString()] = statusBar; + hideWhenDone.then(() => { + this.disposeStatusBar(document.uri.toString()); + }); + } + + private static disposeStatusBar(documentUri: string) { + if (this.statusBarTracker.hasOwnProperty(documentUri)) { + this.statusBarTracker[documentUri].dispose(); + delete this.statusBarTracker[documentUri]; + } + } + + private static disposeAllStatusBars() { + Object.keys(this.statusBarTracker).slice().forEach((key) => this.disposeStatusBar(key)); + } + + } export class DocumentFormatterFeature implements IFeature { From 7a0a142f234a6625c10eda1d53651d27f9ba6958 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Mon, 30 Jan 2017 13:40:37 -0800 Subject: [PATCH 7/7] Add access qualifers to formatter class --- src/features/DocumentFormatter.ts | 41 +++++++++++++------------------ 1 file changed, 17 insertions(+), 24 deletions(-) diff --git a/src/features/DocumentFormatter.ts b/src/features/DocumentFormatter.ts index a188b98f02..7943eb35c7 100644 --- a/src/features/DocumentFormatter.ts +++ b/src/features/DocumentFormatter.ts @@ -180,28 +180,33 @@ class PSDocumentFormattingEditProvider implements DocumentFormattingEditProvider let textEdits: Thenable = this.executeRulesInOrder(editor, range, options, 0); this.lockDocument(document, textEdits); - - // FIXME If the session crashes for any reason during formatting - // then the bar will keep displaying the previous status even after restarting the session. - // A fix is to restart the window - // AnimatedStatusBar.showAnimatedStatusBarMessage("Formatting PowerShell document", textEdits); PSDocumentFormattingEditProvider.showStatusBar(document, textEdits); return textEdits; } - getEditor(document: TextDocument): TextEditor { + setLanguageClient(languageClient: LanguageClient): void { + this.languageClient = languageClient; + + // setLanguageClient is called while restarting a session, + // so this makes sure we clean up the document locker and + // any residual status bars + PSDocumentFormattingEditProvider.documentLocker.unlockAll(); + PSDocumentFormattingEditProvider.disposeAllStatusBars(); + } + + private getEditor(document: TextDocument): TextEditor { return Window.visibleTextEditors.find((e, n, obj) => { return e.document === document; }); } - isDocumentLocked(document: TextDocument): boolean { + private isDocumentLocked(document: TextDocument): boolean { return PSDocumentFormattingEditProvider.documentLocker.isLocked(document); } - lockDocument(document: TextDocument, unlockWhenDone: Thenable): void { + private lockDocument(document: TextDocument, unlockWhenDone: Thenable): void { PSDocumentFormattingEditProvider.documentLocker.lock(document, unlockWhenDone); } - executeRulesInOrder( + private executeRulesInOrder( editor: TextEditor, range: Range, options: FormattingOptions, @@ -267,7 +272,7 @@ class PSDocumentFormattingEditProvider implements DocumentFormattingEditProvider } } - applyEdit( + private applyEdit( editor: TextEditor, edits: ScriptRegion[], range: Range, @@ -303,7 +308,7 @@ class PSDocumentFormattingEditProvider implements DocumentFormattingEditProvider } } - getSelectionRange(document: TextDocument): Range { + private getSelectionRange(document: TextDocument): Range { let editor = vscode.window.visibleTextEditors.find(editor => editor.document === document); if (editor !== undefined) { return editor.selection as Range; @@ -312,17 +317,7 @@ class PSDocumentFormattingEditProvider implements DocumentFormattingEditProvider return null; } - setLanguageClient(languageClient: LanguageClient): void { - this.languageClient = languageClient; - - // setLanguageClient is called while restarting a session, - // so this makes sure we clean up the document locker and - // any residual status bars - PSDocumentFormattingEditProvider.documentLocker.unlockAll(); - PSDocumentFormattingEditProvider.disposeAllStatusBars(); - } - - getSettings(rule: string): any { + private getSettings(rule: string): any { let psSettings: Settings.ISettings = Settings.load(Utils.PowerShellLanguageId); let ruleSettings = new Object(); ruleSettings["Enable"] = true; @@ -364,8 +359,6 @@ class PSDocumentFormattingEditProvider implements DocumentFormattingEditProvider private static disposeAllStatusBars() { Object.keys(this.statusBarTracker).slice().forEach((key) => this.disposeStatusBar(key)); } - - } export class DocumentFormatterFeature implements IFeature {