From 3d09341b1d054e67a5e224f3da683ea1ce92075e Mon Sep 17 00:00:00 2001 From: Andy Jordan <2226434+andyleejordan@users.noreply.github.com> Date: Wed, 16 Aug 2023 12:46:29 -0700 Subject: [PATCH 1/2] Respect file path casing in extension commands Per https://nodejs.org/en/docs/guides/working-with-different-filesystems we should really just keep the file path we're given, no "normalizing" to be done especially since it is wrong to infer from the OS if the filesystem is case-sensitive or not. Gone are the days where Windows and macOS could be presumed case-insensitive, they now both support and are often found with with case-sensitive filesystems. Plus, there was really no reason to be doing this in the first place. Tested interactively, this works just fine since the VS Code APIs we're using handle case-insensitivity for us. Resolves #2960. --- src/features/ExtensionCommands.ts | 147 ++++++++++-------------------- src/session.ts | 17 ++-- 2 files changed, 56 insertions(+), 108 deletions(-) diff --git a/src/features/ExtensionCommands.ts b/src/features/ExtensionCommands.ts index 715aa6f9c4..c7d5275ea2 100644 --- a/src/features/ExtensionCommands.ts +++ b/src/features/ExtensionCommands.ts @@ -1,7 +1,6 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. -import * as os from "os"; import * as path from "path"; import * as vscode from "vscode"; import { @@ -141,6 +140,7 @@ export interface IStatusBarMessageDetails { message: string; timeout?: number; } + interface IInvokeRegisteredEditorCommandParameter { commandName: string; } @@ -161,11 +161,8 @@ export class ExtensionCommandsFeature extends LanguageClientConsumer { vscode.commands.registerCommand("PowerShell.InvokeRegisteredEditorCommand", async (param: IInvokeRegisteredEditorCommandParameter) => { - if (this.extensionCommands.length === 0) { - return; - } - - const commandToExecute = this.extensionCommands.find((x) => x.name === param.commandName); + const commandToExecute = this.extensionCommands.find( + (x) => x.name === param.commandName); if (commandToExecute) { await this.languageClient?.sendRequest( @@ -219,7 +216,8 @@ export class ExtensionCommandsFeature extends LanguageClientConsumer { this.languageClient.onRequest( NewFileRequestType, - // TODO: Shouldn't this use the file path? + // NOTE: The VS Code API does not support naming a file as it's + // opened, only when it's saved. Hence the argument is not used. (_filePath) => this.newFile()), this.languageClient.onRequest( @@ -300,7 +298,7 @@ export class ExtensionCommandsFeature extends LanguageClientConsumer { const selectedCommand = await vscode.window.showQuickPick( quickPickItems, - { placeHolder: "Select a command" }); + { placeHolder: "Select a command..." }); return this.onCommandSelected(selectedCommand, client); } @@ -364,15 +362,16 @@ export class ExtensionCommandsFeature extends LanguageClientConsumer { } private async openFile(openFileDetails: IOpenFileDetails): Promise { - const filePath = await this.normalizeFilePath(openFileDetails.filePath); + const filePath = await this.resolveFilePathWithCwd(openFileDetails.filePath); const doc = await vscode.workspace.openTextDocument(filePath); await vscode.window.showTextDocument(doc, { preview: openFileDetails.preview }); return EditorOperationResponse.Completed; } private async closeFile(filePath: string): Promise { - if (this.findTextDocument(await this.normalizeFilePath(filePath))) { - const doc = await vscode.workspace.openTextDocument(filePath); + filePath = await this.resolveFilePathWithCwd(filePath); + const doc = vscode.workspace.textDocuments.find((x) => x.fileName === filePath); + if (doc != undefined && !doc.isClosed) { await vscode.window.showTextDocument(doc); await vscode.commands.executeCommand("workbench.action.closeActiveEditor"); } @@ -384,7 +383,7 @@ export class ExtensionCommandsFeature extends LanguageClientConsumer { * @param saveFileDetails the object detailing the path of the file to save and optionally its new path to save to */ private async saveFile(saveFileDetails: ISaveFileDetails): Promise { - // Try to interpret the filepath as a URI, defaulting to "file://" if we don't succeed + // Try to interpret the filePath as a URI, defaulting to "file://" if we don't succeed let currentFileUri: vscode.Uri; if (saveFileDetails.filePath.startsWith("untitled") || saveFileDetails.filePath.startsWith("file")) { currentFileUri = vscode.Uri.parse(saveFileDetails.filePath); @@ -392,61 +391,46 @@ export class ExtensionCommandsFeature extends LanguageClientConsumer { currentFileUri = vscode.Uri.file(saveFileDetails.filePath); } - let newFileAbsolutePath: string; - switch (currentFileUri.scheme) { - case "file": { - // If the file to save can't be found, just complete the request - if (!this.findTextDocument(await this.normalizeFilePath(currentFileUri.fsPath))) { - void this.logger.writeAndShowError(`File to save not found: ${currentFileUri.fsPath}.`); - return EditorOperationResponse.Completed; - } + // If the file to save can't be found, just complete the request + const doc = vscode.workspace.textDocuments.find((x) => x.uri === currentFileUri); + if (doc === undefined) { + void this.logger.writeAndShowError(`File to save not found: ${currentFileUri.fsPath}`); + return EditorOperationResponse.Completed; + } + let newFilePath = saveFileDetails.newPath; + if (currentFileUri.scheme === "file") { // If no newFile is given, just save the current file - if (!saveFileDetails.newPath) { - const doc = await vscode.workspace.openTextDocument(currentFileUri.fsPath); + if (newFilePath === undefined) { if (doc.isDirty) { await doc.save(); } return EditorOperationResponse.Completed; } - // Make sure we have an absolute path - if (path.isAbsolute(saveFileDetails.newPath)) { - newFileAbsolutePath = saveFileDetails.newPath; - } else { - // If not, interpret the path as relative to the current file - newFileAbsolutePath = path.join(path.dirname(currentFileUri.fsPath), saveFileDetails.newPath); + // Special case where we interpret a path as relative to the current + // file, not the CWD! + if (!path.isAbsolute(newFilePath)) { + newFilePath = path.join(path.dirname(currentFileUri.fsPath), newFilePath); } - break; } - - case "untitled": { + } else if (currentFileUri.scheme === "untitled") { // We need a new name to save an untitled file - if (!saveFileDetails.newPath) { - // TODO: Create a class handle vscode warnings and errors so we can warn easily - // without logging - void this.logger.writeAndShowWarning("Cannot save untitled file. Try SaveAs(\"path/to/file.ps1\") instead."); + if (newFilePath === undefined) { + void this.logger.writeAndShowError("Cannot save untitled file! Try SaveAs(\"path/to/file.ps1\") instead."); return EditorOperationResponse.Completed; } - // Make sure we have an absolute path - if (path.isAbsolute(saveFileDetails.newPath)) { - newFileAbsolutePath = saveFileDetails.newPath; - } else { - const cwd = await validateCwdSetting(this.logger); - newFileAbsolutePath = path.join(cwd, saveFileDetails.newPath); - } - break; } - - default: { + newFilePath = await this.resolveFilePathWithCwd(newFilePath); + } else { // Other URI schemes are not supported const msg = JSON.stringify(saveFileDetails, undefined, 2); - this.logger.writeVerbose( + void this.logger.writeAndShowError( `<${ExtensionCommandsFeature.name}>: Saving a document with scheme '${currentFileUri.scheme}' ` + - `is currently unsupported. Message: '${msg}'`); - return EditorOperationResponse.Completed; } + `is currently unsupported. Message: '${msg}'`); + return EditorOperationResponse.Completed; } - await this.saveDocumentContentToAbsolutePath(currentFileUri, newFileAbsolutePath); + await this.saveFileAs(doc, newFilePath); return EditorOperationResponse.Completed; } @@ -454,72 +438,33 @@ export class ExtensionCommandsFeature extends LanguageClientConsumer { /** * Take a document available to vscode at the given URI and save it to the given absolute path * @param documentUri the URI of the vscode document to save - * @param destinationAbsolutePath the absolute path to save the document contents to + * @param filePath the absolute path to save the document contents to */ - private async saveDocumentContentToAbsolutePath( - documentUri: vscode.Uri, - destinationAbsolutePath: string): Promise { - // Retrieve the text out of the current document - const oldDocument = await vscode.workspace.openTextDocument(documentUri); - - // Write it to the new document path + private async saveFileAs(doc: vscode.TextDocument, filePath: string): Promise { + // Write the old document's contents to the new document path + const newFileUri = vscode.Uri.file(filePath); try { await vscode.workspace.fs.writeFile( - vscode.Uri.file(destinationAbsolutePath), - Buffer.from(oldDocument.getText())); + newFileUri, + Buffer.from(doc.getText())); } catch (err) { void this.logger.writeAndShowWarning(`<${ExtensionCommandsFeature.name}>: ` + - `Unable to save file to path '${destinationAbsolutePath}': ${err}`); + `Unable to save file to path '${filePath}': ${err}`); return; } // Finally open the new document - const newFileUri = vscode.Uri.file(destinationAbsolutePath); const newFile = await vscode.workspace.openTextDocument(newFileUri); await vscode.window.showTextDocument(newFile, { preview: true }); } - private async normalizeFilePath(filePath: string): Promise { - const cwd = await validateCwdSetting(this.logger); - const platform = os.platform(); - if (platform === "win32") { - // Make sure the file path is absolute - if (!path.win32.isAbsolute(filePath)) { - filePath = path.win32.resolve(cwd, filePath); - } - - // Normalize file path case for comparison for Windows - return filePath.toLowerCase(); - } else { - // Make sure the file path is absolute - if (!path.isAbsolute(filePath)) { - filePath = path.resolve(cwd, filePath); - } - - // macOS is case-insensitive - if (platform === "darwin") { - filePath = filePath.toLowerCase(); - } - - return filePath; + // Resolve file path against user's CWD setting + private async resolveFilePathWithCwd(filePath: string): Promise { + if (!path.isAbsolute(filePath)) { + const cwd = await validateCwdSetting(this.logger); + return path.resolve(cwd, filePath); } - } - - private findTextDocument(filePath: string): boolean { - // since Windows and macOS are case-insensitive, we need to normalize them differently - const canFind = vscode.workspace.textDocuments.find((doc) => { - let docPath: string; - const platform = os.platform(); - if (platform === "win32" || platform === "darwin") { - // for Windows and macOS paths, they are normalized to be lowercase - docPath = doc.fileName.toLowerCase(); - } else { - docPath = doc.fileName; - } - return docPath === filePath; - }); - - return canFind != null; + return filePath; } private setSelection(details: ISetSelectionRequestArguments): EditorOperationResponse { diff --git a/src/session.ts b/src/session.ts index f522ce5fbb..d448de4c8b 100644 --- a/src/session.ts +++ b/src/session.ts @@ -444,10 +444,10 @@ export class SessionManager implements Middleware { // Detect any setting changes that would affect the session. if (!this.suppressRestartPrompt && this.sessionStatus === SessionStatus.Running && - (settings.cwd.toLowerCase() !== this.sessionSettings.cwd.toLowerCase() - || settings.powerShellDefaultVersion.toLowerCase() !== this.sessionSettings.powerShellDefaultVersion.toLowerCase() - || settings.developer.editorServicesLogLevel.toLowerCase() !== this.sessionSettings.developer.editorServicesLogLevel.toLowerCase() - || settings.developer.bundledModulesPath.toLowerCase() !== this.sessionSettings.developer.bundledModulesPath.toLowerCase() + (settings.cwd !== this.sessionSettings.cwd + || settings.powerShellDefaultVersion !== this.sessionSettings.powerShellDefaultVersion + || settings.developer.editorServicesLogLevel !== this.sessionSettings.developer.editorServicesLogLevel + || settings.developer.bundledModulesPath !== this.sessionSettings.developer.bundledModulesPath || settings.developer.editorServicesWaitForDebugger !== this.sessionSettings.developer.editorServicesWaitForDebugger || settings.integratedConsole.useLegacyReadLine !== this.sessionSettings.integratedConsole.useLegacyReadLine || settings.integratedConsole.startInBackground !== this.sessionSettings.integratedConsole.startInBackground @@ -872,7 +872,8 @@ Type 'help' to get help. prompt: "Open an Issue", action: async (): Promise => { await vscode.commands.executeCommand("PowerShell.GenerateBugReport"); - }}] + } + }] ); } @@ -883,7 +884,8 @@ Type 'help' to get help. action: async (): Promise => { await vscode.env.openExternal( vscode.Uri.parse("https://aka.ms/get-powershell-vscode")); - }}] + } + }] ); } @@ -894,7 +896,8 @@ Type 'help' to get help. action: async (): Promise => { await vscode.env.openExternal( vscode.Uri.parse("https://dotnet.microsoft.com/en-us/download/dotnet-framework")); - }}] + } + }] ); } From ae7230700ec690dd5dcf796ac355c73f6cb528e9 Mon Sep 17 00:00:00 2001 From: Andy Jordan <2226434+andyleejordan@users.noreply.github.com> Date: Wed, 16 Aug 2023 18:43:42 -0700 Subject: [PATCH 2/2] Report errors from extension API commands The response is now `Completed` or `Failed` and returned appropriately. `NewFile` now takes (optional) content and fills the new file with it (which is all the VS Code API can do). `OpenFile` doesn't throw a cryptic error in the terminal if the file isn't found. The status bar messages are now properly disposed. --- src/features/ExtensionCommands.ts | 72 ++++++++++++++++++------------- 1 file changed, 42 insertions(+), 30 deletions(-) diff --git a/src/features/ExtensionCommands.ts b/src/features/ExtensionCommands.ts index c7d5275ea2..3f8f77d48e 100644 --- a/src/features/ExtensionCommands.ts +++ b/src/features/ExtensionCommands.ts @@ -68,9 +68,11 @@ export const GetEditorContextRequestType = export interface IGetEditorContextRequestArguments { } +// NOTE: The server at least now expects this response, but it's not used in any +// way. In the future we could actually communicate an error to the user. enum EditorOperationResponse { - Unsupported = 0, Completed, + Failed } export const InsertTextRequestType = @@ -148,6 +150,7 @@ interface IInvokeRegisteredEditorCommandParameter { export class ExtensionCommandsFeature extends LanguageClientConsumer { private commands: vscode.Disposable[]; private handlers: vscode.Disposable[] = []; + private statusBarMessages: vscode.Disposable[] = []; private extensionCommands: IExtensionCommand[] = []; constructor(private logger: ILogger) { @@ -216,9 +219,7 @@ export class ExtensionCommandsFeature extends LanguageClientConsumer { this.languageClient.onRequest( NewFileRequestType, - // NOTE: The VS Code API does not support naming a file as it's - // opened, only when it's saved. Hence the argument is not used. - (_filePath) => this.newFile()), + (_content) => this.newFile(_content)), this.languageClient.onRequest( OpenFileRequestType, @@ -267,6 +268,9 @@ export class ExtensionCommandsFeature extends LanguageClientConsumer { for (const handler of this.handlers) { handler.dispose(); } + for (const statusBarMessage of this.statusBarMessages) { + statusBarMessage.dispose(); + } } private addExtensionCommand(command: IExtensionCommandAddedNotificationBody): void { @@ -355,27 +359,35 @@ export class ExtensionCommandsFeature extends LanguageClientConsumer { }; } - private async newFile(): Promise { - const doc = await vscode.workspace.openTextDocument({ content: "" }); + private async newFile(content: string): Promise { + const doc = await vscode.workspace.openTextDocument( + { language: "powershell", content: content }); await vscode.window.showTextDocument(doc); return EditorOperationResponse.Completed; } private async openFile(openFileDetails: IOpenFileDetails): Promise { const filePath = await this.resolveFilePathWithCwd(openFileDetails.filePath); - const doc = await vscode.workspace.openTextDocument(filePath); - await vscode.window.showTextDocument(doc, { preview: openFileDetails.preview }); + try { + const doc = await vscode.workspace.openTextDocument(filePath); + await vscode.window.showTextDocument(doc, { preview: openFileDetails.preview }); + } catch { + void this.logger.writeAndShowWarning(`File to open not found: ${filePath}`); + return EditorOperationResponse.Failed; + } return EditorOperationResponse.Completed; } private async closeFile(filePath: string): Promise { filePath = await this.resolveFilePathWithCwd(filePath); - const doc = vscode.workspace.textDocuments.find((x) => x.fileName === filePath); + const doc = vscode.workspace.textDocuments.find((x) => x.uri.fsPath === filePath); if (doc != undefined && !doc.isClosed) { await vscode.window.showTextDocument(doc); await vscode.commands.executeCommand("workbench.action.closeActiveEditor"); + return EditorOperationResponse.Completed; } - return EditorOperationResponse.Completed; + void this.logger.writeAndShowWarning(`File to close not found or already closed: ${filePath}`); + return EditorOperationResponse.Failed; } /** @@ -388,17 +400,17 @@ export class ExtensionCommandsFeature extends LanguageClientConsumer { if (saveFileDetails.filePath.startsWith("untitled") || saveFileDetails.filePath.startsWith("file")) { currentFileUri = vscode.Uri.parse(saveFileDetails.filePath); } else { - currentFileUri = vscode.Uri.file(saveFileDetails.filePath); + const filePath = await this.resolveFilePathWithCwd(saveFileDetails.filePath); + currentFileUri = vscode.Uri.file(filePath); } - // If the file to save can't be found, just complete the request - const doc = vscode.workspace.textDocuments.find((x) => x.uri === currentFileUri); + const doc = vscode.workspace.textDocuments.find((x) => x.uri.fsPath === currentFileUri.fsPath); if (doc === undefined) { - void this.logger.writeAndShowError(`File to save not found: ${currentFileUri.fsPath}`); - return EditorOperationResponse.Completed; + void this.logger.writeAndShowWarning(`File to save not found: ${currentFileUri.fsPath}`); + return EditorOperationResponse.Failed; } - let newFilePath = saveFileDetails.newPath; + let newFilePath = saveFileDetails.newPath ?? undefined; // Otherwise it's null. if (currentFileUri.scheme === "file") { // If no newFile is given, just save the current file if (newFilePath === undefined) { @@ -416,23 +428,21 @@ export class ExtensionCommandsFeature extends LanguageClientConsumer { } else if (currentFileUri.scheme === "untitled") { // We need a new name to save an untitled file if (newFilePath === undefined) { - void this.logger.writeAndShowError("Cannot save untitled file! Try SaveAs(\"path/to/file.ps1\") instead."); - return EditorOperationResponse.Completed; + void this.logger.writeAndShowWarning("Cannot save untitled file! Try SaveAs(\"path/to/file.ps1\") instead."); + return EditorOperationResponse.Failed; } newFilePath = await this.resolveFilePathWithCwd(newFilePath); } else { // Other URI schemes are not supported const msg = JSON.stringify(saveFileDetails, undefined, 2); - void this.logger.writeAndShowError( + void this.logger.writeAndShowWarning( `<${ExtensionCommandsFeature.name}>: Saving a document with scheme '${currentFileUri.scheme}' ` + `is currently unsupported. Message: '${msg}'`); - return EditorOperationResponse.Completed; + return EditorOperationResponse.Failed; } - await this.saveFileAs(doc, newFilePath); - return EditorOperationResponse.Completed; - + return await this.saveFileAs(doc, newFilePath); } /** @@ -440,7 +450,7 @@ export class ExtensionCommandsFeature extends LanguageClientConsumer { * @param documentUri the URI of the vscode document to save * @param filePath the absolute path to save the document contents to */ - private async saveFileAs(doc: vscode.TextDocument, filePath: string): Promise { + private async saveFileAs(doc: vscode.TextDocument, filePath: string): Promise { // Write the old document's contents to the new document path const newFileUri = vscode.Uri.file(filePath); try { @@ -450,12 +460,13 @@ export class ExtensionCommandsFeature extends LanguageClientConsumer { } catch (err) { void this.logger.writeAndShowWarning(`<${ExtensionCommandsFeature.name}>: ` + `Unable to save file to path '${filePath}': ${err}`); - return; + return EditorOperationResponse.Failed; } // Finally open the new document const newFile = await vscode.workspace.openTextDocument(newFileUri); await vscode.window.showTextDocument(newFile, { preview: true }); + return EditorOperationResponse.Completed; } // Resolve file path against user's CWD setting @@ -474,9 +485,9 @@ export class ExtensionCommandsFeature extends LanguageClientConsumer { asCodePosition(details.selectionRange.start)!, asCodePosition(details.selectionRange.end)!), ]; + return EditorOperationResponse.Completed; } - - return EditorOperationResponse.Completed; + return EditorOperationResponse.Failed; } private showInformationMessage(message: string): EditorOperationResponse { @@ -496,11 +507,12 @@ export class ExtensionCommandsFeature extends LanguageClientConsumer { private setStatusBarMessage(messageDetails: IStatusBarMessageDetails): EditorOperationResponse { if (messageDetails.timeout) { - vscode.window.setStatusBarMessage(messageDetails.message, messageDetails.timeout); + this.statusBarMessages.push( + vscode.window.setStatusBarMessage(messageDetails.message, messageDetails.timeout)); } else { - vscode.window.setStatusBarMessage(messageDetails.message); + this.statusBarMessages.push( + vscode.window.setStatusBarMessage(messageDetails.message)); } - return EditorOperationResponse.Completed; } }