Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8238080: FXMLLoader: if script engines implement javax.script.Compilable compile scripts #192

Closed

Conversation

@ronyfla
Copy link
Contributor

ronyfla commented Apr 20, 2020

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

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • 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


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8238080: FXMLLoader: if script engines implement javax.script.Compilable compile scripts

Reviewers

  • Kevin Rushforth (kcr - Reviewer)
  • Ajit Ghaisas (aghaisas - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jfx pull/192/head:pull/192
$ git checkout pull/192

ronyfla added 18 commits Feb 22, 2020
…9: FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and ARGV
…59: FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and ARGV
…mpilable compile scripts
…ce error can be resolved (why would jcheck not do it automagically as well as expanding tabs?)
…scripts; PI data can be truei (default) or false
…ther ease spotting the location of script exceptions.
@bridgekeeper
Copy link

bridgekeeper bot commented Apr 20, 2020

👋 Welcome back ronyfla! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request.

@ronyfla
Copy link
Contributor Author

ronyfla commented Apr 22, 2020

============================ test units for: compile PI+fallback

tests/system/src/testscriptapp2/java/mymod/myapp2/FXMLScriptDeployment2Compile_Off.java
... use compile PI to set off compilation of scripts; each script code starts with "demo_02_"

tests/system/src/testscriptapp2/resources/mymod/myapp2/demo_02_off.fxml
... set compilation for scripts off (""), therefore no script gets compiled


tests/system/src/testscriptapp2/java/mymod/myapp2/FXMLScriptDeployment2Compile_Off_On.java
... alternatively uses compile PI to turn compilation off and on; the script code starts alternatively with "demo_02_" and "RgfPseudoCompiledScript.eval(", depending on the state set with the PI

tests/system/src/testscriptapp2/resources/mymod/myapp2/demo_02_off_on.fxml
... starts out explicitly with "" switching the state after each element containing a script


tests/system/src/testscriptapp2/java/mymod/myapp2/FXMLScriptDeployment2Compile_On.java
... no compile PI given, hence compilation of scripts is on; each script code starts with "RgfPseudoCompiledScript.eval("

tests/system/src/testscriptapp2/resources/mymod/myapp2/demo_02_on.fxml
... no compile PI given, starts out to compile scripts by default


tests/system/src/testscriptapp2/java/mymod/myapp2/FXMLScriptDeployment2Compile_On_Off.java
... alternatively uses compile PI to turn compilation on and off; the script code starts alternatively with "RgfPseudoCompiledScript.eval(", and "demo_02_" depending on the state set with the PI

tests/system/src/testscriptapp2/resources/mymod/myapp2/demo_02_on_off.fxml
... starts out explicitly with "" switching the state after each element containing a script (sometimes using PI "" to test setting it to true)


tests/system/src/testscriptapp2/java/mymod/myapp2/FXMLScriptDeployment2Compile_Fail_Compilation.java
... although compile scripts is on, none of the scripts get compiled as they all contain the string "FAIL COMPILATION" which causes RgfPseudoScriptEngineCompilable.compile(...) to throw a ScriptException causing the fallback to be used; all scripts therefore start with "demo_03_"

tests/system/src/testscriptapp2/resources/mymod/myapp2/demo_03_fail_compile.fxml
... explicitly turns on compilation (which is on by default anyway), adds "FAIL COMPILATION" to all inline scripts and to all external scripts with the filename that starts with "demo_03_"

@kevinrushforth
Copy link
Member

kevinrushforth commented May 9, 2020

/reviewers 2

@kevinrushforth
Copy link
Member

kevinrushforth commented May 9, 2020

/csr

@kevinrushforth
Copy link
Member

kevinrushforth commented May 9, 2020

I think the approach proposed in this PR is the best solution for this enhancement. Go ahead and remove the WIP from the title, and we can proceed with the review.

The interface and behavior change will need to be specified in the API docs. This can be done by modifying the Introduction to FXML document. I recommend documenting the new behavior and <?compile> attribute somewhere in the Scripting section.

As discussed on the mailing list, this will need a CSR, which should include the changes to the docs.

@kevinrushforth kevinrushforth self-requested a review May 9, 2020
@openjdk
Copy link

openjdk bot commented May 9, 2020

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@openjdk
Copy link

openjdk bot commented May 9, 2020

@kevinrushforth has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@ronyfla this pull request must refer to an issue in JBS to be able to link it to a CSR request. To refer this pull request to an issue in JBS, please use the /solves command in a comment in this pull request.

Copy link
Member

kevinrushforth left a comment

I reviewed the implementation, and I think this is close to being ready, but I have a couple questions. I also still need to review the test and run it, but that will be later.

I also noticed a few places with minor formatting issues -- mostly missing spaces and extra blank lines added to some files, but also else { when it should be } else {. I'll wait until the substantive part of the review is done to note them all (so if you can fix them ahead of time, that would be good).

…correct formatting).
@ronyfla
Copy link
Contributor Author

ronyfla commented Jun 19, 2020

@CSR-update: looks good!

@kevinrushforth
Copy link
Member

kevinrushforth commented Jun 19, 2020

CSR-update: looks good!

I have moved the CSR to the "proposed" state in preparation for formal review.

@openjdk openjdk bot removed the csr label Jun 24, 2020
Copy link
Member

kevinrushforth left a comment

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.

* Modular test application for testing FXML.
* This is launched by ModuleLauncherTest.
*/
public class FXMLScriptDeployment2Compile_Off extends Application {

This comment has been minimized.

@kevinrushforth

kevinrushforth Jun 25, 2020 Member

Most of the comments I made in the previous test class apply in this and the other tests, too.

@ronyfla
Copy link
Contributor Author

ronyfla commented Jun 26, 2020

Kevin, thank you for your feedback and sponsorship!

Hope that I have applied the changes to all affected files appropriately. (It is interesting for me that despite trying to adhere to the OpenJDK formatting, sometimes my own - decadelong trained :) - formattings slip thru without noticing it.)

Copy link
Member

kevinrushforth left a comment

You got most of them, but there are still a few code style issues. I marked the ones as resolved that were, so you should be able to look at the above comments for those that are stil ourstanding.

One of the comments is more than just code style. I think the following, which appears in a few test files (I noted it in FXMLScriptDeployment2Compile_Fail_Compilation.java), should use an int variable rather than an Integer in the for loop:

         for (Integer invocation = 1; invocation <= invocationList.size(); invocation++) {
Copy link
Member

kevinrushforth left a comment

Looks good.

@ronyfla
Copy link
Contributor Author

ronyfla commented Jun 28, 2020

It seems that there is a need of 2 reviewers though only one is defined (when clicking "Show all reviewers"). Is there anything I can/need to do to advance the state?

@openjdk
Copy link

openjdk bot commented Jun 29, 2020

@ronyfla This change now passes all automated pre-integration checks. When the change also fulfills all project specific requirements, type /integrate in a new comment to proceed. After integration, the commit message will be:

8238080: FXMLLoader: if script engines implement javax.script.Compilable compile scripts

Reviewed-by: kcr, aghaisas
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /issue command.

Since the source branch of this PR was last updated there have been 15 commits pushed to the master branch:

  • 15f97ed: 8240264: iOS: Unnecessary logging on every pulse when GL context changes
  • 2ca509a: 8193800: TreeTableView selection changes on sorting
  • a5878e0: 8214699: Node.getPseudoClassStates must return the same instance on every call
  • 8440b64: 8208169: can not print selected pages of web page
  • 1727dfa: 8247163: JFXPanel throws exception on click when no Scene is set
  • 54e2507: 8247360: Add missing license file for Microsoft DirectShow Samples
  • fb962ac: 8244418: MenuBar: IOOB exception on requestFocus on empty bar
  • bf2e972: 8246348: Crash in libpango on Ubuntu 20.04 with some unicode chars
  • b200891: 8244824: TableView : Incorrect German translation
  • b2b46eb: 8242892: SpinnerValueFactory has an implicit no-arg constructor
  • ... and 5 more: https://git.openjdk.java.net/jfx/compare/6bd0e22d7e33a7e24f17a351914c9ab78a0c3398...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate 15f97ed4b8380ca11d7aa784821ad3617e362057.

As you are not a known OpenJDK Author, an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@kevinrushforth, @aghaisas) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label Jun 29, 2020
@ronyfla
Copy link
Contributor Author

ronyfla commented Jun 29, 2020

/integrate

@openjdk
Copy link

openjdk bot commented Jun 29, 2020

@ronyfla An error occurred during final integration jcheck. No push attempt will be made.

@aghaisas
Copy link
Collaborator

aghaisas commented Jun 30, 2020

@kevinrushforth, I see this is not integrated yet due to jcheck failure. No details about failure can be seen (At least I am unable to see any reported failures). Can you please have a look?

@mlbridge
Copy link

mlbridge bot commented Jun 30, 2020

Mailing list message from Rony G Flatscher on openjfx-dev:

Hi Ajit,

Kevin looked into it already yesterday. There was some problem at github at the time I submitted the /integrate comment, which merely needs to be reissued by me. Having been on the road I was not able to do it yesterday, will be first thing after arriving at the office today.
Also, thank you very much for your review efforts!

Best regards

?-rony

Rony G. Flatscher (mobil/e)

@ronyfla
Copy link
Contributor Author

ronyfla commented Jun 30, 2020

/integrate

@openjdk
Copy link

openjdk bot commented Jun 30, 2020

@ronyfla
Your change (at version 52da8a3) is now ready to be sponsored by a Committer.

@openjdk openjdk bot added the sponsor label Jun 30, 2020
@kevinrushforth
Copy link
Member

kevinrushforth commented Jun 30, 2020

/sponsor

@openjdk openjdk bot closed this Jun 30, 2020
@openjdk openjdk bot added integrated and removed sponsor ready rfr labels Jun 30, 2020
@openjdk
Copy link

openjdk bot commented Jun 30, 2020

@kevinrushforth @ronyfla The following commits have been pushed to master since your change was applied:

  • 15f97ed: 8240264: iOS: Unnecessary logging on every pulse when GL context changes
  • 2ca509a: 8193800: TreeTableView selection changes on sorting
  • a5878e0: 8214699: Node.getPseudoClassStates must return the same instance on every call
  • 8440b64: 8208169: can not print selected pages of web page
  • 1727dfa: 8247163: JFXPanel throws exception on click when no Scene is set
  • 54e2507: 8247360: Add missing license file for Microsoft DirectShow Samples
  • fb962ac: 8244418: MenuBar: IOOB exception on requestFocus on empty bar
  • bf2e972: 8246348: Crash in libpango on Ubuntu 20.04 with some unicode chars
  • b200891: 8244824: TableView : Incorrect German translation
  • b2b46eb: 8242892: SpinnerValueFactory has an implicit no-arg constructor
  • afa805f: 8245575: Show the ContextMenu of input controls with long press gesture on iOS
  • 5304266: 8245635: GlassPasteboard::getUTFs fails on iOS
  • ba501ef: 8246357: Allow static build of webkit library on linux
  • a02e09d: 8246195: ListViewSkin/Behavior: misbehavior on switching skin
  • 9749982: 8246204: No 3D support for newer Intel graphics drivers on Linux

Your commit was automatically rebased without conflicts.

Pushed as commit 45c9854.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.