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

vrubezhny · 2019-11-18T10:34:20Z

…urce that doesn't exist #5667

Signed-off-by: Victor Rubezhny vrubezhny@redhat.com

What it does

The fix adds an error message to be shown in case a file to open doesn't exist and button Create that allows a user to create a new file with the same path and name as originally requested.

How to test

A test extension [1] provided by @tolusha is used to test this fix.
The error message appearance can be tested using the extension [1] as is by finding and executing Open Some File command provided by the extension.
In order to test creation of a file, before loading the extension, one needs to edit /extension/out/extension.js in order to specify an URI with an absolute path that is to be inside the test workspace instead of vscode.Uri.file('/bla-bla'), for example, vscode.Uri.parse('file:///home/user/projects/theia/test-workspace/bla0/bla1/bla2/bla-file.ts'), where:

file:///home/user/projects/theia/test-workspace/ path to a workspace to open in Theia,
bla0/bla1/bla2/bla-file.ts relative path part for not existing file to be opened

[1] https://github.com/tolusha/vscode-test-extensions/blob/master/open-some-file-0.0.1.vsix

Review checklist

as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

as a reviewer, I agree to behave in accordance with the review guidelines

packages/monaco/src/browser/monaco-text-model-service.ts

akosyakov · 2019-11-18T13:37:55Z

Why should it be implemented in monaco extension generically, not in vscode.open? Monaco model should not be directly aware of the filesystem. It should be able to support other resource sources.

vrubezhny · 2020-02-21T19:07:40Z

Why should it be implemented in monaco extension generically, not in vscode.open?

I have updated the PR, the implementation is moved to vscode.open.

akosyakov · 2020-02-25T09:17:21Z

@vrubezhny thank you! Could you update a description please with test steps? It looks very empty right now. Defining it is very important for next time when we are going to change this code.

vrubezhny · 2020-02-25T15:12:56Z

@akosyakov I have updated the How to test description. Please verify if it is clear enough.

akosyakov · 2020-02-26T09:25:14Z

packages/plugin-ext-vscode/src/browser/plugin-vscode-commands-contribution.ts

-                    options = {
-                        ...columnOrOptions
-                    };
+                const stat = await this.fileSystem.getFileStat(new TheiaURI(resource).toString());


resource is not necessary file URIs, it can be any URI supported by extension, i.e. gitlens contribute special schemes. It would break all such extensions.

I found that gitlens tries to invoke vscode.open at least in case the uri.scheme === "gitlens" or in case it wasn't able to open a given URI as a Text Document.
So, it looks like it delegates to vscode.open to take care of any URIs it cannot open by itself.
As I cannot check existence of / create document for any uri.scheme I suggest to make the existence check only for the documents with uri.scheme === "file" while delegating it transparently to vscode.open for any other uri.scheme.

tsmaeder · 2020-03-04T13:16:21Z

packages/plugin-ext-vscode/src/browser/plugin-vscode-commands-contribution.ts

