Re: Issue in color setting
You're setting a new value in the change listener of the iconColor
property, which then fires off another change notification, and so on.
I guess that you're not getting a new object if you use one of the
predefined constants like "red", which breaks the cycle (as there's no
change notification when the object is the same).
So regardless of whether this worked in JFX22, your code seems wrong.
On Sun, Oct 6, 2024 at 7:26 PM Clemens Lanthaler
wrote:
>
> Hi everyone,
>
> I have just moved over my application (Photoslide) from JFX22 to JFX23 and I
> am facing the folling issue:
>
> For any FontIcon (iKonli) where I set the color in web form I am getting the
> follwing exception:
>
> Exception in thread "JavaFX Application Thread" java.lang.StackOverflowError
> at java.base/java.lang.String.substring(String.java:2930)
> at java.base/java.lang.String.substring(String.java:2898)
> at javafx.graphics@24-ea/javafx.scene.paint.Color.web(Color.java:403)
> at javafx.graphics@24-ea/javafx.scene.paint.Color.web(Color.java:646)
> at javafx.graphics@24-ea/javafx.scene.paint.Color.valueOf(Color.java:667)
> at javafx.graphics@24-ea/javafx.scene.paint.Paint.valueOf(Paint.java:124)
> at
> org.photoslide.MainViewController.lambda$initialize$1(MainViewController.java:266)
>
> The corresponding code I am using and which is working with JFX22:
>
> processListIcon.iconColorProperty().addListener((o) -> {
> if (!taskProgressView.getTasks().isEmpty()) {
> processListIcon.setIconColor(Paint.valueOf("lightgreen"));
> } else {
> processListIcon.setIconColor(Paint.valueOf("#c5c5c5"));
> }
> });
> Changing "#c5c5c5" to e.g. "red" than all is fine. Also using the RGB values
> is resulting in an Exception (Color paint = new Color(0.7725, 0.7725, 0.7725,
> 1.0);)
>
>
> This issue is also present in JFX24ea and I have searched if iKonli has an
> open issue. It seems that there is an issue.
>
> cheers,
> Clemens
>
>
> --
> ITArchitects
> CEO: B.Sc. Clemens Lanthaler
> Forchachstrasse 3
> A-6166 Fulpmes
> Tel.: +43 (0)650 855 2954
> email: off...@itarchitects.at
> homepage: http://www.itarchitects.at
> -
> Notice: This e-mail and any attachments are confidential and may be
> privileged.
> If you are not the intended recipient, notify the sender immediately, destroy
> all
> copies from your system and do not disclose or use the information for any
> purpose.
> Diese E-Mail inklusive aller Anhaenge ist vertraulich und koennte
> bevorrechtigtem
> Schutz unterliegen. Wenn Sie nicht der beabsichtigte Adressat sind,
> informieren Sie
> bitte den Absender unverzueglich, loeschen Sie alle Kopien von Ihrem System
> und
> veroeffentlichen Sie oder nutzen Sie die Information keinesfalls, gleich zu
> welchem Zweck.
Re: RFR: 8341514: Add reducedMotion and reducedTransparency preferences [v2]
> This PR adds the `Platform.Preferences.reducedMotion` and
> `Platform.Preferences.reducedTransparency` accessibility preferences:
>
> interface Preferences {
> /**
> * Specifies whether applications should minimize the amount of
> non-essential animations,
> * reducing discomfort for users who experience motion sickness or
> vertigo.
> *
> * If the platform does not report this preference, this property
> defaults to {@code false}.
> *
> * @return the {@code reducedMotion} property
> * @defaultValue {@code false}
> * @since 24
> */
> ReadOnlyBooleanProperty reducedMotionProperty();
>
> /**
> * Specifies whether applications should minimize the amount of
> transparent or translucent
> * layer effects, which can help to increase contrast and readability for
> some users.
> *
> * If the platform does not report this preference, this property
> defaults to {@code false}.
> *
> * @return the {@code reducedTransparency} property
> * @defaultValue {@code false}
> * @since 24
> */
> ReadOnlyBooleanProperty reducedTransparencyProperty();
>
> ...
> }
>
>
> On Windows, these preferences correspond to "Transparency effects" and
> "Animation effects" in the system settings:
> src="https://github.com/user-attachments/assets/b286850d-ebbc-444c-9c09-5380f97d0e91";
> width="400" >
>
> On macOS, these preferences correspond to "Reduce motion" and "Reduce
> transparency" in the system settings:
> src="https://github.com/user-attachments/assets/4c7b92f8-0cd9-4d1f-91df-40cb449f91ff";
> width="400" >
>
> On Linux, the `reducedMotion` preference corresponds to
> `gtk-enable-animations`. On my Ubuntu 24.04 system, GTK3 does not pick up the
> "Settings / Accessibility / Reduce Animations" system setting, and therefore
> GTK always reports `gtk-enable-animations == true`. Changing the preference
> requires manually editing `etc/gtk-3.0/settings.ini`. The
> `reducedTransparency` preference has no corresponding GTK setting.
Michael Strauß has updated the pull request incrementally with one additional
commit since the last revision:
small changes
-
Changes:
- all: https://git.openjdk.org/jfx/pull/1592/files
- new: https://git.openjdk.org/jfx/pull/1592/files/ef95a9f7..c144a794
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jfx&pr=1592&range=01
- incr: https://webrevs.openjdk.org/?repo=jfx&pr=1592&range=00-01
Stats: 8 lines in 5 files changed: 0 ins; 2 del; 6 mod
Patch: https://git.openjdk.org/jfx/pull/1592.diff
Fetch: git fetch https://git.openjdk.org/jfx.git pull/1592/head:pull/1592
PR: https://git.openjdk.org/jfx/pull/1592
Re: RFR: 8341418: Prism/es2 DrawableInfo is never freed (leak) [v2]
On Sun, 6 Oct 2024 19:12:42 GMT, Thiago Milczarek Sayao
wrote:
>> modules/javafx.graphics/src/main/java/com/sun/prism/es2/ES2SwapChain.java
>> line 193:
>>
>>> 191: @Override
>>> 192: public ES2Graphics createGraphics() {
>>> 193: if (drawable == null || drawable.getNativeWindow() !=
>>> pState.getNativeWindow()) {
>>
>> The null check wasn't here before, and it doesn't seem necessary as
>> `createGLDrawable()` does not return `null`.
>
> But drawable can now be disposed and set to null. If that happens and
> `createGraphics` is called again it will end in a NPE.
The NPE would be a good thing then, as no method should be called on a disposed
object (except for `dispose`).
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1586#discussion_r1789427891
Re: RFR: 8341418: Prism/es2 DrawableInfo is never freed (leak)
On Tue, 1 Oct 2024 17:37:15 GMT, Thiago Milczarek Sayao
wrote:
> When creating a Scene, a `DrawableInfo` is allocated with `malloc`. When
> scene changes, this is called on `WindowStage.java`:
>
> `QuantumRenderer.getInstance().disposePresentable(painter.presentable); //
> latched on RT`
>
> But the underlying `DrawableInfo` is never freed.
>
> I also think this should be done when the Stage is closed.
>
> To test:
>
> import javafx.animation.Animation;
> import javafx.animation.KeyFrame;
> import javafx.animation.KeyValue;
> import javafx.animation.Timeline;
> import javafx.application.Application;
> import javafx.scene.Scene;
> import javafx.scene.control.TextField;
> import javafx.scene.control.Label;
> import javafx.scene.layout.Pane;
> import javafx.scene.layout.StackPane;
> import javafx.scene.layout.VBox;
> import javafx.scene.paint.Color;
> import javafx.stage.Stage;
> import javafx.util.Duration;
>
> public class TestScenes extends Application {
>
> @Override
> public void start(Stage stage) {
> Timeline timeline = new Timeline(
> new KeyFrame(Duration.millis(100), e ->
> stage.setScene(createScene("Scene 1", Color.RED))),
> new KeyFrame(Duration.millis(200), e ->
> stage.setScene(createScene("Scene 2", Color.BLUE))),
> new KeyFrame(Duration.millis(300), e ->
> stage.setScene(createScene("Scene 3", Color.GREEN)))
> );
>
> timeline.setCycleCount(Animation.INDEFINITE);
> timeline.play();
>
> stage.show();
> }
>
> private Scene createScene(String text, Color color) {
> return new Scene(new StackPane(), 400, 300, color);
> }
>
> public static void main(String[] args) {
> launch(TestScenes.class, args);
> }
> }
The fix works on Windows and macOS. I've left some additional comments.
modules/javafx.graphics/src/main/java/com/sun/prism/es2/ES2SwapChain.java line
193:
> 191: @Override
> 192: public ES2Graphics createGraphics() {
> 193: if (drawable == null || drawable.getNativeWindow() !=
> pState.getNativeWindow()) {
The null check wasn't here before, and it doesn't seem necessary as
`createGLDrawable()` does not return `null`.
modules/javafx.graphics/src/main/java/com/sun/prism/es2/GLDrawable.java line 56:
> 54: abstract boolean swapBuffers(GLContext glCtx);
> 55:
> 56: abstract void dispose();
`GLDrawable` could also get the dispose method by implementing
`GraphicsResource`.
modules/javafx.graphics/src/main/java/com/sun/prism/es2/IOSGLDrawable.java line
57:
> 55: @Override
> 56: void dispose() {
> 57: nDestroyDrawable(getNativeDrawableInfo());
You should set `nativeDrawableInfo` to zero in this method (and check this
before calling the native method), as otherwise we run the risk of freeing a
stale pointer when the `dispose()` method is called repeatedly. The same should
be done in all other implementations.
modules/javafx.graphics/src/main/native-prism-es2/GLDrawable.c line 44:
> 42: }
> 43:
> 44: void deleteDrawableInfo(DrawableInfo *dInfo)
Maybe delete this whole file, as its only content is now a function that just
calls `memset()`.
-
PR Review: https://git.openjdk.org/jfx/pull/1586#pullrequestreview-2350517717
PR Review Comment: https://git.openjdk.org/jfx/pull/1586#discussion_r1789053938
PR Review Comment: https://git.openjdk.org/jfx/pull/1586#discussion_r1789054227
PR Review Comment: https://git.openjdk.org/jfx/pull/1586#discussion_r1789054946
PR Review Comment: https://git.openjdk.org/jfx/pull/1586#discussion_r1789055920
Re: RFR: 8333374: Cannot invoke "com.sun.prism.RTTexture.contentsUseful()" because "this.txt" is null
On Thu, 3 Oct 2024 09:35:51 GMT, Lukasz Kostyra wrote:
> Contrary to issue description, the problem was not because we referenced a
> texture after it was freed, but we referenced an RTT that we just tried to
> allocate and failed to do so because of lack of space in Prism's vram pool.
> In case of attached `WebViewAnimationTest.java` test app, the problem exists
> because WebView tries to allocate a new RTT for newly opened WebView scene
> and the Prism pool does not have 16MB needed while previous WebView is still
> being displayed. Only after the old WebView is disposed of, then the RTT
> allocation can proceed and WebView can continue as normal.
>
> I looked around and did not find other solution than adding the null-checks
> to silence the NPE being thrown. NPE was originally in `RTImage.java`,
> however after adding a null check there triggered another NPE in
> `WCGraphicsPrismContext.java`, so I also added another null check there.
> Upper layers of web module seem to handle those just fine, the NPEs
> disappeared after that and the test still works properly once the pool gets
> enough room to display.
>
> All of our tests run fine with this change (I do not expect any major
> regressions, as this happens only with a very limited memory pool scenario -
> it was extremely hard and time consuming to trigger this bug on the test app
> with default 512MB pool limit). For the test app, it might take a couple
> frames until the old WebView is disposed of and there is enough room for new
> WebView's RTT in the pool, after that the scene renders properly.
modules/javafx.web/src/main/java/com/sun/javafx/webkit/prism/RTImage.java line
120:
> 118: if (txt == null) {
> 119: log.fine("RTImage::getTexture : return null because rt
> texture not allocated");
> 120: return null;
If `RTImage.getTexture()` can return `null`, then L147 should probably also
check for `null` before calling `readPixels()`.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1590#discussion_r1788512967
RFR: 8341514: Add reducedMotion and reducedTransparency preferences
This PR adds the `Platform.Preferences.reducedMotion` and
`Platform.Preferences.reducedTransparency` accessibility preferences:
interface Preferences {
/**
* Specifies whether applications should minimize the amount of
non-essential animations,
* reducing discomfort for users who experience motion sickness or vertigo.
*
* If the platform does not report this preference, this property defaults
to {@code false}.
*
* @return the {@code reducedMotion} property
* @defaultValue {@code false}
* @since 24
*/
ReadOnlyBooleanProperty reducedMotionProperty();
/**
* Specifies whether applications should minimize the amount of transparent
or translucent
* layer effects, which can help to increase contrast and readability for
some users.
*
* If the platform does not report this preference, this property defaults
to {@code false}.
*
* @return the {@code reducedTransparency} property
* @defaultValue {@code false}
* @since 24
*/
ReadOnlyBooleanProperty reducedTransparencyProperty();
...
}
On Windows, these preferences correspond to "Transparency effects" and
"Animation effects" in the system settings:
https://github.com/user-attachments/assets/b286850d-ebbc-444c-9c09-5380f97d0e91";
width="400" >
On macOS, these preferences correspond to "Reduce motion" and "Reduce
transparency" in the system settings:
https://github.com/user-attachments/assets/4c7b92f8-0cd9-4d1f-91df-40cb449f91ff";
width="400" >
On Linux, the `reducedMotion` preference corresponds to
`gtk-enable-animations`. On my Ubuntu 24.04 system, GTK3 does not pick up the
"Settings / Accessibility / Reduce Animations" system setting, and therefore
GTK always reports `gtk-enable-animations == true`. Changing the preference
requires manually editing `etc/gtk-3.0/settings.ini`. The `reducedTransparency`
preference has no corresponding GTK setting.
-
Commit messages:
- change javadoc
- added reducedMotion and reducedTransparency
Changes: https://git.openjdk.org/jfx/pull/1592/files
Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1592&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8341514
Stats: 403 lines in 19 files changed: 325 ins; 3 del; 75 mod
Patch: https://git.openjdk.org/jfx/pull/1592.diff
Fetch: git fetch https://git.openjdk.org/jfx.git pull/1592/head:pull/1592
PR: https://git.openjdk.org/jfx/pull/1592
Re: RFR: 8341164: Update boot JDK to 23
On Wed, 2 Oct 2024 17:09:59 GMT, Ambarish Rapte wrote:
> Update the boot JDK to version 23.
>
> Since Gradle 8.9 does not support JDK 23, it is necessary to upgrade Gradle
> to the latest release, version 8.10.2, which supports JDK 23.
> It is advised to upgrade directly to Gradle 8.10.2 rather than 8.10. refer to
> [the release notes](https://docs.gradle.org/8.10.2/release-notes.html)
>
> No modifications to the Gradle build script are required for upgrading the
> Gradle version.
Running `gradle wrapper --gradle-version 8.10.2
--gradle-distribution-sha256-sum
31c55713e40233a8303827ceb42ca48a47267a0ad4bab9177123121e71524c26` on my Windows
machine changes `gradlew` and `gradlew.bat` as follows:
diff --git a/gradlew b/gradlew
--- a/gradlew (revision d5432c3e14b06445bc45e34e4aa63ec415c03595)
+++ b/gradlew (date 1727936286998)
@@ -15,8 +15,6 @@
# See the License for the specific language governing permissions and
# limitations under the License.
#
-# SPDX-License-Identifier: Apache-2.0
-#
##
#
@@ -57,7 +55,7 @@
# Darwin, MinGW, and NonStop.
#
# (3) This script is generated from the Groovy template
-#
https://github.com/gradle/gradle/blob/HEAD/platforms/jvm/plugins-application/src/main/resources/org/gradle/api/internal/plugins/unixStartScript.txt
+#
https://github.com/gradle/gradle/blob/HEAD/subprojects/plugins/src/main/resources/org/gradle/api/internal/plugins/unixStartScript.txt
# within the Gradle project.
#
# You can find Gradle at https://github.com/gradle/gradle/.
@@ -86,8 +84,7 @@
# shellcheck disable=SC2034
APP_BASE_NAME=${0##*/}
# Discard cd standard output in case $CDPATH is set
(https://github.com/gradle/gradle/issues/25036)
-APP_HOME=$( cd -P "${APP_HOME:-./}" > /dev/null && printf '%s
-' "$PWD" ) || exit
+APP_HOME=$( cd "${APP_HOME:-./}" > /dev/null && pwd -P ) || exit
# Use the maximum available, or set MAX_FD != -1 to use that value.
MAX_FD=maximum
diff --git a/gradlew.bat b/gradlew.bat
--- a/gradlew.bat (revision d5432c3e14b06445bc45e34e4aa63ec415c03595)
+++ b/gradlew.bat (date 1727936287168)
@@ -13,8 +13,6 @@
@rem See the License for the specific language governing permissions and
@rem limitations under the License.
@rem
-@rem SPDX-License-Identifier: Apache-2.0
-@rem
@if "%DEBUG%"=="" @echo off
@rem ##
-
PR Comment: https://git.openjdk.org/jfx/pull/1589#issuecomment-2390617421
Re: RFR: 8341164: Update boot JDK to 23
On Wed, 2 Oct 2024 17:09:59 GMT, Ambarish Rapte wrote: > Update the boot JDK to version 23. > > Since Gradle 8.9 does not support JDK 23, it is necessary to upgrade Gradle > to the latest release, version 8.10.2, which supports JDK 23. > It is advised to upgrade directly to Gradle 8.10.2 rather than 8.10. refer to > [the release notes](https://docs.gradle.org/8.10.2/release-notes.html) > > No modifications to the Gradle build script are required for upgrading the > Gradle version. Gradle should be upgraded with the `wrapper` task (see [Upgrading your build from Gradle 8.x to the latest](https://docs.gradle.org/current/userguide/upgrading_version_8.html)): gradle wrapper --gradle-version 8.10.2 Doing this will ensure that `gradlew` and `gradlew.bat` are up-to-date with the latest version, which is not the case for this PR. - PR Comment: https://git.openjdk.org/jfx/pull/1589#issuecomment-2389461416
Re: RFR: 8341372: BackgroundPosition, BorderImage, BorderStroke, CornerRadii should be final [v2]
On Wed, 2 Oct 2024 17:27:43 GMT, Andy Goryachev wrote: >> All fields of `BackgroundPosition` are already final. Same for `BorderImage` >> and `BorderStroke`. > > sorry, I meant make them `private` Yes, that's a sensible change. - PR Review Comment: https://git.openjdk.org/jfx/pull/1587#discussion_r1784965466
Re: RFR: 8341372: BackgroundPosition, BorderImage, BorderStroke, CornerRadii should be final [v2]
> Backgrounds and borders are comprised of immutable and final types. The > following types are marked with the `final` modifier: > > * Background > * BackgroundFill > * BackgroundImage > * BackgroundSize > * Border > * BorderWidths > > The following types are not marked with the `final` modifier: > > * BackgroundPosition > * BorderImage > * BorderStroke > * CornerRadii > > This is probably just an oversight, as there is no value in allowing a > subsection of these types to be extended. Michael Strauß has updated the pull request incrementally with one additional commit since the last revision: make fields private - Changes: - all: https://git.openjdk.org/jfx/pull/1587/files - new: https://git.openjdk.org/jfx/pull/1587/files/13025a0c..659d7c93 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1587&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1587&range=00-01 Stats: 76 lines in 11 files changed: 0 ins; 1 del; 75 mod Patch: https://git.openjdk.org/jfx/pull/1587.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1587/head:pull/1587 PR: https://git.openjdk.org/jfx/pull/1587
Re: RFR: 8341372: BackgroundPosition, BorderImage, BorderStroke, CornerRadii should be final
On Wed, 2 Oct 2024 14:43:33 GMT, Andy Goryachev wrote:
>> Backgrounds and borders are comprised of immutable and final types. The
>> following types are marked with the `final` modifier:
>>
>> * Background
>> * BackgroundFill
>> * BackgroundImage
>> * BackgroundSize
>> * Border
>> * BorderWidths
>>
>> The following types are not marked with the `final` modifier:
>>
>> * BackgroundPosition
>> * BorderImage
>> * BorderStroke
>> * CornerRadii
>>
>> This is probably just an oversight, as there is no value in allowing a
>> subsection of these types to be extended.
>
> modules/javafx.graphics/src/main/java/javafx/scene/layout/BackgroundPosition.java
> line 55:
>
>> 53: * @since JavaFX 8.0
>> 54: */
>> 55: public final class BackgroundPosition implements
>> Interpolatable {
>
> should we also declare the fields `final` as well?
> horizontalSide, etc.
All fields of `BackgroundPosition` are already final. Same for `BorderImage`
and `BorderStroke`.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1587#discussion_r1784937266
Re: RFR: 8341372: BackgroundPosition, BorderImage, BorderStroke, CornerRadii should be final
On Tue, 1 Oct 2024 22:29:39 GMT, Andy Goryachev wrote: > There might be a very narrow use case (like debugging) when developer might > need to extend to supply a custom `toString()` or some other method. > > From a high level design standpoint, I do agree with making these classes > final. Maybe, but that's not a sufficient reason to allow subclassing, as the same could be said for all cases of encapsuluation and inheritance control. Alternatives are: 1) Use the debugger to mark objects 2) Use a tool to change method definitions at runtime 3) Download OpenJFX and build it locally with the required changes - PR Comment: https://git.openjdk.org/jfx/pull/1587#issuecomment-2387234029
RFR: 8341372: BackgroundPosition, BorderImage, BorderStroke, CornerRadii should be final
Backgrounds and borders are comprised of immutable and final types. The following types are marked with the `final` modifier: * Background * BackgroundFill * BackgroundImage * BackgroundSize * Border * BorderWidths The following types are not marked with the `final` modifier: * BackgroundPosition * BorderImage * BorderStroke * CornerRadii This is probably just an oversight, as there is no value in allowing a subsection of these types to be extended. - Commit messages: - Make BackgroundPosition, BorderImage, BorderStroke, CornerRadii final Changes: https://git.openjdk.org/jfx/pull/1587/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1587&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8341372 Stats: 12 lines in 4 files changed: 0 ins; 0 del; 12 mod Patch: https://git.openjdk.org/jfx/pull/1587.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1587/head:pull/1587 PR: https://git.openjdk.org/jfx/pull/1587
Integrated: 8332895: Support interpolation for backgrounds and borders
On Tue, 30 Jul 2024 16:03:46 GMT, Michael Strauß wrote:
> This PR completes the CSS Transitions story (see #870) by adding
> interpolation support for backgrounds and borders, making them targetable by
> transitions.
>
> `Background` and `Border` objects are deeply immutable, but not
> interpolatable. Consider the following `Background`, which describes the
> background of a `Region`:
>
>
> Background {
> fills = [
> BackgroundFill {
> fill = Color.RED
> }
> ]
> }
>
>
> Since backgrounds are deeply immutable, changing the region's background to
> another color requires the construction of a new `Background`, containing a
> new `BackgroundFill`, containing the new `Color`.
>
> Animating the background color using a CSS transition therefore requires the
> entire Background object graph to be interpolatable in order to generate
> intermediate backgrounds.
>
> More specifically, the following types will now implement `Interpolatable`.
>
> - `Insets`
> - `Background`
> - `BackgroundFill`
> - `BackgroundImage`
> - `BackgroundPosition`
> - `BackgroundSize`
> - `Border`
> - `BorderImage`
> - `BorderStroke`
> - `BorderWidths`
> - `CornerRadii`
> - `Stop`
> - `Paint` and all of its subclasses
> - `Margins` (internal type)
> - `BorderImageSlices` (internal type)
>
> ## Interpolation of composite objects
>
> As of now, only `Color`, `Point2D`, and `Point3D` are interpolatable. Each of
> these classes is an aggregate of `double` values, which are combined using
> linear interpolation. However, many of the new interpolatable classes
> comprise of not only `double` values, but a whole range of other types. This
> requires us to more precisely define what we mean by "interpolation".
>
> Mirroring the CSS specification, the `Interpolatable` interface defines
> several types of component interpolation:
>
> | Interpolation type | Description |
> |---|---|
> | default | Component types that implement `Interpolatable` are interpolated
> by calling the `interpolate(Object, double)}` method. |
> | linear | Two components are combined by linear interpolation such that `t =
> 0` produces the start value, and `t = 1` produces the end value. This
> interpolation type is usually applicable for numeric components. |
> | discrete | If two components cannot be meaningfully combined, the
> intermediate component value is equal to the start value for `t < 0.5` and
> equal to the end value for `t >= 0.5`. |
> | pairwise | Two lists are combined by pairwise interpolation. If the start
> list has fewer elements than the target list, the missing elements are copied
> from the target list. If the start list has more elements than the ...
This pull request has now been integrated.
Changeset: 01e9e7ea
Author:Michael Strauß
URL:
https://git.openjdk.org/jfx/commit/01e9e7eadb21aabc801d4764ed5bd5e3de8d451b
Stats: 6718 lines in 77 files changed: 5851 ins; 371 del; 496 mod
8332895: Support interpolation for backgrounds and borders
8226911: Interpolatable's contract should be reexamined
Reviewed-by: angorya, jhendrikx, nlisker, kcr
-
PR: https://git.openjdk.org/jfx/pull/1522
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v43]
> This PR completes the CSS Transitions story (see #870) by adding
> interpolation support for backgrounds and borders, making them targetable by
> transitions.
>
> `Background` and `Border` objects are deeply immutable, but not
> interpolatable. Consider the following `Background`, which describes the
> background of a `Region`:
>
>
> Background {
> fills = [
> BackgroundFill {
> fill = Color.RED
> }
> ]
> }
>
>
> Since backgrounds are deeply immutable, changing the region's background to
> another color requires the construction of a new `Background`, containing a
> new `BackgroundFill`, containing the new `Color`.
>
> Animating the background color using a CSS transition therefore requires the
> entire Background object graph to be interpolatable in order to generate
> intermediate backgrounds.
>
> More specifically, the following types will now implement `Interpolatable`.
>
> - `Insets`
> - `Background`
> - `BackgroundFill`
> - `BackgroundImage`
> - `BackgroundPosition`
> - `BackgroundSize`
> - `Border`
> - `BorderImage`
> - `BorderStroke`
> - `BorderWidths`
> - `CornerRadii`
> - `Stop`
> - `Paint` and all of its subclasses
> - `Margins` (internal type)
> - `BorderImageSlices` (internal type)
>
> ## Interpolation of composite objects
>
> As of now, only `Color`, `Point2D`, and `Point3D` are interpolatable. Each of
> these classes is an aggregate of `double` values, which are combined using
> linear interpolation. However, many of the new interpolatable classes
> comprise of not only `double` values, but a whole range of other types. This
> requires us to more precisely define what we mean by "interpolation".
>
> Mirroring the CSS specification, the `Interpolatable` interface defines
> several types of component interpolation:
>
> | Interpolation type | Description |
> |---|---|
> | default | Component types that implement `Interpolatable` are interpolated
> by calling the `interpolate(Object, double)}` method. |
> | linear | Two components are combined by linear interpolation such that `t =
> 0` produces the start value, and `t = 1` produces the end value. This
> interpolation type is usually applicable for numeric components. |
> | discrete | If two components cannot be meaningfully combined, the
> intermediate component value is equal to the start value for `t < 0.5` and
> equal to the end value for `t >= 0.5`. |
> | pairwise | Two lists are combined by pairwise interpolation. If the start
> list has fewer elements than the target list, the missing elements are copied
> from the target list. If the start list has more elements than the ...
Michael Strauß has updated the pull request incrementally with one additional
commit since the last revision:
added @deprecated javadoc tag
-
Changes:
- all: https://git.openjdk.org/jfx/pull/1522/files
- new: https://git.openjdk.org/jfx/pull/1522/files/7d086cf6..332a5c1e
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jfx&pr=1522&range=42
- incr: https://webrevs.openjdk.org/?repo=jfx&pr=1522&range=41-42
Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jfx/pull/1522.diff
Fetch: git fetch https://git.openjdk.org/jfx.git pull/1522/head:pull/1522
PR: https://git.openjdk.org/jfx/pull/1522
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v41]
> This PR completes the CSS Transitions story (see #870) by adding
> interpolation support for backgrounds and borders, making them targetable by
> transitions.
>
> `Background` and `Border` objects are deeply immutable, but not
> interpolatable. Consider the following `Background`, which describes the
> background of a `Region`:
>
>
> Background {
> fills = [
> BackgroundFill {
> fill = Color.RED
> }
> ]
> }
>
>
> Since backgrounds are deeply immutable, changing the region's background to
> another color requires the construction of a new `Background`, containing a
> new `BackgroundFill`, containing the new `Color`.
>
> Animating the background color using a CSS transition therefore requires the
> entire Background object graph to be interpolatable in order to generate
> intermediate backgrounds.
>
> More specifically, the following types will now implement `Interpolatable`.
>
> - `Insets`
> - `Background`
> - `BackgroundFill`
> - `BackgroundImage`
> - `BackgroundPosition`
> - `BackgroundSize`
> - `Border`
> - `BorderImage`
> - `BorderStroke`
> - `BorderWidths`
> - `CornerRadii`
> - `Stop`
> - `Paint` and all of its subclasses
> - `Margins` (internal type)
> - `BorderImageSlices` (internal type)
>
> ## Interpolation of composite objects
>
> As of now, only `Color`, `Point2D`, and `Point3D` are interpolatable. Each of
> these classes is an aggregate of `double` values, which are combined using
> linear interpolation. However, many of the new interpolatable classes
> comprise of not only `double` values, but a whole range of other types. This
> requires us to more precisely define what we mean by "interpolation".
>
> Mirroring the CSS specification, the `Interpolatable` interface defines
> several types of component interpolation:
>
> | Interpolation type | Description |
> |---|---|
> | default | Component types that implement `Interpolatable` are interpolated
> by calling the `interpolate(Object, double)}` method. |
> | linear | Two components are combined by linear interpolation such that `t =
> 0` produces the start value, and `t = 1` produces the end value. This
> interpolation type is usually applicable for numeric components. |
> | discrete | If two components cannot be meaningfully combined, the
> intermediate component value is equal to the start value for `t < 0.5` and
> equal to the end value for `t >= 0.5`. |
> | pairwise | Two lists are combined by pairwise interpolation. If the start
> list has fewer elements than the target list, the missing elements are copied
> from the target list. If the start list has more elements than the ...
Michael Strauß has updated the pull request incrementally with two additional
commits since the last revision:
- documentation changes
- documentation changes
-
Changes:
- all: https://git.openjdk.org/jfx/pull/1522/files
- new: https://git.openjdk.org/jfx/pull/1522/files/3027caa5..e3792cc5
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jfx&pr=1522&range=40
- incr: https://webrevs.openjdk.org/?repo=jfx&pr=1522&range=39-40
Stats: 36 lines in 5 files changed: 19 ins; 10 del; 7 mod
Patch: https://git.openjdk.org/jfx/pull/1522.diff
Fetch: git fetch https://git.openjdk.org/jfx.git pull/1522/head:pull/1522
PR: https://git.openjdk.org/jfx/pull/1522
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v42]
> This PR completes the CSS Transitions story (see #870) by adding
> interpolation support for backgrounds and borders, making them targetable by
> transitions.
>
> `Background` and `Border` objects are deeply immutable, but not
> interpolatable. Consider the following `Background`, which describes the
> background of a `Region`:
>
>
> Background {
> fills = [
> BackgroundFill {
> fill = Color.RED
> }
> ]
> }
>
>
> Since backgrounds are deeply immutable, changing the region's background to
> another color requires the construction of a new `Background`, containing a
> new `BackgroundFill`, containing the new `Color`.
>
> Animating the background color using a CSS transition therefore requires the
> entire Background object graph to be interpolatable in order to generate
> intermediate backgrounds.
>
> More specifically, the following types will now implement `Interpolatable`.
>
> - `Insets`
> - `Background`
> - `BackgroundFill`
> - `BackgroundImage`
> - `BackgroundPosition`
> - `BackgroundSize`
> - `Border`
> - `BorderImage`
> - `BorderStroke`
> - `BorderWidths`
> - `CornerRadii`
> - `Stop`
> - `Paint` and all of its subclasses
> - `Margins` (internal type)
> - `BorderImageSlices` (internal type)
>
> ## Interpolation of composite objects
>
> As of now, only `Color`, `Point2D`, and `Point3D` are interpolatable. Each of
> these classes is an aggregate of `double` values, which are combined using
> linear interpolation. However, many of the new interpolatable classes
> comprise of not only `double` values, but a whole range of other types. This
> requires us to more precisely define what we mean by "interpolation".
>
> Mirroring the CSS specification, the `Interpolatable` interface defines
> several types of component interpolation:
>
> | Interpolation type | Description |
> |---|---|
> | default | Component types that implement `Interpolatable` are interpolated
> by calling the `interpolate(Object, double)}` method. |
> | linear | Two components are combined by linear interpolation such that `t =
> 0` produces the start value, and `t = 1` produces the end value. This
> interpolation type is usually applicable for numeric components. |
> | discrete | If two components cannot be meaningfully combined, the
> intermediate component value is equal to the start value for `t < 0.5` and
> equal to the end value for `t >= 0.5`. |
> | pairwise | Two lists are combined by pairwise interpolation. If the start
> list has fewer elements than the target list, the missing elements are copied
> from the target list. If the start list has more elements than the ...
Michael Strauß has updated the pull request incrementally with one additional
commit since the last revision:
removed unnecessary imports
-
Changes:
- all: https://git.openjdk.org/jfx/pull/1522/files
- new: https://git.openjdk.org/jfx/pull/1522/files/e3792cc5..7d086cf6
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jfx&pr=1522&range=41
- incr: https://webrevs.openjdk.org/?repo=jfx&pr=1522&range=40-41
Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod
Patch: https://git.openjdk.org/jfx/pull/1522.diff
Fetch: git fetch https://git.openjdk.org/jfx.git pull/1522/head:pull/1522
PR: https://git.openjdk.org/jfx/pull/1522
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v40]
> This PR completes the CSS Transitions story (see #870) by adding
> interpolation support for backgrounds and borders, making them targetable by
> transitions.
>
> `Background` and `Border` objects are deeply immutable, but not
> interpolatable. Consider the following `Background`, which describes the
> background of a `Region`:
>
>
> Background {
> fills = [
> BackgroundFill {
> fill = Color.RED
> }
> ]
> }
>
>
> Since backgrounds are deeply immutable, changing the region's background to
> another color requires the construction of a new `Background`, containing a
> new `BackgroundFill`, containing the new `Color`.
>
> Animating the background color using a CSS transition therefore requires the
> entire Background object graph to be interpolatable in order to generate
> intermediate backgrounds.
>
> More specifically, the following types will now implement `Interpolatable`.
>
> - `Insets`
> - `Background`
> - `BackgroundFill`
> - `BackgroundImage`
> - `BackgroundPosition`
> - `BackgroundSize`
> - `Border`
> - `BorderImage`
> - `BorderStroke`
> - `BorderWidths`
> - `CornerRadii`
> - `Stop`
> - `Paint` and all of its subclasses
> - `Margins` (internal type)
> - `BorderImageSlices` (internal type)
>
> ## Interpolation of composite objects
>
> As of now, only `Color`, `Point2D`, and `Point3D` are interpolatable. Each of
> these classes is an aggregate of `double` values, which are combined using
> linear interpolation. However, many of the new interpolatable classes
> comprise of not only `double` values, but a whole range of other types. This
> requires us to more precisely define what we mean by "interpolation".
>
> Mirroring the CSS specification, the `Interpolatable` interface defines
> several types of component interpolation:
>
> | Interpolation type | Description |
> |---|---|
> | default | Component types that implement `Interpolatable` are interpolated
> by calling the `interpolate(Object, double)}` method. |
> | linear | Two components are combined by linear interpolation such that `t =
> 0` produces the start value, and `t = 1` produces the end value. This
> interpolation type is usually applicable for numeric components. |
> | discrete | If two components cannot be meaningfully combined, the
> intermediate component value is equal to the start value for `t < 0.5` and
> equal to the end value for `t >= 0.5`. |
> | pairwise | Two lists are combined by pairwise interpolation. If the start
> list has fewer elements than the target list, the missing elements are copied
> from the target list. If the start list has more elements than the ...
Michael Strauß has updated the pull request with a new target base due to a
merge or a rebase. The pull request now contains 63 commits:
- fix merge conflicts
- Merge branch 'master' into feature/interpolatable
# Conflicts:
#
modules/javafx.graphics/src/test/java/test/javafx/geometry/InsetsTest.java
#
modules/javafx.graphics/src/test/java/test/javafx/scene/layout/BackgroundFillTest.java
#
modules/javafx.graphics/src/test/java/test/javafx/scene/layout/BackgroundImageTest.java
#
modules/javafx.graphics/src/test/java/test/javafx/scene/layout/BackgroundPositionTest.java
#
modules/javafx.graphics/src/test/java/test/javafx/scene/layout/BackgroundSizeTest.java
#
modules/javafx.graphics/src/test/java/test/javafx/scene/layout/BackgroundTest.java
#
modules/javafx.graphics/src/test/java/test/javafx/scene/layout/BorderStrokeTest.java
#
modules/javafx.graphics/src/test/java/test/javafx/scene/layout/BorderTest.java
#
modules/javafx.graphics/src/test/java/test/javafx/scene/layout/BorderWidthsTest.java
#
modules/javafx.graphics/src/test/java/test/javafx/scene/paint/ColorTest.java
#
modules/javafx.graphics/src/test/java/test/javafx/scene/paint/ImagePatternTest.java
#
modules/javafx.graphics/src/test/java/test/javafx/scene/paint/LinearGradientTest.java
#
modules/javafx.graphics/src/test/java/test/javafx/scene/paint/RadialGradientTest.java
#
modules/javafx.graphics/src/test/java/test/javafx/scene/paint/StopListTest.java
#
modules/javafx.graphics/src/test/java/test/javafx/scene/paint/StopTest.java
- javadoc change
- javadoc change
- javadoc change
- small doc changes, copyright header
- fixed a bug when null values require discrete transitions
- javadoc changes
- added tests
- javadoc changes
- ... and 53 more: https://git.openjdk.org/jfx/compare/29004352...3027caa5
-
Changes: https://git.openjdk.org/jfx/pull/1522/files
Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1522&range=39
Stats: 6710 lines in 77 files changed: 5843 ins; 371 del; 496 mod
Patch: https://git.openjdk.org/jfx/pull/1522.diff
Fetch: git fetch https://git.openjdk.org/jfx.git pull/1522/head:pull/1522
PR: https://git.openjdk.org/jfx/pull/1522
Re: [Feature Proposal]: TriangleMesh - Vertex Color Support
> I'm not yet convinced that new vertex formats are the way to go. To be fair, I'm not sure why VertexFormat even exists. It serves no purpose when creating a TriangleMesh, as the vertex format could easily be inferred by the presence (or absence) of vertex data. Having users specify the VertexFormat explicitly is probably not a good API, as it now makes the other data components be dependent on the choice of the vertex format. Of course, not all combinations of vertex data are valid, but this could be specified in documentation and be validated by the implementation. I think it might be best to deprecate VertexFormat for removal. This would also prevent a potential explosion of vertex formats in the public API. I have no objection to the choice to add vertex colors as an additional data component to TriangleMesh. It adds to the API, but this is not a slippery slope as there's only a very finite set of features we might be tempted to add next: The most obvious thing that JavaFX 3D really can't do is shadows. These come in static form (light mapping) and dynamic form (shadow mapping). Light mapping requires a second set of texcoords, while shadow mapping does not. Adding light mapping would make JavaFX 3D competitive with late-90's graphics, and adding shadow mapping would make it competitive with early-2000's graphics.
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v39]
> This PR completes the CSS Transitions story (see #870) by adding
> interpolation support for backgrounds and borders, making them targetable by
> transitions.
>
> `Background` and `Border` objects are deeply immutable, but not
> interpolatable. Consider the following `Background`, which describes the
> background of a `Region`:
>
>
> Background {
> fills = [
> BackgroundFill {
> fill = Color.RED
> }
> ]
> }
>
>
> Since backgrounds are deeply immutable, changing the region's background to
> another color requires the construction of a new `Background`, containing a
> new `BackgroundFill`, containing the new `Color`.
>
> Animating the background color using a CSS transition therefore requires the
> entire Background object graph to be interpolatable in order to generate
> intermediate backgrounds.
>
> More specifically, the following types will now implement `Interpolatable`.
>
> - `Insets`
> - `Background`
> - `BackgroundFill`
> - `BackgroundImage`
> - `BackgroundPosition`
> - `BackgroundSize`
> - `Border`
> - `BorderImage`
> - `BorderStroke`
> - `BorderWidths`
> - `CornerRadii`
> - `Stop`
> - `Paint` and all of its subclasses
> - `Margins` (internal type)
> - `BorderImageSlices` (internal type)
>
> ## Interpolation of composite objects
>
> As of now, only `Color`, `Point2D`, and `Point3D` are interpolatable. Each of
> these classes is an aggregate of `double` values, which are combined using
> linear interpolation. However, many of the new interpolatable classes
> comprise of not only `double` values, but a whole range of other types. This
> requires us to more precisely define what we mean by "interpolation".
>
> Mirroring the CSS specification, the `Interpolatable` interface defines
> several types of component interpolation:
>
> | Interpolation type | Description |
> |---|---|
> | default | Component types that implement `Interpolatable` are interpolated
> by calling the `interpolate(Object, double)}` method. |
> | linear | Two components are combined by linear interpolation such that `t =
> 0` produces the start value, and `t = 1` produces the end value. This
> interpolation type is usually applicable for numeric components. |
> | discrete | If two components cannot be meaningfully combined, the
> intermediate component value is equal to the start value for `t < 0.5` and
> equal to the end value for `t >= 0.5`. |
> | pairwise | Two lists are combined by pairwise interpolation. If the start
> list has fewer elements than the target list, the missing elements are copied
> from the target list. If the start list has more elements than the ...
Michael Strauß has updated the pull request incrementally with one additional
commit since the last revision:
javadoc change
-
Changes:
- all: https://git.openjdk.org/jfx/pull/1522/files
- new: https://git.openjdk.org/jfx/pull/1522/files/1227a79f..1c842ccc
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jfx&pr=1522&range=38
- incr: https://webrevs.openjdk.org/?repo=jfx&pr=1522&range=37-38
Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jfx/pull/1522.diff
Fetch: git fetch https://git.openjdk.org/jfx.git pull/1522/head:pull/1522
PR: https://git.openjdk.org/jfx/pull/1522
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v38]
On Sat, 14 Sep 2024 07:42:45 GMT, Nir Lisker wrote: > Approving from the perspective of the API with the remark that there is a > theoretical API break for `TransitionEvent`. Thanks for the review! I added the changed `TransitionEvent` API as a compatibility risk in the CSR. - PR Comment: https://git.openjdk.org/jfx/pull/1522#issuecomment-2351060449
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v36]
On Sat, 14 Sep 2024 07:08:22 GMT, Nir Lisker wrote: >> Yes, in your example, `red` will be linearly interpolated with `green`. >> That's what I intended to say with "pairwise interpolation": you pair up >> elements of both lists, and then for each pair, you apply the interpolation >> rules as described. I don't understand what you mean that the spec "is for >> the list itself". > > Then maybe mention that paired elements are interpolated? > By "this specs is for the list itself" I meant that pairwise interpolation is > only relevant for the list itself and not for an element in the list (unless > it's a list itself). So a question I would have as a user is what happens > with the elements. I added another sentence, do you think this clears things up? - PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1759688927
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v38]
> This PR completes the CSS Transitions story (see #870) by adding
> interpolation support for backgrounds and borders, making them targetable by
> transitions.
>
> `Background` and `Border` objects are deeply immutable, but not
> interpolatable. Consider the following `Background`, which describes the
> background of a `Region`:
>
>
> Background {
> fills = [
> BackgroundFill {
> fill = Color.RED
> }
> ]
> }
>
>
> Since backgrounds are deeply immutable, changing the region's background to
> another color requires the construction of a new `Background`, containing a
> new `BackgroundFill`, containing the new `Color`.
>
> Animating the background color using a CSS transition therefore requires the
> entire Background object graph to be interpolatable in order to generate
> intermediate backgrounds.
>
> More specifically, the following types will now implement `Interpolatable`.
>
> - `Insets`
> - `Background`
> - `BackgroundFill`
> - `BackgroundImage`
> - `BackgroundPosition`
> - `BackgroundSize`
> - `Border`
> - `BorderImage`
> - `BorderStroke`
> - `BorderWidths`
> - `CornerRadii`
> - `Stop`
> - `Paint` and all of its subclasses
> - `Margins` (internal type)
> - `BorderImageSlices` (internal type)
>
> ## Interpolation of composite objects
>
> As of now, only `Color`, `Point2D`, and `Point3D` are interpolatable. Each of
> these classes is an aggregate of `double` values, which are combined using
> linear interpolation. However, many of the new interpolatable classes
> comprise of not only `double` values, but a whole range of other types. This
> requires us to more precisely define what we mean by "interpolation".
>
> Mirroring the CSS specification, the `Interpolatable` interface defines
> several types of component interpolation:
>
> | Interpolation type | Description |
> |---|---|
> | default | Component types that implement `Interpolatable` are interpolated
> by calling the `interpolate(Object, double)}` method. |
> | linear | Two components are combined by linear interpolation such that `t =
> 0` produces the start value, and `t = 1` produces the end value. This
> interpolation type is usually applicable for numeric components. |
> | discrete | If two components cannot be meaningfully combined, the
> intermediate component value is equal to the start value for `t < 0.5` and
> equal to the end value for `t >= 0.5`. |
> | pairwise | Two lists are combined by pairwise interpolation. If the start
> list has fewer elements than the target list, the missing elements are copied
> from the target list. If the start list has more elements than the ...
Michael Strauß has updated the pull request incrementally with one additional
commit since the last revision:
javadoc change
-
Changes:
- all: https://git.openjdk.org/jfx/pull/1522/files
- new: https://git.openjdk.org/jfx/pull/1522/files/c9cf1392..1227a79f
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jfx&pr=1522&range=37
- incr: https://webrevs.openjdk.org/?repo=jfx&pr=1522&range=36-37
Stats: 5 lines in 1 file changed: 2 ins; 0 del; 3 mod
Patch: https://git.openjdk.org/jfx/pull/1522.diff
Fetch: git fetch https://git.openjdk.org/jfx.git pull/1522/head:pull/1522
PR: https://git.openjdk.org/jfx/pull/1522
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v36]
On Sat, 14 Sep 2024 07:02:33 GMT, Nir Lisker wrote: >> Interpolation with `null` is not possible: `Interpolatable.interpolate(T, >> double)` is specified to throw NPE when `null` is passed in. When a >> styleable property transitions from a null value to a non-null value, it >> always transitions as discrete (i.e. is switches at the 50% mark) and >> doesn't use `Interpolatable.interpolate`. > > So when will these checks fail? They will never fail as currently implemented. I just added them as documentation for future maintainers. - PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1759684905
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v36]
On Sat, 14 Sep 2024 04:36:09 GMT, Nir Lisker wrote:
>> Michael Strauß has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> small doc changes, copyright header
>
> modules/javafx.graphics/src/main/java/javafx/scene/layout/Background.java
> line 292:
>
>> 290: private Background(List fills,
>> List images, int ignored) {
>> 291: Objects.requireNonNull(fills, "fills cannot be null");
>> 292: Objects.requireNonNull(images, "images cannot be null");
>
> Are these reached if there is an interpolation from/to null with a non-null?
Interpolation with `null` is not possible: `Interpolatable.interpolate(T,
double)` is specified to throw NPE when `null` is passed in. When a styleable
property transitions from a null value to a non-null value, it always
transitions as discrete (i.e. is switches at the 50% mark) and doesn't use
`Interpolatable.interpolate`.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1759683672
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v37]
> This PR completes the CSS Transitions story (see #870) by adding
> interpolation support for backgrounds and borders, making them targetable by
> transitions.
>
> `Background` and `Border` objects are deeply immutable, but not
> interpolatable. Consider the following `Background`, which describes the
> background of a `Region`:
>
>
> Background {
> fills = [
> BackgroundFill {
> fill = Color.RED
> }
> ]
> }
>
>
> Since backgrounds are deeply immutable, changing the region's background to
> another color requires the construction of a new `Background`, containing a
> new `BackgroundFill`, containing the new `Color`.
>
> Animating the background color using a CSS transition therefore requires the
> entire Background object graph to be interpolatable in order to generate
> intermediate backgrounds.
>
> More specifically, the following types will now implement `Interpolatable`.
>
> - `Insets`
> - `Background`
> - `BackgroundFill`
> - `BackgroundImage`
> - `BackgroundPosition`
> - `BackgroundSize`
> - `Border`
> - `BorderImage`
> - `BorderStroke`
> - `BorderWidths`
> - `CornerRadii`
> - `Stop`
> - `Paint` and all of its subclasses
> - `Margins` (internal type)
> - `BorderImageSlices` (internal type)
>
> ## Interpolation of composite objects
>
> As of now, only `Color`, `Point2D`, and `Point3D` are interpolatable. Each of
> these classes is an aggregate of `double` values, which are combined using
> linear interpolation. However, many of the new interpolatable classes
> comprise of not only `double` values, but a whole range of other types. This
> requires us to more precisely define what we mean by "interpolation".
>
> Mirroring the CSS specification, the `Interpolatable` interface defines
> several types of component interpolation:
>
> | Interpolation type | Description |
> |---|---|
> | default | Component types that implement `Interpolatable` are interpolated
> by calling the `interpolate(Object, double)}` method. |
> | linear | Two components are combined by linear interpolation such that `t =
> 0` produces the start value, and `t = 1` produces the end value. This
> interpolation type is usually applicable for numeric components. |
> | discrete | If two components cannot be meaningfully combined, the
> intermediate component value is equal to the start value for `t < 0.5` and
> equal to the end value for `t >= 0.5`. |
> | pairwise | Two lists are combined by pairwise interpolation. If the start
> list has fewer elements than the target list, the missing elements are copied
> from the target list. If the start list has more elements than the ...
Michael Strauß has updated the pull request incrementally with one additional
commit since the last revision:
javadoc change
-
Changes:
- all: https://git.openjdk.org/jfx/pull/1522/files
- new: https://git.openjdk.org/jfx/pull/1522/files/f9187a1d..c9cf1392
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jfx&pr=1522&range=36
- incr: https://webrevs.openjdk.org/?repo=jfx&pr=1522&range=35-36
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jfx/pull/1522.diff
Fetch: git fetch https://git.openjdk.org/jfx.git pull/1522/head:pull/1522
PR: https://git.openjdk.org/jfx/pull/1522
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v36]
On Thu, 12 Sep 2024 13:45:04 GMT, Nir Lisker wrote: >> Michael Strauß has updated the pull request incrementally with one >> additional commit since the last revision: >> >> small doc changes, copyright header > > modules/javafx.graphics/src/main/java/javafx/css/TransitionEvent.java line > 109: > >> 107: public TransitionEvent(EventType eventType, >> 108:StyleableProperty property, >> 109:String propertyName, > > Does this break backwards compatibility? Sure, but I don't really mind: 1. `TransitionEvent` is a CSS event, it can not usefully be created by application developers. 2. This API (without `propertyName`) will be introduced with JFX 23, so developers will need to hurry if they intend to misuse it until JFX 24 rolls around... I wouldn't mind removing the public constructor altogether, but this is not something that we usually do for JavaFX events. - PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1759683190
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v36]
On Thu, 12 Sep 2024 13:41:53 GMT, Nir Lisker wrote: >> Michael Strauß has updated the pull request incrementally with one >> additional commit since the last revision: >> >> small doc changes, copyright header > > modules/javafx.graphics/src/main/java/javafx/animation/Interpolatable.java > line 54: > >> 52: * Two lists are combined by pairwise interpolation. If >> the start list has fewer elements than >> 53: * the target list, the missing elements are copied from >> the target list. If the start list has >> 54: * more elements than the target list, the excess >> elements are discarded. > > Are the elements within the lists interpolated as well? For example, if one > list is `[red, blue]` and the other is `[green]`, then `blue` is discarded as > excess, but will `red` be linearly interpolated to `green`? > > I understand that this specs is for the list itself, but in practice the list > can contain interpolatable elements, so it might be clearer to say what > happens with them. Yes, in your example, `red` will be linearly interpolated with `green`. That's what I intended to say with "pairwise interpolation": you pair up elements of both lists, and then for each pair, you apply the interpolation rules as described. I don't understand what you mean that the spec "is for the list itself". - PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1759682578
Re: [Feature Proposal]: TriangleMesh - Vertex Color Support
Just a comment about the culture of the OpenJFX project, which you seem to be struggling with: >From my experience, this project is usually quite democratic in its processes. The features and enhancements that successfully make it into JFX start with a discussion on the mailing list. This is where you gather buy-in from other developers (if no one is interested in your proposed enhancement, you're in a tough spot). Don't worry too much about formal processes, it's about community support at this stage. Get other people to be excited about your enhancement, and hopefully some will find the time to look at the design and your implementation. Some proposals build up inertia and move forward, others end up dying due to a lack of interest. 3D graphics in JavaFX is a bit of a niche feature (and vertex colors is even more niche), which is probably why the response to your proposal is a bit slow. That being said, I'll take a closer look at your proposal during the weekend. On Fri, Sep 13, 2024 at 4:59 AM Knee Snap wrote: > > Thanks Nir! No worries about being busy, my concern was when I didn't get > replies to my questions about what I should be doing and procedural > questions. But, it seems maybe I wasn't super clear about my questions since > you aren't sure what they were. > I also went through many days waiting for responses about expectations all > while he was replying quickly to other emails, seemingly ignoring mine. I'm > not suggesting he's done anything wrong, I can imagine perfectly good reasons > for not having replied, but my questions were related to the contribution > guidelines and expectations. So I've felt like I've been going foward without > clear expectations of both myself and contributors, and like I wasn't even > guaranteed a response at all. The only reference I had for how long I should > expect to wait was based on the other emails I saw, which had fast replies. > I'm hoping to clarify my responsibilities and expectations, and what I should > expect from contributors, since there's a lot of stuff to get familiar with > in the contribution guide, and it's my first time contributing. Due to that, > it felt like those questions went mostly ignored and it wasn't a matter of > being busy, as other emails were getting quick replies, though perhaps I > didn't phrase them clearly. Hoping to now clear up my confusions about > responsibilities. Your email has helped reassure me that I was not getting > ignored, but instead just had a lower priority or something else that could > cause it to take longer to get a reply. > > Overall, I'd like to give some insight into what it has felt like for me to > contribute to JavaFX for my first time. The contribution guide is good, but > it hasn't been enough. > This isn't here to point blame or to criticize, but instead give some > perspective into my experience here and why this has been such a frustrating > experience. Perhaps this will help improve the contribution experience for > newcomers like me going forward keeping this in mind, but ideally it'll help > set the stage for why I am so confused and help you guys resolve my > confusions. > -> The contribution guide misled me in many ways here, and there are many > questions I still do not have an answer to. > -> To start off, it said to include a feature proposal, and it gave > suggestions for various things to include. > -> So, I wrote a long feature proposal email and sent it off. For a week I > had a discussion with a contributor where we hashed out different design > choices, I felt like we had appropriately discussed everything which had been > discussed adequately, and was ready to get my feature proposal approved by > Kevin. > -> Everything was going great until one week in when Kevin replied and said > he "would need to be convinced that multiple applications would benefit from > such a feature, and that your proposed solution -- as documented and exposed > by the public API -- is the best way to go." > -> He also asked if I had read the contribution guide, implying he didn't > think what I had previously sent wasn't me 1:1 following the contribution > guide. > -> I still don't understand why he sent that, I believe I presented exactly > what the contribution guide wanted. There were a handful of things that got > missed, but each were discussed in the email thread. > -> Did he not read the proposal and assume I did it wrong? Did he read it > and think it wasn't convincing? Did he want a new proposal which included the > things which had been discussed in the email thread? > -> So, with this confusion I asked for clarification as well as other > questions relating to Metal like who was working on it, if I could have a > contact for them, etc. > -> Never got a reply! Okay, so what the heck am I supposed to do here? Kevin > asked for new a feature proposal, and I guess for some inexplicable reason > this one wasn't good enough? It doesn'
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v36]
On Fri, 13 Sep 2024 01:41:24 GMT, Nir Lisker wrote: > But the docs specify the interpolation type. Changing it will not be > backwards compatible. The docs only specify the interpolation type, but not the transition type. If users need a discrete transition, they can always use a step easing function. It would be unusual to transition images with a `linear` easing function (seeing as that will as of now not result in a linear transition), so I think the amount of surprise will be limited if, at some point in the future, a `-fx-background-image` transition with a `linear` function will start to smoothly fade between images. - PR Comment: https://git.openjdk.org/jfx/pull/1522#issuecomment-2347974594
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v36]
On Thu, 12 Sep 2024 13:37:07 GMT, Nir Lisker wrote: > I won't be doing a full review, but I want to look at the API in case > something was missed and regret it later :) > > The only other thing I was curious about is how the css specs decides which > interpolation type to use for each type of value, and if we do the same. I > thought that image interpolation is ambiguous: do you just jump to the other > image or do you fade/blend from one to the other? I see that you went with > the former, which is simpler, but the latter is more intuitive for me. One > can use a custom interpolator, but I didn't look if it works with css. Fading between images is really hard, as its implications reach deep into the rendering system. For performance reasons, blending the images shouldn't be done by the CSS system on the CPU; instead, we would need a way to feed both images into the rendering system, such that they can be combined with two render passes. In addition to that, a background or border can layer multiple images on top of each other. In this case, it is unclear whether we would want each image layer to blend separately, or if the stack of images should blend as a whole. This would also require special support in the rendering system. That's probably the reason why smooth interpolation of images is not supported by specifications such as [Backgrounds Level 3](https://drafts.csswg.org/css-backgrounds-3/#propdef-background-image). Support for interpolation of images is coming in [Images Level 4](https://drafts.csswg.org/css-images-4/#interpolating-images), but that has not made its way yet into the work-in-progress [Backgrounds Level 4](https://drafts.csswg.org/css-backgrounds-4/). I'd say we take a slow approach here and wait until the pieces settle down to see where this potential future feature lands. As for the mor general question how we decide which interpolation type we support: the current implementation always tries to support smooth interpolation, except where it would be hard to do so. That's the same approach that the W3C spec uses. I think we can (and should) extend support for smooth interpolation in the future. For example, currently we can't smoothly interpolate between relative and absolute values, but this is a feature that can be added later. - PR Comment: https://git.openjdk.org/jfx/pull/1522#issuecomment-2346429243
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v3]
On Thu, 12 Sep 2024 12:45:25 GMT, Nir Lisker wrote: >> That may be the case, but I think that's preferrable to requiring two >> iterations of the list (one to figure out how many non-null values it >> contains, the second to copy over the values). In practice, there shouldn't >> be any nulls in most cases, as I don't know why someone would construct a >> `Background` or `Border` with many nulls in there. > > Is this code required to be so highly performant that you need to check for > `RandomAccess`? Under normal circumstances, I would just stream the list and > filter for `null`s. This can run tens of thousands of times per second, which is probably not the ideal use case for streams. - PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1756849114
Re: RFR: 8090456: Focus Management
On Wed, 11 Sep 2024 15:06:07 GMT, Andy Goryachev wrote:
> The default one seems to be geometry-based ("directional") _where supported_,
> and the other being a "logical" one which is not explicitly accessible. The
> question, therefore, is do we need a static getter for that kind of a policy?
> (I'd say no since the logical order depends on a particular control).
I don't quite understand what you mean by "not explicitly accessible". Both
categories are exposed with the `TraversalDirection` enum: `UP`, `DOWN`,
`LEFT`, and `RIGHT` are the directional modes, whereas `NEXT`, `NEXT_IN_LINE`,
and `PREVIOUS` are the logical modes.
I think that disentangling those two categories, and having a set of predefined
traversal policies that can be set independently for both of those, will be
good enough for 99% of cases. The reason I'm bringing this up is that
`Algorithm` (now `TraversalPolicy`) is too low-level to be useful for most
application developers. _We_ should be implementing those algorithms, and then
allow developers to compose them as they see fit.
When I look at the existing code, I see quite a lot of bespoke `Algorithm`
implementations. For example, `ToolBarSkin` has almost 100 lines of code
dedicated to a custom implementation. However, when I just remove this
implementation entirely, I get _almost_ the exact same focus traversal
behavior, even in complicated scenarios like when a toolbar item is itself a
focus-traversable container.
This suggests to me that we shouldn't encourage the proliferation of even more
bespoke implementations, but make it easier to compose well-tested existing
algorithms.
>
> A couple of points:
>
> 1. setting via CSS is not a goal (I probably should mention this in the JEP)
> 2. the 4-item enumeration is not complete. For example, application may
> specify some other, or even dynamic, order of traversal, which would require
> a custom policy.
We can add scenarios to this list if it is not complete, and then see if we
still need the option to create a custom traversal algorithm. My point is that
the API should be tailored to be simple and useful in 99% of cases, and then
maybe have an additional extension point for the rest. Right now, you're
proposing to expose the lowest-level API which requires developers to reason
about the scene graph and find individual nodes, which I think should be
reconsidered.
When I look at various JBS issues and browse StackOverflow, I notice that most
developers are only looking for pretty simple things like cyclic traversal in a
container. It's probably prudent to tailor the API for those use cases.
>
> Another question is whether there exists a policy that cannot be implemented
> by the new APIs provided by this PR.
>
> What do you think?
Since the API works directly on the scene graph, you can probably create all
kinds of algorithms with it. The downside of that is, however, that all
implementations are ad-hoc solutions that don't generalize. For example, the
radio group I mentioned earlier suggests that we should have an API to define
an "entry point" into a container of nodes. You can do that with a custom
implementation, but I think having this be elevated to a proper API would be
better.
-
PR Comment: https://git.openjdk.org/jfx/pull/1555#issuecomment-2344259476
Re: RFR: 8090456: Focus Management
On Tue, 3 Sep 2024 19:09:15 GMT, Andy Goryachev wrote:
> Public APIs for focus traversal and the focus traversal policy:
>
> https://github.com/andy-goryachev-oracle/Test/blob/main/doc/FocusTraversal/FocusTraversal.md
>
> This work is loosely based on the patch
> https://cr.openjdk.org/~jgiles/8061673/
By its own admission, this proposal basically promotes an internal
implementation to public API (albeit with some refactoring). This may or may
not be a good idea, so I'd like to take a step back and look at the problem
we're trying to solve.
Looking at several of the linked JBS issues, a missing feature of JavaFX is the
ability to customize the focus traversal logic of a custom control (or a
container of nodes in the widest sense of the word). Any proposed solution
should support complex scenarios like toolbars with overflow, or [radio
groups](https://www.w3.org/TR/2009/WD-wai-aria-practices-20090224/#radiobutton).
I think there are two distinct modes of focus traversal: logical ("tabbing")
and directional (arrow keys). Both modes are independent axes of the problem at
hand, and should be customizable independently from each other.
The current implementation provides a single `Algorithm` (or `TraversalPolicy`
after the refactoring) for both traversal modes. I've played around with the
code for a while, and while in theory I can provide my own `TraversalPolicy`,
it can be quite a lot of code and it's not as easy as it could be.
Additionally, it is hard to customize the traversal policies of the
substructure of skinnable controls.
I question whether we need custom traversal policies at all, as the ways in
which focus can move from one node to the next are quite limited:
1. **Permeable edges**: The next node in a container is selected until an edge
is reached, then the input focus leaves the container.
2. **Confined to container**: The next node in a container is selected until
the end of the container is reached; then the input focus doesn't move on.
3. **Cyclic in container**: Like confined, but at the end of the container, the
input focus wraps around to the first element.
4. **Single focused node in container**: When the input focus enters a
container, it moves to the node that was most recently selected (or to the
first node in the container if none of the nodes was selected before); after
that the input focus leaves the container.
Having two traversal toggles (logical and directional), each with four possible
modes, already solves most of the problem. The advantage of this over a custom
policy implementation is that these properties can be modelled with an
enumeration and easily be set by control and application developers, and they
can also be set via CSS. This allows developers to customize the focus
traversal behavior of existing skinnable controls, as the substructure of a
skin is accessible via CSS.
The "single focused node in container" mode is required for scenarios like
radio groups, when the developers wants to return to the point of previous
focus when tabbing into the group a second time. This requires another API that
allows a focus container to indicate its entry point, which is the initial
element that is selected when the input focus moves into the container.
-
PR Comment: https://git.openjdk.org/jfx/pull/1555#issuecomment-2342418714
Re: RFR: 8339726: Remove pausing code (used for testing) from AbstractPrimaryTimer
On Mon, 9 Sep 2024 06:23:02 GMT, John Hendrikx wrote: > This PR removes the pausing logic from AbstractPrimaryTimer. The test that > is using it wasn't actually testing anything in AbstractPrimaryTimer itself, > and no other code needs this pausing code. Looks good. - Marked as reviewed by mstrauss (Committer). PR Review: https://git.openjdk.org/jfx/pull/1558#pullrequestreview-2290891832
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v36]
> This PR completes the CSS Transitions story (see #870) by adding
> interpolation support for backgrounds and borders, making them targetable by
> transitions.
>
> `Background` and `Border` objects are deeply immutable, but not
> interpolatable. Consider the following `Background`, which describes the
> background of a `Region`:
>
>
> Background {
> fills = [
> BackgroundFill {
> fill = Color.RED
> }
> ]
> }
>
>
> Since backgrounds are deeply immutable, changing the region's background to
> another color requires the construction of a new `Background`, containing a
> new `BackgroundFill`, containing the new `Color`.
>
> Animating the background color using a CSS transition therefore requires the
> entire Background object graph to be interpolatable in order to generate
> intermediate backgrounds.
>
> More specifically, the following types will now implement `Interpolatable`.
>
> - `Insets`
> - `Background`
> - `BackgroundFill`
> - `BackgroundImage`
> - `BackgroundPosition`
> - `BackgroundSize`
> - `Border`
> - `BorderImage`
> - `BorderStroke`
> - `BorderWidths`
> - `CornerRadii`
> - `Stop`
> - `Paint` and all of its subclasses
> - `Margins` (internal type)
> - `BorderImageSlices` (internal type)
>
> ## Interpolation of composite objects
>
> As of now, only `Color`, `Point2D`, and `Point3D` are interpolatable. Each of
> these classes is an aggregate of `double` values, which are combined using
> linear interpolation. However, many of the new interpolatable classes
> comprise of not only `double` values, but a whole range of other types. This
> requires us to more precisely define what we mean by "interpolation".
>
> Mirroring the CSS specification, the `Interpolatable` interface defines
> several types of component interpolation:
>
> | Interpolation type | Description |
> |---|---|
> | default | Component types that implement `Interpolatable` are interpolated
> by calling the `interpolate(Object, double)}` method. |
> | linear | Two components are combined by linear interpolation such that `t =
> 0` produces the start value, and `t = 1` produces the end value. This
> interpolation type is usually applicable for numeric components. |
> | discrete | If two components cannot be meaningfully combined, the
> intermediate component value is equal to the start value for `t < 0.5` and
> equal to the end value for `t >= 0.5`. |
> | pairwise | Two lists are combined by pairwise interpolation. If the start
> list has fewer elements than the target list, the missing elements are copied
> from the target list. If the start list has more elements than the ...
Michael Strauß has updated the pull request incrementally with one additional
commit since the last revision:
small doc changes, copyright header
-
Changes:
- all: https://git.openjdk.org/jfx/pull/1522/files
- new: https://git.openjdk.org/jfx/pull/1522/files/acdec110..f9187a1d
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jfx&pr=1522&range=35
- incr: https://webrevs.openjdk.org/?repo=jfx&pr=1522&range=34-35
Stats: 4 lines in 3 files changed: 0 ins; 0 del; 4 mod
Patch: https://git.openjdk.org/jfx/pull/1522.diff
Fetch: git fetch https://git.openjdk.org/jfx.git pull/1522/head:pull/1522
PR: https://git.openjdk.org/jfx/pull/1522
Re: RFR: 8339603: Seal the class hierarchy of Node, Camera, LightBase, Shape, Shape3D [v2]
On Fri, 6 Sep 2024 16:56:43 GMT, Andy Goryachev wrote:
>> Michael Strauß has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> remove documentation
>
> modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 421:
>
>> 419: public abstract sealed class Node
>> 420: implements EventTarget, Styleable
>> 421: permits AbstractNode, Camera, LightBase, Parent, SubScene,
>> Canvas, ImageView, Shape, Shape3D {
>
> We may need to use stronger wording in javadoc in regards to extending Nodes.
> should -> must, etc. (or, rather, 'cannot' and possibly explain why?)
I've removed the language that you can't extend those classes, as it is now
redundant because it is enforced by the compiler. We shouldn't document obvious
facts without providing any added value (for example, adding context and
reasoning _why_ it's the way it is).
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1556#discussion_r1749339201
Re: RFR: 8339603: Seal the class hierarchy of Node, Camera, LightBase, Shape, Shape3D [v2]
> None of these classes can be extended by user code, and any attempt to do so > will fail at runtime with an exception. For this reason, we can seal the > class hierarchy and remove the run-time checks to turn this into a > compile-time error instead. > > In some cases, `Node` and `Shape` are extended by JavaFX classes in other > modules, preventing those derived classes from being permitted subclasses. A > non-exported `AbstractNode` and `AbstractShape` class is provided just for > these scenarios. Note that introducing a new superclass is a source- and > binary-compatible change (see [JLS ch. > 13](https://docs.oracle.com/javase/specs/jls/se22/html/jls-13.html)). > > I'm not sure if this change requires a CSR, as it doesn't change the > specification in any meaningful way. There can be no valid JavaFX program > that is affected by this change. Michael Strauß has updated the pull request incrementally with one additional commit since the last revision: remove documentation - Changes: - all: https://git.openjdk.org/jfx/pull/1556/files - new: https://git.openjdk.org/jfx/pull/1556/files/c24aed5c..38b7a22e Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1556&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1556&range=00-01 Stats: 23 lines in 5 files changed: 0 ins; 23 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/1556.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1556/head:pull/1556 PR: https://git.openjdk.org/jfx/pull/1556
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v35]
On Fri, 6 Sep 2024 22:02:04 GMT, Andy Goryachev wrote: > Not sure if this is related, but I got an exception flipping through the > monkey tester pages using this PR's workspace: Usually, the go-to explanation for this has something to do with different threads accessing the scene graph. Since we're not dealing with other threads here, maybe it is related to [this](https://mail.openjdk.org/pipermail/openjfx-dev/2024-August/048738.html)? - PR Comment: https://git.openjdk.org/jfx/pull/1522#issuecomment-2334877598
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v34]
On Fri, 6 Sep 2024 17:06:44 GMT, Michael Strauß wrote: >> reproducible. you may need to restart the app. did not see the exception >> all the time, but got it again changing `-fx-border-width:` in the hover >> state from 20 to 200 > >> reproducible. you may need to restart the app. did not see the exception all >> the time, but got it again changing `-fx-border-width:` in the hover state >> from 20 to 200 > > Good catch! The cause for this bug was that when the old value or the new > value is `null`, we fall back to a discrete transition. This wasn't happening > in one particular scenario. I also added a test for this. > I can't think of any other weird scenario to test. Monumental job, thank you > @mstr2 ! Thanks for all the testing and feedback! - PR Comment: https://git.openjdk.org/jfx/pull/1522#issuecomment-2334631715
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v35]
> This PR completes the CSS Transitions story (see #870) by adding
> interpolation support for backgrounds and borders, making them targetable by
> transitions.
>
> `Background` and `Border` objects are deeply immutable, but not
> interpolatable. Consider the following `Background`, which describes the
> background of a `Region`:
>
>
> Background {
> fills = [
> BackgroundFill {
> fill = Color.RED
> }
> ]
> }
>
>
> Since backgrounds are deeply immutable, changing the region's background to
> another color requires the construction of a new `Background`, containing a
> new `BackgroundFill`, containing the new `Color`.
>
> Animating the background color using a CSS transition therefore requires the
> entire Background object graph to be interpolatable in order to generate
> intermediate backgrounds.
>
> More specifically, the following types will now implement `Interpolatable`.
>
> - `Insets`
> - `Background`
> - `BackgroundFill`
> - `BackgroundImage`
> - `BackgroundPosition`
> - `BackgroundSize`
> - `Border`
> - `BorderImage`
> - `BorderStroke`
> - `BorderWidths`
> - `CornerRadii`
> - `Stop`
> - `Paint` and all of its subclasses
> - `Margins` (internal type)
> - `BorderImageSlices` (internal type)
>
> ## Interpolation of composite objects
>
> As of now, only `Color`, `Point2D`, and `Point3D` are interpolatable. Each of
> these classes is an aggregate of `double` values, which are combined using
> linear interpolation. However, many of the new interpolatable classes
> comprise of not only `double` values, but a whole range of other types. This
> requires us to more precisely define what we mean by "interpolation".
>
> Mirroring the CSS specification, the `Interpolatable` interface defines
> several types of component interpolation:
>
> | Interpolation type | Description |
> |---|---|
> | default | Component types that implement `Interpolatable` are interpolated
> by calling the `interpolate(Object, double)}` method. |
> | linear | Two components are combined by linear interpolation such that `t =
> 0` produces the start value, and `t = 1` produces the end value. This
> interpolation type is usually applicable for numeric components. |
> | discrete | If two components cannot be meaningfully combined, the
> intermediate component value is equal to the start value for `t < 0.5` and
> equal to the end value for `t >= 0.5`. |
> | pairwise | Two lists are combined by pairwise interpolation. If the start
> list has fewer elements than the target list, the missing elements are copied
> from the target list. If the start list has more elements than the ...
Michael Strauß has updated the pull request incrementally with one additional
commit since the last revision:
fixed a bug when null values require discrete transitions
-
Changes:
- all: https://git.openjdk.org/jfx/pull/1522/files
- new: https://git.openjdk.org/jfx/pull/1522/files/75e7d98b..acdec110
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jfx&pr=1522&range=34
- incr: https://webrevs.openjdk.org/?repo=jfx&pr=1522&range=33-34
Stats: 36 lines in 2 files changed: 27 ins; 1 del; 8 mod
Patch: https://git.openjdk.org/jfx/pull/1522.diff
Fetch: git fetch https://git.openjdk.org/jfx.git pull/1522/head:pull/1522
PR: https://git.openjdk.org/jfx/pull/1522
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v34]
On Fri, 6 Sep 2024 16:38:41 GMT, Andy Goryachev wrote: > reproducible. you may need to restart the app. did not see the exception all > the time, but got it again changing `-fx-border-width:` in the hover state > from 20 to 200 Good catch! The cause for this bug was that when the old value or the new value is `null`, we fall back to a discrete transition. This wasn't happening in one particular scenario. I also added a test for this. - PR Comment: https://git.openjdk.org/jfx/pull/1522#issuecomment-2334481402
Re: RFR: 8339603: Seal the class hierarchy of Node, Camera, LightBase, Shape, Shape3D
On Fri, 6 Sep 2024 16:01:55 GMT, Andy Goryachev wrote: > "should" and "may" are not the same as "must" and "will". I think this is just suboptimal language, and not a material distinction. You _will_ get a runtime error if you use it in any meaningful way. Anyway, if a developer _really_ wants to create an instance of `Node`, they can use the non-exported `AbstractNode` at their own peril. - PR Comment: https://git.openjdk.org/jfx/pull/1556#issuecomment-2334409097
Re: RFR: 8339603: Seal the class hierarchy of Node, Camera, LightBase, Shape, Shape3D
On Fri, 6 Sep 2024 15:52:17 GMT, Andy Goryachev wrote: > can you point me to where it says one can't extend Node or that it is > verboten? https://github.com/openjdk/jfx/blob/4647367ce4590440fbb85675055225c869502314/modules/javafx.graphics/src/main/java/com/sun/javafx/scene/NodeHelper.java#L81 - PR Comment: https://git.openjdk.org/jfx/pull/1556#issuecomment-2334358365
Re: RFR: 8339603: Seal the class hierarchy of Node, Camera, LightBase, Shape, Shape3D
On Fri, 6 Sep 2024 15:41:45 GMT, Andy Goryachev wrote: > > The fact that you can't extend `Node` is a system invariant > > But I **can**, and the code provided above runs with no exception - in either > `init()` or `start()`, making this change a breaking one. > > Whether such code is meaningful is another story. > > I would vote against this change. But you just proved that you can violate a system invariant. The existing code tries to detect some violations and alerts you that extending Node is not supported by JavaFX. - PR Comment: https://git.openjdk.org/jfx/pull/1556#issuecomment-2334345965
Re: RFR: 8339603: Seal the class hierarchy of Node, Camera, LightBase, Shape, Shape3D
On Thu, 5 Sep 2024 12:01:23 GMT, Michael Strauß wrote: > None of these classes can be extended by user code, and any attempt to do so > will fail at runtime with an exception. For this reason, we can seal the > class hierarchy and remove the run-time checks to turn this into a > compile-time error instead. > > In some cases, `Node` and `Shape` are extended by JavaFX classes in other > modules, preventing those derived classes from being permitted subclasses. A > non-exported `AbstractNode` and `AbstractShape` class is provided just for > these scenarios. Note that introducing a new superclass is a source- and > binary-compatible change (see [JLS ch. > 13](https://docs.oracle.com/javase/specs/jls/se22/html/jls-13.html)). > > I'm not sure if this change requires a CSR, as it doesn't change the > specification in any meaningful way. There can be no valid JavaFX program > that is affected by this change. > If this PR goes forward, it will need a CSR, since it does impact source code > compatibility. As you point out, you can't usefully extend Node today, since > you will get a runtime exception as soon as you try to create an instance of > a subclassed Node, so what this does is move a runtime error to compile time. > That's generally a good thing, but..is it worth doing? We can continue to > discuss. > > /csr In addition to moving a runtime error to compile time, it also documents intent for future JavaFX maintainers. Given that the change is not invasive and has pretty much no downsides, I think it is worth it. - PR Comment: https://git.openjdk.org/jfx/pull/1556#issuecomment-2334328180
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v34]
On Fri, 6 Sep 2024 09:57:58 GMT, John Hendrikx wrote: > Thanks for the refactoring, I think it was a net positive. This looks good to > go. I agree, thanks for the in-depth review! - PR Comment: https://git.openjdk.org/jfx/pull/1522#issuecomment-2333831116
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v30]
On Thu, 5 Sep 2024 20:29:17 GMT, Andy Goryachev wrote:
>> I've added tests for a `null` argument, as well as for an unsupported type
>> for the `BORDER_IMAGE_SOURCE` and `BACKGROUND_IMAGE` mappings.
>>
>> As for your third bullet point, I think you mean `convert` and not
>> `convertBack`. You can only pass "wrong" things into the former, not the
>> latter method. While more tests for `convert` would be good, they shouldn't
>> be done as part of this PR since it is unrelated code.
>
> you are probably right. I was thinking of a rather contrived case like
>
>
> void wrong() {
> SubPropertyConverter c = (SubPropertyConverter)converter;
> c.convertBack(new Object());
> }
>
>
> my question about _documenting_ nullability still stands though.
I've added the appropriate javadoc documentation for that.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1746144798
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v34]
> This PR completes the CSS Transitions story (see #870) by adding
> interpolation support for backgrounds and borders, making them targetable by
> transitions.
>
> `Background` and `Border` objects are deeply immutable, but not
> interpolatable. Consider the following `Background`, which describes the
> background of a `Region`:
>
>
> Background {
> fills = [
> BackgroundFill {
> fill = Color.RED
> }
> ]
> }
>
>
> Since backgrounds are deeply immutable, changing the region's background to
> another color requires the construction of a new `Background`, containing a
> new `BackgroundFill`, containing the new `Color`.
>
> Animating the background color using a CSS transition therefore requires the
> entire Background object graph to be interpolatable in order to generate
> intermediate backgrounds.
>
> More specifically, the following types will now implement `Interpolatable`.
>
> - `Insets`
> - `Background`
> - `BackgroundFill`
> - `BackgroundImage`
> - `BackgroundPosition`
> - `BackgroundSize`
> - `Border`
> - `BorderImage`
> - `BorderStroke`
> - `BorderWidths`
> - `CornerRadii`
> - `Stop`
> - `Paint` and all of its subclasses
> - `Margins` (internal type)
> - `BorderImageSlices` (internal type)
>
> ## Interpolation of composite objects
>
> As of now, only `Color`, `Point2D`, and `Point3D` are interpolatable. Each of
> these classes is an aggregate of `double` values, which are combined using
> linear interpolation. However, many of the new interpolatable classes
> comprise of not only `double` values, but a whole range of other types. This
> requires us to more precisely define what we mean by "interpolation".
>
> Mirroring the CSS specification, the `Interpolatable` interface defines
> several types of component interpolation:
>
> | Interpolation type | Description |
> |---|---|
> | default | Component types that implement `Interpolatable` are interpolated
> by calling the `interpolate(Object, double)}` method. |
> | linear | Two components are combined by linear interpolation such that `t =
> 0` produces the start value, and `t = 1` produces the end value. This
> interpolation type is usually applicable for numeric components. |
> | discrete | If two components cannot be meaningfully combined, the
> intermediate component value is equal to the start value for `t < 0.5` and
> equal to the end value for `t >= 0.5`. |
> | pairwise | Two lists are combined by pairwise interpolation. If the start
> list has fewer elements than the target list, the missing elements are copied
> from the target list. If the start list has more elements than the ...
Michael Strauß has updated the pull request incrementally with one additional
commit since the last revision:
javadoc changes
-
Changes:
- all: https://git.openjdk.org/jfx/pull/1522/files
- new: https://git.openjdk.org/jfx/pull/1522/files/d884a566..75e7d98b
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jfx&pr=1522&range=33
- incr: https://webrevs.openjdk.org/?repo=jfx&pr=1522&range=32-33
Stats: 16 lines in 3 files changed: 13 ins; 0 del; 3 mod
Patch: https://git.openjdk.org/jfx/pull/1522.diff
Fetch: git fetch https://git.openjdk.org/jfx.git pull/1522/head:pull/1522
PR: https://git.openjdk.org/jfx/pull/1522
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v30]
On Thu, 5 Sep 2024 19:26:01 GMT, Andy Goryachev wrote: >> I agree when there's added value, but in this particular case I don't know >> what to add... >> Note that `convertBack()` is exercised in `reconstructedObjectMustBeEqual()`. > > - does `convertBack` need a failing scenario? > - does `convertBack` accept null argument? > - since we are dealing with type erasure and possible quirks of `CssParser`, > would it make sense to test the case when a wrong type is being passed to > `convertBack`? > > Also, a more generic suggestion: in the absence of `@Nullable`, we probably > should specify whether an argument or return value may be null. I've added tests for a `null` argument, as well as for an unsupported type for the `BORDER_IMAGE_SOURCE` and `BACKGROUND_IMAGE` mappings. As for your third bullet point, I think you mean `convert` and not `convertBack`. You can only pass "wrong" things into the former, not the latter method. While more tests for `convert` would be good, they shouldn't be done as part of this PR since it is unrelated code. - PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1746089993
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v33]
> This PR completes the CSS Transitions story (see #870) by adding
> interpolation support for backgrounds and borders, making them targetable by
> transitions.
>
> `Background` and `Border` objects are deeply immutable, but not
> interpolatable. Consider the following `Background`, which describes the
> background of a `Region`:
>
>
> Background {
> fills = [
> BackgroundFill {
> fill = Color.RED
> }
> ]
> }
>
>
> Since backgrounds are deeply immutable, changing the region's background to
> another color requires the construction of a new `Background`, containing a
> new `BackgroundFill`, containing the new `Color`.
>
> Animating the background color using a CSS transition therefore requires the
> entire Background object graph to be interpolatable in order to generate
> intermediate backgrounds.
>
> More specifically, the following types will now implement `Interpolatable`.
>
> - `Insets`
> - `Background`
> - `BackgroundFill`
> - `BackgroundImage`
> - `BackgroundPosition`
> - `BackgroundSize`
> - `Border`
> - `BorderImage`
> - `BorderStroke`
> - `BorderWidths`
> - `CornerRadii`
> - `Stop`
> - `Paint` and all of its subclasses
> - `Margins` (internal type)
> - `BorderImageSlices` (internal type)
>
> ## Interpolation of composite objects
>
> As of now, only `Color`, `Point2D`, and `Point3D` are interpolatable. Each of
> these classes is an aggregate of `double` values, which are combined using
> linear interpolation. However, many of the new interpolatable classes
> comprise of not only `double` values, but a whole range of other types. This
> requires us to more precisely define what we mean by "interpolation".
>
> Mirroring the CSS specification, the `Interpolatable` interface defines
> several types of component interpolation:
>
> | Interpolation type | Description |
> |---|---|
> | default | Component types that implement `Interpolatable` are interpolated
> by calling the `interpolate(Object, double)}` method. |
> | linear | Two components are combined by linear interpolation such that `t =
> 0` produces the start value, and `t = 1` produces the end value. This
> interpolation type is usually applicable for numeric components. |
> | discrete | If two components cannot be meaningfully combined, the
> intermediate component value is equal to the start value for `t < 0.5` and
> equal to the end value for `t >= 0.5`. |
> | pairwise | Two lists are combined by pairwise interpolation. If the start
> list has fewer elements than the target list, the missing elements are copied
> from the target list. If the start list has more elements than the ...
Michael Strauß has updated the pull request incrementally with one additional
commit since the last revision:
added tests
-
Changes:
- all: https://git.openjdk.org/jfx/pull/1522/files
- new: https://git.openjdk.org/jfx/pull/1522/files/9b7f2a53..d884a566
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jfx&pr=1522&range=32
- incr: https://webrevs.openjdk.org/?repo=jfx&pr=1522&range=31-32
Stats: 34 lines in 2 files changed: 26 ins; 8 del; 0 mod
Patch: https://git.openjdk.org/jfx/pull/1522.diff
Fetch: git fetch https://git.openjdk.org/jfx.git pull/1522/head:pull/1522
PR: https://git.openjdk.org/jfx/pull/1522
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v30]
On Thu, 5 Sep 2024 16:22:58 GMT, Andy Goryachev wrote:
>> Michael Strauß has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> changes per review
>
> modules/javafx.graphics/src/test/java/test/javafx/scene/layout/BackgroundConverterTest.java
> line 51:
>
>> 49: import static org.junit.jupiter.api.Assertions.*;
>> 50:
>> 51: public class BackgroundConverterTest {
>
> this is minor, but applicable to other tests:
>
> it would be nice to have a short basic description of the test, for human
> consumption.
>
> Example:
> Tests BackgroundConverter functions
> - from url
> - convert from image
> - equality of reconstructed objects
> (this is just an example)
>
> Also, should it also test `convertBack()` ?
I agree when there's added value, but in this particular case I don't know what
to add...
Note that `convertBack()` is exercised in `reconstructedObjectMustBeEqual()`.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1746050295
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v32]
> This PR completes the CSS Transitions story (see #870) by adding
> interpolation support for backgrounds and borders, making them targetable by
> transitions.
>
> `Background` and `Border` objects are deeply immutable, but not
> interpolatable. Consider the following `Background`, which describes the
> background of a `Region`:
>
>
> Background {
> fills = [
> BackgroundFill {
> fill = Color.RED
> }
> ]
> }
>
>
> Since backgrounds are deeply immutable, changing the region's background to
> another color requires the construction of a new `Background`, containing a
> new `BackgroundFill`, containing the new `Color`.
>
> Animating the background color using a CSS transition therefore requires the
> entire Background object graph to be interpolatable in order to generate
> intermediate backgrounds.
>
> More specifically, the following types will now implement `Interpolatable`.
>
> - `Insets`
> - `Background`
> - `BackgroundFill`
> - `BackgroundImage`
> - `BackgroundPosition`
> - `BackgroundSize`
> - `Border`
> - `BorderImage`
> - `BorderStroke`
> - `BorderWidths`
> - `CornerRadii`
> - `Stop`
> - `Paint` and all of its subclasses
> - `Margins` (internal type)
> - `BorderImageSlices` (internal type)
>
> ## Interpolation of composite objects
>
> As of now, only `Color`, `Point2D`, and `Point3D` are interpolatable. Each of
> these classes is an aggregate of `double` values, which are combined using
> linear interpolation. However, many of the new interpolatable classes
> comprise of not only `double` values, but a whole range of other types. This
> requires us to more precisely define what we mean by "interpolation".
>
> Mirroring the CSS specification, the `Interpolatable` interface defines
> several types of component interpolation:
>
> | Interpolation type | Description |
> |---|---|
> | default | Component types that implement `Interpolatable` are interpolated
> by calling the `interpolate(Object, double)}` method. |
> | linear | Two components are combined by linear interpolation such that `t =
> 0` produces the start value, and `t = 1` produces the end value. This
> interpolation type is usually applicable for numeric components. |
> | discrete | If two components cannot be meaningfully combined, the
> intermediate component value is equal to the start value for `t < 0.5` and
> equal to the end value for `t >= 0.5`. |
> | pairwise | Two lists are combined by pairwise interpolation. If the start
> list has fewer elements than the target list, the missing elements are copied
> from the target list. If the start list has more elements than the ...
Michael Strauß has updated the pull request incrementally with one additional
commit since the last revision:
javadoc changes
-
Changes:
- all: https://git.openjdk.org/jfx/pull/1522/files
- new: https://git.openjdk.org/jfx/pull/1522/files/7328daa9..9b7f2a53
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jfx&pr=1522&range=31
- incr: https://webrevs.openjdk.org/?repo=jfx&pr=1522&range=30-31
Stats: 4 lines in 1 file changed: 3 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jfx/pull/1522.diff
Fetch: git fetch https://git.openjdk.org/jfx.git pull/1522/head:pull/1522
PR: https://git.openjdk.org/jfx/pull/1522
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v30]
On Thu, 5 Sep 2024 16:53:23 GMT, Andy Goryachev wrote:
>> Michael Strauß has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> changes per review
>
> modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 9043:
>
>> 9041: }
>> 9042:
>> 9043: return result;
>
> minor suggestion:
>
> List> subMetadata =
> metadata.getSubProperties();
> if (subMetadata != null) {
> for (int i = 0, max = subMetadata.size(); i < max; ++i) {
> result = collectTransitionTimers(property, result);
> }
> }
> return result;
I usually like flatter code more than nested code, but I don't really mind one
way or the other in this particular situation.
> modules/javafx.graphics/src/main/java/javafx/scene/layout/BackgroundConverter.java
> line 109:
>
>> 107: image = img;
>> 108: } else {
>> 109: throw new
>> IllegalArgumentException("convertedValues");
>
> would it make sense to make this exception message more meaningful to help
> diagnose the issue? for example, what is expected and what is encountered.
>
> (this comment applies to every other converter)
I've reworded the message to include the unexpected type. Note that the
existing code will just throw `ClassCastException` in similar circumstances, so
we probably don't need to get super detailed for this particular situation. If
we want to have really detailed exception messages, the entire method should be
refactored.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1746024861
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1746022540
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v31]
> This PR completes the CSS Transitions story (see #870) by adding
> interpolation support for backgrounds and borders, making them targetable by
> transitions.
>
> `Background` and `Border` objects are deeply immutable, but not
> interpolatable. Consider the following `Background`, which describes the
> background of a `Region`:
>
>
> Background {
> fills = [
> BackgroundFill {
> fill = Color.RED
> }
> ]
> }
>
>
> Since backgrounds are deeply immutable, changing the region's background to
> another color requires the construction of a new `Background`, containing a
> new `BackgroundFill`, containing the new `Color`.
>
> Animating the background color using a CSS transition therefore requires the
> entire Background object graph to be interpolatable in order to generate
> intermediate backgrounds.
>
> More specifically, the following types will now implement `Interpolatable`.
>
> - `Insets`
> - `Background`
> - `BackgroundFill`
> - `BackgroundImage`
> - `BackgroundPosition`
> - `BackgroundSize`
> - `Border`
> - `BorderImage`
> - `BorderStroke`
> - `BorderWidths`
> - `CornerRadii`
> - `Stop`
> - `Paint` and all of its subclasses
> - `Margins` (internal type)
> - `BorderImageSlices` (internal type)
>
> ## Interpolation of composite objects
>
> As of now, only `Color`, `Point2D`, and `Point3D` are interpolatable. Each of
> these classes is an aggregate of `double` values, which are combined using
> linear interpolation. However, many of the new interpolatable classes
> comprise of not only `double` values, but a whole range of other types. This
> requires us to more precisely define what we mean by "interpolation".
>
> Mirroring the CSS specification, the `Interpolatable` interface defines
> several types of component interpolation:
>
> | Interpolation type | Description |
> |---|---|
> | default | Component types that implement `Interpolatable` are interpolated
> by calling the `interpolate(Object, double)}` method. |
> | linear | Two components are combined by linear interpolation such that `t =
> 0` produces the start value, and `t = 1` produces the end value. This
> interpolation type is usually applicable for numeric components. |
> | discrete | If two components cannot be meaningfully combined, the
> intermediate component value is equal to the start value for `t < 0.5` and
> equal to the end value for `t >= 0.5`. |
> | pairwise | Two lists are combined by pairwise interpolation. If the start
> list has fewer elements than the target list, the missing elements are copied
> from the target list. If the start list has more elements than the ...
Michael Strauß has updated the pull request incrementally with one additional
commit since the last revision:
address review comments
-
Changes:
- all: https://git.openjdk.org/jfx/pull/1522/files
- new: https://git.openjdk.org/jfx/pull/1522/files/97ee29e2..7328daa9
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jfx&pr=1522&range=30
- incr: https://webrevs.openjdk.org/?repo=jfx&pr=1522&range=29-30
Stats: 4 lines in 3 files changed: 0 ins; 0 del; 4 mod
Patch: https://git.openjdk.org/jfx/pull/1522.diff
Fetch: git fetch https://git.openjdk.org/jfx.git pull/1522/head:pull/1522
PR: https://git.openjdk.org/jfx/pull/1522
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v30]
On Thu, 5 Sep 2024 16:18:34 GMT, Andy Goryachev wrote:
>> Michael Strauß has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> changes per review
>
> modules/javafx.graphics/src/test/java/test/javafx/css/StyleableProperty_transition_Test.java
> line 198:
>
>> 196: void setup() {
>> 197: toolkit = (StubToolkit)Toolkit.getToolkit();
>> 198: scene = new Scene(new Group());
>
> should we `hide` stage in `@AfterEach` ?
> probably applies to other tests
I'm already doing this in `@AfterEach` by calling `stage.close()`, which is
equivalent to `hide()`.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1746017559
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v30]
On Thu, 5 Sep 2024 16:42:38 GMT, Andy Goryachev wrote:
>> Michael Strauß has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> changes per review
>
> modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 639:
>
>> 637: var definitions = node.miscProperties != null ?
>> node.miscProperties.transitionDefinitions : null;
>> 638: if (definitions == null) {
>> 639: definitions = node.new
>> TransitionDefinitionCollection();
>
> TransitionDefinitionCollection can be a static class, can it?
Yes, good catch!
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1746006803
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v30]
On Thu, 5 Sep 2024 15:38:20 GMT, Andy Goryachev wrote:
>> Michael Strauß has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> changes per review
>
> modules/javafx.graphics/src/main/java/javafx/scene/layout/Border.java line
> 418:
>
>> 416: * {@inheritDoc}
>> 417: *
>> 418: * @throws NullPointerException {@inheritDoc}
>
> @kevinrushforth : is there a bug in javadoc that requires @throws repeated
> here? without it, the base class @throws is not shown.
This seems to be intentional:
https://docs.oracle.com/javase/7/docs/technotes/tools/solaris/javadoc.html#inheritingcomments
_When a `@throws` tag for a particular exception is missing, the `@throws` tag
is copied only if that exception is declared._
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1746004046
Re: RFR: 8339603: Seal the class hierarchy of Node, Camera, LightBase, Shape, Shape3D
On Thu, 5 Sep 2024 17:24:50 GMT, Andy Goryachev wrote:
> What is the benefit of this change?
It enforces a system invariant at compile-time instead of run-time.
> Also, this change is breaking. One can currently create a Node:
>
> ```
> public class CanCreateNode extends Application {
> @Override
> public void init() {
> Node n1 = new Node() {
> };
> class Yo extends Node {
> }
> Yo n2 = new Yo();
> }
> ```
You can't do anything meaningful with this node. As soon as you add it to a
scene graph, you'll get a runtime exception. The fact that you can't extend
`Node` is a system invariant, finding pathological situations in which you
won't get an exception is just a loophole.
-
PR Comment: https://git.openjdk.org/jfx/pull/1556#issuecomment-2332310139
RFR: 8339603: Seal the class hierarchy of Node, Camera, LightBase, Shape, Shape3D
None of these classes can be extended by user code, and any attempt to do so will fail at runtime with an exception. For this reason, we can seal the class hierarchy and remove the run-time checks to turn this into a compile-time error instead. In some cases, `Node` and `Shape` are extended by JavaFX classes in other modules, preventing those derived classes from being permitted subclasses. A non-exported `AbstractNode` and `AbstractShape` class is provided just for these scenarios. Note that introducing a new superclass is a source- and binary-compatible change (see [JLS ch. 13](https://docs.oracle.com/javase/specs/jls/se22/html/jls-13.html)). I'm not sure if this change requires a CSR, as it doesn't change the specification in any meaningful way. There can be no valid JavaFX program that is affected by this change. - Commit messages: - Seal Node, Camera, LightBase, Shape, Shape3D Changes: https://git.openjdk.org/jfx/pull/1556/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1556&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8339603 Stats: 190 lines in 46 files changed: 89 ins; 18 del; 83 mod Patch: https://git.openjdk.org/jfx/pull/1556.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1556/head:pull/1556 PR: https://git.openjdk.org/jfx/pull/1556
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v29]
On Wed, 4 Sep 2024 21:12:00 GMT, Andy Goryachev wrote:
>> Michael Strauß has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> refactoring
>
> build.gradle line 4378:
>
>> 4376: options.tags("implSpec:a:Implementation Requirements:")
>> 4377: options.tags("implNote:a:Implementation Note:")
>> 4378: options.tags("interpolationType:a:Interpolation type:")
>
> should this use title case?
Changed as suggested. The style across the documentation is a bit inconsistent,
but seems to prefer title case.
> modules/javafx.base/src/test/java/test/util/ReflectionUtils.java line 62:
>
>> 60: }
>> 61:
>> 62: throw new AssertionError("Field not found");
>
> suggestion:
> `throw new AssertionError("Field not found:" + fieldName);`
Changed as suggested.
> modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 9041:
>
>> 9039: }
>> 9040:
>> 9041: private TransitionTimerCollection transitionTimers;
>
> Would it make more sense to move this to the `miscProperties`?
>
> I'd expect there will typically be a few nodes with transition times, but
> thousands of Nodes in total.
Sure, we can do that.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1744655402
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1744655501
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1744655604
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v30]
> This PR completes the CSS Transitions story (see #870) by adding
> interpolation support for backgrounds and borders, making them targetable by
> transitions.
>
> `Background` and `Border` objects are deeply immutable, but not
> interpolatable. Consider the following `Background`, which describes the
> background of a `Region`:
>
>
> Background {
> fills = [
> BackgroundFill {
> fill = Color.RED
> }
> ]
> }
>
>
> Since backgrounds are deeply immutable, changing the region's background to
> another color requires the construction of a new `Background`, containing a
> new `BackgroundFill`, containing the new `Color`.
>
> Animating the background color using a CSS transition therefore requires the
> entire Background object graph to be interpolatable in order to generate
> intermediate backgrounds.
>
> More specifically, the following types will now implement `Interpolatable`.
>
> - `Insets`
> - `Background`
> - `BackgroundFill`
> - `BackgroundImage`
> - `BackgroundPosition`
> - `BackgroundSize`
> - `Border`
> - `BorderImage`
> - `BorderStroke`
> - `BorderWidths`
> - `CornerRadii`
> - `Stop`
> - `Paint` and all of its subclasses
> - `Margins` (internal type)
> - `BorderImageSlices` (internal type)
>
> ## Interpolation of composite objects
>
> As of now, only `Color`, `Point2D`, and `Point3D` are interpolatable. Each of
> these classes is an aggregate of `double` values, which are combined using
> linear interpolation. However, many of the new interpolatable classes
> comprise of not only `double` values, but a whole range of other types. This
> requires us to more precisely define what we mean by "interpolation".
>
> Mirroring the CSS specification, the `Interpolatable` interface defines
> several types of component interpolation:
>
> | Interpolation type | Description |
> |---|---|
> | default | Component types that implement `Interpolatable` are interpolated
> by calling the `interpolate(Object, double)}` method. |
> | linear | Two components are combined by linear interpolation such that `t =
> 0` produces the start value, and `t = 1` produces the end value. This
> interpolation type is usually applicable for numeric components. |
> | discrete | If two components cannot be meaningfully combined, the
> intermediate component value is equal to the start value for `t < 0.5` and
> equal to the end value for `t >= 0.5`. |
> | pairwise | Two lists are combined by pairwise interpolation. If the start
> list has fewer elements than the target list, the missing elements are copied
> from the target list. If the start list has more elements than the ...
Michael Strauß has updated the pull request incrementally with one additional
commit since the last revision:
changes per review
-
Changes:
- all: https://git.openjdk.org/jfx/pull/1522/files
- new: https://git.openjdk.org/jfx/pull/1522/files/ba375b57..97ee29e2
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jfx&pr=1522&range=29
- incr: https://webrevs.openjdk.org/?repo=jfx&pr=1522&range=28-29
Stats: 26 lines in 3 files changed: 12 ins; 4 del; 10 mod
Patch: https://git.openjdk.org/jfx/pull/1522.diff
Fetch: git fetch https://git.openjdk.org/jfx.git pull/1522/head:pull/1522
PR: https://git.openjdk.org/jfx/pull/1522
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v29]
On Tue, 3 Sep 2024 17:58:40 GMT, Nir Lisker wrote: > Isn't `CancellationToken` a simplified `Subscription`? Won't the additional > methods on the latter be of use here? Yes it is. I thought about using `Subscription`, but the method name `unsubscribe()` doesn't seem to match what is happening. Maybe we can add something like `Cancellable` in the future, if at any point in time we require similar semantics. - PR Comment: https://git.openjdk.org/jfx/pull/1522#issuecomment-2327121283
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v29]
On Tue, 3 Sep 2024 17:09:51 GMT, Michael Strauß wrote:
>> This PR completes the CSS Transitions story (see #870) by adding
>> interpolation support for backgrounds and borders, making them targetable by
>> transitions.
>>
>> `Background` and `Border` objects are deeply immutable, but not
>> interpolatable. Consider the following `Background`, which describes the
>> background of a `Region`:
>>
>>
>> Background {
>> fills = [
>> BackgroundFill {
>> fill = Color.RED
>> }
>> ]
>> }
>>
>>
>> Since backgrounds are deeply immutable, changing the region's background to
>> another color requires the construction of a new `Background`, containing a
>> new `BackgroundFill`, containing the new `Color`.
>>
>> Animating the background color using a CSS transition therefore requires the
>> entire Background object graph to be interpolatable in order to generate
>> intermediate backgrounds.
>>
>> More specifically, the following types will now implement `Interpolatable`.
>>
>> - `Insets`
>> - `Background`
>> - `BackgroundFill`
>> - `BackgroundImage`
>> - `BackgroundPosition`
>> - `BackgroundSize`
>> - `Border`
>> - `BorderImage`
>> - `BorderStroke`
>> - `BorderWidths`
>> - `CornerRadii`
>> - `Stop`
>> - `Paint` and all of its subclasses
>> - `Margins` (internal type)
>> - `BorderImageSlices` (internal type)
>>
>> ## Interpolation of composite objects
>>
>> As of now, only `Color`, `Point2D`, and `Point3D` are interpolatable. Each
>> of these classes is an aggregate of `double` values, which are combined
>> using linear interpolation. However, many of the new interpolatable classes
>> comprise of not only `double` values, but a whole range of other types. This
>> requires us to more precisely define what we mean by "interpolation".
>>
>> Mirroring the CSS specification, the `Interpolatable` interface defines
>> several types of component interpolation:
>>
>> | Interpolation type | Description |
>> |---|---|
>> | default | Component types that implement `Interpolatable` are interpolated
>> by calling the `interpolate(Object, double)}` method. |
>> | linear | Two components are combined by linear interpolation such that `t
>> = 0` produces the start value, and `t = 1` produces the end value. This
>> interpolation type is usually applicable for numeric components. |
>> | discrete | If two components cannot be meaningfully combined, the
>> intermediate component value is equal to the start value for `t < 0.5` and
>> equal to the end value for `t >= 0.5`. |
>> | pairwise | Two lists are combined by pairwise interpolation. If the start
>> list has fewer elements than the target list, the...
>
> Michael Strauß has updated the pull request incrementally with one additional
> commit since the last revision:
>
> refactoring
I've made some changes as follows:
1. `TransitionTimer.run` returns a `CancellationToken` object instead of a
timer reference, which is only used to cancel the timer. This removes all
usages of `TransitionTimer` from `TransitionMediator` (except for the single
call to the `run` method).
2. The `cancel` method doesn't use a `forceStop` flag any more, which also
removes tracking of `updating` flags from implementations. Instead,
`TransitionMediator.onUpdate` now directly calls the `super.set` method to
avoid cancelling itself.
3. The conditional clearing of the `mediator` field is removed, it is now
cleared unconditionally. To make this work, `TransitionTimer` internally
detects situations when the field shouldn't be cleared, and then elides the
call to `TransitionMediator.onStop`. This avoids having implementations follow
a bespoke clearing protocol.
4. The methods that effectively stop a transition timer have been reduced to
`stop`, `complete`, and `interrupt`, each having a precise definition.
-
PR Comment: https://git.openjdk.org/jfx/pull/1522#issuecomment-2327046946
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v29]
> This PR completes the CSS Transitions story (see #870) by adding
> interpolation support for backgrounds and borders, making them targetable by
> transitions.
>
> `Background` and `Border` objects are deeply immutable, but not
> interpolatable. Consider the following `Background`, which describes the
> background of a `Region`:
>
>
> Background {
> fills = [
> BackgroundFill {
> fill = Color.RED
> }
> ]
> }
>
>
> Since backgrounds are deeply immutable, changing the region's background to
> another color requires the construction of a new `Background`, containing a
> new `BackgroundFill`, containing the new `Color`.
>
> Animating the background color using a CSS transition therefore requires the
> entire Background object graph to be interpolatable in order to generate
> intermediate backgrounds.
>
> More specifically, the following types will now implement `Interpolatable`.
>
> - `Insets`
> - `Background`
> - `BackgroundFill`
> - `BackgroundImage`
> - `BackgroundPosition`
> - `BackgroundSize`
> - `Border`
> - `BorderImage`
> - `BorderStroke`
> - `BorderWidths`
> - `CornerRadii`
> - `Stop`
> - `Paint` and all of its subclasses
> - `Margins` (internal type)
> - `BorderImageSlices` (internal type)
>
> ## Interpolation of composite objects
>
> As of now, only `Color`, `Point2D`, and `Point3D` are interpolatable. Each of
> these classes is an aggregate of `double` values, which are combined using
> linear interpolation. However, many of the new interpolatable classes
> comprise of not only `double` values, but a whole range of other types. This
> requires us to more precisely define what we mean by "interpolation".
>
> Mirroring the CSS specification, the `Interpolatable` interface defines
> several types of component interpolation:
>
> | Interpolation type | Description |
> |---|---|
> | default | Component types that implement `Interpolatable` are interpolated
> by calling the `interpolate(Object, double)}` method. |
> | linear | Two components are combined by linear interpolation such that `t =
> 0` produces the start value, and `t = 1` produces the end value. This
> interpolation type is usually applicable for numeric components. |
> | discrete | If two components cannot be meaningfully combined, the
> intermediate component value is equal to the start value for `t < 0.5` and
> equal to the end value for `t >= 0.5`. |
> | pairwise | Two lists are combined by pairwise interpolation. If the start
> list has fewer elements than the target list, the missing elements are copied
> from the target list. If the start list has more elements than the ...
Michael Strauß has updated the pull request incrementally with one additional
commit since the last revision:
refactoring
-
Changes:
- all: https://git.openjdk.org/jfx/pull/1522/files
- new: https://git.openjdk.org/jfx/pull/1522/files/74b23c43..ba375b57
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jfx&pr=1522&range=28
- incr: https://webrevs.openjdk.org/?repo=jfx&pr=1522&range=27-28
Stats: 336 lines in 10 files changed: 78 ins; 180 del; 78 mod
Patch: https://git.openjdk.org/jfx/pull/1522.diff
Fetch: git fetch https://git.openjdk.org/jfx.git pull/1522/head:pull/1522
PR: https://git.openjdk.org/jfx/pull/1522
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v28]
On Tue, 3 Sep 2024 13:08:01 GMT, John Hendrikx wrote: >> Michael Strauß has updated the pull request incrementally with one >> additional commit since the last revision: >> >> typo > > modules/javafx.graphics/src/main/java/javafx/css/StyleableLongProperty.java > line 140: > >> 138: long diff = endValue - startValue; >> 139: long result = startValue + Math.round(progress * diff); >> 140: set(progress < 1 ? Utils.clamp(startValue, result, >> endValue) : endValue); > > `clamp` won't solve that if `progress = 1.0` that the calculation may result > in a value less than `endValue`. You need both the `clamp` and a special > case: > > set(progress == 1.0 ? endValue : progress < 1 ? Utils.clamp(startValue, > result, endValue) : endValue); > > Use `==` or `>=` I don't know what's better. > > I'm assuming that it is important that `endValue` is returned when `progress > = 1.0` I don't understand this. If progress != 1, then it must be < 1 by definition. It can never be > 1. So the current implementation will just ignore the result of the computation for the special case (result = 1): set(progress < 1 ? Utils.clamp(startValue, result, endValue) : endValue); - PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1742098330
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v26]
On Tue, 3 Sep 2024 02:27:10 GMT, John Hendrikx wrote: >> Michael Strauß has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains 48 additional >> commits since the last revision: >> >> - Merge branch 'master' into feature/interpolatable >> - remove StyleConverter.WithReconstructionSupport >> - fix line separators >> - StyleableStringProperty should be transitionable >> - non-interpolatable values should always transition discretely >> - only call get() when necessary >> - add more documentation >> - replace reconstruction annotation with interface >> - interpolate integers in real number space >> - replace StyleConverter.SupportsDeconstruction interface with annotation >> - ... and 38 more: https://git.openjdk.org/jfx/compare/869f534a...2337ca98 > > modules/javafx.graphics/src/main/java/javafx/animation/Interpolatable.java > line 57: > >> 55: * >> 56: * >> 57: * (see >> prose) > > This could just be a comment below the table? Not sure what "(see prose)" > means here. Yes, good idea. I've moved it to a comment below the table. > modules/javafx.graphics/src/main/java/javafx/css/TransitionEvent.java line > 112: > >> 110: super(Objects.requireNonNull(eventType, "eventType cannot be >> null")); >> 111: this.property = Objects.requireNonNull(property, "property >> cannot be null"); >> 112: this.propertyName = Objects.requireNonNull(propertyName, >> "propertyName cannot be null"); > > Can this be different from `property.getCssMetadata().getProperty()`? What > does it mean if it is? It can be different. For example, a transition targeting `-fx-border-color` will show `property.getCssMetaData().getProperty()` == `-fx-region-border`. I've added a section to the javadoc explaining the difference. - PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1741976433 PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1741982433
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v28]
> This PR completes the CSS Transitions story (see #870) by adding
> interpolation support for backgrounds and borders, making them targetable by
> transitions.
>
> `Background` and `Border` objects are deeply immutable, but not
> interpolatable. Consider the following `Background`, which describes the
> background of a `Region`:
>
>
> Background {
> fills = [
> BackgroundFill {
> fill = Color.RED
> }
> ]
> }
>
>
> Since backgrounds are deeply immutable, changing the region's background to
> another color requires the construction of a new `Background`, containing a
> new `BackgroundFill`, containing the new `Color`.
>
> Animating the background color using a CSS transition therefore requires the
> entire Background object graph to be interpolatable in order to generate
> intermediate backgrounds.
>
> More specifically, the following types will now implement `Interpolatable`.
>
> - `Insets`
> - `Background`
> - `BackgroundFill`
> - `BackgroundImage`
> - `BackgroundPosition`
> - `BackgroundSize`
> - `Border`
> - `BorderImage`
> - `BorderStroke`
> - `BorderWidths`
> - `CornerRadii`
> - `Stop`
> - `Paint` and all of its subclasses
> - `Margins` (internal type)
> - `BorderImageSlices` (internal type)
>
> ## Interpolation of composite objects
>
> As of now, only `Color`, `Point2D`, and `Point3D` are interpolatable. Each of
> these classes is an aggregate of `double` values, which are combined using
> linear interpolation. However, many of the new interpolatable classes
> comprise of not only `double` values, but a whole range of other types. This
> requires us to more precisely define what we mean by "interpolation".
>
> Mirroring the CSS specification, the `Interpolatable` interface defines
> several types of component interpolation:
>
> | Interpolation type | Description |
> |---|---|
> | default | Component types that implement `Interpolatable` are interpolated
> by calling the `interpolate(Object, double)}` method. |
> | linear | Two components are combined by linear interpolation such that `t =
> 0` produces the start value, and `t = 1` produces the end value. This
> interpolation type is usually applicable for numeric components. |
> | discrete | If two components cannot be meaningfully combined, the
> intermediate component value is equal to the start value for `t < 0.5` and
> equal to the end value for `t >= 0.5`. |
> | pairwise | Two lists are combined by pairwise interpolation. If the start
> list has fewer elements than the target list, the missing elements are copied
> from the target list. If the start list has more elements than the ...
Michael Strauß has updated the pull request incrementally with one additional
commit since the last revision:
typo
-
Changes:
- all: https://git.openjdk.org/jfx/pull/1522/files
- new: https://git.openjdk.org/jfx/pull/1522/files/6218e9ab..74b23c43
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jfx&pr=1522&range=27
- incr: https://webrevs.openjdk.org/?repo=jfx&pr=1522&range=26-27
Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod
Patch: https://git.openjdk.org/jfx/pull/1522.diff
Fetch: git fetch https://git.openjdk.org/jfx.git pull/1522/head:pull/1522
PR: https://git.openjdk.org/jfx/pull/1522
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v27]
> This PR completes the CSS Transitions story (see #870) by adding
> interpolation support for backgrounds and borders, making them targetable by
> transitions.
>
> `Background` and `Border` objects are deeply immutable, but not
> interpolatable. Consider the following `Background`, which describes the
> background of a `Region`:
>
>
> Background {
> fills = [
> BackgroundFill {
> fill = Color.RED
> }
> ]
> }
>
>
> Since backgrounds are deeply immutable, changing the region's background to
> another color requires the construction of a new `Background`, containing a
> new `BackgroundFill`, containing the new `Color`.
>
> Animating the background color using a CSS transition therefore requires the
> entire Background object graph to be interpolatable in order to generate
> intermediate backgrounds.
>
> More specifically, the following types will now implement `Interpolatable`.
>
> - `Insets`
> - `Background`
> - `BackgroundFill`
> - `BackgroundImage`
> - `BackgroundPosition`
> - `BackgroundSize`
> - `Border`
> - `BorderImage`
> - `BorderStroke`
> - `BorderWidths`
> - `CornerRadii`
> - `Stop`
> - `Paint` and all of its subclasses
> - `Margins` (internal type)
> - `BorderImageSlices` (internal type)
>
> ## Interpolation of composite objects
>
> As of now, only `Color`, `Point2D`, and `Point3D` are interpolatable. Each of
> these classes is an aggregate of `double` values, which are combined using
> linear interpolation. However, many of the new interpolatable classes
> comprise of not only `double` values, but a whole range of other types. This
> requires us to more precisely define what we mean by "interpolation".
>
> Mirroring the CSS specification, the `Interpolatable` interface defines
> several types of component interpolation:
>
> | Interpolation type | Description |
> |---|---|
> | default | Component types that implement `Interpolatable` are interpolated
> by calling the `interpolate(Object, double)}` method. |
> | linear | Two components are combined by linear interpolation such that `t =
> 0` produces the start value, and `t = 1` produces the end value. This
> interpolation type is usually applicable for numeric components. |
> | discrete | If two components cannot be meaningfully combined, the
> intermediate component value is equal to the start value for `t < 0.5` and
> equal to the end value for `t >= 0.5`. |
> | pairwise | Two lists are combined by pairwise interpolation. If the start
> list has fewer elements than the target list, the missing elements are copied
> from the target list. If the start list has more elements than the ...
Michael Strauß has updated the pull request incrementally with one additional
commit since the last revision:
address review comments
-
Changes:
- all: https://git.openjdk.org/jfx/pull/1522/files
- new: https://git.openjdk.org/jfx/pull/1522/files/2337ca98..6218e9ab
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jfx&pr=1522&range=26
- incr: https://webrevs.openjdk.org/?repo=jfx&pr=1522&range=25-26
Stats: 17 lines in 3 files changed: 12 ins; 4 del; 1 mod
Patch: https://git.openjdk.org/jfx/pull/1522.diff
Fetch: git fetch https://git.openjdk.org/jfx.git pull/1522/head:pull/1522
PR: https://git.openjdk.org/jfx/pull/1522
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v26]
On Mon, 2 Sep 2024 16:31:53 GMT, John Hendrikx wrote: > I've been looking at `TransitionMediator`s and `TransitionTimer`s -- these > two classes seem to always go together. There is some nasty bookkeeping going > on that requires clearing a reference of the mediator. I've had some success > just eliminating the Mediator code and storing the timer directly. Yes, having an adapter between the styleable property implementation and the transition timer was an improvement that [you suggested](https://github.com/openjdk/jfx/pull/870#discussion_r1392551399); it seems like we've come full circle. Sure, clearing the mediator reference is not great, but it is a tiny and isolated part (as in, this implementation detail doesn't leak out of the class in which it is used). I wouldn't call this a serious problem of the design. > To make reversal detection work, you may need to pass something more to > `TransitionTimer.run` -- however, see Q1 -- is this something FX has decided > to provide that is not part of CSS? Should we consider making this smarter > (Q2)? Reversal detection requires each styleable property implementation to be able to update their reversing-adjusted start value. This interaction is one of the purposes of `TransitionMediator`. Should we make this smarter as prescribed by the specification? I don't think so. CSS transitions is not the right tool if you need more than simple animated state transitions. I want to stress the fact that these are _CSS_ transitions, not general-purpose transitions that can be triggered programmatically. This means that, while in theory there are multi-state transitions, these _always_ involve multiple pseudo-class states (for example, scenarios involving both :hover and :pressed). It is not possible to accumulate pseudo-class states in a way that would be similar to your Q2 scenario, where you programmatically set different values. > I could imagine the reversal detection should be working with a start value > that only resets on animation completion (ie. when `tt` is set back to > `null`). The reversal should then be calculated based on how much time we've > been animating already since `tt` became non-null (transition time minus time > spent animating already). If this is negative, then animate normally. Is it > positive, then shorten the transition. I don't think this would support a scenario where a transition is interrupted many times in a row, for example by moving the cursor across a button repeatedly, such that the :hover state is set, unset, then set again, and so on, and none of the transitions are allowed to run to completion. Pretty soon, the time already spent animating will exceed the total time of any single transition, which will prevent faster reversing for all future transitions. This is not what we would want to see. As for your ideas about refactoring the code to get rid of `TransitionMediator`, I'd like to understand a bit more context because this seems pretty random to me. Sure, I can imagine any number of implementations that would do the job, but what is your main motivation for rejecting the current implementation in favor for another approach? The purpose of `TransitionMediator` and `TransitionTimer` seem sufficiently clear-cut to me, and the implementation is reasonably clean given the complex problem being solved. I don't see an _obvious_ shortcoming of the current implementation. - PR Comment: https://git.openjdk.org/jfx/pull/1522#issuecomment-2325124450
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v26]
On Mon, 2 Sep 2024 15:37:38 GMT, John Hendrikx wrote: > Q1: I see a lot of work in the code to handle reverse transitions. Is this > actually part of the CSS specification? Yes, it's a sizable chunk of the specification: https://www.w3.org/TR/css-transitions-1/#starting > Q2: Reversal detection seems very limited. If I change a value multiple times > within the duration of the animation, then reset it to its original value, > then I think the reversal code will not trigger. As an example, let's say I'm > animating scrolling. I'm at the top. I press page down once and then press > home. The value changes for example from 0 -> 20 -> 0 -- this triggers the > reversal code, and the animated scrolling will quickly return to its top > position. However, if I do 0 -> 20 -> 40 -> 0 (page down, page down, home), > then I think the reversal is not detected and I'm left with a very slow > return to the top position. There doesn't seem to be a one-size-fits-all solution for faster transition reversal. The CSS specification [acknowledges](https://www.w3.org/TR/css-transitions-1/#reversing) this: _Note that these rules do not fully address the problem for transition patterns that involve more than two states._ The algorithm as prescribed by the specification is one of several algorithms that were considered by the W3C working group back in 2013. It is clearly tuned for two-state application scenarios, and might simply not be the right solution for your problem. I've thought a lot about the problem, and while it may be possible to come up with an algorithm that covers more edge cases, that takes a huge toll on the complexity/maintainability budget. > > Q3: How does the user do calculations for properties that are being > interpolated? Let's say I have a simple system where the `+` and `-` change a > value by +10 and -10. So I press `+`; I read the current value, add 10 and > set it (say from 0 to 10). Now I press `+` again. Reading the current value > would get me some intermediate animated value. Adding 10 to this is not what > the user would expect (they would expect to go to 20, not to 15 if the > animation was only halfway completed). > > As an example, I've implemented (without CSS transitions) a smooth scrolling > system; this system tracks the target value to do the scrolling. Pressing 3x > page down in a row will go 3 pages down, no matter what the state of the > animation. With CSS transitions, should the programmer be aware of the > interpolation (which may be added later in CSS) and track their own "target" > value to make this work as you'd expect? I don't think this would work at all, because you're setting the value programmatically. You'll only get a transition if you let the CSS system set the value. If you do this, and then query the value of the property under transition, you will get an intermediate value. This simply wouldn't work otherwise, considering how JavaFX properties work. We can't have properties change their value over time, but when you look at them programmatically, they already have the target value. This behavior is also consistent with the [specification](https://www.w3.org/TR/css-transitions-1/#transitions): _Transitions are a presentational effect. The computed value of a property transitions over time from the old value to the new value. Therefore if a script queries the computed value of a property (or other data depending on it) as it is transitioning, it will see an intermediate value that represents the current animated value of the property._ - PR Comment: https://git.openjdk.org/jfx/pull/1522#issuecomment-2325065092
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v16]
On Sun, 4 Aug 2024 22:56:17 GMT, John Hendrikx wrote: >> Michael Strauß has updated the pull request incrementally with one >> additional commit since the last revision: >> >> use HashMap.newHashMap() > > This is still a large PR, and I do want to help reviewing it, I will do my > best to look it over further this week. @hjohn @andy-goryachev-oracle I think that I've addressed all comments so far. To summarize the aspects that will not be covered in this PR: Interpolation for fonts and a wholesale change of cssref (adding a new column for all properties) will be done separately. The discussion about a place to collect JEP-style design documents is not resolved yet, and will also be done separately. Do you think there is anything left preventing this PR from moving forward? - PR Comment: https://git.openjdk.org/jfx/pull/1522#issuecomment-2324600126
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v26]
> This PR completes the CSS Transitions story (see #870) by adding
> interpolation support for backgrounds and borders, making them targetable by
> transitions.
>
> `Background` and `Border` objects are deeply immutable, but not
> interpolatable. Consider the following `Background`, which describes the
> background of a `Region`:
>
>
> Background {
> fills = [
> BackgroundFill {
> fill = Color.RED
> }
> ]
> }
>
>
> Since backgrounds are deeply immutable, changing the region's background to
> another color requires the construction of a new `Background`, containing a
> new `BackgroundFill`, containing the new `Color`.
>
> Animating the background color using a CSS transition therefore requires the
> entire Background object graph to be interpolatable in order to generate
> intermediate backgrounds.
>
> More specifically, the following types will now implement `Interpolatable`.
>
> - `Insets`
> - `Background`
> - `BackgroundFill`
> - `BackgroundImage`
> - `BackgroundPosition`
> - `BackgroundSize`
> - `Border`
> - `BorderImage`
> - `BorderStroke`
> - `BorderWidths`
> - `CornerRadii`
> - `Stop`
> - `Paint` and all of its subclasses
> - `Margins` (internal type)
> - `BorderImageSlices` (internal type)
>
> ## Interpolation of composite objects
>
> As of now, only `Color`, `Point2D`, and `Point3D` are interpolatable. Each of
> these classes is an aggregate of `double` values, which are combined using
> linear interpolation. However, many of the new interpolatable classes
> comprise of not only `double` values, but a whole range of other types. This
> requires us to more precisely define what we mean by "interpolation".
>
> Mirroring the CSS specification, the `Interpolatable` interface defines
> several types of component interpolation:
>
> | Interpolation type | Description |
> |---|---|
> | default | Component types that implement `Interpolatable` are interpolated
> by calling the `interpolate(Object, double)}` method. |
> | linear | Two components are combined by linear interpolation such that `t =
> 0` produces the start value, and `t = 1` produces the end value. This
> interpolation type is usually applicable for numeric components. |
> | discrete | If two components cannot be meaningfully combined, the
> intermediate component value is equal to the start value for `t < 0.5` and
> equal to the end value for `t >= 0.5`. |
> | pairwise | Two lists are combined by pairwise interpolation. If the start
> list has fewer elements than the target list, the missing elements are copied
> from the target list. If the start list has more elements than the ...
Michael Strauß has updated the pull request with a new target base due to a
merge or a rebase. The incremental webrev excludes the unrelated changes
brought in by the merge/rebase. The pull request contains 48 additional commits
since the last revision:
- Merge branch 'master' into feature/interpolatable
- remove StyleConverter.WithReconstructionSupport
- fix line separators
- StyleableStringProperty should be transitionable
- non-interpolatable values should always transition discretely
- only call get() when necessary
- add more documentation
- replace reconstruction annotation with interface
- interpolate integers in real number space
- replace StyleConverter.SupportsDeconstruction interface with annotation
- ... and 38 more: https://git.openjdk.org/jfx/compare/a8e9e109...2337ca98
-
Changes:
- all: https://git.openjdk.org/jfx/pull/1522/files
- new: https://git.openjdk.org/jfx/pull/1522/files/9bdea0a4..2337ca98
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jfx&pr=1522&range=25
- incr: https://webrevs.openjdk.org/?repo=jfx&pr=1522&range=24-25
Stats: 28890 lines in 624 files changed: 15229 ins; 7889 del; 5772 mod
Patch: https://git.openjdk.org/jfx/pull/1522.diff
Fetch: git fetch https://git.openjdk.org/jfx.git pull/1522/head:pull/1522
PR: https://git.openjdk.org/jfx/pull/1522
Re: Bug: Times passed to AnimationTimer should not fluctuate
Yes, that makes sense. In any case, we shouldn't be using a system
timer, but simply record the timestamp at v-sync, and then pass this
precise timestamp to all AnimationTimers. It shouldn't matter when
AnimationTimers are invoked between frames, as long as the timestamp
corresponds to the v-sync signal. (Well, unless the timer callback
measures its own time, which it shouldn't do.)
On Thu, Aug 29, 2024 at 8:20 PM John Hendrikx wrote:
>
> I think they're a bit separate. Even with VSync, the time it takes to kick
> the FX thread in action is still going to be between 0-30ms. If it then
> passes `System.nanoTime()` to the AnimationRunnables, you're basically saying
> that they should render a frame at the precise time of VSync-Time + random
> time it took to schedule the FX thread... suffice to say that the extra
> accuracy of the more accurate VSync timer (just like my far more accurate
> timer) is made completely redundant by the jitter introduced by the scheduler.
>
> This brings me back to my original point: we should not be passing
> `System.nanoTime()` to AnimationRunnables. Passing `System.nanoTime()` is
> basically asking to create a frame with a time index that will NEVER be
> rendered, so why are we asking Animations to use this value for calculating
> animation locations/offsets/sizes ?
>
> This problem is also present on Mac and Linux, just less noticeable because
> their schedulers generally react within 0-2 ms (vs 0-30 ms on Windows). 2 ms
> is "close enough" to the most commonly used frame rates (60 fps, at 16.667 ms
> per frame), but on Windows it can practically be a two frame difference.
>
> Even in the absence of V-sync, when JavaFX arbitrarily picks 60 Hz as its
> refresh frequency, the times passed to AnimationTimer should be multiples of
> 16.667 ms, not 16.667 ms + however long it took to wake up the FX thread. In
> other words this code in AbstactPrimaryTimer:
>
> private long nextPulseTime = nanos();
>
> private long lastPulseDuration = Integer.MIN_VALUE;
>
> @Override
>
> public void run() {
>
> if (paused) {
>
> return;
>
> }
>
> final long now = nanos();
>
> recordStart((nextPulseTime - now) / 100);
>
> timePulseImpl(now);
>
> recordEnd();
>
> updateNextPulseTime(now);
>
> // reschedule animation runnable if needed
>
> updateAnimationRunnable();
>
> }
>
> ...would be far better if it passed "nextPulseTime" to `timePulseImpl` (which
> eventually calls the AnimationRunnables) instead of "now".
>
> Note: this is assuming the adaptive pulse flag is disabled. If it is
> enabled, nextPulseTime won't be a nice multiple of the frame rate -- so when
> this is enabled we may want to round it up/down before passing it to the
> AnimationRunnables.
>
> Note 2: you can **already** achieve far smoother animation even on Windows by
> rounding the value you get passed in to a multiple of 1/frameRate. This only
> works when you have access to the this time. It won't solve Timeline
> calculations -- they will still calculate positions and values for frames
> that will never exist, subject to FX thread scheduling jitter...
>
> --John
Re: Bug: Times passed to AnimationTimer should not fluctuate
The entire idea of using a system timer in the presence of v-sync is wrong, it simply can't work reliably. We get accurate frame synchronization for free from Direct3D, we should use that instead of system timers. There's a JBS issue with a detailed analysis: https://bugs.openjdk.org/browse/JDK-8136536 On Thu, Aug 29, 2024 at 5:40 PM John Hendrikx wrote: > > TLDR; AnimationTimer is being fed a time not based on the time the next > frame is expected to be rendered but the time of when the FX thread is > finally scheduled to do the callbacks. This time fluctuates wildly, and > is a time that's useless for doing frame accurate calculations of > animations (such a time shouldn't fluctuate at all, unless a frame gets > skipped). > > Longer version: > > I've been investigating an "issue" with AnimationTimers. > > Timers have a `void handle(long)` method that is called with a time in > nano seconds that is supposed to be the reference value to be used for > animations. The reference value is used to calculate in the > implementation how the animation should look at a specific point in time. > > However, I've found that the values being received by the AnimationTimer > fluctuate wildly, and the differences between the current and previous > calls of the time passed to the handle method can be anywhere from 1ms > to 30ms. > > The frequency of the calls is however still close to 60 fps -- now, > correct me if I'm wrong, but when drawing animations that are supposed > to be displayed on a 60 fps screen, shouldn't the AnimationTimer be > called (which is supposed to "prepare" things for the **next** frame) > with a relatively consistently increasing value? (ie. 0, 16, 32, 48, 64 > ms) instead of what I'm seeing now (0, 30, 33, 60, 66 ms). Assuming > that the animation will simply be visible on the next frame, then my > calculations using this fluctuating timer are going to be quite jittery > when put on a screen that refreshes at exactly 60 fps. My animation > calculations would look like: > > |--|-|--|-||-|--|-|-- > > While being displayed on a screen that does this: > > |---|---|---|---|---|---|---|---|---|--- > > I compared the values I receive from `handle(long)` with > `System.nanoTime()` and found no significant differences between them; > in other words, the actual calling code is already quite a jittery process. > > So I dug a bit deeper -- internally, a subclass of Timer is used (in my > case WinTimer, the Windows one). Assuming that the problem stemmed from > there, I replaced this with a **highly** accurate timer that simply busy > waits until the appropriate frame time is reached. The accuracy of this > is almost nanosecond perfect (ie, within 0.01 of a millisecond, at > the cost of a CPU core of course, but that's not relevant for the > problem). This however had 0 impact on the accuracy of the > AnimationTimer -- it was still fluctuating wildly, on the order of 20-30 > milliseconds (several million times less accurate than my Timer > implementation). > > Digging further, the AnimationTimer is not actually called directly from > the timing thread (which is a different thread than the FX thread, so > that makes sense). Instead, the timer is called from the FX thread by > having my highly accurate timer call Application::invokeLater (similar > to Platform::runLater). Once the FX thread picks up this task, it calls > `System.nanoTime()` as a "base" for all AnimationTimer callbacks... > however, this value is now distorted by how long it takes the scheduler > to wake up the FX thread and pick up the invokeLater task. > > I find this highly surprising. To make jitter free animations, you want > a base time that increments consistently, and maybe sometimes skips a > frame. So you'd expect the base time value to increment with 1 / > frameRate, not some arbitrary time for when the work was started > (fluctuating with thread scheduling delays). I mean, I'm preparing > things for the **next** frame, so the time I receive should reflect when > that frame is likely to be drawn, not roughly be the same time my method > is actually being called (if I wanted that, I can > call `System.nanoTime()` myself...). > > I feel this is actually a bug. It is probably a bit worse on the > Windows platform where the scheduler generally has a 15 ms period (which > explains the fluctuations I'm seeing between 0 and 30 ms). However, this > really shouldn't matter. When doing animations, you want to call not > with the current system time, but with the expected render time of the > next frame. Calling `System.nanoTime` and using that as base time is > just simply incorrect, although can be "close" to correct if your > platform schedules say with a 1 ms period (it will only fluctuate > between 0 and 2 ms then). > > There should however be 0 fluctuation, unless a frame was skipped. With > a 60 fps frame rate, all the nano values passed to AnimationTimer should > be multiples of 16,666
Re: New CSS parser for JavaFX
> > I did not mean a specific class, but a stylesheet in general. I just think > that there should be a way to construct a stylesheet in memory that does not > require parsing, and ideally with an option to do a limited type check. > Unless it significantly increases the overall effort, in which case the app > can go the usual route of generating a data url. In theory, this can work (just add a bunch of CSS tokens to a javafx.css.Declaration). The problem is that you can't do anything with a Stylesheet, there's no API for you to set it on a Scene or a Parent. This would most likely be a separate enhancement and not part of the CSS parser rework. > > Sorry for pointing out the obvious, the testing might represent overwhelming > portion of the overall effort. Not only we must ensure that any the legacy > CSS continues to produce the same results, the new features work as expected, > and all multiplied at least by 3 supported desktop platforms (or 5, if we > count ios and android for mobile). I agree, it will need to be well-tested.
Re: New CSS parser for JavaFX
I don't think that's practical, as the proposed enhancement will change parsing, representation (Declaration, ParsedValue) and runtime processing (CssStyleHelper). I don't know how we would keep the old implementations around. On Tue, Aug 27, 2024 at 12:01 AM Andy Goryachev wrote: > > I wonder if this could be done as an incubator module, or hidden behind an > experimental system property. > > > > -andy
Re: New CSS parser for JavaFX
On Mon, Aug 26, 2024 at 11:14 PM John Hendrikx wrote: > > The CSS code, and its API, to put it diplomatically, is IMHO a bit under > cooked. > > I've had quite a few looks at it, and each time I've come to the > conclusion that it is near impossible to do any worthwhile improvements > to it without at a minimum getting rid of the ParsedValue abomination. > This class has generics that are used like 1% of the time, and in all > other cases is either used raw or just instanceof'd and casted to > whatever type the code knows will be in there. It literally pollutes > the code and makes it plain unreadable, not to mention giving you a > completely false sense of security -- it's about as useful as just > passing Objects around (which can be arrays, or even arrays of arrays...). > > So I'm already 100% in favor as this proposal will be getting rid of > ParsedValue. The converter example looks like a breath of fresh air > compared to the current code seen in most converters. The fact that ParsedValue is almost unusable by third-party applications works to our advantage here, as this is the reason why we can remove it in the first place. > > Introducing a new method here would have to opportunity to give this a > less confusing name (so not all three of these variants are called > "convert"). As far as I can see there are three main functions for a > StyleConverter: > > - Interpret "raw" CSS values to FX types (like Border, Color, String, > double) > - Take a map of FX types (already interpreted values) and consolidate > these into an FX type, which I think is used or can be used for > shorthand properties and is used to merge values into say a Border or > Background > - The reverse of the above function (for CSS transitions that are > applied to properties that consist of multiple distinct values). > - There is no reverse of the interpret function (there is no need to > convert FX types back to CSS, the code that writes CSS files operates at > a lower level). > > Ideally something like: > > - interpret/parse/convertCSS for T convert(ComponentValue, Font) > - consolidate/merge for T convert(Map) > - split for Map split(T) > > Did you any thoughts on this as well? Not beyond what you wrote down here. I think these are quite different concerns, and don't need to be solved together. > PS. > > Under Solution 1, I think the data structure should read "rgb > (100 50 0 )" (rgb instead of gray) ? Yes, thanks for pointing that out!
Re: New CSS parser for JavaFX
It's certainly true that CSS is a fragile area, but I would argue that one of the reasons it's fragile is because the implementation is a bit unsound in many places. Cleaning up tech debt could make this progressively more robust in the long run. Note that I'm also not proposing a refactor to make the code more elegant or modern, but to solve a systematic issue. On Mon, Aug 26, 2024 at 8:52 PM Kevin Rushforth wrote: > > Lots of things might be good ideas. I'm rather skeptical of the cost / > benefit of implementing a new CSS parser. This would be a large effort and > run a significant risk of regression, especially since CSS is a fragile area > (although parsing less so than the runtime CSS processing). > > -- Kevin
Re: New CSS parser for JavaFX
> can we create stylesheets programmatically without parsing (i.e. construct > the token tree directly)? Maybe, but `Stylesheet` and its constituent classes really seem like exposed implementation details of JavaFX. I would rather deprecate their use and remove them from the public API eventually. > would it be possible to add a diagnostic (at the moment of applyCSS()) to > show the actual rule(s) being used? Sure, the grammar of a declaration is defined by its StyleConverter, which can emit diagnostics if so required. This is not so different from how it is done currently. > are we going to track the evolution of https://www.w3.org/TR/css-syntax-3/ > "spec"? As far as it pertains to features implemented by JavaFX, yes. > are we going to clarify the w3 "spec" (what is the value of "EOF code > point"?)? I'm not sure what you mean by that. The specification is quite clear and unambiguous, so I don't know what we would need to clarify. If you refer specifically to the EOF code point: there is no value of any kind associated with that in the syntax API, as it is conceptual only and not a preserved token. > are there incompatibilities between web-style CSS and FX-style CSS? Yes, the grammar and semantics of JavaFX CSS is defined by its particular implementation, which is different from a HTML document model. However, there are several things we can do to make them less dissimilar, for example by supporting shorthand notations in the future, or supporting notations that are defined in other W3C modules (color functions come to mind). > what is the testing strategy? I don't have anything in mind that goes beyond the usual testing that we do for all enhancements.
New CSS parser for JavaFX
I've written up a proposal to implement a new CSS parser for JavaFX: https://gist.github.com/mstr2/f416996caf48e11193f0b6a5883a3926 The goal is not add new features at this point, but to resolve some long-standing issues with the existing CSS parsing (though if you read carefully, you might spot a new feature). I'm interested in your opinion whether this is a good idea to move forward.
Re: [Feature Proposal] Vertex Colors on TriangleMesh
when individual objects > are desired, they can exist / act as wrappers in user-code, which benefits of > objects we cannot provide automatically, as it requires information only the > user knows about the organization of the arrays. > > But what about if TVertex is not a vertex, but instead a definition of what > buffers the mesh has? Well, we already have that, and it's called > VertexFormat. Making it a generic parameter also wouldn't really provide any > benefit anyways. Instead of making CustomMesh, I propose > expanding VertexFormat to allow for additional arbitrarily defined buffers. > However, I do not think we need to expose this functionality publicly yet, > which is why I've not documented it after the suggestion. We can keep it as > internal implementation details until it's time to add user-supplied shaders. > Doing so will give us maximum flexibility when it is time to make it public. > > This way also has the benefit of us being able to retroactively include > TriangleMesh's points/texCoords/normals arrays in the shader system with very > little complexity, as they are already part of VertexFormat. > > Also thanks for the suggestion about the JEPs, I'll keep this in mind making > future proposals, and it sounds like I should follow-up discussing various > different implementation options and why I've chosen the one I've chosen > instead. I suspect the reason this feels somewhat underdeveloped from the API > perspective is because it's the simplest option I came up with that had the > best API outcome and I didn't elaborate as much as I could have on why others > I thought about weren't satisfactory. > > Thanks again for the feedback, I look forward to hearing back again 😁 > > On Thu, Aug 22, 2024, 8:08 PM Michael Strauß wrote: >> >> I understand that you propose to add a special-purpose mesh >> (GouraudShadedTriangleMesh) instead of adding yet another buffer to >> the existing TriangleMesh. That might be a valid idea if the goal is >> to not overload the TriangleMesh class with special-purpose stuff. >> >> However, I still feel that the solution space in terms of API isn't >> explored in enough detail here. It might be the case that >> CustomMesh is not implementable (and it might also be the >> case that CustomMesh isn't a good idea to begin with). But at >> this point, none of this is obvious to me. >> >> Usually, when you propose a new feature, you should explain the >> motivation, goals and non-goals, alternatives, and so on (you can use >> a JEP template for that if you like). You adequately addressed the >> motivation for your proposed enhancement, but I feel that the >> discussion of different approaches should be expanded upon. I'm not >> convinced that CustomMesh is impossible to implement: if >> TVertex can only ever be PositionTexCoord, PositionNormalTexCoord, >> PositionColorTexCoord, and PositionNormalColorTexCoord (and this is >> enforced, for example using sealed interfaces), then why wouldn't we >> be able to connect this to our existing shaders? >> >> Again, I'm not saying that this is a good idea; it might not work for >> any number of reasons. But I think these alternative approaches should >> at least be explored a little bit before dismissing them. Maybe it >> will be GouraudShadedTriangleMesh in the end. >> >> >> On Fri, Aug 23, 2024 at 4:45 AM Knee Snap wrote: >> > >> > Was hoping to get feedback on my suggestion instead, but another >> > suggestion doesn't work. >> > >> > The idea of a CustomMesh is impossible to implement until after >> > we have fully user-supplied shader support, which I've already addressed >> > as being not the scope of this change (but instead it is a separate future >> > change which is not impacted by this) it also feels like this point may >> > have been missed as well.
Re: [Feature Proposal] Vertex Colors on TriangleMesh
I understand that you propose to add a special-purpose mesh (GouraudShadedTriangleMesh) instead of adding yet another buffer to the existing TriangleMesh. That might be a valid idea if the goal is to not overload the TriangleMesh class with special-purpose stuff. However, I still feel that the solution space in terms of API isn't explored in enough detail here. It might be the case that CustomMesh is not implementable (and it might also be the case that CustomMesh isn't a good idea to begin with). But at this point, none of this is obvious to me. Usually, when you propose a new feature, you should explain the motivation, goals and non-goals, alternatives, and so on (you can use a JEP template for that if you like). You adequately addressed the motivation for your proposed enhancement, but I feel that the discussion of different approaches should be expanded upon. I'm not convinced that CustomMesh is impossible to implement: if TVertex can only ever be PositionTexCoord, PositionNormalTexCoord, PositionColorTexCoord, and PositionNormalColorTexCoord (and this is enforced, for example using sealed interfaces), then why wouldn't we be able to connect this to our existing shaders? Again, I'm not saying that this is a good idea; it might not work for any number of reasons. But I think these alternative approaches should at least be explored a little bit before dismissing them. Maybe it will be GouraudShadedTriangleMesh in the end. On Fri, Aug 23, 2024 at 4:45 AM Knee Snap wrote: > > Was hoping to get feedback on my suggestion instead, but another suggestion > doesn't work. > > The idea of a CustomMesh is impossible to implement until after we > have fully user-supplied shader support, which I've already addressed as > being not the scope of this change (but instead it is a separate future > change which is not impacted by this) it also feels like this point may have > been missed as well.
Re: Accidentally reproduced NPE in synchronizeNodes in combination with enterNestedEventLoop
That seems to be a tough one.
Delaying the invocation of listeners sounds interesting, as it might
allow using a pattern like the following:
childrenTriggerPermutation = true;
try (var scope = new DelayedEventScope(children)) {
children.remove(node);
children.add(node);
} finally {
childrenTriggerPermutation = false;
}
The semantics would be that the property implementation will still
receive notifications with their invalidated() method as the property
is being modified, but events will only be fired at the end of the
scope.
List properties will need a new listChanged() method to allow for the
same pattern of overriding the method instead of adding a change
listener.
Of course, the implementation will be challenging. We'd need to keep
track of all modifications, and then aggregate those modifications
into a single event. In this particular example, the two "add" and
"remove" events would probably be consolidated into a "permutation"
event.
In general, delayed notification scopes for properties could also be
very useful for application developers.
On Thu, Aug 22, 2024 at 9:59 AM John Hendrikx wrote:
>
> I think I figured out the reason why this fails. The root cause lies in a
> misconception I've seen in a lot of FX code.
>
> JavaFX uses a single event thread model, which ensures all structures are
> only ever accessed by a single thread. This frees FX from having to do
> synchronization on almost every modification you make to properties or the
> scene graph.
>
> However, in many areas it makes the assumption that such code will always run
> sequentially to completion without interruption, and uses instance fields
> judiciously to communicate things to deeper nested code or to code further
> down the line. But code using instance fields in this way is not safe to
> re-enter (it is not reentrant-safe) without precautions -- sharing instance
> fields in this way safely can easily get as complicated as writing
> multi-threaded code.
>
> A simple example that I saw in Parent's toFront code:
>
> childrenTriggerPermutation = true;
>
> try {
>
> children.remove(node);
>
> children.add(node);
>
> } finally {
>
> childrenTriggerPermutation = false;
>
> }
>
> The above code uses an instance field "childrenTriggerPermutation" to
> activate an optimization. The optimization will assume that the children are
> only re-arranged, and no new ones were added or removed. However, "children"
> is an ObservableList, which means the user can register listeners on it,
> which do who knows what. If such a listener modifies the children list in
> another way then the code is entered again, but the
> "childrenTriggerPermutation" optimization will still be enabled causing it to
> not notice the change the user did.
>
> This problem is similar to the ChangeListener old value bug. When within a
> change listener you do another change (and so the same code is called
> **deeper** in the same stack), downstream change listeners will not receive
> the correct old values because the code is insufficiently reentrant-safe.
> ExpressionHelper **tries** to mitigate some of these issues (for cases where
> listeners are added/removed reentrantly) by making copies of the listener
> list, but it does not handle this case.
>
> Similarly, the bug I encountered in my original post is also such an issue.
> While processing the children list changes, several **properties** are being
> manipulated. Being properties, these can have listeners of their own that
> could trigger further modifications and, in complex enough programs, they may
> even re-enter the same class's code that is sharing instance fields in an
> unsafe way. And that's exactly what is happening:
>
> 1. The children list change processing is registering the offset of the first
> changed child in the children list (called "startIdx") as an instance field
> -- this field is used as an optimization for updatePeer (so it doesn't have
> to check/copy all children). It assumes the processing always finishes
> completely and it will get to the point where it sets "startIdx" but...
>
> 2. Before it sets "startIdx" but after the children list is already modified,
> it modifies several properties. Being properties, these can have listeners,
> and as such this can trigger a cascade of further calls in complicated
> applications.
>
> 3. In this case, the cascade of calls included an "enterNestedEventLoop".
> Pulses (and things like Platform#runLater) can be handled on such a nested
> loop, and FX decides that now is as good a time as any to handle a new pulse.
>
> 4. The pulse triggers updatePeer calls, among which is the Parent that is
> still (higher in the stack) midway its children list processing code.
>
> 5. The updatePeer code looks at "startIdx", the shared instance field that
> Parent uses for its optimizations. This field is NOT modified yet. The
> field indicates the first child that
Re: [Feature Proposal] Vertex Colors on TriangleMesh
> Any solution which was good enough for normals is also good enough for vertex > colors right? Not necessarily. Designing APIs is hard, and one should try to start from a point of asking "how would the API look like if we had considered all of the things we know now from the beginning". An idea that might be worth exploring is the following. Instead of storing positions, normals, texture coordinates, etc. in separate buffers as it is currently done in TriangleMesh, we could have a CustomMesh, where TVertex is a structure that stores all relevant components in the same place. This could be a sealed interface that only allows a limited set of implementations (because we don't have the option of user-defined vertex structures at the moment), but could allow for future extension with user-defined vertex structures.
Re: [Feature Proposal] Vertex Colors on TriangleMesh
Vertex colors usually refers to a technique where the vertex format is enriched with an additional color component, and this color component is then used in one of the shader stages to affect the computation in some way (usually by combining it with the texture color). As you correctly say, this technique is decades-old, it existed even back in the days of fixed function pipelines. However, with the introduction of programmable shaders, the structure of vertices can be freely defined by developers. It seems to me that your proposed API would pick just one (maybe useful) scenario, and eternally bake it into JavaFX APIs. What if we get custom shaders one day? Then we would need a way to customize the vertex format again. Currently we have two predefined vertex formats: VertexFormat.POINT_TEXCOORD and VertexFormat.POINT_NORMAL_TEXCOORD. It doesn't seem obvious to me that the best way forward is to add more bespoke formats here. Have you thought about ways how we could allow deverlopers to define custom formats? Maybe that would also require a new kind of Mesh that can accept custom data.
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v25]
On Sat, 17 Aug 2024 15:57:34 GMT, Nir Lisker wrote: > > Mirroring the CSS specification, the `Interpolatable` interface defines > > several types of component interpolation > > Can you give a link to these CSS specifications? I shouldn't say "mirroring", it's more "inspired by". https://www.w3.org/TR/web-animations/#animating-properties - PR Comment: https://git.openjdk.org/jfx/pull/1522#issuecomment-2294924353
Re: RFR: 8338016: SplitMenuButton constructors should match MenuButton [v2]
On Thu, 15 Aug 2024 17:53:00 GMT, Andy Goryachev wrote: >> I do that too, typically. But here I just copied some code from a sibling >> test. >> >> If we are converting, then we also should convert MenuButtonTest as well, >> and now we are widening the scope. There is no gain in going with junit5 >> with this test specifically, so I don't really want to do it right now, I >> would rather do it as a part of a bulk change. >> >> Do you want me to convert this test? > > created https://bugs.openjdk.org/browse/JDK-8338468 It's okay, I'd just like to see it get done _eventually_. - PR Review Comment: https://git.openjdk.org/jfx/pull/1535#discussion_r1718798147
Re: RFR: 8338016: SplitMenuButton constructors should match MenuButton [v2]
On Thu, 15 Aug 2024 14:46:23 GMT, Andy Goryachev wrote: >> Adding SplitMenuButton constructors matching MenuButton: >> >> - public SplitMenuButton(String text) >> - public SplitMenuButton(String text, Node graphic) >> - public SplitMenuButton(String text, Node graphic, MenuItem... items) > > Andy Goryachev has updated the pull request incrementally with one additional > commit since the last revision: > > tests Marked as reviewed by mstrauss (Committer). - PR Review: https://git.openjdk.org/jfx/pull/1535#pullrequestreview-2241022648
Re: RFR: 8338016: SplitMenuButton constructors should match MenuButton [v2]
On Thu, 15 Aug 2024 17:07:40 GMT, Andy Goryachev wrote: >> modules/javafx.controls/src/test/java/test/javafx/scene/control/SplitMenuButtonTest.java >> line 34: >> >>> 32: import org.junit.Test; >>> 33: >>> 34: import static org.junit.Assert.*; >> >> Since you're modifying this file significantly, you could also use the >> JUnit5 imports. > > I think that would be out of scope for this PR. > > We are considering updating tests to junit5 in bulk at some point, but I > suspect it's a fairly low priority, unless we need to address something like > https://bugs.openjdk.org/browse/JDK-8328629 Well, you _do_ modify this file significantly, so it's not entirely out of scope. I've been converting files over to JUnit5 in the past as part of significant modifications, I think it's a valid path to get us to eventually move completely to JUnit5 (because as you say, bulk conversion isn't probably happening any time soon). - PR Review Comment: https://git.openjdk.org/jfx/pull/1535#discussion_r1718773749
Re: RFR: 8338016: SplitMenuButton constructors should match MenuButton [v2]
On Thu, 15 Aug 2024 14:46:23 GMT, Andy Goryachev wrote: >> Adding SplitMenuButton constructors matching MenuButton: >> >> - public SplitMenuButton(String text) >> - public SplitMenuButton(String text, Node graphic) >> - public SplitMenuButton(String text, Node graphic, MenuItem... items) > > Andy Goryachev has updated the pull request incrementally with one additional > commit since the last revision: > > tests modules/javafx.controls/src/test/java/test/javafx/scene/control/SplitMenuButtonTest.java line 34: > 32: import org.junit.Test; > 33: > 34: import static org.junit.Assert.*; Since you're modifying this file significantly, you could also use the JUnit5 imports. - PR Review Comment: https://git.openjdk.org/jfx/pull/1535#discussion_r1718701002
Re: RFR: 8338016: SplitMenuButton constructors should match MenuButton
On Tue, 13 Aug 2024 22:27:58 GMT, Andy Goryachev wrote: > Adding SplitMenuButton constructors matching MenuButton: > > - public SplitMenuButton(String text) > - public SplitMenuButton(String text, Node graphic) > - public SplitMenuButton(String text, Node graphic, MenuItem... items) modules/javafx.controls/src/main/java/javafx/scene/control/SplitMenuButton.java line 123: > 121: * @param text the text to display on the menu button > 122: * @param graphic the graphic to display on the menu button > 123: * @param items The items to display in the popup menu. Minor: description should start with a small letter and not end with a dot. - PR Review Comment: https://git.openjdk.org/jfx/pull/1535#discussion_r1717226223
Re: RFR: 8334900: IOOBE when adding data to a Series of a BarChart that already contains data [v4]
On Sat, 10 Aug 2024 16:21:11 GMT, Markus Mack wrote: >> This PR is a fix for another IOOBE that I discovered while working on #1476. >> >> The PR simplifies the code for adding a series that already contains data by >> adding the data points one-by-one. >> As far as I can see no attempt was previously made to optimize the bulk >> operation except for some trivial O(1) operations, so this should have no >> noticable performance impact. >> >> Accidentally this fixes another bug related to the missing "negative" style >> class when negative data values are added. >> >> Also, the PR aligns the handling of duplicate categories with the behavior >> clarified in #1476, when there are duplicates in the data that was already >> in the series before the series was added to the chart. >> >> Note a change was made to the createTestSeries() test method, letting it >> start at index 1, avoiding the duplicate data items resulting from >> multiplying by 0. >> Without this change `testSeriesRemoveAnimatedStyleClasses` would fail >> because it counts the number of plot children, where the duplicates are now >> removed. > > Markus Mack has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains six commits: > > - test "negative" style class in BarChartTest tests > - Merge remote-tracking branch 'refs/remotes/origin/master' into > fixes/bar-chart-add-nonempty-series > - fix "negative" style class when series is changed > - Merge remote-tracking branch 'refs/remotes/origin/master' into > fixes/bar-chart-add-nonempty-series > ># Conflicts: ># > modules/javafx.controls/src/test/java/test/javafx/scene/chart/BarChartTest.java > - BarChart: Fix adding non-empty series > - BarChart: Add styleClass "negative" for added data Marked as reviewed by mstrauss (Committer). - PR Review: https://git.openjdk.org/jfx/pull/1488#pullrequestreview-2238671326
Re: RFR: 8337246: SpinnerSkin does not consume ENTER KeyEvent when editor ActionEvent is consumed [v2]
On Tue, 13 Aug 2024 21:35:11 GMT, Andy Goryachev wrote: >> Enable backpropagation of `isConsumed` flag to the ancestor(s) of events >> cloned via `Event.copyFor()`. >> >> This change has a minimal API impact and allows for a fine-grained control >> of when to propagate and when not to propagate. >> >> The proposed change could make >> [JDK-8303209](https://bugs.openjdk.org/browse/JDK-8303209) unnecessary. > > Andy Goryachev has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 10 additional > commits since the last revision: > > - avoid csr > - Merge remote-tracking branch 'origin/master' into ag.consume > - only if consumed > - only when consumed > - propagate > - cleanup > - simpler > - event helper > - copy for test > - propagate consume action > /csr unneeded You will need a CSR, because you propose to make the public `consumed` field private (and change its type). - PR Comment: https://git.openjdk.org/jfx/pull/1523#issuecomment-2289245880
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v25]
On Fri, 9 Aug 2024 22:32:46 GMT, Michael Strauß wrote:
>> This PR completes the CSS Transitions story (see #870) by adding
>> interpolation support for backgrounds and borders, making them targetable by
>> transitions.
>>
>> `Background` and `Border` objects are deeply immutable, but not
>> interpolatable. Consider the following `Background`, which describes the
>> background of a `Region`:
>>
>>
>> Background {
>> fills = [
>> BackgroundFill {
>> fill = Color.RED
>> }
>> ]
>> }
>>
>>
>> Since backgrounds are deeply immutable, changing the region's background to
>> another color requires the construction of a new `Background`, containing a
>> new `BackgroundFill`, containing the new `Color`.
>>
>> Animating the background color using a CSS transition therefore requires the
>> entire Background object graph to be interpolatable in order to generate
>> intermediate backgrounds.
>>
>> More specifically, the following types will now implement `Interpolatable`.
>>
>> - `Insets`
>> - `Background`
>> - `BackgroundFill`
>> - `BackgroundImage`
>> - `BackgroundPosition`
>> - `BackgroundSize`
>> - `Border`
>> - `BorderImage`
>> - `BorderStroke`
>> - `BorderWidths`
>> - `CornerRadii`
>> - `Stop`
>> - `Paint` and all of its subclasses
>> - `Margins` (internal type)
>> - `BorderImageSlices` (internal type)
>>
>> ## Interpolation of composite objects
>>
>> As of now, only `Color`, `Point2D`, and `Point3D` are interpolatable. Each
>> of these classes is an aggregate of `double` values, which are combined
>> using linear interpolation. However, many of the new interpolatable classes
>> comprise of not only `double` values, but a whole range of other types. This
>> requires us to more precisely define what we mean by "interpolation".
>>
>> Mirroring the CSS specification, the `Interpolatable` interface defines
>> several types of component interpolation:
>>
>> | Interpolation type | Description |
>> |---|---|
>> | default | Component types that implement `Interpolatable` are interpolated
>> by calling the `interpolate(Object, double)}` method. |
>> | linear | Two components are combined by linear interpolation such that `t
>> = 0` produces the start value, and `t = 1` produces the end value. This
>> interpolation type is usually applicable for numeric components. |
>> | discrete | If two components cannot be meaningfully combined, the
>> intermediate component value is equal to the start value for `t < 0.5` and
>> equal to the end value for `t >= 0.5`. |
>> | pairwise | Two lists are combined by pairwise interpolation. If the start
>> list has fewer elements than the target list, the...
>
> Michael Strauß has updated the pull request incrementally with one additional
> commit since the last revision:
>
> remove StyleConverter.WithReconstructionSupport
CSS properties don't always correspond to JavaFX properties (for example,
`-fx-background-color` has no corresponding JavaFX property, it is combined
with other CSS properties and mapped to `backgroundProperty()`). It might be a
good idea to add a column to the CSS property tables, indicating the animation
type of each CSS property. It would look like this:
However, this is a large documentation change in and of itself, and I strongly
suggest to do this as part of
[JDK-8338121](https://bugs.openjdk.org/browse/JDK-8338121) to keep this PR
manageable and focused.
-
PR Comment: https://git.openjdk.org/jfx/pull/1522#issuecomment-2282228208
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v18]
On Sat, 10 Aug 2024 11:33:43 GMT, John Hendrikx wrote: >> Yes, that's a good idea, I was mainly worried about introducing a new public >> API that we can't remove later when we haven't figured out yet what the best >> direction would be. With this API removed, I think we can get this merged. > > One more thing. You introduced `transition` short hand version. Is it maybe > a good idea to not do this yet until we have a better idea how to deal with > them? Shorthand properties are just a convenience; the feature will work > without them. If it was already released, then can ignore this as we're too > late. The `transition` shorthand is already on track to be released, and we’re deep into RDP2. However, `TransitionDefinitionConverter` is not public API, it’s implementation can move around freely. - PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1712646742
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v22]
On Fri, 9 Aug 2024 15:37:56 GMT, Andy Goryachev wrote: >> `reconstruct` is a misnomer, as reconstruction is a two-step operation >> (`convertBack` followed by `convert`). To be fair, I don't like any of these >> better than `convertBack`, mostly because I want to stress that those two >> operations are closely related, and none of the alternatives evoke that >> sentiment. That being said, this is a minor API and I don't really mind >> either way, but absent a compelling reason, I'd prefer to stick to >> `convertBack`. > > That's fine, I just think `convertBack` implies an arrow of time or a > preferred spatial direction. > > Maybe `toMap()`, but as long as the method is sufficiently documented, we > should be fine. Naming _is_ hard. I've moved this to an internal interface (it doesn't need to be public for this PR), so we can have the discussion later if we choose to make this API public (and if so, how we go about doing that). - PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1712445361
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v18]
On Fri, 9 Aug 2024 19:42:56 GMT, John Hendrikx wrote: >> But the converter is not reconstructable (in comparison to Interpolatable, >> which is something the marked types actually are). > > Sorry to keep harping on this, but so far, I think all the options are quite > poor from the perspective of the user, and from a perspective of "could it > have been designed this way in the first place". > > I also think I smell something fishy. The `StyleConverter` seems to serve > two unrelated purposes. One is to convert raw parsed values for the CSS > parser (`convert(ParsedValue, Font)`) to a Java object. > > The other is to support sub-properties (which you can also detect by checking > if `CssMetaData.getSubProperties` is not empty). Although that method is > also called `convert` it is only used by `CssStyleHelper`, which is > completely unrelated to parsing. A more accurate name for that method would > be "consolidateSubPropertyValues" (and the opposite would then be something > like "extractSubPropertyValues", or `consolidate` and `extract` for short). > > There does not seem to be any overlap between these two purposes. That is it > to say, a style converter that is used for decoding sub-properties is never > used for parsing, and vice versa. FX also does not support special syntax > for specifying say a border in short hand form (like `-fx-border: dashed > red`). > > I think it may be a good idea to perhaps have a look if these purposes > shouldn't just be split. On the one hand, you have a style converter which > uses `convert` for parsing. On the other hand, you have `CssMetaData`, which > when it has sub-properties **must** support a back-and-forth translation > (`convert` + `convertBack`) -- this is currently not enforced (ie. you can > create `CssMetaData` with sub-properties but then **not** implement > `convert(Map)` in the used style converter... this causes problems later on. > > The only CSS property currently that supports both short-hand and > sub-properties is actually the newly introduced `transition` property. This > could either implement two interfaces, or it could just be two classes. > > I've done a quick attempt to do such a split (not looking too much at API) > and the whole thing compiles and passes all tests still. This is surprising, > because I deleted several pieces of code, that are apparently never used (or > perhaps not tested for, we can investigate this further). > > See here: #1533 > > Diff: > https://github.com/openjdk/jfx/pull/1533/commits/8f6d1e56a43068184599ad1ea47b4da98eb70bff#diff-fc96cf3204909a6110e64c37a935927be56efedc9f3f4f6f6843cc4fa7da5a1f > > Let me know if you feel this has merit. I also think that we're probably dealing with two different things that are mixed up in `StyleConverter`. But instead of having this discussion here, it probably makes more sense to have it in the context of [JDK-8333121](https://bugs.openjdk.org/browse/JDK-8333121) because it seems relevant to eventually support both short-hand and long-hand property notations. I've renamed `WithReconstructionSupport` to `SubPropertyConverter` and moved it to an internal package for now. Since both `BackgroundConverter` and `BorderConverter` are not public, we can change it any way we want at a later time. - PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1712306610
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v25]
> This PR completes the CSS Transitions story (see #870) by adding
> interpolation support for backgrounds and borders, making them targetable by
> transitions.
>
> `Background` and `Border` objects are deeply immutable, but not
> interpolatable. Consider the following `Background`, which describes the
> background of a `Region`:
>
>
> Background {
> fills = [
> BackgroundFill {
> fill = Color.RED
> }
> ]
> }
>
>
> Since backgrounds are deeply immutable, changing the region's background to
> another color requires the construction of a new `Background`, containing a
> new `BackgroundFill`, containing the new `Color`.
>
> Animating the background color using a CSS transition therefore requires the
> entire Background object graph to be interpolatable in order to generate
> intermediate backgrounds.
>
> More specifically, the following types will now implement `Interpolatable`.
>
> - `Insets`
> - `Background`
> - `BackgroundFill`
> - `BackgroundImage`
> - `BackgroundPosition`
> - `BackgroundSize`
> - `Border`
> - `BorderImage`
> - `BorderStroke`
> - `BorderWidths`
> - `CornerRadii`
> - `Stop`
> - `Paint` and all of its subclasses
> - `Margins` (internal type)
> - `BorderImageSlices` (internal type)
>
> ## Interpolation of composite objects
>
> As of now, only `Color`, `Point2D`, and `Point3D` are interpolatable. Each of
> these classes is an aggregate of `double` values, which are combined using
> linear interpolation. However, many of the new interpolatable classes
> comprise of not only `double` values, but a whole range of other types. This
> requires us to more precisely define what we mean by "interpolation".
>
> Mirroring the CSS specification, the `Interpolatable` interface defines
> several types of component interpolation:
>
> | Interpolation type | Description |
> |---|---|
> | default | Component types that implement `Interpolatable` are interpolated
> by calling the `interpolate(Object, double)}` method. |
> | linear | Two components are combined by linear interpolation such that `t =
> 0` produces the start value, and `t = 1` produces the end value. This
> interpolation type is usually applicable for numeric components. |
> | discrete | If two components cannot be meaningfully combined, the
> intermediate component value is equal to the start value for `t < 0.5` and
> equal to the end value for `t >= 0.5`. |
> | pairwise | Two lists are combined by pairwise interpolation. If the start
> list has fewer elements than the target list, the missing elements are copied
> from the target list. If the start list has more elements than the ...
Michael Strauß has updated the pull request incrementally with one additional
commit since the last revision:
remove StyleConverter.WithReconstructionSupport
-
Changes:
- all: https://git.openjdk.org/jfx/pull/1522/files
- new: https://git.openjdk.org/jfx/pull/1522/files/a0557b8f..9bdea0a4
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jfx&pr=1522&range=24
- incr: https://webrevs.openjdk.org/?repo=jfx&pr=1522&range=23-24
Stats: 99 lines in 7 files changed: 62 ins; 27 del; 10 mod
Patch: https://git.openjdk.org/jfx/pull/1522.diff
Fetch: git fetch https://git.openjdk.org/jfx.git pull/1522/head:pull/1522
PR: https://git.openjdk.org/jfx/pull/1522
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v24]
> This PR completes the CSS Transitions story (see #870) by adding
> interpolation support for backgrounds and borders, making them targetable by
> transitions.
>
> `Background` and `Border` objects are deeply immutable, but not
> interpolatable. Consider the following `Background`, which describes the
> background of a `Region`:
>
>
> Background {
> fills = [
> BackgroundFill {
> fill = Color.RED
> }
> ]
> }
>
>
> Since backgrounds are deeply immutable, changing the region's background to
> another color requires the construction of a new `Background`, containing a
> new `BackgroundFill`, containing the new `Color`.
>
> Animating the background color using a CSS transition therefore requires the
> entire Background object graph to be interpolatable in order to generate
> intermediate backgrounds.
>
> More specifically, the following types will now implement `Interpolatable`.
>
> - `Insets`
> - `Background`
> - `BackgroundFill`
> - `BackgroundImage`
> - `BackgroundPosition`
> - `BackgroundSize`
> - `Border`
> - `BorderImage`
> - `BorderStroke`
> - `BorderWidths`
> - `CornerRadii`
> - `Stop`
> - `Paint` and all of its subclasses
> - `Margins` (internal type)
> - `BorderImageSlices` (internal type)
>
> ## Interpolation of composite objects
>
> As of now, only `Color`, `Point2D`, and `Point3D` are interpolatable. Each of
> these classes is an aggregate of `double` values, which are combined using
> linear interpolation. However, many of the new interpolatable classes
> comprise of not only `double` values, but a whole range of other types. This
> requires us to more precisely define what we mean by "interpolation".
>
> Mirroring the CSS specification, the `Interpolatable` interface defines
> several types of component interpolation:
>
> | Interpolation type | Description |
> |---|---|
> | default | Component types that implement `Interpolatable` are interpolated
> by calling the `interpolate(Object, double)}` method. |
> | linear | Two components are combined by linear interpolation such that `t =
> 0` produces the start value, and `t = 1` produces the end value. This
> interpolation type is usually applicable for numeric components. |
> | discrete | If two components cannot be meaningfully combined, the
> intermediate component value is equal to the start value for `t < 0.5` and
> equal to the end value for `t >= 0.5`. |
> | pairwise | Two lists are combined by pairwise interpolation. If the start
> list has fewer elements than the target list, the missing elements are copied
> from the target list. If the start list has more elements than the ...
Michael Strauß has updated the pull request incrementally with two additional
commits since the last revision:
- fix line separators
- StyleableStringProperty should be transitionable
-
Changes:
- all: https://git.openjdk.org/jfx/pull/1522/files
- new: https://git.openjdk.org/jfx/pull/1522/files/abef4325..a0557b8f
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jfx&pr=1522&range=23
- incr: https://webrevs.openjdk.org/?repo=jfx&pr=1522&range=22-23
Stats: 111 lines in 2 files changed: 103 ins; 0 del; 8 mod
Patch: https://git.openjdk.org/jfx/pull/1522.diff
Fetch: git fetch https://git.openjdk.org/jfx.git pull/1522/head:pull/1522
PR: https://git.openjdk.org/jfx/pull/1522
