Skip to content

Commit

Permalink
Don't apply edits on server
Browse files Browse the repository at this point in the history 
The code action provider and rename provider were expecting the server to apply the file changes, which then messes up incremental buffer changes. This sends for false for these parameters, and refactors the handling of code action responses to do the renames locally. As part of that, I also refactored the protocol to accurately reflect O#'s hierarchy, and share the edit building between the fixAllProvider and the runCodeAction provider. Fixes dotnet#4128.
  • Loading branch information
@333fred
333fred committed Oct 21, 2020
1 parent f97ba36 commit f0ea762
Show file tree
Hide file tree
Showing 5 changed files with 94 additions and 151 deletions.
87 changes: 9 additions & 78 deletions src/features/codeActionProvider.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,11 @@ import * as vscode from 'vscode';
import { OmniSharpServer } from '../omnisharp/server';
import AbstractProvider from './abstractProvider';
import * as protocol from '../omnisharp/protocol';
import { toRange2 } from '../omnisharp/typeConversion';
import * as serverUtils from '../omnisharp/utils';
import { FileModificationType } from '../omnisharp/protocol';
import { Uri } from 'vscode';
import CompositeDisposable from '../CompositeDisposable';
import OptionProvider from '../observers/OptionProvider';
import { LanguageMiddlewareFeature } from '../omnisharp/LanguageMiddlewareFeature';
import { buildEditForResponse } from '../omnisharp/fileOperationsResponseEditBuilder';

export default class CodeActionProvider extends AbstractProvider implements vscode.CodeActionProvider {

Expand Down Expand Up @@ -84,7 +82,8 @@ export default class CodeActionProvider extends AbstractProvider implements vsco
Selection: selection,
Identifier: codeAction.Identifier,
WantsTextChanges: true,
WantsAllCodeActionOperations: true
WantsAllCodeActionOperations: true,
ApplyTextChanges: false
};

