Fix 'window.showTextDocument' to open resources with 'untitled' schema #6803

ShimonBenYair · 2019-12-31T08:59:53Z

Signed-off-by: Shimon Ben Yair shimon.ben.yair@sap.com

What it does

Currently "window.showTextDocument" Does Not open Resources With "untitled" Scheme.
i.e. Using the following code :
const fileUri = Uri.parse(untitled:${uri}).with({ scheme }); await window.showTextDocument(fileUri);
does not open a file in Theia an raises an error.
With changes in this pull request, when calling the above API a file is created and the user can modify and save the file with no problems.
This pull request Fixes #6565 bug.

How to test

Add this SQLTools extension (current version 0.21.5) under your plugins folder in Theia.
sqltools-0.21.5.zip
Create in workspace/theia a new file called "tom.db".
3.Add to User preferences the following values:
{
"sqltools.useNodeRuntime": true,
"sqltools.dependencyManager": {
"autoAccept": true
},
"sqltools.connections": [
{
"name": "sqlite",
"dialect": "SQLite",
"database": "workspace/theia/tom.db"
}
]
}
Click on SQLTools extension.
In "sqlite" linem, click on connect:

When clicking on the "connect" button , code similar to the following is executed :
const uri = vscode.Uri.file(os.homedir + '/newnew.txt'); const fileUri = vscode.Uri.parse(untitled:${uri}).with({ scheme: 'untitled' }); vscode.window.showTextDocument(fileUri);

Verify a file called "sqlite Session.sql" was created and opened.
Modify this file (write something and save) and verify there are no exceptions and errors.

I have found some more issues in this code but as their fix is not required for this feature, it is out of the scope of this pull request.

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

ShimonBenYair · 2019-12-31T09:31:22Z

@akosyakov
please review

akosyakov · 2020-01-01T18:27:31Z

With changes in this pull request, when calling the above API a file is created and the user can modify and save the file with no problems.

I thought that untitled resources should not have physical representation till a user explicitly saved them? If a user just close the editor without saving then there should not be a trace of the file? cc @amiramw @TomHermanSAP @offer8

It would be nice if someone setups simple VS Code extension to manipulate resource with untitled schema. And then run it against VS Code and Theia to make sure that resources are treated the same. Running the sql extension is overkill.

akosyakov · 2020-01-01T18:28:45Z

packages/plugin-ext/src/plugin/documents.ts

