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

Added the ReadmePreviewViewModel & edited NuGetPackageFileService #6029

Merged

jgonz120 merged 11 commits into dev-feature-ReadMeRenderingInPMUI from dev-jgonz120-AddReadmeViewmodel

Oct 7, 2024

Contributor

jgonz120 commented Sep 13, 2024

… readme

Bug

Part of: NuGet/Home#12583

Description

Adds a new ViewModel for getting the README contents, leveraging the NuGetPackageFileService to get the file from the package.

PR Checklist

Meaningful title, helpful description and a linked NuGet/Home issue
Added tests
~~Link to an issue or pull request to update docs if this PR changes settings, environment variables, new feature, etc.~~

jgonz120 requested a review from a team as a code owner

September 13, 2024 21:46

nkolev92 added the Merge next release label

Contributor Author

jgonz120 commented Sep 23, 2024

@nkolev92 this is going to a feature branch so I don't think there's a worry about merging.

jgonz120 removed the Merge next release label

jgonz120 requested review from jebriede and donnie-msft

September 23, 2024 21:10

jebriede requested changes

View reviewed changes

...uGet.Clients.Tests/NuGet.PackageManagement.UI.Test/ViewModels/ReadmePreviewViewModelTests.cs Outdated Show resolved Hide resolved

...uGet.Clients.Tests/NuGet.PackageManagement.UI.Test/ViewModels/ReadmePreviewViewModelTests.cs Outdated Show resolved Hide resolved

src/NuGet.Clients/NuGet.PackageManagement.UI/ViewModels/ReadmePreviewViewModel.cs Show resolved Hide resolved

src/NuGet.Clients/NuGet.PackageManagement.UI/ViewModels/ReadmePreviewViewModel.cs Show resolved Hide resolved

src/NuGet.Clients/NuGet.PackageManagement.UI/ViewModels/ReadmePreviewViewModel.cs Outdated

+                                      newReadmeValue = readme;
+                                  }
+                              }
+                              await NuGetUIThreadHelper.JoinableTaskFactory.SwitchToMainThreadAsync(cancellationToken);

Contributor

jebriede Sep 26, 2024

We shouldn't switch to the main thread at the end of this method.

Contributor

donnie-msft Sep 27, 2024

I don't think updating properties on a background thread will work with WPF.
Jonatan, what happens if you remove the switch to main thread? I'm guessing either no UI update or an exception?

Contributor Author

jgonz120 Sep 27, 2024

From what I remember I got an exception when I tried to set the view model properties without switching to the main thread. I can remove it and we can see what happens in the next pr, it looks like the unit test passes either way

Contributor

jebriede Sep 27, 2024 •

edited

Loading

A better way to handle this is to use the Dispatcher to check if we're on the main thread or to marshal to the main thread.

Explicitly switching to the main thread is expensive and error-prone because it forces this background operation onto the main thread where it will remain, even when returning to the caller.

See: https://stackoverflow.com/questions/590590/making-%20sure-onpropertychanged-is-called-on-ui-thread-in-mvvm-wpf-app

src/NuGet.Clients/NuGet.PackageManagement.UI/ViewModels/ReadmePreviewViewModel.cs Outdated Show resolved Hide resolved

src/NuGet.Clients/NuGet.PackageManagement.UI/ViewModels/ReadmePreviewViewModel.cs Outdated Show resolved Hide resolved

src/NuGet.Clients/NuGet.PackageManagement.UI/ViewModels/ReadmePreviewViewModel.cs Outdated Show resolved Hide resolved

src/NuGet.Clients/NuGet.PackageManagement.UI/ViewModels/ReadmePreviewViewModel.cs Outdated Show resolved Hide resolved

src/NuGet.Clients/NuGet.PackageManagement.UI/ViewModels/ReadmePreviewViewModel.cs Outdated Show resolved Hide resolved

donnie-msft approved these changes

View reviewed changes

Contributor

donnie-msft left a comment

Thanks for making smaller PRs for this effort! I would reword the title since it also updates the file service and not just the VM. Approving with suggestions.

src/NuGet.Clients/NuGet.PackageManagement.UI/ViewModels/ReadmePreviewViewModel.cs Outdated Show resolved Hide resolved

src/NuGet.Clients/NuGet.PackageManagement.UI/ViewModels/ReadmePreviewViewModel.cs Outdated

+                                      newReadmeValue = readme;
+                                  }
+                              }
+                              await NuGetUIThreadHelper.JoinableTaskFactory.SwitchToMainThreadAsync(cancellationToken);

Contributor

donnie-msft Sep 27, 2024

I don't think updating properties on a background thread will work with WPF.
Jonatan, what happens if you remove the switch to main thread? I'm guessing either no UI update or an exception?

src/NuGet.Clients/NuGet.PackageManagement.VisualStudio/Services/NuGetPackageFileService.cs

+                          Assumes.NotNull(readmeUri);
+                          Stream? stream;
+                          if (IsEmbeddedUri(readmeUri))

Contributor

donnie-msft Sep 27, 2024

