Skip to content

editor: fix 'auto-save' listener #8727

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

Closed
wants to merge 1 commit into from
Closed

Conversation

vince-fugnitto
Copy link
Member

What it does

Fixes: #8722

The following pull-request addresses an issue with the preference change event for editor.autoSave which would incorrectly process the event on app startup leading to an incorrect shell.saveAll() call. The logic for handling the event has been updated to only execute the handler when the autoSave preference is actually updated.

Thanks to @kittaakos for finding the bug and providing a quick fix.

How to test

  1. start the application with autoSave off - the listener should not call shell.autoSave on startup
  2. start the application with autoSave on - the listener should not call shell.autoSave on startup
  3. start the application with autoSave off
  4. make some changes to editors, and keep them dirty
  5. update autoSave to on
  6. the dirty editors should be saved

Review checklist

Reminder for reviewers

Signed-off-by: vince-fugnitto [email protected]
Co-authored-by: Akos Kitta [email protected]

The following commit fixes an issue with the listener to the
`editor.autoSave` preference update which would call `saveAll`
prematurely when the application is loaded.

The logic has been updated in order to only call `saveAll` when it is
appropriate (a true update to the preference value).

Signed-off-by: vince-fugnitto <[email protected]>
Co-authored-by: Akos Kitta <[email protected]>
@vince-fugnitto vince-fugnitto added bug bugs found in the application preferences issues related to preferences editor issues related to the editor labels Nov 6, 2020
@vince-fugnitto vince-fugnitto self-assigned this Nov 6, 2020
@kittaakos
Copy link
Contributor

Much appreciated that you've put together a PR with the fix, @vince-fugnitto. I think the root cause is that the default preference is off, but we ignore it. Ideally, we should get the default value if the oldValue is undefined. But it's not possible right now; the behavior is not in sync with the defaults.

I wouldn't rush with the merge; there is a workaround.

@vince-fugnitto
Copy link
Member Author

I wouldn't rush with the merge; there is a workaround.

Closing for the moment 👍

@vince-fugnitto vince-fugnitto deleted the vf/autoSave-fix branch October 7, 2021 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug bugs found in the application editor issues related to the editor preferences issues related to preferences
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Theia runs save on app startup if "editor.autoSave": "on" is configured in the settings.json
2 participants