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

westbury · 2022-01-10T10:09:41Z

What it does

This allows the user to save documents with 'untitled' scheme. These documents are often opened by plugins that want to show content that is not in underlying storage (typically dynamically generated)

How to test

To test this you will need a plugin that creates documents with 'untitled' scheme. The AWS toolkit extension does this (https://open-vsx.org/extension/amazonwebservices/aws-toolkit-vscode). Having configured an Access Key ID and a Secret Access Key and connected to your AWS account, in the AWS Explorer you can either:

view a policy under IoT / Policies, right-click on a policy version and View...
view a schema under Schemas / aws.events, right-click and View Schema.

Either of these should open an editor with a document with 'untitled' scheme.

Without this fix you will not be able so use the 'Save As...' menu to save untitled documents. It silently fails. (The FileSystem copy fails because 'untitled' is not a supported scheme).

Note on the fix: It might have been simpler to create an empty file in all cases. However I decided to leave it as is when the FileSystem supports the scheme because I assume there are reasons the file was being copied. This could be to preserve file permissions for example. The FileSystem implementation does support copying across from one provider to another.

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

msujew

For anyone interested in testing this, but unwilling to go through the AWS setup, I created a small sample extension for this. Run the Show test editor command to create a simple untitled file.

For the change itself: While this works for the Save As command, simply saving the document using Ctrl+S will throw an error: NoPermissions (FileSystemError): Error: EPERM: operation not permitted, open 'C:\Untitled-0'.

For feature completeness and to better align with vscode, I would recommend that we automatically delegate to the Save As command when saving a file with the untitled scheme. What do you think @westbury?

westbury · 2022-01-10T15:49:33Z

For feature completeness and to better align with vscode, I would recommend that we automatically delegate to the Save As command when saving a file with the untitled scheme.

Sounds good. It is certainly common for applications to open up the 'Save As' dialog when saving an untitled file. I just need to think about a good way to do that because 'Save' is in core package and 'Save As' is in workspace package. 'Save As' can and does call 'Save', see

theia/packages/workspace/src/browser/workspace-frontend-contribution.ts

Line 497 in 52a6529

await this.commandRegistry.executeCommand(CommonCommands.SAVE.id);

but the other way around is more problematic. I'll find a clean way of doing it.

westbury · 2022-01-11T10:23:38Z

For feature completeness and to better align with vscode, I would recommend that we automatically delegate to the Save As command when saving a file with the untitled scheme.

This is far harder than it sounds. See the comments in #6803 in which Anton requested this in another PR and accepted it was not straightforward. The problem is that 'save' support is in the core package and baked into the core and monaco packages. Implementations can be placed in the Resource or ResourceResolver but that is really too late to switch from 'save' to 'save as'.

Also note that 'save' should work for 'untitled', see

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

Lines 94 to 102 in 52a6529

    
           async saveContents(content: string, options?: { encoding?: string, overwriteEncoding?: boolean }): Promise<void> { 
        
               if (!this.fileResource) { 
        
                   this.fileResource = await this.fileResourceResolver.resolve(new URI(this.uri.path.toString())); 
        
                   if (this.fileResource.onDidChangeContents) { 
        
                       this.fileResource.onDidChangeContents(() => this.fireDidChangeContents()); 
        
                   } 
        
               } 
        
               await this.fileResource.saveContents(content, options); 
        
           }

However, even if it is fixed, the file is saved silently in a location where users are not likely to find it. So I would be inclined to remove the optional saveContents method, leaving untitled documents as read-only. This would be consistent, and would mean users are not prompted to save their untitled documents when they 'save all'. If we want the alternative, to align with vscode, then we need to be prepared to accept more significant architectural changes.

I think this needs to be handled in a separate PR.

msujew · 2022-01-11T10:38:18Z

@westbury I see. My idea would be to perhaps create a SaveAsService, which is defined at core level, but only bound in the workspace package. If the workspace package isn't used (which is unlikely) we keep the current behavior. What do you think about that? I don't see too many architectural changes in that, since it would just move the saveAs implementation in its own service.

westbury · 2022-01-13T09:52:29Z

@msujew I've pushed a second commit which adds support for the 'Save' action on untitled resources, invoking the 'Save As' code to prompt the user for a file name. In addition to the 'save' service, there are also a few changes so we can determine early if a widget contains an untitled resource.

This replaces most of the changes added by #6803 . The way saving of untitled documents was handled by the Resource itself was very limiting. It was not possible to have user interactions such as prompts for file names. Furthermore there is good code in the 'Save As' that seamlessly replaces the 'untitled' editor with the 'file' editor, which also could not work when saving is done by the Resource. I therefore removed all the saving code from untitled-resource.ts. I did leave a stub implementation of saveContents. This was necessary to ensure the untitled documents remain editable.

It would be logical to make untitled documents read-only. The user would then have to save to a file before starting to make edits. However it may not be intuitive to the user that they need to do this in order to start editing the contents. Also it would not be aligned with vscode.

'Save All' does not save untitled documents. This is as it currently works and it may be confusing if the user is prompted for lots of file names when they 'Save All' with lots of untitled documents.

msujew

The new approach works quite well. I have some smaller suggestions.

Additionally I noticed that this does not play well with the new Close all saved context menu command. It will also close the untitled file since it counts as being "saved". I don't really have a huge problem with it, since it deals with saving untitled files nicely, which is a higher priority in my opinion.

@colin-grant-work You implemented the context menu entry, your opinion on that?

msujew · 2022-01-13T13:26:04Z

packages/core/src/browser/save-resource-service.ts

+ ********************************************************************************/
+
+import { injectable } from 'inversify';
+import { Saveable, SaveOptions, Widget } from '.';


Minor: Don't use index exports from the same package.

msujew · 2022-01-13T13:27:28Z

packages/workspace/src/browser/workspace-frontend-contribution.ts

@@ -434,7 +434,7 @@ export class WorkspaceFrontendContribution implements CommandContribution, Keybi
     * - `widget.saveable.createSnapshot` is defined.
     * - `widget.saveable.revert` is defined.
     */
-    protected canBeSavedAs(widget: Widget | undefined): widget is Widget & SaveableSource & Navigatable {
+    public canBeSavedAs(widget: Widget | undefined): widget is Widget & SaveableSource & Navigatable {


Minor: methods are public by default, no need for the public modifier.

msujew · 2022-01-13T13:27:33Z

packages/workspace/src/browser/workspace-frontend-contribution.ts

@@ -446,12 +446,17 @@ export class WorkspaceFrontendContribution implements CommandContribution, Keybi
    /**
     * Save `sourceWidget` to a new file picked by the user.
     */
-    protected async saveAs(sourceWidget: Widget & SaveableSource & Navigatable): Promise<void> {
+    public async saveAs(sourceWidget: Widget & SaveableSource & Navigatable): Promise<void> {


Minor: methods are public by default, no need for the public modifier.

msujew · 2022-01-13T13:31:30Z

packages/workspace/src/browser/workspace-save-resource-service.ts

+        return Saveable.isDirty(saveable) || Saveable.isUntitled(saveable);
+    }
+
+    public async save(widget: Widget | undefined, options?: SaveOptions): Promise<void> {


Minor: methods are public by default, no need for the public modifier.

msujew · 2022-01-17T14:36:09Z

packages/core/src/browser/common-frontend-contribution.ts

+    protected async save(options?: SaveOptions): Promise<void> {
+        const widget = this.shell.currentWidget;
+        this.saveResourceService.save(widget, options);
+    }


Suggestion: I think it would be nice from an API perspective to replace the existing ApplicationShell.save with this implementation here. Having multiple saving mechanisms in different places will probably lead to some confusion.

colin-grant-work · 2022-01-18T16:05:07Z

Additionally I noticed that this does not play well with the new Close all saved context menu command. It will also close the untitled file since it counts as being "saved". I don't really have a huge problem with it, since it deals with saving untitled files nicely, which is a higher priority in my opinion.

@colin-grant-work You implemented the context menu entry, your opinion on that?

@msujew, this is definitely a deviation from VSCode, but the real issue isn't with the 'Close All Saved' command. When you open a test document in VSCode using your example plugin, the document is automatically marked as dirty, whereas in Theia it is not. The Close All Saved command just trusts the Saveable implementation to know whether something is dirty, so the easiest solution here would be to mark the underlying model dirty when the editor is opened - then the behavior would be the same as VSCode's.

I also noticed that the Copy Path and Copy Relative Path commands are active for untitled resources in Theia, but not VSCode. In that case, we may want to modify the enabled status of the commands depending on the URI scheme they find, but I don't think that necessarily has to go in this PR.

msujew · 2022-01-31T14:03:19Z

@westbury I agree with Colin that untitled resources should always be marked as dirty. I think this would probably alleviate most of the issues encountered here.

colin-grant-work · 2022-03-07T21:50:36Z

@westbury, are you interested in continuing work on this feature? I was thinking of moving some of the machinery for creating untitled resources out of the plugin package so that it could be used by the 'New File' command to create an empty editor rather than requiring the user to immediately create a new file, and this PR significantly improves our handling untitled resources; if you are busy with other work, I'd be happy to pick it up address the remaining comments.

westbury · 2022-03-07T22:07:13Z

@colin-grant-work The problem is that marking all untitled documents as 'unsaved' would not be the correct thing to do in our case. In VSCode, when one creates a new file using File, New, the file is not marked as dirty and one can close it without getting a prompt. In the case of the AWS plugin we definitely would not want the untitled document to be marked as dirty. It should be marked as dirty only if the user changes the text.

The 'untitled' is used in Theia not for a new file but when text data is to shown and that text data comes from a source that is not in a file system that can provide a file name. If the user doesn't change the data then there is no unsaved data.

Sorry, I meant to respond earlier but I was not sure how to resolve these different expectations.

colin-grant-work · 2022-03-07T22:30:14Z

VSCode has this interesting comment about opening untitled editors:

https://github.com/microsoft/vscode/blob/ab49f4f310a014c9e03839f37c1ca3c48196aeca/src/vs/workbench/common/editor.ts#L417-L433

So it seems that the decision about whether to mark the editor as dirty has in part to do with way it's originally opened and the point at which a URI is created. At the moment, we don't have distinct handling for different untitled cases (untitled because new, untitled because just a temporary reference to some text data, etc.), but we would need to implement some to match VSCode's behavior in this area.

colin-grant-work · 2022-03-07T23:19:26Z

@westbury, @msujew, what do you think of proceeding with the PR as it currently is, and then iterating on it as part of a separate PR aimed at addressing #3354?

msujew · 2022-03-08T10:54:36Z

@colin-grant-work I see, yeah, I'd be fine with that 👍

colin-grant-work

This is a significant improvement over the current code and does not produce any noticeable regressions. The behavior can be further refined in later PR's.

colin-grant-work · 2022-03-08T15:09:43Z

@westbury, would you mind rebasing the PR so that it can be merged? I'm not sure exactly what's going on with the 3PP check (@vince-fugnitto) - I don't see any changes in dependencies in the PR - but perhaps that will resolve itself with another push.

westbury · 2022-03-09T16:52:38Z

@colin-grant-work Thanks for approving this. I've rebased and retested with the AWS plugin and it works as expected. I have not made all untitled as dirty, as discussed.

Also #10608 (comment) is still outstanding because it still calls Saveable.save and saves the current resource so is not really the same.

colin-grant-work · 2022-03-09T19:02:43Z

In the case of the AWS plugin we definitely would not want the untitled document to be marked as dirty.

@westbury, have you confirmed this behavior? It appears that if you use the View Schema command, you end up in this code:

https://github.com/aws/aws-toolkit-vscode/blob/3ea63e0952b322b6f24e811a341c02ef99cac0c3/src/eventSchemas/commands/viewSchemaItem.ts#L44-L54

When I mimic that behavior in a VSCode plugin command:

	context.subscriptions.push(vscode.commands.registerCommand('untitled-editors.language.lateContent', async () => {
		const newDoc = await vscode.workspace.openTextDocument({
			language: 'json',
		})
		const editor = await vscode.window.showTextDocument(newDoc, vscode.ViewColumn.One, false)
		await editor.edit(edit => edit.insert(new vscode.Position(/*line*/ 0, /*character*/ 0), '{"content": "for example"}'))
	}));

The resulting editor is marked as dirty in VSCode. Similarly, the other command you mentioned to view policy version gets to this code:

https://github.com/aws/aws-toolkit-vscode/blob/3ea63e0952b322b6f24e811a341c02ef99cac0c3/src/iot/commands/viewPolicyVersion.ts#L37-L47

And a command that mimics that behavior:

	context.subscriptions.push(vscode.commands.registerCommand('untitled-editors.language.earlyContent', async () => {
		const newDoc = await vscode.workspace.openTextDocument({
			language: 'json',
			content: JSONContent,
		})
		const editor = await vscode.window.showTextDocument(newDoc, vscode.ViewColumn.One, false)
	}));

Also produces a dirty editor. So far, it appears that any untitled editor that is not completely empty is marked as dirty, which seems correct to me: if there's any content that doesn't match the backing store (none, in this case), then the editor is dirty.

westbury · 2022-03-10T22:40:30Z

@colin-grant-work Here is a recording of what I see. I view a couple of policies but I don't edit the text. When I 'close all' they are both closed without prompt, as I would expect. I then make a change. Now you see the dirty indicator and when I close the editor, I am prompted to save, as I would expect. Also if I select 'Save' then I am prompted for a file as though I had selected 'Save As'.

Screencast.2022-03-10.22.31.44.mp4

In your test code it appears you are creating the documents as empty documents. This is not the usual case, as when you view a textual representation of some data it would not normally be empty.

colin-grant-work · 2022-03-10T22:43:36Z

@westbury, thanks for the recording. It looks like you're demonstrating what happens in Theia, where we know that they're not marked as dirty, but in order to match the expectations of VSCode plugins, we would want to mark the editors dirty as VSCode does - do you know if the editors created by the AWS plugin show up as dirty in VSCode? My tests suggests that they would, but it's possible that my code differs in execution in some way from the actual plugin's.

westbury · 2022-03-11T10:30:53Z

@colin-grant-work Sorry I misunderstood your comment. You are correct that untitled documents show immediately as dirty in VSCode. Which means if users view lots of AWS policies, schemas etc. then they have to click 'don't save' for each one

Screencast.2022-03-11.10.34.16.mp4

msujew self-requested a review January 10, 2022 11:49

westbury force-pushed the save-untitled branch from 7d1ed38 to 422dd9a Compare January 10, 2022 12:04

msujew reviewed Jan 10, 2022

View reviewed changes

colin-grant-work added filesystem issues related to the filesystem vscode issues related to VSCode compatibility labels Jan 10, 2022

westbury force-pushed the save-untitled branch 2 times, most recently from 3a8792b to e7b8bad Compare January 13, 2022 09:10

msujew self-requested a review January 13, 2022 10:59

msujew reviewed Jan 17, 2022

View reviewed changes

colin-grant-work self-requested a review January 18, 2022 15:48

colin-grant-work approved these changes Mar 8, 2022

View reviewed changes

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

2f64430

westbury force-pushed the save-untitled branch from e7b8bad to ccb6b06 Compare March 9, 2022 15:38

'Save' action now same as 'Save As...' for untitled scheme

dc4b1dc

westbury force-pushed the save-untitled branch from ccb6b06 to dc4b1dc Compare March 9, 2022 16:16

westbury merged commit e62fd33 into eclipse-theia:master Mar 9, 2022

westbury deleted the save-untitled branch March 9, 2022 16:53

colin-grant-work mentioned this pull request Mar 10, 2022

New File opens untitled editor #10868

Merged

1 task

vince-fugnitto added this to the 1.24.0 milestone Mar 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

westbury commented Jan 10, 2022

msujew left a comment

westbury commented Jan 10, 2022

westbury commented Jan 11, 2022

msujew commented Jan 11, 2022 •

edited

Loading

westbury commented Jan 13, 2022

msujew left a comment

msujew Jan 13, 2022

msujew Jan 13, 2022

msujew Jan 13, 2022

msujew Jan 13, 2022

msujew Jan 17, 2022

colin-grant-work commented Jan 18, 2022

msujew commented Jan 31, 2022

colin-grant-work commented Mar 7, 2022

westbury commented Mar 7, 2022

colin-grant-work commented Mar 7, 2022

colin-grant-work commented Mar 7, 2022

msujew commented Mar 8, 2022

colin-grant-work left a comment

colin-grant-work commented Mar 8, 2022

westbury commented Mar 9, 2022

colin-grant-work commented Mar 9, 2022 •

edited

Loading

westbury commented Mar 10, 2022

colin-grant-work commented Mar 10, 2022 •

edited

Loading

westbury commented Mar 11, 2022 •

edited

Loading

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

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

Conversation

westbury commented Jan 10, 2022

What it does

How to test

Review checklist

Reminder for reviewers

msujew left a comment

Choose a reason for hiding this comment

westbury commented Jan 10, 2022

westbury commented Jan 11, 2022

msujew commented Jan 11, 2022 • edited Loading

westbury commented Jan 13, 2022

msujew left a comment

Choose a reason for hiding this comment

msujew Jan 13, 2022

Choose a reason for hiding this comment

msujew Jan 13, 2022

Choose a reason for hiding this comment

msujew Jan 13, 2022

Choose a reason for hiding this comment

msujew Jan 13, 2022

Choose a reason for hiding this comment

msujew Jan 17, 2022

Choose a reason for hiding this comment

colin-grant-work commented Jan 18, 2022

msujew commented Jan 31, 2022

colin-grant-work commented Mar 7, 2022

westbury commented Mar 7, 2022

colin-grant-work commented Mar 7, 2022

colin-grant-work commented Mar 7, 2022

msujew commented Mar 8, 2022

colin-grant-work left a comment

Choose a reason for hiding this comment

colin-grant-work commented Mar 8, 2022

westbury commented Mar 9, 2022

colin-grant-work commented Mar 9, 2022 • edited Loading

westbury commented Mar 10, 2022

colin-grant-work commented Mar 10, 2022 • edited Loading

westbury commented Mar 11, 2022 • edited Loading

msujew commented Jan 11, 2022 •

edited

Loading

colin-grant-work commented Mar 9, 2022 •

edited

Loading

colin-grant-work commented Mar 10, 2022 •

edited

Loading

westbury commented Mar 11, 2022 •

edited

Loading