return {
Expand All @@ -101,80 +100,12 @@ export default class CodeActionProvider extends AbstractProvider implements vsco

private async _runCodeAction(req: protocol.V2.RunCodeActionRequest, token: vscode.CancellationToken): Promise<boolean | string | {}> {

return serverUtils.runCodeAction(this._server, req).then(async response => {
if (response && Array.isArray(response.Changes)) {

let edit = new vscode.WorkspaceEdit();

let fileToOpen: Uri = null;
let renamedFiles: Uri[] = [];

for (let change of response.Changes) {
if (change.ModificationType == FileModificationType.Renamed)
{
// The file was renamed. Omnisharp has already persisted
// the right changes to disk. We don't need to try to
// apply text changes (and will skip this file if we see an edit)
renamedFiles.push(Uri.file(change.FileName));
}
}

for (let change of response.Changes) {
if (change.ModificationType == FileModificationType.Opened)
{
// The CodeAction requested that we open a file.
// Record that file name and keep processing CodeActions.
// If a CodeAction requests that we open multiple files
// we only open the last one (what would it mean to open multiple files?)
fileToOpen = vscode.Uri.file(change.FileName);
}

if (change.ModificationType == FileModificationType.Modified)
{
let uri = vscode.Uri.file(change.FileName);
if (renamedFiles.some(r => r == uri))
{
// This file got renamed. OmniSharp has already
// persisted the new file with any applicable changes.
continue;
}

let edits: vscode.TextEdit[] = [];
for (let textChange of change.Changes) {
edits.push(vscode.TextEdit.replace(toRange2(textChange), textChange.NewText));
}

edit.set(uri, edits);
}
}

// Allow language middlewares to re-map its edits if necessary.
edit = await this._languageMiddlewareFeature.remap("remapWorkspaceEdit", edit, token);

let applyEditPromise = vscode.workspace.applyEdit(edit);

// Unfortunately, the textEditor.Close() API has been deprecated
// and replaced with a command that can only close the active editor.
// If files were renamed that weren't the active editor, their tabs will
// be left open and marked as "deleted" by VS Code
let next = applyEditPromise;
if (renamedFiles.some(r => r.fsPath == vscode.window.activeTextEditor.document.uri.fsPath))
{
next = applyEditPromise.then(_ =>
{
return vscode.commands.executeCommand("workbench.action.closeActiveEditor");
});
}

return fileToOpen != null
? next.then(_ =>
{
return vscode.commands.executeCommand("vscode.open", fileToOpen);
})
: next;
}
}, async (error) => {
return serverUtils.runCodeAction(this._server, req).then(response => {
if (response) {
return buildEditForResponse(response.Changes, this._languageMiddlewareFeature, token);
}
}, async (error) => {
return Promise.reject(`Problem invoking 'RunCodeAction' on OmniSharp server: ${error}`);
});
}
}
}
74 changes: 5 additions & 69 deletions src/features/fixAllProvider.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,12 @@ import * as vscode from 'vscode';
import * as serverUtils from '../omnisharp/utils';
import * as protocol from '../omnisharp/protocol';
import { OmniSharpServer } from '../omnisharp/server';
import { FixAllScope, FixAllItem, FileModificationType } from '../omnisharp/protocol';
import { Uri } from 'vscode';
import { FixAllScope, FixAllItem } from '../omnisharp/protocol';
import CompositeDisposable from '../CompositeDisposable';
import AbstractProvider from './abstractProvider';
import { toRange2 } from '../omnisharp/typeConversion';
import { LanguageMiddlewareFeature } from '../omnisharp/LanguageMiddlewareFeature';
import { buildEditForResponse } from '../omnisharp/fileOperationsResponseEditBuilder';
import { CancellationToken } from 'vscode-languageserver-protocol';

export class FixAllProvider extends AbstractProvider implements vscode.CodeActionProvider {
public constructor(private server: OmniSharpServer, languageMiddlewareFeature: LanguageMiddlewareFeature) {
Expand Down Expand Up @@ -80,72 +80,8 @@ export class FixAllProvider extends AbstractProvider implements vscode.CodeActio
ApplyChanges: false
});

if (response && Array.isArray(response.Changes)) {
let edit = new vscode.WorkspaceEdit();

let fileToOpen: Uri = null;
let renamedFiles: Uri[] = [];

for (let change of response.Changes) {
if (change.ModificationType == FileModificationType.Renamed)
{
// The file was renamed. Omnisharp has already persisted
// the right changes to disk. We don't need to try to
// apply text changes (and will skip this file if we see an edit)
renamedFiles.push(Uri.file(change.FileName));
}
}

for (let change of response.Changes) {
if (change.ModificationType == FileModificationType.Opened)
{
// The CodeAction requested that we open a file.
// Record that file name and keep processing CodeActions.
// If a CodeAction requests that we open multiple files
// we only open the last one (what would it mean to open multiple files?)
fileToOpen = vscode.Uri.file(change.FileName);
}

if (change.ModificationType == FileModificationType.Modified)
{
let uri = vscode.Uri.file(change.FileName);
if (renamedFiles.some(r => r == uri))
{
// This file got renamed. Omnisharp has already
// persisted the new file with any applicable changes.
continue;
}

let edits: vscode.TextEdit[] = [];
for (let textChange of change.Changes) {
edits.push(vscode.TextEdit.replace(toRange2(textChange), textChange.NewText));
}

edit.set(uri, edits);
}
}

let applyEditPromise = vscode.workspace.applyEdit(edit);

// Unfortunately, the textEditor.Close() API has been deprecated
// and replaced with a command that can only close the active editor.
// If files were renamed that weren't the active editor, their tabs will
// be left open and marked as "deleted" by VS Code
let next = applyEditPromise;
if (renamedFiles.some(r => r.fsPath == vscode.window.activeTextEditor.document.uri.fsPath))
{
next = applyEditPromise.then(_ =>
{
return vscode.commands.executeCommand("workbench.action.closeActiveEditor");
});
}

return fileToOpen != null
? next.then(_ =>
{
return vscode.commands.executeCommand("vscode.open", fileToOpen);
})
: next;
if (response) {
return buildEditForResponse(response.Changes, this._languageMiddlewareFeature, CancellationToken.None);
}
}
}
1 change: 1 addition & 0 deletions src/features/renameProvider.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export default class OmnisharpRenameProvider extends AbstractSupport implements
let req = createRequest<protocol.RenameRequest>(document, position);
req.WantsTextChanges = true;
req.RenameTo = newName;
req.ApplyTextChanges = false;

