Closed PierreV23 closed 1 year ago
I edited the the pull request. (I didn't mean to pull both continue_with
and the reset fix at the same time, im a newbie...)
Yes, I agree a API change might be needed, but I was afraid of changing too much and I didn't want to change already existing code.
The biggest issue at the moment is the fact that ZlibDecoder::reset
uses bufread::reset_decoder_data
.
pub fn reset_decoder_data<R>(zlib: &mut ZlibDecoder<R>) {
zlib.data = Decompress::new(true);
}
It recreates an entirely new Decompress
instance without taking into account possible custom parameters (zlib_header and window_bits). This causes deflate errors. reset_with_custom
can be seen as a replacement.
continue_with
allows for a refreshment of the stream, without touching the Decompress
instance
Thanks for the examples, they are certainly a straightforward way of solving this particular issue. Due to the cost in maintenance that this additional API surface would incur, I'd like to investigate alternatives.
Can you instead try to create a
Read
implementation that allows to change an internal reader? Think along the lines ofstruct Foo(Rc<RefCell<impl std::io::Read>>)
. Then one can cloneFoo
to pass it as reader to the decompressor, but also callfoo.reset(new_reader)
on it. This has the same effect as this API, but is entirely under control of the user.If properly implemented, tested and documented, I think it could start out as part of an
example
, similar to the one you already provide.Thanks for your understanding and for considering continuing this PR despite the change in direction.
So if I understood you correctly, a new implementation of Read
gets passed to bufreader::ZlibDecoder
. Where you can directly manipulate the new Read
instead manipulating it via the ZlibDecoder?
Yes. You can implement it as part of an official example/*.rs
which could be what you presented here, but solving it with a Read
wrapper that allows to swap out the inner Read
instance.
That would mean we would not need a reset function for any of the top layer wrappers right? I was thinking of how to remove some of the boilerplate I came by. What if we just make Config struct(s) that are mandatorily passed to ::new? This would also mean we could abandon the separate gz and zlib directories, generalizing them into a single Decoder and Encoder.
We can make default instances like ZLIB_DEFAULT and GZ_DEFAULT, or use functions: Config::zlib_default() and Config::gz_default().
I think there is a lot of code that can practically be discarded, with the result of creating a new interface that is prettier, more implicit and easier to use.
It would mean a move to a new major version though.
Update: I read more into the source, but GZ is more unique than i thought. I still think the codebase could use improvement.
It's great that you are taking the time to get acquainted, this will definitely be helpful!
That would mean we would not need a reset function for any of the top layer wrappers right?
I don't know what that means, but recommend trying to do as described by me earlier. There is no need to adjust anything in this crate to get your example to work.
It would mean a move to a new major version though.
Any breaking change could be explored in an own crate, and maybe from there one could conclude that it's worth making such a change here as well. This needs a lot of experimentation, time and discussion though to assure it's the right move, it has great implications.
I think to get this PR merged it has to pivot towards a utility type and an example as described earlier, and I urge to stay on topic or close this PR and work on a new crate that shows how to do better than what you find here. Thanks for your understanding.
I think I will drop working on this now then. I am not experienced enough with Read to implement such things and I'd rather work on something else (i.e. generalized / config system).
I will take a break for now and look back once I have enough spare time again.
Good luck.
Examples:
This should help #312 and #276
NOTE: This does not affect read&write ZlibEncoder and write ZlibDecoder. These are yet to be written.