Proposal for a patch for Imaging-159 using a POJO #5
Conversation
| @@ -411,12 +417,14 @@ public static ICC_Profile getICCProfile(final InputStream is, final String filen | |||
| * @param filename | |||
| * Filename associated with image data (optional). | |||
| * @param params | |||
| * Map of optional parameters, defined in ImagingConstants. | |||
| * Optional parameters, defined in ImagingParameters. | |||
There was a problem hiding this comment.
This is something we should work on after we have added ImagingParameters. IMHO if the parameter is optional, there should be a method overload that doesn't take the parameter.
|
Looks like the code from the pull request and the code in the master branch have moved into too different directions, and it can't be updated any longer. @mgmechanics let me know if you are able to update your pull request to the latest code, please. Thanks |
| * for internal use by ImageParser implementations. | ||
| * A utility method to whether we have a parameter object and if the STRICT | ||
| * flag is set or not. | ||
| * Intended for internal use by ImageParser implementations. |
There was a problem hiding this comment.
Maybe some re-wording here
"Return whether to use a strict mode or not when reading or writing images."
or similar?
| ip.setFileNameHint("Teststring"); | ||
| assertEquals("Teststring", ip.getFileNameHint()); | ||
| } | ||
| } |
| @@ -741,9 +728,11 @@ public void writeImage(final BufferedImage src, final OutputStream os, Map<Strin | |||
| * @param params | |||
| * Map of optional parameters, defined in ImagingConstants. | |||
| * @return Xmp Xml as String, if present. Otherwise, returns null. | |||
| * @throws org.apache.commons.imaging.ImageReadException | |||
| * @throws java.io.IOException | |||
There was a problem hiding this comment.
Not necessary to use fully qualified names. But doesn't hurt I guess.
| tmpReadThumbnails = Boolean.TRUE.equals(params | ||
| .get(ImagingConstants.PARAM_KEY_READ_THUMBNAILS)); | ||
| if (params != null) { | ||
| if (params instanceof ImagingParametersTiff) { |
There was a problem hiding this comment.
if statement can be combined I think.
| if (ix0 == null && iy0 == null && iwidth == null && iheight == null) { | ||
| return null; | ||
| } | ||
|
|
||
|
|
||
| // if we got at lease one of theseparameters: x, y, width or height |
There was a problem hiding this comment.
s/at lease/at least
s/theseparameters/these parameters
| @@ -334,6 +325,12 @@ public void writeImage(final BufferedImage src, final OutputStream os, Map<Strin | |||
| writer = new PamWriter(); | |||
| } | |||
| } | |||
|
|
|||
| // read parameters specific for the PNM format | |||
| if (params instanceof ImagingParametersPnm) { | |||
There was a problem hiding this comment.
Would be nice if we could use generics, instead of checking instance types. The parameters of PnmImageParser must definitely be an ImagingParametersPnm I believe.
| // text chunks are parameters specific for PNG images | ||
| // so we need to ask first if parameters specific for PNG images are provided | ||
| // than ask for text chunks | ||
| if (parameters instanceof ImagingParametersPng) { |
There was a problem hiding this comment.
Would be good if generics could be used here to avoid the instanceof, or another way to model objects.
|
|
||
| /** | ||
| * This class is a POJO holding data for parameters as requested in IMAGING-159. | ||
| * It holds additional data needed for the PCX format only. |
There was a problem hiding this comment.
Could be rewritten I think. Not necessary to say that it was requested and include what issue was.
| * Parameter used to hint the filename when reading from a byte array | ||
| * or InputStream. The filename hint can help disambiguate what file the | ||
| * image format. | ||
| * <p> |
|
Needs a rebase and pre-review to make sure this is needed: Git master has an |
Proposal for a patch for Imaging-159 using a POJO.
I already applied the POJO Parameter Object classes so that they are replace the Map<String, Object> in all classes including test code.