Always refresh after Package Source changes in VS Options (#7092)

donnie-msft · web-flow · commit 32272e9dcc41 · 2026-02-03T15:51:59.000-08:00
diff --git a/src/NuGet.Clients/NuGet.PackageManagement.VisualStudio/Options/PackageSourcesPage.cs b/src/NuGet.Clients/NuGet.PackageManagement.VisualStudio/Options/PackageSourcesPage.cs
@@ -106,7 +106,7 @@ public override async Task<ExternalSettingOperationResult<T>> GetValueAsync<T>(s
 
         public override async Task<ExternalSettingOperationResult> SetValueAsync<T>(string moniker, T value, CancellationToken cancellationToken)
         {
-            bool hasAnyHiddenPropertyChanged = false;
+            bool isRefreshNeeded = false;
 
             try
             {
@@ -122,19 +122,18 @@ public override async Task<ExternalSettingOperationResult> SetValueAsync<T>(stri
                         return await Task.Run(
                             () =>
                             {
-                                (ExternalSettingOperationResult result, bool hasAnyHiddenPropertyChanged) savePackageSourcesResult = SavePackageSources(packageSourcesList, cancellationToken);
-                                hasAnyHiddenPropertyChanged = savePackageSourcesResult.hasAnyHiddenPropertyChanged;
-                                return savePackageSourcesResult.result;
+                                ExternalSettingOperationResult savePackageSourcesResult = SavePackageSources(packageSourcesList, cancellationToken);
+                                isRefreshNeeded = savePackageSourcesResult == ExternalSettingOperationResult.Success.Instance;
+                                return savePackageSourcesResult;
                             },
                             cancellationToken);
                     case MonikerAuditSources:
                         var auditSourceList = (IReadOnlyList<IDictionary<string, object>>)value;
                         return await Task.Run(
                             () =>
                             {
-                                (ExternalSettingOperationResult result, bool hasAnyHiddenPropertyChanged) saveAuditSourcesResult = SaveAuditSources(auditSourceList, cancellationToken);
-                                hasAnyHiddenPropertyChanged = saveAuditSourcesResult.hasAnyHiddenPropertyChanged;
-                                return saveAuditSourcesResult.result;
+                                ExternalSettingOperationResult saveAuditSourcesResult = SaveAuditSources(auditSourceList, cancellationToken);
+                                return saveAuditSourcesResult;
                             },
                             cancellationToken);
                     case MonikerMachineWideSources:
@@ -152,7 +151,7 @@ public override async Task<ExternalSettingOperationResult> SetValueAsync<T>(stri
                 // Resume listening to setting changes after saving.
                 _suppressSettingValuesChanged = false;
 
-                if (hasAnyHiddenPropertyChanged)
+                if (isRefreshNeeded)
                 {
                     VsSettings_SettingsChanged(this, EventArgs.Empty);
                 }
@@ -206,18 +205,16 @@ private ExternalSettingOperationResult SetIsEnabledOnMachineWidePackageSources(
             return result;
         }
 
-        private (ExternalSettingOperationResult result, bool hasAnyHiddenPropertyChanged) SavePackageSources(
+        private ExternalSettingOperationResult SavePackageSources(
             IReadOnlyList<IDictionary<string, object>> packageSourceDictionaryList,
             CancellationToken cancellationToken)
         {
-            bool hasAnyHiddenPropertyChanged = false;
             ExternalSettingOperationResult result;
 
             try
             {
                 List<PackageSource> packageSources = new List<PackageSource>(capacity: packageSourceDictionaryList.Count);
                 IReadOnlyList<PackageSource> existingPackageSources = LoadPackageSources(isMachineWide: false);
-                bool hasAnyPackageSourceNameChanged = false;
 
                 foreach (Dictionary<string, object> packageSourceDictionary in packageSourceDictionaryList)
                 {
@@ -230,12 +227,6 @@ private ExternalSettingOperationResult SetIsEnabledOnMachineWidePackageSources(
                     if (packageSourceDictionary.TryGetValue(MonikerPackageSourceId, out object packageSourceIdObj))
                     {
                         lookupName = packageSourceIdObj.ToString();
-
-                        if (!string.Equals(lookupName, name, StringComparison.CurrentCultureIgnoreCase))
-                        {
-                            // Changing the ID needs to refresh Unified Settings since the ID is a hidden property.
-                            hasAnyPackageSourceNameChanged = true;
-                        }
                     }
                     else // Newly added Package Sources will not have an ID yet.
                     {
@@ -260,8 +251,6 @@ private ExternalSettingOperationResult SetIsEnabledOnMachineWidePackageSources(
 
                 _packageSourceProvider.SavePackageSources(packageSources);
 
-                hasAnyHiddenPropertyChanged = hasAnyPackageSourceNameChanged;
-
                 result = ExternalSettingOperationResult.Success.Instance;
             }
 #pragma warning disable CA1031 // Do not catch general exception types
@@ -272,21 +261,19 @@ private ExternalSettingOperationResult SetIsEnabledOnMachineWidePackageSources(
                 ActivityLog.LogError(ExceptionHelper.LogEntrySource, ex.ToString());
             }
 
-            return (result, hasAnyHiddenPropertyChanged);
+            return result;
         }
 
-        private (ExternalSettingOperationResult result, bool hasAnyHiddenPropertyChanged) SaveAuditSources(
+        private ExternalSettingOperationResult SaveAuditSources(
             IReadOnlyList<IDictionary<string, object>> auditSourceDictionaryList,
             CancellationToken cancellationToken)
         {
-            bool hasAnyHiddenPropertyChanged = false;
             ExternalSettingOperationResult result;
 
             try
             {
                 List<PackageSource> auditSources = new List<PackageSource>(capacity: auditSourceDictionaryList.Count);
                 IReadOnlyList<PackageSource> existingAuditSources = LoadAuditSources();
-                bool hasAnyPackageSourceNameChanged = false;
 
                 foreach (Dictionary<string, object> packageSourceDictionary in auditSourceDictionaryList)
                 {
@@ -299,12 +286,6 @@ private ExternalSettingOperationResult SetIsEnabledOnMachineWidePackageSources(
                     if (packageSourceDictionary.TryGetValue(MonikerPackageSourceId, out object packageSourceIdObj))
                     {
                         lookupName = packageSourceIdObj.ToString();
-
-                        if (!string.Equals(lookupName, name, StringComparison.CurrentCultureIgnoreCase))
-                        {
-                            // Changing the ID needs to refresh Unified Settings since the ID is a hidden property.
-                            hasAnyPackageSourceNameChanged = true;
-                        }
                     }
                     else // Newly added Package Sources will not have an ID yet.
                     {
@@ -327,8 +308,6 @@ private ExternalSettingOperationResult SetIsEnabledOnMachineWidePackageSources(
 
                 _packageSourceProvider.SaveAuditSources(auditSources);
 
-                hasAnyHiddenPropertyChanged = hasAnyPackageSourceNameChanged;
-
                 result = ExternalSettingOperationResult.Success.Instance;
             }
 #pragma warning disable CA1031 // Do not catch general exception types
@@ -339,7 +318,7 @@ private ExternalSettingOperationResult SetIsEnabledOnMachineWidePackageSources(
                 ActivityLog.LogError(ExceptionHelper.LogEntrySource, ex.ToString());
             }
 
-            return (result, hasAnyHiddenPropertyChanged);
+            return result;
         }
 
 
diff --git a/test/NuGet.Clients.Tests/NuGet.PackageManagement.VisualStudio.Test/Options/PackageSourcesPageTests.cs b/test/NuGet.Clients.Tests/NuGet.PackageManagement.VisualStudio.Test/Options/PackageSourcesPageTests.cs
@@ -71,7 +71,7 @@ protected override PackageSourcesPage CreateInstance(VSSettings? vsSettings)
         [Theory]
         [InlineData(true)]
         [InlineData(false)]
-        public async Task SetValueAsync_AuditSources_WhenNameChanging_RaisesSettingValuesChangedAsync(bool isNameChanging)
+        public async Task SetValueAsync_AuditSources_Never_RaisesSettingValuesChangedAsync(bool isNameChanging)
         {
             // Arrange
             string originalSourceName = "auditTestingSourceName";
@@ -224,6 +224,9 @@ public async Task GetValueAsync_WhenNuGetConfigHasInvalidSource_ReturnsFailureRe
             failure.ErrorMessage.Should().StartWith(Strings.Error_NuGetConfig_InvalidState);
         }
 
+        /// <summary>
+        /// Any edit will cause the event to be raised. Here we test changing the source's name followed by the source's URL.
+        /// </summary>
         [Theory]
         [InlineData(true)]
         [InlineData(false)]
@@ -273,7 +276,7 @@ public async Task SetValueAsync_WhenPackageNameChanging_RaisesSettingValuesChang
             // Assert
             result.Should().NotBeNull();
             result.Should().BeOfType<ExternalSettingOperationResult.Success>();
-            wasVsSettingsSettingsChangedCalled.Should().Be(isPackageNameChanging);
+            wasVsSettingsSettingsChangedCalled.Should().Be(true);
         }
 
         [Theory]

Original file line number	Diff line number	Diff line change
`@@ -106,7 +106,7 @@ public override async Task<ExternalSettingOperationResult<T>> GetValueAsync<T>(s`
`106`	`106`
`107`	`107`	`public override async Task<ExternalSettingOperationResult> SetValueAsync<T>(string moniker, T value, CancellationToken cancellationToken)`
`108`	`108`	`{`
`109`		`- bool hasAnyHiddenPropertyChanged = false;`
	`109`	`+ bool isRefreshNeeded = false;`
`110`	`110`
`111`	`111`	`try`
`112`	`112`	`{`
`@@ -122,19 +122,18 @@ public override async Task<ExternalSettingOperationResult> SetValueAsync<T>(stri`
`122`	`122`	`return await Task.Run(`
`123`	`123`	`() =>`
`124`	`124`	`{`
`125`		`- (ExternalSettingOperationResult result, bool hasAnyHiddenPropertyChanged) savePackageSourcesResult = SavePackageSources(packageSourcesList, cancellationToken);`
`126`		`- hasAnyHiddenPropertyChanged = savePackageSourcesResult.hasAnyHiddenPropertyChanged;`
`127`		`- return savePackageSourcesResult.result;`
	`125`	`+ ExternalSettingOperationResult savePackageSourcesResult = SavePackageSources(packageSourcesList, cancellationToken);`
	`126`	`+ isRefreshNeeded = savePackageSourcesResult == ExternalSettingOperationResult.Success.Instance;`
	`127`	`+ return savePackageSourcesResult;`
`128`	`128`	`},`
`129`	`129`	`cancellationToken);`
`130`	`130`	`case MonikerAuditSources:`
`131`	`131`	`var auditSourceList = (IReadOnlyList<IDictionary<string, object>>)value;`
`132`	`132`	`return await Task.Run(`
`133`	`133`	`() =>`
`134`	`134`	`{`
`135`		`- (ExternalSettingOperationResult result, bool hasAnyHiddenPropertyChanged) saveAuditSourcesResult = SaveAuditSources(auditSourceList, cancellationToken);`
`136`		`- hasAnyHiddenPropertyChanged = saveAuditSourcesResult.hasAnyHiddenPropertyChanged;`
`137`		`- return saveAuditSourcesResult.result;`
	`135`	`+ ExternalSettingOperationResult saveAuditSourcesResult = SaveAuditSources(auditSourceList, cancellationToken);`
	`136`	`+ return saveAuditSourcesResult;`
`138`	`137`	`},`
`139`	`138`	`cancellationToken);`
`140`	`139`	`case MonikerMachineWideSources:`
`@@ -152,7 +151,7 @@ public override async Task<ExternalSettingOperationResult> SetValueAsync<T>(stri`
`152`	`151`	`// Resume listening to setting changes after saving.`
`153`	`152`	`_suppressSettingValuesChanged = false;`
`154`	`153`
`155`		`- if (hasAnyHiddenPropertyChanged)`
	`154`	`+ if (isRefreshNeeded)`
`156`	`155`	`{`
`157`	`156`	`VsSettings_SettingsChanged(this, EventArgs.Empty);`
`158`	`157`	`}`
`@@ -206,18 +205,16 @@ private ExternalSettingOperationResult SetIsEnabledOnMachineWidePackageSources(`
`206`	`205`	`return result;`
`207`	`206`	`}`
`208`	`207`
`209`		`- private (ExternalSettingOperationResult result, bool hasAnyHiddenPropertyChanged) SavePackageSources(`
	`208`	`+ private ExternalSettingOperationResult SavePackageSources(`
`210`	`209`	`IReadOnlyList<IDictionary<string, object>> packageSourceDictionaryList,`
`211`	`210`	`CancellationToken cancellationToken)`
`212`	`211`	`{`
`213`		`- bool hasAnyHiddenPropertyChanged = false;`
`214`	`212`	`ExternalSettingOperationResult result;`
`215`	`213`
`216`	`214`	`try`
`217`	`215`	`{`
`218`	`216`	`List<PackageSource> packageSources = new List<PackageSource>(capacity: packageSourceDictionaryList.Count);`
`219`	`217`	`IReadOnlyList<PackageSource> existingPackageSources = LoadPackageSources(isMachineWide: false);`
`220`		`- bool hasAnyPackageSourceNameChanged = false;`
`221`	`218`
`222`	`219`	`foreach (Dictionary<string, object> packageSourceDictionary in packageSourceDictionaryList)`
`223`	`220`	`{`
`@@ -230,12 +227,6 @@ private ExternalSettingOperationResult SetIsEnabledOnMachineWidePackageSources(`
`230`	`227`	`if (packageSourceDictionary.TryGetValue(MonikerPackageSourceId, out object packageSourceIdObj))`
`231`	`228`	`{`
`232`	`229`	`lookupName = packageSourceIdObj.ToString();`
`233`		`-`
`234`		`- if (!string.Equals(lookupName, name, StringComparison.CurrentCultureIgnoreCase))`
`235`		`- {`
`236`		`- // Changing the ID needs to refresh Unified Settings since the ID is a hidden property.`
`237`		`- hasAnyPackageSourceNameChanged = true;`
`238`		`- }`
`239`	`230`	`}`
`240`	`231`	`else // Newly added Package Sources will not have an ID yet.`
`241`	`232`	`{`
`@@ -260,8 +251,6 @@ private ExternalSettingOperationResult SetIsEnabledOnMachineWidePackageSources(`
`260`	`251`
`261`	`252`	`_packageSourceProvider.SavePackageSources(packageSources);`
`262`	`253`
`263`		`- hasAnyHiddenPropertyChanged = hasAnyPackageSourceNameChanged;`
`264`		`-`
`265`	`254`	`result = ExternalSettingOperationResult.Success.Instance;`
`266`	`255`	`}`
`267`	`256`	`#pragma warning disable CA1031 // Do not catch general exception types`
`@@ -272,21 +261,19 @@ private ExternalSettingOperationResult SetIsEnabledOnMachineWidePackageSources(`
`272`	`261`	`ActivityLog.LogError(ExceptionHelper.LogEntrySource, ex.ToString());`
`273`	`262`	`}`
`274`	`263`
`275`		`- return (result, hasAnyHiddenPropertyChanged);`
	`264`	`+ return result;`
`276`	`265`	`}`
`277`	`266`
`278`		`- private (ExternalSettingOperationResult result, bool hasAnyHiddenPropertyChanged) SaveAuditSources(`
	`267`	`+ private ExternalSettingOperationResult SaveAuditSources(`
`279`	`268`	`IReadOnlyList<IDictionary<string, object>> auditSourceDictionaryList,`
`280`	`269`	`CancellationToken cancellationToken)`
`281`	`270`	`{`
`282`		`- bool hasAnyHiddenPropertyChanged = false;`
`283`	`271`	`ExternalSettingOperationResult result;`
`284`	`272`
`285`	`273`	`try`
`286`	`274`	`{`
`287`	`275`	`List<PackageSource> auditSources = new List<PackageSource>(capacity: auditSourceDictionaryList.Count);`
`288`	`276`	`IReadOnlyList<PackageSource> existingAuditSources = LoadAuditSources();`
`289`		`- bool hasAnyPackageSourceNameChanged = false;`
`290`	`277`
`291`	`278`	`foreach (Dictionary<string, object> packageSourceDictionary in auditSourceDictionaryList)`
`292`	`279`	`{`
`@@ -299,12 +286,6 @@ private ExternalSettingOperationResult SetIsEnabledOnMachineWidePackageSources(`
`299`	`286`	`if (packageSourceDictionary.TryGetValue(MonikerPackageSourceId, out object packageSourceIdObj))`
`300`	`287`	`{`
`301`	`288`	`lookupName = packageSourceIdObj.ToString();`
`302`		`-`
`303`		`- if (!string.Equals(lookupName, name, StringComparison.CurrentCultureIgnoreCase))`
`304`		`- {`
`305`		`- // Changing the ID needs to refresh Unified Settings since the ID is a hidden property.`
`306`		`- hasAnyPackageSourceNameChanged = true;`
`307`		`- }`
`308`	`289`	`}`
`309`	`290`	`else // Newly added Package Sources will not have an ID yet.`
`310`	`291`	`{`
`@@ -327,8 +308,6 @@ private ExternalSettingOperationResult SetIsEnabledOnMachineWidePackageSources(`
`327`	`308`
`328`	`309`	`_packageSourceProvider.SaveAuditSources(auditSources);`
`329`	`310`
`330`		`- hasAnyHiddenPropertyChanged = hasAnyPackageSourceNameChanged;`
`331`		`-`
`332`	`311`	`result = ExternalSettingOperationResult.Success.Instance;`
`333`	`312`	`}`
`334`	`313`	`#pragma warning disable CA1031 // Do not catch general exception types`
`@@ -339,7 +318,7 @@ private ExternalSettingOperationResult SetIsEnabledOnMachineWidePackageSources(`
`339`	`318`	`ActivityLog.LogError(ExceptionHelper.LogEntrySource, ex.ToString());`
`340`	`319`	`}`
`341`	`320`
`342`		`- return (result, hasAnyHiddenPropertyChanged);`
	`321`	`+ return result;`
`343`	`322`	`}`
`344`	`323`
`345`	`324`