On Mon, 31 Jul 2023 00:09:38 GMT, Michael Strauß <mstra...@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: > > Make TransitionEvent final Some early feedback, it's a lot of code :) modules/javafx.graphics/src/main/docs/javafx/scene/doc-files/cssref.html line 1719: > 1717: <tr> > 1718: <td style="width: 120px; vertical-align: top"><span > class="grammar">jump-start</span></td> > 1719: <td>the interval starts with a rise point when the input > progress value is 0</span></td> This has a span closing tag without an opening one modules/javafx.graphics/src/main/docs/javafx/scene/doc-files/cssref.html line 1723: > 1721: <tr> > 1722: <td style="width: 120px; vertical-align: top"><span > class="grammar">jump-end</span></td> > 1723: <td>the interval ends with a rise point when the input > progress value is 1</span></td> This has a span closing tag without an opening one modules/javafx.graphics/src/main/docs/javafx/scene/doc-files/cssref.html line 1727: > 1725: <tr> > 1726: <td style="width: 120px; vertical-align: top"><span > class="grammar">jump-none</span></td> > 1727: <td>all rise points are within the open interval > (0..1)</span></td> This has a span closing tag without an opening one modules/javafx.graphics/src/main/docs/javafx/scene/doc-files/cssref.html line 1732: > 1730: <td style="width: 120px; vertical-align: top"><span > class="grammar">jump-both</span></td> > 1731: <td>the interval starts with a rise point when the input > progress value is 0, > 1732: and ends with a rise point when the input progress > value is 1</span></td> This has a span closing tag without an opening one modules/javafx.graphics/src/main/java/com/sun/javafx/css/TransitionDefinition.java line 29: > 27: > 28: import javafx.animation.Interpolator; > 29: import javafx.css.StyleableProperty; minor: this is only imported for the Javadoc, but not actually used by code; you could consider using a fully qualified name in the javadoc modules/javafx.graphics/src/main/java/com/sun/javafx/css/TransitionDefinition.java line 45: > 43: */ > 44: public record TransitionDefinition(String propertyName, Duration duration, > 45: Duration delay, Interpolator > interpolator) { I see this is an internal class that makes use of `javafx.util.Duration`. I think it's not further exposed. I see a lot of calls getting the duration/delay which almost instantly converts the value to nano seconds. It might be an idea to use nanos here immediately (ie. use `long` in the constructor) and have the converter supply longs. modules/javafx.graphics/src/main/java/com/sun/javafx/css/TransitionDefinition.java line 54: > 52: */ > 53: public TransitionDefinition(String propertyName, Duration duration, > 54: Duration delay, Interpolator > interpolator) { I think you should not repeat the parameters here, just use: Suggestion: public TransitionDefinition { I would also move the "@throws" documentation tags to the record class definition modules/javafx.graphics/src/main/java/com/sun/javafx/css/TransitionDefinitionConverter.java line 77: > 75: } else { > 76: propertyName = "all"; > 77: } minor: could consider: Suggestion: String propertyName = parsedProperty != null ? parsedProperty.convert(null) : "all"; Same for the other variable initializer block below this one. modules/javafx.graphics/src/main/java/com/sun/javafx/css/TransitionDefinitionConverter.java line 111: > 109: Duration[] durations = null; > 110: Duration[] delays = null; > 111: Interpolator[] timingFunctions = null; minor: you could consider initializing these with empty arrays, which would make the checks later in the code simpler; however, if the `convertedValues` can hold `null`s for values, this won't work (I don't think it does though). This would also make initializing the variables short enough to be perhaps be done immediately: Duration delay = delays.length == 0 ? Duration.ZERO : delays[i % delays.length]; modules/javafx.graphics/src/main/java/com/sun/javafx/css/TransitionDefinitionCssMetaData.java line 63: > 61: private static final Duration[] DURATION_ZERO = new Duration[] { > Duration.ZERO }; > 62: > 63: private static final Interpolator[] INTERPOLATOR_EASE = new > Interpolator[] { InterpolatorConverter.CSS_EASE }; Careful, the array contents could still be modified modules/javafx.graphics/src/main/java/com/sun/javafx/css/TransitionDefinitionCssMetaData.java line 89: > 87: } > 88: }, > 89: new CssMetaData<S, Duration[]>( "transition-duration", minor: Inconsistent spacing used for these properties Suggestion: new CssMetaData<S, Duration[]>("transition-duration", modules/javafx.graphics/src/main/java/com/sun/javafx/css/TransitionTimer.java line 49: > 47: private final P property; > 48: private Interpolator interpolator; > 49: private long startTime, endTime, delay, duration; minor: If these are not in the standard unit (seconds), it might be good to clarify the unit used. It seems to be nano seconds. modules/javafx.graphics/src/main/java/com/sun/javafx/css/TransitionTimer.java line 66: > 64: */ > 65: @SuppressWarnings("unchecked") > 66: public static <T, P extends Property<T> & StyleableProperty<T>> > TransitionTimer<T, P> run( minor: this should either be a comment, or a complete javadoc modules/javafx.graphics/src/main/java/com/sun/javafx/css/TransitionTimer.java line 126: > 124: * @param forceStop if {@code true}, the timer is stopped > unconditionally > 125: * @return {@code true} if the timer was cancelled or {@code timer} > is {@code null}, > 126: * {@code false} otherwise minor: Suggestion: * @return {@code true} if the timer was cancelled, or {@code timer} is {@code null}, * otherwise {@code false} modules/javafx.graphics/src/main/java/com/sun/javafx/css/TransitionTimer.java line 157: > 155: */ > 156: private static void adjustReversingTimer(TransitionTimer<?, ?> > existingTimer, TransitionTimer<?, ?> newTimer, > 157: TransitionDefinition > transition, long now) { minor: This should either be a comment or a full javadoc (all tags present) modules/javafx.graphics/src/main/java/com/sun/javafx/css/TransitionTimer.java line 179: > 177: * The elapsed time is computed according to the CSS Transitions > specification. > 178: */ > 179: private static <T, P extends Property<T> & StyleableProperty<T>> > void fireTransitionEvent( minor: incomplete javadoc, change to comment? modules/javafx.graphics/src/main/java/com/sun/javafx/css/TransitionTimer.java line 206: > 204: * Gets the styleable property targeted by this {@code > TransitionTimer}. > 205: */ > 206: public final P getProperty() { minor: incomplete javadoc, missing return tag modules/javafx.graphics/src/main/java/com/sun/javafx/css/TransitionTimer.java line 263: > 261: * when the transition is interrupted by another transition, or when > it ends normally. > 262: */ > 263: public final void stop(EventType<TransitionEvent> eventType) { minor: Incomplete javadoc modules/javafx.graphics/src/main/java/com/sun/javafx/css/TransitionTimer.java line 302: > 300: * After this method is called, the {@link #updating} flag is {@code > false}. > 301: */ > 302: private boolean pollUpdating() { minor: Incomplete javadoc, missing return tag modules/javafx.graphics/src/main/java/com/sun/javafx/css/TransitionTimer.java line 311: > 309: * Gets the progress of this timer along the input progress axis, > ranging from 0 to 1. > 310: */ > 311: private double getProgress() { minor: missing return tag modules/javafx.graphics/src/main/java/com/sun/javafx/css/TransitionTimer.java line 345: > 343: * > 344: * @param timer the other timer > 345: * @return {@code true} if the target values are equal, {@code > false} otherwise minor: I think this is more common: Suggestion: * @return {@code true} if the target values are equal, otherwise {@code false} modules/javafx.graphics/src/main/java/com/sun/scenario/animation/StepInterpolator.java line 73: > 71: } > 72: > 73: if (t >= 0 && step < 0) { `t >= 0` always holds, perhaps the original `t` was meant here? The `t` parameter is re-assigned, which may be confusing. modules/javafx.graphics/src/main/java/com/sun/scenario/animation/StepInterpolator.java line 83: > 81: }; > 82: > 83: if (t <= 1 && step > jumps) { `t <= 1` is always true, perhaps the original `t` was intended here? See other comment. modules/javafx.graphics/src/main/java/com/sun/scenario/animation/StepInterpolator.java line 97: > 95: @Override > 96: public boolean equals(Object obj) { > 97: return obj instanceof StepInterpolator other minor: Perhaps make `StepInterpolator` `final` modules/javafx.graphics/src/main/java/javafx/animation/Interpolator.java line 305: > 303: * @since 22 > 304: */ > 305: public static Interpolator STEP_START = STEPS(1, StepPosition.START); Suggestion: public static final Interpolator STEP_START = STEPS(1, StepPosition.START); modules/javafx.graphics/src/main/java/javafx/animation/Interpolator.java line 312: > 310: * @since 22 > 311: */ > 312: public static Interpolator STEP_END = STEPS(1, StepPosition.END); Suggestion: public static final Interpolator STEP_END = STEPS(1, StepPosition.END); modules/javafx.graphics/src/main/java/javafx/animation/Interpolator.java line 319: > 317: * The output time value is determined by the {@link StepPosition}. > 318: * > 319: * @param intervals the number of intervals in the step interpolator minor: When I see a plural like `intervals` (or `employees`) I think of a list of objects. Perhaps `intervalCount` would be better? modules/javafx.graphics/src/main/java/javafx/animation/Interpolator.java line 325: > 323: * @since 22 > 324: */ > 325: public static Interpolator STEPS(int intervals, StepPosition > position) { This doesn't specify what happens when `position` is `null`, and is also missing the restrictions on `intervals`. modules/javafx.graphics/src/main/java/javafx/css/CssParser.java line 678: > 676: LOGGER.finest("Expected \'<duration>\'"); > 677: } > 678: ParseException re = new ParseException("Expected > \'<duration>\'",token, this); Suggestion: ParseException re = new ParseException("Expected '<duration>'", token, this); modules/javafx.graphics/src/main/java/javafx/css/CssParser.java line 1022: > 1020: default -> new ParsedValueImpl<>(key, null, true); > 1021: }; > 1022: } minor: No need for `else` (saves nesting) modules/javafx.graphics/src/main/java/javafx/css/CssParser.java line 3949: > 3947: > 3948: private ParsedValueImpl<ParsedValue[], TransitionDefinition> > parseTransition(Term term) > 3949: throws ParseException{ Suggestion: throws ParseException { modules/javafx.graphics/src/main/java/javafx/css/CssParser.java line 3958: > 3956: if (term == null) { > 3957: break; > 3958: } else if (isEasingFunction(term.token)) { minor: No need for `else` Suggestion: } if (isEasingFunction(term.token)) { modules/javafx.graphics/src/main/java/javafx/css/StyleableBooleanProperty.java line 80: > 78: setValue(v); > 79: } > 80: I'm not sure how I feel about this; the changes made don't really seem to belong here. The `StyleableXXXProperty` classes are convenient helpers, but all that matters is that they are `StyleableProperty` implementations. There is no requirement to use the helpers. So what happens when a property defines their own helper, or implements `StyleableProperty` directly? Or if classes are refactored at some point and they decide to stop using these helpers? modules/javafx.graphics/src/main/java/javafx/css/TransitionEvent.java line 43: > 41: public final class TransitionEvent extends Event { > 42: > 43: private static final long serialVersionUID = 20220820L; minor: may I suggest `1L` as in version 1? It's not an existing class that needs to be compatible with older implementations for which a version was derived by the compiler. modules/javafx.graphics/src/main/java/javafx/css/TransitionEvent.java line 88: > 86: * @param eventType the event type > 87: * @param property the {@code StyleableProperty} that is targeted by > the transition > 88: * @param elapsedTime the time that has elapsed since the transition > has entered its active period Can property be `null`? Any restrictions on `elapsedTime`? modules/javafx.graphics/src/main/java/javafx/css/TransitionEvent.java line 111: > 109: * not including the time spent in the delay phase. > 110: * > 111: * @return the elapsed time Any guarantees here? Can it be `null`, negative, zero, infinite? modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 8937: > 8935: * a running {@link TransitionTimer} with this {@code Node}. This > allows the node > 8936: * to keep track of running timers that are targeting its > properties. > 8937: */ minor: incomplete javadoc modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 8951: > 8949: * when their {@link TransitionTimer} has completed. > 8950: */ > 8951: private void removeTransitionTimer(TransitionTimer<?, ?> timer) { minor: incomplete javadoc modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 8960: > 8958: * Finds the transition timer that targets the specified {@code > property}. > 8959: * > 8960: * @return the transition timer, or {@code null} minor: missing @param tag modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 8968: > 8966: > 8967: for (TransitionTimer<?, ?> timer : transitionTimers) { > 8968: if (timer.getProperty() == property) { minor: this probably works, but I'd still use `equals` here ------------- PR Review: https://git.openjdk.org/jfx/pull/870#pullrequestreview-1554590248 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1279184581 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1279185366 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1279185442 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1279185777 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1279195463 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1279235400 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1279197467 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1279199846 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1279204340 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1279208647 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1279209361 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1279217008 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1279222846 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1279225319 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1279226794 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1279236379 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1279237951 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1279241079 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1279242702 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1279249222 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1279251667 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1279256926 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1279258025 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1279263450 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1279266478 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1279266767 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1279271364 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1279268169 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1279273159 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1279275071 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1279276790 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1279277822 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1279295992 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1279300672 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1279303855 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1279302999 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1279308190 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1279308636 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1279309244 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1279310798
![https://user-images.githubusercontent.com/43553916/184781143-0520fbfe-54bf-4b8d-93ac-834708e46500.gif"](https://user-images.githubusercontent.com/43553916/184781143-0520fbfe-54bf-4b8d-93ac-834708e46500.gif"){kind=link}