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

[PT Run]Removed BinaryFormatter #19036

Merged
merged 1 commit into from
Jul 1, 2022

Conversation

davidegiacometti
Copy link
Collaborator

Summary of the Pull Request

Replaced insicure BinaryFormatter.

PR Checklist

Detailed Description of the Pull Request / Additional comments

Replaced the BInaryFormatter with the already existing JsonStorage.

Validation Steps Performed

  • Launch PT Run from the PT Runner
  • File %LOCALAPPDATA%\Microsoft\PowerToys\PowerToys Run\Settings\ImageCache.json should be created
  • Perform some searches in PT Run
  • Close PT
  • File %LOCALAPPDATA%\Microsoft\PowerToys\PowerToys Run\Settings\ImageCache.json should be updated with cached images

@jaimecbernardo
Copy link
Collaborator

This is pretty interesting. Somehow I feel this might fix those Image.cache can't be accessed errors we get from time to time ;) There's only hoping.

@jaimecbernardo
Copy link
Collaborator

So, I look for notepad:
image

Then, I exit PowerToys, start it again, and look for it again:
image

I think the current solution is not really caching the images.

@davidegiacometti
Copy link
Collaborator Author

I will look into this

@stefansjfw stefansjfw self-requested a review June 28, 2022 16:01
@davidegiacometti
Copy link
Collaborator Author

davidegiacometti commented Jun 28, 2022

I have retested my changes.
Images are preloaded on startup from ImageCache.json.
Sometimes the program fail to extract thumbnail, but AFAIK this is an issue that I have always seen.

-------------------------- Begin exception --------------------------
Message: Failed to get thumbnail for C:\ProgramData\Microsoft\Windows\Start Menu\Programs\NAPS2\NAPS2.lnk

Exception full name  : System.Runtime.InteropServices.InvalidComObjectException
Exception message    : Error while extracting thumbnail for C:\ProgramData\Microsoft\Windows\Start Menu\Programs\NAPS2\NAPS2.lnk
Exception stack trace:
   at Wox.Infrastructure.Image.WindowsThumbnailProvider.GetHBitmap(String fileName, Int32 width, Int32 height, ThumbnailOptions options) in C:\Users\david\source\repos\PowerToys\src\modules\launcher\Wox.Infrastructure\Image\WindowsThumbnailProvider.cs:line 126
   at Wox.Infrastructure.Image.WindowsThumbnailProvider.GetThumbnail(String fileName, Int32 width, Int32 height, ThumbnailOptions options) in C:\Users\david\source\repos\PowerToys\src\modules\launcher\Wox.Infrastructure\Image\WindowsThumbnailProvider.cs:line 82
   at Wox.Infrastructure.Image.ImageLoader.LoadInternal(String path, Boolean loadFullImage) in C:\Users\david\source\repos\PowerToys\src\modules\launcher\Wox.Infrastructure\Image\ImageLoader.cs:line 183
Exception source     : Wox.Infrastructure
Exception target site: IntPtr GetHBitmap(System.String, Int32, Int32, Wox.Infrastructure.Image.ThumbnailOptions)
Exception HResult    : -2146233049

Exception full name  : System.Runtime.InteropServices.COMException
Exception message    : The data necessary to complete this operation is not yet available. (0x8000000A)
Exception stack trace:

Exception source     : 
Exception target site: 
Exception HResult    : -2147483638
-------------------------- End exception --------------------------

@jaimecbernardo are you able to reproduce it?

EDIT: the interesting thing is that the code for extract thumbnails fail only when executed in parallel. I am not able to repro the issue with a foreach.

image

@jaimecbernardo jaimecbernardo added the Needs-Review This Pull Request awaits the review of a maintainer. label Jun 29, 2022
@jaimecbernardo
Copy link
Collaborator

You're right, I'm able to repro it on 0.59.1 as well, so no regression here. 🤔
Might there be something wrong just with the cache logic in general? Either way, this is OK to go in.

Copy link
Collaborator

@jaimecbernardo jaimecbernardo left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for the contribution!

@jaimecbernardo jaimecbernardo merged commit f4dbdbd into main Jul 1, 2022
@davidegiacometti
Copy link
Collaborator Author

I will check if there is already an issue for this, if not I will open one.

@davidegiacometti davidegiacometti deleted the users/davidegiacometti/binary-storage branch July 1, 2022 18:05
@davidegiacometti davidegiacometti mentioned this pull request Jul 2, 2022
1 task
@davidegiacometti
Copy link
Collaborator Author

Created issue #19138

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs-Review This Pull Request awaits the review of a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants