RFR: 8311895: CSS Transitions [v2]
John Hendrikx
jhendrikx at openjdk.org
Mon Jul 31 13:47:17 UTC 2023
On Mon, 31 Jul 2023 00:09:38 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:
>
> 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
More information about the openjfx-dev
mailing list