Re: [12] RFR: JDK-8210092: Remove old javafx.swing implementation
On 22/09/2018 00:53, Kevin Rushforth wrote: Please review the following on GitHub: https://bugs.openjdk.java.net/browse/JDK-8210092 https://github.com/javafxports/openjdk-jfx/pull/207 https://github.com/javafxports/openjdk-jfx/pull/207/files This will remove the old JDK-10-based implementation of FX / Swing interop and cleanup the build to remove the legacy qualified exports and the logic that optionally excludes the new implementation. It also changes the dependency that javafx.swing has on jdk.unsupported.desktop from "requires static" (optional) to "requires", which will fix an existing bug with jlinked images that include javafx.swing. I don't know the workflow or how reviews work in the javafxports/openjdk-jfx project work (I didn't want to hit the "Review changes" button) but the change to the make the dependency non-optional looks okay to me. -Alan.
Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop
On 09/05/2018 15:42, Philip Race wrote: : Qn. to Mandy & Alan : it seems there is no need to mention this module in make/common/Modules.gmk in order to get it built, but is there any advantage in doing so ? I mean without it, there is no conscious listing of it as a module nor classification of it. Right, it's not necessary to list modules that are defined to the application class loader and are only in the JDK image. There's a comment at the top of the make file on this but it probably could be a bit clearer. -Alan
Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop
On 08/05/2018 06:51, Prasanta Sadhukhan wrote: Modified webrev to rename to InteropProviderImpl http://cr.openjdk.java.net/~psadhukhan/fxswing.10/ This looks okay to me. -Alan
Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop
On 07/05/2018 10:26, Prasanta Sadhukhan wrote: : Modified webrev to use InteropProvider http://cr.openjdk.java.net/~psadhukhan/fxswing.9/ This looks okay although for consistent then InteropImpl could be renamed too. -Alan
Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop
On 05/05/2018 16:20, Prasanta Sadhukhan wrote: Updated webrev to modify java.desktop module-info.java (only difference between webrev7 and this) to remove the duplicate exports of sun.awt so we will have now exports sun.awt to jdk.accessibility, jdk.unsupported.desktop; http://cr.openjdk.java.net/~psadhukhan/fxswing.8/ At a high-level, the module declarations look good and using service binding to ensure that the jdk.unsupported.desktop module is resolved looks good. The only thing is the name of the service provider interface, it might be clearer if you use InteropProvider rather than InteropInterface. I assume dependencies/java.desktop/module-info.java.extra will be removed once the remaining dependency on sun.print goes away. -Alan
Re: [9] Review request: 8177566: FX user module gets IllegalAccessException from sun.reflect.misc.Trampoline
On 02/05/2017 01:21, Kevin Rushforth wrote: This review is being cross-posted to both openjfx-dev and jigsaw-dev. Please review the proposed fix for: https://bugs.openjdk.java.net/browse/JDK-8177566 http://cr.openjdk.java.net/~kcr/8177566/webrev.00/complete-webrev/ Details of the fix as well as notes to reviewers are in the bug report [1] (e.g., I've also generated separate webrevs for the fix itself, the doc changes, and the test changes). Overall I think the approach is okay but I wonder how easy it will be for joe developer to understand. By this I mean the description in the javadoc is lengthy and I wonder if it might be better to split up the two "solutions" so that it the choice is clearer. Related is that the IAE thrown by MethodHelper.invoke could make it clear that the package is not exported. -Alan.
Re: [9] Review request: 8178015: Clarify requirement for app modules to export/open packages to javafx modules
On 20/04/2017 19:06, Kevin Rushforth wrote: Here is an updated webrev with a few suggested wording changes (e.g., removed the reference to ModuleDescriptor, changed "accessible by" back to "accessible to"). http://cr.openjdk.java.net/~kcr/8178015/webrev.02/ Additionally, I removed the example in the FXML annotation showing the use of "opens to" in module-info.java (but left the example in Application). What would you think about dropping the "Alternative the module can open " from the Application javadoc. I say this because it could confuse the reader. An advanced developer may follow the link to isExported and read more but it seems a bit much to try to put in the Application javadoc. FXML looks good. -Alan
Re: [9] Review request: 8178015: Clarify requirement for app modules to export/open packages to javafx modules
On 18/04/2017 19:19, Kevin Rushforth wrote: Good suggestion. Here is an updated webrev with Mandy's suggestion and yours: http://cr.openjdk.java.net/~kcr/8178015/webrev.01/ -- Kevin This looks mostly okay. I guess for FXML then I assume that the annotated member could be public, in which case the package just needs to be exported (no need to open). -Alan
Re: [9] Review request: 8178015: Clarify requirement for app modules to export/open packages to javafx modules
On 18/04/2017 01:00, Kevin Rushforth wrote:
Please review the following javadoc change:
https://bugs.openjdk.java.net/browse/JDK-8178015
http://cr.openjdk.java.net/~kcr/8178015/webrev.00/
This restores the links to the Module class that had to be removed
during the transition period for the move of Module and Layer from
java.lang.reflect to java.lang, and makes the requirements for running
an application in a named module more clear. I also fixed a couple
related typos in the modified paragraphs.
"Applications in a module": What would you think about changing the
heading to "Deploying an Applications as a module" to make it a bit
clearer what the section is about? A partial code example might be
useful to inline too, it could be as simple as:
module foo.app {
exports com.foo to javax.graphics;
:
}
-Alan
Re: [9] Review request: JDK-8170485: Switch to building JavaFX with new module-info syntax
On 06/12/2016 16:10, Kevin Rushforth wrote: Chien & Dave, Please review the preliminary webrev to allow building JavaFX with jdk-9+148 and later: https://bugs.openjdk.java.net/browse/JDK-8170485 http://cr.openjdk.java.net/~kcr/8170485/webrev.00/ The updates to the module-info.java sources look good. -Alan
Re: [9] review request: 8131888: Deliver javafx.swt as a modular jar in JDK 9
On 27/05/2016 13:47, Kevin Rushforth wrote: The qualified exports are done using reflection to the calling module that contains the javafx.embed.swt.FXCanvas class, irrespective of the name of the module (so it works even when the javafx.embed.swt package is in the unnamed module). I plan to file a follow-on bug to tighten the integrity checks, which may or may not include requiring it to be in a module named "javafx.swt", depending on whether all of the use cases can be done with javafx-swt.jar being loaded in a named module (e.g., Mandy's recommendation of using the Layer API). Okay and using addExports is really the only way this will work when javafx.swt is a child layer. As regards the mixing of code in named modules and unnamed modules then it should work fine. javafx.swt will read all modules in the boot layer. Code this module will also link to SWT types and for that to work then they must be visible by means of its class loader. The simplest is to just specify that loader as the parent class loader when creating the child layer. -Alan
Re: [9] review request: 8131888: Deliver javafx.swt as a modular jar in JDK 9
On 26/05/2016 16:38, Kevin Rushforth wrote: Yes, I've tested it in both modes (with a simple HelloFXCanvas program) -- as an automatic jar file and as just an ordinary jar on the classpath. Just curious, if there are qualified exports to javafx.swt then how it does when on the class path? -Alan
Re: [9] Review request: 8154203: Use StackWalker instead of the now-deprecated sun.reflect.Reflection class
On 14/04/2016 21:27, Kevin Rushforth wrote: Hi Vadim, Please review: https://bugs.openjdk.java.net/browse/JDK-8154203 http://cr.openjdk.java.net/~kcr/8154203/webrev.00/ It's a straight-forward fix to replace Reflection::getCallerClass with StackWalker::getCallerClass and remove the (no longer needed) CallerSensitive annotation. This looks good.
Re: [9] Review request for FX Jigsaw changes
On 17/03/2016 03:04, Kevin Rushforth wrote: I should add that the only changes are related to: 1) Renamed JDK9_HOME to JIGSAW_HOME Is this for transition purposes and it would revert again to JDK9_HOME later? -Alan
