Re: Build instructions for Ubuntu 20.04

2020-03-23 Thread Nir Lisker
Updated

On Tue, Mar 24, 2020 at 3:26 AM Thiago Milczarek Sayao <
thiago.sa...@clamed.com.br> wrote:

> Hi,
>
> Can someone  update
> https://wiki.openjdk.java.net/display/OpenJFX/Building+OpenJFX for Ubuntu
> 20.04?
>
>
> 1) libavformat-ffmpeg57 -> libavformat58
>
> 2) Need to add libxxf86vm-dev
>
>
> Thanks
>
>


RFR: 8241476: Linux build warning issued on updated compilers (Ubuntu 18.04.4 / 20.04)

2020-03-23 Thread Thiago Milczarek Sayao
Simple fix to remove annoying warnings.

-

Commit messages:
 - Remove build warning
 - Merge pull request #7 from openjdk/master
 - Merge pull request #6 from openjdk/master
 - Merge pull request #5 from openjdk/master
 - Merge pull request #4 from openjdk/master

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

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


RFR: 8241474: Build failing on Ubuntu 20.04

2020-03-23 Thread Thiago Milczarek Sayao
Small fix for building on ubuntu 20.04.

-

Commit messages:
 - Fix build on 20.04
 - Merge pull request #7 from openjdk/master
 - Merge pull request #6 from openjdk/master
 - Merge pull request #5 from openjdk/master
 - Merge pull request #4 from openjdk/master

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

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


Build instructions for Ubuntu 20.04

2020-03-23 Thread Thiago Milczarek Sayao
Hi,

Can someone  update 
https://wiki.openjdk.java.net/display/OpenJFX/Building+OpenJFX for Ubuntu 20.04?


1) libavformat-ffmpeg57 -> libavformat58

2) Need to add libxxf86vm-dev


Thanks



Re: [Rev 02] RFR: 8240692: Cleanup of the javafx property objects

2020-03-23 Thread Kevin Rushforth
On Tue, 24 Mar 2020 00:06:21 GMT, Kevin Rushforth  wrote:

>> Nir Lisker has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Re-added newline at the end
>
> Marked as reviewed by kcr (Lead).

Given that this PR is now just a cleanup fix, it is simple enough that it 
doesn't need a second reviewer.

-

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


Re: [Rev 02] RFR: 8240692: Cleanup of the javafx property objects

2020-03-23 Thread Nir Lisker
> A simple readability cleanup for the files that were changed in #113.
> 
> Note that the boolean property has an `instanceof` check that doesn't exist 
> in the other properties.

Nir Lisker has updated the pull request incrementally with one additional 
commit since the last revision:

  Re-added newline at the end

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/141/files
  - new: https://git.openjdk.java.net/jfx/pull/141/files/6876a037..93f50005

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

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

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


Re: [Rev 02] RFR: 8240692: Cleanup of the javafx property objects

2020-03-23 Thread Kevin Rushforth
On Tue, 24 Mar 2020 00:07:04 GMT, Nir Lisker  wrote:

>> A simple readability cleanup for the files that were changed in #113.
>> 
>> Note that the boolean property has an `instanceof` check that doesn't exist 
>> in the other properties.
>
> Nir Lisker has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Re-added newline at the end

Marked as reviewed by kcr (Lead).

-

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


Re: RFR: 8234471: Canvas in webview displayed with wrong scale on Windows

2020-03-23 Thread Kevin Rushforth
On Mon, 6 Jan 2020 04:42:54 GMT, Guru Hb  wrote:

>> This bug can be reproduced when the screen resolution is at 125%, 150% and 
>> 175% for Windows, which correpsonds to
>> `pixelScale` values of 1.25, 1.5 and 1.75, respectively.
>> Issue: The rectangle inside canvas is rendered on `pixelScale` while the 
>> borders are rendered on `Math.ceil(pixelScale)`
>> 
>> Fix: Use `Math.ceil(pixelScale)` for calculating `pixelScaleTransform`
>
> have you tested this on Linux and Mac by changing JVM option 
> -Dglass.win.uiScale=.

