Onboard NuGet to Unified Settings and create General page #6229

donnie-msft · 2025-01-14T17:27:55Z

Bug

Fixes: NuGet/Home#14040

Description

General Options

The General Options are onboarded to the Unified Settings platform (USX) with the following major changes:

Our repo has a new registration.json which declares the property string values and types.
Our repo implements the IExternalSettingsProvider to react to user input from the Unified Settings general page.
The provider wraps our existing NuGet.Configuration types so that the monikers we list in our registration.json can be passed to NuGet's IExternalSettingsProvider implementation by USX.
A GeneralStub page was added which points to the USX General page when the customer has enabled USX. The reason for this is so that if a customer opens a legacy options NuGet page (eg, Configuration Files), they can still see "General" listed there and be able to open the USX General page directly.

Settings stored in NuGet.config

The link will open the Configuration Files settings page (which is currently a legacy VS Options dialog).

Details

Clear NuGet local resources

Pressing "Clear NuGet Local resources" results in a modal dialog so that customers have an opportunity to cancel the operation, as shown below.

Details

As with legacy VS options, once started, the command cannot be cancelled (though I've proposed an enhancement to this if we see customers are using the feature).
- The default button is "No" for "Clear all NuGet resources?".
Pressing "Yes" will open a modal dialog showing:
- A status message
- An indeterminate progress indicator
- A close button once the execution completes
The modal window is a WPF window bound to a ViewModel setup to Execute NuGet's clear all LocalsCommand, and indicate state for whether the command is in-progress, and provide a status message upon completion.
Once the window loads, it asks the VM to begin executing.
The command logs information to the Output Window in VS.
Once complete, the completion time is shown (legacy settings behavior) along with a completion status message (success or error text).
The close button is enabled.
If any error occurs, the exception will be shown in the dialog. Scrolling is supported for longer messages.
*Note that the aka.ms help URL shown is not directly clickable. This can be enhanced in a future iteration to be pulled out into its own Hyperlink.
Theming works properly in the modal dialog.

General Stub page

When a customer has opted-into Unified Settings then opens the legacy VS options for NuGet (eg, Package Sources), the General page will still appear.

Details

Instead of providing content, the stub page provides a link to directly open the General USX page. Before closing the legacy options dialog, it will prompt to save any pending changes:

Psuedo-localization testing

Tested PLOC at minimum supported resolution of 1366x768 at 100% DPI scaling.

PLOC Screenshots

Note, I used a WPF runtime tool to add extra text that's shown here so I could be sure that a very long error message will scroll correctly.

Accessibility testing

The Clear Locals modal dialog shows all green in Accessibility Insights.
I will schedule a test pass for this change as well.

Accessibility Screenshots

PR Checklist

Meaningful title, helpful description and a linked NuGet/Home issue
Added tests - (Also see UI testing sections above). The settings provider just wraps existing NuGet.Configuration types, so relying on that test coverage for most of the NuGet settings logic.
Link to an issue or pull request to update docs if this PR changes settings, environment variables, new feature, etc. Document the General settings page in Unified Settings Home#14039

microsoft-github-policy-service · 2025-01-14T17:28:16Z

This PR contains changes to XAML files. Verify that you've completed the following tasks:

Complete accessibility testing.
Complete localizability testing.

src/NuGet.Clients/NuGet.PackageManagement.UI/Resources.resx

src/NuGet.Clients/NuGet.PackageManagement.UI/Xamls/ClearNuGetLocalResourcesWindow.xaml.cs

src/NuGet.Clients/NuGet.PackageManagement.UI/ViewModels/ClearNuGetLocalsViewModel.cs

...Get.Clients/NuGet.PackageManagement.VisualStudio/Services/ExternalSettingsProviderService.cs

zivkan · 2025-01-15T06:01:14Z

...Get.Clients/NuGet.PackageManagement.VisualStudio/Services/ExternalSettingsProviderService.cs

+                default: break;
+            }
+
+            throw new ApplicationException("Unknown setting!");


just a thought, when the moniker is known, the code silently ignores when the value has the wrong type, so it doesn't feel entirely consistent to me to throw when the moniker is unknown.

