Re: [Integrated] RFR: 8242548: Wrapped labeled controls using -fx-line-spacing cut text off

[John Hendrikx]

On Sun, 12 Apr 2020 21:21:07 GMT, John Hendrikx  wrote:

> This is a solution for 8242548.  There was zero test coverage, so I added a 
> few tests for this as well.

This pull request has now been integrated.

Changeset: 7b061900
Author:John Hendrikx 
Committer: Ajit Ghaisas 
URL:   https://git.openjdk.java.net/jfx/commit/7b061900
Stats: 53 lines in 4 files changed: 0 ins; 48 del; 5 mod

8242548: Wrapped labeled controls using -fx-line-spacing cut text off

Reviewed-by: aghaisas, kcr

-

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

Re: RFR: 8242523: Update the animation and clip envelope classes

[Nir Lisker]

On Tue, 5 May 2020 05:47:30 GMT, Ambarish Rapte  wrote:

>> Mostly refactoring in preparation of the upcoming fixes. The changes might 
>> look like a lot, but it's mostly rearranging
>> of methods. Summery of changes:
>> ### Animation
>> * Added `isNearZero` and `areNearEqual` methods that deal with `EPSILON` 
>> checks.
>> * Added `isStopped`, `isRunning` and `isPaused` convenience methods.
>> * Added `runHandler` method to deal with running the handler.
>> * Moved methods to be grouped closer to where they are used rather than by 
>> visibility.
>> * Removed the static import for `TickCalculation`.
>> * Various small subjective readability changes.
>> * Behavioral changes: switching `autoReverse` and `onFinished` properties 
>> from "Simple" to "Base" properties; and lazily
>>   initializing the `cuePoints` map.
>> 
>> ### Clip Envelopes
>> * Added `MultiLoopClipEnvelope` as an intermediate parent for infinite and 
>> finite clip envelopes.
>> * Rearranged methods order to be consistent.
>> * Replaced the `checkBounds` method with a new overload of `Utils.clamp`.
>> * Renamed `pos` to `cyclePos`.
>> * Extracted common methods: `changedDirection` and `ticksRateChange`
>> * Added internal documentation.
>> 
>> Also corrected a typo in `TicksCalculation` and added an explanation for an 
>> animation test.
>
> modules/javafx.graphics/src/main/java/com/sun/scenario/animation/shared/MultiLoopClipEnvelope.java
>  line 48:
> 
>> 47:
>> 48: protected boolean autoReverse() {
>> 49: return autoReverse;
> 
> I would suggest the name to be `isAutoReverese`

That would be the usual naming convention, yes, but I find that  this can be 
more fluent to read. Perhaps I'm
influenced by the upcoming `record`s feature. Can change.

-

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

Re: RFR: 8242523: Update the animation and clip envelope classes

[Nir Lisker]

On Mon, 11 May 2020 04:30:28 GMT, Ambarish Rapte  wrote:

>> Mostly refactoring in preparation of the upcoming fixes. The changes might 
>> look like a lot, but it's mostly rearranging
>> of methods. Summery of changes:
>> ### Animation
>> * Added `isNearZero` and `areNearEqual` methods that deal with `EPSILON` 
>> checks.
>> * Added `isStopped`, `isRunning` and `isPaused` convenience methods.
>> * Added `runHandler` method to deal with running the handler.
>> * Moved methods to be grouped closer to where they are used rather than by 
>> visibility.
>> * Removed the static import for `TickCalculation`.
>> * Various small subjective readability changes.
>> * Behavioral changes: switching `autoReverse` and `onFinished` properties 
>> from "Simple" to "Base" properties; and lazily
>>   initializing the `cuePoints` map.
>> 
>> ### Clip Envelopes
>> * Added `MultiLoopClipEnvelope` as an intermediate parent for infinite and 
>> finite clip envelopes.
>> * Rearranged methods order to be consistent.
>> * Replaced the `checkBounds` method with a new overload of `Utils.clamp`.
>> * Renamed `pos` to `cyclePos`.
>> * Extracted common methods: `changedDirection` and `ticksRateChange`
>> * Added internal documentation.
>> 
>> Also corrected a typo in `TicksCalculation` and added an explanation for an 
>> animation test.
>
> modules/javafx.graphics/src/main/java/com/sun/scenario/animation/shared/MultiLoopClipEnvelope.java
>  line 59:
> 
>> 58: return Math.round((ticks - deltaTicks) * Math.abs(newRate / 
>> rate));
>> 59:  }
>> 60:
> 
> This is similar to `ClipEnvelope.ticksRateChange()` with a minor difference. 
> Can this be removed from here and
> `ClipEnvelope.ticksRateChange()` be reused ?

