From ae390a7dd71969277dd5d2f78d582f281243e47d Mon Sep 17 00:00:00 2001 From: Joey Robichaud Date: Mon, 18 Mar 2019 10:25:53 -0700 Subject: [PATCH 1/2] Fix saving naming styles options page - Allow page controls to store settings to option store before updating service - Reload page control settings after a cancel or save --- src/Tools/ILAsm/IlAsmDeploy.csproj | 3 -- .../Core/Impl/Options/AbstractOptionPage.cs | 34 +++++++++++++++---- .../Impl/Options/AbstractOptionPageControl.cs | 10 +----- .../NamingStyleOptionPageControl.xaml.cs | 5 +-- 4 files changed, 30 insertions(+), 22 deletions(-) diff --git a/src/Tools/ILAsm/IlAsmDeploy.csproj b/src/Tools/ILAsm/IlAsmDeploy.csproj index bedf1092119a8..2cfd9d80a6b25 100644 --- a/src/Tools/ILAsm/IlAsmDeploy.csproj +++ b/src/Tools/ILAsm/IlAsmDeploy.csproj @@ -12,9 +12,6 @@ false true - - true - $(ArtifactsDir)tools\ILAsm\$(Configuration)\ diff --git a/src/VisualStudio/Core/Impl/Options/AbstractOptionPage.cs b/src/VisualStudio/Core/Impl/Options/AbstractOptionPage.cs index 0ad4df41ec501..a5b11694c33eb 100644 --- a/src/VisualStudio/Core/Impl/Options/AbstractOptionPage.cs +++ b/src/VisualStudio/Core/Impl/Options/AbstractOptionPage.cs @@ -12,7 +12,9 @@ internal abstract class AbstractOptionPage : UIElementDialogPage { private static IOptionService s_optionService; private static OptionStore s_optionStore; - private static bool s_needsLoadOnNextActivate = true; + private static bool s_needsToUpdateOptionStore = true; + + private bool _needsLoadOnNextActivate = true; protected abstract AbstractOptionPageControl CreateOptionPage(IServiceProvider serviceProvider, OptionStore optionStore); @@ -50,16 +52,22 @@ protected override void OnActivate(System.ComponentModel.CancelEventArgs e) { EnsureOptionPageCreated(); - if (s_needsLoadOnNextActivate) + if (s_needsToUpdateOptionStore) { // Reset the option store to the current state of the options. s_optionStore.SetOptions(s_optionService.GetOptions()); s_optionStore.SetRegisteredOptions(s_optionService.GetRegisteredOptions()); - s_needsLoadOnNextActivate = false; + s_needsToUpdateOptionStore = false; } - pageControl.LoadSettings(); + if (_needsLoadOnNextActivate) + { + // For pages that don't use option bindings we need to load setting changes. + pageControl.LoadSettings(); + + _needsLoadOnNextActivate = false; + } } public override void LoadSettingsFromStorage() @@ -81,19 +89,33 @@ public override void LoadSettingsFromStorage() // they may have been changed programmatically. Therefore, we'll set a flag so we load // next time - s_needsLoadOnNextActivate = pageControl != null; + // If we are being canceled after the pageControl has already been created then + // we need to reset the option store on next load. + s_needsToUpdateOptionStore = pageControl != null; + + // For pages that don't use option bindings we need to load settings when it is + // activated next. + _needsLoadOnNextActivate = true; } public override void SaveSettingsToStorage() { EnsureOptionPageCreated(); + // Allow page controls to perist their settings to the option store before updating the + // option service. + pageControl.SaveSettings(); + // Save the changes that were accumulated in the option store. s_optionService.SetOptions(s_optionStore.GetOptions()); // Make sure we load the next time a page is activated, in case that options changed // programmatically between now and the next time the page is activated - s_needsLoadOnNextActivate = true; + s_needsToUpdateOptionStore = true; + + // For pages that don't use option bindings we need to load settings when it is + // activated next. + _needsLoadOnNextActivate = true; } protected override void OnClosed(EventArgs e) diff --git a/src/VisualStudio/Core/Impl/Options/AbstractOptionPageControl.cs b/src/VisualStudio/Core/Impl/Options/AbstractOptionPageControl.cs index 335e2177c4daf..1f4df44e926c7 100644 --- a/src/VisualStudio/Core/Impl/Options/AbstractOptionPageControl.cs +++ b/src/VisualStudio/Core/Impl/Options/AbstractOptionPageControl.cs @@ -158,15 +158,7 @@ internal virtual void LoadSettings() internal virtual void SaveSettings() { - foreach (var bindingExpression in _bindingExpressions) - { - if (!bindingExpression.IsDirty) - { - continue; - } - - bindingExpression.UpdateSource(); - } + } internal virtual void Close() diff --git a/src/VisualStudio/Core/Impl/Options/Style/NamingPreferences/NamingStyleOptionPageControl.xaml.cs b/src/VisualStudio/Core/Impl/Options/Style/NamingPreferences/NamingStyleOptionPageControl.xaml.cs index d01909223ce30..446b35f1006b5 100644 --- a/src/VisualStudio/Core/Impl/Options/Style/NamingPreferences/NamingStyleOptionPageControl.xaml.cs +++ b/src/VisualStudio/Core/Impl/Options/Style/NamingPreferences/NamingStyleOptionPageControl.xaml.cs @@ -177,10 +177,7 @@ internal override void SaveSettings() namingStyles.ToImmutableAndFree(), namingRules.ToImmutableAndFree()); - var oldOptions = OptionStore.GetOptions(); - var newOptions = oldOptions.WithChangedOption(SimplificationOptions.NamingPreferences, _languageName, info); - OptionStore.SetOptions(newOptions); - OptionLogger.Log(oldOptions, newOptions); + OptionStore.SetOption(SimplificationOptions.NamingPreferences, _languageName, info); } internal override void LoadSettings() From 9b86bf74354e205190a2e3bf0704eb6990cb2429 Mon Sep 17 00:00:00 2001 From: Joey Robichaud Date: Mon, 18 Mar 2019 17:08:39 -0700 Subject: [PATCH 2/2] Renamed AbstractOptionPageControl methods to be more clear --- .../Core/Impl/Options/AbstractOptionPage.cs | 13 ++++++++----- .../Core/Impl/Options/AbstractOptionPageControl.cs | 5 ++--- .../Impl/Options/GridOptionPreviewControl.xaml.cs | 2 +- .../Core/Impl/Options/OptionPreviewControl.xaml.cs | 2 +- src/VisualStudio/Core/Impl/Options/OptionStore.cs | 10 +++------- .../NamingStyleOptionPageControl.xaml.cs | 8 ++++---- 6 files changed, 19 insertions(+), 21 deletions(-) diff --git a/src/VisualStudio/Core/Impl/Options/AbstractOptionPage.cs b/src/VisualStudio/Core/Impl/Options/AbstractOptionPage.cs index a5b11694c33eb..d7d07ce5141b7 100644 --- a/src/VisualStudio/Core/Impl/Options/AbstractOptionPage.cs +++ b/src/VisualStudio/Core/Impl/Options/AbstractOptionPage.cs @@ -64,7 +64,7 @@ protected override void OnActivate(System.ComponentModel.CancelEventArgs e) if (_needsLoadOnNextActivate) { // For pages that don't use option bindings we need to load setting changes. - pageControl.LoadSettings(); + pageControl.OnLoad(); _needsLoadOnNextActivate = false; } @@ -89,8 +89,8 @@ public override void LoadSettingsFromStorage() // they may have been changed programmatically. Therefore, we'll set a flag so we load // next time - // If we are being canceled after the pageControl has already been created then - // we need to reset the option store on next load. + // When pageControl is null we know that Activation has not happened for this page. + // We only need to update the OptionStore after a cancel or close click (2). s_needsToUpdateOptionStore = pageControl != null; // For pages that don't use option bindings we need to load settings when it is @@ -104,10 +104,13 @@ public override void SaveSettingsToStorage() // Allow page controls to perist their settings to the option store before updating the // option service. - pageControl.SaveSettings(); + pageControl.OnSave(); // Save the changes that were accumulated in the option store. - s_optionService.SetOptions(s_optionStore.GetOptions()); + var oldOptions = s_optionService.GetOptions(); + var newOptions = s_optionStore.GetOptions(); + s_optionService.SetOptions(newOptions); + OptionLogger.Log(oldOptions, newOptions); // Make sure we load the next time a page is activated, in case that options changed // programmatically between now and the next time the page is activated diff --git a/src/VisualStudio/Core/Impl/Options/AbstractOptionPageControl.cs b/src/VisualStudio/Core/Impl/Options/AbstractOptionPageControl.cs index 1f4df44e926c7..10acaaf458955 100644 --- a/src/VisualStudio/Core/Impl/Options/AbstractOptionPageControl.cs +++ b/src/VisualStudio/Core/Impl/Options/AbstractOptionPageControl.cs @@ -148,7 +148,7 @@ protected void BindToFullSolutionAnalysisOption(CheckBox checkbox, string langua _bindingExpressions.Add(bindingExpression); } - internal virtual void LoadSettings() + internal virtual void OnLoad() { foreach (var bindingExpression in _bindingExpressions) { @@ -156,9 +156,8 @@ internal virtual void LoadSettings() } } - internal virtual void SaveSettings() + internal virtual void OnSave() { - } internal virtual void Close() diff --git a/src/VisualStudio/Core/Impl/Options/GridOptionPreviewControl.xaml.cs b/src/VisualStudio/Core/Impl/Options/GridOptionPreviewControl.xaml.cs index 1037a7fdb857e..a808e26179ade 100644 --- a/src/VisualStudio/Core/Impl/Options/GridOptionPreviewControl.xaml.cs +++ b/src/VisualStudio/Core/Impl/Options/GridOptionPreviewControl.xaml.cs @@ -86,7 +86,7 @@ private void Options_PreviewKeyDown(object sender, KeyEventArgs e) } } - internal override void LoadSettings() + internal override void OnLoad() { this.ViewModel = _createViewModel(OptionStore, _serviceProvider); diff --git a/src/VisualStudio/Core/Impl/Options/OptionPreviewControl.xaml.cs b/src/VisualStudio/Core/Impl/Options/OptionPreviewControl.xaml.cs index 09f4799e7e11b..8eb0ab00181a5 100644 --- a/src/VisualStudio/Core/Impl/Options/OptionPreviewControl.xaml.cs +++ b/src/VisualStudio/Core/Impl/Options/OptionPreviewControl.xaml.cs @@ -87,7 +87,7 @@ private void Options_PreviewKeyDown(object sender, KeyEventArgs e) } } - internal override void LoadSettings() + internal override void OnLoad() { this.ViewModel = _createViewModel(this.OptionStore, _serviceProvider); diff --git a/src/VisualStudio/Core/Impl/Options/OptionStore.cs b/src/VisualStudio/Core/Impl/Options/OptionStore.cs index a98b4d68bf985..c0655a25e4dd9 100644 --- a/src/VisualStudio/Core/Impl/Options/OptionStore.cs +++ b/src/VisualStudio/Core/Impl/Options/OptionStore.cs @@ -1,4 +1,6 @@ -using System; +// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System; using System.Collections.Generic; using Microsoft.CodeAnalysis.Options; @@ -28,27 +30,21 @@ public OptionStore(OptionSet optionSet, IEnumerable registeredOptions) public void SetOption(OptionKey optionKey, object value) { - var oldOptions = _optionSet; _optionSet = _optionSet.WithChangedOption(optionKey, value); - OptionLogger.Log(oldOptions, _optionSet); OnOptionChanged(optionKey); } public void SetOption(Option option, T value) { - var oldOptions = _optionSet; _optionSet = _optionSet.WithChangedOption(option, value); - OptionLogger.Log(oldOptions, _optionSet); OnOptionChanged(new OptionKey(option)); } public void SetOption(PerLanguageOption option, string language, T value) { - var oldOptionSet = _optionSet; _optionSet = _optionSet.WithChangedOption(option, language, value); - OptionLogger.Log(oldOptionSet, _optionSet); OnOptionChanged(new OptionKey(option, language)); } diff --git a/src/VisualStudio/Core/Impl/Options/Style/NamingPreferences/NamingStyleOptionPageControl.xaml.cs b/src/VisualStudio/Core/Impl/Options/Style/NamingPreferences/NamingStyleOptionPageControl.xaml.cs index 446b35f1006b5..a536b215f83f1 100644 --- a/src/VisualStudio/Core/Impl/Options/Style/NamingPreferences/NamingStyleOptionPageControl.xaml.cs +++ b/src/VisualStudio/Core/Impl/Options/Style/NamingPreferences/NamingStyleOptionPageControl.xaml.cs @@ -50,7 +50,7 @@ internal NamingStyleOptionPageControl(OptionStore optionStore, INotificationServ _notificationService = notificationService; InitializeComponent(); - LoadSettings(); + OnLoad(); } private NamingStyleOptionPageViewModel.NamingRuleViewModel CreateItemWithNoSelections() @@ -139,7 +139,7 @@ private void SetFocusToSelectedRow() } } - internal override void SaveSettings() + internal override void OnSave() { var symbolSpecifications = ArrayBuilder.GetInstance(); var namingRules = ArrayBuilder.GetInstance(); @@ -180,9 +180,9 @@ internal override void SaveSettings() OptionStore.SetOption(SimplificationOptions.NamingPreferences, _languageName, info); } - internal override void LoadSettings() + internal override void OnLoad() { - base.LoadSettings(); + base.OnLoad(); var preferences = OptionStore.GetOption(SimplificationOptions.NamingPreferences, _languageName); if (preferences == null)