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

🐛 Fix various thumbnail generation issues #381

Merged
merged 9 commits into from
Aug 5, 2024

Conversation

aaronleopold
Copy link
Collaborator

There are a good amount of other changes in this diff, e.g. upgrading the rust version, some crates, clippy fixes, etc:

  • Upgrade image-related crates
    • This required upgrading rust version to 1.79.0 (one minor behind the very recent stable). Multiple lint issues surfaced after the upgrade
  • Add ProcessorError to (eventually) replace the FileError usage throughout the image processors. This was mostly because not all errors during image processing are file errors, and it would feel out of place to add them to FileError
  • Added test images for all supported (and future supported) images for various tests
  • Added tests to assert GenericImageProcessor behavior
  • Added tests to assert WebpProcessor behavior
  • Added tests where easily addable (e.g. user permissions, etc)
  • Minimally improve client-side validation and reporting for invalid thumbnail configuration fields
    • The UI combined with the intersection types makes it a bit awkward, but is alright for now IMO
  • Add a validate fn to the ImageProcessorOptions struct to enforce valid configurations server-side
  • Remove remove_thumbnails_of_type and replace the single usage with remove_thumbnails to fix the issue where regenerating/removing thumbnails doesn't clean up thumbnails of differing formats than the configured format

There are a good amount of other changes in this diff, e.g. upgrading the rust version, some crates, clippy fixes, etc.
@aaronleopold
Copy link
Collaborator Author

I'm aiming to have time tomorrow to re-validate the fixes in this PR, then I'll merge it in. I'll start prepping the next versioned release within a week or so after that point.

@aaronleopold
Copy link
Collaborator Author

I just tested the following without issue:

  • Updating library thumbnail config, initialize regeneration (keeping old thumbs)
  • Delete all thumbnails (old and new thumbs affected, different formats)
  • PNG, JPEG, WebP generation without issue, individually

Added a small fix to correctly render the library settings form according to fresh update

@aaronleopold aaronleopold marked this pull request as ready for review August 4, 2024 19:25
Copy link
Collaborator

@JMicheli JMicheli left a comment

Choose a reason for hiding this comment

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

This looks awesome - no issues on my end when testing.

I have a minor suggestion in my review comments both otherwise LGTM.

core/src/filesystem/image/process.rs Outdated Show resolved Hide resolved
core/src/filesystem/image/process.rs Outdated Show resolved Hide resolved
core/src/filesystem/image/process.rs Outdated Show resolved Hide resolved
@aaronleopold aaronleopold merged commit 7f09503 into develop Aug 5, 2024
6 checks passed
@aaronleopold aaronleopold deleted the al/thumbnail-fixes branch August 5, 2024 15:34
@aaronleopold aaronleopold mentioned this pull request Aug 15, 2024
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.

2 participants