Show Package Source Mapping Status in Details Pane

[donnie-msft]

Bug

Fixes: NuGet/Home#12586

Regression? Last working version:

Description

• PM UI (project and solution levels) will now show a state indicating whether: Package Source Mapping is off; the package requires a mapping, or the package is mapped.

• Shortcut to Settings page for mappings is shown beside the status

• External changes to a nuget.config are handled. The cached mappings are refreshed, and bound property changed events are raised to update the XAML.

• Accessibility Insights passed and manual Narrator testing and tab-order works as expected:
  

• PLOC testing done to ensure the strings display and wrap appropriately:
  
  

PR Checklist

• PR has a meaningful title

• PR has a linked issue.

• Described changes

• Tests

  • Automated tests added
  • OR
  • Test exception
  • OR
  • N/A

• Documentation

  • Documentation PR or issue filled Describe Package Source Mappings in the Details Pane docs.microsoft.com-nuget#3063
  • OR
  • N/A

[jebriede]

Great work! I like how this is using MVVM for the new package source mapping section. The addition of the ViewModelBase will be useful for common ViewModel functionality. Left some comments, mostly relating to MVVM responsibilities.

[jebriede]

The ViewModel should not take a direct dependency on the INuGetUI. Currently, the dependency on INuGetUI in this ViewModel enables this class to handle two things which can be handled in their appropriate layers:

1. Providing the PackageSourceMappingControl access to the UIController. The Control then asks the UIController to launch the Options dialog which is a View concept so the ViewModel should not need to know about the View-specific helper class. The Control should take a direct dependency on the UIController instead of using the ViewModel as a pass-through.
2. The UIController has the UIContext which holds on to the PackageSourceMapping instance. The ViewModel is holding on to the INuGetUI to gain access to the UIContext and in turn, the PackageSourceMapping instance. Instead, the ViewModel should take a direct dependency on the PackageSourceMapping model that should be supplied to it when the ViewModel is created.

[donnie-msft]

Appreciate your diligence digging into the patterns used here. Just wanted to mention that I am only focusing on new code being respectful of MVVM, but am not refactoring any existing monolith UI logic to follow the pattern. Large-scale refactoring is out of scope for this PR.

The interaction with the View and ViewModel makes it so the View listens for changes to the Settings model directly[…]
The Control then asks the UIController to launch the Options dialog which is a View concept […]
Currently, the View is acting as the moderator between the PackageSourceMappingActionViewModel and the ISettings model[…]

The way I think about this interaction is how I think of Commands. Commands may be bound in the View (similar to attaching Click event handlers), but Command implementation is actually found in VMs, not the View (still a prominent pattern mentioned in recent MAUI docs, for example: Model-View-ViewModel | Microsoft Learn). Most commands interact with other VMs, and UIController is (partly) a VM. Unfortunately, our PackageManagerControl doesn't have its own VM which could host the same logic rather than in a Click event handler.

[...] the ViewModel should take a direct dependency on the PackageSourceMapping model [...]

UIController doesn't follow MVVM, but to me this is a mixed class with Model and ViewModel aspects. For instance, it knows how to create a (psuedo-ViewModel) class instance and pass that to a View which displays a Modal popup window. At the same time, it models properties like RecommendedCount which is something we'd expect to see in a VM.
In lieu of any significant refactoring to break apart PackageManagerControl's DataContext, I believe PackageSourceMapping is another component making up this model.

[jebriede]

Sorry for the misunderstanding. I am not suggesting we refactor the INugetUI / UIController. I agree that is out of scope for this PR and we don't need to touch that in order to make the changes I proposed. The changes I'm proposing should be fairly simple: Instead of passing the UIController as a dependency to the ViewModel, pass the objects that the ViewModel needs from the UIController. Looking carefully at the code, I don't see the ViewModel invokes anything directly on the UIController. So instead of depending on the container object for the actual dependencies the ViewModel needs, the ViewModel should depend directly on the objects it's getting from the UIController. Specifically, the ViewModel uses only the PackageSourceMapping which it gets from the UIContext from the UIController. The real dependency here is PackageSourceMapping so let's pass that in directly.

[jebriede]

The interaction with the View and ViewModel makes it so the View listens for changes to the Settings model directly, then invokes a method on the ViewModel to raise property change events for properties that the View is supposed to react to. Currently, the View is acting as the moderator between the PackageSourceMappingActionViewModel and the ISettings model which violates the MVVM pattern.

Instead, the ViewModel should have a dependency on the ISettings model and listen for changes by subscribing to the SettingsChanged event. When SettingsChanged is invoked by Settings, the ViewModel's private event handler will raise the necessary property changed events. The handler should be private though because only the ViewModel should invoke it or know when to invoke it. Since the View is listening for PropertyChanged events on the ViewModel (through data binding), the View will get updated when the property changed invocations happen.

[donnie-msft]

Consolidating similar comments to this thread: #5166 (comment)

[jebriede]

In this code, the View (PackageManagerControl) listens directly to the Model (ISettings) and then instructs the ViewModel (PackageSourceMappingActionViewModel) to invoke the SettingsChanged which in turn tells the View to update. There are a few reasons this is not good:

1. Since the View is directly responding to the Model's event, there's no reason for the View to then tell the ViewModel to fire a PropertyChanged event which just tells the View that the properties changed. The View already knows things changed. It doesn't need to tell another object to tell itself that the model changed.
2. The proper relationship between the View, ViewModel, and Model is like this:

