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

8234959: FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and ARGV #122

Closed
wants to merge 11 commits into from

Conversation

@ronyfla
Copy link
Contributor

@ronyfla ronyfla commented Feb 22, 2020

…9: FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and ARGV


Progress

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

Issue

  • JDK-8234959: FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and ARGV

Reviewers

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

Download

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

ronyfla added 2 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
@ronyfla ronyfla changed the title fix for <https://bugs.openjdk.java.net/browse/JDK-8234959> JDK-823495… 8234959: FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and ARGV Feb 22, 2020
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Feb 22, 2020

Hi ronyfla, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user ronyfla" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

@bridgekeeper bridgekeeper bot added the oca label Feb 22, 2020
@ronyfla
Copy link
Contributor Author

@ronyfla ronyfla commented Feb 22, 2020

/signed

@bridgekeeper bridgekeeper bot added the oca-verify label Feb 22, 2020
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Feb 22, 2020

Thank you! Please allow for up to two weeks to process your OCA, although it is usually done within one to two business days. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

ronyfla added 2 commits Feb 22, 2020
@ronyfla ronyfla requested a review from kevinrushforth Feb 22, 2020
@ronyfla
Copy link
Contributor Author

@ronyfla ronyfla commented Feb 23, 2020

OK, forgot to submit an explanatory text related to this fix. Posted [1] which explains the problem and the suggested solution for discussion.

  • This pull request relates to: https://bugs.openjdk.java.net/browse/JDK-8234959

  • Brief background: the controller code for a FXML file can be written in
    any of the Java script languages that implement the javax.script framework
    introduced with Java 6

There are three possible types of script code definitions possible in fxml files:

  1. in an empty script element where the source attribute denotes an external file
    that contains the code, e.g.:

    <fx:script source="somescript.rex" />

    This pull request uses the source attribute value as the filename.

    There are no arguments to be supplied.

  2. text of a script element, e.g.:

    fx:scriptsay 'code executed at:' .dateTime~new </fx:script>

    or:

    fx:script</fx:script>

    This pull request uses the fxml-filename appended with the string
    "-script_starting_at_line_" and the starting line number of the code.

    There are no arguments to be supplied.

    Reasonings:

     - in the case that multiple fxml files employ script code it is important
       to learn the name of the fxml file that hosts the code that gets executed
    
     - the dash after the fxml filename is meant to ease parsing
    
     - in the case that there are multiple fx:script elements in a fxml file
       with inline code it is important to learn in which line the code starts
       that gets executed
    
     - the decorated filename thereby becomes self documentary and unambiguous to
       the script code developer when debugging
    
  3. PCDATA text in an event attribute which by convention starts with "on", e.g. "onAction":

    <fx:button text="Hi!" onAction="say 'click at:' .dateTime~new', event='arg(1)~toString" />

    This pull request uses the fxml-filename appended with a dash "-", appended with the
    name of the event attribute, the string "attribute_in_element_ending_at_line" and
    the ending line number of the element.

    In the ScriptEventHandler constructor the decorated filename gets stored in addition
    to the script and the scriptEngine.

    Each time the event fires the event object argument will be stored in a copy of the
    engineBindings with the index EVENT_KEY as well as with a single capacity Object array
    stored with the index ScriptEngine.ARGV and the filename with the index
    ScriptEngine.FILENAME. The script's evaluation will get this engineBindings as
    its ENGINE_SCOPE Bindings supplied.

    Reasonings:

     - in the case that multiple fxml files employ script code it is important
       to learn the name of the fxml file that hosts the event code that gets
       executed
    
     - the dash after the fxml filename is meant to ease parsing
    
     - in the case that there are multiple event attributes in elements
       in a fxml file it is important to learn which event attribute in which
       element hosts the event script code that gets executed
    
     - the decorated filename thereby becomes self documentary and unambiguous to
       the script code developer when debugging
    
     - the structural change to the constructor (adding the decorated filename as
       a third argument) of the private static ScriptEventHandler class is confined
       to the FXMLLoader class and has no side effects
    

[1] "Ad suggested test unit for 'JDK-8234959 FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and ARGV'": https://mail.openjdk.java.net/pipermail/openjfx-dev/2020-February/025104.html

