API REVIEW REQUEST: Public API for Node Orientation

[Pavel Safrata]

Hi Steve,

On 29.10.2012 16:17, steve.x.northover at oracle.com wrote:
>
>
> On 29/10/2012 7:49 AM, Pavel Safrata wrote:
>> Hi Steve,
>>
>> On 26.10.2012 18:41, steve.x.northover at oracle.com wrote:
>>>  Hi Pavel!
>>>
>>> > the inheritance ignoring reparenting.
>>>
>>> I don't think this was explained well in the documentation. There 
>>> should be no difference in visual behavior for the final result with 
>>> respect to the ordering of "orientate" and "insert" operations.
>>
>> It seems to be explained well. This is how I understand it, please 
>> tell me which of the two statements is incorrect and why:
>>
>> I have a left-to-right parent with an inheriting child. I create new 
>> parent, "orientate" it to right-to-left and "insert" it between the 
>> original parent and the child. Based on "If an application explicitly 
>> sets the root of a hierarchy to left-to-right and then reparents the 
>> hierarchy into a parent that is right-to-left, the hierarchy will 
>> remain left-to-right" I understand that the child will remain 
>> left-to-right.
>>
>> Again, I have a left-to-right parent with an inheriting child. I 
>> create a new parent and "insert" it between the original parent and 
>> the child. Then I "orientate" it to right-to-left. Based on 
>> "Inheritance of node orientation allows application developers to 
>> specify the orientation of a root node and have it apply to all 
>> children" I understand that the new orientation will be applied to 
>> the child, so it will become right-to-left.
>>
>
> The second statement is true.  The behavior can be summarized as: 
> "When not explicitly set, orientation is inherited".  I'm not sure 
> about the confusion in the first statement.  The sentence is meant to 
> mean that a hierarchy of nodes with an explicitly set root will always 
> have the explicitly set orientation of the root no matter where the 
> root is reparented.  Perhaps I should delete the sentence and replace 
> it with something like what I just said.

Got it. The confusion is that you mean reparenting the hierarchy 
including the root, I thought you meant leaving the root on place and 
reparenting its children to a different root. So I think it is ok (but 
yes, rewording the sentence may be useful, I'm not the only one to 
understand it that way).

>
>>>
>>> > How will mirroring cooperate with transformations?
>>>
>>> The mirroring transformation is transparent to the application and 
>>> is included automatically in local-to-scene (it's a bug if it is 
>>> not).  A public Mirror (or rather Flip) transformation would provide 
>>> API for this transformation, but I'm not sure why we would need to 
>>> do this.
>>
>> Ah, that sounds quite good. The only thing that slightly bothers me 
>> is the state where there are no transformations anywhere and 
>> local-to-scene transform still reports it is not an identity 
>> transform, which seems confusing. But perhaps I'm too picky.
>>
>>>
>>> > Shouldn't effectiveNodeOrientation be a property?
>>>
>>> That's a possibility.  It would be a properly that changed when 
>>> inherited orientation up the ancestor tree changed.  Do we have any 
>>> other properties like this in FX?
>>
>> localToSceneTransform :-) But I admit there is some extra logic 
>> needed for such properties that we don't want to add blindly for 
>> performance reasons. So it may be better to just rename the getter to 
>> simply effectiveNodeOrientation().
>>
>
> It might be that this needs to be a property after all.  The issue is 
> that a child may have state that is sets based on effective 
> orientation (say alignment of a text node) and this state needs to be 
> kept up to date with effective orientation.  However, providing the 
> method is defined correctly, there is nothing stopping it from 
> becoming a property in future.  I understand the performance issue.  I 
> will investigate further.

For a property we'd have effectiveNodeOrientationProperty() and 
getEffectiveNodeOrientation(). For a non-property we'd have something 
like effectiveNodeOrientation(). So I think we need to decide in the 
beginning..

>
>>>
>>> > 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.

> I am not a fan of protected.  Other than indicating explicitly that 
> subclasses are supposed to override this method, are there any other 
> benefits?

