Re: [Rev 01] RFR: 8231692: Test Infrastructure: enhance KeyEventFirer to inject keyEvents into scene

[Ajit Ghaisas]

On Tue, 5 Nov 2019 15:43:03 GMT, Jeanette Winzenburg  
wrote:

> The pull request has been updated with additional changes.
> 
> 
> 
> Added commits:
>  - 0366a0a5: added copyright header
> 
> Changes:
>   - all: https://git.openjdk.java.net/jfx/pull/20/files
>   - new: https://git.openjdk.java.net/jfx/pull/20/files/aabea139..0366a0a5
> 
> Webrevs:
>  - full: https://webrevs.openjdk.java.net/jfx/20/webrev.01
>  - incr: https://webrevs.openjdk.java.net/jfx/20/webrev.00-01
> 
>   Issue: https://bugs.openjdk.java.net/browse/JDK-8231692
>   Stats: 22 lines in 1 file changed: 21 ins; 0 del; 1 mod
>   Patch: https://git.openjdk.java.net/jfx/pull/20.diff
>   Fetch: git fetch https://git.openjdk.java.net/jfx pull/20/head:pull/20

modules/javafx.controls/src/test/java/test/com/sun/javafx/scene/control/infrastructure/KeyEventFirerTest.java
 line 204:

> 203: public void setup() {
> 204: // This step is not needed (Just to make sure StubToolkit is 
> loaded into VM)
> 205: // @SuppressWarnings("unused")

Remove this commented code.

modules/javafx.controls/src/test/java/test/com/sun/javafx/scene/control/infrastructure/KeyEventFirerTest.java
 line 55:

> 54:  * Most of these tests are meant to document how to use the KeyEventFirer 
> and what
> 55:  * happens if used incorrectly. The latter are ignored, because the build 
> should pass.
> 56:  *

I see ignored tests - for false greens.
As these tests are written for new code in KeyEventFirer test infrastructure 
class, I suggest not to ignore these tests. Rather adjust asserts to pass them. 
A comment explaining the expected result should be added.

modules/javafx.controls/src/test/java/test/com/sun/javafx/scene/control/infrastructure/KeyEventFirer.java
 line 85:

