Re: [Rev 02] RFR: 8234959: FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and ARGV
Kevin and Ajit, thank you very much for your reviews! Will apply the changes (including changing CRLF to LF) ASAP. ---rony On 23.03.2020 22:45, Kevin Rushforth wrote: > On Sat, 21 Mar 2020 19:19:18 GMT, Kevin Rushforth wrote: > >>> Rony G. Flatscher has updated the pull request incrementally with one >>> additional commit since the last revision: >>> >>> corrected wrong test string >> The fix looks good. I left a few comments on the test. One of them is >> substantive, the rest are formatting. Once you >> make those changes, I'll approve it. > One more minor observation. I noticed the following have DOS line endings: > > tests/system/src/testscriptapp1/resources/mymod/META-INF/services/javax.script.ScriptEngineFactory: > ASCII text, with > CRLF line terminators > tests/system/src/testscriptapp1/resources/mymod/myapp1/demo_01_bottomscript.rpsl: >ASCII text, with > CRLF line terminators > tests/system/src/testscriptapp1/resources/mymod/myapp1/demo_01_middlescript.rpsl: >ASCII text, with > CRLF line terminators > tests/system/src/testscriptapp1/resources/mymod/myapp1/demo_01_topscript.rpsl: > ASCII text, with > CRLF line terminators > > Since they aren't source code files, `git jcheck` won't complain, but as long > as you are fixing the other issues, would > you mind fixing these too? > > - > > PR: https://git.openjdk.java.net/jfx/pull/122
Re: [Rev 02] RFR: 8234959: FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and ARGV
On Sat, 21 Mar 2020 19:19:18 GMT, Kevin Rushforth wrote: >> Rony G. Flatscher has updated the pull request incrementally with one >> additional commit since the last revision: >> >> corrected wrong test string > > The fix looks good. I left a few comments on the test. One of them is > substantive, the rest are formatting. Once you > make those changes, I'll approve it. One more minor observation. I noticed the following have DOS line endings: tests/system/src/testscriptapp1/resources/mymod/META-INF/services/javax.script.ScriptEngineFactory: ASCII text, with CRLF line terminators tests/system/src/testscriptapp1/resources/mymod/myapp1/demo_01_bottomscript.rpsl: ASCII text, with CRLF line terminators tests/system/src/testscriptapp1/resources/mymod/myapp1/demo_01_middlescript.rpsl: ASCII text, with CRLF line terminators tests/system/src/testscriptapp1/resources/mymod/myapp1/demo_01_topscript.rpsl: ASCII text, with CRLF line terminators Since they aren't source code files, `git jcheck` won't complain, but as long as you are fixing the other issues, would you mind fixing these too? - PR: https://git.openjdk.java.net/jfx/pull/122
Re: [Rev 02] RFR: 8234959: FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and ARGV
On Thu, 27 Feb 2020 15:56:25 GMT, Rony G. Flatscher wrote: >> …9: FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and ARGV > > Rony G. Flatscher has updated the pull request incrementally with one > additional commit since the last revision: > > corrected wrong test string The fix looks good. I left a few comments on the test. One of them is substantive, the rest are formatting. Once you make those changes, I'll approve it. tests/system/src/testscriptapp1/java/mymod/myapp1/FXMLScriptDeployment.java line 104: > 103: ioe.printStackTrace(); > 104: System.exit(-1); > 105: } This should be `System.exit(ERROR_UNEXPECTED_EXCEPTION);` tests/system/src/testscriptapp1/java/mymod/myapp1/FXMLScriptDeployment.java line 185: > 184: > 185: // global Bindings > 186: Util.assertExists(IDROOT + " in global scope Bindings", > globalBindings.containsKey(IDROOT)); Minor: indentation is wrong (indented too far) tests/system/src/testscriptapp1/java/mymod/myapp1/FXMLScriptDeployment.java line 208: > 207: > 208: // engine Bindings > 209: Util.assertExists(FILENAME + " in engine scope Bindings", > engineBindings.containsKey(FILENAME)); Minor: indentation tests/system/src/testscriptapp1/java/mymod/pseudoScriptEngine/InvocationInfos.java line 55: > 54: * @return string formatted to ease debugging > 55: */ > 56: public String toDebugFormat(String indentation) { Minor: the `/**` should be on a line by itself. Also, the closing `*/` needs one more space of indentation. tests/system/src/testscriptapp1/java/mymod/myapp1/FXMLScriptDeployment.java line 109: > 108: btn.fireEvent(new ActionEvent()); > 109: btn.fireEvent(new MouseEvent( MouseEvent.MOUSE_CLICKED, > 110:0, // double x, Minor: remove the extra space after the last `(` tests/system/src/testscriptapp1/java/mymod/myapp1/FXMLScriptDeployment.java line 229: > 228: } > 229: else { > 230: Util.assertType(EVENT, ActionEvent.class, obj); Minor: the `}` should be on the same line as the `else {` tests/system/src/testscriptapp1/java/mymod/pseudoScriptEngine/RgfPseudoScriptEngine.java line 47: > 46: public class RgfPseudoScriptEngine extends AbstractScriptEngine > 47: { > 48: static final boolean bDebug = false; // true; The `{` should be on the previous line. tests/system/src/testscriptapp1/java/mymod/pseudoScriptEngine/RgfPseudoScriptEngine.java line 89: > 88: // create copies of the Bindings for later inspection as they > may > 89: // get reused and changed on each eval() invocation > 90: TreeMap bindings = new TreeMap(); Minor: indentation - PR: https://git.openjdk.java.net/jfx/pull/122
Re: [Rev 02] RFR: 8234959: FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and ARGV
On Fri, 20 Mar 2020 11:28:46 GMT, Ajit Ghaisas wrote: >> Rony G. Flatscher has updated the pull request incrementally with one >> additional commit since the last revision: >> >> corrected wrong test string > > tests/system/src/testscriptapp1/java/mymod/myapp1/Util.java line 2: > >> 1: /* >> 2: * Copyright (c) 2017, 2020, Oracle and/or its affiliates. All rights >> reserved. >> 3: * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. > > Please remove "2017, " Since this file was copied verbatim from one of the other test apps, the initial `2017,` is correct. > tests/system/src/testscriptapp1/java/mymod/myapp1/Constants.java line 2: > >> 1: /* >> 2: * Copyright (c) 2017, 2020, Oracle and/or its affiliates. All rights >> reserved. >> 3: * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. > > Please remove "2017, " Same as above. The `2017,` is correct. - PR: https://git.openjdk.java.net/jfx/pull/122
Re: [Rev 02] RFR: 8234959: FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and ARGV
On Thu, 27 Feb 2020 15:56:25 GMT, Rony G. Flatscher wrote: >> …9: FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and ARGV > > Rony G. Flatscher has updated the pull request incrementally with one > additional commit since the last revision: > > corrected wrong test string tests/system/src/testscriptapp1/java/mymod/myapp1/Util.java line 2: > 1: /* > 2: * Copyright (c) 2017, 2020, Oracle and/or its affiliates. All rights > reserved. > 3: * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. Please remove "2017, " tests/system/src/testscriptapp1/java/mymod/myapp1/Constants.java line 2: > 1: /* > 2: * Copyright (c) 2017, 2020, Oracle and/or its affiliates. All rights > reserved. > 3: * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. Please remove "2017, " - PR: https://git.openjdk.java.net/jfx/pull/122
Re: [Rev 02] RFR: 8234959: FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and ARGV
> …9: FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and ARGV The pull request has been updated with 1 additional commit. - Added commits: - a42fed5c: corrected wrong test string Changes: - all: https://git.openjdk.java.net/jfx/pull/122/files - new: https://git.openjdk.java.net/jfx/pull/122/files/8821d25a..a42fed5c Webrevs: - full: https://webrevs.openjdk.java.net/jfx/122/webrev.02 - incr: https://webrevs.openjdk.java.net/jfx/122/webrev.01-02 Stats: 5 lines in 1 file changed: 0 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jfx/pull/122.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/122/head:pull/122 PR: https://git.openjdk.java.net/jfx/pull/122