RFR: 8311895: CSS Transitions [v9]

[Michael Strauß]

On Tue, 14 Nov 2023 09:41:08 GMT, John Hendrikx <jhendrikx at 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