@@ -221,6 +221,7 @@ export class DocumentsExtImpl implements DocumentsExt {
            // return opened document
            return document;
        } catch (error) {
+            console.log(error);


left overs? Should not it be handled by clients since we throw it?

done, fixed

packages/plugin-ext/src/main/browser/editor/untitled-resource.ts

akosyakov · 2020-01-01T18:31:57Z

@ShimonBenYair btw i thought @amiramw @TomHermanSAP @offer8 were working on it, please give a notice that you want to work on something which is obviously in discussions to avoid duplicate effort

amiramw · 2020-01-01T19:34:57Z

@ShimonBenYair is in our team so we are all synced. Sorry for the confusion.

akosyakov · 2020-01-01T19:41:40Z

@amiramw ok, thanks for letting to know. Could you take care about the review please and testing that it is aligned with VS Code interpretation of untitled schema? Don't get why i cannot assign you as a reviewer. I was pretty sure that you are an Eclipse Theia committer.

amiramw · 2020-01-01T19:43:32Z

Sure

ShimonBenYair · 2020-01-02T18:57:19Z

With changes in this pull request, when calling the above API a file is created and the user can modify and save the file with no problems.

I thought that untitled resources should not have physical representation till a user explicitly saved them? If a user just close the editor without saving then there should not be a trace of the file? cc @amiramw @TomHermanSAP @offer8

It would be nice if someone setups simple VS Code extension to manipulate resource with untitled schema. And then run it against VS Code and Theia to make sure that resources are treated the same. Running the sql extension is overkill.

After consulting with @amiramw i have fixed it so now when calling the 'vscode.window.showTextDocument' API with 'untitled' schema,the untitled resource will not have physical representation till the user will save it

packages/plugin-ext/src/main/browser/editor/untitled-resource.ts

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

packages/plugin-ext/src/main/browser/editor/untitled-resource.ts

akosyakov · 2020-01-03T03:55:23Z

VS Code uses the save file dialog to let a user to pick the proper file name, why we cannot do the same with https://github.com/eclipse-theia/theia/blob/master/packages/filesystem/src/browser/file-dialog/file-dialog-service.ts#L63?

akosyakov · 2020-01-03T07:00:27Z

@chrisguindon @eclipsewebmaster Some committers don't seem to belong to Eclipse Theia org, for example @amiramw. Is it the issue with sync script or some actions are expected from the committer?

packages/plugin-ext/src/main/browser/editor/untitled-resource.ts

akosyakov

It still does not behave like in VS Code and create a file with weird name instead of using a file dialog. Is it on purpose or there are some technical limitations to implement it properly?

packages/filesystem/src/browser/file-resource.ts

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

packages/plugin-ext/src/main/browser/editor/untitled-resource.ts

packages/plugin-ext/src/main/browser/documents-main.ts

packages/plugin-ext/src/main/browser/editor/untitled-resource.ts

chrisguindon · 2020-01-16T14:05:25Z

@chrisguindon @eclipsewebmaster Some committers don't seem to belong to Eclipse Theia org, for example @amiramw. Is it the issue with sync script or some actions are expected from the committer?

From what I can tell, @amiramw is a committer on the project: https://accounts.eclipse.org/users/amiramw#tab-projects

amiramw · 2020-01-16T14:11:48Z

@chrisguindon my issue was resolved after i fixed my github id in eclipse profile

akosyakov · 2020-01-17T03:03:11Z

It still does not behave like in VS Code and create a file with weird name instead of using a file dialog. Is it on purpose or there are some technical limitations to implement it properly?

@amiramw @ShimonBenYair Could you elaborate on it? We have FileDialogService, it should not be a problem to insert it in UntitledResourceResolver and prompt a user on saveContent method?

packages/plugin-ext/src/main/browser/editor/untitled-resource.ts

packages/filesystem/src/browser/file-resource.ts

akosyakov · 2020-01-17T03:16:48Z

Merge branch 'master' into untitledShimon

please use fetch + rebase (not merge) to keep the history straight, please see https://github.com/eclipse-theia/theia/blob/master/doc/pull-requests.md#checklist-commit-history

amiramw · 2020-01-21T09:05:56Z

@amiramw @ShimonBenYair Could you elaborate on it? We have FileDialogService, it should not be a problem to insert it in UntitledResourceResolver and prompt a user on saveContent method?

In the original issue vscode API was called with uri already so when the untitled document is saved there is no file save dialog.
@akosyakov are you aware of a vscode API where untitled is opens without uri so that the file dialog should open on save?

akosyakov · 2020-01-22T03:31:21Z

@ShimonBenYair @amiramw there is this path: https://github.com/ShimonBenYair/theia/blob/7e2e4afd88d18c7365cee7c03588c0a00600779a/packages/plugin-ext-vscode/src/browser/plugin-vscode-commands-contribution.ts#L141-L143

What happens if VS Code extension calls this command workbench.action.files.newUntitledFile ? Anyway If you are saying that there is not save dialog, then filing an issue workbench.action.files.newUntitledFile could be enough for now if it requires the save dialog.

Please though address remaining comments before landing.

amiramw · 2020-01-22T12:35:58Z

@akosyakov it seems that after the below code in the VS Code extension, fileUri contains inline a uri with schema "file".

const uri = vscode.Uri.file(os.homedir + '/newnew.txt');
const fileUri = vscode.Uri.parse(`untitled:${uri}`).with({ scheme: 'untitled' });

If we want to be generic to other schemas (inside untitled schema) then we should not hold FileResourceResolver but should hold ResourceProvider and call its getter to get any underlined resource upon change.

Does it seem ok with you or untitled resource and file resolved are coupled by definition?

ShimonBenYair · 2020-02-16T19:38:12Z

yes, if changes to part outside of the plugin system are reverted, we can redesign it is later. Is it really important issue? Why a user will try to open already opened file?

So maybe I shall remove from the PR the part that tries to handle the tab duplication and open a new issue for it with minor priority ?

akosyakov · 2020-02-17T08:28:46Z

So maybe I shall remove from the PR the part that tries to handle the tab duplication and open a new issue for it with minor priority ?

yes, it was my suggestion, we don't affect anything, it still works for you to some extent and we can redesign it in another PR

ShimonBenYair · 2020-02-17T09:07:54Z

So maybe I shall remove from the PR the part that tries to handle the tab duplication and open a new issue for it with minor priority ?

yes, it was my suggestion, we don't affect anything, it still works for you to some extent and we can redesign it in another PR

Ok i will update the PR

ShimonBenYair · 2020-02-18T06:56:45Z

@akosyakov
PR was updated
please review

packages/plugin-ext/src/main/browser/editor/untitled-resource.ts

akosyakov

code-wise it looks good, except one comment

someone has to test that it does what expected, cc @amiramw

akosyakov · 2020-02-18T10:07:51Z

@ShimonBenYair Please file an issue for duplicate editors? I think we need a general issue to support untitled editors properly in core. And think about the holistic design instead of trying to fix it as a separate case in the plugin system.

amiramw · 2020-02-18T11:18:12Z

I will test it later this week.

akosyakov · 2020-02-19T05:40:27Z

@ShimonBenYair Just noticed that not all Resource APIs were covered: https://github.com/eclipse-theia/theia/blob/master/packages/core/src/common/resource.ts#L40 resourceVersion, onDidChangeContents and guessEncoding are missing.

ShimonBenYair · 2020-02-19T12:48:07Z

@ShimonBenYair Just noticed that not all Resource APIs were covered: https://github.com/eclipse-theia/theia/blob/master/packages/core/src/common/resource.ts#L40 resourceVersion, onDidChangeContents and guessEncoding are missing.

@akosyakov
I saw that those methods are optional and that not all the classes that implements 'Resource' implements them. e.g.
https://github.com/eclipse-theia/theia/blob/master/packages/debug/src/browser/debugresource.ts#L23

https://github.com/eclipse-theia/theia/blob/master/packages/java/src/browser/java-resource.ts#L24

Are you sure i need to implement them on UntitledResource ?
Also for example, the guessEncoding method of the file-resource.ts calls the 'filesystem.gueseEncoding' method (Guess encoding of a given file based on its content) but our untitled resource does not have yet a representation in the file system.

What is the 'onDidChangeContents' responsible for ? ( i didn't find any documentation for it and i was not able to stop there when setting a breakpoint) .
What do i need to implement in 'onDidChangeContents' ?

akosyakov · 2020-02-20T09:34:12Z

@ShimonBenYair UntitledResource should implement everything which is implemented by FileResource to support it completely, otherwise it will misbehave. When it does not have representation on the file disk it should return nothing. onDidChangeContents is responsible to notify then resource is changed, i.e. file is changed outside of Theia.

ShimonBenYair · 2020-02-21T00:51:45Z

@ShimonBenYair UntitledResource should implement everything which is implemented by FileResource to support it completely, otherwise it will misbehave. When it does not have representation on the file disk it should return nothing. onDidChangeContents is responsible to notify then resource is changed, i.e. file is changed outside of Theia.

done
Please merge this PR

packages/plugin-ext/src/main/browser/editor/untitled-resource.ts

Fixes eclipse-theia#6565 Signed-off-by: Shimon Ben Yair <shimon.ben.yair@sap.com>

ShimonBenYair · 2020-02-24T09:44:42Z

@akosyakov

please review again
I have fixed everything
please merge

akosyakov · 2020-02-24T09:58:46Z

please merge

@ShimonBenYair just to be clear, i'm not trying to prevent your progress or something like that. Each time when I look I find something like it will leak memory, it will ignore changes on the disk and so on. When I see that it does not break, then I merge.

Code looks good now, I'm going to test it and if it is fine I merge it. We have a rule that we don't merge things which were tested only by the author: https://github.com/eclipse-theia/theia/blob/master/doc/pull-requests.md#landing-pr That's why asked someone else to test it.

akosyakov · 2020-02-24T10:33:49Z

Tested, it behaves well, did not find memory leaks and it reacts to changes on the disk as well.

The only error was:

root ERROR [hosted-plugin: 10377] Promise rejection not handled in one second: Error: Command with id 'SQLTools.getConnectionStatus' is not registered. , reason: Error: Command with id 'SQLTools.getConnectionStatus' is not registered.
With stack trace: Error: Command with id 'SQLTools.getConnectionStatus' is not registered.
    at CommandRegistryMainImpl.<anonymous> (https://3000-e0a15803-6cd7-4df2-8e51-fd074f296fde.ws-eu01.gitpod-staging.com/60.bundle.js:2027:35)
    at step (https://3000-e0a15803-6cd7-4df2-8e51-fd074f296fde.ws-eu01.gitpod-staging.com/60.bundle.js:1938:23)
    at Object.next (https://3000-e0a15803-6cd7-4df2-8e51-fd074f296fde.ws-eu01.gitpod-staging.com/60.bundle.js:1919:53)
    at https://3000-e0a15803-6cd7-4df2-8e51-fd074f296fde.ws-eu01.gitpod-staging.com/60.bundle.js:1913:71
    at new Promise (<anonymous>)
    at push.../../packages/plugin-ext/lib/main/browser/command-registry-main.js.__awaiter (https://3000-e0a15803-6cd7-4df2-8e51-fd074f296fde.ws-eu01.gitpod-staging.com/60.bundle.js:1909:12)
    at CommandRegistryMainImpl.push.../../packages/plugin-ext/lib/main/browser/command-registry-main.js.CommandRegistryMainImpl.$executeCommand (https://3000-e0a15803-6cd7-4df2-8e51-fd074f296fde.ws-eu01.gitpod-staging.com/60.bundle.js:2020:16)
    at RPCProtocolImpl.push.../../packages/plugin-ext/lib/common/rpc-protocol.js.RPCProtocolImpl.doInvokeHandler (https://3000-e0a15803-6cd7-4df2-8e51-fd074f296fde.ws-eu01.gitpod-staging.com/39.bundle.js:849:23)
    at RPCProtocolImpl.push.../../packages/plugin-ext/lib/common/rpc-protocol.js.RPCProtocolImpl.invokeHandler (https://3000-e0a15803-6cd7-4df2-8e51-fd074f296fde.ws-eu01.gitpod-staging.com/39.bundle.js:834:41)
    at RPCProtocolImpl.push.../../packages/plugin-ext/lib/common/rpc-protocol.js.RPCProtocolImpl.receiveRequest (https://3000-e0a15803-6cd7-4df2-8e51-fd074f296fde.ws-eu01.gitpod-staging.com/39.bundle.js:798:31)

It is unrelated to the PR, it would be worth to file a PR to fix it as well.

akosyakov · 2020-02-24T10:34:26Z

@ShimonBenYair opening follow-up issues would be good, about missing commands and duplicate editors for untitled and file schemes for the same path

ShimonBenYair changed the title ~~Fix 'window.showTextDocument' to open resources with 'untitled' schem…~~ Fix 'window.showTextDocument' to open resources with 'untitled' schema Dec 31, 2019

akosyakov added the vscode issues related to VSCode compatibility label Jan 1, 2020

akosyakov mentioned this pull request Jan 1, 2020

"window.showTextDocument" Does Not open Resources With "untitled" Scheme #6565

Closed

akosyakov reviewed Jan 1, 2020

View reviewed changes

packages/plugin-ext/src/main/browser/editor/untitled-resource.ts Outdated Show resolved Hide resolved

akosyakov reviewed Jan 1, 2020

View reviewed changes

packages/plugin-ext/src/main/browser/editor/untitled-resource.ts Outdated Show resolved Hide resolved

ShimonBenYair force-pushed the untitledShimon branch from 61d96f6 to c216481 Compare January 2, 2020 16:29

akosyakov reviewed Jan 3, 2020

View reviewed changes

amiramw reviewed Jan 5, 2020

View reviewed changes

packages/plugin-ext/src/main/browser/editor/untitled-resource.ts Outdated Show resolved Hide resolved

ShimonBenYair force-pushed the untitledShimon branch from c216481 to d6052d6 Compare January 15, 2020 14:49

akosyakov reviewed Jan 16, 2020

View reviewed changes

ShimonBenYair force-pushed the untitledShimon branch from de302a9 to 3a6ae10 Compare January 16, 2020 14:41

akosyakov reviewed Jan 17, 2020

View reviewed changes

ShimonBenYair force-pushed the untitledShimon branch from 7e2e4af to d86edea Compare January 22, 2020 13:38

ShimonBenYair force-pushed the untitledShimon branch from eb637a7 to ee339b6 Compare February 16, 2020 19:31

ShimonBenYair force-pushed the untitledShimon branch from ee339b6 to 24e86b1 Compare February 18, 2020 03:54

akosyakov reviewed Feb 18, 2020

View reviewed changes

packages/plugin-ext/src/main/browser/editor/untitled-resource.ts Outdated Show resolved Hide resolved

akosyakov approved these changes Feb 18, 2020

View reviewed changes

ShimonBenYair force-pushed the untitledShimon branch from 24e86b1 to 3ef0192 Compare February 18, 2020 20:40

ShimonBenYair force-pushed the untitledShimon branch from 3ef0192 to ab7573e Compare February 19, 2020 09:34

ShimonBenYair force-pushed the untitledShimon branch from ab7573e to a49238f Compare February 21, 2020 00:47

akosyakov reviewed Feb 21, 2020

View reviewed changes

packages/plugin-ext/src/main/browser/editor/untitled-resource.ts Show resolved Hide resolved

packages/plugin-ext/src/main/browser/editor/untitled-resource.ts Show resolved Hide resolved

packages/plugin-ext/src/main/browser/editor/untitled-resource.ts Outdated Show resolved Hide resolved

ShimonBenYair force-pushed the untitledShimon branch from a49238f to fd43f72 Compare February 22, 2020 22:29

akosyakov reviewed Feb 24, 2020

View reviewed changes

packages/plugin-ext/src/main/browser/editor/untitled-resource.ts Show resolved Hide resolved

akosyakov reviewed Feb 24, 2020

View reviewed changes

packages/plugin-ext/src/main/browser/editor/untitled-resource.ts Outdated Show resolved Hide resolved

Fix 'window.showTextDocument' to open resources with 'untitled' schema,

7b3771f

Fixes eclipse-theia#6565 Signed-off-by: Shimon Ben Yair <shimon.ben.yair@sap.com>

ShimonBenYair force-pushed the untitledShimon branch from fd43f72 to 7b3771f Compare February 24, 2020 09:40

akosyakov merged commit 0efc669 into eclipse-theia:master Feb 24, 2020

akosyakov mentioned this pull request Feb 25, 2020

Missing command - 'SQLTools.getConnectionStatus' #7212

Open

westbury mentioned this pull request Jan 11, 2022

Fix 'Save As' when saving a document with 'untitled' scheme #10608

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix 'window.showTextDocument' to open resources with 'untitled' schema #6803

Fix 'window.showTextDocument' to open resources with 'untitled' schema #6803

ShimonBenYair commented Dec 31, 2019 •

edited

Loading

ShimonBenYair commented Dec 31, 2019

akosyakov commented Jan 1, 2020 •

edited

Loading

akosyakov Jan 1, 2020

ShimonBenYair Jan 16, 2020

akosyakov commented Jan 1, 2020

amiramw commented Jan 1, 2020

akosyakov commented Jan 1, 2020 •

edited

Loading

amiramw commented Jan 1, 2020

ShimonBenYair commented Jan 2, 2020

akosyakov commented Jan 3, 2020

akosyakov commented Jan 3, 2020

akosyakov left a comment

chrisguindon commented Jan 16, 2020

amiramw commented Jan 16, 2020

akosyakov commented Jan 17, 2020

akosyakov commented Jan 17, 2020

amiramw commented Jan 21, 2020

akosyakov commented Jan 22, 2020

amiramw commented Jan 22, 2020

ShimonBenYair commented Feb 16, 2020

akosyakov commented Feb 17, 2020

ShimonBenYair commented Feb 17, 2020

ShimonBenYair commented Feb 18, 2020

akosyakov left a comment

akosyakov commented Feb 18, 2020

amiramw commented Feb 18, 2020

akosyakov commented Feb 19, 2020

ShimonBenYair commented Feb 19, 2020 •

edited

Loading

akosyakov commented Feb 20, 2020

ShimonBenYair commented Feb 21, 2020 •

edited

Loading

ShimonBenYair commented Feb 24, 2020

akosyakov commented Feb 24, 2020 •

edited

Loading

akosyakov commented Feb 24, 2020

akosyakov commented Feb 24, 2020

Fix 'window.showTextDocument' to open resources with 'untitled' schema #6803

Fix 'window.showTextDocument' to open resources with 'untitled' schema #6803

Conversation

ShimonBenYair commented Dec 31, 2019 • edited Loading

What it does

How to test

Review checklist

Reminder for reviewers

ShimonBenYair commented Dec 31, 2019

akosyakov commented Jan 1, 2020 • edited Loading

akosyakov Jan 1, 2020

Choose a reason for hiding this comment

ShimonBenYair Jan 16, 2020

Choose a reason for hiding this comment

akosyakov commented Jan 1, 2020

amiramw commented Jan 1, 2020

akosyakov commented Jan 1, 2020 • edited Loading

amiramw commented Jan 1, 2020

ShimonBenYair commented Jan 2, 2020

akosyakov commented Jan 3, 2020

akosyakov commented Jan 3, 2020

akosyakov left a comment

Choose a reason for hiding this comment

chrisguindon commented Jan 16, 2020

amiramw commented Jan 16, 2020

akosyakov commented Jan 17, 2020

akosyakov commented Jan 17, 2020

amiramw commented Jan 21, 2020

akosyakov commented Jan 22, 2020

amiramw commented Jan 22, 2020

ShimonBenYair commented Feb 16, 2020

akosyakov commented Feb 17, 2020

ShimonBenYair commented Feb 17, 2020

ShimonBenYair commented Feb 18, 2020

akosyakov left a comment

Choose a reason for hiding this comment

akosyakov commented Feb 18, 2020

amiramw commented Feb 18, 2020

akosyakov commented Feb 19, 2020

ShimonBenYair commented Feb 19, 2020 • edited Loading

akosyakov commented Feb 20, 2020

ShimonBenYair commented Feb 21, 2020 • edited Loading

ShimonBenYair commented Feb 24, 2020

akosyakov commented Feb 24, 2020 • edited Loading

akosyakov commented Feb 24, 2020

akosyakov commented Feb 24, 2020

ShimonBenYair commented Dec 31, 2019 •

edited

Loading

akosyakov commented Jan 1, 2020 •

edited

Loading

akosyakov commented Jan 1, 2020 •

edited

Loading

ShimonBenYair commented Feb 19, 2020 •

edited

Loading

ShimonBenYair commented Feb 21, 2020 •

edited

Loading

akosyakov commented Feb 24, 2020 •

edited

Loading