JDK-8102424 : Node Orientation - API issue: Is isAutomaticallyMirrored() named properly?
  • Type: Enhancement
  • Component: javafx
  • Sub-Component: controls
  • Affected Version: 8
  • Priority: P2
  • Status: Closed
  • Resolution: Fixed
  • Submitted: 2012-11-08
  • Updated: 2015-06-16
  • Resolved: 2013-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.
JDK 8
8Fixed
Related Reports
Blocks :  
JDK-8100124 - Create API for determining the layout direction of a component
Relates :  
JDK-8099289 - Support component orientation in common UI controls
Relates :  
JDK-8116555 - Node Orientation - API issue: Should isAutomaticallyMirrored() be protected?
Relates :  
JDK-8100148 - Add RTL support to Text node
Description
>
>>>
>>> > The same applies to isAutomaticallyMirrored.
>>>
>>> This is a mechanism that allows controls to opt out of mirroring. Conceptually, it should be "... set once in the constructor and never changed...".  I am not particularly happy with this method. Do you have a better suggestion?
>>
>> I've just discussed it locally, there are other options but not particularly nice as well. Guys here also prefer your solution because there is no need to store the value. So I'm withdrawing my objections, however, we believe that the method
>> - needs a better documentation that will state explicitly that it's supposed to return a constant
>> - should be protected (is there any reason for it to be public?)
>> - needs a name that doesn't start with "get" or "is"
>
> I will update the documentation to be better.  Can you show me other examples where the "get" and "is" are not used in FX where they might normally be used? 

For instance Point2D.magnitude() or Transform.determinant().

This is for the compatibility with tools and IDEs that use the naming to determine if it is a property or not.
Comments
I see method usesMirroring(), so close as verified on 8.0b97
08-07-2013
Changeset: http://hg.openjdk.java.net/openjfx/8/controls/rt/rev/6dfca7697482
21-05-2013
I guess what we're saying is that it could be a read-only property, but this does not generate any advantage. The value should only be settable by the implementing class and there is no reason for anybody else to listen to changes. In a way it's too bad we can't leave it as an is*() method and leave it to future generations to decide if they want to make it a property.
13-05-2013
BTW, I didn't get from the comments why this isn't an immutable property (which can then have is/get naming convention)? Or is it a mutable thing?
23-01-2013
Holy crap! I didn't realize the scaleShapeProperty and positionShapeProperty methods were private! Or that it was called positionShapeProperty instead of centerShapeProperty. For the love! Thanks or catching that! (same for cacheShapeProperty. Copy paste bug gone berserk).
23-01-2013
Rolled back changeset pending API review.
22-01-2013
This should have gone through the API process before the API name was changed. Can you run it through the openjfx-dev list, at least a pointer to the issue? I sent a message just last Friday to the list as to how this is done. Thanks!
21-01-2013
Changeset: http://hg.openjdk.java.net/openjfx/8/graphics/rt/rev/67fd43b5577a
21-01-2013
>This goes far beyond the scope of this RT. We have a certain naming convention and encourage tools to rely on it - if you want to change that, please >open a wider discussion on that topic. Meanwhile, we should adhere to the existing conventions. Hmm, I don't think we should or could change naming conventions in the middle of the game. That said, if my intent was to help tools I would have used annotations instead. >> Another suggestion: usesMirroring(). >Sounds good to me. Richard Bair should have the final word here. Yes, agreed.
13-11-2012
> As for not using is* when is not property, although is common to have the property for is* and get*, that is not always true. See Region#isScaleShape() or isCenterShape() Yes, we've violated the rule a few times in layouts, but it is a failure, not a precedent to follow. (The two examples you mentioned actually do have properties, but they are private, I don't know the reasons for that) > I think that forcing every method that start with is* and get* to have a property is a very strong mandate. It should not be that way. This goes far beyond the scope of this RT. We have a certain naming convention and encourage tools to rely on it - if you want to change that, please open a wider discussion on that topic. Meanwhile, we should adhere to the existing conventions. > Another suggestion: usesMirroring(). Sounds good to me. Richard Bair should have the final word here.
13-11-2012
Another suggestion: usesMirroring(). I agree that this should not be a property.
12-11-2012
>So, in a pure LTR scenario, most nodes would return true for isMirrored() because it only applies to RTL? That just seems a bit misleading. What's wrong with including ForRTL in the name? by default, isMirrored() returns orientation==RTL Text for example, would reimplement it to return false. This allows Text (or any other Node) to be RTL (honoring setNodeOrienation/getNodeOrienation) without being mirrored. So, isMirrored() can not be a property because it can be overritten. If the application needs to know if the value changed it has to listem to nodeOrienation or whatever property is relevant for the given Node. In the case of Text, that would be text. How important is that anyway ? As for not using is* when is not property, although is common to have the property for is* and get*, that is not always true. See Region#isScaleShape() or isCenterShape() I think that forcing every method that start with is* and get* to have a property is a very strong mandate. It should not be that way.
12-11-2012
If you read the original bug comment by Pavel, "is" is bad when not a property. Also, the name needs to idicate that it wants (or does not want) a mirrored implementation, not that the node itself is currently mirrored.
12-11-2012
So, in a pure LTR scenario, most nodes would return true for isMirrored() because it only applies to RTL? That just seems a bit misleading. What's wrong with including ForRTL in the name?
12-11-2012
>I think a name like isMirrored() could be interpreted as whether the node is actually mirrored (depending on effective orientation), and that is fine, because the answer will be correct. Allowing implementors to take the correct decision to handle coordinate translation, key down events, etc. Internally Node will use this property to implement mirroring. This allow for Nodes like Text, to be RTL without mirroring. I am sure I am missing something simple here, what is the problem with the is* pattern ? (we use it everywhere)
12-11-2012
In order to avoid the is* pattern, how about something like useMirroringForRTL()?
12-11-2012
I think a name like isMirrored() could be interpreted as whether the node is actually mirrored (depending on effective orientation), while the intended meaning is more whether the node should be automatically mirrored if the actual orientation is RTL.
12-11-2012
I like isMirrored(), what is wrong with that ? Note: we already have Text#isUnderline(), Node#isVisivle(), Node#isDisabled(), etc
12-11-2012
Raising prioirty to indicate that we cannot ship with this unresolved.
12-11-2012