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

Adds QOI support to Peek #29919

Merged
merged 8 commits into from
Dec 18, 2023
Merged

Conversation

pedrolamas
Copy link
Contributor

Summary of the Pull Request

Adds QOI image files support to Peek!

Light theme

image

Dark theme

image

PR Checklist

Detailed Description of the Pull Request / Additional comments

A couple of notes on this PR:

  • I moved the QoiImage and Gcode* classes from PreviewHandlerCommon to FilePreviewCommon, and by doing that, I had to enable <UseWindowsForms>, <AllowUnsafeBlocks> and <Nullable> on the project.
  • As I enabled nullable types on the step above, I fixed all the nullable issues on FilePreviewCommon
  • FilePreviewCommon has been added as a project dependency to:
    • GcodePreviewHandler
    • GcodeThumbnailProvider
    • QoiPreviewHandler
    • QoiThumbnailProvider
  • IFileSystemItemExtensions.GetQoiSize is just going directly to the bytes where we expect the "width" and "height" of a QOI image to be. I did not add a file header validation, but that should be rather trivial to do as it is already implemented in QoiImage.
  • BitmapHelper.GetBitmapFromHBitmapAsync has been refactored so that I could extract the code that converts a System.Drawing.Bitmap to a Microsoft.Xaml...BitmapSource <- you might want to take a good look at this one to make sure all is ok!

I understand that Peek can now show Preview Pane Handlers directly, but considering #26726, I think it best to prepare to a situation where PT stops supporting the Preview Pane Handlers completely

Validation Steps Performed

Manual validation with the test images pack in https://qoiformat.org/

@htcfreek htcfreek added the Needs-Review This Pull Request awaits the review of a maintainer. label Nov 19, 2023
@htcfreek
Copy link
Collaborator

@pedrolamas
Isn't it ready? Then we can remove the review label. Thought it was ready.

@pedrolamas
Copy link
Contributor Author

@htcfreek from my part, this PR is ready to go!

@jaimecbernardo jaimecbernardo added revisit-0.77 and removed Needs-Review This Pull Request awaits the review of a maintainer. labels Nov 23, 2023
@jaimecbernardo
Copy link
Collaborator

Thanks for opening the PR, @pedrolamas .
At this moment, we're thinking of reviewing the contribution for 0.77 only. Sorry for the delay. 😄

@pedrolamas
Copy link
Contributor Author

No problem, thanks for letting me know, @jaimecbernardo

I will try and keep the branch in sync with main branch to make sure it does not have conflicts.

@stefansjfw
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

# Conflicts:
#	src/modules/peek/Peek.FilePreviewer/Previewers/MediaPreviewer/ImagePreviewer.cs
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.

Hi @pedrolamas ,
Thank you for the contribution!
I've tested and it seems to look good, but it seems to bring a regression:
image
When the preview pane is on a qoi file, you can't delete that file. This was OK in 0.76.0, it seems. Can you please look into it? I suspect the refactor caused some resources to not be freed correctly.

@jaimecbernardo
Copy link
Collaborator

I've merged latest main in and added QOI to the notice section about Peek as well. Running a Dart CI test to check if it still builds OK for release.

@pedrolamas
Copy link
Contributor Author

When the preview pane is on a qoi file, you can't delete that file.

Thanks @jaimecbernardo, I have a feeling this is due to the fact I changed the BinaryReader in QoiReader to leave the stream open (in theory that makes sense to me as I would expect the stream to be closed upstream, but maybe that is not the case?)

@pedrolamas
Copy link
Contributor Author

@jaimecbernardo I have now fixed the problem by adding a using to the file stream

https://github.com/microsoft/PowerToys/pull/29919/files#diff-1d420d0e6cfb94016296228da268defbf9677c8414f378b21b45abb19ce22779R66

For the record, my thinking here is that if the Image.FromStream method leaves the stream open, I think it is reasonable to expect the same behavior from QoiImage.FromStream, hence why I changed the BinaryReader to leave it open.

Disposing the FileStream in this case solves the problem very nicely, and I would argue that it is reasonable that (where appropriate) the same change should be made on ALL other preview handlers, what do you think?

@jaimecbernardo
Copy link
Collaborator

Solves it for me. Not sure 🤔 . The other previewers don't seem to be suffering from the same issue.

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 4c3e534 into microsoft:main Dec 18, 2023
10 checks passed
@pedrolamas pedrolamas deleted the pedrolamas/peek-qoi branch December 18, 2023 16:15
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.

4 participants