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

WorkspaceEdit: edit followed by deleteResource does not work #42640

Closed
aeschli opened this issue Jan 31, 2018 · 17 comments
Closed

WorkspaceEdit: edit followed by deleteResource does not work #42640

aeschli opened this issue Jan 31, 2018 · 17 comments
Assignees
@bpasero
Labels
debt Code quality issues plan-item VS Code - planned item for upcoming verification-needed Verification of issue is requested verified Verification succeeded workspace-edit
Milestone
June 2018

Comments

@aeschli
Copy link
Contributor

aeschli commented Jan 31, 2018

Testing #42334

Run the extension below.

  • inserts text to the current document
  • deletes the resource

The deleteResource seems to be ignore

'use strict';
import * as vscode from 'vscode';
import * as path from 'path';

export function activate(context: vscode.ExtensionContext) {
    let disposable = vscode.commands.registerCommand('extension.sayHello', () => {
        let editor = vscode.window.activeTextEditor;
        if (editor) {
            let doc = editor.document;
            let docUri = doc.uri;

            let we = new vscode.WorkspaceEdit();
            we.insert(docUri, new vscode.Position(0, 0), 'Hello');
            we.deleteResource(docUri);
            vscode.workspace.applyEdit(we);
        }
    });

    context.subscriptions.push(disposable);
}
@jrieken
Copy link
Member

jrieken commented Jan 31, 2018

@aeschli It should show like this: '(deleted from disk)'.

screen shot 2018-01-31 at 16 34 07

@bpasero Is there a way to prevent this? Don't we know that is was us, e.g. our file service, deleting the file so that we don't keep the editor around?

@bpasero
Copy link
Member

bpasero commented Jan 31, 2018

@jrieken it will close by default, unless workbench.editor.closeOnFileDelete is configured, which you maybe did?

@jrieken
Copy link
Member

jrieken commented Jan 31, 2018

which you maybe did?

Nope... It also doesn't explain @aeschli's observation which is having both editors visible...

@bpasero
Copy link
Member

bpasero commented Jan 31, 2018

@jrieken ah I see, the file is dirty and gets deleted from disk. we never ever close an editor that is dirty, no matter what happens.

@jrieken
Copy link
Member

jrieken commented Jan 31, 2018 •
edited

So, should we save it then before deleting?

@jrieken jrieken added this to the January 2018 milestone Jan 31, 2018
@jrieken
Copy link
Member

jrieken commented Jan 31, 2018 •
edited

we never ever close an editor that is dirty, no matter what happens.

Wait. I have an editor with unsaved changes and then I delete its form the explorer and the editor doesn't stick around. @bpasero How is that controlled? I always thought that's why we have file events and file service events?

@bpasero
Copy link
Member

bpasero commented Jan 31, 2018 •
edited

@jrieken yeah, the explorer reverts the file before then deleting it. I think it would be fair if you do the same in this case.

@jrieken
Copy link
Member

jrieken commented Jan 31, 2018

Hm... Uses a service isn't available in the common-layer (in contrast to the file service)... Would a simple save work?

@jrieken
Copy link
Member

jrieken commented Jan 31, 2018

Would a simple save work?

I believe no

@bpasero
Copy link
Member

bpasero commented Jan 31, 2018

@jrieken you mean ITextFileService? that one should be accessible from common, at least the interface lives in vs/workbench/services/textfile/common/textfiles.ts

@jrieken
Copy link
Member

jrieken commented Feb 1, 2018

Sorry, imprecise wording. By 'common-layer' I have meant platform or accessible from the editor

@bpasero
Copy link
Member

bpasero commented Feb 1, 2018 •
edited

@jrieken it does not seem like ITextFileService could not live in platform, there are not a lot of workbench dependencies (just a few interfaces that could also be moved). We would have to provide some kind of bogus default implementation for the standalone editor though.

@jrieken
Copy link
Member

jrieken commented Feb 1, 2018

We would have to provide some kind of bogus default implementation for the standalone editor though.

Yeah, I'd opt for the other direction and that we move the workspace edit up, so rename and code actions would be gone from the standalone editor

@jrieken jrieken modified the milestones: January 2018, February 2018 Feb 1, 2018
@jrieken jrieken modified the milestones: February 2018, March 2018 Feb 27, 2018
@jrieken jrieken removed this from the March 2018 milestone Mar 8, 2018
@jrieken
Copy link
Member

jrieken commented Apr 30, 2018 •
edited

@bpasero I have moved the rename/bulk-edit logic into a separate IBulkEditorService. The real implementation lives in the workbench layer and can use whatever it takes to implement rename/delete/create. Today it uses the IFileService for that which seems to be insufficient. I'll leave it up to you to correct this

@jrieken jrieken assigned bpasero and unassigned jrieken Apr 30, 2018
@bpasero bpasero added the debt Code quality issues label Apr 30, 2018
@bpasero bpasero mentioned this issue Jun 12, 2018
Closed
@bpasero bpasero added this to the June 2018 milestone Jun 14, 2018
@bpasero bpasero added the plan-item VS Code - planned item for upcoming label Jun 14, 2018
@bpasero
Copy link
Member

bpasero commented Jun 15, 2018 •
edited

@jrieken I adopted new methods ITextFileService#delete and ITextFileService#move in the bulkEditService. These new methods will:

  • revert a dirty file if it gets deleted
  • preserve the contents of a dirty file if it gets moved

There are 3 outstanding questions:

  • currently there is no prompt happening when the file is dirty and about to be deleted, would you expect one?
  • currently I do not expect the URIs in these methods to be folders, just files because the text file service is typically just talking about files. would the bulk service ever pass in folder resources?
  • I left the createFile call as it was going through the IFileService because I do not assume this one was causing any issues?

Edit
I guess we want to support folders at least for the scenario in #43768.

@kieferrm kieferrm mentioned this issue Jun 15, 2018
Closed
52 tasks
@bpasero bpasero reopened this Jun 16, 2018
@jrieken
Copy link
Member

jrieken commented Jun 18, 2018 •
edited

Yes, folder support is needed. And, 'no' I don't expect a prompt to happen

@bpasero
Copy link
Member

bpasero commented Jun 18, 2018

@jrieken added folder support. The explorer is now using the ITextFileService#move() method for both renaming a file/folder or when using drag and drop. This opens the door for participation in these operations from extensions. I checked that nobody else is using this functionality so far besides the explorer.

@bpasero bpasero added the verification-needed Verification of issue is requested label Jun 19, 2018
@jrieken jrieken added the verified Verification succeeded label Jun 28, 2018
@vscodebot vscodebot bot locked and limited conversation to collaborators Aug 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@bpasero bpasero

Labels
debt Code quality issues plan-item VS Code - planned item for upcoming verification-needed Verification of issue is requested verified Verification succeeded workspace-edit
Projects
None yet
Milestone
June 2018
Development

No branches or pull requests
3 participants
@bpasero @jrieken @aeschli