> 84: 
> 85: public void doUpArrowPress(KeyModifier... modifiers) {
> 86: doKeyPress(KeyCode.UP, modifiers);

Enclose in braces {}

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

RE: RFR: 8130738: TextFlow's tab width is static

[David Grieve]

What happens if you do text.tabSizeProperty().setValue(null) ? 

> -Original Message-
> From: openjfx-dev  On Behalf Of
> Scott Palmer
> Sent: Wednesday, November 6, 2019 11:12 AM
> To: openjfx-dev@openjdk.java.net
> Subject: RFR: 8130738: TextFlow's tab width is static
> 
> Added tabSize property to Text and TextFlow and -fx-tab-size CSS attribute to
> both.  TextFlow's tab size overrides that of contained Text nodes.
> 
> 
> 
> Commits:
>  - 68d221c7: 8130738: TextFlow's tab width is static
> 
> Changes:
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.o
> penjdk.java.net%2Fjfx%2Fpull%2F32%2Ffiles&data=02%7C01%7CDavid.
> Grieve%40microsoft.com%7C6044739a564243b00a3b08d762edb49b%7C72f9
> 88bf86f141af91ab2d7cd011db47%7C1%7C0%7C637086645878076773&
> sdata=ov9cwdWgDaJxX0waFCXv3ca0CStlZm39q%2FQMNt9X%2BQU%3D&am
> p;reserved=0
>  Webrev:
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwebr
> evs.openjdk.java.net%2Fjfx%2F32%2Fwebrev.00&data=02%7C01%7CDa
> vid.Grieve%40microsoft.com%7C6044739a564243b00a3b08d762edb49b%7C
> 72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637086645878076773&a
> mp;sdata=00p7TljKs9FhkblloXDQhY74l4j3GqORzH%2ByRfwn40E%3D&re
> served=0
>   Issue:
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs
> .openjdk.java.net%2Fbrowse%2FJDK-
> 8130738&data=02%7C01%7CDavid.Grieve%40microsoft.com%7C604473
> 9a564243b00a3b08d762edb49b%7C72f988bf86f141af91ab2d7cd011db47%7
> C1%7C0%7C637086645878076773&sdata=iZyZ5RTtfjzCd5bME%2Bpf3nA
> OI%2BtE8sjJGkRMl%2BQdUL8%3D&reserved=0
>   Stats: 324 lines in 8 files changed: 260 ins; 0 del; 64 mod
>   Patch:
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.o
> penjdk.java.net%2Fjfx%2Fpull%2F32.diff&data=02%7C01%7CDavid.Grie
> ve%40microsoft.com%7C6044739a564243b00a3b08d762edb49b%7C72f988b
> f86f141af91ab2d7cd011db47%7C1%7C0%7C637086645878086767&sdat
> a=L5BuXwvyCYwG%2BEb1hktl2ahXDPXgHkeFMLDthRSr2BY%3D&reserve
> d=0
>   Fetch: git fetch
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.o
> penjdk.java.net%2Fjfx&data=02%7C01%7CDavid.Grieve%40microsoft.co
> m%7C6044739a564243b00a3b08d762edb49b%7C72f988bf86f141af91ab2d7c
> d011db47%7C1%7C0%7C637086645878086767&sdata=waZ9DLUjPqyBv
> kOrgJQDjtp6gp9Y8chyhVFlbsXplU0%3D&reserved=0
> pull/32/head:pull/32
> 
> PR:
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.o
> penjdk.java.net%2Fjfx%2Fpull%2F32&data=02%7C01%7CDavid.Grieve%
> 40microsoft.com%7C6044739a564243b00a3b08d762edb49b%7C72f988bf86f
> 141af91ab2d7cd011db47%7C1%7C0%7C637086645878086767&sdata=G
> kwyOTeBRFTdQnZ0Ovttf3vz7F4rkhrJZ9nT0pwUZrQ%3D&reserved=0

RFR: 8130738: TextFlow's tab width is static

[Scott Palmer]

Added tabSize property to Text and TextFlow and -fx-tab-size CSS attribute to 
both.  TextFlow's tab size overrides that of contained Text nodes.



Commits:
 - 68d221c7: 8130738: TextFlow's tab width is static

Changes: https://git.openjdk.java.net/jfx/pull/32/files
 Webrev: https://webrevs.openjdk.java.net/jfx/32/webrev.00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8130738
  Stats: 324 lines in 8 files changed: 260 ins; 0 del; 64 mod
  Patch: https://git.openjdk.java.net/jfx/pull/32.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/32/head:pull/32

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

FXMLLoader: not supplying filename to script engine, not supplying event object as argument to script

[Rony G. Flatscher]

Using a script engine (javax.script.ScriptEngine) for implementing a FXML 
controller there are two
important information missing in the ScriptContext.ENGINE_SCOPE Bindings 
supplied to the script used
to eval() the script code:

  * ScriptEngine.FILENAME
  o This value denotes the file name from where the script code was fetched 
that is being eval()'d.
  o When debugging script controllers in a complex JavaFX application it is 
mandatory to know
the file name the script code was taken from (as such scripts could be 
called/run from
different FXML files). Also, in the case of script runtime errors, 
usually the file name is
given by the script engine where the error has occurred to ease 
debugging, such that it is
important to really supply the filename.
  + Note: the 'location'-URL in ScriptContext.GLOBAL_SCOPE refers the 
FXML file,  not to the
file that hosts the script that gets run if using the "diff --git a/modules/javafx.fxml/src/main/java/javafx/fxml/FXMLLoader.java 
b/modules/javafx.fxml/src/main/java/javafx/fxml/FXMLLoader.java
index 7f3d2f3083..eab4541659 100644
--- a/modules/javafx.fxml/src/main/java/javafx/fxml/FXMLLoader.java
+++ b/modules/javafx.fxml/src/main/java/javafx/fxml/FXMLLoader.java
@@ -1558,6 +1558,12 @@ public class FXMLLoader {
 location = new URL(FXMLLoader.this.location, source);
 }
 
+Bindings engineBindings = 
engine.getBindings(ScriptContext.ENGINE_SCOPE);
+Bindings localBindings = engine.createBindings();
+localBindings.put(engine.FILENAME, location.getPath());
+localBindings.putAll(engineBindings);
+scriptEngine.setBindings(localBindings, 
ScriptContext.ENGINE_SCOPE);
+
 InputStreamReader scriptReader = null;
 try {
 scriptReader = new 
InputStreamReader(location.openStream(), charset);
@@ -1569,6 +1575,9 @@ public class FXMLLoader {
 scriptReader.close();
 }
 }
+
+// Restore the original bindings
+engine.setBindings(engineBindings, 
ScriptContext.ENGINE_SCOPE);
 } catch (IOException exception) {
 throw constructLoadException(exception);
 }
