darkma773r commented on pull request #116:
URL: https://github.com/apache/commons-imaging/pull/116#issuecomment-958597172
@kinow
> If the JpegImageParser requires a parameter different than the
TiffImageParser, I assume we would have to create a new JpegImagingParameter.
In that case, wouldn't we have to keep the old constructor for backward
compatibility until a new major release?
I'm not totally sure what you mean here. Can you give an example?
On a related note, in my merge request, I removed all of the parameter
classes that did not actually have any of their own properties (such as
JpegImageParameters). I'm not sure if this is a great idea, though, since it
makes it it kind of hard to tell what parameters to use when creating images of
a certain type. If might be better to add them back just for the convenience of
not having to look in the documentation to find what parameters class to use.
For example, if you are creating a JPEG, you automatically know that you need a
JpegImageParameters; BMP implies BmpImageParameters, etc.
One idea for making this readily available in the API would be to add
convenience factory methods in Imaging for creating instances of each
parameters type. Ex:
```
public static BmpImageParameters bmpParameters() { return new
BmpImageParameters(); }
public static TiffImageParameters tiffParameters() { return new
TiffImageParameters(); }
```
This way, users would be able to find the parameters class they want by
looking at a single class. The methods would return the specific
ImageParameters subclass so users could immediately access all of the available
properties.
Side note: Is there a reason the JpegImageParser uses TiffImageParameters?
The two formats are not related, correct? Perhaps we could have a common base
class for them?
> I think it would be best if cases like this were not allowed
PcxImagingParameters parser = new PcxImagingParameters(new
TiffImagingParameters()); // no error in compile or runtime
Is the idea that it is confusing on what properties are copied over? My
thought is that the copy constructor copies over as many fields as it can and
ignores the rest. For example, say you are writing an image out in multiple
formats. You could create a very specific parameter instance and then use the
copy constructors to copy relevant fields for the other formats.
```
TiffImageParameters tiffParams = new TiffImageParameters();
// populate params...
// create images
Imaging.writeImage(src, fileA, ImageFormats.TIFF, tiffParams);
Imaging.writeImage(src, fileB, ImageFormats.BMP, new
BmpImageParameters(tiffParams));
Imaging.writeImage(src, fileC, ImageFormats.PNG, new
PngImageParameters(tiffParams));
```
> I think you removed the option to allow null parameters in the
Imaging#writeImage method (and other methods). I thought about doing that too,
that would remove one generic suppress I think, but there are unit tests and
examples. The API has allowed it since Sanselan, so not too sure about
enforcing parameters to be non-null now.
I attempted to retain the null parameter functionality. In the
`normalizeParameters` method, if a null parameter is passed, a default instance
is created and returned, allowing downstream code to bypass null checks. `mvn
clean install` passes with my current setup. Is there a unit test I missed?
> I liked the error in this one new PngImageParser().writeImage(null, null,
new TiffImagingParameters());, which gives java.lang.IllegalArgumentException:
Invalid imaging parameters: expected type...
Thanks! :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@commons.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
