Proposal for a patch for Imaging-159 using a POJO #5
Conversation
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()); | ||
| } | ||
| } No newline at end of file |
| * 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 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
| } | ||
|
|
||
| // 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 |
4 participants
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.