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

add optional "filter" type to image resize function #2623

Open
wants to merge 3 commits into
base: next
Choose a base branch
from

Conversation

aaron404
Copy link

IMPORTANT: Please do not create a Pull Request adding a new feature without discussing it first.

The place to discuss new features is the forum: https://zola.discourse.group/
If you want to add a new feature, please open a thread there first in the feature requests section.

Sanity check:

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Code changes

(Delete or ignore this section for documentation changes)

  • Are you doing the PR on the next branch?

If the change is a new feature or adding to/changing an existing one:

  • Have you created/updated the relevant documentation page(s)?
  • I have added a new filter parameter to the resize_image function. It defaults to lanczos3 which is what was previously hardcoded. I had to add a mirror of the Filter type from the image crate to impl Hash for it. I'm not sure if there's a cleaner way to do this.
  • I added a sprite from pixelfrog that has public domain usage rights to help demonstrate the differences of the filters in the updated docs.

@aaron404
Copy link
Author

I'm not sure if I have incorrectly configured something, but it seems that running cargo test from the repo root does not execute all the tests that are used by the automated builder. I have a few test cases to fix up

@Keats
Copy link
Collaborator

Keats commented Aug 27, 2024

cargo test will only run the root crate, you need to run cargo test --all to run tests for all the crates in a workspace

"catmullrom" => FilterType::CatmullRom,
"gaussian" => FilterType::Gaussian,
_ => return Err(anyhow!("Invalid filter type: {}", filter)),
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we convert directly to the filter type from the image crate and skip our own?

Copy link
Author

Choose a reason for hiding this comment

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

I went with a custom type because FilterType from the image crate is not Hash, so it breaks downstream functionality when the processed image paths are generated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've just made a PR to impl it, let's see if it works

Copy link
Collaborator

Choose a reason for hiding this comment

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

i think it's released in the most recent version

@Keats
Copy link
Collaborator

Keats commented Dec 26, 2024

@aaron404 are you interested in updating that PR?

@aaron404
Copy link
Author

@Keats I think I have missed something. You mentioned you made a PR to impl it (I assume impl Hash for image's FilterType, but I wasn't able to find it. Can you point me to it?

@Keats
Copy link
Collaborator

Keats commented Dec 29, 2024

It's image-rs/image@02a4928
It should be available in recent image version

@aaron404
Copy link
Author

It looks like we also need FilterType to impl Eq. I have opened a PR for that

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