Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix empty combobox when package is not present in project file #4844

Merged
merged 3 commits into from
Oct 6, 2022
Merged

Fix empty combobox when package is not present in project file #4844

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
97d5cc8
fix empty combobox when package is not present in project file
martinrrm Oct 5, 2022
49ecb35
remove unnecesary guard
martinrrm Oct 5, 2022
d0a0ab0
fix filtering and disable button
martinrrm Oct 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/NuGet.Clients/NuGet.PackageManagement.UI/Models/DetailControlModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -842,7 +842,7 @@ protected void AddBlockedVersions(List<NuGetVersion> blockedVersions)
// add all the versions blocked to disable the update button
foreach (var version in blockedVersions)
{
_versions.Add(new DisplayVersion(version, string.Empty, isValidVersion: false));
_versions.Add(new DisplayVersion(version, additionalInfo: null, isValidVersion: false));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm doing this to keep the same format like in https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Clients/NuGet.PackageManagement.UI/Models/PackageDetailControlModel.cs#L218

}
}

Expand Down
5 changes: 3 additions & 2 deletions src/NuGet.Clients/NuGet.PackageManagement.UI/Models/PackageDetailControlModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ protected override Task CreateVersionsAsync(CancellationToken cancellationToken)
var latestStableVersion = allVersionsAllowed.FirstOrDefault(v => !v.version.IsPrerelease);

// Add installed version if the project is PackageReference
if (_nugetProjects.Any() && installedDependency != null && installedDependency.VersionRange != null && _nugetProjects.First().ProjectStyle.Equals(ProjectModel.ProjectStyle.PackageReference))
if (_nugetProjects.Any() && installedDependency != null && installedDependency.VersionRange.OriginalString != null && _nugetProjects.First().ProjectStyle.Equals(ProjectModel.ProjectStyle.PackageReference))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to understand why original string would be null when a package is not in the csproj file.

Is the package not being in the csproj file the problem, or is it the fact that it's autoreferenced?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if it is because is auto referenced but it could also be the problem. I was taking the .csproj as the only source for packages. When creating the versions from the project file we create the object from a string using VersionRange.Parse(string), this is the only way the attribute OriginalString is populated, so I think because this package is autoreferenced, im not sure how but probably using new VersionRange(Version)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd find it surprising if we're generating the Version Range differently for autoreferenced package.

At least in the nomination workflow. Maybe there's something we do differently in the PM UI side?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll double check this behavior

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found that when the package is autoreferenced we use new VersionRange() and this doesnt populates the OriginalString attribute. https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Core/NuGet.PackageManagement/BuildIntegratedPackageReference.cs#L91

Copy link
Member

@nkolev92 nkolev92 Oct 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see.
A few ideas:

  • We can consider making sure that we have an original version there no? Just doing that is probably not the right fix, but it's a valid consideration.
  • If OriginalString isn't an option should we show the normalized version instead?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please note that for an auto referenced version you should not be able to update the version through the PM UI.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • OriginalString cannot be set, only by using VersionRange.Parse()
  • The change in L170 is not affecting functionality, right we are displaying the version that the user wrote in the .csproj at the top and then all the versions, with this fix, if the OriginalString is null then we will display the latest version and then all the versions, like before but with filtering

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried considering using the normalized version when the OriginalString was null, but this created more issues when adding the blocking versions later in the code and would need more changes.

{
VersionRange installedVersionRange = VersionRange.Parse(installedDependency.VersionRange.OriginalString, true);
NuGetVersion bestVersion = installedVersionRange.FindBestMatch(allVersionsAllowed.Select(v => v.version));
Expand Down Expand Up @@ -311,6 +311,7 @@ public override void OnSelectedVersionChanged()
OnPropertyChanged(nameof(IsInstalledVersionTopLevel));
}


public bool IsSelectedVersionInstalled
{
get
Expand All @@ -326,7 +327,7 @@ public bool IsInstallorUpdateButtonEnabled
{
get
{
return SelectedVersion != null && !IsSelectedVersionInstalled;
return SelectedVersion != null && !IsSelectedVersionInstalled && !InstalledVersionIsAutoReferenced;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the package is autoreferenced then block the Update button

}
}

Expand Down