-
Notifications
You must be signed in to change notification settings - Fork 30.2k
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
Improve migrating old editorAssociations setting format #126577
Conversation
- respect old value while reading
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.
👍 Nice!
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.
Minor nit: if the this.hostService.hadLastFocus()
would move into convertOldAssociationFormat
, we could also protect from the following case:
- user has 10 windows opened
- user manually changes settings in a way that we start the migration
- all 10 windows start to migrate the setting at the same time
I'm not sure what the difference between last focus and focus is. Here is what I thought the change would look like.
That way once any window gets focused we would convert the format. I would argue that since we convert so aggressively we should either call convertOldAssociationFormat() in the read if we somehow still read an old setting. We are duplicating the logic over into the read to be the same as update but if we always update on focus then it should never be the old setting. |
We simply mimic what other code was already doing, like release notes that wants to open them only in the active window but not all windows, so we have a bit of confidence that that works:
|
This PR fixes #125970