I think that in the next patches there will be another change here anyway that 
will make this more sensible. I just
tried to split the final patch into more manageable chunks so some changes need 
future vision.

-

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

Re: RFR: 8242523: Update the animation and clip envelope classes

[Nir Lisker]

On Wed, 6 May 2020 14:18:06 GMT, Ambarish Rapte  wrote:

>> Mostly refactoring in preparation of the upcoming fixes. The changes might 
>> look like a lot, but it's mostly rearranging
>> of methods. Summery of changes:
>> ### Animation
>> * Added `isNearZero` and `areNearEqual` methods that deal with `EPSILON` 
>> checks.
>> * Added `isStopped`, `isRunning` and `isPaused` convenience methods.
>> * Added `runHandler` method to deal with running the handler.
>> * Moved methods to be grouped closer to where they are used rather than by 
>> visibility.
>> * Removed the static import for `TickCalculation`.
>> * Various small subjective readability changes.
>> * Behavioral changes: switching `autoReverse` and `onFinished` properties 
>> from "Simple" to "Base" properties; and lazily
>>   initializing the `cuePoints` map.
>> 
>> ### Clip Envelopes
>> * Added `MultiLoopClipEnvelope` as an intermediate parent for infinite and 
>> finite clip envelopes.
>> * Rearranged methods order to be consistent.
>> * Replaced the `checkBounds` method with a new overload of `Utils.clamp`.
>> * Renamed `pos` to `cyclePos`.
>> * Extracted common methods: `changedDirection` and `ticksRateChange`
>> * Added internal documentation.
>> 
>> Also corrected a typo in `TicksCalculation` and added an explanation for an 
>> animation test.
>
> modules/javafx.graphics/src/main/java/javafx/animation/Animation.java line 
> 142:
> 
>> 141: }
>> 142:
>> 143: /*
> 
> Both these methods seem like they belong to Utils.java. If moved to 
> Utils.java then the all the calls
> `(Math.abs(currentRate - rate) < EPSILON)` can be changed to use these 
> methods from Utils.java. If we move these
> methods then, Utils.java also needs to declare  `static final double EPSILON 
> = 1e-12;`  and the EPSILON here can be
> initialized as `private static final double EPSILON = Utils.EPSILON;`

I agree that these methods are better suited there, but I'm not sure the same 
epsilon value will be suitable for other
places that will want to use these. That value is somewhat arbitrary anyway I 
think.

-

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

Re: RFR: 8242523: Update the animation and clip envelope classes

[Nir Lisker]

On Tue, 5 May 2020 11:19:55 GMT, Ambarish Rapte  wrote:

>> Mostly refactoring in preparation of the upcoming fixes. The changes might 
>> look like a lot, but it's mostly rearranging
>> of methods. Summery of changes:
>> ### Animation
>> * Added `isNearZero` and `areNearEqual` methods that deal with `EPSILON` 
>> checks.
>> * Added `isStopped`, `isRunning` and `isPaused` convenience methods.
>> * Added `runHandler` method to deal with running the handler.
>> * Moved methods to be grouped closer to where they are used rather than by 
>> visibility.
>> * Removed the static import for `TickCalculation`.
>> * Various small subjective readability changes.
>> * Behavioral changes: switching `autoReverse` and `onFinished` properties 
>> from "Simple" to "Base" properties; and lazily
>>   initializing the `cuePoints` map.
>> 
>> ### Clip Envelopes
>> * Added `MultiLoopClipEnvelope` as an intermediate parent for infinite and 
>> finite clip envelopes.
>> * Rearranged methods order to be consistent.
>> * Replaced the `checkBounds` method with a new overload of `Utils.clamp`.
>> * Renamed `pos` to `cyclePos`.
>> * Extracted common methods: `changedDirection` and `ticksRateChange`
>> * Added internal documentation.
>> 
>> Also corrected a typo in `TicksCalculation` and added an explanation for an 
>> animation test.
>
> modules/javafx.graphics/src/main/java/com/sun/scenario/animation/shared/ClipEnvelope.java
>  line 46:
> 
>> 45:  */
>> 46: public abstract class ClipEnvelope {
>> 47:
> 
> I think the removal of line 46 was unintended change.

I think that there is no empty line between the docs and the declaration, but 
can revert.

-

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

TableView help

[John Scancella]

Hello All,

I am hoping to get some help with setting up a TableView. What I want is a
simple two column table with each column containing Strings. I want to be
able to double click to create a new row if I double clicked on empty
space. And I want to be able to edit any cell already in the table.

Currently I am struggling with editing the data already in the table. I can
click and edit, but then if I double click again on the cell it reverts
back to the original value. If I try and get the value programmatically, it
is still getting the original value, even though the display is showing the
new value.

You can see my code here: https://github.com/jscancella/heirloom
I am using java version "1.8.0_251"

Thank you in advance for any help!


-- 
~John Scancella

Re: [Integrated] RFR: 8202296: Monocle MouseInput doesn't send keyboard modifiers in events.

[Tom Schindl]

On Fri, 10 Apr 2020 10:26:06 GMT, Tom Schindl  wrote:

> Extract keystate and add to the existing modifier mask, to support eg
> multi-select
> 
> https://bugs.openjdk.java.net/browse/JDK-8202296

This pull request has now been integrated.

Changeset: 435671ee
Author:Tom Schindl 
Committer: Kevin Rushforth 
URL:   https://git.openjdk.java.net/jfx/commit/435671ee
Stats: 74 lines in 2 files changed: 0 ins; 68 del; 6 mod

8202296: Monocle MouseInput doesn't send keyboard modifiers in events.

Reviewed-by: kcr

-

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

Re: RFR: 8243115: Spurious invalidations due to bug in IntegerBinding and other classes

[Nir Lisker]

On Tue, 12 May 2020 17:58:09 GMT, John Hendrikx  wrote:

> I'm fine with doing a fix, but I need to know which one. Avoiding NPE's and 
> silently doing nothing is IMHO not very
> desirable as this will give the user of the API no feedback that something 
> went wrong.
> So I would prefer to fix this by documenting that these cases will result in 
> a NPE.
> 
> The `bind` method has a similar issue -- it doesn't check its array elements 
> for `null`, and will throw a NPE when
> attempting to add a listener to `null`. Again, I would just document the NPE 
> so what is clearly a mistake doesn't go
> unnoticed.

I think that checking the array elements doesn't help much because by the time 
you can check them they will already be
used, and that will throw an NPE implicitly.

`bind` is no-op for `null` or 0-length arrays, but should have probably throw 
an NPE on the `null` case. The 0-length
check saves creating the `observer`, so I think it makes sense. `unbind` should 
probably only throw an NPE on `null`
arrays, but that happens implicitly anyway, so maybe there is no change needed 
unless it's for clarity because we can
add a custom error message.

-

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

Re: [Rev 03] RFR: 8242548: Wrapped labeled controls using -fx-line-spacing cut text off

[John Hendrikx]

On Tue, 12 May 2020 13:44:53 GMT, Ajit Ghaisas  wrote:

>> Marked as reviewed by kcr (Lead).
>
> @hjohn, this PR is ready to be merged.
> You need to comment /integrate on this PR as instructed by the bot above. I 
> will sponsor it once you do it.

Sorry I figured only a committer was allowed to use that command.

-

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

Re: RFR: 8243115: Spurious invalidations due to bug in IntegerBinding and other classes

[John Hendrikx]

On Mon, 11 May 2020 22:27:27 GMT, Nir Lisker  wrote:

> As I started my review I noticed that `unbind` does not null-check its 
> argument `dependencies` like `bind` does and it
> can lead to NPEs. If it is out of scope for this PR to fix this, a new issue 
> should be filed.

I'm fine with doing a fix, but I need to know which one.  Avoiding NPE's and 
silently doing nothing is IMHO not very
desirable as this will give the user of the API no feedback that something went 
wrong.

So I would prefer to fix this by documenting that these cases will result in a 
NPE.

The `bind` method has a similar issue -- it doesn't check its array elements 
for `null`, and will throw a NPE when
attempting to add a listener to `null`.  Again, I would just document the NPE 
so what is clearly a mistake doesn't go
unnoticed.

-

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

Re: RFR: 8243115: Spurious invalidations due to bug in IntegerBinding and other classes

[John Hendrikx]

On Tue, 12 May 2020 15:56:32 GMT, Nir Lisker  wrote:

> The fix looks correct and the tests pass. I just wonder why the change to 
> reflection-based construction with
> `bindingMockClassConstructor`?

The Parameterized test constructs some standard Binding objects to run multiple 
tests with.  This works fine if those
objects are immutable (or aren't modified during the tests).  My new test 
however does modify them, and other tests
would fail with such modified objects (as `unbind` was called on some).  So I 
rewrote this a little bit to construct
fresh objects for each test, and for that I used some reflection magic to avoid 
having to write a specific test for
each of the 8 binding types.

-

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

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

[Rony G . Flatscher]

On Tue, 12 May 2020 16:51:08 GMT, Rony G. Flatscher 
 wrote:

>> 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](https://github.com/openjdk/jfx/blob/master/modules/javafx.fxml/src/main/docs/javafx/fxml/doc-files/introduction_to_fxml.html)
>> document. I recommend documenting the new behavior and `` 
>> attribute somewhere in the
>> [Scripting](https://github.com/openjdk/jfx/blob/master/modules/javafx.fxml/src/main/docs/javafx/fxml/doc-files/introduction_to_fxml.html#L769)
>> section.  As discussed on the mailing list, this will need a CSR, which 
>> should include the changes to the docs.
>
> Suggested CSR:
> 
> Summary
> ===
> Have javafx.fxml.FXMLLoader compile FXML scripts before evaluating them, if 
> the script engine
> implements the javax.script.Compilable interface to speed up execution. In 
> case compilation
> throws a javax.script.ScriptException fall back to evaluating the uncompiled 
> script. Allow
> control of script compilation with a "compile" PI for FXML files.
> 
> Problem
> ===
> javafx.fxml.FXMLLoader is able to execute scripts in Java script languages
> (javax.script.ScriptEngine implementations) referred to or embedded in a FXML 
> file.
> 
> If a script engine implements the javax.script.Compilable interface, then 
> such scripts could be
> compiled and the resulting javax.script.CompiledScript could be executed 
> instead using its
> eval() methods.
> 
> Evaluating the javax.script.CompiledScript objects may help speed up the 
> execution of script
> invocations, especially for scripts defined for event attributes in FXML 
> elements (e.g. like
> onMouseMove) which may be repetitively invoked and evaluated.
> 
> Solution
> 
> Before evaluating the script code test whether the javax.script.ScriptEngine 
> implements
> javax.script.Compilable. If so, compile the script to a 
> javax.script.CompiledScript object first
> and then use its eval() method to evaluate the script, otherwise continue to 
> use the
> javax.script.ScriptEngine's eval() method instead. Should compilation of a 
> script yield
> (unexpectedly) a javax.script.ScriptException then fall back to using the
> javax.script.ScriptEngine's eval() method. A new process instruction 
> "compile" allows control of
> the compilation of scripts ("true" sets compilation on, "false" to set 
> compilation off) in FXML
> files.
> 
> Specification
> =
> If a javax.script.ScriptEngine implements the javax.script.Compilable 
> interface, then use its
> compile() method to compile the script to a javax.script.CompiledScript 
> object and use its
> eval() method to run the script. In the case that the compilation throws 
> (unexpectedly) a
> javax.script.ScriptException log a warning and fall back to using the
> javax.script.ScriptEngine's eval() method instead.
> To allow setting this feature off and on while processing the FXML file a 
> "compile" process
> instruction ("" or "") gets defined that 
> allows to turn
> compilation off and on throughout a FXML file.

Doc addition for the CSR:
=
The following text was added to the [Introduction to
FXML](https://github.com/openjdk/jfx/blob/master/modules/javafx.fxml/src/main/docs/javafx/fxml/doc-files/introduction_to_fxml.html)
at two locations, the "Script Event Handlers" ("#script_event_handlers") and 
the "Scripting" (#scripting") sections:

> Hint: to turn off automatic compilation of script code place the 
> processing instruction  class="code">?compile false? before the script. To turn on 
> compilation of script code again use the
> processing instruction ?compile true? (or 
> short:  class="code">?compile?). The compile processing instruction 
> can be used repeatedly to turn compilation
> of script code off and on.

This documents the compile processing instruction, that it compilation is on by 
default and that it can be turned off
and on repeatedly. It is hoped that this formulation plays well with the 
context where this paragraph got introduced.

-

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

Re: Next steps ? (Re: An attempt of a CSR draft ... (Re: A new WIP (PR # 192) (Re: WIP version with PI compile (Re: A WIP for JDK-8238080 for review/discussion (Re: "Internal review ID 9063426: "FXMLL

[Rony G. Flatscher]

Hi Kevin,

in the meantime I have tried to come up with a formulation for the 
"Introduction to FXML"
specification about the new compile processing instruction which is brief and 
complete. While being
there I fixed a typo in the document and added a missing language processing 
instruction to an
existing script example, hope that was o.k. as I have not filed an explicit bug 
for it.

Also closed the PR 129 and 187, removed the WIP from PR 192, and supplied the 
CSR draft as a comment.

If there is anything else I should do, please let me know.

Cheers,

---rony


On 11.05.2020 14:12, Rony G. Flatscher wrote:
> Hi Kevin,
>
> On 09.05.2020 17:16, Kevin Rushforth wrote:
>> I'm finally getting back to this. I took a look at 
>> https://github.com/openjdk/jfx/pull/192 and I
>> like that as the direction for this enhancement.
>>
>> The initial CSR you have is a good start.
> Thank you!
>
>> Next steps are:
>>
>> 1. Update the "Introduction to FXML" specification (see my comment in the PR)
>> 2. Update PR 192 with the draft CSR as a comment, modifying it to include 
>> the above additions to
>> "Introduction to FXML"
>> 3. Remove WIP from the title
>>
>> You can then close the other two PRs (129 and 187).
> will do (may take a little bit).
>
> Cheers
>
> ---rony
>
>
>> On 4/28/2020 6:15 AM, Rony G. Flatscher wrote:
>>> Hi Kevin,
>>>
>>> what should be the next steps?
>>>
>>> Should I remove "WIP" from the title in 
>>>  and add the CSR
>>> draft text of my last e-mail as a "CSR" comment with PR # 192, thereby 
>>> requesting it to be added
>>> to ?
>>>
>>> Please advise.
>>>
>>> TIA,
>>>
>>> ---rony
>>>
>>> P.S.: This is the RFE:
>>>
>>>   * RFE (2020-01-24): 
>>> 
>>>
>>> These are the three versions (all with appropriate unit tests) that I came 
>>> up with
>>> chronologically to implement the RFE, would prefer the latest version (PR # 
>>> 192):
>>>
>>>   * Compile if Compilable implemented (2020-02-28): 
>>> 
>>>   * Compile if compile PI and Compilable is implemented (2020-04-11):
>>> 
>>>   * Compile with fallback, if Compilable is implemented, compile PI for 
>>> fine-grained control
>>> (2020-04-14): 
>>>
>>>
>>> On 22.04.2020 20:01, Rony G. Flatscher wrote:
 Hi Kevin,

 as I am not able to file a CSR with the issue you suggested to come up 
 with a draft, so here it goes:

 Summary
 ===
 Have javafx.fxml.FXMLLoader compile FXML scripts before evaluating 
 them, if the script engine
 implements the javax.script.Compilable interface to speed up 
 execution. In case compilation
 throws a javax.script.ScriptException fall back to evaluating the 
 uncompiled script. Allow
 control of script compilation with a "compile" PI for FXML files.

 Problem
 ===
 javafx.fxml.FXMLLoader is able to execute scripts in Java script 
 languages
 (javax.script.ScriptEngine implementations) referred to or embedded in 
 a FXML file.

 If a script engine implements the javax.script.Compilable interface, 
 then such scripts could be
 compiled and the resulting javax.script.CompiledScript could be 
 executed instead using its
 eval() methods.

 Evaluating the javax.script.CompiledScript objects may help speed up 
 the execution of script
 invocations, especially for scripts defined for event attributes in 
 FXML elements (e.g. like
 onMouseMove) which may be repetitively invoked and evaluated.

 Solution 
 
 Before evaluating the script code test whether the 
 javax.script.ScriptEngine implements
 javax.script.Compilable. If so, compile the script to a 
 javax.script.CompiledScript object first
 and then use its eval() method to evaluate the script, otherwise 
 continue to use the
 javax.script.ScriptEngine's eval() method instead. Should compilation 
 of a script yield
 (unexpectedly) a javax.script.ScriptException then fall back to using 
 the
 javax.script.ScriptEngine's eval() method. A new process instruction 
 "compile" allows control of
 the compilation of scripts ("true" sets compilation on, "false" to set 
 compilation off) in FXML
 files.

 Specification
 =
 If a javax.script.ScriptEngine implements the javax.script.Compilable 
 interface, then use its
 compile() method to compile the script to a 
 javax.script.CompiledScript object and use its
 eval() method to run the script. In the case that the compilation 
 throws (unexpectedly) a

Re: [Closed] RFR: WIP 8238080: FXMLLoader: if script engines implement javax.script.Compilable compile scripts

[Rony G . Flatscher]

On Fri, 28 Feb 2020 17:46:58 GMT, Rony G. Flatscher 
 wrote:

> TODO: ADD DESCRIPTION OF PROPOSED FIX HERE.

This pull request has been closed without being integrated.

-

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

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

[Rony G . Flatscher]

On Sat, 9 May 2020 15:15:52 GMT, Kevin Rushforth  wrote:

>>  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_"
>
> 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](https://github.com/openjdk/jfx/blob/master/modules/javafx.fxml/src/main/docs/javafx/fxml/doc-files/introduction_to_fxml.html)
> document. I recommend documenting the new behavior and `` attribute 
> somewhere in the
> [Scripting](https://github.com/openjdk/jfx/blob/master/modules/javafx.fxml/src/main/docs/javafx/fxml/doc-files/introduction_to_fxml.html#L769)
> section.  As discussed on the mailing list, this will need a CSR, which 
> should include the changes to the docs.

Suggested CSR:

Summary
===
Have javafx.fxml.FXMLLoader compile FXML scripts before evaluating them, if the 
script engine
implements the javax.script.Compilable interface to speed up execution. In case 
compilation
throws a javax.script.ScriptException fall back to evaluating the uncompiled 
script. Allow
control of script compilation with a "compile" PI for FXML files.

Problem
===
javafx.fxml.FXMLLoader is able to execute scripts in Java script languages
(javax.script.ScriptEngine implementations) referred to or embedded in a FXML 
file.

If a script engine implements the javax.script.Compilable interface, then such 
scripts could be
compiled and the resulting javax.script.CompiledScript could be executed 
instead using its
eval() methods.

Evaluating the javax.script.CompiledScript objects may help speed up the 
execution of script
invocations, especially for scripts defined for event attributes in FXML 
elements (e.g. like
onMouseMove) which may be repetitively invoked and evaluated.

Solution

Before evaluating the script code test whether the javax.script.ScriptEngine 
implements
javax.script.Compilable. If so, compile the script to a 
javax.script.CompiledScript object first
and then use its eval() method to evaluate the script, otherwise continue to 
use the
javax.script.ScriptEngine's eval() method instead. Should compilation of a 
script yield
(unexpectedly) a javax.script.ScriptException then fall back to using the
javax.script.ScriptEngine's eval() method. A new process instruction "compile" 
allows control of
the compilation of 

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

[Kevin Rushforth]

On Wed, 22 Apr 2020 16:33:47 GMT, Rony G. Flatscher 
 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 
>> .  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).
>
>  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_"

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](https://github.com/openjdk/jfx/blob/master/modules/javafx.fxml/src/main/docs/javafx/fxml/doc-files/introduction_to_fxml.html)
document. I recommend documenting the new behavior and `` attribute 
somewhere in the
[Scripting](https://github.com/openjdk/jfx/blob/master/modules/javafx.fxml/src/main/docs/javafx/fxml/doc-files/introduction_to_fxml.html#L769)
section.

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

-

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

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

[Rony G . Flatscher]

On Mon, 20 Apr 2020 12:45:30 GMT, Rony G. Flatscher 
 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 
> .  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).

 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_"

-

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

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

[Rony G . Flatscher]

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 
.

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).

-

Commit messages:
 - Document the compile processing instruction for scripts.
 - Add missing language processing instruction.
 - Correct typo, replace tabs, remove trailing blanks.
 - Make sure we test the default behaviour to compile script by leaving out the 
compile PI.
 - Revert temporary rename of test method.
 - Correct ModuleLauncherTest (remove non-existing test), correct formatting.
 - Always supply the script's filename in the error message first to further 
ease spotting the location of script exceptions.
 - Make message more pregnant.
 - Compile by default, have fallback if compilation fails, adapt/add test unig
 - add compile process instruction to control compilation of compilable 
scripts; PI data can be truei (default) or false
 - ... and 14 more: https://git.openjdk.java.net/jfx/compare/159f6516...44b0f9f8

Changes: https://git.openjdk.java.net/jfx/pull/192/files
 Webrev: https://webrevs.openjdk.java.net/jfx/192/webrev.00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8238080
  Stats: 2544 lines in 29 files changed: 2514 ins; 2 del; 28 mod
  Patch: https://git.openjdk.java.net/jfx/pull/192.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/192/head:pull/192

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

Re: RFR: 8243115: Spurious invalidations due to bug in IntegerBinding and other classes

[Nir Lisker]

On Mon, 11 May 2020 22:27:27 GMT, Nir Lisker  wrote:

>>> I will review this too anyway.
>> 
>> Thank you. That will be helpful.
>
> As I started my review I noticed that `unbind` does not null-check its 
> argument `dependencies` like `bind` does and it
> can lead to NPEs. If it is out of scope for this PR to fix this, a new issue 
> should be filed.

The fix looks correct and the tests pass. I just wonder why the change to 
reflection-based construction with
`bindingMockClassConstructor`?

-

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

Re: [Integrated] RFR: 8244417: support static build for Windows

[Joeri Sykora]

On Thu, 7 May 2020 21:15:02 GMT, Joeri Sykora  wrote:

> Adds support for building static libraries of the OpenJFX modules for the 
> Windows platform. Building static libraries
> is done by providing the gradle property `-PSTATIC_BUILD=true`.
> Changes include:
> 
> - add static flags for the compiler and linker
> - use `lib` for linking instead of `link`
> - only build and include version.rc once (from GlassResources.rc)
> - include library specific `JNI_OnLoad_xxx` methods
> - functionality loaded via `DllMain` has been implemented at a different 
> location

This pull request has now been integrated.

Changeset: c14cc44e
Author:Joeri Sykora 
Committer: Johan Vos 
URL:   https://git.openjdk.java.net/jfx/commit/c14cc44e
Stats: 83 lines in 9 files changed: 2 ins; 69 del; 12 mod

8244417: support static build for Windows

Reviewed-by: kcr, jvos

-

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

Re: [Rev 03] RFR: 8242548: Wrapped labeled controls using -fx-line-spacing cut text off

[Ajit Ghaisas]

On Fri, 1 May 2020 12:33:36 GMT, Kevin Rushforth  wrote:

>> John Hendrikx has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Fix typo in comment
>
> Marked as reviewed by kcr (Lead).

@hjohn, this PR is ready to be merged.
You need to comment /integrate on this PR as instructed by the bot above. I 
will sponsor it once you do it.

-

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

Re: RFR: 8244417: support static build for Windows

[Johan Vos]

On Thu, 7 May 2020 21:15:02 GMT, Joeri Sykora  wrote:

> Adds support for building static libraries of the OpenJFX modules for the 
> Windows platform. Building static libraries
> is done by providing the gradle property `-PSTATIC_BUILD=true`.
> Changes include:
> 
> - add static flags for the compiler and linker
> - use `lib` for linking instead of `link`
> - only build and include version.rc once (from GlassResources.rc)
> - include library specific `JNI_OnLoad_xxx` methods
> - functionality loaded via `DllMain` has been implemented at a different 
> location

confirmed to work with VS2019 as well.

-

Marked as reviewed by jvos (Reviewer).

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

Re: [Rev 01] RFR: 8244418: MenuBar: IOOB exception on requestFocus on empty bar

[Ajit Ghaisas]

> Issue :
> https://bugs.openjdk.java.net/browse/JDK-8244418
> 
> Root Cause :
> Incorrect assumption about menu list size.
> 
> Fix :
> Added check for empty menu list before trying to access it.
> 
> Test :
> Added a unit test that fails before fix and passes after it.

Ajit Ghaisas has updated the pull request incrementally with one additional 
commit since the last revision:

  review_fixes

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/216/files
  - new: https://git.openjdk.java.net/jfx/pull/216/files/d37f9c14..15c4de98

Webrevs:
 - full: https://webrevs.openjdk.java.net/jfx/216/webrev.01
 - incr: https://webrevs.openjdk.java.net/jfx/216/webrev.00-01

  Stats: 12 lines in 1 file changed: 1 ins; 5 del; 6 mod
  Patch: https://git.openjdk.java.net/jfx/pull/216.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/216/head:pull/216

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

Re: [Rev 01] RFR: 8244418: MenuBar: IOOB exception on requestFocus on empty bar

[Ajit Ghaisas]

On Fri, 8 May 2020 13:51:34 GMT, Jeanette Winzenburg  
wrote:

>> Ajit Ghaisas has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   review_fixes
>
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java
>  line 485:
> 
>> 484: }
>> 485:
>> 486: if (focusedMenu != null && focusedMenuIndex != -1) {
> 
> don't quite understand why this change is necessary? The guard in the 
> listener seems to be enough, at least the test
> passes without this. If it's needed to fix another potentially breaking path 
> to this, would it be possible to add a
> test method that fails/passes before/after?

This was the place where exception was occurring. Hence, I added this check.
When I ran against the test, I still got the exception at caller lambda. I 
added that check later.
Wanted to remove this check - but simple code scan revealed this method gets 
called from engine.addTraverseListener()
listener with index = 0. Hence, I have kept this check. Covering this scenario 
in test seems tough.

> modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java
>  line 307:
> 
>> 306: unSelectMenus();
>> 307: if (!container.getChildren().isEmpty()) {
>> 308: menuModeStart(0);
> 
> wondering whether this would be an appropriate time to simplify the logic a 
> bit: as unSelectMenus is called in both if
> and else block, it could be condensed to
> unSelectMenus();
> if (t1 && !container.getChildren().isEmpty()) {
>...
> }
> 
> might overlook something, though

Good suggestion. Fixed.

-

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

Re: [Rev 03] RFR: 8202296: Monocle MouseInput doesn't send keyboard modifiers in events.

[Kevin Rushforth]

On Tue, 12 May 2020 12:03:36 GMT, Tom Schindl  wrote:

>> Looks good to me. I verified that the new test fails without your fix and 
>> passes with your fix.
>> 
>> In case anyone else is looking at this and wants to run the test, it isn't 
>> enabled by default, so you need to run it
>> like this:
>> gradle -PUNSTABLE_TEST=true -PFULL_TEST=true -PUSE_ROBOT=true \
>> :systemTests:test --tests test.robot.com.sun.glass.ui.monocle.RobotTest
>
> @kevinrushforth is there anything left todo or did you simply forget to merge 
> it?

@tomsontom Yes, there is something you need to do. As the author of the patch, 
you need to `/integrate` it after which
I'll `/sponsor` it.

-

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

Re: [Integrated] RFR: 8244735: Error on iOS passing keys with unicode values greater than 255

[Jose Pereda]

On Mon, 11 May 2020 12:50:48 GMT, Jose Pereda  wrote:

> With this PR, we pass directly the 16-bit unsigned short for a character 
> (unicode value) to the Java layer, avoiding
> the cast with the C++ 8-bit char, that fails for non-ascii characters like 
> euro (€) or quote (").
> We also avoid the mapping between iOS keys and JavaFX `KeyCode`, except for 
> `ENTER` and `BACK_SPACE`, as the mapping
> for some keys was wrong, like for "%", with ascii value 0x25, that was mapped 
> to KeyCode.LEFT.
> Finally, the iOS keyboard is set to `UIKeyboardTypeASCIICapable`, to prevent 
> the display of the emoji keyboard, that
> can't be currently processed.

This pull request has now been integrated.

Changeset: b14e0858
Author:Jose Pereda 
Committer: Johan Vos 
URL:   https://git.openjdk.java.net/jfx/commit/b14e0858
Stats: 39 lines in 6 files changed: 7 ins; 20 del; 12 mod

8244735: Error on iOS passing keys with unicode values greater than 255

Reviewed-by: jvos

-

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

Re: [Rev 03] RFR: 8202296: Monocle MouseInput doesn't send keyboard modifiers in events.

[Tom Schindl]

On Tue, 28 Apr 2020 22:36:47 GMT, Kevin Rushforth  wrote:

>> Tom Schindl has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   8202296: Monocle MouseInput doesn't send keyboard modifiers in events.
>>   
>>   Fix whitespace errors
>
> Looks good to me. I verified that the new test fails without your fix and 
> passes with your fix.
> 
> In case anyone else is looking at this and wants to run the test, it isn't 
> enabled by default, so you need to run it
> like this:
> gradle -PUNSTABLE_TEST=true -PFULL_TEST=true -PUSE_ROBOT=true \
> :systemTests:test --tests test.robot.com.sun.glass.ui.monocle.RobotTest

@kevinrushforth is there anything left todo or did you simply forget to merge 
it?

-

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

Re: RFR: 8244735: Error on iOS passing keys with unicode values greater than 255

[Johan Vos]

On Mon, 11 May 2020 12:50:48 GMT, Jose Pereda  wrote:

> With this PR, we pass directly the 16-bit unsigned short for a character 
> (unicode value) to the Java layer, avoiding
> the cast with the C++ 8-bit char, that fails for non-ascii characters like 
> euro (€) or quote (").
> We also avoid the mapping between iOS keys and JavaFX `KeyCode`, except for 
> `ENTER` and `BACK_SPACE`, as the mapping
> for some keys was wrong, like for "%", with ascii value 0x25, that was mapped 
> to KeyCode.LEFT.
> Finally, the iOS keyboard is set to `UIKeyboardTypeASCIICapable`, to prevent 
> the display of the emoji keyboard, that
> can't be currently processed.

This indeed fixes JDK-8244735.
It only touches ios-specific files, hence there is no impact on other platforms.

-

Marked as reviewed by jvos (Reviewer).

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