RFR: 8311895: CSS Transitions [v9]

Tue Nov 14 14:23:53 UTC 2023

On Tue, 31 Oct 2023 17:24:05 GMT, Michael Strauß <mstrauss at openjdk.org> wrote:

>> Implementation of [CSS Transitions](https://gist.github.com/mstr2/c72f8c9faa87de14926978f517a6018a).
>> 
>> ### Example
>> 
>> .button {
>>     -fx-background-color: dodgerblue;
>> }
>> 
>> .button:hover {
>>     -fx-background-color: red;
>>     -fx-scale-x: 1.1;
>>     -fx-scale-y: 1.1;
>> 
>>     transition: -fx-background-color 0.5s ease,
>>                 -fx-scale-x 0.5s ease,
>>                 -fx-scale-y 0.5s ease;
>> }
>> 
>> <img src="https://user-images.githubusercontent.com/43553916/184781143-0520fbfe-54bf-4b8d-93ac-834708e46500.gif" width="200"/>
>> 
>> ### Limitations
>> This implementation supports both shorthand and longhand notations for the `transition` property. However, due to limitations of JavaFX CSS, mixing both notations doesn't work:
>> 
>> .button {
>>     transition: -fx-background-color 1s;
>>     transition-easing-function: linear;
>> }
>> 
>> This issue should be addressed in a follow-up enhancement.
>
> 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.

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.

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);

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))));

modules/javafx.graphics/src/main/java/javafx/css/StyleableDoubleProperty.java line 78:

> 76: 
> 77:         if (transition != null) {
> 78:             timer = TransitionTimer.run(new TransitionTimerImpl(this, v), transition);

Would it be possible to check here if this timer is going to the same target?  Also see other comment about a simplification.

     if (timer == null || timer.newValue.equals(v))) {
          timer = TransitionTimer.run(new TransitionTimerImpl(this, v), transition);
     }

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)

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

PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1392288693
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1392313184
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1392315492
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1392304130
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1392555447
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1392551399