JDK-8091485 : Ensemble8: Review each sample description, playground, appearance, related docs and links
  • Type: Enhancement
  • Component: javafx
  • Sub-Component: samples
  • Affected Version: 8,9
  • Priority: P3
  • Status: Resolved
  • Resolution: Fixed
  • Submitted: 2013-01-16
  • Updated: 2020-01-31
  • Resolved: 2016-02-17
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.
8u152Fixed 9Fixed
Related Reports
Blocks :  
Relates :  
Webrev for 8u backport: http://cr.openjdk.java.net/~kcr/Ensemble8/separate/04-8091485/webrev/

This is part of an aggregate backport request to port Ensemble8 fixes from FX 9 to FX 8u. See JDK-8168611 for details and review.

Fixed in 9-dev with this change set. Changeset: 4f83dc142ae6 Author: Morris Meyer <morris.meyer@oracle.com> Date: 2016-02-17 17:40 -0500 URL: http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/4f83dc142ae6 8091485: [Ensemble] Review each sample description, playground, appearance, related docs and links Reviewed-by: ddhill, vadim

+1 I had some really minor nits, but they were nothing to make a change over. The overall impression I had was this was a majorly needed cleanup :-)

Thanks for those Vadim. Will try to get a new web rev up today.

LiveScatterChartApp constructor could use Timeline(KeyFrame... keyFrames) and SequentialTransition(Animation... children) constructors. PaginationApp.createContent:75 - maybe String.format("/ensemble/samples/shared-resources/Animal%d.jpg", (i + 1)); would be better? DisplayShelfApp, XylophoneApp, AudioClipApp - the same. SpinnerApp:127 - return new Group(intBlock, doubleBlock, stringBlock); would be better? BidiApp:91 - maybe move textFlow constructor to line 89 and use new TextFlow(text1, text2) and later just return new Group(textFlow); ? ReflectionApp:77 - return new Group(sample); ? SepiaToneApp:70 - the same? ImageCreationApp - what's the point of creating a Group with a single VBox in it? ImagePropertiesApp:65 - leftover debug? SwingInterop - somehow listeners remained non-lambdified?

I sanity tested this on Windows and didn't see any obvious issues or problems. I will leave the detailed review to Dave Hill and to Vadim.

To provide context for reviewers without having to go back to the mailing list, here is the latest webrev link, and the e-mail from Morris: WEBREV - http://cr.openjdk.java.net/~morris/JDK-8091485.03/ ---------- [email from Morris] Enclosed is my very large webrev for Ensemble8 that has visual appearance changes for each and every sample file. Every sample was gone through and checked for appearance, related docs and links. Every sample code file was scrubbed for bad variable names, long-lines, readability, clarity, and comment format. Every sample now has a link to the JavaFX documentation relevant to that sample, an ordered list of API links for the sample and a populated list of related samples. Each link in each sample was tested to verify correct linkage and accuracy. All changes were to the samples source code and not Samples.java file. Every import statement in each sample was tested to see if the source code actually used it. Lots of imports were culled. After these changes Ensemble8 would be much more ready for the Apple App Store.

Thanks Vadim.

Actually there's like 12 of them.

Morris, there's a typo in the apps/samples/Ensemble8/src/samples/java/ensemble/samples/animation/interpolator/InterpolatorApp.java at line 57 Trransitions

Filed JDK-8143274

It is invoked from build.xml. And I see there is mismatch between build.gradle and build.xml. Need to file a bug on that.

Yes, it is checked in, but it is not expected to be edited manually. It is instead rebuilt by apps\samples\Ensemble8\src\compiletime\java\ensemble\compiletime\EnsembleCompiletimeMain.java It takes information from particular demo files headers. See the following changesets, for example: changeset: 5970:53fbe438d684 user: Alexander Kouznetsov date: Tue Dec 10 03:01:58 2013 -0800 summary: Ensemble8: Fix for RT-34787 Ensemble8: Rebuild SamplesAll java and search index for Ensemble8 changeset: 5963:674968d9c3b6 user: Alexander Kouznetsov date: Mon Dec 09 20:44:03 2013 -0800 summary: Ensemble8: Fix for RT-34716 Ensemble8: xAxis label playground warning

Do you have screenshots of how SpinnerApp looked before and after your change? I'm surprised VBox didn't work.

