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

Make save as command not overwrite original file and open the new file. #8514

Conversation

leopoldwe
Copy link

@leopoldwe leopoldwe commented Sep 16, 2020

Signed-off-by: Leopold Eckert leopoldwe@gmail.com

What it does

Fixes #8373
Fixes #8504

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

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

Review checklist

Reminder for reviewers

…e. Signed-off-by: Leopold Eckert <leopoldwe@gmail.com>
@kittaakos kittaakos self-requested a review September 17, 2020 06:50
@vince-fugnitto vince-fugnitto added filesystem issues related to the filesystem workspace issues related to the workspace labels Sep 17, 2020
@@ -417,12 +417,18 @@ export class WorkspaceFrontendContribution implements CommandContribution, Keybi
}
} while (selected && exist && !overwrite);
if (selected) {
const temp = new URI(uri.toString() + '(copy).tmp');
Copy link
Member

Choose a reason for hiding this comment

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

The variable temp is not used outside of the try block, therefore I think it should be placed inside.

Copy link
Author

Choose a reason for hiding this comment

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

Ok. I moved it inside.

Comment on lines 430 to 431
await this.commandRegistry.executeCommand(CommonCommands.CLOSE_TAB.id);
await open(this.openerService, selected);
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to perform these operations even if there is an error in the try-catch?
Would it not be more valid to only perform these operations if the save was successful?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it makes more sense that way. I put the statements inside the try block.

Signed-off-by: Leopold Eckert <leopoldwe@gmail.com>
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 do no work well unfortunately, once a save as is performed the explorer is closed.

  1. perform a save as choosing a new location under the same workspace.
  2. the explorer is randomly closed after the operation.

@@ -3,6 +3,7 @@
## v1.6.0

- [core] added functionality for handling context menu for `tab-bars` without activating the shell tab-bar [#6965](https://github.com/eclipse-theia/theia/pull/6965)
- [workspace] "save as" command does not save changes to old file and opens the new file. [#8514](https://github.com/eclipse-theia/theia/pull/8514)
Copy link
Member

Choose a reason for hiding this comment

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

Please be sure to update the changelog by rebasing it and placing it under the correct release (1.7.0).

@leopoldwe leopoldwe force-pushed the stop-save-as-from-overwriting-original-file branch from 35a4117 to f417f94 Compare October 2, 2020 22:47
Signed-off-by: Leopold Eckert <leopoldwe@gmail.com>
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.

@leopoldwe the current approach does not work well, and results in weird behaviors:

  • perform a save as on a file and choose a location in the same workspace
  • the (copy) file is visible in the UI (I would not expect this as an end-user)
  • the original file is also marked as Deleted (by source control)

save-as

Signed-off-by: Leopold Eckert <leopoldwe@gmail.com>
@leopoldwe
Copy link
Author

I've tried to fix the source control problem, but I find it is outside of my abilities. I can't find a way to make the save as work and not a delete show in the source control temporarily.

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
2 participants