try {
let response = await serverUtils.rename(this._server, req, token);
Expand Down
63 changes: 63 additions & 0 deletions src/omnisharp/fileOperationsResponseEditBuilder.ts
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
/*---------------------------------------------------------------------------------------------
* Copyright (c) Microsoft Corporation. All rights reserved.
* Licensed under the MIT License. See License.txt in the project root for license information.
*--------------------------------------------------------------------------------------------*/

import * as vscode from 'vscode';
import { Uri } from 'vscode';
import { LanguageMiddlewareFeature } from './LanguageMiddlewareFeature';
import { FileModificationType, FileOperationResponse, ModifiedFileResponse, RenamedFileResponse } from "./protocol";
import { toRange2 } from './typeConversion';

export async function buildEditForResponse(changes: FileOperationResponse[], languageMiddlewareFeature: LanguageMiddlewareFeature, token: vscode.CancellationToken): Promise<boolean> {
let edit = new vscode.WorkspaceEdit();

let fileToOpen: Uri = null;

if (!changes || !Array.isArray(changes) || !changes.length) {
return true;
}

for (const change of changes) {
if (change.ModificationType == FileModificationType.Opened) {
// The CodeAction requested that we open a file.
// Record that file name and keep processing CodeActions.
// If a CodeAction requests that we open multiple files
// we only open the last one (what would it mean to open multiple files?)
fileToOpen = vscode.Uri.file(change.FileName);
}

if (change.ModificationType == FileModificationType.Modified) {
const modifiedChange = <ModifiedFileResponse>change;
const uri = vscode.Uri.file(modifiedChange.FileName);
let edits: vscode.TextEdit[] = [];
for (let textChange of modifiedChange.Changes) {
edits.push(vscode.TextEdit.replace(toRange2(textChange), textChange.NewText));
}

edit.set(uri, edits);
}
}

for (const change of changes) {
if (change.ModificationType == FileModificationType.Renamed) {
const renamedChange = <RenamedFileResponse>change;
edit.renameFile(vscode.Uri.file(renamedChange.FileName), vscode.Uri.file(renamedChange.NewFileName));
}
}

// Allow language middlewares to re-map its edits if necessary.
edit = await languageMiddlewareFeature.remap("remapWorkspaceEdit", edit, token);

const applyEditPromise = vscode.workspace.applyEdit(edit);

// Unfortunately, the textEditor.Close() API has been deprecated
// and replaced with a command that can only close the active editor.
// If files were renamed that weren't the active editor, their tabs will
// be left open and marked as "deleted" by VS Code
return fileToOpen != null
? applyEditPromise.then(_ => {
return vscode.commands.executeCommand("vscode.open", fileToOpen);
})
: applyEditPromise;
}
20 changes: 16 additions & 4 deletions src/omnisharp/protocol.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ export interface GetCodeActionsResponse {

export interface RunFixAllActionResponse {
Text: string;
Changes: ModifiedFileResponse[];
Changes: FileOperationResponse[];
}

export interface FixAllItem {
Expand Down Expand Up @@ -371,13 +371,24 @@ export interface DotNetFramework {
export interface RenameRequest extends Request {
RenameTo: string;
WantsTextChanges?: boolean;
ApplyTextChanges: boolean;
}

export interface ModifiedFileResponse {
export interface FileOperationResponse {
FileName: string;
ModificationType: FileModificationType;
}

export interface ModifiedFileResponse extends FileOperationResponse {
Buffer: string;
Changes: TextChange[];
ModificationType: FileModificationType;
}

export interface RenamedFileResponse extends FileOperationResponse {
NewFileName: string;
}

export interface OpenFileResponse extends FileOperationResponse {
}

export enum FileModificationType {
Expand Down Expand Up @@ -596,10 +607,11 @@ export namespace V2 {
Selection?: Range;
WantsTextChanges: boolean;
WantsAllCodeActionOperations: boolean;
ApplyTextChanges: boolean;
}

export interface RunCodeActionResponse {
Changes: ModifiedFileResponse[];
Changes: FileOperationResponse[];
}

export interface MSBuildProjectDiagnostics {
Expand Down

0 comments on commit f0ea762

Please sign in to comment.