diff --git a/vsintegration/src/FSharp.Editor/Options/EditorOptions.fs b/vsintegration/src/FSharp.Editor/Options/EditorOptions.fs index 14578e617c691fd8e4dba58698cc3bc33097d051..269ae085680c0701c934e3a31a07d705e98ac8f5 100644 --- a/vsintegration/src/FSharp.Editor/Options/EditorOptions.fs +++ b/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) [] diff --git a/vsintegration/src/FSharp.Editor/Options/SettingsPersistence.fs b/vsintegration/src/FSharp.Editor/Options/SettingsPersistence.fs index cc985a00585a8c380e081d910d2bff579226104c..d4e2c84dce60ae7d82c47aa0e3b4f790d3cf4966 100644 --- a/vsintegration/src/FSharp.Editor/Options/SettingsPersistence.fs +++ b/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 [] type SVsSettingsPersistenceManager = class end @@ -18,42 +18,69 @@ type SVsSettingsPersistenceManager = class end type SettingsStore(serviceProvider: IServiceProvider) = let settingsManager = serviceProvider.GetService(typeof) :?> 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() + // 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() - 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 [] + 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 diff --git a/vsintegration/src/FSharp.Editor/Options/UIHelpers.fs b/vsintegration/src/FSharp.Editor/Options/UIHelpers.fs index 1a7c8584133bdbf82cd908a482c1c905e75139ea..2adae7a7a46f33146e54ac6f62befcdfa7cfcc3d 100644 --- a/vsintegration/src/FSharp.Editor/Options/UIHelpers.fs +++ b/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 =