search

image-rs / image-png

PNG decoding and encoding library in pure Rust
https://docs.rs/png
Apache License 2.0
355 stars 140 forks source link

Merge `Filter` and `AdaptiveFilter` enums in the public API #505

Open Shnatsel opened 2 hours ago

Shnatsel commented 2 hours ago

Right now the API for encoding is kinda awkward in that we have a separate Filter and AdaptiveFilter enums that are separately applied to the encoder, and the API does not communicate that the Filter setting is ignored if AdaptiveFilter::Adaptive is also set.

By contrast, the image crate exposes the Filter as a single enum already which lists all the individual filters plus Filter::Adaptive, which I think makes for a much nicer API.

There may be cases where adaptive filtering is not an option (e.g. in decoding or in per-row APIs), in which case we can keep that as a RowFilter or some such and ideally make that enum private.

Shnatsel commented 2 hours ago

I'm happy to try and apply this change unless somebody objects, since we have some semver-breaking API changes queued already.

fintelia commented 1 hour ago

I've been wanting to make this change. Would also be nice to make the enum non-exhaustive so other adaptive filter strategies could be added in the future.

Could you make the change is the same semver-breaking PR? That way we can look at the entire new API design in one place.