Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Don't apply edits on server #4133

Merged
merged 2 commits into from
Oct 22, 2020
Merged

Don't apply edits on server #4133

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f0ea762
Don't apply edits on server
333fred Oct 21, 2020
1ca331b
Update changelog.
333fred Oct 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

## 1.23.5 (Not yet released)
* Set meaning of UseGlobalMono "auto" to "never" since Mono 6.12.0 still ships with MSBuild 16.7 (PR: [#4130](https://github.com/OmniSharp/omnisharp-vscode/pull/4130))
* Ensure that the rename identifier and run code action providers do not apply changes twice PR: [4133](https://github.com/OmniSharp/omnisharp-vscode/pull/4133)

## 1.23.4 (October, 19, 2020)
* Use incremental changes to update language server (PR: [#4088](https://github.com/OmniSharp/omnisharp-vscode/pull/4088))
Expand Down
87 changes: 9 additions & 78 deletions src/features/codeActionProvider.ts
View file
Open in desktop
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) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The after change replaced this logic.

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)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This if is unchanged.

{
// 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))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic is removed.

{
// 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) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic is unchanged, other than using const where appropriate and correcting for the necessary cast on change to the correct type.

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))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic is removed and unneeded, as we now do the rename ourselves.

{
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
Open in desktop
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
Open in desktop
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
Open in desktop
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) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the actual after change here.

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
Open in desktop
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