@ronyfla
Copy link
Contributor Author

@ronyfla ronyfla commented Feb 23, 2020

Sorry, mixed up the link with the test unit WIP! :(

This is the correct link with the explanatory text related to this suggested fix. Posted [1] which explains the problem and the suggested solution for discussion.
[1] 'Ad suggested fix for "JDK-8234959 FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and ARGV"': https://mail.openjdk.java.net/pipermail/openjfx-dev/2020-February/025102.html

@openjdk openjdk bot added the rfr label Feb 25, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Feb 25, 2020

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Feb 25, 2020

/reviewers 2

@openjdk
Copy link

@openjdk openjdk bot commented Feb 25, 2020

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

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Feb 25, 2020

@aghaisas can you also review this?

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Feb 26, 2020

Overall this looks good to me. As I mentioned in PR #123 you should fold that the unit test into this PR.

@openjdk openjdk bot removed the rfr label Feb 27, 2020
@ronyfla
Copy link
Contributor Author

@ronyfla ronyfla commented Feb 27, 2020

Removed executable bits from gradlew (which I manually have added to become able to run ./gradlew) and pushed it, however the commit has not appeared. Not wanting to add more noise, I stop until advised differently.

@openjdk openjdk bot added the rfr label Feb 27, 2020
@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Mar 2, 2020

Btw, I recommend to run sh gradlew ... so you don't have to change the permission locally.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 2, 2020

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

Hi Kevin,

On 02.03.2020 17:51, Kevin Rushforth wrote:

On Thu, 27 Feb 2020 14:36:17 GMT, Rony G. Flatscher <github.com+60214806+ronyfla at openjdk.org> wrote:

Overall this looks good to me. As I mentioned in PR #123 you should fold that the unit test into this PR.
Removed executable bits from gradlew (which I manually have added to become able to run ./gradlew) and pushed it, however the commit has not appeared. Not wanting to add more noise, I stop until advised differently.
Btw, I recommend to run `sh gradlew ...` so you don't have to change the permission locally.

-------------

PR: https://git.openjdk.java.net/jfx/pull/122

thank you very much for this hint!

As you can probably tell I have never really worked with git (other than pulling open source
projects for inspection, experiments) I learn my lessons step by step (svn would be another story).
Just trying to not bother experts like yourself and others, but then, not everything can be
accounted for by a git-rookie! :) When learning that the executable bit caused a problem on the
push, I felt quite embarrassed to have caused noise and stopped doing a "chmod +x gradlew" (did that
out of pure laziness in the first place to forgo the 'sh' command) and restarted doing the 'sh
gradlew ...' (needless to say now that I have never worked with gradle beforehand, but being? *very*
impressed about it, making the creation of such complex libraries like JavaFX or webkit on Linux a
breeze, wow).

BTW, mistakingly (pressed "Reply" instead of "Reply-to-all") sent my answer to your e-mail address
only, resending it via the list now, because currently there seems to be a problem with
<kcr at openjdk.java.net>:

The original message was received at Mon, 2 Mar 2020 17:50:57 GMT
from aserp3030.oracle.com [127.0.0.1]

?? ----- The following addresses had permanent fatal errors -----
<kcr at openjdk.java.net>
??? (reason: 550 5.1.1 <kcr at openjdk.java.net>: Recipient address rejected: User unknown in local
recipient table)

?? ----- Transcript of session follows -----
... while talking to [137.254.59.6]:
>>> DATA
<<< 550 5.1.1 <kcr at openjdk.java.net>: Recipient address rejected: User unknown in local
recipient table
550 5.1.1 <kcr at openjdk.java.net>... User unknown
<<< 554 5.5.1 Error: no valid recipients

Best regards

---rony

@ronyfla
Copy link
Contributor Author

@ronyfla ronyfla commented Mar 13, 2020

Is there anything I can do to keep the ball rolling ?

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Mar 13, 2020

I just need time to do final testing of this, along with the review of the updated test.

@aghaisas will also review.

@aghaisas
Copy link
Collaborator

@aghaisas aghaisas commented Mar 20, 2020

The changes look good to me.
I verified that the test fails without fix and passes with it.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

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.

