-
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
Ensure preference files only in problems view if open #10562
Conversation
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.
The changes look really good to me 👍
The editor reference seems to be disposed as expected, removing the associated problems. As a consequence, the problems view isn't polluted with any configuration related issues.
I'll let others review as well.
packages/preferences/src/browser/preference-transaction-manager.ts
Outdated
Show resolved
Hide resolved
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.
The changes look good to me 👍
- confirmed the markers in the
settings.json
file only appear if the file is opened (similarly to vscode) so there is no noise on startup in the problems-view - confirmed that markers in the
settings.json
file are properly populated (ex: dangling commas, invalid preferences) - confirmed that updating preferences take effect like today
I noticed #10599 when testing but I confirmed that it also exists on master.
bc954df
to
44a5a5c
Compare
69b5f31
to
2c29751
Compare
@vince-fugnitto, I couldn't come up with a better solution to flakiness of these tests than just extending the timeout, and that seems to have gotten it through CI, at least. If you're OK with that, I'll write up breaking changes from this PR and merge it this week. |
2c29751
to
e97dc7d
Compare
@msujew, just want to confirm your (infomal) approval stands here? |
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.
@colin-grant-work Yep, still stands :)
I did a brief look over the changed code and performed another look at the App running with your changes. Everything is still looking good 👍
Please help I'm working through alot |
What it does
Fixes #7154 by disposing of the preference system's reference to the
MonacoEditorModel
when not in use. This is similar to VSCode's approach in which the model reference is disposed of at the end of the write.How to test
Review checklist
Reminder for reviewers