This looks like the correct fix, and I can confirm that it does fix the 
problem. I'll run a set of regression tests on
Windows 10 with 125% scaling (which is my normal environment) and then finish 
my review.

-

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


Re: RFR: 8236840: Memory leak when switching ButtonSkin

2020-03-23 Thread Kevin Rushforth
On Mon, 23 Mar 2020 07:26:46 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.

In general, there are two approaches to avoiding listener-related memory leaks. 
One is to use a WeakListener; the other
is to explicitly remove the listener when the object is removed or otherwise no 
longer needed.

Using a WeakListener is certainly easier, but runs the risk of the listener 
being removed too early and not cleaning up
after itself. I'm not suggesting that's the case here, but it is worth looking 
at. The one thing I would ask you to
take a look at is whether it would matter if the old skin didn't call 
`setDefaultButton(oldScene, false)` when removed
(and similarly `setCancelButton`).

-

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


Re: RFR: 8241455: ChoiceBox - memory leak on replacing selectionModel

2020-03-23 Thread Kevin Rushforth
On Mon, 23 Mar 2020 23:45:21 GMT, Kevin Rushforth  wrote:

>> ChoiceBox leaves a memory leak when replacing the selectionModel. Culprit is 
>> ChoiceBoxSelectionModel which registers
>> listener with strong references. Fix is to change these to weak references.
>> Added test that fails before and passes after the test.
>> 
>> for convenience, the bug reference 
>> https://bugs.openjdk.java.net/browse/JDK-8241455
>
> I have basically the same comment / question as I asked in #147
> 
> In general, there are two approaches to avoiding listener-related memory 
> leaks. One is to use a WeakListener; the other
> is to explicitly remove the listener when the object is removed or otherwise 
> no longer needed.
> Using a WeakListener is certainly easier, but runs the risk of the listener 
> being removed too early and not cleaning up
> after itself. I'm not suggesting that's the case here, but it is worth 
> looking at.

@arapte @aghaisas - Can you both review this?

-

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


Re: RFR: 8241455: ChoiceBox - memory leak on replacing selectionModel

2020-03-23 Thread Kevin Rushforth
On Mon, 23 Mar 2020 16:32:16 GMT, Jeanette Winzenburg  
wrote:

> ChoiceBox leaves a memory leak when replacing the selectionModel. Culprit is 
> ChoiceBoxSelectionModel which registers
> listener with strong references. Fix is to change these to weak references.
> Added test that fails before and passes after the test.
> 
> for convenience, the bug reference 
> https://bugs.openjdk.java.net/browse/JDK-8241455

I have basically the same comment / question as I asked in #147

In general, there are two approaches to avoiding listener-related memory leaks. 
One is to use a WeakListener; the other
is to explicitly remove the listener when the object is removed or otherwise no 
longer needed.

Using a WeakListener is certainly easier, but runs the risk of the listener 
being removed too early and not cleaning up
after itself. I'm not suggesting that's the case here, but it is worth looking 
at.

-

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


Re: [Rev 07] RFR: 8235480: Regression: [RTL] Arrow keys navigation doesn't respect TableView orientation

2020-03-23 Thread Jeanette Winzenburg
On Tue, 17 Mar 2020 07:38:45 GMT, Ajit Ghaisas  wrote:

>> Bug : https://bugs.openjdk.java.net/browse/JDK-8235480
>> 
>> Fix : Added the missed out RTL checks to the key mappings in 
>> TableViewBehaviorBase class.
>> 
>> Testing : Added a test to take NodeOrientation as a parameter and test 
>> horizontal key navigation for the TableView.
>
> Ajit Ghaisas has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   minor test corrections

yeah sure .. thought I did already approve, looks like I didn't find the 
correct way - trying again

