Package Source Mapping Options UI #4711

mcnallyella · 2022-07-01T19:27:21Z

Bug

Regression? Last working version:

Description

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 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.cs

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

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

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

donnie-msft

Things are really coming along nicely! 🥳
Few considerations, nitpicks, and suggestions...

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

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

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

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

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

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

donnie-msft · 2022-07-13T04:50:11Z

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

+        }
+
+        //converts from list of packagesourcemappingsourceItems to a dictonary that can be read by UI
+        private ItemsChangeObservableCollection<PackageItem> ReadMappingsFromConfigToUI(IReadOnlyList<PackageSourceMappingSourceItem> originalMappings)


Can this method be reduced? As another reviewer mentioned, I think more ideal would be using a more generic type than ObservableCollection, then interacting with the single SourceMappingsCollection directly.

zivkan · 2022-07-13T13:20:35Z

src/NuGet.Core/NuGet.Configuration/PackageSourceMapping/PackageSourceMapping.cs

        /// Source name to package patterns list.
        /// </summary>
-        internal IReadOnlyDictionary<string, IReadOnlyList<string>> Patterns { get; }
+        public IReadOnlyDictionary<string, IReadOnlyList<string>> Patterns { get; }


This PR is not in draft mode, but this change (and others in src/NuGet.Core) are from this PR: #4709

Please focus one completing dependent PRs before marking new PRs that need the same change as "ready for review", to minimize workload on the rest of the team.

zivkan · 2022-07-13T13:22:34Z

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

+using Microsoft.VisualStudio.Shell;
+
+
+namespace NuGet.Options


This namespace doesn't feel right to me. Usually project default namespaces are the project name + folder within the project, so shouldn't the namespace be NuGet.PackageManagement.UI.Options?

Same question for the other new files in this PR.

I decided not to change the namespace since the two existing options pages are in the NuGet.Options namespace.

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

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

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

dominoFire

Good job. Some comments.

dominoFire · 2022-07-14T23:39:03Z

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

@@ -0,0 +1,89 @@
+<Window x:Class="NuGet.Options.AddMappingDialog"


Like, using our VsDialogWindow class?

Suggested change

<Window x:Class="NuGet.Options.AddMappingDialog"

<nuget:VsDialogWindow x:Class="NuGet.Options.AddMappingDialog"

src/NuGet.Clients/NuGet.PackageManagement.UI/GlobalSuppressions.cs

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

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

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

src/NuGet.Core/NuGet.Configuration/PackageSourceMapping/PackageSourceMappingProvider.cs

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

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

erdembayar · 2022-07-27T02:19:31Z

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

+            IReadOnlyList<PackageSourceMappingSourceItem> packageSourceMappingsSourceItems = ReadMappingsFromUIToConfig(SourceMappingsCollection);
+            try
+            {
+                if (SourceMappingsChanged(_originalPackageSourceMappings, packageSourceMappingsSourceItems))


Do we need to call this method to find out if Source Mapping changed?
Doesn't packageSourceMappingProvider.SavePackageSourceMappings method only save if there is change?

No SavePackageSourceMappings will go through all of the mappings and call AddOrUpdate even if nothing is changed (it won't modify the config file, though). It seems like it makes more sense to check if Save even needs to be called in the first place

erdembayar · 2022-07-27T02:55:26Z

Is edit button considered at this point? Currently remove button simply remove the patterns with all the sources.

Many commits since review

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

erdembayar

Another feedbacks, I believe your PR is very close. 🚀

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

erdembayar · 2022-08-02T01:49:07Z

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

+{
+    public class MappingUIDisplay
+    {
+        public string ID { get; set; }


Is it just package Id or packagePattern? Maybe I rename to PackageId or PackagePattern, it's quite hard to remember it was packageId to begin.

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

erdembayar · 2022-08-02T01:56:44Z

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

+                        mappingsDictonary[source.Name] = new List<PackagePatternItem>();
+                    }
+                    PackagePatternItem tempID = new PackagePatternItem(mappingUIDisplay.ID);
+                    bool newID = true;


I believe line 224-236 can be replaced with below, which is more readable.

PackagePatternItem tempID = new PackagePatternItem(mappingUIDisplay.ID); if (!mappingsDictonary[source.Name].Any(id => id.Pattern == tempID.Pattern)) { mappingsDictonary[source.Name].Add(tempID); }

will do. thanks

kartheekp-ms · 2022-08-02T17:18:08Z

Team Triage: Work with @dominoFire regarding the localization of strings.

dominoFire

Please address button overflow. Keep up the good work!

dominoFire · 2022-08-06T00:29:19Z

src/NuGet.Clients/NuGet.PackageManagement.UI/NuGet.PackageManagement.UI.csproj

    <Reference Include="System.Drawing" />
    <Reference Include="System.Runtime.Caching" />
    <Reference Include="System.Runtime.Serialization" />
+    <Reference Include="System.ServiceProcess" />


Do we need this namespace? I don't see any namespace reference to System.ServiceProcess

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

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

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

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

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

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

dominoFire · 2022-08-06T01:57:33Z

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

+
+namespace NuGet.Options
+{
+    internal class ButtonCommand : ICommand


I may be wrong but, I believe you can remove this class and achieve the same result using CommandBinding directly in XAML control.

Buttons in both dialogs (option dialog and add source mapping dialog) can map to method actions using XAML CommandBinding construc.

An example can be found in PackageManagerControl for search command:

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

Lines 34 to 36 in 3ec6f02

<CommandBinding

Command="{x:Static nuget:Commands.RestartSearchCommand}"

Executed="ExecuteRestartSearchCommand"/>

Ping me if you have more questions.

ghost · 2022-08-23T19:01:25Z

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 7 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.

ghost · 2022-08-30T19:01:31Z

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 7 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.

donnie-msft · 2022-08-30T23:20:47Z

Like, using our VsDialogWindow class?

#4711 (comment)

@dominoFire - I decided to delete that class :) #4778

ghost · 2022-09-07T19:01:22Z

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 7 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.

This reverts commit 9f30979.

ghost · 2022-09-15T07:01:36Z

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 7 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.

donnie-msft · 2022-09-29T01:47:34Z

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. ff324ec

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

mcnallyella mentioned this pull request Jul 7, 2022

Added save for PackageSourceMappingsProvider #4709

Merged

8 tasks

mcnallyella marked this pull request as ready for review July 12, 2022 17:42

mcnallyella requested a review from a team as a code owner July 12, 2022 17:42

mcnallyella commented Jul 12, 2022

View reviewed changes

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

mcnallyella commented Jul 12, 2022

View reviewed changes

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

mcnallyella commented Jul 12, 2022

View reviewed changes

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

mcnallyella commented Jul 12, 2022

View reviewed changes

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

mcnallyella commented Jul 12, 2022

View reviewed changes

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

mcnallyella force-pushed the dev-mcnallyella-packageSourceMappingOptions branch from 829a07d to 849bb52 Compare July 12, 2022 19:09

donnie-msft previously requested changes Jul 13, 2022

View reviewed changes

zivkan reviewed Jul 13, 2022

View reviewed changes

donnie-msft reviewed Jul 13, 2022

View reviewed changes

mcnallyella force-pushed the dev-mcnallyella-packageSourceMappingOptions branch from 2722c90 to a907efa Compare July 14, 2022 21:44

dominoFire reviewed Jul 15, 2022

View reviewed changes

mcnallyella force-pushed the dev-mcnallyella-packageSourceMappingOptions branch 2 times, most recently from 1ad6fea to a9ee7dc Compare July 25, 2022 19:08

erdembayar reviewed Jul 26, 2022

View reviewed changes

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

erdembayar reviewed Jul 26, 2022

View reviewed changes

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

erdembayar reviewed Jul 27, 2022

View reviewed changes

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

erdembayar reviewed Jul 27, 2022

View reviewed changes

mcnallyella force-pushed the dev-mcnallyella-packageSourceMappingOptions branch 2 times, most recently from 05d592e to e91e3db Compare August 1, 2022 23:35

erdembayar reviewed Aug 2, 2022

View reviewed changes

src/NuGet.Clients/NuGet.PackageManagement.UI/Models/ItemsChangeObservableCollectionBase.cs Show resolved Hide resolved

erdembayar reviewed Aug 2, 2022

View reviewed changes

dominoFire reviewed Aug 6, 2022

View reviewed changes

jeffkl assigned donnie-msft Aug 16, 2022

ghost added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Aug 23, 2022

ghost removed the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Aug 30, 2022

donnie-msft force-pushed the dev-mcnallyella-packageSourceMappingOptions branch from 6ab4bae to 21f7a87 Compare August 31, 2022 04:03

ghost added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Sep 7, 2022

mcnallyella and others added 13 commits September 7, 2022 21:46

squashing commits

c0d898c

change to API

ec7ad55

Added more space between listview items

7e6d209

use AddRange() for ItemsChangeObservableCollection

c310259

Add contains

6c894aa

Revert "Add contains"

a9976f6

This reverts commit 9f30979.

change scrollbar color

cf8d684

change binding for scroll bar color

8a3a15f

Address PR feedback

6f4db5a

get add button binding working for checkboxes

d425933

Got add button binding working

01b59d7

Rename MappingUIDisplay to SourceMappingViewModel

5d34ffc

***NO_CI*** WIP rename PackageSourceCountextInfoChecked

95f3603

donnie-msft force-pushed the dev-mcnallyella-packageSourceMappingOptions branch from 21f7a87 to 95f3603 Compare September 8, 2022 04:47

ghost removed the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Sep 8, 2022

ghost added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Sep 15, 2022

ghost closed this Sep 22, 2022

ghost removed the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Sep 29, 2022

donnie-msft mentioned this pull request Oct 5, 2022

Package Source Mapping Options UI #4847

Merged

6 tasks

donnie-msft removed their assignment Oct 5, 2022

This pull request was closed.

		@@ -0,0 +1,89 @@
		<Window x:Class="NuGet.Options.AddMappingDialog"

	<Window x:Class="NuGet.Options.AddMappingDialog"
	<nuget:VsDialogWindow x:Class="NuGet.Options.AddMappingDialog"

	<CommandBinding
	Command="{x:Static nuget:Commands.RestartSearchCommand}"
	Executed="ExecuteRestartSearchCommand"/>

Package Source Mapping Options UI #4711

Package Source Mapping Options UI #4711

Uh oh!

Conversation

mcnallyella commented Jul 1, 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

donnie-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

dominoFire left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erdembayar commented Jul 27, 2022

Uh oh!

Uh oh!

erdembayar left a comment • 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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

erdembayar Aug 2, 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!

kartheekp-ms commented Aug 2, 2022

Uh oh!

dominoFire left a comment

Choose a reason for hiding this comment

Uh oh!

mcnallyella commented Jul 1, 2022 •

edited

Loading

erdembayar left a comment •

edited

Loading

erdembayar Aug 2, 2022 •

edited

Loading