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

Jump to bottom

Clear Versions combobox when selecting a different Package #3894

Merged
merged 3 commits into from
Nov 24, 2021
Merged

Clear Versions combobox when selecting a different Package #3894

merged 3 commits into from
Nov 24, 2021

Conversation

donnie-msft
Copy link
Contributor

@donnie-msft donnie-msft commented Feb 10, 2021 •
edited
Loading

Bug

Fixes: NuGet/Home#10557

Regression? Last working version:

Description

When Selecting a package in the packages list, the Details Pane will immediately clear its versions list, so the combobox cannot reflect stale data.

To demo the difference, I injected a 5 second delay into the code:

Before (Versions list stale after selection change):

notClearing

After (Versions list cleared immediately):

afterClearing

PR Checklist

  • PR has a meaningful title

  • PR has a linked issue.

  • Described changes

  • Tests

    • Automated tests added - Test case ensures that a PropertyChanged event is raised on Versions, and that list is Empty.
    • OR
    • Test exception
    • OR
    • N/A

  • Documentation

    • Documentation PR or issue filled
    • OR
    • N/A

@donnie-msft donnie-msft force-pushed the dev-donnie-msft-versionListLag2 branch from 43b4e14 to f32d006 Compare February 10, 2021 22:19
@donnie-msft donnie-msft marked this pull request as ready for review February 10, 2021 22:20
@donnie-msft donnie-msft requested a review from a team as a code owner February 10, 2021 22:20
@donnie-msft donnie-msft force-pushed the dev-donnie-msft-versionListLag2 branch 2 times, most recently from a85c1cf to 8a59d93 Compare February 10, 2021 23:41
@donnie-msft donnie-msft marked this pull request as draft February 11, 2021 02:45
@nkolev92 nkolev92 deleted the dev-donnie-msft-versionListLag2 branch May 19, 2021 17:11
@donnie-msft donnie-msft restored the dev-donnie-msft-versionListLag2 branch November 2, 2021 23:20
@donnie-msft donnie-msft reopened this Nov 2, 2021
@donnie-msft donnie-msft force-pushed the dev-donnie-msft-versionListLag2 branch from 987914c to 67708f0 Compare November 2, 2021 23:32
zivkan
zivkan reviewed Nov 4, 2021
View reviewed changes
Copy link
Member

@zivkan zivkan left a comment

Choose a reason for hiding this comment

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

Would it make sense to show a "Loading..." message, rather than an empty box, while the version list is being loaded?

test/NuGet.Clients.Tests/NuGet.PackageManagement.UI.Test/Models/V3DetailControlModelTests.cs
@@ -50,6 +52,28 @@ public V3DetailControlModelTestBase(GlobalServiceProvider sp, V3PackageSearchMet
Sources = new List<PackageSourceContextInfo> { new PackageSourceContextInfo("nuget.psm.test") },
};
}

/// <summary>
/// Due to embedding the types we need to compare based on IsEquivalentTo
Copy link
Member

Choose a reason for hiding this comment

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

Embedding as in "embed interop types"? What types?

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 didn't write this comment. I'm also unclear what is meant by "embedding". Also potentially this is dead code? I don't see any calls that use the comparer.

@donnie-msft
Copy link
Contributor Author

Would it make sense to show a "Loading..." message, rather than an empty box, while the version list is being loaded?

It absolutely would be ideal to indicate that the box is populating. Whether that needs to be part of this PR, or would just be nice, could be debated. The issue is a UX review would be warranted, so I'm leaning separate PR.

@donnie-msft donnie-msft force-pushed the dev-donnie-msft-versionListLag2 branch from 8fff76d to c2a20b3 Compare November 19, 2021 04:01
@NuGet NuGet deleted a comment from alpr31 Nov 19, 2021
@donnie-msft donnie-msft force-pushed the dev-donnie-msft-versionListLag2 branch 2 times, most recently from 9d37ed9 to 5f220b9 Compare November 20, 2021 02:16
@donnie-msft donnie-msft marked this pull request as ready for review November 23, 2021 04:09
@donnie-msft donnie-msft requested review from zivkan and a team November 23, 2021 04:13
zivkan
zivkan reviewed Nov 23, 2021
View reviewed changes
test/NuGet.Clients.Tests/NuGet.PackageManagement.UI.Test/Models/V3DetailControlModelTests.cs Outdated
#pragma warning restore ISB001 // Dispose of proxies

ServiceLocator.InitializePackageServiceProvider(this);
Copy link
Member

Choose a reason for hiding this comment

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

This will cause flakey errors if any other test also calls ServiceLocator.InitializePackageServiceProvider, and the tests run in parallel.

Can we pass the IAsyncServiceProvider instance to classes via their class constructor, instead of using the static ServiceLocator class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This sounds like a DCR? I see we have code that references the static ServiceLocator already, so even if I change the test pattern we're using, it won't mitigate the problem you raised - right?

Example, https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Clients/NuGet.PackageManagement.VisualStudio/Services/NuGetSolutionManagerService.cs#L68

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed all ServiceLocator references and the tests still pass, so either this is no longer necessary or was never necessary.

@donnie-msft donnie-msft force-pushed the dev-donnie-msft-versionListLag2 branch from 5f220b9 to bcf0f8b Compare November 24, 2021 00:02
@donnie-msft
Copy link
Contributor Author

Next step, loading indicator to actually show some loading is in-progress: NuGet/Home#11410

@donnie-msft donnie-msft changed the title Clear Versions combobox when selecting a Package Clear Versions combobox when selecting a different Package Nov 24, 2021
@donnie-msft donnie-msft merged commit edd8d7a into dev Nov 24, 2021
@donnie-msft donnie-msft deleted the dev-donnie-msft-versionListLag2 branch November 24, 2021 03:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zivkan zivkan zivkan left review comments

@kartheekp-ms kartheekp-ms kartheekp-ms approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Versions List in Details Pane is not kept in sync when changing Selected Package
3 participants
@donnie-msft @zivkan @kartheekp-ms