On Fri, 24 May 2024 11:18:35 GMT, Michael Strauß <mstra...@openjdk.org> wrote:
>> Implementation of [CSS >> Transitions](https://gist.github.com/mstr2/c72f8c9faa87de14926978f517a6018a). >> >> ### Future enhancements >> CSS transitions requires all participating objects to implement the >> `Interpolatable` interface. For example, targeting `-fx-background-color` >> only works if all background-related objects are interpolatable: `Color`, >> `BackgroundFill`, and `Background`. >> >> In a follow-up PR, the following types will implement the `Interpolatable` >> interface: >> `LinearGradient`, `RadialGradient`, `Stop`, `Background`, `BackgroundFill`, >> `BackgroundImage`, `BackgroundPosition`, `BackgroundSize`, >> `BackgroundStroke`, `BorderWidths`, `CornerRadii`, `Insets`. >> >> ### 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 with a new target base due to a > merge or a rebase. The pull request now contains 57 commits: > > - Merge branch 'refs/heads/master' into feature/css-transitions > - extract magic string to named constant > - use existing property in test > - fixed documentation > - Merge branch 'master' into feature/css-transitions > - update 'since' tags > - Fix javadoc error > - Change javadoc comment > - Merge branch 'master' into feature/css-transitions > - Discard redundant transitions in StyleableProperty impls > - ... and 47 more: https://git.openjdk.org/jfx/compare/94aa2b68...a43dee30 I did a quick review of some of the code, mostly the API. I still don't know what happens when a value is programmatically set while a css transition is in progress. What I understood is that binding the property will not allow the transition to start/continue, but didn't see where setting a value was mentioned. Otherwise looks good. I don't intend to review this beyond my current comments, so you can integrate once resolved. modules/javafx.graphics/src/main/docs/javafx/scene/doc-files/cssref.html line 688: > 686: <h3><a id="introtransitions">Transitions</a></h3> > 687: <p>JavaFX supports <em>implicit transitions</em> for properties that > are styled by CSS. When a property value is > 688: changed, it smoothly transitions to the new value over a period > of time. Implicit transitions are supported Maybe not so smoothly when using a step interpolator? modules/javafx.graphics/src/main/docs/javafx/scene/doc-files/cssref.html line 690: > 688: changed, it smoothly transitions to the new value over a period > of time. Implicit transitions are supported > 689: for all primitive types, as well as for types that implement > <code>javafx.animation.Interpolatable</code>.</p> > 690: <p>Transitions can be defined for any node in the JavaFX scene graph > with the following properties:</p> The way this is phrased makes it sound like the node has "the following properties", not the transition. Maybe move that part: "Transitions with the following properties can be defined for any node in the JavaFX scene graph", or just add a comma. modules/javafx.graphics/src/main/java/com/sun/javafx/css/TransitionDefinition.java line 40: > 38: * @param duration duration of the transition > 39: * @param delay delay after which the transition is started; if negative, > the transition starts > 40: * immediately, but will appear to have begun at an earlier > point in time Why accept a negative delay? An [`Animation`](https://openjfx.io/javadoc/22/javafx.graphics/javafx/animation/Animation.html#getDelay()) doesn't accept it. modules/javafx.graphics/src/main/java/javafx/animation/Interpolator.java line 277: > 275: * @since 23 > 276: */ > 277: public enum StepPosition { I think it would be helpful to include (or link to) images that show what the steps for each option looks like. The verbal description is a bit technical. modules/javafx.graphics/src/main/java/javafx/animation/Interpolator.java line 328: > 326: * @since 23 > 327: */ > 328: public static Interpolator STEPS(int intervals, StepPosition > position) { Static method names shouldn't be named like constants, although `Interpolator` does this for other methods already. Not sure if this trend should continue. ------------- PR Review: https://git.openjdk.org/jfx/pull/870#pullrequestreview-2078912077 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1614833351 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1614836286 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1614815486 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1614554508 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1614789417