Samples.java file is generated file. You did not manually edit it, right? Changes inside it are hard to review. So I expect every change to be reflected in some other file change.

Thanks for the comments Vadim. Please review: http://cr.openjdk.java.net/~morris/JDK-8091485.02/

Morris, Could you also remove excessive parentheses "(Math.random() * 10)" from two last lines in the BubbleChartApp.java?

Alexander - this is all of the changes except for 1 and 2 which will be filed as follow-on bugs. Most of the changes were in Samples.java which mainly consisted of sorting and trimming related classes and adding related samples to those samples that were missing those links. Will change the layout class in SpinnerApp

It seems to be just first part of the changes right? It doesn't seem to cover all 26 issues listed above.

With regards to the following: apps/samples/Ensemble8/src/app/java/ensemble/SampleInfo.java public List<URL> getSources() { + Arrays.sort(resourceUrls); return sources; } I'd expect sorting to happen once at initialization. Although it is fine if it happens only during code generation.

I'm surprised we have that many changes in apps/samples/Ensemble8/src/generated/java/ensemble/generated/Samples.java but don't have them anywhere else in the code. Is it because of the links reordering?

Using a Group in Layout may not be considered as a good practice. apps/samples/Ensemble8/src/samples/java/ensemble/samples/controls/spinner/SpinnerApp.java - return new VBox(25, intBlock, doubleBlock, stringBlock); + Group group = new Group(); + group.getChildren().addAll(intBlock, doubleBlock, stringBlock); + return group;

Please review: http://cr.openjdk.java.net/~morris/JDK-8091485.01/

Nits 1. Timeline Events loses its grow / shrink animation as you click API doc links and come back 2. Interpolator the leftmost axis of the interpolation shifts as you switch away and back - which further and further narrows in interpolation such that it looks like the animation hangs. 3. Advanced BubbleChart the color contrast between the two chart bubble colors could be more distinct. 4. ColorPicker and DatePicker could use each other to populate their Related Samples. 5. HTMLEditor imports should be alphabetized. 6. Source code files should be alphabetized. E.g. ListViewCellFactory, TableCellFactory. 7. MenuApp uses System.err 8. SpinnerApp should center cluster of spinners. 9. Search Box should have related sample (perhaps Text Field). 10. Simple Label should add javafx.scene.control.ContentLabel to related documents. 11. Text Field should have related sample (perhaps Search Box). 12. TextFlow should have related sample (perhaps BiDi). 13. Styled Tool Bar needs to add Button and Slider to related documents. 14. Tool Bar needs to add Button to related documents. Needs to add Styled Tool Bar to related samples. 15. TableTreeView should have related sample (perhaps TableView). 16. BouncingBalls java.util.ArrayList and java.util.List API docs links have "compact1, compact2, compact3" above the package line of the class documentation. Related documents for BouncingBalls should reflect the utility of the app. Circle, Reflection, Interpolator missing. 17. BrickBreaker sample legend and score are hard to read. Paddle should start out wider and get smaller as game progresses. Application and Platform not necessary in related documents. No related sample (perhaps Calculator). 18. Calculator sample needs to trim down related documents to relevant classes. No related sample (perhaps BrickBreaker). 19. Canvas Fireworks, Colorful Circles, Digital Clock, Display Shelf needs related sample. 20. Drop Shadow, Gaussian Blur, Inner Shadow, Reflection and Sepia Tone need related sample. Inner Shadow needs StackPane and Text import. Reflection and Sepia Tone need ImageView import. 21. 3d samples need to have related sample. 3D Sphere needs more space for Related Documents (steal from Playground). 22. Language samples need to have related sample. FXML Login - trim java. imports. SwingInterop doesn't need Application, Stage, Scene import. Service and Task need related samples. 23.. StackPane needs related sample. TilePane could add Button and ImageView imports. VBox sample could add CheckBox and Label imports. 24.. Advanced Media, Alpha Media Player, Overlay Media Player and Streaming Media Player need MediaView import. Audio Clip needs Light and Lighting import. 25. Stage doesn't use or need Platform import. AdvancedStage could use Color and Circle import. Cursor, Key Strong Motion, MouseEvent need related sample. Gesture Event could do without FXCollections, ObservableList and EventHandler imports. Key Stroke Motion could do with Timeline and KeyFrame imports. 26. ChangeListener needs related doc sort and related sample.

This is pretty much done for Lombard but needs to be repeated in 9.