RFR: 8238080: FXMLLoader: if script engines implement javax.script.Compilable compile scripts [v4]
Kevin Rushforth
kcr at openjdk.java.net
Fri Jun 26 00:01:59 UTC 2020
On Fri, 19 Jun 2020 16:04:16 GMT, Rony G. Flatscher <github.com+60214806+ronyfla at openjdk.org> 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: </html>
> 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 empty string then default to true, otherwise use Boolean.parseBoolean(string) to
> determine the boolean value
Add space before and after `=`
modules/javafx.fxml/src/main/java/javafx/fxml/FXMLLoader.java line 2759:
> 2758: // if PIData() is empty string then default to true, otherwise use Boolean.parseBoolean(string) to
> determine the boolean value 2759: compileScript = (strCompile.length()==0 ? true :
> Boolean.parseBoolean(strCompile)); 2760: }
Add space before and after `==`
modules/javafx.fxml/src/main/java/javafx/fxml/FXMLLoader.java line 3638:
> 3637:
> 3638:
Extra 2 blank lines were added (should be removed)
tests/system/src/test/java/test/launchertest/ModuleLauncherTest.java line 310:
> 309:
> 310:
Extra 2 blank lines were added (should be removed)
tests/system/src/testscriptapp2/java/mymod/myapp2/FXMLScriptDeployment2Compile_Fail_Compilation.java line 110:
> 109: btn.fireEvent(new MouseEvent(MouseEvent.MOUSE_CLICKED,
> 110: 0, // double x,
> 111: 0, // double y,
Optional: you may want to align this and successive lines under `MouseEvent.MOUSE_CLICKED,` (it's up to you).
tests/system/src/testscriptapp2/java/mymod/myapp2/FXMLScriptDeployment2Compile_Fail_Compilation.java line 174:
> 173:
> 174: for (Integer invocation = 1; invocation <= invocationList.size(); invocation++) {
> 175: InvocationInfos entry = (InvocationInfos) invocationList.get(invocation - 1);
I think this should be `int` instead of `Integer`.
tests/system/src/testscriptapp2/java/mymod/myapp2/FXMLScriptDeployment2Compile_Fail_Compilation.java line 271:
> 270:
> Util.assertEndsWith("demo_03_fail_compile.fxml-onAction_attribute_in_element_ending_at_line_46", filename);
> 271: Util.assertStartsWith("demo_03_fail_compile.fxml embedded event - ActionEvent - line # 45 - LF
> entity (
) forces linebreak in attribute value:", script); 272: break;
You might want to consider wrapping some of the longer lines?
tests/system/src/testscriptapp2/java/mymod/myapp2/FXMLScriptDeployment2Compile_Fail_Compilation.java line 288:
> 287:
> 288:
There are two extra blank lines in this file that can be removed.
tests/system/src/testscriptapp2/java/mymod/myapp2/FXMLScriptDeployment2Compile_Off.java line 60:
> 59: */
> 60: public class FXMLScriptDeployment2Compile_Off extends Application {
> 61:
Most of the comments I made in the previous test class apply in this and the other tests, too.
tests/system/src/testscriptapp2/java/mymod/pseudoScriptEngineCompilable/InvocationInfos.java line 78:
> 77: }
> 78:
Extra blank line can be removed.
tests/system/src/testscriptapp2/java/mymod/pseudoScriptEngineCompilable/RgfPseudoCompiledScript.java line 35:
> 34: public class RgfPseudoCompiledScript extends CompiledScript
> 35: {
> 36: String code = null;
This brace should go on the previous line
tests/system/src/testscriptapp2/java/mymod/pseudoScriptEngineCompilable/RgfPseudoCompiledScript.java line 60:
> 59: }
> 60:
Extra blank line can be removed.
tests/system/src/testscriptapp2/java/mymod/pseudoScriptEngineCompilable/RgfPseudoScriptEngineCompilable.java line 143:
> 142:
> 143:
Two extra blank lines can be removed.
-------------
PR: https://git.openjdk.java.net/jfx/pull/192
More information about the openjfx-dev
mailing list