Re: RFR: 8236651: Simplify and update glass gtk backend

[Thiago Milczarek Sayao]

On Wed, 1 Apr 2020 03:21:31 GMT, Thiago Milczarek Sayao  
wrote:

>> Ubuntu 20.04 Test Results
>> 
>> Updated April 2nd.
>> 
>> ![image](https://user-images.githubusercontent.com/30704286/78299385-28a23d80-750c-11ea-9edd-ac264f16c194.png)
>
> Test on 16.04 (without Webkit - it does not build on 16.04 anymore)
> 
> Updated April 2nd.
> 
> ![image](https://user-images.githubusercontent.com/30704286/78316727-c14db300-7536-11ea-86e9-4d5c56e4d92c.png)
> 
> Note: DatePickerTest works when run alone.

I will keep testing it, but I think it's looking good.

-

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

Re: [Rev 42] RFR: 8236651: Simplify and update glass gtk backend

[Thiago Milczarek Sayao]

> This proposed change does the following:
> 
> - Ports DND target to use GTK reducing code size and adding extra text / 
> image formats (such as .gif);
> - Use gtk signals instead of gdk events (also to reduce code size);
> - Simplifies geometry (sizing/positioning) with a more straightforward code 
> (less special cases), making it easier to
>   understand and maintain;
> - Replaces (pointer and focus) grabbing with a gtk approach;
> - Reworked frame extents (the wm extension to get decoration sizes) to reduce 
> size and complexity;
> - Simplified cursor changing (for gtk3);
> - Reduced the use of gtk/gdk deprecated functions;
> - Removes Applet/Web Start code;
> - Fixes https://bugs.openjdk.java.net/browse/JDK-8237491;
> 
> In general it reduces code size and complexity and hands more work to gtk.
> 
> Updated on 2020-01-29:
> ![image](https://user-images.githubusercontent.com/30704286/73354728-2ce47d00-4275-11ea-935c-414fc26163d7.png)

Thiago Milczarek Sayao has refreshed the contents of this pull request, and 
previous commits have been removed. The
incremental views will show differences compared to the previous content of the 
PR. The pull request contains one new
commit since the last revision:

  JDK-8236651 Simplify and update glass gtk backend
  
  Cleaning
  
  Cleaning + change year to 2020
  
  Fix crash
  
  Fix crash
  
  Fix crash
  
  Revert idea files
  
  Fix flickering and sizing issues
  
  Pass more tests
  
  Small fixes
  
  Use gtk_window_set_default_size for before-map sizing which is the 
appropriate function
  
  Better alternative calculation for no _NET_FRAME_EXTENTS WM extension
  
  Fix dialog with owner sizing
  
  Maybe fix background
  
  Big cleanup
  
  Allow undecorated windows to be maximized.
  
  Mouse pointer grab
  
  Work on mouse grab
  
  8233747: JVM crash in com.sun.webkit.dom.DocumentImpl.createAttribute
  
  Reviewed-by: kcr, ghb
  
  8234474: [macos 10.15] Crash in file dialog in sandbox mode
  
  Reviewed-by: arapte, prr
  
  8236648: javadoc warning on Text::tabSizeProperty method
  
  Reviewed-by: kcr
  
  8233798: Ctrl-L character mistakenly removed from gstreamer.md
  
  Reviewed-by: almatvee
  
  8232589: Remove CoreAudio Utility Classes
  
  Reviewed-by: kcr, jvos
  
  8236448: Remove unused and repair broken Android/Dalvik code
  
  Reviewed-by: kcr
  
  8236808: javafx_iio can not be used in static environment
  
  Reviewed-by: kcr
  
  8236733: Change JavaFX release version to 15
  
  Reviewed-by: arapte
  
  8232128: Better formatting for numbers
  
  Reviewed-by: rhalade, ghb
  
  8232214: Improved internal validations
  
  Reviewed-by: ghb, rhalade
  
  8237078: [macOS] Media build broken on XCode 11
  
  Reviewed-by: kcr, almatvee
  
  JDK-8236651 Simplify and update glass gtk backend
  
  Cleaning
  
  Cleaning + change year to 2020
  
  Fix crash
  
  Fix crash
  
  Fix crash
  
  Revert idea files
  
  Fix flickering and sizing issues
  
  Pass more tests
  
  Small fixes
  
  Use gtk_window_set_default_size for before-map sizing which is the 
appropriate function
  
  Better alternative calculation for no _NET_FRAME_EXTENTS WM extension
  
  Fix dialog with owner sizing
  
  Maybe fix background
  
  Big cleanup
  
  Allow undecorated windows to be maximized.
  
  Mouse pointer grab
  
  Work on mouse grab
  
  Fix Initial Size
  
  Revert "Fix Initial Size"
  
  This reverts commit 0c982d60
  
  Better fix for initial size
  
  8157224: isNPOTSupported check is too strict
  
  Reviewed-by: kcr
  
  8233942: Update to 609.1 version of WebKit
  
  Co-authored-by: Guru HB 
  Co-authored-by: Arun Joseph 
  Co-authored-by: Kevin Rushforth 
  Reviewed-by: kcr, jvos, ajoseph
  
  8236753: Animations do not play backwards after being stopped
  
  Reviewed-by: kcr, arapte
  
  8237823: Mark TextTest.testTabSize as unstable
  
  Reviewed-by: prr
  
  8236912: NullPointerException when clicking in WebView with Button 4 or 
Button 5
  
  Reviewed-by: ghb, kcr
  
  8232824: Removing TabPane with strong referenced content causes memory leak 
from weak one
  
  Reviewed-by: kcr, aghaisas
  
  8237372: NullPointerException in TabPaneSkin.stopDrag
  
  Reviewed-by: arapte
  
  8237003: Remove hardcoded WebAnimationsCSSIntegrationEnabled flag in 
DumpRenderTree
  
  Reviewed-by: kcr
  
  8238249: GetPrimitiveArrayCritical passed with hardcoded FALSE value
  
  Reviewed-by: kcr
  
  8088198: Exception thrown from snapshot if dimensions are larger than max 
texture size
  
  Reviewed-by: arapte, kcr
  
  8237833: Check glyph size before adding to glyph texture cache
  
  Reviewed-by: kcr
  
  8237782: Only read advances up to the minimum of the numHorMetrics or the 
available font data.
  
  Reviewed-by: kcr
  
  8237770: Error creating fragment phong shader on iOS
  
  Reviewed-by: kcr
  
  8237944: webview native cl "-m32" unknown option for windows 32-bit build
  
  Reviewed-by: kcr
  
  8231513: JavaFX cause Keystroke Receiving prompt on MacOS 10.15 

Re: [Rev 41] RFR: 8236651: Simplify and update glass gtk backend

[Thiago Milczarek Sayao]

> This proposed change does the following:
> 
> - Ports DND target to use GTK reducing code size and adding extra text / 
> image formats (such as .gif);
> - Use gtk signals instead of gdk events (also to reduce code size);
> - Simplifies geometry (sizing/positioning) with a more straightforward code 
> (less special cases), making it easier to
>   understand and maintain;
> - Replaces (pointer and focus) grabbing with a gtk approach;
> - Reworked frame extents (the wm extension to get decoration sizes) to reduce 
> size and complexity;
> - Simplified cursor changing (for gtk3);
> - Reduced the use of gtk/gdk deprecated functions;
> - Removes Applet/Web Start code;
> - Fixes https://bugs.openjdk.java.net/browse/JDK-8237491;
> 
> In general it reduces code size and complexity and hands more work to gtk.
> 
> Updated on 2020-01-29:
> ![image](https://user-images.githubusercontent.com/30704286/73354728-2ce47d00-4275-11ea-935c-414fc26163d7.png)

Thiago Milczarek Sayao has updated the pull request incrementally with one 
additional commit since the last revision:

  Fix unfullscreen bug on older gtk+

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/77/files
  - new: https://git.openjdk.java.net/jfx/pull/77/files/031d2287..11688aee

Webrevs:
 - full: https://webrevs.openjdk.java.net/jfx/77/webrev.41
 - incr: https://webrevs.openjdk.java.net/jfx/77/webrev.40-41

  Stats: 19 lines in 2 files changed: 19 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jfx/pull/77.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/77/head:pull/77

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

Re: RFR: 8129123: ComboBox popup list view does not scrollTo when ComboBox value/selection is set

[Craig Cavanaugh]

On Thu, 2 Apr 2020 22:20:18 GMT, Kevin Rushforth  wrote:

> Can you please provide a unit test? One that fails before your fix and passes 
> after your fix.

I can provide a manual test the next couple of days that demonstrates it before 
and after, but I'm not sure how to
create an automated unit test because the issue is visual.

-

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

Re: A WIP for JDK-8238080 for review/discussion (Re: "Internal review ID 9063426: "FXMLLoader: if script engines implement javax.script.Compilabel compile scripts"

[Kevin Rushforth]

Hi Rony,

I see that you updated the PR and sent it for review.

Before we formally review it in the PR, let's finish the discussion as 
to whether this is a useful feature, and if so, what form this feature 
should take.


From my point of view, this does seem like a useful feature. Would 
other users of FXML benefit from it?


I'm not certain whether we want it to be implicit, compiling the script 
if the script engine in question implements Compilable, or via a new 
keyword or tag. What are the pros / cons?


What do others think?

In either case, we would need to make sure that this doesn't present any 
security concerns in the presence of a security manager. As long as a 
user-provided class is on the stack, it will be fine, but we would need 
to ensure that.


-- Kevin


On 4/2/2020 10:41 AM, Rony G. Flatscher wrote:

After merging master, applying some fixes and changing the title to reflect the 
change from WIP to a
pull request I would kindly request a review of this pull request!

Here the URL: , title changed to: 
"8238080: FXMLLoader: if
script engines implement javax.script.Compilable compile scripts".

---rony


On 28.02.2020 19:22, Rony G. Flatscher wrote:

Here is a WIP [1] implementation of [2]. The WIP is based on [3], which is 
currently in review, and
has an appropriate test unit going with it as well, please review.

There should be no compatibility issue with this implementation.

Discussion: another solution could be to not compile by default. Rather 
compile, if some new
information is present with the FXML file which cannot possibly be present in 
existing FXML files.
In this scenario one possible and probably simple solution would be to only 
compile scripts if the
language process instruction (e.g. ) contains an appropriate 
attribute with a value
indicating that compilation should be carried out (e.g.: compile="true"). This 
way only FXML with
PIs having this attribute set to true would be affected. If desired I could try 
to come up with a
respective solution as well.

---rony

[1] "Implementation and test unit": 

[2] "JDK-8238080 : FXMLLoader: if script engines implement javax.script.Compilable 
compile scripts":


[3] "8234959: FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and 
ARGV":



On 24.01.2020 16:26, Kevin Rushforth wrote:


Thank you for filing this enhancement request. As an enhancement it should be 
discussed on this
list before proceeding with a pull request (although a "WIP" or Draft PR can be 
used to illustrate
the concept).

For my part, this seems like a reasonable enhancement, as long as there are no 
compatibility
issues, but it would be good to hear from application developers who heavily 
use FXML.

-- Kevin


On 1/24/2020 7:21 AM, Rony G. Flatscher wrote:

Just filed a RFE with the following information:

    * Component:
    o JavaFX

    * Subcomponent:
    o fxml: JavaFX FXML

    * Synopsis:
    o "FXMLLoader: if script engines implement javax.script.Compilabel compile 
scripts"

    * Descriptions:
    o "FXMLLoader is able to execute scripts in Java script languages 
(javax.script.ScriptEngine
  implementations) if such a Java script language gets defined as the 
controller language in
  the 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 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 repetitevly invoked and evaluated."

    * System /OS/Java Runtime Information:
    o "All systems."

Received the internal review ID: "9063426"

---rony

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

[Rony G . Flatscher]

TODO: ADD DESCRIPTION OF PROPOSED FIX HERE.

-

Commit messages:
 - Correct error constant.
 - appease the jcheck script, such that that important trailing Whitespace 
error can be resolved (why would jcheck not do it automagically as well as 
expanding tabs?)
 - Merge master, correct ovesights.
 - Merge master
 - JDK-8238080 : FXMLLoader: if script engines implement 
javax.script.Compilable compile scripts
 - corrected wrong test string
 - Removed executable bit from gradlew as this causes an error when
 - Merge branch 'scripttest' into script as requested by reviewer Kevin
 - Incorporating Kevin Rushfort's review comments to this WIP.
 - Incorporating Kevin Rushfort's review comments to this WIP.
 - Removed/replaced white space.
 - Removed/replaced white space.
 - test for  JDK-8234959: 
FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and ARGV
 - fix for  JDK-8234959: 
FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and ARGV

Changes: https://git.openjdk.java.net/jfx/pull/129/files
 Webrev: https://webrevs.openjdk.java.net/jfx/129/webrev.00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8238080
  Stats: 1003 lines in 17 files changed: 996 ins; 1 del; 6 mod
  Patch: https://git.openjdk.java.net/jfx/pull/129.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/129/head:pull/129

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

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

[Kevin Rushforth]

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

> TODO: ADD DESCRIPTION OF PROPOSED FIX HERE.

We will need to finish the discussion on this proposed new feature on the 
openjfx-dev mailing list. The PR can be used
to illustrate what you want to do, but use [this email
thread](https://mail.openjdk.java.net/pipermail/openjfx-dev/2020-April/025623.html),
 rather than as a reply to the PR,
for the discussion.

Once we have agreement, then the next step will be to review the proposed spec 
change and implementation in this PR.

-

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

Re: RFR: 8129123: ComboBox popup list view does not scrollTo when ComboBox value/selection is set

[Kevin Rushforth]

On Thu, 2 Apr 2020 22:20:18 GMT, Kevin Rushforth  wrote:

>> This pull request fixes JDK-8129123.  This bug also effects Windows and 
>> Linux platforms.
>> Also, I believe JDK-8196037 is a duplicate of this issue.
>> 
>> I've tested this against OpenJDK 11.0.6 on Ubuntu 18.04, Arch Linux and 
>> Windows 10.
>> 
>> Thanks!
>
> Can you please provide a unit test? One that fails before your fix and passes 
> after your fix.

@aghaisas can you be one of the reviewers?

-

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

Re: RFR: 8129123: ComboBox popup list view does not scrollTo when ComboBox value/selection is set

[Kevin Rushforth]

On Wed, 4 Mar 2020 22:43:45 GMT, Craig Cavanaugh 
 wrote:

> This pull request fixes JDK-8129123.  This bug also effects Windows and Linux 
> platforms.
> Also, I believe JDK-8196037 is a duplicate of this issue.
> 
> I've tested this against OpenJDK 11.0.6 on Ubuntu 18.04, Arch Linux and 
> Windows 10.
> 
> Thanks!

Can you please provide a unit test? One that fails before your fix and passes 
after your fix.

-

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

RFR: 8236971: [macos] Gestures handled incorrectly due to missing events

[Kevin Rushforth]

As noted in the JBS issue, this bug is a result of a deliberate change by Apple 
that affects applications (in this case
the JDK) linked against MacOSX 10.11 SDK or later -- they no longer call two 
methods that we are relying on,
`beginGestureWithEvent` and `endGestureWithEvent`. There is no deprecation 
warning or any other indication at compile
time or runtime that these methods are ineffective. They just stopped calling 
them. It is documented in their developer
guide:

[developer.apple.com/documentation/appkit/nsresponder/1526368-begingesturewithevent](https://developer.apple.com/documentation/appkit/nsresponder/1526368-begingesturewithevent?language=objc)

Note that it's the version of the MacOSX SDK that the JDK is linked with that 
matters. JavaFX gesture notification
works when run on a JDK that was linked against the 10.10 SDK or earlier 
(regardless of what MacOSX SDK was used to
link the JavaFX libraries). JavaFX gesture notification fails when run on a JDK 
that was linked against the 10.11 SDK
or later.

The solution, as indicated in the Apple documentation referred to above, is to 
use the phase information from gesture
events to track when to call begin / end gesture.

The fix does the following things:
1. Removes the `beginGestureWithEvent` and `endGestureWithEvent` responder 
methods in `GlassView2D` and `GlassView3D`
2. Calls new local methods from each gesture to track when to call the 
associated `GlassViewDelegate` notification
methods, `sendJavaGestureBeginEvent` and `sendJavaGestureEndEvent`

I've tested this on macOS 10.13.6 and 10.15.3 (Catalina) and the attached 
program now runs correctly. I also tested the
GestureEvent sample in Ensemble8 and it reproduces the problem before the fix 
and works correctly after the fix.

I instrumented the code with print statements (which I have since reverted) and 
verified that the stream of events is
as expected, and matches JDK 8u241 with bundled FX.

-

Commit messages:
 - Revert "TEMPORARY: add debugging print statements (will be reverted before 
final review)"
 - TEMPORARY: add debugging print statements (will be reverted before final 
review)
 - 8236971: [macos] Gestures handled incorrectly due to missing events

Changes: https://git.openjdk.java.net/jfx/pull/156/files
 Webrev: https://webrevs.openjdk.java.net/jfx/156/webrev.00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8236971
  Stats: 117 lines in 4 files changed: 85 ins; 20 del; 12 mod
  Patch: https://git.openjdk.java.net/jfx/pull/156.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/156/head:pull/156

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

Re: RFR: 8236971: [macos] Gestures handled incorrectly due to missing events

[Kevin Rushforth]

On Thu, 2 Apr 2020 14:55:35 GMT, Kevin Rushforth  wrote:

> As noted in the JBS issue, this bug is a result of a deliberate change by 
> Apple that affects applications (in this case
> the JDK) linked against MacOSX 10.11 SDK or later -- they no longer call two 
> methods that we are relying on,
> `beginGestureWithEvent` and `endGestureWithEvent`. There is no deprecation 
> warning or any other indication at compile
> time or runtime that these methods are ineffective. They just stopped calling 
> them. It is documented in their developer
> guide:
> [developer.apple.com/documentation/appkit/nsresponder/1526368-begingesturewithevent](https://developer.apple.com/documentation/appkit/nsresponder/1526368-begingesturewithevent?language=objc)
> Note that it's the version of the MacOSX SDK that the JDK is linked with that 
> matters. JavaFX gesture notification
> works when run on a JDK that was linked against the 10.10 SDK or earlier 
> (regardless of what MacOSX SDK was used to
> link the JavaFX libraries). JavaFX gesture notification fails when run on a 
> JDK that was linked against the 10.11 SDK
> or later.   The solution, as indicated in the Apple documentation referred to 
> above, is to use the phase information
> from gesture events to track when to call begin / end gesture.  The fix does 
> the following things: 1. Removes the
> `beginGestureWithEvent` and `endGestureWithEvent` responder methods in 
> `GlassView2D` and `GlassView3D` 2. Calls new
> local methods from each gesture to track when to call the associated 
> `GlassViewDelegate` notification methods,
> `sendJavaGestureBeginEvent` and `sendJavaGestureEndEvent`  I've tested this 
> on macOS 10.13.6 and 10.15.3 (Catalina) and
> the attached program now runs correctly. I also tested the GestureEvent 
> sample in Ensemble8 and it reproduces the
> problem before the fix and works correctly after the fix.  I instrumented the 
> code with print statements (which I have
> since reverted) and verified that the stream of events is as expected, and 
> matches JDK 8u241 with bundled FX.

@arapte Can you be one of the reviewers?

@mipastgt I ran it against your test application, but if you have other tests 
you could run, that would be helpful.

-

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

Re: [Rev 40] RFR: 8236651: Simplify and update glass gtk backend

[Thiago Milczarek Sayao]

> This proposed change does the following:
> 
> - Ports DND target to use GTK reducing code size and adding extra text / 
> image formats (such as .gif);
> - Use gtk signals instead of gdk events (also to reduce code size);
> - Simplifies geometry (sizing/positioning) with a more straightforward code 
> (less special cases), making it easier to
>   understand and maintain;
> - Replaces (pointer and focus) grabbing with a gtk approach;
> - Reworked frame extents (the wm extension to get decoration sizes) to reduce 
> size and complexity;
> - Simplified cursor changing (for gtk3);
> - Reduced the use of gtk/gdk deprecated functions;
> - Removes Applet/Web Start code;
> - Fixes https://bugs.openjdk.java.net/browse/JDK-8237491;
> 
> In general it reduces code size and complexity and hands more work to gtk.
> 
> Updated on 2020-01-29:
> ![image](https://user-images.githubusercontent.com/30704286/73354728-2ce47d00-4275-11ea-935c-414fc26163d7.png)

Thiago Milczarek Sayao has updated the pull request incrementally with one 
additional commit since the last revision:

  More fixes for 16.04 (i mean gtk+ version that ships on 16.04, plus the 
different window manager - Unity).

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/77/files
  - new: https://git.openjdk.java.net/jfx/pull/77/files/e499a54d..031d2287

Webrevs:
 - full: https://webrevs.openjdk.java.net/jfx/77/webrev.40
 - incr: https://webrevs.openjdk.java.net/jfx/77/webrev.39-40

  Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jfx/pull/77.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/77/head:pull/77

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

Re: [Rev 39] RFR: 8236651: Simplify and update glass gtk backend

[Thiago Milczarek Sayao]

> This proposed change does the following:
> 
> - Ports DND target to use GTK reducing code size and adding extra text / 
> image formats (such as .gif);
> - Use gtk signals instead of gdk events (also to reduce code size);
> - Simplifies geometry (sizing/positioning) with a more straightforward code 
> (less special cases), making it easier to
>   understand and maintain;
> - Replaces (pointer and focus) grabbing with a gtk approach;
> - Reworked frame extents (the wm extension to get decoration sizes) to reduce 
> size and complexity;
> - Simplified cursor changing (for gtk3);
> - Reduced the use of gtk/gdk deprecated functions;
> - Removes Applet/Web Start code;
> - Fixes https://bugs.openjdk.java.net/browse/JDK-8237491;
> 
> In general it reduces code size and complexity and hands more work to gtk.
> 
> Updated on 2020-01-29:
> ![image](https://user-images.githubusercontent.com/30704286/73354728-2ce47d00-4275-11ea-935c-414fc26163d7.png)

Thiago Milczarek Sayao has updated the pull request incrementally with one 
additional commit since the last revision:

  Just comment fixing

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/77/files
  - new: https://git.openjdk.java.net/jfx/pull/77/files/547c3cb0..e499a54d

Webrevs:
 - full: https://webrevs.openjdk.java.net/jfx/77/webrev.39
 - incr: https://webrevs.openjdk.java.net/jfx/77/webrev.38-39

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

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

Re: [Rev 38] RFR: 8236651: Simplify and update glass gtk backend

[Thiago Milczarek Sayao]

> This proposed change does the following:
> 
> - Ports DND target to use GTK reducing code size and adding extra text / 
> image formats (such as .gif);
> - Use gtk signals instead of gdk events (also to reduce code size);
> - Simplifies geometry (sizing/positioning) with a more straightforward code 
> (less special cases), making it easier to
>   understand and maintain;
> - Replaces (pointer and focus) grabbing with a gtk approach;
> - Reworked frame extents (the wm extension to get decoration sizes) to reduce 
> size and complexity;
> - Simplified cursor changing (for gtk3);
> - Reduced the use of gtk/gdk deprecated functions;
> - Removes Applet/Web Start code;
> - Fixes https://bugs.openjdk.java.net/browse/JDK-8237491;
> 
> In general it reduces code size and complexity and hands more work to gtk.
> 
> Updated on 2020-01-29:
> ![image](https://user-images.githubusercontent.com/30704286/73354728-2ce47d00-4275-11ea-935c-414fc26163d7.png)

Thiago Milczarek Sayao has updated the pull request incrementally with one 
additional commit since the last revision:

  Fixes for ubuntu 16.04 (which delays frame extents)

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/77/files
  - new: https://git.openjdk.java.net/jfx/pull/77/files/f367c9a3..547c3cb0

Webrevs:
 - full: https://webrevs.openjdk.java.net/jfx/77/webrev.38
 - incr: https://webrevs.openjdk.java.net/jfx/77/webrev.37-38

  Stats: 28 lines in 2 files changed: 16 ins; 8 del; 4 mod
  Patch: https://git.openjdk.java.net/jfx/pull/77.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/77/head:pull/77

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

Re: Seeking help with latest master ...

[Johan Vos]

I have no scientific evidence for this, but I remember issues related to
SSL with some JDK's as well, which may or may not be related to the cacerts
file that is bundled.

- Johan

On Thu, Apr 2, 2020 at 6:52 PM Rony G. Flatscher 
wrote:

> Being stuck (tried the previous working gradle 5.6.4, could get "gradle
> clean", but not further) in
> the end I just tried Java 14 and that allowed me to run "gradle"
> successfully (currently building
> webkit to get going again). Just wanted to let interested readers know.
> (Would be great though, if
> it worked with the 11 LTS version as well.)
>
> ---rony
>
>
> On 31.03.2020 20:36, Rony G. Flatscher wrote:
> > On 31.03.2020 20:21, Scott Palmer wrote:
> >>> Could not resolve all dependencies for configuration ':apps:lucene'.
> >>   > Multiple build operations failed.
> >> java.lang.ExceptionInInitializerError
> >> java.lang.NoClassDefFoundError: Could not initialize class
> >>sun.security.ssl.SSLContextImpl$TLSContext
> >> java.lang.NoClassDefFoundError: Could not initialize class
> >>sun.security.ssl.SSLContextImpl$TLSContext
> >>  > java.lang.ExceptionInInitializerError
> >>  > java.lang.NoClassDefFoundError: Could not initialize class
> >>sun.security.ssl.SSLContextImpl$TLSContext
> >>  > java.lang.NoClassDefFoundError: Could not initialize class
> >>sun.security.ssl.SSLContextImpl$TLSContext
> >>
> >>
> >> This to me looks like it may be failing to make an https connection to
> a repository when
> >> attempting to download dependencies.
> >>
> >> There are reports of NoClassDefFoundError w.r.t. SSLContextImpl when
> running gradle builds, see:
> >> https://github.com/gradle/gradle/issues/7842
> >>
> >> Claims to be fixed, but maybe not?
> >>
> >> If it is related to Gradle changing java.home while compiling, I wonder
> if using --no-parallel
> >> might work around it?  Of course try with --debug as well to see if it
> offers more clues.
> > Scott, thank you!
> >
> > Not having any knowledge about gradle I am unfortunately lost in this
> case. However, I used  "gradle
> > --no-parallel --debug" which did not succeed but created a 1.2 MB text
> file which I keep temporarily
> > in my DrobBox at: <
> https://www.dropbox.com/sh/e7seg9kgnm4wn4j/AAB4H4beZd9cJKbpvjyxdNMBa?dl=0
> >.
> >
> > Will have to leave in a few minutes for today so can only try other
> things tomorrow.
> >
> > ---rony
>
>

Re: A WIP for JDK-8238080 for review/discussion (Re: "Internal review ID 9063426: "FXMLLoader: if script engines implement javax.script.Compilabel compile scripts"

[Rony G. Flatscher]

After merging master, applying some fixes and changing the title to reflect the 
change from WIP to a
pull request I would kindly request a review of this pull request!

Here the URL: , title changed to: 
"8238080: FXMLLoader: if
script engines implement javax.script.Compilable compile scripts".

---rony


On 28.02.2020 19:22, Rony G. Flatscher wrote:
> Here is a WIP [1] implementation of [2]. The WIP is based on [3], which is 
> currently in review, and
> has an appropriate test unit going with it as well, please review.
>
> There should be no compatibility issue with this implementation.
>
> Discussion: another solution could be to not compile by default. Rather 
> compile, if some new
> information is present with the FXML file which cannot possibly be present in 
> existing FXML files.
> In this scenario one possible and probably simple solution would be to only 
> compile scripts if the
> language process instruction (e.g. ) contains an appropriate 
> attribute with a value
> indicating that compilation should be carried out (e.g.: compile="true"). 
> This way only FXML with
> PIs having this attribute set to true would be affected. If desired I could 
> try to come up with a
> respective solution as well.
>
> ---rony
>
> [1] "Implementation and test unit": 
>
> [2] "JDK-8238080 : FXMLLoader: if script engines implement 
> javax.script.Compilable compile scripts":
> 
>
> [3] "8234959: FXMLLoader does not populate ENGINE_SCOPE Bindings with 
> FILENAME and ARGV":
> 
>
>
> On 24.01.2020 16:26, Kevin Rushforth wrote:
>
>> Thank you for filing this enhancement request. As an enhancement it should 
>> be discussed on this
>> list before proceeding with a pull request (although a "WIP" or Draft PR can 
>> be used to illustrate
>> the concept).
>>
>> For my part, this seems like a reasonable enhancement, as long as there are 
>> no compatibility
>> issues, but it would be good to hear from application developers who heavily 
>> use FXML.
>>
>> -- Kevin
>>
>>
>> On 1/24/2020 7:21 AM, Rony G. Flatscher wrote:
>>> Just filed a RFE with the following information:
>>>
>>>    * Component:
>>>    o JavaFX
>>>
>>>    * Subcomponent:
>>>    o fxml: JavaFX FXML
>>>
>>>    * Synopsis:
>>>    o "FXMLLoader: if script engines implement javax.script.Compilabel 
>>> compile scripts"
>>>
>>>    * Descriptions:
>>>    o "FXMLLoader is able to execute scripts in Java script languages 
>>> (javax.script.ScriptEngine
>>>  implementations) if such a Java script language gets defined as 
>>> the controller language in
>>>  the 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 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 repetitevly invoked and evaluated."
>>>
>>>    * System /OS/Java Runtime Information:
>>>    o "All systems."
>>>
>>> Received the internal review ID: "9063426"
>>>
>>> ---rony 

RFR: 8129123: ComboBox popup list view does not scrollTo when ComboBox value/selection is set

[Craig Cavanaugh]

This pull request fixes JDK-8129123.  This bug also effects Windows and Linux 
platforms.
Also, I believe JDK-8196037 is a duplicate of this issue.

I've tested this against OpenJDK 11.0.6 on Ubuntu 18.04, Arch Linux and Windows 
10.

Thanks!

-

Commit messages:
 - Merge branch 'master' of https://github.com/openjdk/jfx
 - Merge branch 'master' of https://github.com/openjdk/jfx
 - Fix for JDK-8129123

Changes: https://git.openjdk.java.net/jfx/pull/136/files
 Webrev: https://webrevs.openjdk.java.net/jfx/136/webrev.00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8129123
  Stats: 4 lines in 1 file changed: 3 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jfx/pull/136.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/136/head:pull/136

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

Re: Seeking help with latest master ...

[Rony G. Flatscher]

Being stuck (tried the previous working gradle 5.6.4, could get "gradle clean", 
but not further) in
the end I just tried Java 14 and that allowed me to run "gradle" successfully 
(currently building
webkit to get going again). Just wanted to let interested readers know. (Would 
be great though, if
it worked with the 11 LTS version as well.)

---rony


On 31.03.2020 20:36, Rony G. Flatscher wrote:
> On 31.03.2020 20:21, Scott Palmer wrote:
>>> Could not resolve all dependencies for configuration ':apps:lucene'.
>>   > Multiple build operations failed.
>>     java.lang.ExceptionInInitializerError
>>     java.lang.NoClassDefFoundError: Could not initialize class
>>    sun.security.ssl.SSLContextImpl$TLSContext
>>     java.lang.NoClassDefFoundError: Could not initialize class
>>    sun.security.ssl.SSLContextImpl$TLSContext
>>  > java.lang.ExceptionInInitializerError
>>  > java.lang.NoClassDefFoundError: Could not initialize class
>>    sun.security.ssl.SSLContextImpl$TLSContext
>>  > java.lang.NoClassDefFoundError: Could not initialize class
>>    sun.security.ssl.SSLContextImpl$TLSContext
>>
>>
>> This to me looks like it may be failing to make an https connection to a 
>> repository when
>> attempting to download dependencies.
>>
>> There are reports of NoClassDefFoundError w.r.t. SSLContextImpl when running 
>> gradle builds, see: 
>> https://github.com/gradle/gradle/issues/7842
>>
>> Claims to be fixed, but maybe not?
>>
>> If it is related to Gradle changing java.home while compiling, I wonder if 
>> using --no-parallel
>> might work around it?  Of course try with --debug as well to see if it 
>> offers more clues.
> Scott, thank you!
>
> Not having any knowledge about gradle I am unfortunately lost in this case. 
> However, I used  "gradle
> --no-parallel --debug" which did not succeed but created a 1.2 MB text file 
> which I keep temporarily
> in my DrobBox at: 
> .
>
> Will have to leave in a few minutes for today so can only try other things 
> tomorrow.
>
> ---rony

Re: [Rev 03] RFR: 8236840: Memory leak when switching ButtonSkin

[Jeanette Winzenburg]

On Thu, 2 Apr 2020 12:58:11 GMT, Ambarish Rapte  wrote:

>> ButtonSkin adds a `ChangeListener` to `Control.sceneProperty()` which 
>> results in leaking the `ButtonSkin` itself when
>> the `Button`'s skin is changed to a new `ButtonSkin`.  Using a 
>> `WeakChangeListener` instead of `ChangeListener` solves
>> the issue.
>> Please take a look.
>
> Ambarish Rapte has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fixed review comment: cleanup the accelerator

looks good :)

-

Marked as reviewed by fastegal (Author).

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

Re: [Rev 01] RFR: 8236840: Memory leak when switching ButtonSkin

[Ambarish Rapte]

On Mon, 30 Mar 2020 10:36:13 GMT, Jeanette Winzenburg  
wrote:

>> Thanks for the test case, I did minor changes to it and included in the next 
>> commit.
>> The NPE can occur even without `button.setDefaultButton(true);`.
>> Please take a look
>
> good catch! You are right, without explicit removal of the listener, the NPE 
> happens always.
> 
> Actually, I had been sloppy and got distracted by the NPE from my actual goal 
> which was to dig into Kevin's "not
> cleaning up after itself" and .. finally found a (concededly extreme 
> corner-case :) where that's happening: when
> setting the skin to null. Two failing tests:
> @Test
> public void testDefaultButtonNullSkinReleased() {
> Button button = new Button();
> button.setDefaultButton(true);
> Group root = new Group(button);
> Scene scene = new Scene(root);
> Stage stage = new Stage();
> stage.setScene(scene);
> stage.show();
> WeakReference defSkinRef = new 
> WeakReference<>((ButtonSkin)button.getSkin());
> button.setSkin(null);
> 
> attemptGC(defSkinRef);
> assertNull("skin must be gc'ed", defSkinRef.get());
> }
> 
> @Test
> public void testDefaultButtonNullSkinAcceleratorRemoved() {
> Button button = new Button();
> button.setDefaultButton(true);
> Group root = new Group(button);
> Scene scene = new Scene(root);
> Stage stage = new Stage();
> stage.setScene(scene);
> stage.show();
> KeyCodeCombination key = new KeyCodeCombination(KeyCode.ENTER);
> 
> assertNotNull(scene.getAccelerators().get(key));
> button.setSkin(null);
> assertNull(scene.getAccelerators().get(key));
> 
> }
> 
> An explicitly cleanup in dispose makes them pass:
> 
> @Override
> public void dispose() {
> setDefaultButton(false);
> setCancelButton(false);
> getSkinnable().sceneProperty().removeListener(weakChangeListener);

I have included this change and the test, with slight modification to include 
same test for Cancel button.

-

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

Re: [Rev 03] RFR: 8236840: Memory leak when switching ButtonSkin

[Ambarish Rapte]

> ButtonSkin adds a `ChangeListener` to `Control.sceneProperty()` which results 
> in leaking the `ButtonSkin` itself when
> the `Button`'s skin is changed to a new `ButtonSkin`.  Using a 
> `WeakChangeListener` instead of `ChangeListener` solves
> the issue.
> Please take a look.

Ambarish Rapte has updated the pull request incrementally with one additional 
commit since the last revision:

  Fixed review comment: cleanup the accelerator

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/147/files
  - new: https://git.openjdk.java.net/jfx/pull/147/files/e81f175a..bf974f6a

Webrevs:
 - full: https://webrevs.openjdk.java.net/jfx/147/webrev.03
 - incr: https://webrevs.openjdk.java.net/jfx/147/webrev.02-03

  Stats: 54 lines in 2 files changed: 46 ins; 0 del; 8 mod
  Patch: https://git.openjdk.java.net/jfx/pull/147.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/147/head:pull/147

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

Re: [Rev 03] RFR: 8217472: Add attenuation for PointLight

[Nir Lisker]

On Fri, 3 Jan 2020 09:26:43 GMT, Ambarish Rapte  wrote:

>> Nir Lisker has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Attenuation and range changed internally to floats from doubles
>
> I have added few comments, but have not run tests and sample yet.

@arapte Can you please test the performance changes too?

-

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

Re: [Rev 01] RFR: 8241370: Crash in JPEGImageLoader after fix for JDK-8212034

[Kevin Rushforth]

On Thu, 2 Apr 2020 07:04:08 GMT, Ambarish Rapte  wrote:

>> This is a regression of 
>> [JDK-8212034](https://bugs.openjdk.java.net/browse/JDK-8212034).
>> When image is loaded in WebView usinga url, WebView attempts to load a image 
>> frames with partial image data. This was
>> implemented under, JDK-8153148 -> WCImageDecoderImpl.addImageData() -> calls 
>> loadFrames() with partial image data.
>> 
>> Call to jpeg_read_header() may fail when the partial image data has 
>> incomplete header information.
>> 
>> In the given case the jpeg_read_header() call fails and code execution flow 
>> enters the 'if
>> (setjmp(jerr->setjmp_buffer)) {}' block and results in call to 
>> disposeIIO(env, data);, which in turn calls
>> imageio_dispose. This will free cinfo->err and set it to NULL, and the 
>> subsequent call to (*cinfo->err->format_message)
>> crashes.  Verified All test run, Sanity tests with Ensemble app and Tested 
>> different web pages. Added a test, The test
>> passes with fix and causes a native crash without the fix.
>
> Ambarish Rapte has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fixed reiew comments on test

Marked as reviewed by kcr (Lead).

-

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

Re: [Rev 01] RFR: 8241455: Memory leak on replacing selection/focusModel

[Ambarish Rapte]

On Thu, 2 Apr 2020 09:12:55 GMT, Jeanette Winzenburg  
wrote:

>> modules/javafx.controls/src/test/java/test/javafx/scene/control/SelectionFocusModelMemoryTest.java
>>  line 285:
>> 
>>> 284: }
>>> 285: root.getChildren().add(node);
>>> 286: if (!stage.isShowing()) {
>> 
>> Will it be good to add a call to `root.getChildren().removeAll()` before 
>> adding the `node` to `root` ?
>
>> 
>> Will it be good to add a call to `root.getChildren().removeAll()` before 
>> adding the `node` to `root` ?
> 
> hmm .. don't think that it's necessary to clear out other children: the thing 
> that actually matters is that the control
> goes active in the scenegraph and has a skin attached. Can you think of any 
> context where any of the tests might get
> whacky if there are more than one nodes in the root?   Actually, that's a 
> pattern (*cough, read that's more or less
> c&p'ed) I use in other tests, where it sometimes is allowed and expected that 
> there are more than one.

Agree that It would not cause any problem in test execution.

-

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

Re: [Rev 02] RFR: 8241455: Memory leak on replacing selection/focusModel

[Ambarish Rapte]

On Thu, 2 Apr 2020 09:11:57 GMT, Jeanette Winzenburg  
wrote:

>> Several controls with selection/focusModels leave memory leaks on replacing 
>> the model.
>> 
>> Added tests for all such, those for the misbehaving models fail before and 
>> pass after the fix.
>> 
>> for convenience, the bug reference 
>> https://bugs.openjdk.java.net/browse/JDK-8241455
>
> Jeanette Winzenburg has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   cleanup test as suggested in review

looks good to me.

-

Marked as reviewed by arapte (Reviewer).

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

Re: [Rev 01] RFR: 8241455: Memory leak on replacing selection/focusModel

[Jeanette Winzenburg]

On Wed, 1 Apr 2020 19:34:09 GMT, Ambarish Rapte  wrote:

> 
> Will it be good to add a call to `root.getChildren().removeAll()` before 
> adding the `node` to `root` ?

hmm .. don't think that it's necessary to clear out other children: the thing 
that actually matters is that the control
goes active in the scenegraph and has a skin attached. Can you think of any 
context where any of the tests might get
whacky if there are more than one nodes in the root?

Actually, that's a pattern (*cough, read that's more or less c&p'ed) I use in 
other tests, where it sometimes is
allowed and expected that there are more than one.

-

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

Re: [Rev 02] RFR: 8241455: Memory leak on replacing selection/focusModel

[Jeanette Winzenburg]

> Several controls with selection/focusModels leave memory leaks on replacing 
> the model.
> 
> Added tests for all such, those for the misbehaving models fail before and 
> pass after the fix.
> 
> for convenience, the bug reference 
> https://bugs.openjdk.java.net/browse/JDK-8241455

Jeanette Winzenburg has updated the pull request incrementally with one 
additional commit since the last revision:

  cleanup test as suggested in review

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/148/files
  - new: https://git.openjdk.java.net/jfx/pull/148/files/d2918fb5..8bed244f

Webrevs:
 - full: https://webrevs.openjdk.java.net/jfx/148/webrev.02
 - incr: https://webrevs.openjdk.java.net/jfx/148/webrev.01-02

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

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

Re: [Rev 01] RFR: 8241370: Crash in JPEGImageLoader after fix for JDK-8212034

[Ambarish Rapte]

> This is a regression of 
> [JDK-8212034](https://bugs.openjdk.java.net/browse/JDK-8212034).
> When image is loaded in WebView usinga url, WebView attempts to load a image 
> frames with partial image data. This was
> implemented under, JDK-8153148 -> WCImageDecoderImpl.addImageData() -> calls 
> loadFrames() with partial image data.
> 
> Call to jpeg_read_header() may fail when the partial image data has 
> incomplete header information.
> 
> In the given case the jpeg_read_header() call fails and code execution flow 
> enters the 'if
> (setjmp(jerr->setjmp_buffer)) {}' block and results in call to 
> disposeIIO(env, data);, which in turn calls
> imageio_dispose. This will free cinfo->err and set it to NULL, and the 
> subsequent call to (*cinfo->err->format_message)
> crashes.  Verified All test run, Sanity tests with Ensemble app and Tested 
> different web pages. Added a test, The test
> passes with fix and causes a native crash without the fix.

Ambarish Rapte has updated the pull request incrementally with one additional 
commit since the last revision:

  Fixed reiew comments on test

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/154/files
  - new: https://git.openjdk.java.net/jfx/pull/154/files/49cb0f36..a452cd62

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

  Stats: 235 lines in 3 files changed: 110 ins; 125 del; 0 mod
  Patch: https://git.openjdk.java.net/jfx/pull/154.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/154/head:pull/154

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