I recently changed the Icon algorithm to check for a local file first, then a remote file.
Do you think this is a good opportunity to do that for README? That is, check for the md file in GPF first, and only if not found, go to the remote?

Contributor Author

jgonz120 Sep 27, 2024

I don't think that will work for the Readme, I want to decide which source it's being loaded from. We don't want to display README in the browse tab if they aren't available from the remote source.

Contributor

donnie-msft Sep 27, 2024

I quickly re-read the draft spec and didn't see anything obvious indicating why READMEs should only be downloaded and not read from disk. https://github.com/NuGet/Home/pull/13200/files?short_path=6c7eb8e#diff-6c7eb8e1ec6b63a9782e440a235ee32bc0d10f4b7ec8a718f3d24a53660f679c

Could you elaborate on why "it won't work"? You can see the source that populated the GPF via the .nupkg.metadata file.

We don't want to display README in the browse tab if they aren't available from the remote source.
Great thing to explain the spec. Or if it's already there, please give me a pointer to that.

Contributor Author

jgonz120 Oct 7, 2024

To clarify the requirement. We only want to display README from local storage in the installed tab. That way if there isn't a remote source setup with README exposed, they won't have as much of a inconsistent README experience in the browse tab.

So I feel like this solution won't work because it's leaving it up to the PackageFileService to determine where to pull the README from, but it doesn't have the context if we're viewing the details from the Browse or Installed tabs

Contributor

donnie-msft Oct 7, 2024

I'm not seeing this explicitly in the Spec. I suggest updating that. I'd like to discuss the design, and the spec would be the best place for that.

...nts.Tests/NuGet.PackageManagement.VisualStudio.Test/Services/NuGetPackageFileServiceTests.cs Outdated Show resolved Hide resolved

jgonz120 changed the title ~~Added the ReadmePreviewViewModel~~ Added the ReadmePreviewViewModel & Edited NuGetPackageFileService

jgonz120 changed the title ~~Added the ReadmePreviewViewModel & Edited NuGetPackageFileService~~ Added the ReadmePreviewViewModel & edited NuGetPackageFileService

jebriede approved these changes

View reviewed changes

jgonz120 force-pushed the dev-jgonz120-AddReadmeViewmodel branch from 7338389 to 8581b86 Compare

October 7, 2024 16:36

jgonz120 force-pushed the dev-feature-ReadMeRenderingInPMUI branch from 04b7b97 to 12ae5e7 Compare

October 7, 2024 16:41

jgonz120 and others added 11 commits

October 7, 2024 09:42


          Added the ReadmePreviewViewModel and updated file service to load the…

1e09627

… readme


          Added unit test for new readme function

ce47120


          Added unit test for viewmodel

90d9938


          Update test/NuGet.Clients.Tests/NuGet.PackageManagement.UI.Test/ViewM…

65639f2

…odels/ReadmePreviewViewModelTests.cs

Co-authored-by: Jean-Pierre Briedé <jebriede@microsoft.com>


          Update test/NuGet.Clients.Tests/NuGet.PackageManagement.UI.Test/ViewM…

e4dc439

…odels/ReadmePreviewViewModelTests.cs

Co-authored-by: Jean-Pierre Briedé <jebriede@microsoft.com>


          Rename tests

77526dd


          remove extra line

611b3bd


          pr feedback

c2b0a5c


          PR suggestions


          More pr notes

5995c83


          Update test

75a56db

jgonz120 force-pushed the dev-jgonz120-AddReadmeViewmodel branch from 8581b86 to 75a56db Compare

October 7, 2024 16:43

jgonz120 merged commit 70823bf into dev-feature-ReadMeRenderingInPMUI

28 checks passed

jgonz120 deleted the dev-jgonz120-AddReadmeViewmodel branch

October 7, 2024 20:42

donnie-msft mentioned this pull request

Add Tech Spec for rendering README in PM UI NuGet/Home#13200

Merged

jgonz120 added a commit that referenced this pull request


          Added the ReadmePreviewViewModel & edited NuGetPackageFileService (#6029

3295ed1

)

Adds a new ViewModel for getting the README contents, leveraging the NuGetPackageFileService to get the file from the package.

---------

Co-authored-by: Jean-Pierre Briedé <jebriede@microsoft.com>

jgonz120 added a commit that referenced this pull request


          Added the ReadmePreviewViewModel & edited NuGetPackageFileService (#6029

ba123e6

)

Adds a new ViewModel for getting the README contents, leveraging the NuGetPackageFileService to get the file from the package.

---------

Co-authored-by: Jean-Pierre Briedé <jebriede@microsoft.com>

jgonz120 added a commit that referenced this pull request


          Added the ReadmePreviewViewModel & edited NuGetPackageFileService (#6029

575b1e8

)

Adds a new ViewModel for getting the README contents, leveraging the NuGetPackageFileService to get the file from the package.

---------

Co-authored-by: Jean-Pierre Briedé <jebriede@microsoft.com>

jgonz120 mentioned this pull request

[Feature] Render package README from local storage in the PM UI #6110

Merged

3 tasks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet