Visual Studio Editor Options bug fixes #5999
Conversation
| let storageKey (typ: Type) = typ.Namespace + "." + typ.Name | ||
|
|
||
| let storageKeyVersions (typ: Type) = | ||
| // "TextEditor" prefix seems to be required for settings changes to be synced between IDE instances |
There was a problem hiding this comment.
That's very good to know. Thank you for the comment on this.
There was a problem hiding this comment.
I should've probably added that it's a lucky guess from looking at Roslyn source 😄. I haven't found it documented anywhere in the open.
|
|
||
| member __.Read() = read() | ||
| // make a deep copy so that PopulateObject does not alter the original | ||
| let copy = clone settings |
There was a problem hiding this comment.
I feel like we should not have to do any deep cloning here. If the reason is because we are modifying existing immutable objects, we need to stop doing that altogether because that gives me goosebumps.
There was a problem hiding this comment.
If we can find a way to stop modifying immutable objects, we should never have to call JsonConvert.PopulateObject and get rid of the [<CLIMutable>] attribute on the options. I strongly suggest trying design something with that in mind.
There was a problem hiding this comment.
Yes, I absolutely agree both of these smell bad. In my defence, at the time I was trying to get something up and running quickly, that would be easy to use so we could have some way to configure things, hoping that in the long run we could tap into the Roslyn infrastructure, but this is still internal, I think.
The proper thing to do here would bo to declare, store and retrieve each and every configurable value separately, the way Roslyn does, that would let us get rid of the PopulateObject stuff. Technically the CLIMutable is not required when someone is willing to write the boilerplate to bind everything to the UI.
majocha
changed the title
|
@TIHan this PR is pretty minimal, bug fixes and to document the things as they are right now. I agree a more robust, fully immutable design would be nice to have at some point, but it's beyond the scope of this PR. |
| @@ -130,8 +130,8 @@ type EditorOptions | |||
| interface Microsoft.CodeAnalysis.Host.IWorkspaceService | |||
|
|
|||
| interface IPersistSettings with | |||
| member __.Read() = store.Read() | |||
| member __.Write(settings) = store.Write(settings) | |||
| member __.LoadSettings() = store.LoadSettings() | |||
|
It's not an immutable design honestly, in regards to I understand your view. I'm happy to take what you have since it resolves the bugs. |
|
Thanks @TIHan, I think I have some fresh idea how to deal with the issue in a unobtrusive way. |
* fix options ui logic and syncing between IDE instances * add comments * feedback
Settings changes now sync properly between two or more open Visual Studio instances. (Fixes #5997)
UI logic for saving and restoring options is more robust and follows what Roslyn does.
Fixes #5998
Some comments added.
edit:
Tested on another machine and the settings sync nicely both locally and between two PCs.