Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

[mandy chung]

Thanks for confirming that.

Mandy

On 6/14/18 3:53 PM, Kevin Rushforth wrote:
I verified on an earlier version of the patch that it wasn't in the 
docs, but it would be good for Prasanta to double-check.


-- Kevin


On 6/14/2018 1:29 PM, Phil Race wrote:

you surely mean
@since 11

I believe it has been verified that it is excluded from the docs build 
.. right Prasanta ?


-phil

On 06/14/2018 01:26 PM, mandy chung wrote:
I reviewed the module-info.java change. @since 12 is missing in 
jdk.unsupported.desktop module-info.java


Otherwise it's fine.

Does the docs build exclude jdk.unsupported.desktop?  You might have 
confirmed that that I missed.


Mandy

On 6/13/18 12:48 AM, Prasanta Sadhukhan wrote:

Hi Phil, All

Please find modified webrev taking care of streamlining 
SwingInteropUtils class and handling of native window handle in 
LightweightFrameWrapper class by using JNI call in FX

http://cr.openjdk.java.net/~psadhukhan/fxswing.14/

Corresponding CSR: https://bugs.openjdk.java.net/browse/JDK-8202175

Regards
Prasanta

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

[Kevin Rushforth]

I verified on an earlier version of the patch that it wasn't in the 
docs, but it would be good for Prasanta to double-check.


-- Kevin


On 6/14/2018 1:29 PM, Phil Race wrote:

you surely mean
@since 11

I believe it has been verified that it is excluded from the docs build 
.. right Prasanta ?


-phil

On 06/14/2018 01:26 PM, mandy chung wrote:
I reviewed the module-info.java change. @since 12 is missing in 
jdk.unsupported.desktop module-info.java


Otherwise it's fine.

Does the docs build exclude jdk.unsupported.desktop?  You might have 
confirmed that that I missed.


Mandy

On 6/13/18 12:48 AM, Prasanta Sadhukhan wrote:

Hi Phil, All

Please find modified webrev taking care of streamlining 
SwingInteropUtils class and handling of native window handle in 
LightweightFrameWrapper class by using JNI call in FX

http://cr.openjdk.java.net/~psadhukhan/fxswing.14/

Corresponding CSR: https://bugs.openjdk.java.net/browse/JDK-8202175

Regards
Prasanta

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

[mandy chung]

On 6/14/18 1:29 PM, Phil Race wrote:

you surely mean
@since 11


Oops typo. Yes @since 11

Mandy

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

[Phil Race]

you surely mean
@since 11

I believe it has been verified that it is excluded from the docs build 
.. right Prasanta ?


-phil

On 06/14/2018 01:26 PM, mandy chung wrote:
I reviewed the module-info.java change.  @since 12 is missing in 
jdk.unsupported.desktop module-info.java


Otherwise it's fine.

Does the docs build exclude jdk.unsupported.desktop?  You might have 
confirmed that that I missed.


Mandy

On 6/13/18 12:48 AM, Prasanta Sadhukhan wrote:

Hi Phil, All

Please find modified webrev taking care of streamlining 
SwingInteropUtils class and handling of native window handle in 
LightweightFrameWrapper class by using JNI call in FX

http://cr.openjdk.java.net/~psadhukhan/fxswing.14/

Corresponding CSR: https://bugs.openjdk.java.net/browse/JDK-8202175

Regards
Prasanta

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

[mandy chung]

I reviewed the module-info.java change.  @since 12 is missing in 
jdk.unsupported.desktop module-info.java


Otherwise it's fine.

Does the docs build exclude jdk.unsupported.desktop?  You might have 
confirmed that that I missed.


Mandy

On 6/13/18 12:48 AM, Prasanta Sadhukhan wrote:

Hi Phil, All

Please find modified webrev taking care of streamlining 
SwingInteropUtils class and handling of native window handle in 
LightweightFrameWrapper class by using JNI call in FX

http://cr.openjdk.java.net/~psadhukhan/fxswing.14/

Corresponding CSR: https://bugs.openjdk.java.net/browse/JDK-8202175

Regards
Prasanta

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

[Phil Race]

+1 from me for the JDK changes.

-phil.

On 06/13/2018 06:04 PM, Kevin Rushforth wrote:
The JDK changes look good to me, and as far as I have tested them, it 
works for me. I haven't tried Drag and Drop yet with the latest 
interface changes.


To repeat something I've mentioned before, just so there is no 
confusion, the FX side of the changes are intended to be a proof of 
concept to test the JDK side at this point. They will need refactoring 
before they can go in, so that FX will still be buildable and runnable 
with JDK 10.


-- Kevin


