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.

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

Handle Rename and OpenDocument responses from Omnisharp #1854

Merged
merged 11 commits into from
Jan 24, 2018
72 changes: 63 additions & 9 deletions src/features/codeActionProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import { toRange2 } from '../omnisharp/typeConvertion';
import * as serverUtils from '../omnisharp/utils';
import { Options } from '../omnisharp/options';
import TelemetryReporter from 'vscode-extension-telemetry';
import { FileModificationType } from '../omnisharp/protocol';
import { Uri } from 'vscode';

export default class CodeActionProvider extends AbstractProvider implements vscode.CodeActionProvider {

Expand Down Expand Up @@ -90,7 +92,8 @@ export default class CodeActionProvider extends AbstractProvider implements vsco
Column: column,
Selection: selection,
Identifier: codeAction.Identifier,
WantsTextChanges: true
WantsTextChanges: true,
WantsAllCodeActionOperations: true
};

return {
Expand All @@ -112,20 +115,71 @@ export default class CodeActionProvider extends AbstractProvider implements vsco

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) {
let uri = vscode.Uri.file(change.FileName);
let edits: vscode.TextEdit[] = [];
for (let textChange of change.Changes) {
edits.push(vscode.TextEdit.replace(toRange2(textChange), textChange.NewText));
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);
}

edit.set(uri, edits);
if (change.ModificationType == FileModificationType.Modified)
{
let uri = vscode.Uri.file(change.FileName);
Copy link
Member

Choose a reason for hiding this comment

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

Based on your comment when FileModificationType.Renamed is true, should we check to see if this file is included in renamedFiles? And if so, does that change the order in which we loop through the changes?

Copy link
Author

Choose a reason for hiding this comment

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

Right. We should probably collect all the renames first, so we don't try to edit files that got renamed.

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);
}
}

return vscode.workspace.applyEdit(edit);
}
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
Copy link
Member

Choose a reason for hiding this comment

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

Do we have a bug on VS Code about this?

Copy link
Author

Choose a reason for hiding this comment

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

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");
});
}

}, (error) => {
return fileToOpen != null
? next.then(_ =>
{
return vscode.commands.executeCommand("vscode.open", fileToOpen);
})
: next;
}
}, (error) => {
return Promise.reject(`Problem invoking 'RunCodeAction' on OmniSharp server: ${error}`);
});
}
Expand Down
10 changes: 9 additions & 1 deletion src/omnisharp/protocol.ts
Original file line number Diff line number Diff line change
Expand Up @@ -373,6 +373,14 @@ export interface ModifiedFileResponse {
FileName: string;
Buffer: string;
Changes: TextChange[];
ModificationType: FileModificationType;
}

export enum FileModificationType
{
Modified,
Opened,
Renamed,
}

export interface RenameResponse {
Expand Down Expand Up @@ -485,13 +493,13 @@ export namespace V2 {
Identifier: string;
Selection?: Range;
WantsTextChanges: boolean;
WantsAllCodeActionOperations: boolean;
}

export interface RunCodeActionResponse {
Changes: ModifiedFileResponse[];
}


export interface MSBuildProjectDiagnostics {
FileName: string;
Warnings: MSBuildDiagnosticsMessage[];
Expand Down
38 changes: 38 additions & 0 deletions test/integrationTests/codeActionRename.integration.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
/*---------------------------------------------------------------------------------------------
* Copyright (c) Microsoft Corporation. All rights reserved.
* Licensed under the MIT License. See License.txt in the project root for license information.
*--------------------------------------------------------------------------------------------*/

import * as fs from 'async-file';
import * as vscode from 'vscode';

import poll from './poll';
import { should, expect } from 'chai';
import testAssetWorkspace from './testAssets/testAssetWorkspace';

const chai = require('chai');
chai.use(require('chai-arrays'));
chai.use(require('chai-fs'));

suite(`Code Action Rename ${testAssetWorkspace.description}`, function() {
suiteSetup(async function() {
should();

let csharpExtension = vscode.extensions.getExtension("ms-vscode.csharp");
if (!csharpExtension.isActive) {
await csharpExtension.activate();
}

await csharpExtension.exports.initializationFinished;

});

test("Code actions can remame and open files", async () => {
let fileUri = await testAssetWorkspace.projects[0].addFileWithContents("test.cs", "class C {}");

Choose a reason for hiding this comment

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

running this test more than once will give us false positives. The first execution will put C.cs in the workspace, the second will simply find it.

My first thought was to add a line that deletes both test.cs and C.cs... but this feels like we will have lots of this sort of cleanup code running around. What if we added a method on e.g. testAssetWorkspace that performed a git clean -xfd? This would allow tests to easily put the asset back into a known state [we could eliminate the custom methods for deleting bin/obj, as well as for .vscode]. Thoughts?

Copy link
Author

Choose a reason for hiding this comment

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

I somewhat dislike having the projects manipulated by tests under the repo because they show up in my git status, so it would be nice if we could do something to get them out of there.

Relatedly, Omnisharp-roslyn has a pretty nice system where you can request a real on-disk project from a test helper, and one is created for you under a random folder under %temp%. If we implemented that, we wouldn't have to worry about cleaning up test projects after we're done testing because a test would never see the same instance of a project.

O#r's build knows about the test projects so it can build/restore them as part of the solution build. That means deploying a project for test simply copies all the files into a temp directory.

I think that might be a good solution for these tests. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Agreed 100%. This is why the tests in omnisharp-roslyn that operate on test projects go to an effort to copy test projects to a temporary folder and delete them when the test is finished.

Choose a reason for hiding this comment

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

CLI does the same, @DustinCampbell. However, in this scenario we have a non-trivial startup cost for opening a new folder in VS Code. We also don't have a known model for plugging in an outer loop test orchestrator. Such a thing would need to enumerate all the tests, understand which test asset is needed by which test, make a copy of the asset & launch VS Code in that copy's folder, and then get vscode to execute just that test. I agree that this would be cool to have [if we can make it fast] but it feels like a considerable amount of work...

await vscode.commands.executeCommand("vscode.open", fileUri);
let c = await vscode.commands.executeCommand("vscode.executeCodeActionProvider", fileUri, new vscode.Range(0, 7, 0, 7)) as {command: string, arguments: string[]}[];
expect(c.length).to.equal(2);
await vscode.commands.executeCommand(c[1].command, ...c[1].arguments)
expect(vscode.window.activeTextEditor.document.fileName).contains("C.cs");
});
});