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