On 6/13/2018 12:48 AM, Prasanta Sadhukhan wrote:


Hi Phil, All

Please find modified webrev taking care of streamlining 
SwingInteropUtils class and handling of native window handle in 
LightweightFrameWrapper class by using JNI call in FX

http://cr.openjdk.java.net/~psadhukhan/fxswing.14/

Corresponding CSR: https://bugs.openjdk.java.net/browse/JDK-8202175

Regards
Prasanta
On 5/30/2018 4:13 AM, Philip Race wrote:

Some follow up comments.

On 5/10/18, 5:49 PM, Philip Race wrote:
I've looked over the Swing code. No time to actually try it out so 
I can only comment on what I see.


I am not sure what I think about class docs like "This class wraps 
 .."
I know no javadoc is generated but this module is exported and 
probably should say something

less specific.

In general I really have to hold my nose when looking at this whole 
module and I

find it distasteful to endorse it.

I really don't like all the things SwingInterOpUtils.java does.


Specific things we should look at in this file
- getDefaultScaleX/Y .. JDK 9 will set a transform on the graphics that
is passed to JComponent.paintComponent() .. so I wonder if we really
need this internal API. I doubt any other code is updating the transform
in a way that would make it a problem so the information you get 
should be valid.


The code to get the data array for the raster and associated information
can be extracted via standard API like this :

DataBuffer  db = BufferedImage.getRaster().getDataBuffer();
if (db instanceof DataBufferInt) {
 data = (DataBufferInt)db.getData();
}

So all of those can be fixed.

The code that accepts a native window handle maybe should require it 
is accessed via JNI ...
It could still be unsupported, but it would be package private or 
similar.

If you have a native ID, then you are already using native code.
This sidesteps any questions about the stability of such an API which
accepts a native resource.

The code that finds a component by location is probably harmless ...

I'm less sure about the event posting and focus grabbing support.
I'd like to have Sergey comment on those.

the DnD code is too much for me to examine in detail.

I worry we are creating something we'll come to regret here.
The "unsupportedness" needs to be backed up by 
deprecated-for-removal being included

today so we can get rid of it the next release if we want to.


I've looked at jdk.unsupported and they don't seem to have 
deprecation tags so

maybe that isn't an issue.

-phil


No @since tags anywhere

-phil.

On 5/10/18, 8:32 AM, Kevin Rushforth wrote:
I confirm that this fixes the issue. The interface change to make 
the two static methods e instance methods is a good change in any 
case.


I am not a Swing "R"eviewer, but the .13 webrev gets a +1 from me.

-- Kevin


On 5/10/2018 8:20 AM, Prasanta Sadhukhan wrote:


Hi Kevin,All,

Please find the modified webrev fixing this #1 issue
http://cr.openjdk.java.net/~psadhukhan/fxswing.13/
via change in 
jdk/swing/interop/DropTargetContextWrapper.java#setDropTargetContext 
and FXDnD.java#postDropTargetEvent


For me, #2 works, #3 doesn't work even now due to JDK-8141391 
 and #4 works 
for me.


Regards
Prasanta
On 5/9/2018 11:29 PM, Kevin Rushforth wrote:

Hi Prasanta,

The API looks good now.

All of our automated tests work (except for the ones with a 
security manager due to JDK-8202451 
).


The only functional problem that I see is that Drag and Drop 
onto a SwingNode doesn't work. We need to make sure that we test 
the following four cases:


1. Drag / drop onto a Swing component in a SwingNode
2. Drag / drop from a Swing component in a SwingNode
3. Drag / drop onto a JavaFX control in a JFXPanel
4. Drag / drop from a JavaFX control in a JFXPanel

So far I only tried the first one; the others still need to be 
validated.


-- Kevin



On 5/9/2018 7:14 AM, Prasanta Sadhukhan wrote:

Modified webrev to cater to this

http://cr.openjdk.java.net/~psadhukhan/fxswing.12/

Regards
Prasanta
On 5/9/2018 5:58 PM, Kevin Rushforth wrote:

The following can also be abstract:

LightweightContentWrapper:
  getComponent, createDragGestureRecognizer, 
createDragSourceContextPeer


DropTargetContextWrapper:
  getTargetActions, getDropTarget, getTransferDataFlavors, 
getTransferable, isTransferableJVMLocal


DispatcherWrapper:
  isDispatchThread, 

[11] RFR JDK-8202277:WebView image capture fails with standalone FX due to dependency on javafx.swing

[Prasanta Sadhukhan]

Hi Kevin, All

Please review the fix to remove the dependancy on javafx.swing module 
from javafx.web


Bug: https://bugs.openjdk.java.net/browse/JDK-8202277
webrev: http://cr.openjdk.java.net/~psadhukhan/fx/8202277/webrev/

