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

[vscode] No error message if vscode.open command is invoked with reso… #6568

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ import { MonacoEditor } from '@theia/monaco/lib/browser/monaco-editor';
import { inject, injectable } from 'inversify';
import { Position } from '@theia/plugin-ext/lib/common/plugin-api-rpc';
import URI from 'vscode-uri';
import { FileSystem, FileStat } from '@theia/filesystem/lib/common';
import { MessageClient, MessageType } from '@theia/core/lib/common';

export namespace VscodeCommands {
export const OPEN: Command = {
Expand Down Expand Up @@ -89,6 +91,10 @@ export class PluginVscodeCommandsContribution implements CommandContribution {
protected readonly quickOpen: PrefixQuickOpenService;
@inject(WorkspaceService)
protected readonly workspaceService: WorkspaceService;
@inject(FileSystem)
protected readonly fileSystem: FileSystem;
@inject(MessageClient)
protected readonly messages: MessageClient;

registerCommands(commands: CommandRegistry): void {
commands.registerCommand(VscodeCommands.OPEN, {
Expand All @@ -97,22 +103,39 @@ export class PluginVscodeCommandsContribution implements CommandContribution {
if (!resource) {
throw new Error(`${VscodeCommands.OPEN.id} command requires at least URI argument.`);
}

if (!URI.isUri(resource)) {
throw new Error(`Invalid argument for ${VscodeCommands.OPEN.id} command with URI argument. Found ${resource}`);
}

let options: TextDocumentShowOptions | undefined;
if (typeof columnOrOptions === 'number') {
options = {
viewColumn: columnOrOptions
};
} else if (columnOrOptions) {
options = {
...columnOrOptions
};
if (await this.shouldCreateBeforeOpen(resource)) {
const uri = new TheiaURI(resource);
const msg = `Unable to open '${uri.displayName}': Unable to read file '${uri.path}'`;
const createButton = 'Create File';
this.messages.showMessage({ type: MessageType.Error, text: msg, actions: [`${createButton}`] })
Copy link
Member

Choose a reason for hiding this comment

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

please use async/await not promise chaining, it is easy to read

.then(res => {
if (res === createButton) {
this.getOrCreateDirectory(uri.parent).then(parent => {
if (parent) {
this.fileSystem.createFile(resource.toString()).then(async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

What if you try to create this in a place where you don't have permission? Do you know what VS code does in that case?

Copy link
Contributor Author

@vrubezhny vrubezhny Mar 4, 2020

Choose a reason for hiding this comment

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

For me it prints an error to console:

root ERROR Request createFolder failed with error: EACCES: permission denied, mkdir '/home/jeremy1' Error: EACCES: permission denied, mkdir '/home/NotExistingUser'

No error messages in Theia IDE.

And it's able to create a file in a folder that is placed somewhere outside of the workspace (if permissions allow creating a folder/file)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In case te required file cannot be created vscode shows an error message like: Unable to write file '/home/wrongUser/projects/project/aFile.ts'. So, I've added a similar error processing.

await this.openEditor(resource, columnOrOptions);
}).catch(error => {
this.messages.showMessage({
type: MessageType.Error,
text: `Unable to write file '${uri.path}'`
});
});
}
}).catch(error => {
this.messages.showMessage({
type: MessageType.Error,
text: `Unable to write file '${uri.path}'`
});
});
}
});
} else {
await this.openEditor(resource, columnOrOptions);
}
const editorOptions = DocumentsMainImpl.toEditorOpenerOptions(this.shell, options);
await open(this.openerService, new TheiaURI(resource), editorOptions);
}
});

Expand Down Expand Up @@ -445,4 +468,39 @@ export class PluginVscodeCommandsContribution implements CommandContribution {
}
);
}

protected async shouldCreateBeforeOpen(uri: URI): Promise<boolean> {
// Do not try to create a document which scheme is not a file
if (uri.scheme !== 'file') {
return false;
Copy link
Member

Choose a reason for hiding this comment

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

I think we should do #7127 after that we will have the proper file system API which respect all kind of schemes.

}
const stat = await this.fileSystem.getFileStat(uri.toString());
if (stat && !stat.isDirectory) {
return false;
}
return true;
}

protected async openEditor(uri: URI, columnOrOptions?: ViewColumn | TextDocumentShowOptions): Promise<object | undefined> {
let options: TextDocumentShowOptions | undefined;
if (typeof columnOrOptions === 'number') {
options = {
viewColumn: columnOrOptions
};
} else if (columnOrOptions) {
options = {
...columnOrOptions
};
}
const editorOptions = DocumentsMainImpl.toEditorOpenerOptions(this.shell, options);
return open(this.openerService, new TheiaURI(uri), editorOptions);
}

protected async getOrCreateDirectory(uri: TheiaURI): Promise<FileStat | undefined> {
const stat = await this.fileSystem.getFileStat(uri.toString());
if (stat && stat.isDirectory) {
return stat;
}
return this.fileSystem.createFolder(uri.toString());
}
}