Visual Studio Editor Options bug fixes (#5999)

* fix options ui logic and syncing between IDE instances

* add comments

* feedback

vsintegration/src/FSharp.Editor/Options/EditorOptions.fs

@@ -119,19 +119,19 @@ type EditorOptions
         store.Register CodeLensOptions.Default
         store.Register FormattingOptions.Default
 
-    member __.IntelliSense : IntelliSenseOptions = store.Read()
-    member __.QuickInfo : QuickInfoOptions = store.Read()
-    member __.CodeFixes : CodeFixesOptions = store.Read()
-    member __.LanguageServicePerformance : LanguageServicePerformanceOptions = store.Read()
-    member __.Advanced: AdvancedOptions = store.Read()
-    member __.CodeLens: CodeLensOptions = store.Read()
-    member __.Formatting : FormattingOptions = store.Read()
+    member __.IntelliSense : IntelliSenseOptions = store.Get()
+    member __.QuickInfo : QuickInfoOptions = store.Get()
+    member __.CodeFixes : CodeFixesOptions = store.Get()
+    member __.LanguageServicePerformance : LanguageServicePerformanceOptions = store.Get()
+    member __.Advanced: AdvancedOptions = store.Get()
+    member __.CodeLens: CodeLensOptions = store.Get()
+    member __.Formatting : FormattingOptions = store.Get()
 
     interface Microsoft.CodeAnalysis.Host.IWorkspaceService
 
     interface IPersistSettings with
-        member __.Read() = store.Read()
-        member __.Write(settings) = store.Write(settings)
+        member __.LoadSettings() = store.LoadSettings()
+        member __.SaveSettings(settings) = store.SaveSettings(settings)
 
 
 [<AutoOpen>]

vsintegration/src/FSharp.Editor/Options/SettingsPersistence.fs

@@ -9,8 +9,8 @@ open Microsoft.VisualStudio.Settings
 open Newtonsoft.Json
 
 type IPersistSettings =
-    abstract member Read : unit -> 't
-    abstract member Write : 't -> unit
+    abstract member LoadSettings : unit -> 't
+    abstract member SaveSettings : 't -> unit
 
 [<Guid(Guids.svsSettingsPersistenceManagerIdString)>]
 type SVsSettingsPersistenceManager = class end
@@ -18,42 +18,69 @@ type SVsSettingsPersistenceManager = class end
 type SettingsStore(serviceProvider: IServiceProvider) =
 
     let settingsManager = serviceProvider.GetService(typeof<SVsSettingsPersistenceManager>) :?> ISettingsManager
-    // settings quallified type names are used as keys, this should be enough to avoid collisions
-    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
+        [ "TextEditor.FSharp." + typ.Namespace + "." + typ.Name
+          // we keep this old storage key to upgrade without reverting user changes
+          typ.Namespace + "." + typ.Name ]
+        
+    let storageKey (typ: Type) = storageKeyVersions typ |> List.head
 
     // Each group of settings is a value of some named type, for example 'IntelliSenseOptions', 'QuickInfoOptions'
-    // We cache exactly one instance of each, treating them as immutable.
-    // This cache is updated by the SettingsStore when the user changes an option.
-    let cache = System.Collections.Concurrent.ConcurrentDictionary<Type, obj>()
+    // and it is usually representing one separate option page in the UI.
+    // We cache exactly one immutable value of each type.
+    // This cache is updated by the SettingsStore when the user makes changes in the Options dialog
+    // or when a change is propagated from another VS IDE instance by SVsSettingsPersistenceManager. 
+    let cache = ConcurrentDictionary<Type, obj>()
 
-    let read() =
+    let getCached() =
         match cache.TryGetValue(typeof<'t>) with
-        | true, value -> value :?> 't
-        | _ -> failwithf "Settings %s are not registered." typeof<'t>.Name 
+        | true, (:? 't as value) -> value
+        | _ -> failwithf "Settings %s are not registered." typeof<'t>.Name
 
-    let write settings = cache.[settings.GetType()] <- settings
+    let keepInCache settings = cache.[settings.GetType()] <- settings
+
+    // The settings record, even though immutable, is being effectively mutated in two instances:
+    //   when it is passed to the UI (provided it is marked with CLIMutable attribute);
+    //   when it is being populated from JSON using JsonConvert.PopulateObject;
+    // We make a deep copy in these instances to isolate and contain the mutation
+    let clone (v: 't) = JsonConvert.SerializeObject v |> JsonConvert.DeserializeObject<'t>
 
     let updateFromStore settings =
-        let result, json = settings.GetType() |> storageKey |> settingsManager.TryGetValue
-        if result = GetValueResult.Success then
-            // if it fails we just return what we got
-            try JsonConvert.PopulateObject(json, settings) with _ -> () 
-        settings
-        
-    member __.Read() = read()
+        // make a deep copy so that PopulateObject does not alter the original
+        let copy = clone settings
+        // if the new key is not found by ISettingsManager, we try the old keys
+        // so that user settings are not lost
+        settings.GetType() |> storageKeyVersions 
+        |> Seq.map (settingsManager.TryGetValue)
+        |> Seq.tryPick ( function GetValueResult.Success, json -> Some json | _ -> None )
+        |> Option.iter (fun json -> try JsonConvert.PopulateObject(json, copy) with _ -> ())
+        copy
 
-    member __.Write settings =
-        write settings
-        // we replace default serialization with Newtonsoft.Json for easy schema evolution
+    member __.Get() = getCached()
+
+    // Used by the AbstractOptionPage to populate dialog controls.
+    // We always have the latest value in the cache so we just return
+    // cloned value here because it may be altered by the UI if declared with [<CLIMutable>]
+    member __.LoadSettings() = getCached() |> clone
+
+    member __.SaveSettings settings =
+        // We replace default serialization with Newtonsoft.Json for easy schema evolution.
+        // For example, if we add a new bool field to the record, representing another checkbox in Options dialog
+        // deserialization will still work fine. When we pass default value to JsonConvert.PopulateObject it will
+        // fill just the known fields.
         settingsManager.SetValueAsync(settings.GetType() |> storageKey, JsonConvert.SerializeObject settings, false)
-        |> Async.AwaitTask |> Async.StartImmediate   
+        |> Async.AwaitTask |> Async.Start
 
-    member __.Register (defaultSettings : 'options) =                       
-        defaultSettings |> updateFromStore |> write
+    // This is the point we retrieve the initial value and subscribe to watch for changes
+    member __.Register (defaultSettings : 'options) =
+        defaultSettings |> updateFromStore |> keepInCache
         let subset = defaultSettings.GetType() |> storageKey |> settingsManager.GetSubset
-
+        // this event is also raised when a setting change occurs in another VS instance, so we can keep everything in sync
         PropertyChangedAsyncEventHandler ( fun _ _ ->
-            (read() :'options) |> updateFromStore |> write
+            (getCached(): 'options) |> updateFromStore |> keepInCache
             System.Threading.Tasks.Task.CompletedTask )
         |> subset.add_SettingChangedAsync
+
          
\ No newline at end of file

vsintegration/src/FSharp.Editor/Options/UIHelpers.fs

@@ -13,8 +13,12 @@ module internal OptionsUIHelpers =
     type AbstractOptionPage<'options>() as this =
         inherit UIElementDialogPage()
 
+        // this replicates the logic used by Roslyn option pages. Some following comments have been copied from
+        // https://github.com/dotnet/roslyn/blob/5b125935f891b3c20405459f8f7e1cdfdc2cfa3d/src/VisualStudio/Core/Impl/Options/AbstractOptionPage.cs
+        let mutable needsLoadOnNextActivate = true
+
         let view = lazy this.CreateView()
-        
+
         let optionService =
             // lazy, so GetService is called from UI thread
             lazy
@@ -25,11 +29,33 @@ module internal OptionsUIHelpers =
 
         override this.Child = upcast view.Value
 
+        override this.OnActivate _ =
+            if needsLoadOnNextActivate then
+                // It looks like the bindings do not always pick up new source, unless we cycle the DataContext like this
+                view.Value.DataContext <- DependencyProperty.UnsetValue
+                view.Value.DataContext <- optionService.Value.LoadSettings<'options>()
+                needsLoadOnNextActivate <- false
+
         override this.SaveSettingsToStorage() = 
-            downcast view.Value.DataContext |> optionService.Value.Write<'options>
+            downcast view.Value.DataContext |> optionService.Value.SaveSettings<'options>
+            // Make sure we load the next time the page is activated, in case if options changed
+            // programmatically between now and the next time the page is activated
+            needsLoadOnNextActivate <- true
 
-        override this.LoadSettingsFromStorage() = 
-            view.Value.DataContext <- optionService.Value.Read<'options>()
+        override this.LoadSettingsFromStorage() =
+            // This gets called in two situations:
+            //
+            // 1) during the initial page load when you first activate the page, before OnActivate
+            //    is called.
+            // 2) during the closing of the dialog via Cancel/close when options don't need to be
+            //    saved. The intent here is the settings get reloaded so the next time you open the
+            //    page they are properly populated.
+            //
+            // This second one is tricky, because we don't actually want to update our controls
+            // right then, because they'd be wrong the next time the page opens -- it's possible
+            // they may have been changed programmatically. Therefore, we'll set a flag so we load
+            // next time         
+            needsLoadOnNextActivate <- true
 
     //data binding helpers
     let radioButtonCoverter =