Regards
Prasanta

Re: [11] Review request: 8200446: Update minimum boot JDK to 10

[Kevin Rushforth]

Thanks for the reminder. This is good advice that we should all strive 
to follow.


-- Kevin

On 6/14/2018 6:37 AM, Nir Lisker wrote:


Once this is done, we can start using JDK 10 features, such as
'var' for local variables (although please don't take this as a
license to use 'var' gratuitously).


I'll remind that there's a style guide at 
http://openjdk.java.net/projects/amber/LVTIstyle.html.


- Nir

On Thu, Jun 14, 2018 at 4:33 PM, Kevin Rushforth 
mailto:kevin.rushfo...@oracle.com>> wrote:


Please review the following to bump the minimum boot JDK required
for JavaFX builds to JDK 10. Note that the review will be done on
github, but it is OK to reply to this thread with comments if you
don't have a github account.

https://bugs.openjdk.java.net/browse/JDK-8200446

https://github.com/javafxports/openjdk-jfx/pull/61


This also bumps the class file format to that of JDK 10. I note
that we have been using JDK 10 to build the openjfx-11 builds for
several months, and now that we've upgraded to gradle 4.8 we are
ready to make JDK 10 the minimum.

Once this is done, we can start using JDK 10 features, such as
'var' for local variables (although please don't take this as a
license to use 'var' gratuitously).

Thanks.

-- Kevin

Re: [11] Review request: 8200446: Update minimum boot JDK to 10

[Nir Lisker]

>
> Once this is done, we can start using JDK 10 features, such as 'var' for
> local variables (although please don't take this as a license to use 'var'
> gratuitously).


I'll remind that there's a style guide at
http://openjdk.java.net/projects/amber/LVTIstyle.html.

- Nir

On Thu, Jun 14, 2018 at 4:33 PM, Kevin Rushforth  wrote:

> Please review the following to bump the minimum boot JDK required for
> JavaFX builds to JDK 10. Note that the review will be done on github, but
> it is OK to reply to this thread with comments if you don't have a github
> account.
>
> https://bugs.openjdk.java.net/browse/JDK-8200446
> https://github.com/javafxports/openjdk-jfx/pull/61
>
> This also bumps the class file format to that of JDK 10. I note that we
> have been using JDK 10 to build the openjfx-11 builds for several months,
> and now that we've upgraded to gradle 4.8 we are ready to make JDK 10 the
> minimum.
>
> Once this is done, we can start using JDK 10 features, such as 'var' for
> local variables (although please don't take this as a license to use 'var'
> gratuitously).
>
> Thanks.
>
> -- Kevin
>
>

[11] Review request: 8200446: Update minimum boot JDK to 10

[Kevin Rushforth]

Please review the following to bump the minimum boot JDK required for 
JavaFX builds to JDK 10. Note that the review will be done on github, 
but it is OK to reply to this thread with comments if you don't have a 
github account.


https://bugs.openjdk.java.net/browse/JDK-8200446
https://github.com/javafxports/openjdk-jfx/pull/61

This also bumps the class file format to that of JDK 10. I note that we 
have been using JDK 10 to build the openjfx-11 builds for several 
months, and now that we've upgraded to gradle 4.8 we are ready to make 
JDK 10 the minimum.


Once this is done, we can start using JDK 10 features, such as 'var' for 
local variables (although please don't take this as a license to use 
'var' gratuitously).


Thanks.

-- Kevin

[11] RFR JDK-8204956 : Cleanup whitespace after fix for JDK-8200285

[Ambarish Rapte]

Hi Kevin,

 

Please review this correction fix :

Bug: https://bugs.openjdk.java.net/browse/JDK-8204956

Webrev: http://cr.openjdk.java.net/~arapte/fx/8204956/webrev.00/

 

Regards,

Ambarish

Re: negative line spacing in Text

[Pedro Duque Vieira]

I'm all in favor of following W3C CSS, but you can't specify negative
values in W3C CSS line-height property either. The minimum value is 0 which
just collapses everything into the same line, so negative values wouldn't
make sense since it would layout the paragraph upside down.

Cheers,

I looked at the source and it is a double property that is just added to
> the default line height
> So I don't see why it would just cause line wrap to fail such that text
> was all on the same line,
> which is how I am interpreting what you are saying.
> Perhaps if you are using too large a negative value something I'm
> overlooking is clamping it ..
> FWIW line-spacing is not a W3C CSS property so far as I can tell, so it
> is just FX CSS
> There is line-height in the W3C spec but it is specified differently.
> -phil.

-- 
Pedro Duque Vieira