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.

Sign up for GitHub

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

Jump to bottom

Changes to our options, in Unified Settings at least, are delayed #10200

Open
davidwengier opened this issue Apr 2, 2024 · 3 comments
Open

Changes to our options, in Unified Settings at least, are delayed #10200

davidwengier opened this issue Apr 2, 2024 · 3 comments
Assignees
@ryzngard
Labels
bug Something isn't working
Milestone
Backlog

Comments

@davidwengier
Copy link
Contributor

Not sure if this is us, or a Unified Settings bug, but we're not handling changes correctly. @ryzngard do you know if this is expected to work yet?

d807d633-4167-4ac7-bb67-cca47dac7ade
@davidwengier davidwengier mentioned this issue Apr 2, 2024
Merged
@ryzngard
Copy link
Contributor

ryzngard commented Apr 2, 2024

hmm, I'm unsure. It's hard to tell exactly from the gif (without watching over and over again). I'll chat offline with you to understand the problem better.

I'm assuming if you go back to the traditional settings everything works as expected?

davidwengier added a commit that referenced this issue Apr 2, 2024
@davidwengier
Remove MS.Ext.Logging and Abstractions from most of our repo (#10199)
8c34fdd 
We don't get much from using MS.Ext.Logging for our logging, and we pay
for it with ~two~ three (?) DLL loads in VS. This fixes that by
implementing the logging bits outselves, and removing the abstraction
for options monitoring which we weren't really using.

I also removed the notion of "scopes" from the logging because we never
actually implemented them, so this also closes
#8232. We don't actually do any
structured logging so we're really not missing much.

The only place that does still use MS.Ext.Logging is our one DLL that is
referenced by Roslyn in VS Code, since they pass us an ILogger, so we
don't really have a choice about that bit :)

I still need to do a little more testing of the options bit, ~because
I'm seeing some oddities, but I think the issue is really one with the
unified settings experience and/or our options updating code.~ Okay
yeah, this is happening in VS main too. Logged
#10200 but will make sure this
doesn't get any worse :)
@davidwengier
Copy link
Contributor Author

Debugged through this. When a setting changes in Unified settings, we get a call to OnUnifiedSettingsChanged which tells us which setting has changed, but when we fire events etc the Get methods call into _writableSettingsStore but it seems like that doesn't have the new value yet. Suspect this is either a bug in the Unified settings system, or we're supposed to be checking something different, but I don't think there is anything we can actually do explicitly to fix it. The event we get doesn't have the value of the setting, only the name, so we can't force it into the store at that point.

@ryzngard
Copy link
Contributor

ryzngard commented Apr 8, 2024

Yea, the only thing I can think of is that either it's a unified settings bug or our mapping for the setting is off. If it's eventually updated, I would say it's the former. The whole point in us providing a mapping is so that they know what "traditional" location to update.

There could also be changes in how we're supposed to be doing things. I can reach out to the team this week.

@ryzngard ryzngard added this to the Backlog milestone Apr 11, 2024
@ryzngard ryzngard added the bug Something isn't working label Apr 11, 2024
@ryzngard ryzngard self-assigned this Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ryzngard ryzngard

Labels
bug Something isn't working
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
2 participants
@ryzngard @davidwengier