On Fri, 19 Jun 2020 16:04:16 GMT, Rony G. Flatscher
wrote:
>> This PR adds a "compile" process instruction to FXML files with the optional
>> PI data "true" (default) and "false". The
>> PI data is turned into a boolean value using "Boolean.parseBoolean(String)".
>> This makes it possible to inject the compile PI everywhere in a FXML file
>> and turn on and off compilation of scripts if
>> the scripting engine implements the javax.script.Compilable interface. The
>> PR adds the ability for a fallback in case
>> compilation of scripts fails, in which case a warning gets issued about this
>> fact and evaluation of the script will be
>> done without compilation. Because of the fallback scripts get compiled with
>> this version by default.
>> -
>> ### Progress
>> - [x] Change must not contain extraneous whitespace
>> - [x] Commit message must refer to an issue
>> - [ ] Change must be properly reviewed
>>
>> ### Issue
>> * [JDK-8238080](https://bugs.openjdk.java.net/browse/JDK-8238080):
>> FXMLLoader: if script engines implement
>>javax.script.Compilable compile scripts
>>
>>
>> ### Download
>> `$ git fetch https://git.openjdk.java.net/jfx pull/192/head:pull/192`
>> `$ git checkout pull/192`
>
> Rony G. Flatscher has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Incorporating Kevin's review comments (final int, fix for loop test,
> correct formatting).
The API changes look good. Also, the CSR has been approved.
The code changes in FXMLLoader look good.
The newly added tests pass with your fix.
I also verified that at least some of the newly added tests fail without fix
(good)
Most of the rest of the comments are on formatting and code style, so this
looks about ready to go in.
modules/javafx.fxml/src/main/docs/javafx/fxml/doc-files/introduction_to_fxml.html
line 1114:
> 1113:
> 1114:
Extra blank line added (should be removed)
modules/javafx.fxml/src/main/java/javafx/fxml/FXMLLoader.java line 1575:
> 1574: do {
> 1575: n = scriptReader.read(charBuffer,0,bufSize);
> 1576: if (n > 0) {
Indentation should be 4 spaces
modules/javafx.fxml/src/main/java/javafx/fxml/FXMLLoader.java line 1594:
> 1593: } catch (ScriptException compileExc) {
> 1594:
> Logging.getJavaFXLogger().warning(filename+": compiling caused
> \""+compileExc+"\",
> falling back to evaluating script in uncompiled mode"); 1595:
> }
Minor: add space before and after `+` (and maybe wrap since the line is getting
rather long)
modules/javafx.fxml/src/main/java/javafx/fxml/FXMLLoader.java line 1605:
> 1604: } catch (ScriptException exception) {
> 1605: System.err.println(filename+": caused
> ScriptException");
> 1606: exception.printStackTrace();
Minor: add space before and after `+`
modules/javafx.fxml/src/main/java/javafx/fxml/FXMLLoader.java line 1633:
> 1632: } catch (ScriptException compileExc) {
> 1633:
> Logging.getJavaFXLogger().warning(filename+": compiling caused
> \""+compileExc+"\",
> falling back to evaluating script in uncompiled mode"); 1634:
> }
Minor: add space before and after `+` (and maybe wrap since the line is getting
rather long)
modules/javafx.fxml/src/main/java/javafx/fxml/FXMLLoader.java line 1644:
> 1643: } catch (ScriptException exception) {
> 1644: System.err.println(filename+": caused
> ScriptException\n"+exception.getMessage());
> 1645: }
Minor: add space before and after `+`
modules/javafx.fxml/src/main/java/javafx/fxml/FXMLLoader.java line 1737:
> 1736: public CompiledScript compiledScript;
> 1737: public boolean isCompiled=false;
> 1738:
Add space before and after `=`
modules/javafx.fxml/src/main/java/javafx/fxml/FXMLLoader.java line 1750:
> 1749:} catch (ScriptException compileExc){
> 1750: Logging.getJavaFXLogger().warning(filename+":
> compiling caused \""+compileExc+"\", falling
> back to evaluating script in uncompiled mode"); 1751:}
Minor: add space before and after `+` (and maybe wrap since the line is getting
rather long)
modules/javafx.fxml/src/main/java/javafx/fxml/FXMLLoader.java line 1772:
> 1771: } catch (ScriptException exception){
> 1772: throw new RuntimeException(filename+": caused
> ScriptException", exception);
> 1773: }
Minor: add space before and after `+`
modules/javafx.fxml/src/main/java/javafx/fxml/FXMLLoader.java line 2757:
> 2756: } else if (piTarget.equals(COMPILE_PROCESSING_INSTRUCTION)) {
> 2757: String strCompile=xmlStreamReader.getPIData().trim();
> 2758: // if PIData() is
