JDK-8134716 : Remove use of internal classes methods from toys/Hello
  • Type: Bug
  • Component: javafx
  • Sub-Component: samples
  • Affected Version: 9
  • Priority: P3
  • Status: Resolved
  • Resolution: Fixed
  • Submitted: 2015-08-29
  • Updated: 2015-12-02
  • Resolved: 2015-12-02
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 9
9Fixed
Related Reports
Blocks :  
JDK-8077916 - Complete development work for project one: make UI control skins public API
Blocks :  
JDK-8077918 - Complete development work for project two: review and make public relevant CSS API
Relates :  
JDK-8136898 - [TOYS] HelloDialogs throws NPE for ExceptionDialog, showLinearWizard, and showBranchingWizard
Relates :  
JDK-8144130 - Remove use of internal classes methods from IPodAccessTest.java
Relates :  
JDK-8144353 - gradle apps fails to build on linux-arm platforms
Description
This is a follow up work from JDK-8088670 that should be done once the modularization work for Control is done.  Here is the remaining internal classes referenced in toys: 

./Hello/src/main/java/hello/dialog/dialogs/CommandLinksDialog.java:import com.sun.javafx.scene.control.skin.AccordionSkin; 
./Hello/src/main/java/hello/dialog/dialogs/CommandLinksDialog.java:import com.sun.javafx.scene.control.skin.resources.ControlResources; 
./Hello/src/main/java/hello/dialog/dialogs/ExceptionDialog.java:import com.sun.javafx.scene.control.skin.AccordionSkin; 
./Hello/src/main/java/hello/dialog/dialogs/FontSelectorDialog.java:import com.sun.javafx.scene.control.skin.AccordionSkin; 
./Hello/src/main/java/hello/dialog/wizard/Wizard.java:import com.sun.javafx.scene.control.skin.AccordionSkin; 
./Hello/src/main/java/hello/HelloFPS.java:import com.sun.javafx.perf.PerformanceTracker; 
./Hello/src/main/java/hello/HelloHighContrast.java:import com.sun.javafx.css.StyleManager; 
./iOS/iPodAccessTest/src/ipodaccesstest/IPodAccessTest.java:import com.sun.javafx.ext.device.ios.ipod.MediaQuery; 
./iOS/iPodAccessTest/src/ipodaccesstest/IPodAccessTest.java:import com.sun.javafx.ext.device.ios.ipod.MediaItem; 
./iOS/iPodAccessTest/src/ipodaccesstest/IPodAccessTest.java:import com.sun.javafx.ext.device.ios.ipod.MediaFilter;
Comments
http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/84642f430f0d
02-12-2015
OK, it looks like this might be a webrev issue then. Given your rechecking of the patch, this is good to go. +1
30-11-2015
Separated issue https://bugs.openjdk.java.net/browse/JDK-8144130 opened regarding next internal classes: ./iOS/iPodAccessTest/src/ipodaccesstest/IPodAccessTest.java:import com.sun.javafx.ext.device.ios.ipod.MediaQuery; ./iOS/iPodAccessTest/src/ipodaccesstest/IPodAccessTest.java:import com.sun.javafx.ext.device.ios.ipod.MediaItem; ./iOS/iPodAccessTest/src/ipodaccesstest/IPodAccessTest.java:import com.sun.javafx.ext.device.ios.ipod.MediaFilter;
26-11-2015
Rechecked: Made: hg revert --all; gradle clean. Applying the patch by: "hg import --no-commit rt.patch" is removing Robot folder from rt/tests/manual. In addition result of command ���hg diff --git | grep rename��� on my work space is: rename from apps/toys/Hello/src/main/java/hello/HelloFPS.java rename to apps/tests/HelloTest/src/hellotest/HelloFPS.java rename from apps/toys/Hello/src/main/java/hello/HelloHighContrast.java rename to apps/tests/HelloTest/src/hellotest/HelloHighContrast.java rename from tests/manual/Robot/build.xml rename to apps/tests/Robot/build.xml rename from tests/manual/Robot/manifest.mf rename to apps/tests/Robot/manifest.mf rename from tests/manual/Robot/nbproject/build-impl.xml rename to apps/tests/Robot/nbproject/build-impl.xml rename from tests/manual/Robot/nbproject/genfiles.properties rename to apps/tests/Robot/nbproject/genfiles.properties rename from tests/manual/Robot/nbproject/project.properties rename to apps/tests/Robot/nbproject/project.properties rename from tests/manual/Robot/nbproject/project.xml rename to apps/tests/Robot/nbproject/project.xml rename from tests/manual/Robot/src/robottest/BMPOutputStream.java rename to apps/tests/Robot/src/robottest/BMPOutputStream.java rename from tests/manual/Robot/src/robottest/RobotBuilder.java rename to apps/tests/Robot/src/robottest/RobotBuilder.java rename from tests/manual/Robot/src/robottest/RobotTest.java rename to apps/tests/Robot/src/robottest/RobotTest.java rename from tests/manual/Robot/src/robottest/RobotTestStyles.css rename to apps/tests/Robot/src/robottest/RobotTestStyles.css Therefore I think that commit of this patch will not leave no "garbage".
26-11-2015
The code changes and build changes look good. I see the new source files in Robot now (which is good), but after applying the patch the files still also exist in the old place, as if they were copied rather than moved. This must be fixed so we don't end up with two copies and so that the files are tracked in hg as renamed files. Also, since the webrev doesn't provide git diffs, I can't tell if the source files for the two moved Hello* apps are going to be tracked by hg as renamed files (otherwise you lose the history of those files). You can check this by running "hg diff --git". You should see a line that looks like this for each file: diff --git a/apps/toys/Hello/src/main/java/hello/HelloFPS.java b/apps/tests/HelloTest/src/hellotest/HelloFPS.java rename from apps/toys/Hello/src/main/java/hello/HelloFPS.java rename to apps/tests/HelloTest/src/hellotest/HelloFPS.java
26-11-2015
Updated webrev to solve the problem of HelloFPS.java & HelloHighContrast.java: http://cr.openjdk.java.net/~ekleyman/JDK-8134716-25/ Please notice that samples (including Robot) are being moved to rt/apps/tests in order to possible compilation by "gradle apps"
25-11-2015
The attached patch (from last week) adds the new Robot tests rather than moving them with "hg mv". This must be fixed so we don't end up with two copies. Please attach a pointer to an updated webrev after fixing this. Also, it will make it easier to review if everything is in a single webrev (rather than having to apply your earlier webrev and then David's patch).
24-11-2015
One other comment. The "addExports" file is not relevant for the mainline repo and should be removed. It can be pushed to the FX jake repo, along with any build changes needed to use it.
24-11-2015
Adding a combined patch on top of Elina's latest webrev. tests are now in apps/tests hooked up for a build added a apps/tests/README.txt file added addExports for each app, for use with JDK9 This is a mq patch, so can be pulled in with hg qimport testsMoved.patch It has the moved files as moved.
18-11-2015
I think that apps/test would be easier to script, a little less convoluted. As I said, I could certainly tell the apps build.xml to do a ant -f ../test/manual if we want. But that seems a convolution. That said - we still have tests/manual/printing under tests. Part of me would like to auto build pretty much everything in a nightly.
18-11-2015
A fair point. Perhaps we could revisit the location of those test programs that were formerly in apps/toys but which are really used as test programs that use internal interfaces. This would apply to the Robot-enabled version of HelloSanity and the two toys that Elina is moving now. Maybe apps/tests would be a better location?
18-11-2015
@Kevin - currently nothing under ./tests/ is part of 'gradle apps' While anything is possible - perhaps these should land under ./apps/ instead ?
18-11-2015
The moved tests are not wired up to the build. They should be built as part of "gradle apps"
18-11-2015
Webrev to solve the problem of HelloFPS.java & HelloHighContrast.java: http://cr.openjdk.java.net/~ekleyman/JDK-8134716/
17-11-2015
setTitle() was removed from CommandLinksDialog.java in http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/a153c084cf1d
17-11-2015
Agreed. Getting rid of it of it is better than just replacing it with just a different internal usage.
11-11-2015
I think the setTitle() statement in CommandLinksDialog can be removed, or at least replaced with a hard coded string. There is no need for l10n here, and as a demo app it shouldn't be digging into internal messages. Besides, the title for the dialog is overridden after it is created in HelloDialogs: CommandLinksDialog dlg = new CommandLinksDialog(links); dlg.setTitle("Manually connect to wireless network");
11-11-2015
I'm not sure this will work. It no longer uses an internal class (which is good), but it still references a resource bundle that is inside a JavaFX module. Have you run this with a jigsaw build and verified that the title appears?
11-11-2015
Suggested fix for CommandLinksDialog.java: http://cr.openjdk.java.net/~ekleyman/JDK-8134716-part2/ After the related bug https://bugs.openjdk.java.net/browse/JDK-8136898 will be solved, suggested fix should be verified by execution HelloDialogs with Jigsaw and committed.
11-11-2015
Seems that com.sun.javafx.scene.control.skin.AccordionSkin was already changed to javafx.scene.control.skin.AccordionSkin in classes: CommandLinksDialog.java ExceptionDialog.java, FontSelectorDialog.java Wizard.java. And StyleManager is not used in HelloHighContrast.java
04-11-2015
The understanding is that PerformanceTracker and StyleManager will stay put so we will need to decide what to do with HelloFPS.java and HelloHighContrast.java. One possible alternative is to move them to tests.
03-11-2015