On Tue, 14 Nov 2023 09:41:08 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> Michael Strauß has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Test whether two Interpolatable instances are compatible
>
> modules/javafx.graphics/src/main/java/com/sun/javafx/css/TransitionTimer.java
> line 179:
>
>> 177: if (newTimer.delay < 0) {
>> 178: double adjustedDelay = nanosToMillis(newTimer.delay) *
>> newTimer.reversingShorteningFactor;
>> 179: newTimer.startTime = now + millisToNanos(adjustedDelay);
>
> I don't see the value of converting these back and forth to milliseconds.
> Why not just:
>
> Suggestion:
>
> newTimer.startTime = now + newTimer.delay *
> newTimer.reversingShorteningFactor;
>
> I checked the calculations, and it makes no difference (the millis aren't
> rounded or anything), so you're just moving the decimal point before/after
> doing a multiplication that doesn't care where the decimal point is.
>
> If there is a reason that I missed, then I think this really needs a comment.
I've removed the conversions.
> modules/javafx.graphics/src/main/java/com/sun/javafx/css/TransitionTimer.java
> line 196:
>
>> 194: double frac = (double)(nanos - (millis * 1_000_000L)) /
>> 1_000_000D;
>> 195: return (double)millis + frac;
>> 196: }
>
> This function seems to want to preserve extra decimals in some way. If the
> original long has a magnitude that is so large that some least significant
> digits get lost in the double conversion, then trying to add them later
> (`millis + frac`) will not restore them.
>
> In other words, you can just do:
>
> Suggestion:
>
> private static double nanosToMillis(long nanos) {
> return nanos / 1_000_000.0;
> }
>
>
> Or avoiding the (generally slower) division completely:
>
> Suggestion:
>
> private static double nanosToMillis(long nanos) {
> return nanos * 0.000_001;
> }
>
>
> All the extra operations are only likely to introduce small errors.
Changed as suggested.
> modules/javafx.graphics/src/main/java/com/sun/javafx/css/TransitionTimer.java
> line 207:
>
>> 205: long wholeMillis = (long)millis;
>> 206: double frac = millis - (double)wholeMillis;
>> 207: return wholeMillis * 1_000_000L + (long)(frac * 1_000_000D);
>
> I'd recommend keeping this simple (it seems to want to recover extra
> significant digits, but that's not possible):
>
> Suggestion:
>
> return ((long)(millis * 1_000_000.0);
Changed as suggested.
> modules/javafx.graphics/src/main/java/com/sun/javafx/css/TransitionTimer.java
> line 234:
>
>> 232:
>> 233: Node node = (Node)timer.getProperty().getBean();
>> 234: node.fireEvent(new TransitionEvent(eventType,
>> timer.getProperty(), elapsedTime));
>
> minor:
> Suggestion:
>
> long elapsedTime;
>
> // Elapsed time specification:
> https://www.w3.org/TR/css-transitions-1/#event-transitionevent
> if (eventType == TransitionEvent.RUN || eventType ==
> TransitionEvent.START) {
> elapsedTime = Math.min(Math.max(-timer.delay, 0),
> timer.duration);
> } else if (eventType == TransitionEvent.CANCEL) {
> elapsedTime = Math.max(0, timer.currentTime -
> timer.startTime);
> } else if (eventType == TransitionEvent.END) {
> elapsedTime = timer.duration;
> } else {
> throw new IllegalArgumentException("eventType");
> }
>
> Node node = (Node)timer.getProperty().getBean();
> node.fireEvent(new TransitionEvent(eventType,
> timer.getProperty(), Duration.millis(nanosToMillis(elapsedTime))));
Changed as suggested.
> modules/javafx.graphics/src/main/java/javafx/css/StyleableDoubleProperty.java
> line 111:
>
>> 109: private TransitionTimer<?, ?> timer = null;
>> 110:
>> 111: private static class TransitionTimerImpl extends
>> TransitionTimer<Number, StyleableDoubleProperty> {
>
> I think you can simplify this. TransitionTimer does not need any of its
> generic parameters if you provide an abstract method to get the property
> (just the general interface), which is sufficient for the `getBean` call you
> do on it, and for passing it along as the source of events.
>
> The constructor, `onUpdate` and `onStop` don't need the property parameter at
> all if you make the `TransistionTimerImpl` non-static.
>
> I also have the feeling that instead of subclassing `TransitionTimer`,
> implementing an interface, which acts as the adapter you need between the
> general timer infrastructure and a specific property, would be more clean.
> My main reason for thinking so is the akward way the timer is put to use:
>
> TransitionTimer.run(new TransitionTimerImpl(this, v), transition)
>
> ...calling a static method on `TransitionTimer` while providing a subclass
> instance of it.
>
> It could then look something like this:
>
> class InternalTransitionAdapter implements TransitionAdapter {
> InternalTransitionAdapter(Number value) {
> this.oldValue = property.get();
> this.newValue = value != null ? value.doubleValue() : 0;
> }
>
> @Override
> protected void onUpdate(double progress) {
> set(progress < 1 ? oldValue + (newValue - oldValue) * progress :
> newValue);
> }
>
> @Override
> public void onStop() {
> timer = null;
> }
>
> @Override
> public Property<?> getProperty() {
> return StyleableDoubleProperty.this;
> }
>
> @Override
> protected boolean equalsTargetValue(TransitionAdapter adapter) {
> return newValue ==
> ((InternalTransitionTimerImpl)adapter).newValue;
> }
> }
>
> I also think the `equalsTargetValue` stuff can be done inside this property
> itself (see other comment)
This is now implemented with `TransitionMediator`.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1529033318
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1529033525
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1529033615
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1529033418
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1529033655
