On Wed, 13 May 2020 08:46:11 GMT, Rony G. Flatscher <github.com+60214806+rony...@openjdk.org> wrote:
>> This WIP 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. It extends PR 187 >> <https://github.com/openjdk/jfx/pull/187>. To further ease >> spotting scripts that cause a ScriptException a message in the form of >> "filename: caused ScriptException" gets added to >> the exception handling in either of the three locations: an error message, a >> stack trace or a wrap-up into a >> RuntimeException (having three different kinds of reporting ScriptExceptions >> may be questioned, however none of these >> tear down the FXML GUI). > > Rony G. Flatscher has updated the pull request incrementally with one > additional commit since the last revision: > > Reword the compile processing instructions (replace 'Hint' with 'Note:', > adjust text to context), remove spurious empty > line in script example code. Here are a couple overall comments. 1. Please update the Description of this PR now that this is no longer WIP. Also, can you copy any relevant information from PR #187 so that this PR is standalone (reviewers shouldn't have to go to the earlier PRs for any information)? 2. The text you provided for the CSR is a good start, so I created a Draft CSR, [JDK-8245873](https://bugs.openjdk.java.net/browse/JDK-8245873), based on what you have. I'll have additional comments later, but one thing I noticed that needs to be added is that the "Specification" section needs to list the diffs for the public API docs in FXMLLoader and for the FXML intro. I also left some inline comments below. I haven't done a code review yet because I want to get the API documentation and CSR in shape first. modules/javafx.fxml/src/main/docs/javafx/fxml/doc-files/introduction_to_fxml.html line 723: > 722: <span class="code">ObservableList</span>, <span > class="code">ObservableMap</span> or <span > class="code">ObservableSet</span> 723: uses a special <span > class="code">onChange</span> attribute that points to a > handler method with a <span class="code">ListChangeListener.Change</span>, > <span > class="code">MapChangeListener.Change</span> or <span > class="code">SetChangeListener.Change</span> parameter > respectively. 724: </p> Please revert this change since it is unrelated. Especially in this case where it is public API docs that will be noted in the CSR, this will seem out of place. I filed JDK-nnnnn to track and accumulate these sort of typos. modules/javafx.fxml/src/main/java/javafx/fxml/FXMLLoader.java line 1924: > 1923: */ > 1924: public static final String COMPILE_PROCESSING_INSTRUCTION = > "compile"; > 1925: You need to add an `@since 15` javadoc tag for this new public field. modules/javafx.fxml/src/main/docs/javafx/fxml/doc-files/introduction_to_fxml.html line 775: > 774: > 775: <p>Note: to turn off automatic compilation of script code place the > processing instruction <span > class="code"><?compile false?></span> before the script element. To > turn on compilation of script code again use > the processing instruction <span class="code"><?compile true?></span> > (or short: <span > class="code"><?compile?></span>). The compile processing instruction > can be used repeatedly to turn compilation > of script code off and on.</p> 776: I would start by saying that scripts are compiled by default if the script engine implements `javax.script.Compilable`. Maybe add a sentence something like this: Scripts are compiled by default, when they are first loaded, if the `ScriptEngine` implements the `javax.script.Compilable` interface. If compilation fails, the `FXMLLoader` will fall back to interpreted mode. ------------- PR: https://git.openjdk.java.net/jfx/pull/192
