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

[Peek] show thumbnail and fallback to icon on unsupported files #27979

Merged
merged 1 commit into from
Aug 24, 2023

Conversation

pedrolamas
Copy link
Contributor

@pedrolamas pedrolamas commented Aug 14, 2023

Summary of the Pull Request

Allow Peek to retrieve the thumbnail of unsupported files and use that as icon; if a thumbnail is not available, fallback to the file icon (as it currently does).

image

image

image

PR Checklist

  • Closes: #xxx
  • Communication: I've discussed this with core contributors already. If work hasn't been agreed, this work might be rejected
  • Tests: Added/updated and all pass
  • Localization: All end user facing strings can be localized
  • Dev docs: Added/updated
  • New binaries: Added on the required places
  • Documentation updated: If checked, please file a pull request on our docs repo and link it here: #xxx

Detailed Description of the Pull Request / Additional comments

While looking through this code, I've found a couple of places where some potential memory leaks could be occurring, and I have tried to fix those as I found them.

(Specifically, when ((IShellItemImageFactory)nativeShellItem).GetImage method was called, we immediately followed it by cancellationToken.ThrowIfCancellationRequested() but never destroyed the hbitmap instance from the previous!)

Validation Steps Performed

Manual validation against a couple of .gcode, .3mf, and .unknown files

@pedrolamas pedrolamas marked this pull request as draft August 14, 2023 16:30
@pedrolamas
Copy link
Contributor Author

I've converted this to draft as thumbnails transparency is not working correctly (then again, the currently in use bitmap.MakeTransparent() is not fit for purpose...)

@htcfreek
Copy link
Collaborator

For file/shortcut icons the transparency is not working very well too. But this is a bug since the first release of Peek.

@pedrolamas pedrolamas marked this pull request as ready for review August 15, 2023 09:41
@pedrolamas
Copy link
Contributor Author

I've fixed the transparency issue, I think this should work with any icon/thumbnail, though I have only tested with a couple of files.

Bitmap.MakeTransparent() is NOT the correct approach here as it does not honor any potential alpha channel, it just takes the lower left pixel of the image and considers that color as transparent...

My approach is to take the current 32bit RGB bitmap, and copy the data directly to a new 32 bit ARGB bitmap (so no change to data at all, I just assume alpha is already there) and it seems to fix the transparency issue!

Copy link
Collaborator

@stefansjfw stefansjfw left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for the contribution!

@stefansjfw stefansjfw merged commit c1316fc into microsoft:main Aug 24, 2023
7 checks passed
@pedrolamas pedrolamas deleted the pedrolamas/peek-thumbnails branch August 24, 2023 10:20
@jaimecbernardo jaimecbernardo added this to In progress in 0.73 Release via automation Sep 4, 2023
@jaimecbernardo jaimecbernardo moved this from In progress to Done in 0.73 Release Sep 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants