-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Refactor auto save mechanism #13683
Refactor auto save mechanism #13683
Conversation
55e4531
to
e7da65a
Compare
e7da65a
to
9f05af6
Compare
9f05af6
to
94c6552
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First reading of the code.
@@ -364,7 +366,7 @@ export class MonacoEditorModel implements IResolvedTextEditorModel, TextEditorDo | |||
} | |||
|
|||
save(options?: SaveOptions): Promise<void> { | |||
return this.scheduleSave(TextDocumentSaveReason.Manual, undefined, undefined, options); | |||
return this.scheduleSave(options?.saveReason ?? TextDocumentSaveReason.Manual, undefined, undefined, options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not quite sure why we differentiate the different "save" operations by reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the vscode API does.
packages/filesystem/src/browser/filesystem-save-resource-service.ts
Outdated
Show resolved
Hide resolved
454a534
to
14b673a
Compare
I've found a weird little bug:
Works as expected.
Note that I can have the settings open and observe the "autosave" setting change to "off" when I uncheck the menu item, but I only get the "save"-dialog back once I manually edit the setting in the settings editor. |
From the linked issue :
😎 🤣 |
What it does
Closes #12863
Closes #12844
Closes #8722
Enhances the
SaveResourceService
to perform all the auto save preference work. Previously, all editor classes had to do their own handling of theafterDelay
auto save functionality. This has been centralized into one service. Therefore, custom editors and notebook editors now behave exactly the same as monaco text editors in regards to auto saves.This also fixes a long standing issue, where
onFocusChange
andonWindowChange
have been handled as ifafterDelay
was selected as the auto save mode. These two values are now properly handled.Note that
onWindowChange
doesn't work well with custom editors. This is due to the usage ofiframe
and the inability to look into an iframe. It still works, but is a bit too eager with them.How to test
Basically, confirm that the auto save functionality behaves as expected with all different values of the enum preference. This should be tested for all 3 kinds of editors (text, notebook, custom editor).
Also test that closing unsaved editors prompts (or automatically saves) the user whether or not to save those files.
Follow-ups
We might want to find a way to fix
onWindowChange
for custom editors as well based on mouse position event information.Review checklist
Reminder for reviewers