Moving Setter methods of Encoder to Info struct

[DeniseBryson]

Hi,
we thought about moving all methods that change properties of Info within the encoder, such as this one:

    /// Sets the raw byte contents of the PLTE chunk. This method accepts
    /// both borrowed and owned byte data.
    pub fn set_palette<T: Into<Cow<'a, [u8]>>>(&mut self, palette: T) {
        self.info.palette = Some(palette.into());
    }

Since they changes properties of Info it seems more intuitive for them to be methods of the info struct.

Is there something we missing? Would this be an improvement?
If yes I could prepare a pull request, copying all methods to info and marking the methods of the encoder as deprecated?

Would be happy to hear your thoughts on this. :)

[fintelia]

I don't think that would be an improvement. If anything, users would have to write slightly more code. And making the change would require any existing user to update their own code.

Right now info is a private field within the encoder, so users generally (EDIT: don't) need to think about it

[hoijui]

... users generally need to think about it

what do you mean with this?

... ah you mean.. they have to setup their Info, calling methods on it, and then they have to remember to set that info on the encoder?
I can not see how they could forget about that. setting up the Info and then not using it, would produce a compiler warning (unused value or the like).

Also, if the current functions only get deprecated instead of removed, which is in the above proposal, then current users do not have to update their code.

The main improvement that I see, is that in the future, the functions on the encoder can be removed (in a major release), and then one has to setup Info before giving it to the Encoder, and then the settings could not be changed anymore after initiating the encoder, which means one can not change any settings/infos after starting to encode, which .. I would say, should be illegal anyway, either because it would create an undefined situation, or because it could potentially create an invalidly encoded image.

[fintelia]

Currently the way users generally use the API is:

let mut encoder = png::Encoder::new(w, 100, 100);
encoder.set_color(png::ColorType::Rgba);
encoder.set_depth(png::BitDepth::Eight);
encoder.set_source_gamma(png::ScaledFloat::from_scaled(45455));

let mut writer = encoder.write_header().unwrap();
writer.write_image_data(&data).unwrap(); // Save

With your proposed changes they'd instead do:

let mut info = png::Info::with_size(100, 100);
info.set_color(png::ColorType::Rgba);
info.set_depth(png::BitDepth::Eight);
info.set_source_gamma(png::ScaledFloat::from_scaled(45455));

let mut encoder = png::Encoder::with_info(w, info);
let mut writer = encoder.write_header().unwrap();
writer.write_image_data(&data).unwrap(); // Save

To me, that looks basically the same.

the settings could not be changed anymore after initiating the encoder, which means one can not change any settings/infos after starting to encode

This is already the case. Notice that the call to write_header consumes the Encoder and produces a Writer. After doing so, it is no longer possible to modify the settings/info

[hoijui]

ahh indeed! thanks for that explanation (encoder gets consumed).

I personally think. it would be better and more flexible to have the settings separated from the encoder. one could more easily write many files with the same settings, or just separate the settings from the actual writing, logically. It would have been good to have this API from the start, but I also understand, that the benefit is not massive, and the deprecation of these methods in encoder should probably not happen.
Also, one can already set the Info struct to ones linking before handing it to the Encoder, as of now.
What is missing however, is the error-checking/validation of the settings in Info, when setting them directly in the Info struct, vs setting them through the Encoder::set_* methods; for example:

impl<'a, W: Write> Encoder<'a, W> {
    pub fn set_animated(&mut self, num_frames: u32, num_plays: u32) -> Result<()> {
        if num_frames == 0 {
            return Err(EncodingError::Format(FormatErrorKind::ZeroFrames.into()));
        }

        let actl = AnimationControl {
            num_frames,
            num_plays,
        };

        let fctl = FrameControl {
            sequence_number: 0,
            width: self.info.width,
            height: self.info.height,
            ..Default::default()
        };

        self.info.animation_control = Some(actl);
        self.info.frame_control = Some(fctl);
        Ok(())
    }
}

What do you think about moving that to the Info struct instead, and then just forward the function in Encoder to the Info one (without deprecation)?
All the Info structs variables would still have to be pub because of the Decoder and for reading and backwards compatibility in user code, however.

[fintelia]

If we need to do validation of info fields, I think that has to happen when the Decoder is constructed. Since the Info struct has public fields, any validation we did using setters would be trivially bypassed. I just created #442 to correct this.