// fire three events on the button
btn.fire();
btn.fireEvent(new ActionEvent());
btn.fireEvent(new MouseEvent( MouseEvent.MOUSE_CLICKED,

This comment has been minimized.

@kevinrushforth

kevinrushforth Mar 21, 2020
Member

Minor: remove the extra space after the last (

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Mar 23, 2020

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?

@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 24, 2020

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

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 <kcr at openjdk.org> 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

@openjdk openjdk bot removed the rfr label Mar 25, 2020
@openjdk openjdk bot added the rfr label Mar 25, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Mar 31, 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:

8234959: FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and ARGV

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 /solves command.

Since the source branch of this PR was last updated there have been 33 commits pushed to the master branch. Since 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 d7f13f457ebbf8608dc5169737b34b8cc9c9d242.

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 Mar 31, 2020
@ronyfla
Copy link
Contributor Author

@ronyfla ronyfla commented Mar 31, 2020

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Mar 31, 2020

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

@openjdk openjdk bot added the sponsor label Mar 31, 2020
@ronyfla
Copy link
Contributor Author

@ronyfla ronyfla commented Mar 31, 2020

Kevin, Ajit, could you please /sponsor this PR? TIA, ---rony

@aghaisas
Copy link
Collaborator

@aghaisas aghaisas commented Mar 31, 2020

/sponsor

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

@openjdk openjdk bot commented Mar 31, 2020

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

  • d7f13f4: 8089828: RTL Orientation, the flag of a mnemonic is not placed under the mnemonic letter.
  • 9ecc107: 8240539: Upgrade gradle to version 6.3
  • f3a3ea0: 8234471: Canvas in webview displayed with wrong scale on Windows
  • d12e71c: 8241474: Build failing on Ubuntu 20.04
  • 2a7ab36: 8089134: [2D traversal, RTL] TraversalEngine only handles left/right key traversal correctly in RTL for top-level engine in ToolBar
  • 2aa8218: 8235480: Regression: [RTL] Arrow keys navigation doesn't respect TableView orientation
  • e9c6119: 8240692: Cleanup of the javafx property objects
  • 90289a2: 8239107: Update libjpeg to version 9d
  • b81cf32: 8236259: MemoryLeak in ProgressIndicator
  • 9ea7f96: 8240832: Remove unused applecoreaudio.md third-party legal file
  • 0fc1420: Merge
  • 50e15fc: 8240466: AppJSCallback* apps launched by ModuleLauncherTest intermittently hang
  • e3026b9: 8240688: Remove the JavaBeanXxxPropertyBuilders constructors
  • f8c235b: 8240631: Create release notes for JavaFX 14
  • cfa1193: 8236685: [macOs] Remove obsolete file dialog subclasses
  • f25e8cf: 8212034: Potential memory leaks in jpegLoader.c in error case
  • b2ac76a: 8240451: JavaFX javadoc build fails with JDK 14
  • cf0bba6: 8240211: Stack overflow on Windows 32-bit can lead to crash
  • 337ed72: 8237926: Potential memory leak of model data in javafx.scene.control.ListView
  • 960f039: 8208761: Update constant collections to use the new immutable collections
  • 10c9528: 8240265: iOS: Unnecessary logging on pinch gestures
  • 4c132cd: 8237889: Update libxml2 to version 2.9.10
  • 20328b3: 8240218: IOS Webkit implementation broken
  • 4c82af8: 8236832: [macos 10.15] JavaFX Application hangs on video play on Cata…
  • 9cd6f79: 8196586: Remove use of deprecated finalize methods from javafx property objects
  • ef2f9ce: 8238755: allow to create static lib for javafx.media on linux
  • c3ee1a3: 8239822: Intermittent unit test failures in RegionCSSTest
  • 3150562: 8235772: Remove use of deprecated finalize method from PiscesRenderer and AbstractSurface
  • 4eaff0d: 8239109: Update SQLite to version 3.31.1
  • d8e7f85: 8239454: LLIntData : invalid opcode returned for 16 and 32 bit wide instructions
  • 48ddd80: Merge
  • 35a01ca: 8228867: Fix mistakes in FX API docs
  • b5e65ec: 8238434: Ensemble: Update version of Lucene to 7.7.2

Your commit was automatically rebased without conflicts.

Pushed as commit 6d098fe.

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.