@@ -1582,7 +1591,16 @@ public class FXMLLoader {
 if (value != null && !staticLoad) {
 // Evaluate the script
 try {
+Bindings engineBindings = 
scriptEngine.getBindings(ScriptContext.ENGINE_SCOPE);
+Bindings localBindings = scriptEngine.createBindings();
+localBindings.put(scriptEngine.FILENAME, 
location.getPath());
+localBindings.putAll(engineBindings);
+scriptEngine.setBindings(localBindings, 
ScriptContext.ENGINE_SCOPE);
+
 scriptEngine.eval((String)value);
+
+   // Restore the original bindings
+scriptEngine.setBindings(engineBindings, 
ScriptContext.ENGINE_SCOPE);
 } catch (ScriptException exception) {
 System.err.println(exception.getMessage());
 }
@@ -1687,6 +1705,9 @@ public class FXMLLoader {
 Bindings engineBindings = 
scriptEngine.getBindings(ScriptContext.ENGINE_SCOPE);
 Bindings localBindings = scriptEngine.createBindings();
 localBindings.put(EVENT_KEY, event);
+localBindings.put(scriptEngine.ARGV, new Object[]{event});
+URL location=(URL) 
scriptEngine.getBindings(ScriptContext.GLOBAL_SCOPE).get(LOCATION_KEY);
+localBindings.put(scriptEngine.FILENAME, location.getPath());
 localBindings.putAll(engineBindings);
 scriptEngine.setBindings(localBindings, 
ScriptContext.ENGINE_SCOPE);
 

Re: [Rev 01] RFR: 8229472: Deprecate for removal JavaBeanXxxPropertyBuilders constructors

[Nir Lisker]

The pull request has been updated with additional changes.



Added commits:
 - a1cdb37b: Removed whitespaces in @Deprecated comments

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/30/files
  - new: https://git.openjdk.java.net/jfx/pull/30/files/6d29e034..a1cdb37b

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

  Issue: https://bugs.openjdk.java.net/browse/JDK-8229472
  Stats: 7 lines in 7 files changed: 0 ins; 0 del; 7 mod
  Patch: https://git.openjdk.java.net/jfx/pull/30.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/30/head:pull/30

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

Re: JDK-8130738 TextFlow's tab width is static

[Kevin Rushforth]

Hi Scott,

The CSS reference is here:

modules/javafx.graphics/src/main/docs/javafx/scene/doc-files/cssref.html

As for your question about indentation, reformatting entire files or 
large blocks of code you aren't touching is discouraged. In your 
specific case, reformatting the methods in the StyleableProperties 
nested class that are adjacent to code you add or modify seems fine, as 
long as you only change the indentation and not the line wrapping. This 
will allow reviewers to turn on "hide whitespace changes" (which is an 
option of the GitHub diffs view, and is default for the webrev).


-- Kevin


On 11/5/2019 6:03 PM, Scott Palmer wrote:

I finally had a moment to get back at this.

I've changed the name of the property from tabWidth to tabSize and 
implemented the CSS property '-fx-tab-size'. The CSS documentation 
will need to be updated. I didn't see where the CSS document is 
located in the source tree.


While adding CSS support I noticed that in both Text.java and 
TextFlow.java the indentation is wrong for the StyleableProperties 
nested class (has one extra space).  I wasn't sure how much I should 
change in the scope of this feature.  If I don't fix some indenting my 
changes will look funny beside the lines of existing code.  If I fix 
all of the broken indenting I'll change a lot of lines that have 
nothing to do with this feature. If I just properly indent the lines I 
change or add, the indenting will be more chaotic than it is currently.


I have noticed a rendering anomaly with a tab size of zero (the value 
I was initially clamping to) and rather than dealing with that I took 
the easy way out and changed it to agree with the minimum of 1 that 
Kevin indicated for the range.


I have not limited the maximum tab size.  I know Phil has expressed 
that MAXINT is not his preference, but I can't think of a good 
criteria for what the maximum should be.  I can imagine someone using 
a tab separator to align columns of text that have much more than 16 
characters for example.  I did a quick test with the tab size set to 
Integer.MAX_VALUE-1 and nothing blew up.  Of course the text after the 
tab was nowhere to be seen.



Regards,

Scott


On Thu, Oct 17, 2019 at 3:42 PM Phil Race > wrote:




On 10/17/19 12:32 PM, Kevin Rushforth wrote:
> It seems that you have a path forward then. So, there are a couple
> questions to answer:
>
> 1. Should the type of the property be int or double? If it
really is
> only ever going to be a number of spaces, then int seems fine.
That,
> along with the name of the property, would underscore the fact that
> no, this isn't a size where units make sense; it's a number of
spaces.

A discrete number of spaces, so int.

>
> 2. Should the range be [1,MAXINT] or [1,16] or [1,something-else]?
> Once that is decided, I think clamp on use would be the most
> consistent with other similar properties, but that's worth checking.

If we go with "MAXINT" (which is not my preference), I think there'd
need to be some testing of
the various code paths and pipelines to be sure that various
values from
large, through extreme,
all do something sensible, and of course, no exceptions or crashes.

Various code paths means Text, TextFlow,  linewrapping or not ...
complex text and simple text too.
Perhaps complex text should be tested anyway, although I've been
assuming that we are handling
tab spacing outside of that but didn't verify it.

-phil.

>
> -- Kevin
>
>
> On 10/17/2019 12:11 PM, Scott Palmer wrote:
>> You’re right.  Something I was reading indicated that while
there was
>> no default unit, ‘px’ was implicitly appended.  I can’t find that
>> now. Go figure.
>>
>> Everything I find now about CSS tab-size is in agreement with what
>> David wrote.  It’s a number of spaces.  With units it would be a
>> ‘length’ but nothing supports that.
>>
>> So I think it makes sense to add -fx-tab-size as a CSS
property, only
>> supporting a number with no units.
>>
>> Scott
>>
>>
>>> On Oct 17, 2019, at 2:32 PM, Phil Race mailto:philip.r...@oracle.com>> wrote:
>>>
>>> Really ? It defaults to pixels ? Is that just inherited as
being the
>>> default CSS unit ?
>>> Is that FX's implementation
>>>
>>> Hmm. A bit of reading about web CSS says that strictly anything
>>> without an explicit unit should be ignored.
>>> The only exception is zero, where it doesn't matter.
>>>
>>> Eg see :
>>>
https://stackoverflow.com/questions/7907749/default-unit-for-font-size
>>>
>>> Although I am sure there are more authoratative sources than that.
>>>
>>> -phil.
>>>
>>> On 10/17/19 11:22 AM, Scott Palmer wrote:
 So do we go ahead and implement tabSize without the CSS support?
 I’m not sure 

Re: RFR: 8229472: Deprecate JavaBeanXxxPropertyBuilders public constructors

[Kevin Rushforth]

On Wed, 6 Nov 2019 11:50:27 GMT, Nir Lisker  wrote:

> On Wed, 6 Nov 2019 07:12:26 GMT, Robin Westberg  wrote:
> 
>> On Wed, 6 Nov 2019 05:07:56 GMT, Kevin Rushforth  wrote:
>> 
>>> On Wed, 6 Nov 2019 05:04:28 GMT, Kevin Rushforth  wrote:
>>> 
 On Tue, 5 Nov 2019 18:10:57 GMT, Nir Lisker  wrote:
 
> https://bugs.openjdk.java.net/browse/JDK-8229472
> 
> CSR will be created after the changes are approved.
> 
> 
> 
> Commits:
>  - 6d29e034: Initial push of 8229472
> 
> Changes: https://git.openjdk.java.net/jfx/pull/30/files
>  Webrev: https://webrevs.openjdk.java.net/jfx/30/webrev.00
>   Issue: https://bugs.openjdk.java.net/browse/JDK-8229472
>   Stats: 14 lines in 7 files changed: 7 ins; 0 del; 7 mod
>   Patch: https://git.openjdk.java.net/jfx/pull/30.diff
>   Fetch: git fetch https://git.openjdk.java.net/jfx pull/30/head:pull/30
 
 modules/javafx.base/src/main/java/javafx/beans/property/adapter/JavaBeanBooleanPropertyBuilder.java
  line 68:
 
> 67: @Deprecated(since = "14", forRemoval = true)
> 68: public JavaBeanBooleanPropertyBuilder() {}
> 69: 
 
 Minor: I checked other places in JavaFX and JDK, and they consistently 
 omit the spaces surrounding the `=`.
>>> 
>>> I changed the bug summary to include `for removal` in the title. Can you 
>>> change the PR title to match?
>> 
>> Thanks for the notification, looks like GitHub returned 500 for a few 
>> minutes. This seem to happen from time to time, so nice to know that the 
>> retry logic works. :)
> 
> For both `since` and `forRemoval`?

Yes. The pattern used consistently (in all cases that I could find) would be:

@Deprecated(since="14", forRemoval=true)

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

Re: [Integrated] RFR: 8230492: font-family not set in HTMLEditor if font name has a number in it

[Kevin Rushforth]

Changeset: 286d1b54
Author:Arun Joseph 
Committer: Kevin Rushforth 
Date:  2019-11-06 12:43:43 +
URL:   https://git.openjdk.java.net/jfx/commit/286d1b54

8230492: font-family not set in HTMLEditor if font name has a number in it

Reviewed-by: kcr, shadzic

! modules/javafx.web/src/main/java/javafx/scene/web/HTMLEditorSkin.java
! tests/system/src/test/java/test/javafx/scene/web/HTMLEditorTest.java
= 
tests/system/src/test/resources/test/javafx/scene/web/WebKit_Layout_Tests_2.ttf

Re: RFR: 8229472: Deprecate JavaBeanXxxPropertyBuilders public constructors

[Nir Lisker]

On Wed, 6 Nov 2019 07:12:26 GMT, Robin Westberg  wrote:

> On Wed, 6 Nov 2019 05:07:56 GMT, Kevin Rushforth  wrote:
> 
>> On Wed, 6 Nov 2019 05:04:28 GMT, Kevin Rushforth  wrote:
>> 
>>> On Tue, 5 Nov 2019 18:10:57 GMT, Nir Lisker  wrote:
>>> 
 https://bugs.openjdk.java.net/browse/JDK-8229472
 
 CSR will be created after the changes are approved.
 
 
 
 Commits:
  - 6d29e034: Initial push of 8229472
 
 Changes: https://git.openjdk.java.net/jfx/pull/30/files
  Webrev: https://webrevs.openjdk.java.net/jfx/30/webrev.00
   Issue: https://bugs.openjdk.java.net/browse/JDK-8229472
   Stats: 14 lines in 7 files changed: 7 ins; 0 del; 7 mod
   Patch: https://git.openjdk.java.net/jfx/pull/30.diff
   Fetch: git fetch https://git.openjdk.java.net/jfx pull/30/head:pull/30
>>> 
>>> modules/javafx.base/src/main/java/javafx/beans/property/adapter/JavaBeanBooleanPropertyBuilder.java
>>>  line 68:
>>> 
 67: @Deprecated(since = "14", forRemoval = true)
 68: public JavaBeanBooleanPropertyBuilder() {}
 69: 
>>> 
>>> Minor: I checked other places in JavaFX and JDK, and they consistently omit 
>>> the spaces surrounding the `=`.
>> 
>> I changed the bug summary to include `for removal` in the title. Can you 
>> change the PR title to match?
> 
> Thanks for the notification, looks like GitHub returned 500 for a few 
> minutes. This seem to happen from time to time, so nice to know that the 
> retry logic works. :)

For both `since` and `forRemoval`?

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

RFR: 8231188: Update SQLite to version 3.30.1

[Arun Joseph]

We currently use SQLite version 3.28.0. We should update to the latest stable 
release version 3.30.1 released.
https://www.sqlite.org/index.html



Commits:
 - 24c6375d: 8231188: Update SQLite to version 3.30.1

Changes: https://git.openjdk.java.net/jfx/pull/31/files
 Webrev: https://webrevs.openjdk.java.net/jfx/31/webrev.00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8231188
  Stats: 7934 lines in 4 files changed: 3028 ins; 734 del; 4172 mod
  Patch: https://git.openjdk.java.net/jfx/pull/31.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/31/head:pull/31

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

Re: [Rev 01] RFR: 8230492: font-family not set in HTMLEditor if font name has a number in it

[Hadzic Samir]

On Tue, 5 Nov 2019 11:17:55 GMT, Arun Joseph 
 wrote:

> The pull request has been updated with additional changes.
> 
> 
> 
> Added commits:
>  - afc7f17a: Minor formatting
> 
> Changes:
>   - all: https://git.openjdk.java.net/jfx/pull/27/files
>   - new: https://git.openjdk.java.net/jfx/pull/27/files/5a1fbade..afc7f17a
> 
> Webrevs:
>  - full: https://webrevs.openjdk.java.net/jfx/27/webrev.01
>  - incr: https://webrevs.openjdk.java.net/jfx/27/webrev.00-01
> 
>   Issue: https://bugs.openjdk.java.net/browse/JDK-8230492
>   Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
>   Patch: https://git.openjdk.java.net/jfx/pull/27.diff
>   Fetch: git fetch https://git.openjdk.java.net/jfx pull/27/head:pull/27

Approved by shadzic (Author).

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

Re: RFR: 8200224: Multiple press event when JFXPanel gains focus

[Kevin Rushforth]

On Tue, 29 Oct 2019 11:27:35 GMT, Florian Kirmaier 
 wrote:

> Original PR: https://github.com/javafxports/openjdk-jfx/pull/591
> 
> A side effect of JDK-8200224 is, that the first click on a JFXPanel is always 
> a double click.
> The cause for JDK-8200224 is the fix for JDK-8087914 (Clicking on Menu in 
> JFXPanel ignored if another swing component has focus).
> This fix introduced synthesized press-events, which is an unsafe fix for the 
> problem.
> 
> The pull request introduces a new fix for the problem.
> Instead of simulating new input-events, we set JavaFX Scene/Window to focused.
> We do so, by using setFocused.
> When the original Swing-Focus event is called slightly later, it won't have 
> any side-effects,
> because calling setFocused just sets the value of a boolean property.
> 
> I've now also added a unit-test for the problem.
> 
> 
> 
> Commits:
>  - 314e423c: JDK-8200224
>  - 725e5fef: JDK-8200224
> 
> Changes: https://git.openjdk.java.net/jfx/pull/25/files
>  Webrev: https://webrevs.openjdk.java.net/jfx/25/webrev.00
>   Issue: https://bugs.openjdk.java.net/browse/JDK-8200224
>   Stats: 140 lines in 2 files changed: 133 ins; 2 del; 5 mod
>   Patch: https://git.openjdk.java.net/jfx/pull/25.diff
>   Fetch: git fetch https://git.openjdk.java.net/jfx pull/25/head:pull/25

modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line 457:

> 456: 
> 457: sendMouseEventToFX(e);
> 458: super.processMouseEvent(e);

typo: save --> safe

modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java 
line 2:

> 1: /*
> 2:  * Copyright (c) 2014, 2016 Oracle and/or its affiliates. All rights 
> reserved.
> 3:  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.

replace `2014, 2016` with `2019,`

modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java 
line 28:

> 27: 
> 28: import javafx.application.Application;
> 29: import javafx.application.Platform;

Remove unused import (several of the below imports are similarly unused). Also, 
since this is a new test, please sort the imports.

modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java 
line 48:

> 47: 
> 48: import static junit.framework.Assert.assertEquals;
> 49: import static org.junit.Assert.assertNotNull;

This method should be imported from `org.junit` package.

modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java 
line 116:

> 115: MouseEvent e = new MouseEvent(fxPnl, 
> MouseEvent.MOUSE_PRESSED, 0, MouseEvent.BUTTON1_DOWN_MASK,
> 116: 5, 5, 1, false, MouseEvent.BUTTON1);
> 117: 

This doesn't appear to trigger the bug -- at least on Windows. The test passes 
for me with or without your fix. You might consider moving it to the system 
tests project, under `tests/system/src/test/java/test/robot` and using `Robot` 
to effect the mouse press.

modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java 
line 57:

> 56: 
> 57: 
> 58: @BeforeClass

You can remove the extra blank line.

modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java 
line 65:

> 64: 
> 65: 
> 66: try {

You can remove the extra blank line.

modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java 
line 79:

> 78: 
> 79: class TestFXPanel extends JFXPanel {
> 80: protected void processMouseEventPublic(MouseEvent e) {

I think this can be a static class.

modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java 
line 88:

> 87: 
> 88: CountDownLatch firstPressedEventLatch = new CountDownLatch(1);
> 89: 

This can be final.

modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java 
line 91:

> 90: // It's an array, so we can mutate it inside of lambda statement
> 91: int[] pressedEventCounter = {0};
> 92: 

This can be final.

modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java 
line 132:

> 131: 
> 132: 
> 133: }

You can remove the extra blank line.

modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java 
line 123:

> 122: 
> 123: if(!firstPressedEventLatch.await(5000, TimeUnit.MILLISECONDS)) {
> 124: throw new Exception();

Add a space after the `if`.

The fix seems fine. Have you tested it on all three platforms?

I have several comments on the test.



Disapproved by kcr (Lead).

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