-

Marked as reviewed by fastegal (Author).

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


Re: RFR: 8089828: RTL Orientation, the flag of a mnemonic is not placed under the mnemonic letter.

2020-03-23 Thread Kevin Rushforth
On Mon, 23 Mar 2020 06:36:26 GMT, Ajit Ghaisas  wrote:

> **Bug :**
> https://bugs.openjdk.java.net/browse/JDK-8089828
> 
> **Root Cause :**
> RIGHT_TO_LEFT NodeOrientation was not considered during mnemonic position 
> calculation.
> 
> **Fix :**
> Corrected code to consider RIGHT_TO_LEFT NodeOrientation while calculating 
> mnemonic position.
> 
> **Testing :**
> Existing manual test is modified to test RTL buttons as well.

The fix looks good to me, although I'd like a second reviewer.

I did note one issue with the format of the copyright year.

tests/manual/UI/ButtonMnemonicPositionTest.java line 2:

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

There is a missing `,` after `2020`

-

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


Re: RFR: 8089134: [2D traversal, RTL] TraversalEngine only handles left/right key traversal correctly in RTL for top-level engine in ToolBar

2020-03-23 Thread Kevin Rushforth
On Wed, 18 Mar 2020 15:13:40 GMT, Ajit Ghaisas  wrote:

> **Issue**
> https://bugs.openjdk.java.net/browse/JDK-8089134
> 
> **Fix**
> Added a fix to modify selection direction based on NodeOrientation of the 
> ToolBar.
> 
> **Testing**
> Added a unit test to test common focus movement scenarios using tab and arrow 
> keys for the ToolBar.

Marked as reviewed by kcr (Lead).

-

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


Re: [Rev 07] RFR: 8235480: Regression: [RTL] Arrow keys navigation doesn't respect TableView orientation

2020-03-23 Thread Kevin Rushforth
On Tue, 17 Mar 2020 07:38:45 GMT, Ajit Ghaisas  wrote:

>> Bug : https://bugs.openjdk.java.net/browse/JDK-8235480
>> 
>> Fix : Added the missed out RTL checks to the key mappings in 
>> TableViewBehaviorBase class.
>> 
>> Testing : Added a test to take NodeOrientation as a parameter and test 
>> horizontal key navigation for the TableView.
>
> Ajit Ghaisas has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   minor test corrections

Looks good to me.

This needs one more reviewer. @kleopatra would you be willing?

-

Marked as reviewed by kcr (Lead).

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


Re: [Rev 01] RFR: 8240692: Cleanup of the javafx property objects

2020-03-23 Thread Kevin Rushforth
On Mon, 23 Mar 2020 05:32:29 GMT, Nir Lisker  wrote:

>> A simple readability cleanup for the files that were changed in #113.
>> 
>> Note that the boolean property has an `instanceof` check that doesn't exist 
>> in the other properties.
>
> Nir Lisker has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Revert changes of anonymous classes usage

Looks good with one minor formatting comment (which applies to all files).

modules/javafx.base/src/main/java/javafx/beans/property/BooleanProperty.java 
line 183:

> 182: }
> 183: }

Minor: can you add the trailing newline back in? We typically end each file 
with a single new-line; also this shows up
as an unnecessary diff.

-

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


Re: [Rev 02] RFR: 8234959: FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and ARGV

2020-03-23 Thread Kevin Rushforth
On Sat, 21 Mar 2020 19:19:18 GMT, Kevin Rushforth  wrote:

>> Rony G. Flatscher has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   corrected wrong test string
>
> The fix looks good. I left a few comments on the test. One of them is 
> substantive, the rest are formatting. Once you
> make those changes, I'll approve it.

One more minor observation. I noticed the following have DOS line endings:

tests/system/src/testscriptapp1/resources/mymod/META-INF/services/javax.script.ScriptEngineFactory:
 ASCII text, with