-                    options = {
-                        ...columnOrOptions
-                    };
+                if (await this.shouldCreateBeforeOpen(new TheiaURI(resource))) {


I don't think we need to convert the URI here. The only stuff we need is #scheme and #toString(), both of which are availble on the URI type.

Yes, you're right.
I changed the argument of shouldCreateBeforeOpen to be URI instead of TheiaURI in updated PR

tsmaeder · 2020-03-04T13:24:25Z

packages/plugin-ext-vscode/src/browser/plugin-vscode-commands-contribution.ts

+            if (stat.isDirectory) {
+                return stat;
+            }
+            return this.getOrCreateDirectory(uri.parent);


Not sure what you're trying to achieve here.

In case the URI contains a path to a not existing file that should be placed in some not existing folder tree, then we have to create the required folders before creating the file itself. Don't we?

I don't think the logic is right here: if something exists, but it's not a directory we should fail. But anyway I think the node file system will create any required parents of a new directory already, no need to implement this yourself.

I was wrong. No need to create the sub-directories in recursion.

tsmaeder · 2020-03-04T14:08:10Z

packages/plugin-ext-vscode/src/browser/plugin-vscode-commands-contribution.ts

+                            if (res === 'CREATE') {
+                                this.getOrCreateDirectory(uri.parent).then(parent => {
+                                    if (parent) {
+                                        this.fileSystem.createFile(resource.toString()).then(async () => {


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?

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)

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.

…urce that doesn't exist eclipse-theia#5667 Fixes eclipse-theia#5667 Signed-off-by: Victor Rubezhny <vrubezhny@redhat.com>

akosyakov · 2020-03-12T11:53:47Z

packages/plugin-ext-vscode/src/browser/plugin-vscode-commands-contribution.ts

+    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;


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

akosyakov · 2020-03-12T11:54:23Z

packages/plugin-ext-vscode/src/browser/plugin-vscode-commands-contribution.ts

+                    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}`] })


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

tsmaeder · 2022-05-03T13:52:13Z

I'll have a look at this again.

msujew · 2022-11-14T15:40:04Z

@tsmaeder Does it make sense to simply close this PR? We cannot easily merge it anyway due to merge conflicts and I don't expect the original contributor to get back to this. If anyone is interested in contributing this fix, they can use the changes in this PR to create a new PR.

This commit finishes imlementations of eclipse-theia#6568 This commit closes eclipse-theia#5667 Signed-Off-By: FernandoAscencio <fernando.ascencio.cama@ericsson.com>

akosyakov reviewed Nov 18, 2019

View reviewed changes

packages/monaco/src/browser/monaco-text-model-service.ts Outdated Show resolved Hide resolved

akosyakov added monaco issues related to monaco vscode issues related to VSCode compatibility labels Nov 18, 2019

akosyakov reviewed Nov 18, 2019

View reviewed changes

packages/monaco/src/browser/monaco-text-model-service.ts Outdated Show resolved Hide resolved

tsmaeder mentioned this pull request Nov 27, 2019

Languages Sprint 176 eclipse-che/che#15269

Closed

26 tasks

tsmaeder mentioned this pull request Dec 13, 2019

Langugages Sprint 177 eclipse-che/che#15478

Closed

28 tasks

tsmaeder mentioned this pull request Jan 8, 2020

Languages Sprint 178 eclipse-che/che#15629

Closed

35 tasks

tsmaeder mentioned this pull request Jan 23, 2020

Languages Sprint 179 eclipse-che/che#15796

Closed

36 tasks

tsmaeder mentioned this pull request Feb 18, 2020

Languages Sprint 180 eclipse-che/che#16069

Closed

34 tasks

vrubezhny force-pushed the et5667 branch from cf0b91e to fd3a595 Compare February 21, 2020 19:04

akosyakov reviewed Feb 26, 2020

View reviewed changes

vrubezhny force-pushed the et5667 branch 2 times, most recently from a730644 to c29970a Compare March 3, 2020 16:26

tsmaeder reviewed Mar 4, 2020

View reviewed changes

vrubezhny force-pushed the et5667 branch from c29970a to c41b777 Compare March 4, 2020 14:01

tsmaeder reviewed Mar 4, 2020

View reviewed changes

vrubezhny force-pushed the et5667 branch from c41b777 to 40af527 Compare March 6, 2020 13:21

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

c430f74

…urce that doesn't exist eclipse-theia#5667 Fixes eclipse-theia#5667 Signed-off-by: Victor Rubezhny <vrubezhny@redhat.com>

vrubezhny force-pushed the et5667 branch from 40af527 to c430f74 Compare March 9, 2020 15:46

akosyakov reviewed Mar 12, 2020

View reviewed changes

JonasHelming added the decision_needed issues that require decisions on ways forward (ex: if they should be updated or closed) label Mar 8, 2022

JonasHelming assigned tsmaeder Mar 9, 2022

tsmaeder removed the decision_needed issues that require decisions on ways forward (ex: if they should be updated or closed) label May 3, 2022

msujew closed this Dec 13, 2022

FernandoAscencio mentioned this pull request Mar 10, 2023

vscode: provide error feedback for the vscode.open command #12284

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

vrubezhny commented Nov 18, 2019 •

edited

Loading

akosyakov commented Nov 18, 2019 •

edited

Loading

vrubezhny commented Feb 21, 2020

akosyakov commented Feb 25, 2020 •

edited

Loading

vrubezhny commented Feb 25, 2020

akosyakov Feb 26, 2020

vrubezhny Feb 28, 2020 •

edited

Loading

tsmaeder Mar 4, 2020

vrubezhny Mar 4, 2020

tsmaeder Mar 4, 2020

vrubezhny Mar 4, 2020

tsmaeder Mar 4, 2020

vrubezhny Mar 4, 2020

tsmaeder Mar 4, 2020

vrubezhny Mar 4, 2020 •

edited

Loading

vrubezhny Mar 6, 2020

akosyakov Mar 12, 2020

akosyakov Mar 12, 2020

tsmaeder commented May 3, 2022

msujew commented Nov 14, 2022

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

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

Conversation

vrubezhny commented Nov 18, 2019 • edited Loading

What it does

How to test

Review checklist

Reminder for reviewers

akosyakov commented Nov 18, 2019 • edited Loading

vrubezhny commented Feb 21, 2020

akosyakov commented Feb 25, 2020 • edited Loading

vrubezhny commented Feb 25, 2020

Choose a reason for hiding this comment

vrubezhny Feb 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrubezhny Mar 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsmaeder commented May 3, 2022

msujew commented Nov 14, 2022

vrubezhny commented Nov 18, 2019 •

edited

Loading

akosyakov commented Nov 18, 2019 •

edited

Loading

akosyakov commented Feb 25, 2020 •

edited

Loading

vrubezhny Feb 28, 2020 •

edited

Loading

vrubezhny Mar 4, 2020 •

edited

Loading