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.

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

Various fixes and improvements #582

Merged
merged 10 commits into from
Oct 18, 2023
Merged

Various fixes and improvements #582

merged 10 commits into from
Oct 18, 2023

Conversation

popara96
Copy link
Collaborator

@popara96 popara96 commented Oct 13, 2023

I apologize upfront for a bit larger PR. Wanted to fix a couple of small things but one thing led to another and we ended up with this. Basically the only 'larger' change here is how the Update tab works, and how the tab responsibilities are split, other than that it's really just minor fixes.

Changes introduced:

  1. Fixed refreshAssets propagation

    We are now refreshing assets only for the topmost package when installing/updating packages. This will make sure that while installing or updating we refresh the assets only once. It also fixes one dependency being marked as explicit wrongly during installation.

  2. Fixed update tab for V2 packages

    Until now every version of the package newer than the version installed was shown as a separate package. This is now fixed by showing the dropdown selection list (similar to how V3 looks), with stacking Release Notes in the Details section showing release notes for every newer version in descending order.

  3. Cleaner tab responsibilities

    Every tab now has a separate reponsibility depending on what it should do:

    • Online tab will show dropdown list of versions along with Install button if the package is not installed, otherwise it will only show Installed Version to indicate what is already installed.

    • Installed tab shows what version is installed and options to uninstall packages or mark dependencies as explicit packages.

    • Updates tab shows installed version and shows, in a dropdown list, selection of newer versions with Update button (if Show Downgrades is not checked), or lower versions with Downgrade button (if Show Downgrades is checked).

    • Update All button is now shown only if Show Downgrades is not checked as we don't want to offer downgrading all packages at once. Default selected versions in dropdown lists are always the highest newer or highest lower version, depending on the checkbox, and the dropdown does NOT contain the currently installed version.

    NOTE: when fetching packages, list of versions is now packed in descending instead of ascending order. This is done so we don't have to sort them by descending order every time we refresh the dropdown list of available packages, so as not to throttle the UI.

  4. Fixed finding best framework group dependencies

    Small fix that caches the framework group after we first find it, this also fixes logging the best target framework group every time we expand Details on a package.

- Fixed "refreshAssets" propagation
- Fixed update tab for V2 packages
- Cleaner tab responsibilities
- Fixed finding best framework group dependencies
@popara96 popara96 requested review from JoC0de and igor84 October 13, 2023 10:44
@JoC0de JoC0de linked an issue Oct 14, 2023 that may be closed by this pull request
Copy link
Collaborator

@JoC0de JoC0de left a comment

Choose a reason for hiding this comment

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

The PR actually isn't that big I had bigger ones 😄
It was not that much of a problem to see what change was related to which feature.
I like the caching of the selected dependencies. Eventually we can even extend this to provide warnings if a package has no supported dependency group / e.g. a user tries to install a .Net Framework package into a .Net Standard project.
For the UI changes I am not sure if we really should remove the possibility to update / uninstall packages on the other tabs. I added some screenshots of how it works in the Visual Studio package manager window.
What is the reasoning behind adding the showDowngrades toggle? If I have a dropdown I know what version to select / or are there to many versions and the Unity dropdown has a to bad usability?
If we Update the UI we should also update the screenshots used in the Readme.md.

src/NuGetForUnity/Editor/Models/NugetPackageV2Base.cs Outdated Show resolved Hide resolved
src/NuGetForUnity/Editor/Models/INugetPackage.cs Outdated Show resolved Hide resolved
src/NuGetForUnity/Editor/Models/NugetPackageV3.cs Outdated Show resolved Hide resolved
src/NuGetForUnity/Editor/Ui/NugetWindow.cs Show resolved Hide resolved
src/NuGetForUnity/Editor/Ui/NugetWindow.cs Outdated Show resolved Hide resolved
src/NuGetForUnity/Editor/Ui/NugetWindow.cs Show resolved Hide resolved
src/NuGetForUnity/Editor/Ui/NugetWindow.cs Show resolved Hide resolved
src/NuGetForUnity/Editor/Ui/NugetWindow.cs Show resolved Hide resolved
@popara96 popara96 requested a review from JoC0de October 16, 2023 09:41
@JoC0de
Copy link
Collaborator

JoC0de commented Oct 16, 2023

Nice 👍 with the screenshots. Only some portions of te readme.md need to be updated to conform with the new UI and then we can merge it.

@JoC0de
Copy link
Collaborator

JoC0de commented Oct 16, 2023

Nice 👍 you even changed more in the Readme than I expected 👍
I fixed the merge conflict + run autoformatter script so we can merge (from my side)

@popara96
Copy link
Collaborator Author

@JoC0de We had a bug where we could select different versions of a package because of differently calculated hash code, so we could have the list filled with the same package but different versions and we don't want that. It's split now and this way we also have selected packages remembered across 'Installed' tab and 'Updates' tab with updates/downgrades.

@popara96 popara96 requested a review from JoC0de October 17, 2023 09:27
Copy link
Collaborator

@JoC0de JoC0de left a comment

Choose a reason for hiding this comment

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

I fixed a bug: you didn't used selectedPackageUninstalls so uninstall selected didn't worked.
I also added switched to HashSet with custom comparer I think it is much easier to handle.
I didn't tested it 😃 , so could you pleas test it.

@popara96
Copy link
Collaborator Author

Thank you for the fix! I added the missing meta file for the new equality comparer and tested uninstall/update/downgrade selected and everything seems to work as intended 😄 so I will squash&merge after all the checks pass.

@popara96 popara96 merged commit a3b0459 into master Oct 18, 2023
@popara96 popara96 deleted the various-fixes branch October 18, 2023 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] NugetForUnity spams identical messages in the console
3 participants