CRLF line terminators
tests/system/src/testscriptapp1/resources/mymod/myapp1/demo_01_bottomscript.rpsl:
   ASCII text, with
CRLF line terminators
tests/system/src/testscriptapp1/resources/mymod/myapp1/demo_01_middlescript.rpsl:
   ASCII text, with
CRLF line terminators
tests/system/src/testscriptapp1/resources/mymod/myapp1/demo_01_topscript.rpsl:  
ASCII text, with
CRLF line terminators

Since they aren't source code files, `git jcheck` won't complain, but as long 
as you are fixing the other issues, would
you mind fixing these too?

-

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


RFR: 8241455: ChoiceBox - memory leak on replacing selectionModel

2020-03-23 Thread Jeanette Winzenburg
ChoiceBox leaves a memory leak when replacing the selectionModel. Culprit is 
ChoiceBoxSelectionModel which registers
listener with strong references. Fix is to change these to weak references.

Added test that fails before and passes after the test.

for convenience, the bug reference 
https://bugs.openjdk.java.net/browse/JDK-8241455

-

Commit messages:
 - 8241455: ChoiceBox - memory leak on replacing selectionModel

Changes: https://git.openjdk.java.net/jfx/pull/148/files
 Webrev: https://webrevs.openjdk.java.net/jfx/148/webrev.00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8241455
  Stats: 50 lines in 2 files changed: 42 ins; 0 del; 8 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: Behavior of jumpTo

2020-03-23 Thread Kevin Rushforth
I agree. If jumpTo took a relative (delta) Duration then it would make 
sense for it to take the direction of rate into account. Given that it 
is an absolute position, the current behavior just seems wrong.


The spec says:

    Jumps to a given position in this |Animation|

I would interpret that as being an absolute position, but clarifying 
that with a spec change sounds like a good idea.


The only possible concern is compatibility. It's possible that some 
applications have worked around this bug, but that doesn't seem 
compelling enough to keep the buggy behavior.


-- Kevin


On 3/22/2020 8:08 PM, Nir Lisker wrote:

Hi,

As I'm continuing my work on the animations front, I came across an
ambiguous behavior of JumpTo. The specifications don't say anything about
it, but in FiniteClipEnvelop, if the rate is negative, it jumps to the
specified time *counted from the end*. This seems intentional because it's
checked explicitly:

 pos = ticks % cycleTicks;
 if (rate < 0) {
 pos = cycleTicks - pos;
 }
 if ((pos == 0) && (ticks > 0)) {
 pos = cycleTicks;
 }

SingleLoopClipEnvelope does not behave this way. I submitted [1] to track
this. This also causes an issue with jumping when rate=0, however, there
are other issues with rate=0, for example [2], and it's hard to separate
them.

What should be the proper behavior of jumpTo with respect to the rate? I
think that they shouldn't be coupled: jumping should always be calculated
from the start. The application can always calculate (totalTime -
requiredTime) and send that to jumpTo.

- Nir

[1] https://bugs.openjdk.java.net/browse/JDK-8237973
[2] https://bugs.openjdk.java.net/browse/JDK-8237757




Re: RFR: 8240466: AppJSCallback* apps launched by ModuleLauncherTest intermittently hang

2020-03-23 Thread Kevin Rushforth
WebKit has never used Nashorn so this is unrelated to the impending 
Nashorn removal.


-- Kevin


On 3/23/2020 5:49 AM, Eric Bresie wrote:

This may be a little off topic but given this is a Javascript concern, is this 
(or will this) be impacted by Nashhorns depreciation (as of jdk11 I believe) 
and potential removal at some point?

I see reference to Netscape JObject do maybe that circumvents some of that but 
does this need to eventually transition to graalvm or other means?

Eric Bresie
ebre...@gmail.com

On March 10, 2020 at 6:22:01 AM CDT, Ajit Ghaisas  
wrote:
On Fri, 6 Mar 2020 19:53:04 GMT, Kevin Rushforth  wrote:


Please ignore this comment as well, it is also for debugging issues with Skara 
and mailman :email: 

@aghaisas can you review this?

I executed these tests without this patch. They did not hang on my Macbook.

I applied patch and executed the tests. They continue to pass.
I see the merit of this change to make tests predictable. +1.

-

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




Re: Re: RFR: 8240688: Remove the JavaBeanXxxPropertyBuilders constructors

2020-03-23 Thread Eric Bresie
Is there any external skara documentation available on all these flags etc?

Eric Bresie
ebre...@gmail.com
> On March 7, 2020 at 8:25:02 AM CST, Kevin Rushforth  
> wrote:
> On Fri, 6 Mar 2020 20:46:39 GMT, Kevin Rushforth  wrote:
>
> > > Marked as reviewed by kcr (Lead).
> >
> > @arapte can you also review this?
>
> Now the the CSR is approved, we need something to "wake up" the Skara bot to 
> notice that.
> @edvbld @rwestberg is there a command that you would recommend to do this? 
> Maybe typing `/summary` without actually adding a summary? Failing that we 
> can wait for the second reviewer, but it would be nice to test Skara's CSR 
> support prior to that (this is the first one where we've had an approved CSR 
> since starting to use the `/csr` feature).
>
> -
>
> PR: https://git.openjdk.java.net/jfx/pull/140


Re: Re: RFR: 8240466: AppJSCallback* apps launched by ModuleLauncherTest intermittently hang

2020-03-23 Thread Eric Bresie
This may be a little off topic but given this is a Javascript concern, is this 
(or will this) be impacted by Nashhorns depreciation (as of jdk11 I believe) 
and potential removal at some point?

I see reference to Netscape JObject do maybe that circumvents some of that but 
does this need to eventually transition to graalvm or other means?

Eric Bresie
ebre...@gmail.com
> On March 10, 2020 at 6:22:01 AM CDT, Ajit Ghaisas  
> wrote:
> On Fri, 6 Mar 2020 19:53:04 GMT, Kevin Rushforth  wrote:
>
> > > Please ignore this comment as well, it is also for debugging issues with 
> > > Skara and mailman :email: 
> >
> > @aghaisas can you review this?
>
> I executed these tests without this patch. They did not hang on my Macbook.
>
> I applied patch and executed the tests. They continue to pass.
> I see the merit of this change to make tests predictable. +1.
>
> -
>
> PR: https://git.openjdk.java.net/jfx/pull/134


RFR: 8236840: Memory leak when switching ButtonSkin

2020-03-23 Thread 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.

-

Commit messages:
 - 8236840: Memory leak when switching ButtonSkin

Changes: https://git.openjdk.java.net/jfx/pull/147/files
 Webrev: https://webrevs.openjdk.java.net/jfx/147/webrev.00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8236840
  Stats: 84 lines in 2 files changed: 64 ins; 17 del; 3 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


RFR: 8089828: RTL Orientation, the flag of a mnemonic is not placed under the mnemonic letter.

2020-03-23 Thread Ajit Ghaisas
**Bug :**
https://bugs.openjdk.java.net/browse/JDK-8089828

**Root Cause :**
RIGHT_TO_LEFT NodeOrientation was not considered during mnemonic position 
calculation.

**Fix :**
Corrected code to consider RIGHT_TO_LEFT NodeOrientation while calculating 
mnemonic position.

**Testing :**
Existing manual test is modified to test RTL buttons as well.

-

Commit messages:
 - rtl_mnemonic_fix

Changes: https://git.openjdk.java.net/jfx/pull/146/files
 Webrev: https://webrevs.openjdk.java.net/jfx/146/webrev.00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8089828
  Stats: 46 lines in 3 files changed: 37 ins; 0 del; 9 mod
  Patch: https://git.openjdk.java.net/jfx/pull/146.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/146/head:pull/146

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