[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    Re: [9] Code Review Request For 8157350: Encapsulate impl_ methods in Shapes related classes
From:       Jim Graham <james.graham () oracle ! com>
Date:       2016-05-20 23:23:42
Message-ID: 1a8c4ab3-e41e-5e5f-57b3-ac8ab7da2617 () oracle ! com
[Download RAW message or body]

Sounds good...

			...jim

On 5/20/16 3:56 PM, Kevin Rushforth wrote:
> 
> 
> Jim Graham wrote:
> > 
> > 
> > On 5/20/16 3:33 PM, Kevin Rushforth wrote:
> > > This is needed for those cases where we need to encapsulate a method in the \
> > > base Shape class that used to be public and overridden in the subclasses, not \
> > > all of which are in the same package. It may seem like overkill, but we need a \
> > > way to associate the the Shape instance of a particular subtype with the helper \
> > > instance of the correct subtype. Each class in the hierarchy calls the specific \
> > > XxxxxHelper.initHelper(this) method so that it can store back an instance of \
> > > the right helper in the base class. A package-private method wouldn't work \
> > > given that some shapes (e.g., Text) are in different packages.
> > 
> > Right, but (taking Arc as an example) Arc makes a specific reference to ArcHelper \
> > which turns around and hands a specific instance to its own instance field to a \
> > method that stores the value in the shapeHelper field.  How is that any different \
> > from just putting shapeHelper = ArcHelper.instance without 2 method calls and an \
> > accessor in the way?
> 
> But the shapeHelper field is in the base Shape class not in the Arc class. If we \
> wanted a different pattern for classes in the same package as the base class from \
> classes in a different package then I guess I can see how this is solvable by \
> making the ArcHelper.getInstance() method public and having the Arc() constructors \
> call the package-scope setHelper(ShapeHelper) method in Shape, but as soon as Chien \
> move the stored helper instance up to Node (which is the next step) it would stop \
> working. 
> > Also, what if someone creates a custom sub-class of Shape?  (Not sure if that is \
> > supported or possible, but it is a public class with a public constructor so I \
> > don't think it is impossible.)
> 
> Since we don't have public API for many of the things they would need even today, \
> an application isn't able to do that. They couldn't really do it anyway before this \
> change, since impl_getPeer() and several other methods aren't implementable by an \
> application (NGNode is not publicly exported for example). 
> 
> > > Good reminder about the implicit "public Shape()" constructor. Chien already \
> > > had to add an explicit public no-arg constructor in two classes. We really \
> > > shouldn't rely on the implicit constructor in our public classes, since it \
> > > makes it easy to make such a mistake.
> > 
> > It would be good to have a tool and/or automated test that warns about this.  \
> > Another reason is that the implicit constructor has no javadocs associated with \
> > it...
> 
> Indeed. I wonder if doclint will help (we are in the process of making our docs \
> doclint clean). 
> -- Kevin
> 
> 
> > 
> > ...jim


[prev in list] [next in list] [prev in thread] [next in thread]