On Tue, 1 Aug 2023 10:49:42 GMT, John Hendrikx <jhendr...@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
