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

Update Scoop support #402

Merged
merged 8 commits into from
Sep 2, 2022
Merged

Conversation

davidxuang
Copy link
Contributor

@davidxuang davidxuang commented Aug 17, 2022

Closes #401


  • Parse JSON export format
    • Custom DateTimeOffset converter for Windows PowerShell compatibility
    • Set InstallDate via Updated
    • Set InstallSource via Source
    • Set RatingId via Source
  • Parse app install and manifest JSON file
    • Custom string[] converter for bin and env_add_path
    • Set AboutUrl via homepage
    • Set SortedExecutables via shortcuts, bin, and env_add_path
      • architecture awareness
    • Set IconBitmap via shortcuts with explicit ico file
  • JsonSerializerContext support for accelerating JSON parsing

@davidxuang
Copy link
Contributor Author

davidxuang commented Aug 17, 2022

Should be working now, but not sure if bucket entry should be parsed (for InstallSource or RatingId maybe). Also the code is still a bit messy.

图片

@Klocman
Copy link
Owner

Klocman commented Aug 17, 2022

Thank you for the PR! I'll review it in detail and merge when I get the chance.

  • Set InstallSource via Source
  • Set RatingId via Source

InstallSource should be a path, so using value of Source won't work as it is. RatingId should end up being the same as it was in the old version so that user ratings are not lost.

@davidxuang
Copy link
Contributor Author

InstallSource should be a path, so using value of Source won't work as it is. RatingId should end up being the same as it was in the old version so that user ratings are not lost.

Source could be $env:SCOOP\buckets\$($bucket)\bucket, though it's a bit strange if file manager don't scroll to the specific JSON file, since there isn't such property as InstallerName.

Copy link
Owner

@Klocman Klocman left a comment

Choose a reason for hiding this comment

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

Everything looks to be working correctly, I only noticed one small issue.

Source could be $env:SCOOP\buckets\$($bucket)\bucket, though it's a bit strange if file manager don't scroll to the specific JSON file, since there isn't such property as InstallerName.

InstallSource can be left empty in this case. RatingId stays the same. In that case the PR looks good to merge after you comment on this suggestion (or apply it as it is).

Also, what do you think about #388? --purge could be added to all Scoop uninstall commands.

source/UninstallTools/Factory/ScoopFactory.cs Outdated Show resolved Hide resolved
Co-authored-by: Marcin Szeniak <14913904+Klocman@users.noreply.github.com>
@davidxuang
Copy link
Contributor Author

davidxuang commented Sep 1, 2022

InstallSource can be left empty in this case. RatingId stays the same. In that case the PR looks good to merge after you comment on this suggestion (or apply it as it is).

This is fine since opening the source JSON file doesn't seems helpful in most cases.

Also, what do you think about #388? --purge could be added to all Scoop uninstall commands.

I'd say "persist" should be deleted only when explicitly opted in. Also the persist are quite easy to locate, so maybe just add it to clean-up candidates.

@Klocman Klocman merged commit 1d9abaa into Klocman:master Sep 2, 2022
@davidxuang
Copy link
Contributor Author

Ahh, I notice that there might be a typo/misnaming. (isPublic should be isGlobal)

@Klocman
Copy link
Owner

Klocman commented Sep 4, 2022

It doesn't really matter, in this context it means the same thing.

@davidxuang davidxuang mentioned this pull request Oct 6, 2022
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.

Scoop app detection has been broken
2 participants