View -> ViewModel -> Model

The way this code is structured, the interaction is like this:

View -> Model
View -> ViewModel

The View should only interact with the ViewModel and not the model (ISettings) directly. The ViewModel should listen for changes to the ISettings and when they're found, fire the property changed notification to inform the View things changed.

I'm not suggesting we do any refactoring in the View to eliminate its dependency on the ISettings for other code. That's out of scope for this change. The View probably uses ISettings for other things and that's ok for now and not something we should fix in this feature PR. However, we can make some small changes in this PR to not continue that pattern. Simply by passing ISettings as a dependency for the ViewModel and interacting with the ViewModel from the View for this feature we will be on the right track. Also, we should definitely fix how the View tells the ViewModel to tell itself that things changed. That's a public API on the ViewModel that doesn't offer any value.

[donnie-msft]

In this code, the View (PackageManagerControl) listens directly to the Model (ISettings)

Could you link to the line you're referring to? The comment is on a line accessing _detailModel which is not a View, and that's from within an event handler, Settings_SettingsChanged, which is also not attached to a View.
https://github.com/NuGet/NuGet.Client/blob/1ed4af017cf61be56b38583baf04a7f6c437e456/src/NuGet.Clients/NuGet.PackageManagement.UI/Xamls/PackageManagerControl.xaml.cs#LL188C13-L188C25

[jebriede]

Could you link to the line you're referring to?

Yep! It's just a few lines up in this PackageManagerControl.xaml.cs:

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

Line 183 in 1ed4af0

Settings.SettingsChanged += Settings_SettingsChanged;

[donnie-msft]

In this code, the View (PackageManagerControl) listens directly to the Model (ISettings)

Could you link to the line you're referring to?

Yep! It's just a few lines up in this PackageManagerControl.xaml.cs:

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

Line 183 in 1ed4af0

Settings.SettingsChanged += Settings_SettingsChanged;

Thanks for providing that, but neither Settings nor SettingsChanged nor the implementation of SettingsChanged are a View, and none of them manipulate a View.

[jebriede]

I didn't say that the Settings manipulates the View.

Settings is a Model. PackageManagerControl is a View. This PR has the the PackageManagerControl View listening directly to the Settings Model for changes. This is not correct. Furthermore, I pointed out code that is not providing any value above. This should have been fixed before this was merged. Can you please submit a follow-up PR addressing this comment? I'm happy to have a chat offline to clarify any confusion.

[jebriede]

@donnie-msft: I made the change to illustrate how the ViewModel (PackageSourceMappingActionViewModel) would listen to the Model (Settings) and update the View (PackageManagerControl) via the PropertyChanged notifications. This change eliminates the need for the PackageManagerControl to handle the Settings.SettingsChanged directly and now it relies on the ViewModel to tell it when things in the Settings updated which is what we want in MVVM interactions.

It also eliminates the public method on the ViewModel and relies on a private event handler instead, so the ViewModel can determine when it needs to raise its property changed notifications, not the View (PackageManagerControl).

Here's the PR, currently in draft: #5173

pushed new commits

[erdembayar]

@donnie-msft
I tested this branch for onboarding a repository that has its own nuget.config file. However, it's creating the following entry in the default nuget.config file in AppData>>Roaming. Is this the expected behavior? I would expect it to be in the repository's nuget.config, not the default one, because mappings are usually repo specific, 2 different repos may have different mapping rules for same package Id.

  <packageSourceMapping>
    <packageSource key="nuget">
        <package pattern="NuGet1" />
      </packageSource>
  </packageSourceMapping>

[erdembayar]

Also it accept 2 different entries for same source nuget. Is it expected?

[donnie-msft]

@donnie-msft I tested this branch for onboarding a repository that has its own nuget.config file. However, it's creating the following entry in the default nuget.config file in AppData>>Roaming. Is this the expected behavior? I would expect it to be in the repository's nuget.config, not the default one, because mappings are usually repo specific, 2 different repos may have different mapping rules for same package Id.

  <packageSourceMapping>
    <packageSource key="nuget">
        <package pattern="NuGet1" />
      </packageSource>
  </packageSourceMapping>

This PR doesn't change how mappings are created. It's just a UI for status. Perhaps there's behavior there that's not expected. Do you have a <clear /> in your solution config?

[erdembayar]

For me it's not clear "Requires a package source mapping.", it doesn't tell if I'm already onboarded to source mapping or not, if not how to onboard.

[donnie-msft]

Also it accept 2 different entries for same source nuget. Is it expected?

You're testing VS Options, which is not what this PR modifies. Please test the PM UI changes for this PR.
(FYI, yes, that's known behavior. It will save 1 entry, but the UI isn't merging things together as you add them. There's other Options design changes needed, like NuGet/Home#12155)

[donnie-msft]

For me it's not clear "Requires a package source mapping.", it doesn't tell if I'm already onboarded to source mapping or not, if not how to onboard.

"Requires" implies you're onboarded. We don't require source mappings for restore unless you're onboarded.
There's a different informational message for not being onboarded:

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

Lines 990 to 992 in 3f66805

	<data name="Text_PackageMappingsDisabled" xml:space="preserve">
	<value>Package source mapping is off.</value>
	</data>

[donnie-msft]

Synced offline with @erdembayar and we added some more discussion points to the spec for improving nuget.config interaction/validation: NuGet/Home#12557