I believe it is a good approach not to publish things that don't need to 
be public. It is a node implementation thing, it should not confuse 
users in the list of publicly accessible methods. Other than you not 
being a fan, are there any concrete reasons for it to be public? (the 
fan thing doesn't leave much room for discussion)

>
>>
>>>
>>> > Could you please elaborate on "the application will need to 
>>> configure parameters that are appropriate for the effect in both 
>>> orientations"?
>>>
>>> For example, if you want a light source effect to come from the 
>>> upper left corner when a control is RTL, you will need to create an 
>>> effect where the light source comes from the upper right corner so 
>>> that when the control is mirrored, it will come from the left.
>>
>> Hmm, I would prefer to do that automatically, I don't think anybody 
>> wants the reversed shadow just because the reading direction is 
>> different. But it looks like it would require serious rework of 
>> effects which is probably not feasible..
>
> This issue is this:  You can't know what the application wants. In 
> some cases, it is using an effect as part of a control theme and it 
> makes sense for the effect to go from right-to-left when the 
> orientation changes.  In other cases, there is directionality involved 
> that should remain constant (like the car example in the documentation).

I think that effects are quite independent of what the application 
wants. The reflection always has to reflect the rendered node (having a 
right-to-left node with left-to-right reflection doesn't make any 
sense), and I think shadow is always dropped the same way based on the 
light source, regardless of it being right-to-left text or a car door. 
But again, I don't see any reasonable way to implement this right now.

Pavel

>
>>
>> Pavel
>>
>>>
>>> Steve
>>>
>>> On 26/10/2012 9:16 AM, Pavel Safrata wrote:
>>>> Hi Steve,
>>>> I have a few comments/questions.
>>>>
>>>> I'm not sure about the inheritance ignoring reparenting. I think 
>>>> that if an application will use orientation extensively it will 
>>>> reach a hard-to-trace "mess state" where most of the nodes 
>>>> "inherit" but they don't actually have the parent's value. Also it 
>>>> means that peforming "orientate parent" - "insert it into scene" 
>>>> will result in a different behavior than "insert" first and then 
>>>> "orientate", which seems confusing. What if I create a new node and 
>>>> insert it into scene, will it inherit form its new parent? In 
>>>> summary, I find this behavior hard to track and I think that when 
>>>> the value is Inherit it should always match the parent's orientation.
>>>>
>>>> How will mirroring cooperate with transformations? For instance 
>>>> user can obtain local-to-scene transformation and if the mirrorring 
>>>> is not contained there, the computations with the transform (such 
>>>> as transforming points) will be wrong. Maybe we could just 
>>>> introduce a public Mirror (or rather Flip) transformation and use 
>>>> it publicly for the mirrorring?
>>>>
>>>> How will it behave in 3D? Mirror nodes along X axis regardless of 
>>>> their z-direction volume?
>>>>
>>>> Shouldn't effectiveNodeOrientation be a property? It seems it might 
>>>> make sense to observe the value. Also our naming convention is that 
>>>> you should not use getSomthing unless "something" is a property.
>>>>
>>>> The same applies to isAutomaticallyMirrored. This method seems 
>>>> weird anyway. When and how often is it called? Can a node change 
>>>> the value dynamically? If yes, we should have a property, if not, 
>>>> we should make sure it doesn't - let the node call some init method 
>>>> in the constructor or something like that.
>>>>
>>>> Could you please elaborate on "the application will need to 
>>>> configure parameters that are appropriate for the effect in both 
>>>> orientations"? How do I drop the shadow to the same direction for 
>>>> all nodes, specifically?
>>>>
>>>> Thanks,
>>>> Pavel
>>>>
>>>> On 23.10.2012 22:30, steve.x.northover at oracle.com wrote:
>>>>> Hi all,
>>>>>
>>>>> I have been looking into Node Orientation which is an API that 
>>>>> controls the directionality of a Node.  This is different from 
>>>>> BIDI or the BIDI algorithm which governs the direction of text. 
>>>>> Node orientation concerns the flow of visual data which is either 
>>>>> left-to-right or right-to-left.  The best example is a tree 
>>>>> control.  In tree control that is oriented right-to-left, the 
>>>>> expansion arrows point to the right and the branches of the tree 
>>>>> expand from the right to the left.
>>>>>
>>>>> https://wikis.oracle.com/display/OpenJDK/Node+Orientation+in+JavaFX
>>>>>
>>>>> Steve
>>>>
>>