JDK-8157350 : Encapsulate impl_ methods in Shapes related classes
  • Type: Enhancement
  • Component: javafx
  • Sub-Component: graphics
  • Affected Version: 9
  • Priority: P3
  • Status: Resolved
  • Resolution: Fixed
  • Submitted: 2016-05-19
  • Updated: 2016-05-24
  • Resolved: 2016-05-21
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.
Related Reports
Blocks :  
This is to make JDK-8144585 more manageable and easier for reviewer to review by breaking the work into smaller pieces.
Changeset: 75f8a4f1d1b9 Author: ckyang Date: 2016-05-20 16:56 -0700 URL: http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/75f8a4f1d1b9

+1 on version .00 with the change to ArcHelper.java that Jim noticed (and, if you want, you can add my recommended additional comment in ShapeHelper).

We looked through all the others and Arc was the only one.

It sounds like you have a reason for the current helper paradigm as we move forward so that is good. The naming anomalies aren't functional, but annoying. So, no real open issues from my perspective. Note that Arc was the only helper I looked at so I can't say that there aren't other naming typos in there...

I am OK with version .00 after the changes pointed out above. Let's make sure that the openjfx-dev thread with Jim's comments are addressed before proceeding.

One thing I missed in my review that Jim pointed out is that there is a typo in ArcHelper.java: 49 public static void initHelper(Arc quadCurve) { 50 setHelper(quadCurve, getInstance()); 51 } that should be "arc" rather than "quadCurve".

Looks good. Two minor comments (no need for a new webrev if you decide to fix them). ShapeAccessor.java: * You can also add the "TODO" comment here, since these will move to NodeAccessor: 115 ShapeHelper getHelper(Shape shape); 116 void setHelper(Shape shape, ShapeHelper shapeHelper); * You have an extra blank line or two at the end of most of the new Helper classes (e.g., ArcHelper.java, ArcToHelper.java, CircleHelper.java) [edited to remove the +1 while discussion is ongoing]

Jim raised some points on the openjfx-dev list, so hold off pushing for now.

Please review the proposed fix. In this fix I also did a minor fix to getMXWindowType() in Window and Stage including the helper classes to match the pattern we will use for overriding helper method. webrev: http://cr.openjdk.java.net/~ckyang/JDK-8157350/webrev.00/