darkma773r commented on a change in pull request #116: URL: https://github.com/apache/commons-imaging/pull/116#discussion_r785343858
########## File path: src/main/java/org/apache/commons/imaging/internal/Util.java ########## @@ -0,0 +1,54 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.commons.imaging.internal; + +import java.util.function.Predicate; +import java.util.function.Supplier; + +import org.apache.commons.imaging.ImageFormat; +import org.apache.commons.imaging.ImageParser; +import org.apache.commons.imaging.ImagingParameters; + +/** + * Internal utilities. + * + * @since 1.0-alpha3 + */ +public class Util { + + private Util() {} + + public static <T extends ImagingParameters> ImageParser<T> getImageParser(ImageFormat format) { + return getImageParser((parser) -> parser.canAcceptType(format), () -> new RuntimeException("Unknown Format: " + format)); + } + + public static <T extends ImagingParameters> ImageParser<T> getImageParser(String fileExtension) { + return getImageParser((parser) -> parser.canAcceptExtension(fileExtension), () -> new RuntimeException("Unknown Extension: " + fileExtension)); + } + + // This generics suppression is as good as the predicate given. If the predicate violates a generics design, + // then there will be an error during runtime. + @SuppressWarnings("unchecked") + private static <T extends ImagingParameters> ImageParser<T> getImageParser(Predicate<ImageParser<?>> pred, Supplier<? extends RuntimeException> supl) { + return (ImageParser<T>) ImageParser + .getAllImageParsers() + .stream() + .filter((parser) -> pred.test(parser)) + .findFirst() Review comment: The generic parameter T is actually not needed with the way this method is being used. Removing the `<T extends ImagingParameters>` part of the signatures here and just returning `ImageParser<?>` for all of them removes the need for the warning suppression. ########## File path: src/main/java/org/apache/commons/imaging/ImageParser.java ########## @@ -563,27 +561,22 @@ public final BufferedImage getBufferedImage(final File file, final Map<String, O * <p>The params argument provides a mechanism for individual * implementations to pass optional information into the parser. * Not all formats will support this capability. Currently, - * some of the parsers do not check for null arguments. So in cases - * where no optional specifications are supported, application - * code should pass in an empty instance of an implementation of - * the map interface (i.e. an empty HashMap). + * some of the parsers do not check for null arguments.</p> Review comment: Should there be a future issue to make sure that all parsers check for null? ########## File path: src/main/java/org/apache/commons/imaging/Imaging.java ########## @@ -50,62 +48,40 @@ * * <h3>Using this class</h3> * - * <p> - * Almost all of the Apache Commons Imaging library's core functionality can + * <p>Almost all of the Apache Commons Imaging library's core functionality can * be accessed through the methods provided by this class. * The use of the Imaging class is similar to the Java API's ImageIO class, - * though Imaging supports formats and options not included in the standard - * Java API. - * </p> + * though Imaging supports formats not included in the standard Java API.</p> * - * <p> - * All of methods provided by the Imaging class are declared static. - * </p> + * <p>All of methods provided by the Imaging class are declared static.</p> * - * <p> - * The Apache Commons Imaging package is a pure Java implementation. - * </p> + * <p>The Apache Commons Imaging package is a pure Java implementation.</p> * * <h3>Format support</h3> * - * <p> - * While the Apache Commons Imaging package handles a number of different + * <p>While the Apache Commons Imaging package handles a number of different * graphics formats, support for some formats is not yet complete. * For the most recent information on support for specific formats, refer to * <a href="https://commons.apache.org/imaging/formatsupport.html";>Format Support</a> - * at the main project development web site. - * </p> + * at the main project development web site.</p> * * <h3>Optional parameters for image reading and writing</h3> * - * <p> - * Some of the methods provided by this class accept an optional - * <strong>params</strong> argument that permits the application to specify - * elements for special handling. If these specifications are not required by - * the application, the params argument may be omitted (as appropriate) or - * a null argument may be provided. In image-writing operations, the option - * parameters may include options such as data-compression type (if any), - * color model, or other format-specific data representations. The parameters - * map may also be used to provide EXIF Tags and other metadata to those - * formats that support them. In image-reading operations, - * the parameters may include information about special handling in reading - * the image data. - * </p> + * <p>Many of the operations provided in this class as static calls can be accessed directly + * using {@link ImageParser}'s. It is, however, difficult to design an API that is generic + * enough so users can safely read or write byte arrays when specifying the type of parameters, + * since the only way to confirm the parameters are valid for a specific format is by reading + * the byte array (when it is already too late.)</p> Review comment: I like this design. I think it will provide a good base for future extensions. I don't see a need to mention the design difficulties in the docs, though. The paragraph here could just be ``` Many of the operations provided in this class as static calls can be accessed directly using format-specific {@link ImageParser} instances. These static methods are provided for convenience in simple use cases. ``` This leads right into the next paragraph, in which case the two could be combined. ########## File path: src/main/java/org/apache/commons/imaging/ImageParser.java ########## @@ -955,18 +944,15 @@ protected BufferedImageFactory getBufferedImageFactory(final Map<String, Object> /** * A utility method to search a params specification and determine - * whether it contains the ImagingConstants.PARAM_KEY_STRICT - * specification. Intended + * whether it contains the parameters contain the strict flag. Intended * for internal use by ImageParser implementations. * - * @param params A valid Map object (or a null). + * @param params optional parameters. * @return If the params specify strict format compliance, true; * otherwise, false. + * @param <T> type of parameters used by this image parser */ - public static boolean isStrict(final Map<String, Object> params) { - if (params == null || !params.containsKey(ImagingConstants.PARAM_KEY_STRICT)) { - return false; - } - return ((Boolean) params.get(ImagingConstants.PARAM_KEY_STRICT)).booleanValue(); + public static <T extends ImagingParameters> boolean isStrict(final T params) { + return params.isStrict(); Review comment: This method doesn't seem very useful anymore, especially since I only see 2 places where it is used. Remove? ########## File path: src/changes/changes.xml ########## @@ -45,6 +45,21 @@ The <action> type attribute can be add,update,fix,remove. </properties> <body> <release version="1.0-alpha3" date="2020-??-??" description="Third 1.0 alpha release"> + <action issue="IMAGING-159" dev="kinow" type="fix" due-to="Bruno P. Kinoshita, Gary Lucas, Matt Juntunen"> Review comment: No problem! ########## File path: src/main/java/org/apache/commons/imaging/internal/Util.java ########## @@ -0,0 +1,73 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.commons.imaging.internal; + +import org.apache.commons.imaging.ImageFormat; +import org.apache.commons.imaging.ImageFormats; +import org.apache.commons.imaging.ImageParser; +import org.apache.commons.imaging.Imaging; +import org.apache.commons.imaging.ImagingParameters; +import org.apache.commons.imaging.common.bytesource.ByteSource; + +import java.io.IOException; +import java.util.function.Predicate; +import java.util.function.Supplier; + +/** + * Internal utilities. + * + * @since 1.0-alpha3 + */ +public class Util { + + private Util() {} + + public static <T extends ImagingParameters> ImageParser<T> getImageParser(ImageFormat format) { + return getImageParser((parser) -> parser.canAcceptType(format), () -> new RuntimeException("Unknown Format: " + format)); + } + + public static <T extends ImagingParameters> ImageParser<T> getImageParser(String fileExtension) { + return getImageParser((parser) -> parser.canAcceptExtension(fileExtension), () -> new RuntimeException("Unknown Extension: " + fileExtension)); Review comment: Should probably use `IllegalArgumentException` for consistency with line 71. Also applies to line 40. ########## File path: src/main/java/org/apache/commons/imaging/ImageFormat.java ########## @@ -33,6 +34,7 @@ * * @return String extension */ - String getExtension(); + String[] getExtensions(); Review comment: Sounds good to me! -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
