Conversation
|
@CyrusNajmabadi PTAL |
jasonmalinowski
left a comment
There was a problem hiding this comment.
Overall I love getting services off from using the workspace just to grab global stuff, so nice. I wouldn't mind though if @sharwell at least took at look at the GlobalOptionService.cs changes; there's one or two comments that implied some ordering was important there, and I can't say I understood why that was there originally so I hesitate to say I can really sign off on it.
src/VisualStudio/Core/Def/Implementation/KeybindingReset/KeybindingResetDetector.cs
Show resolved
Hide resolved
| // We need to switch to UI thread to invoke TryApplyChanges. | ||
| await ThreadingContext.JoinableTaskFactory.SwitchToMainThreadAsync(cancellationToken); | ||
| _workspace.TryApplyChanges(_workspace.CurrentSolution.WithOptions(options)); | ||
| _optionService.SetGlobalOptions(s_statusOptions, ImmutableArray.Create<object>(currentStatus, needsReset)); |
There was a problem hiding this comment.
Do we really need to be setting both of these at once? I can't imagine there's a performance problem but the ease in which we could reorder something here and break it is worrisome to me.
There was a problem hiding this comment.
I don't know if we really need to, but it seems reasonable since the code implements a state machine that transitions between state that's defined by both values.
There was a problem hiding this comment.
I'm not sure what you mean by reordering something and breaking it.
| /// Gets the current values of specified options. | ||
| /// All options are read atomically. | ||
| /// </summary> | ||
| ImmutableArray<object?> GetOptions(ImmutableArray<OptionKey> optionKeys); |
There was a problem hiding this comment.
Do we really need this? The lack of typesafety here really worries me.
There was a problem hiding this comment.
This is not less type safe then GetOption<T> - using the latter API the caller passes arbitrary type as T and we blindly cast object to it. It's just hidden behind the call.
There was a problem hiding this comment.
| // Ensure the option persisters are available before taking the global lock | ||
| var persisters = GetOptionPersisters(); |
There was a problem hiding this comment.
Was the ordering significant here? Did we have persisters updating the global state as a part of load?
There was a problem hiding this comment.
Persisters are called by GetOption if the option hasn't been loaded yet. They are not updating anything anything just fetching value from the underlying storage. GetOption does update the dictionary of current values if it fetches value from a persister that wasn't fetch before. All that should be independent of persisting the new option value.
3f29c3a to
5946b43
Compare
Separate global client options from solution options - see #55728.
KeybindingResetDetector refactoring: Remove dependency on (VisualStudio)Workspace, remove IExperiment.