RFR: 8311895: CSS Transitions [v2]

[Michael Strauß]

On Tue, 1 Aug 2023 10:49:42 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>> The constructor ensures that any spelling of "ALL" is converted to the interned constant "all", which is important as we would otherwise need a more computationally expensive case-insensitive string comparison in `Node.Transitions.find()`.
>> Removing the constructor would mean that some unrelated piece of code would need to do this conversion.
>> 
>> The `@throws` tag is not allowed at the class level.
>
> I meant that you can use the short form of the constructor (without repeating the parameters):
> 
>       public record TransitionDefinition(String propertyName, Duration duration,
>                                    Duration delay, Interpolator interpolator) {
>           public TransitionDefinition {  // no parameters necessary (short form constructor)
>                     // do checks and assignments here
>           }
>       }

Done.

>> This algorithm is implemented as [specified](https://www.w3.org/TR/css-easing-1/#step-easing-algo), which accounts for the possibility that the interpolator is used for a back-filling animation (`t < 0`). While this is not possible in JavaFX (but may be possible in the future), I think it makes sense to keep the algorithm as is, because it ensures that its results are always correct, even though negative values for `t` are currently out of spec.
>
> Yeah, I figured as much.  I just don't like code that you can't get covered in a unit test, or that contradicts itself.  As you can see, I was confused, is it a bug or a feature?  Perhaps a comment then to indicate that it's intentional.

This is covered in `StepInterpolatorTest.testStart`: the test will assert that for t<0, the step interpolator produces the expected value. I also added a comment that this is intended.

>> Added a `@throws` tag.
>> Interestingly, `Event` doesn't specify or assert that its `eventType` parameter is non-null. Maybe we should investigate this further.
>
> `eventType` is not used for a lot of things, aside from deciding which list of handlers to pick.  In theory, you could have a `null` `EventType`, and register an event handler on the `null` type; except that most `addEventHandler` methods don't allow this -- they check that the event type is not `null`.
> 
> It probably would be good to reject `null` when creating an `Event`, but that's not for this PR.

I added a null check for `eventType` on the `TransitionEvent` class.

>> Why would this be a sensible thing to do? I'm quite explicitly comparing the identity of `property`, since I'm interested in finding the one property that I'm looking for, not potentially any property that is in some way "equal" to the property.
>> 
>> For (hopefully) all property implementations, `equals` trivially works because it is not an overridden method and therefore falls back to an identity comparison. What would it even mean for a property to be equal, but not identical to another property?
>
> It's up to you, but this is really standard Java practice: you use `equals` which may defer to identity checks, unless there is a very specific reason that you **must** have an identity check.  This leaves the decision of how equality works firmly in the hands of the class author.  
> 
> For styleable properties there may not be an issue, although it is an interface that can have 3rd party implementations. In general though, you never know when something might be a proxy, wrapper or adapter that may not be the exact same instance, but are considered equal for most purposes.
> 
> So if you think it must be an identity check, and you want to avoid others thinking this is a mistake, could you add a comment with the reasoning?

Added a comment.

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1280922738
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1280919648
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1280921115
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1280921286