zivkan · 2025-01-15T06:03:45Z

...Get.Clients/NuGet.PackageManagement.VisualStudio/Services/ExternalSettingsProviderService.cs

+            if (input is T value)
+            {
+                return Task.FromResult(ExternalSettingOperationResult.SuccessResult(value));
+            }
+
+            throw new ApplicationException("Error reading setting!");


Would it perhaps be better to cast, so that the exception message tells us what the expected and actual types are, rather than a static string that we'll only be able to understand via debugging (perfview trace attached to "record your steps" VS feedback won't have details of the error)

zivkan · 2025-01-15T06:04:49Z

...Get.Clients/NuGet.PackageManagement.VisualStudio/Services/ExternalSettingsProviderService.cs

+        {
+            if (typeof(T) != typeof(string))
+            {
+                throw new ApplicationException("Error reading setting!");


Should the error say what the actual type is, so we have more information without needing to debug?

zivkan · 2025-01-15T06:05:12Z

...Get.Clients/NuGet.PackageManagement.VisualStudio/Services/ExternalSettingsProviderService.cs

+            {
+                0 => (T)(object)MonikerPackagesConfig,
+                1 => (T)(object)MonikerPackageReference,
+                _ => throw new ApplicationException("Error reading setting!"),


Should we report the input value, so we have more information without needing to debug?

src/NuGet.Clients/NuGet.Tools/NuGetPackage.cs

jeffkl

Looks good overall, thanks for including all of the screenshots. I'll review again after Andy's comments are resolved

...Get.Clients/NuGet.PackageManagement.VisualStudio/Services/ExternalSettingsProviderService.cs

src/NuGet.Clients/NuGet.PackageManagement.UI/ViewModels/ClearNuGetLocalsViewModel.cs

JoshuaBStevens · 2025-01-15T21:59:48Z

src/NuGet.Clients/NuGet.PackageManagement.UI/Xamls/ClearNuGetLocalResourcesWindow.xaml

+
+    <!-- Content -->
+    <ScrollViewer Grid.Row="1" VerticalScrollBarVisibility="Auto" HorizontalScrollBarVisibility="Disabled">
+      <TextBlock Margin="0,12,0,12" TextWrapping="Wrap" Text="{Binding CommandCompleteText}" Visibility="{Binding IsCommandComplete, Converter={StaticResource BooleanToVisibilityConverter}}"/>


For accessibility, consider making this a LiveTextBlock (which is from the VS SDK) so it will "announce" changes to its text or visibility.

src/NuGet.Clients/NuGet.PackageManagement.UI/Xamls/ClearNuGetLocalResourcesWindow.xaml.cs

...Get.Clients/NuGet.PackageManagement.VisualStudio/Services/ExternalSettingsProviderService.cs

src/NuGet.Clients/NuGet.Tools/NuGetPackage.cs

src/NuGet.Clients/NuGet.VisualStudio.Client/registration.json

donnie-msft requested a review from a team as a code owner January 14, 2025 17:27

donnie-msft changed the title ~~Dev donnie msft unified settings init three~~ Onboard NuGet to Unified Settings and migrate General page Jan 14, 2025

donnie-msft changed the title ~~Onboard NuGet to Unified Settings and migrate General page~~ Onboard NuGet to Unified Settings and create General page Jan 14, 2025

jeffkl requested review from zivkan and jeffkl January 14, 2025 21:14

zivkan reviewed Jan 15, 2025

View reviewed changes

jeffkl reviewed Jan 15, 2025

View reviewed changes

...Get.Clients/NuGet.PackageManagement.VisualStudio/Services/ExternalSettingsProviderService.cs Show resolved Hide resolved

donnie-msft added 22 commits January 15, 2025 12:49

Init from branch two

ee4ef22

Fix rebase

eb350dc

WIP: Command opens PM UI

c8e2322

***NO_CI*** WIP stub new clearNuGetLocals MessageBox

5f4abf2

***NO_CI*** WIP Modal window for clearing local resources

f8d713e

***NO_CI*** WIP layout fix

b73eaf0

***NO_CI*** WIP stub general page

2dfbd72

Close Tools Options

d90e6c1

Remove ComVisible

60b6773

Prompt to save changes

7cac8fa

Remove stub general activator

d14beac

Update StubGeneral strings

74cde44

Update visibility context

aed451c

***NO_CI*** WIP locals command

ed10086

WIP clear locals execute command

a62333d

Clear Locals command dialog with Wait for testing

7e924af

Error handling for Clear Locals command

6a49a44

Undo test code

98e5dcc

Open Configuration Files settings from hyperlink

53997ef

Set determinate progress to 100

c745fbd

Add ellipses to Clearing all NuGet storage...

eba2726

Show status above progressBar

f869c43

donnie-msft added 17 commits January 15, 2025 12:49

MicrosoftBuildVersion to 17.11.4

ffbff7a

Fix namespace

dc10dbe

Pkgdef for General Stub page property

925389f

Text change: Prompt for format selection

8633329

Themed colors and formatting

97d53b0

Set default button to Cancel clearing locals

5a83ae7

cleanup

f69fef3

Comment pkgdef + modal height

d68fa29

More help links

7bfe1a8

Strings in StubGeneralOptionControl.xaml

ebc3a4c

Strings in ClearNuGetLocals and NuGetPackage

96a84e4

Create string SettingsStoredInNuGetConfiguration

7cc7cf7

Fix strings and remove unused code

e1ffa3a

Use JoinableTaskFactory and add tests

8513d2b

Remove margin + add ProgressBar AutomationProperty

404b0d8

Cast and await DataContext

694ec13

ExecuteBackgroundWorkAsync remove try-catch

3cffcea

JoshuaBStevens approved these changes Jan 15, 2025

View reviewed changes

donnie-msft added 2 commits January 15, 2025 16:00

Fix IDE0120

2386844

Create ClearNuGetLocalResourcesCommand

79d5164

donnie-msft force-pushed the dev-donnie-msft-unifiedSettingsInitThree branch from 781c5b5 to 79d5164 Compare January 16, 2025 00:09

donnie-msft added 9 commits January 15, 2025 16:36

Comment about CS0067 analyzer warning

38531d6

SettingValuesChanged?.Invoke

44094b3

Readonly Func<Task>

1eb2a3b

use NuGetUIThreadHelper

fc6a985

Remove extra JTF.Run

09798c9

Move OpenBackingStoreAsync comment to resx

3db2f38

remove namespace for AsyncLazy

5013c46

Enable nullable part 1

0736e99

Enable nullable part 2

f868c9c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Onboard NuGet to Unified Settings and create General page #6229

Onboard NuGet to Unified Settings and create General page #6229

donnie-msft commented Jan 14, 2025 •

edited

Loading

microsoft-github-policy-service bot commented Jan 14, 2025 •

edited by donnie-msft

Loading

zivkan Jan 15, 2025

zivkan Jan 15, 2025

zivkan Jan 15, 2025

zivkan Jan 15, 2025

jeffkl left a comment

JoshuaBStevens Jan 15, 2025

Onboard NuGet to Unified Settings and create General page #6229

Are you sure you want to change the base?

Onboard NuGet to Unified Settings and create General page #6229

Conversation

donnie-msft commented Jan 14, 2025 • edited Loading

Bug

Description

General Options

Settings stored in NuGet.config

Clear NuGet local resources

General Stub page

Psuedo-localization testing

Accessibility testing

PR Checklist

microsoft-github-policy-service bot commented Jan 14, 2025 • edited by donnie-msft Loading

zivkan Jan 15, 2025

Choose a reason for hiding this comment

zivkan Jan 15, 2025

Choose a reason for hiding this comment

zivkan Jan 15, 2025

Choose a reason for hiding this comment

zivkan Jan 15, 2025

Choose a reason for hiding this comment

jeffkl left a comment

Choose a reason for hiding this comment

JoshuaBStevens Jan 15, 2025

Choose a reason for hiding this comment

donnie-msft commented Jan 14, 2025 •

edited

Loading

microsoft-github-policy-service bot commented Jan 14, 2025 •

edited by donnie-msft

Loading