-
Notifications
You must be signed in to change notification settings - Fork 618
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: make set_icc_profile function available #2322
Conversation
I think this should probably be added as a method on |
4afeeca
to
562cfbb
Compare
@fintelia I've changed it the way you suggested |
src/image.rs
Outdated
fn set_icc_profile(&mut self, icc_profile: Vec<u8>) { | ||
let _ = icc_profile; | ||
panic!("ICC profiles are not supported for this format"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @fintelia meant an error, not a panic. We're already seeing bug reports for returning honest and correct error returns when calling functions on formats that do not support them (e.g. encoding floats into a png). I can't imagine error handling by panic to fare any better, it's probably significantly worse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should return Result<(), …>
.
I'm not sure if a generic error or a dedicated UnsupportedError
.
it would be good to also document that it doesn't check ICC for validity at this point, so doesn't guarantee to fail if the profile is invalid.
@kornelski Now it returns an Result |
Looks like there's a few compile errors:
|
src/image.rs
Outdated
/// # Panics | ||
/// | ||
/// Panics if the ICC profile is not implemented for the format. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update
Thanks! |
We need to set the icc profile for webp. This PR makes the set_icc_profile function available from the outside.