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

editor: save dirty editors when toggling auto-save on #8163

Merged
merged 1 commit into from
Jul 20, 2020

Conversation

vince-fugnitto
Copy link
Member

What it does

The following pull-request updates the toggleAutoSave method to trigger a saveAll (for all dirty content), when the auto-save preference value is turned on. This behavior of saving dirty content across multiple editors is consistent with vscode and improves the user-experience.

How to test

  1. start the application with a workspace present
  2. ensure that auto-save is off
  3. make dirty (unsaved) changes in multiple files
  4. update the auto-save by enabling it in the file menu
  5. the dirty editors should be saved

Review checklist

Reminder for reviewers

Signed-off-by: vince-fugnitto [email protected]

@vince-fugnitto vince-fugnitto added enhancement issues that are enhancements to current functionality - nice to haves editor issues related to the editor labels Jul 13, 2020
@vince-fugnitto vince-fugnitto self-assigned this Jul 13, 2020
@vince-fugnitto vince-fugnitto force-pushed the vf/auto-save branch 3 times, most recently from c68d444 to 86ee904 Compare July 13, 2020 15:15
Copy link
Contributor

@Anasshahidd21 Anasshahidd21 left a comment

Choose a reason for hiding this comment

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

Looks well for me.

  1. Tested with setting the preference on the autoSave to on.
  2. Tested with the autoSave toggle in the file-menu.

OS: Windows.

@vince-fugnitto vince-fugnitto requested a review from lmcbout July 13, 2020 19:00
@lmcbout
Copy link
Contributor

lmcbout commented Jul 13, 2020

Tested on UBUNTU 18.04 and Chrome

  • Set the File-> Auto Save to "OFF"
  • I opened the file setting.json from the workspace
  • modify a few files with auto-save "OFF"
  • Open menu, then select "Auto Save"
    --> Since the file setting.json was in the editor, it puts the auto-save:"on" but did not save the preference file setting, so the other file did not get saved either.
    This is a corner case here, otherwise, turning the "Auto Save" to "ON" works fine with any other files.

The following commit triggers a `saveAll` (for all dirty content), when
the `auto-save` is turned on.

Signed-off-by: vince-fugnitto <[email protected]>
@vince-fugnitto
Copy link
Member Author

@lmcbout do you mind re-reviewing the pull-request?

This is a corner case here, otherwise, turning the "Auto Save" to "ON" works fine with any other files.

I do not believe the comment you had about the settings.json is a bug.
We trigger a saveAll based on the preference changed event and if the settings.json is simply modified (off to on) this does not trigger an event, only explicitly saving the document will. We should not try to interpret that the user has unmodified changes for autoSave and trigger a save in my opinion.

@lmcbout
Copy link
Contributor

lmcbout commented Jul 14, 2020

Tested with commit 42a09db
On Ubuntu 18.04 and Chrome
If the file "setting.json" is not open in the editor, it works fine. OK
If the file "settings.json" is OPEN in the editor and I set "AutoSave" = "ON"
Then it does not autoSave

AutoSaveWithSettingOpen

@vince-fugnitto
Copy link
Member Author

vince-fugnitto commented Jul 14, 2020

@lmcbout the bug about the toggle command causing dirty changes to the settings.json is on master as well.

Copy link
Contributor

@lmcbout lmcbout left a comment

Choose a reason for hiding this comment

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

Thanks @vince-fugnitto
LGTM

@vince-fugnitto
Copy link
Member Author

I'll merge tomorrow if there are no objections :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editor issues related to the editor enhancement issues that are enhancements to current functionality - nice to haves
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants