Package Source Mapping Options UI by donnie-msft · Pull Request #4847 · NuGet/NuGet.Client

donnie-msft · 2022-10-05T22:57:52Z

Bug

Fixes: NuGet/Home#11362
Fixes: NuGet/Home#11363

Regression? Last working version:

Description

(Content copied from #4711 which this PR supersedes).

There is currently no support for package source mapping in the VS options UI. These changes allow the user to see all package source mappings in a new Package Source Mapping options dialog, and to Add, Remove, or ~~Clear~~ Remove All mappings. Here is a screenshot of this dialog showing the mappings for NuGet.sln:

When a user clicks Add, a new window appears, and the user can type in a package namespace and select from all the previously configured sources for that project to map to. Here is a screenshot showing this window:

There were some concerns about the modal dialog and the amount of whitespace in the ListView showing package source mappings, but the UX Board checked it and said it was okay.

PR Checklist

PR has a meaningful title
PR has a linked issue.
Described changes
Tests
- Automated tests added - There are 4 tests for changes to the API. Most of this feature is UI, however, so I have not written tests. This feature currently passes all checks for Accessibility Insights but one. The one accessibility issue is that the ListView showing the package source mappings in the Package Source Mappings options dialog shows the wrong value for the IsOffScreen property for mappings not in view. This issue appears in other VS options dialogs that have a ListView and is mentioned in this issue: Inconsistent UIA IsOffscreen and ClickablePoint properties for WPF ListViewItems that are scrolled out of view dotnet/wpf#4631 , so I decided not to fix it.

OR
- [ ] Test exception
- OR
- [ ] N/A
Documentation
- Documentation PR or issue filled https://github.com/NuGet/docs.microsoft.com-nuget/issues/2799
- OR
- N/A

src/NuGet.Clients/NuGet.PackageManagement.UI/Options/AddMappingDialog.xaml.cs

donnie-msft · 2022-10-05T23:01:30Z

src/NuGet.Clients/NuGet.PackageManagement.UI/Options/PackageSourceMappingOptionsPage.cs

+                OnActivateAsync(e, CancellationToken);
+
+            }, Resources.PackageSourceMappingOptions_OnActivated);


For those curious, yes there's a UI for a slow loading Options page. It looks like this (with a forced thread delay to make the situation happen):

donnie-msft · 2022-10-05T23:05:53Z

src/NuGet.Clients/NuGet.PackageManagement.UI/Options/SourceMappingViewModel.cs

+
+        public override string ToString()
+        {
+            return ID + " " + SourcesString;


I was unable to drill-in to the controls using Narrator or NVDA, but I think it's important to announce the Sources List when visiting a list item. Let me know any better suggestions. Our Package Sources page also announces both Source Name and URL in one take.

The way I'm implementing this is a string concatenation. Narrator and NVDA pause briefly between each comma in the sources list.

donnie-msft · 2022-10-05T23:06:51Z

src/NuGet.Clients/NuGet.PackageManagement.UI/Resources.Designer.cs

+        /// </summary>
+        public static string PackageSourceMappingOptions_OnActivated {
+            get {
+                return ResourceManager.GetString("PackageSourceMappingOptions_OnActivated", resourceCulture);


See OnActivate comment if you're curious where this string actually gets used. #4847 (comment)

donnie-msft · 2022-10-05T23:08:14Z

src/NuGet.Core/NuGet.Configuration/Settings/Items/PackageSourceMappingSourceItem.cs

            Dictionary<PackagePatternItem, PackagePatternItem> otherPatterns = packageSourceMappingSourceItem.Patterns.ToDictionary(c => c, c => c);
-            var immutablePatterns = new List<PackagePatternItem>(Patterns);
-            foreach (PackagePatternItem packagePatternItem in immutablePatterns)
+            var clonedPatterns = new List<PackagePatternItem>(Patterns);


While reading this code, I realized that "immutable" isn't accurate.

donnie-msft · 2022-10-05T23:10:19Z

src/NuGet.Clients/NuGet.PackageManagement.UI/Options/AddMappingDialog.xaml.cs

+
+            var newValue = (e.RoutedEvent == CheckBox.CheckedEvent);
+            var oldValue = !newValue; // Assume the state has actually toggled.
+            AutomationPeer peer = UIElementAutomationPeer.FromElement(itemContainer);


Tested that Toggle pattern is working correctly for adding new mappings. Added Newtonsoft.* mapped to nuget.org entirely with the keyboard:

donnie-msft · 2022-10-05T23:13:44Z

src/NuGet.Clients/NuGet.PackageManagement.UI/Options/AddMappingDialog.xaml

+                                 Background="{Binding Path=(Window.Background), RelativeSource={RelativeSource FindAncestor, AncestorType={x:Type Window}}}"
+                                 TextChanged="PackageID_TextChanged"/>
+        <StackPanel Orientation="Vertical"  Grid.Row="1">
+          <nuget:ProjectsListView AutomationProperties.Name="Sources List"


The control I needed to reuse was unfortunately named specifically for Projects list, but I'll refactor that in a separate PR: https://github.com/NuGet/Client.Engineering/issues/1903

donnie-msft · 2022-10-05T23:14:49Z

src/NuGet.Clients/NuGet.PackageManagement.UI/Options/PackageSourceMappingOptionsControl.xaml.cs

+                var viewModel = new SourceMappingViewModel(packageID, uiSourceMappings[packageID]);
+                mappingsCollection.Add(viewModel);
+            }
+            mappingsCollection.Sort((a, b) => StringComparer.OrdinalIgnoreCase.Compare(a.ID, b.ID));


Decided to Sort the mappings by Package ID in the options page, since otherwise it's just a random order of how the IDs are read from the config.

Note that when adding new mappings, they appear initially at the bottom until "OK" is pressed. I think this is standard/expected behavior.

src/NuGet.Clients/NuGet.PackageManagement.UI/Options/PackageSourceMappingOptionsControl.xaml.cs

loic-sharma · 2022-10-06T00:06:42Z

Looks great! Some minor UI feedback:

Should the Package Source Mappings pane provide some information as to what mappings are, how to use them? Should the Add New Package Source Mapping pane also provide some information? A Learn more link to docs would be great.
The Add New Package Source Mapping pane's Add Valid Package ID placeholder doesn't indicate you can use package ID patterns. Maybe the placeholder could give a hint like Add Package ID Pattern. Ex: 'Newtonsoft.*'
Should the Add New Package Source Mapping pane show both the source's name and URL? In your screenshot, it's hard to distinguish the vside_vs-impl and vside_vssdk sources.

donnie-msft · 2022-10-06T17:57:34Z

Great to hear from you!

Looks great! Some minor UI feedback:

Should the Package Source Mappings pane provide some information as to what mappings are, how to use them? Should the Add New Package Source Mapping pane also provide some information? A Learn more link to docs would be great.

Yes, in VS Options, we use a Help button "?" (see below), and that's why the Documentation section of this PR calls out creating a new Docs page:

The Add New Package Source Mapping pane's Add Valid Package ID placeholder doesn't indicate you can use package ID patterns. Maybe the placeholder could give a hint like Add Package ID Pattern. Ex: 'Newtonsoft.*'

@nkolev92 did we agree that "package ID" implies pattern, or do we need to state the word pattern? I believe we discussed this, so I don't want to make last minute changes. Also consider that the docs will help explain how this works.

Should the Add New Package Source Mapping pane show both the source's name and URL? In your screenshot, it's hard to distinguish the vside_vs-impl and vside_vssdk sources.

I'm not sure that adding lengthy URLs into the list is going to make anything easier to distinguish.
I'll see what UX thinks. Maybe a stronger separator between names would help?

nkolev92 · 2022-10-06T18:02:06Z

@nkolev92 did we agree that "package ID" implies pattern, or do we need to state the word pattern? I believe we discussed this, so I don't want to make last minute changes. Also consider that the docs will help explain how this works.

I think we should call it package pattern, https://learn.microsoft.com/en-us/nuget/consume-packages/package-source-mapping#package-pattern-syntax.

The docs call out Package ID pattern and Package Prefix Pattern.

I'll review the whole PR, but I'm pretty confident package pattern is the right one in this case, as it's what specifically came up with to be able to refer to both package id and package prefixes.
After all, the xml element is called package pattern

loic-sharma · 2022-10-06T18:44:42Z

Has that help button always been there??? I've never noticed it before 🤯

I'm not sure that adding lengthy URLs into the list is going to make anything easier to distinguish.
I'll see what UX thinks. Maybe a stronger separator between names would help?

Hm, another potential alternative could be a tooltip on hover that adds more details.

dominoFire

Partial review. Please address AutomationProperties.Name comments. Thanks! Good job!

src/NuGet.Clients/NuGet.PackageManagement.UI/Options/SourceMappingViewModel.cs

src/NuGet.Clients/NuGet.PackageManagement.UI/Models/ItemsChangeObservableCollectionBase.cs

src/NuGet.Clients/NuGet.PackageManagement.UI/Options/AddMappingDialog.xaml

dominoFire · 2022-10-07T19:17:18Z

src/NuGet.Clients/NuGet.PackageManagement.UI/Options/AddMappingDialog.xaml

+                <GridViewColumn>
+                  <GridViewColumn.CellTemplate>
+                    <DataTemplate>
+                      <CheckBox AutomationProperties.Name="SourcesCheckBox"


Suggested change

<CheckBox AutomationProperties.Name="SourcesCheckBox"

<CheckBox AutomationProperties.Name={x:Static nuget:Resources.PackageSourceCheckBox_AccessibleName}

I found 2 approaches in existing code:

Package Items have a checkbox for the Updates tab. I decided to mimic this behavior. Narrator reads "Selected" when visiting the checkbox:

NuGet.Client/src/NuGet.Clients/NuGet.PackageManagement.UI/Xamls/PackageItemControl.xaml

Line 91 in 8d54642

AutomationProperties.Name="{x:Static nuget:Resources.CheckBox_Selected}"

In the Projects List of Solution PMUI, we have a more complicated usage of AutomationProperties. The Name repeats the projectname. HelpText gives a sentence about what it means to select this item.
I'm not sure if either is warranted for this checkbox. Result for Narrator when drilling-in to this checkbox, it announces "Selected checkbox (Un)Checked".

I feel approach 1 is a better first step given the intent of the window and the behavior of the checkbox are announced indirectly by the screen-reader. PMUI Package list works similarly. We can expand the verbosity of AutomationProperties, if necessary.

I'm good with Approach #1 as long as it's reviewed by A11y team.

src/NuGet.Clients/NuGet.PackageManagement.UI/Options/AddMappingDialog.xaml.cs

src/NuGet.Clients/NuGet.PackageManagement.UI/Options/AddMappingDialog.xaml

donnie-msft · 2022-10-08T00:35:02Z

Has that help button always been there??? I've never noticed it before 🤯

Yes! Although I think not all pages implement it, it used to be the de facto documentation button.

I'm not sure that adding lengthy URLs into the list is going to make anything easier to distinguish.
I'll see what UX thinks. Maybe a stronger separator between names would help?

Hm, another potential alternative could be a tooltip on hover that adds more details.

@loic-sharma These are good ideas; however, reviewing this with UX folks, they don't feel there's a problem with distinguishing between the comma-delimited list. They actually suggested an Edit dialog as a solution to that problem, which we have considered after this first iteration. The idea is if you can re-open the dialog to edit a Mapping, the user can see the checkbox-list of sources (more concrete than the list). Do you feel that would resolve your concern?

donnie-msft · 2022-10-08T00:47:03Z

Modified Watermark in textbox for adding a new mapping to include an example: e99fd73

The font color is also incorrect. Couldn't fix this with a quick attempt, so filed NuGet/Home#12141

I think we should call it package pattern, https://learn.microsoft.com/en-us/nuget/consume-packages/package-source-mapping#package-pattern-syntax.

The docs call out Package ID pattern and Package Prefix Pattern.

@nkolev92 please let me know if the new text adequately addresses your concern.

donnie-msft · 2022-10-11T19:35:21Z

Changed "Clear" to "Remove All" based on UX feedback.

dominoFire

Signing off

src/NuGet.Clients/NuGet.PackageManagement.UI/Options/AddMappingDialog.xaml.cs

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

src/NuGet.Clients/NuGet.PackageManagement.UI/Options/PackageSourceMappingOptionsControl.xaml.cs

This reverts commit 9f30979.

…ng guidelines

…apping

…g for slow loading

…eaders

donnie-msft · 2022-10-12T20:22:09Z

Is there any harm in providing it here then?

"Fixing" the analyzer seems to be less noisy than suppressing it.

@nkolev92 - The harm is that it will wrap the call in a JTF.Run, which is not accomplishing anything logically.

https://learn.microsoft.com/en-us/dotnet/api/microsoft.visualstudio.platformui.delegatecommand-1.-ctor?view=visualstudiosdk-2022

donnie-msft commented Oct 5, 2022

View reviewed changes

donnie-msft marked this pull request as ready for review October 5, 2022 23:15

donnie-msft requested a review from a team as a code owner October 5, 2022 23:15

donnie-msft commented Oct 5, 2022

View reviewed changes

src/NuGet.Clients/NuGet.PackageManagement.UI/Options/PackageSourceMappingOptionsControl.xaml.cs Outdated Show resolved Hide resolved

donnie-msft commented Oct 5, 2022

View reviewed changes

src/NuGet.Clients/NuGet.PackageManagement.UI/Options/PackageSourceMappingOptionsControl.xaml.cs Outdated Show resolved Hide resolved

dominoFire reviewed Oct 8, 2022

View reviewed changes

donnie-msft force-pushed the dev-mcnallyella-packageSourceMappingOptions branch from 804eff1 to e99fd73 Compare October 8, 2022 00:46

donnie-msft requested a review from dominoFire October 11, 2022 19:28

dominoFire previously approved these changes Oct 11, 2022

View reviewed changes

nkolev92 reviewed Oct 12, 2022

View reviewed changes

mcnallyella and others added 13 commits October 12, 2022 13:01

squashing commits

6779ebe

change to API

71261ad

Added more space between listview items

55f6acc

use AddRange() for ItemsChangeObservableCollection

a69cd62

Add contains

5e0f15a

Revert "Add contains"

c35806a

This reverts commit 9f30979.

change scrollbar color

acbc453

change binding for scroll bar color

ede793f

Address PR feedback

6a53c24

get add button binding working for checkboxes

a183948

Got add button binding working

ebf94df

Rename MappingUIDisplay to SourceMappingViewModel

c02de59

***NO_CI*** WIP rename PackageSourceCountextInfoChecked

c78b22b

donnie-msft added 21 commits October 12, 2022 13:01

Changes after rebase

e87b562

Remove unused System.ServiceProcess

f527715

Dispose of INuGetSourcesService

b77ac1d

Prevent whitespace Package ID; some renaming/refactoring to meet codi…

19b8477

…ng guidelines

Sort mappings by Package ID

ebf6faa

Use DelegateCommand, remove ButtonCommand; rename several commands

6ce69f1

No longer use hard-coded values for Grid Layout; refactor ExecuteAddM…

680f641

…apping

Make Sources ReadOnly

9c6221e

Refactor PackageSourceMappingOptionsControl

47b1e10

InitializeOnActivated no longer Async; Created Getting Mappings strin…

2fc524f

…g for slow loading

ArgumentNullException on ViewModel properties

e0793e5

Fix screen reader for Mapping items

6c3e036

Add Mapping list Toggles appropriately with screen readers

e562222

Add Mapping list items use Source Name with screen readers

e0403ed

Mapping viewModels have ToString of ID and SourcesString for Screen R…

a101045

…eaders

Refactor ConvertViewModelToSourceMappingsSourceItems

95920a9

Rename to _mappingList

29a8274

Provide Example in Watermark for Package ID pattern

67db722

Change Clear to Remove All

3c1c50b

Address nits and a11y strings

27590e8

Change Package ID Pattern verbiage

0934c5f

donnie-msft dismissed dominoFire’s stale review via 0934c5f October 12, 2022 21:13

donnie-msft force-pushed the dev-mcnallyella-packageSourceMappingOptions branch from 18f3037 to 0934c5f Compare October 12, 2022 21:13

donnie-msft mentioned this pull request Oct 12, 2022

[DCR]: Adding a new Package Source in VS Options should be immediately available for a Package Source Mapping NuGet/Home#12155

Closed

nkolev92 approved these changes Oct 13, 2022

View reviewed changes

donnie-msft merged commit f08ff09 into dev Oct 13, 2022

donnie-msft deleted the dev-mcnallyella-packageSourceMappingOptions branch October 13, 2022 00:29

		OnActivateAsync(e, CancellationToken);

		}, Resources.PackageSourceMappingOptions_OnActivated);

	<CheckBox AutomationProperties.Name="SourcesCheckBox"
	<CheckBox AutomationProperties.Name={x:Static nuget:Resources.PackageSourceCheckBox_AccessibleName}

Conversation

donnie-msft commented Oct 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bug

Description

PR Checklist

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

donnie-msft Oct 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

loic-sharma commented Oct 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

donnie-msft commented Oct 6, 2022

Uh oh!

nkolev92 commented Oct 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

loic-sharma commented Oct 6, 2022

Uh oh!

dominoFire left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

donnie-msft Oct 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

donnie-msft commented Oct 8, 2022

Uh oh!

donnie-msft commented Oct 8, 2022

Uh oh!

donnie-msft commented Oct 11, 2022

Uh oh!

dominoFire left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

donnie-msft commented Oct 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

donnie-msft commented Oct 5, 2022 •

edited

Loading

donnie-msft Oct 5, 2022 •

edited

Loading

loic-sharma commented Oct 6, 2022 •

edited

Loading

nkolev92 commented Oct 6, 2022 •

edited

Loading

donnie-msft Oct 11, 2022 •

edited

Loading