Skip to content

Save as command correctly creates a new file before saving#9022

Merged
vince-fugnitto merged 1 commit intoeclipse-theia:masterfrom
msujew:save-as-create-file
Apr 19, 2021
Merged

Save as command correctly creates a new file before saving#9022
vince-fugnitto merged 1 commit intoeclipse-theia:masterfrom
msujew:save-as-create-file

Conversation

@msujew
Copy link
Member

@msujew msujew commented Feb 3, 2021

What it does

Fixes #8373
Fixes #8504
Closes #8514

When using the "save as" command, a new file will be created with the new changes, and the old file will be unchanged. The text editor will then close the old file and open the new one.

How to test

  • Open a file and make some changes
  • Use "Save as" command
  • The text editor will now open the new file created with the changes. The old file will be closed.
  • The old file will be unchanged.

Review checklist

Reminder for reviewers

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

@Darkgaja thank you for taking care of the issue, the changes look very good already.
We should possibly change the enabled of the command to be available only when an editor is opened. With these changes, if a file node is selected in the explorer, the command is available, prompts the user to choose a name but does not perform an actual save (given that sourceWidget is undefined).

Else, we may not want to show the dialog at all when no editor is opened (similarly to vscode).

@vince-fugnitto vince-fugnitto added filesystem issues related to the filesystem workspace issues related to the workspace labels Feb 3, 2021
@msujew
Copy link
Member Author

msujew commented Feb 3, 2021

@vince-fugnitto sure, I'll look into this as well

@msujew msujew requested a review from vince-fugnitto February 3, 2021 18:52
@paul-marechal
Copy link
Member

The change seems to work, but I don't know what to think about a new tab being opened and the old one closing? Is there a way to not do that?

@vince-fugnitto
Copy link
Member

The change seems to work, but I don't know what to think about a new tab being opened and the old one closing? Is there a way to not do that?

@marechal-p I believe that is the intended behavior (similarly to vscode), when save as... is performed, the original editor is closed while the new one is opened. I believe vscode handles it more gracefully (without the UI change) by replacing the editor if that's what you're referring to.

@paul-marechal
Copy link
Member

by replacing the editor if that's what you're referring to.

This is what I meant. It is weird how it works right now, but if that's all we can do I'm fine.

@msujew msujew force-pushed the save-as-create-file branch from b2cc265 to 9143093 Compare February 5, 2021 15:59
Copy link
Contributor

@DucNgn DucNgn left a comment

Choose a reason for hiding this comment

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

  1. Open 2 editors: ./a.ts and ./folder/b.ts
  2. Make changes in a.ts and call Save as ... in the file, overwrite ./folder/b.ts
  3. A dialog displays upon saving:

image

Do you know if we can bypass this dialog?
The PR looks good to me otherwise

@msujew
Copy link
Member Author

msujew commented Feb 19, 2021

@dukengn I noticed that the dialog comes up because of the fileSystem.copy call. Checking whether the file exists beforehand eliminates the dialog.

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

@Darkgaja do you mind squashing your changes to a single commit with a nice commit message? I'll perform a review in the meantime.

@vince-fugnitto
Copy link
Member

Do you know if we can bypass this dialog?

@dukengn the dialog is presented when we attempt to overwrite a file (it should ask for confirmation first).
The same behavior is present in other applications, like vscode for instance under the same use-case.

@msujew
Copy link
Member Author

msujew commented Feb 23, 2021

@vince-fugnitto Actually, this is a different dialog. The confirmation dialog correctly pops up while selecting a file. The dialog @dukengn mentioned pops up because the fileSystem.copy call modifies a file that is opened within a dirty editor. In this case a competely unrelated event triggers and asks the user whether they want to overwrite the dirty editor state with the changes made on the filesystem. This additional dialog doesn't trigger in vscode.

Nevertheless, I will squash my commits.

@msujew msujew force-pushed the save-as-create-file branch 2 times, most recently from 6ed70d9 to b217bbc Compare February 24, 2021 16:44
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

@Darkgaja do you mind rebasing and resolving conflicts?

@msujew msujew force-pushed the save-as-create-file branch from b217bbc to 9333761 Compare February 26, 2021 15:50
@msujew
Copy link
Member Author

msujew commented Feb 26, 2021

@vince-fugnitto done.

@msujew msujew force-pushed the save-as-create-file branch from 80f0002 to 8de47fa Compare March 5, 2021 20:10
Copy link
Member

@paul-marechal paul-marechal left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@msujew msujew force-pushed the save-as-create-file branch from 8de47fa to 851abb9 Compare April 13, 2021 11:43
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

The changes look good 👍 I did encounter a minor bug that can likely be handled:

  1. open a file, and perform some dirty changes (ex: 'readme.md')
  2. execute the command Save As...
  3. save the file with the original name ('readme.md')
  4. a confirmation box will appear asking if you want to overwrite the original file
  5. the file is saved and immediately closed
save-as-bug.mp4

Signed-off-by: Mark Sujew <mark.sujew@typefox.io>
@msujew msujew force-pushed the save-as-create-file branch from 851abb9 to 62da848 Compare April 15, 2021 14:08
@msujew
Copy link
Member Author

msujew commented Apr 15, 2021

@vince-fugnitto When encountering the same URI as the source widget, my changes will now only execute the normal SAVE command.

@vince-fugnitto
Copy link
Member

@vince-fugnitto When encountering the same URI as the source widget, my changes will now only execute the normal SAVE command.

@Darkgaja I'll verify the new behavior, vscode behaves the following way:

  1. document is dirty.
  2. save as is performed with the same URI.
  3. the confirmation dialog appears.
  4. the save occurs, but the file is not closed.

@msujew
Copy link
Member Author

msujew commented Apr 15, 2021

Thanks. Right, I also took vscodes behavior as a sort of template. It should behave as you've laid out.

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

I confirmed that the changes work well 👍

Copy link
Contributor

@DucNgn DucNgn left a comment

Choose a reason for hiding this comment

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

The PR looks good to me 👍
I confirmed it aligns our save as behaviour with vscode.

@vince-fugnitto vince-fugnitto merged commit 199fb89 into eclipse-theia:master Apr 19, 2021
@msujew msujew deleted the save-as-create-file branch April 19, 2021 14:19
@vince-fugnitto vince-fugnitto added this to the 1.13.0 milestone Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

filesystem issues related to the filesystem workspace issues related to the workspace

Projects

None yet

Development

Successfully merging this pull request may close these issues.

"Save as..." should not save changes to original file "Save As..." should open the new file

5 participants