Re: RFR: 8332895: Support interpolation for backgrounds and borders [v3]
On Thu, 4 Jul 2024 19:40:57 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`
>> - `ImagePattern`
>> - `LinearGradient`
>> - `RadialGradient`
>> - `Stop`
>>
>> ## 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 li...
>
> 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 13 additional
> commits since the last revision:
>
> - fix line separators
> - add documentation to Point2D/3D
> - change documentation
> - add specification
> - add exports
> - revert change
> - revert change
> - added more tests
> - added specification and tests
> - Merge branch 'master' into feature/interpolatable
> - ... and 3 more: https://git.openjdk.org/jfx/compare/60cc590f...08ed751b
modules/javafx.graphics/src/main/java/javafx/scene/paint/Stop.java line 267:
> 265: public Stop(@NamedArg("offset") double offset,
> @NamedArg(value="color", defaultValue="BLACK") Color color) {
> 266: this.offset = offset;
> 267: this.color = Objects.requireNonNullElse(color,
> Color.TRANSPARENT);
Note that a `null` color is now treated as `TRANSPARENT` for the following
reasons:
1. The previous implementation was broken: if a `Stop` is constructed with a
`null` color, the `hashCode()` method throws a NPE because it doesn't check for
`null`.
2. It doesn't make sense to have a stop with `null` color. What does it even
mean?
3. When the stop list is normalized, an empty or null list is treated as a
two-stop list with `TRANSPARENT` color (see `normalize()`). So we already have
a scenario where `null` is treated as `TRANSPARENT`.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1471#discussion_r1666040056
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v3]
On Thu, 4 Jul 2024 19:40:57 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`
>> - `ImagePattern`
>> - `LinearGradient`
>> - `RadialGradient`
>> - `Stop`
>>
>> ## 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 li...
>
> 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 13 additional
> commits since the last revision:
>
> - fix line separators
> - add documentation to Point2D/3D
> - change documentation
> - add specification
> - add exports
> - revert change
> - revert change
> - added more tests
> - added specification and tests
> - Merge branch 'master' into feature/interpolatable
> - ... and 3 more: https://git.openjdk.org/jfx/compare/d436b7df...08ed751b
I've added a new "interpolation type" specification for all components of
interpolatable classes. This mirrors the "animation type" of the CSS
specification, which is listed as an entry in the property table. Please also
read the updated top-level post of this PR, which goes into more detail about
this new specification.
@kevinrushforth This might be a candidate for a late enhancement, because the
risk is relatively low (it's mostly the addition of the `interpolate()` method
to various types), and it seems to be important to make the CSS transitions
feature useful (backgrounds and borders will probably be one of the most
obvious things developers will want to animate).
-
PR Comment: https://git.openjdk.org/jfx/pull/1471#issuecomment-2209504560
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v3]
> 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`
> - `ImagePattern`
> - `LinearGradient`
> - `RadialGradient`
> - `Stop`
>
> ## 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 target
> list, the excess elements are discarded
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 13 additional commits
since the last revision:
- fix line separators
- add documentation to Point2D/3D
- change documentation
- add specification
- add exports
- revert change
- revert change
- added more tests
- added specification and tests
- Merge branch 'master' into feature/interpolatable
- ... and 3 more: https://git.openjdk.org/jfx/compare/c74fcdf3...08ed751b
-
Changes:
- all: https://git.openjdk.org/jfx/pull/1471/files
- new: https://git.openjdk.org/jfx/pull/1471/files/bb84c57d..08ed751b
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jfx=1471=02
- incr: https://webrevs.openjdk.org/?repo=jfx=1471=01-02
Stats: 24002 lines in 240 files changed: 23316 ins; 106 del; 580 mod
Patch: https://git.openjdk.org/jfx/pull/1471.diff
Fetch: git fetch https://git.openjdk.org/jfx.git pull/1471/head:pull/1471
PR: https://git.openjdk.org/jfx/pull/1471
Integrated: 8289115: Touch events is not dispatched after upgrade to JAVAFX17+
On Tue, 21 May 2024 14:25:51 GMT, Michael Strauß wrote: > This PR fixes a bug > ([JDK-8289115](https://bugs.openjdk.org/browse/JDK-8289115)) that was > introduced with #394, which changed the following line in the > `NotifyTouchInput` function (ViewContainer.cpp): > > - const bool isDirect = true; > + const bool isDirect = IsTouchEvent(); > > > `IsTouchEvent` is a small utility function that uses the Windows SDK function > `GetMessageExtraInfo` to distinguish between mouse events and pen events as > described in this article: > https://learn.microsoft.com/en-us/windows/win32/tablet/system-events-and-mouse-messages > > I think that using this function to distinguish between touchscreen events > and pen events is erroneous. The linked article states: > _"When your application receives a **mouse message** (such as WM_LBUTTONDOWN) > [...]"_ > > But we are not receiving a mouse message in `NotifyTouchInput`, we are > receiving a `WM_TOUCH` message. > I couldn't find any information on whether this API usage is also supported > for `WM_TOUCH` messages, but my testing shows that that's probably not the > case (I tested this with a XPS 15 touchscreen laptop, where > `GetMessageExtraInfo` returns 0 for `WM_TOUCH` messages). > > My suggestion is to check the `TOUCHEVENTF_PEN` flag of the `TOUCHINPUT` > structure instead: > > const bool isDirect = (ti->dwFlags & TOUCHEVENTF_PEN) == 0; This pull request has now been integrated. Changeset: 6c3fb5f5 Author:Michael Strauß URL: https://git.openjdk.org/jfx/commit/6c3fb5f557597a5a226d4a7bd41d84b7feb4a162 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8289115: Touch events is not dispatched after upgrade to JAVAFX17+ Reviewed-by: jpereda, kcr - PR: https://git.openjdk.org/jfx/pull/1459
Re: Should we document Styleable properties?
Further testing shows that while the javadoc tool does support custom tags, it doesn’t lift custom tags defined on JavaFX property fields to the property method documentation. Lifting documentation from the property field to property methods is a JavaFX-specific feature of Javadoc. At first glance, there seems to be no particular reason why custom tags are ignored in this situation; probably it just wasn’t implemented. So if we add this custom tag, it won’t show up on the generated HTML. We could start using a custom tag, and then file an enhancement request for Javadoc to lift custom tags to methods just as it does with other tags. What’s your opinion on that? Andy Goryachev schrieb am Mo. 1. Juli 2024 um 23:20: > Would even work with Eclipse out of the box: > > > > > > I also like the fact that we won't need to maintain links manually as > there would not be any. > > > > -andy >
Re: Should we document Styleable properties?
The javadoc tool already supports custom tags out of the box with the
"-tag" command line option. For example, adding this line in the
gradle javadoc task (build.gradle L4241) would introduce a custom tag:
options.tags("styleableProperty:a:CSS property name:")
Of course, there's no special processing of custom tags, so we would
have to add a link manually if we want one. But that's certainly not a
worse situation compared to adding a manual link in prose text.
On Mon, Jul 1, 2024 at 9:16 PM Andy Goryachev wrote:
>
> Thank you for your feedback, Michael!
>
>
>
> Let me first make sure I understand your suggestion correctly:
>
>
>
> add an annotation, let's say @css-prop
> modify javadoc tool to create an automated link from the property name to a
> place in cssref.html
> javadoc will render this as
>
>
>
> @css-prop this property can be styled with CSS using href="...">-fx-prop-name
>
>
>
> I don't know whether javadoc people will be happy about this idea: javadoc is
> a part of jdk and unfortunately javafx is not, though javadoc does offer
> certain features to make it play nice with javafx properties.
>
>
>
> This would also require non-trivial modification to cssref.html to add
> anchors where each property name is defined. Alternatively, it could simply
> point to a class, for which we do have an id (e.g. Cell)
>
>
>
> Overall, it is much more extensive | expensive proposition with external
> dependencies. Meaning the probability of it happening is very low.
>
>
>
> Having said that, this is a great idea, very developer-friendly.
>
>
>
> -andy
Re: Should we document Styleable properties?
If we do this, I suggest to add a custom tag for this purpose instead
of repeating prose for every property.
The javadoc tool will then render the content of this tag in the
specification list section of the documentation (where tags such as
@defaultValue will also appear).
On Fri, Jun 28, 2024 at 11:21 PM Andy Goryachev
wrote:
>
> Dear fellow developers:
>
>
>
> Should we document which properties are styleable with CSS in javadoc? Would
> that be useful?
>
>
>
> Example:
>
>
>
> /**
>
> * Determines the caret blink period. This property cannot be set to
> {@code null}.
>
> *
>
> * This is a {@link StyleableProperty} with the name {@code
> -fx-caret-blink-period}.
>
> *
>
>
>
> alternative:
>
>
>
> * This property can be styled with CSS using {@code
> -fx-caret-blink-period} name.
>
> * @implNote The property object implements {@link StyleableProperty}
> interface.
>
>
>
>
>
> Other ideas are also welcome.
>
>
>
> -andy
Re: RFR: 8334900: IOOBE when adding data to a Series of a BarChart that already contains data
On Mon, 24 Jun 2024 22:10:38 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. Good idea to deduplicate the code in `dataItemAdded` and `seriesAdded`. The patch looks good. - Marked as reviewed by mstrauss (Committer). PR Review: https://git.openjdk.org/jfx/pull/1488#pullrequestreview-2146094390
Re: RFR: 8290310: ChangeListener events are incorrect or misleading when a nested change occurs
On Sun, 16 Apr 2023 12:34:44 GMT, John Hendrikx wrote:
>> I agree with you there, and I've been looking what would be a good way to
>> achieve this. I will take another look soon. My primary concern is that
>> this is a somewhat critical path, and I would want to ensure that it doesn't
>> cause too much performance regressions (I've already been optimizing all of
>> this code with the help of a JMH test)
>
> While looking that code over to see if it could be merged without impacting
> the general case, I discovered a small bug in the OldValueCaching version.
> After I fixed it, the code was even more similar than it was already. The
> only different still is the fact that the latest value must be kept track of
> whenever ObservableValue#getValue is called.
>
> I've now added an extra parameter to the generic version to allow for storing
> the latest value when it is queried (and not storing it if it's not needed).
> This seems to have a minimal performance impact only, so I think the trade
> off is acceptable.
Have you considered adding a method like `void valueUpdated(T value) {}` to
`ListenerList`? This will require `ListenerList` to have a type variable `T`
(which `OldValueCachingListenerList` adds anyway).
This method could then be called instead of
`latestValueTracker.accept(newValue)`, and `OldValueCachingListenerList` can
override it and store the value. The advantage of that would be that we don't
need the `latestValueTracker` field.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1167979736
Re: RFR: 8290310: ChangeListener events are incorrect or misleading when a nested change occurs
On Mon, 24 Jul 2023 22:09:49 GMT, John Hendrikx wrote:
>> modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerManager.java
>> line 143:
>>
>>> 141: */
>>> 142: public void fireValueChanged(I instance, T oldValue) {
>>> 143: Object data = getData(instance);
>>
>> The `data` value could be passed into this method, which would save a
>> (potentially not devirtualized) method call.
>
> Thanks, I'll look into that, it might speed up the 1 listener cases a bit.
> The same applies to OldValueCachingListenerManager#getValue I think. I know
> it isn't possible for the add/remove calls, as the data may change if they're
> nested, but for `fireValueChanged` I never really checked after going to this
> strategy.
Have you considered passing `data` directly into the method? What is your
conclusion?
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1361519194
Re: RFR: 8299753: Tree/TableView: Column Resizing With Fractional Scale
On Wed, 5 Jul 2023 20:31:09 GMT, Michael Strauß wrote: >> Modified the resize algorithm to work well with fractional scale, thanks for >> deeper understanding of the problem thanks to @hjohn and @mstr2 . >> >> Removed earlier manual tester in favor of the monkey tester. >> >> It is important to note that even though the constraints are given by the >> user in unsnapped coordinates, they are converted to snapped values, since >> the snapped values correspond to the actual pixels on the display. This >> means the tests that validate honoring constraints should, in all the cases >> where (scale != 1.0), assume possibly error not exceeding (1.0 / scale). > > I agree with John that a layout algorithm that uses incremental calculations > will always be flawed in principle. The correct approach is to store the > initial configuration, and then for each configuration change, go back to the > initial configuration and recompute the layout solution. > > Now, we might still accept a bugfix for a flawed algorithm. But JDK-8299753 > is an enhancement, not a bugfix. I'm not sure what to make of this: it's > obviously a flawed approach, and basing an enhancement on a flawed approach > means that someone would have to come back to this issue in the future and > solve it correctly. > > I don't think that the issue at hand is so severe that it's a forced move to > integrate this interim solution. > I respectfully disagree, @mstr2 . > > This fix is not an interim solution - unlike any theoretical considerations > of "flawed approach", in practice this code does work as expected with > integer and fractional scales, and, given our situation, it's highly unlikely > that any alternative solutions will ever be considered, especially those that > would affect existing public APIs. It isn't just a theoretical issue. The proposed patch fails to keep the divider precisely at the cursor location, depending on frame rate and mouse movement speed. This is how the behavior manifests on my machine: https://github.com/openjdk/jfx/assets/43553916/79cf04be-bd18-4cfe-9c34-912978ee96ee - PR Comment: https://git.openjdk.org/jfx/pull/1156#issuecomment-1622726851
Re: RFR: 8299753: Tree/TableView: Column Resizing With Fractional Scale
On Thu, 15 Jun 2023 19:38:00 GMT, Andy Goryachev wrote: > Modified the resize algorithm to work well with fractional scale, thanks for > deeper understanding of the problem thanks to @hjohn and @mstr2 . > > Removed earlier manual tester in favor of the monkey tester. > > It is important to note that even though the constraints are given by the > user in unsnapped coordinates, they are converted to snapped values, since > the snapped values correspond to the actual pixels on the display. This > means the tests that validate honoring constraints should, in all the cases > where (scale != 1.0), assume possibly error not exceeding (1.0 / scale). I agree with John that a layout algorithm that uses incremental calculations will always be flawed in principle. The correct approach is to store the initial configuration, and then for each configuration change, go back to the initial configuration and recompute the layout solution. Now, we might still accept a bugfix for a flawed algorithm. But JDK-8299753 is an enhancement, not a bugfix. I'm not sure what to make of this: it's obviously a flawed approach, and basing an enhancement on a flawed approach means that someone would have to come back to this issue in the future and solve it correctly. I don't think that the issue at hand is so severe that it's a forced move to integrate this interim solution. - PR Comment: https://git.openjdk.org/jfx/pull/1156#issuecomment-1622458602
Re: RFR: 8290310: ChangeListener events are incorrect or misleading when a nested change occurs
On Tue, 4 Apr 2023 15:22:48 GMT, John Hendrikx wrote:
> This provides and uses a new implementation of `ExpressionHelper`, called
> `ListenerManager` with improved semantics.
>
> # Behavior
>
> |Listener...|ExpressionHelper|ListenerManager|
> |---|---|---|
> |Invocation Order|In order they were registered, invalidation listeners
> always before change listeners|(unchanged)|
> |Removal during Notification|All listeners present when notification started
> are notified, but excluded for any nested changes|Listeners are removed
> immediately regardless of nesting|
> |Addition during Notification|Only listeners present when notification
> started are notified, but included for any nested changes|New listeners are
> never called during the current notification regardless of nesting|
>
> ## Nested notifications:
>
> | |ExpressionHelper|ListenerManager|
> |---|---|---|
> |Type|Depth first (call stack increases for each nested level)|(same)|
> |# of Calls|Listeners * Depth (using incorrect old values)|Collapses nested
> changes, skipping non-changes|
> |Vetoing Possible?|No|Yes|
> |Old Value correctness|Only for listeners called before listeners making
> nested changes|Always|
>
> # Performance
>
> |Listener|ExpressionHelper|ListenerManager|
> |---|---|---|
> |Addition|Array based, append in empty slot, resize as needed|(same)|
> |Removal|Array based, shift array, resize as needed|(same)|
> |Addition during notification|Array is copied, removing collected
> WeakListeners in the process|Appended when notification finishes|
> |Removal during notification|As above|Entry is `null`ed (to avoid moving
> elements in array that is being iterated)|
> |Notification completion with changes|-|Null entries (and collected
> WeakListeners) are removed|
> |Notifying Invalidation Listeners|1 ns each|(same)|
> |Notifying Change Listeners|1 ns each (*)|2-3 ns each|
>
> (*) a simple for loop is close to optimal, but unfortunately does not provide
> correct old values
>
> # Memory Use
>
> Does not include alignment, and assumes a 32-bit VM or one that is using
> compressed oops.
>
> |Listener|ExpressionHelper|ListenerManager|OldValueCaching ListenerManager|
> |---|---|---|---|
> |No Listeners|none|none|none|
> |Single InvalidationListener|16 bytes overhead|none|none|
> |Single ChangeListener|20 bytes overhead|none|16 bytes overhead|
> |Multiple listeners|57 + 4 per listener (excluding unused slots)|57 + 4 per
> listener (excluding unused slots)|61 + 4 per listener (excluding unused
> slots)|
>
> # About nested changes
>
> Nested changes are simply changes that are made to a property that is
> currently in the process of notifying its listeners. This...
This change is a great improvement over the current implementation, I'm looking
forward to it.
Some comments below:
`ListenerManager` is an obvious improvement, as it fixes incorrect behavior and
allows listeners to veto changes. However, the behavior of `ListenerManager` is
also an implementation detail and not documented anywhere. This leads me to the
following questions:
1. How will users know that they can now do all of the things that were
previously broken? Do we need a specification for what is allowed and what's
not allowed?
2. Should this behavior be required for all valid `ObservableValue`
implementations? (This would render many existing implementations defective.)
3. If `ObservableValue` implementations are not required to replicate the
`ListenerManager` behavior, we should probably make it easily discoverable
whether any particular implementation (most of them are properties) supports
nested changes/vetoing. In most of the public API, there's no obvious way to
see (without looking at the source code) whether a property implementation
extends one of the `*PropertyBase` classes.
modules/javafx.base/src/main/java/com/sun/javafx/binding/ArrayManager.java line
58:
> 56: * Constructs a new instance.
> 57: *
> 58: * @param accessor an {@link Accessor}, cannot be {@code null}
There is no `accessor` parameter.
modules/javafx.base/src/main/java/com/sun/javafx/binding/ArrayManager.java line
94:
> 92: *
> 93: * @param instance the instance it is located in, cannot be {@code
> null}
> 94: * @param array the occupied slots of the array to set
The parameter is named `occupiedSlots`, not `array`.
modules/javafx.base/src/main/java/com/sun/javafx/binding/ArrayManager.java line
168:
> 166: * @param instance a reference to the instance where the array is
> stored, cannot be {@code null}
> 167: * @param index an index to remove, cannot be negative, or greater
> than or equal to the number of occupied slots
> 168: * @returns the element that was removed, can be {@code null} if the
> element at the given index was {@code null}
Should be `@return`.
modules/javafx.base/src/main/java/com/sun/javafx/binding/ArrayManager.java line
217:
> 215: * @param instance a reference to the instance
Re: RFR: 8088923: IOOBE when adding duplicate categories to the BarChart [v5]
On Sat, 22 Jun 2024 10:55:28 GMT, Markus Mack wrote: >> This PR provides the test case given in the JBS issue, and a simple fix for >> the index calculation when inserting data after previous data with duplicate >> categories. >> >> Also, I've added a comment to `BarChart`s javadoc, clarifying the behavior >> that was apparently assumed (but broken) previously. >> >> The index lookup is skipped for performance reasons if there are no >> duplicates, corresponding to the previous implementation. >> Further optimizations would be possible, but probably are not really helpful >> without more extensive changes. The previous code already loops over all >> categories to check if they are present, typically nested in a loop adding >> many data items, thus already scaling quadratically when adding lots of >> mostly unique data points. > > Markus Mack has updated the pull request incrementally with one additional > commit since the last revision: > > add javadoc clarification Marked as reviewed by mstrauss (Committer). - PR Review: https://git.openjdk.org/jfx/pull/1476#pullrequestreview-2133846362
Re: RFR: 8088923: IOOBE when adding duplicate categories to the BarChart [v3]
On Fri, 21 Jun 2024 15:22:41 GMT, Markus Mack wrote: >> This PR provides the test case given in the JBS issue, and a simple fix for >> the index calculation when inserting data after previous data with duplicate >> categories. >> >> Also, I've added a comment to `BarChart`s javadoc, clarifying the behavior >> that was apparently assumed (but broken) previously. >> >> The index lookup is skipped for performance reasons if there are no >> duplicates, corresponding to the previous implementation. >> Further optimizations would be possible, but probably are not really helpful >> without more extensive changes. The previous code already loops over all >> categories to check if they are present, typically nested in a loop adding >> many data items, thus already scaling quadratically when adding lots of >> mostly unique data points. > > Markus Mack has updated the pull request incrementally with one additional > commit since the last revision: > > additional BarChart tests & updated fix: Insert new categories according to > the concatenation of the data of all series, skipping duplicates The changes look good. - Marked as reviewed by mstrauss (Committer). PR Review: https://git.openjdk.org/jfx/pull/1476#pullrequestreview-2133355545
Integrated: 8311895: CSS Transitions
On Tue, 16 Aug 2022 03:01:23 GMT, Michael Strauß wrote:
> Implementation of [CSS Transitions](https://www.w3.org/TR/css-transitions-1/).
>
> ### Future enhancements
> CSS transition support for backgrounds and borders: #1471
>
> ### Limitations
> This implementation supports both shorthand and longhand notations for the
> `transition` property. However, due to limitations of JavaFX CSS, mixing both
> notations doesn't work:
>
> .button {
> transition: -fx-background-color 1s;
> transition-easing-function: linear;
> }
>
> This issue should be addressed in a follow-up enhancement.
This pull request has now been integrated.
Changeset: 762f5902
Author:Michael Strauß
URL:
https://git.openjdk.org/jfx/commit/762f5902cdd2e6297996baf24f7a3e3ee8e26560
Stats: 4698 lines in 43 files changed: 4655 ins; 4 del; 39 mod
8311895: CSS Transitions
Reviewed-by: mmack, nlisker, kcr
-
PR: https://git.openjdk.org/jfx/pull/870
Re: RFR: 8311895: CSS Transitions [v17]
On Tue, 4 Jun 2024 21:14:54 GMT, Kevin Rushforth wrote: >> I think it's fine. Another option is to link to the part of the reference >> where they are. > > I would prefer to keep them in one place (scene) and link to the ones there > (`../scene/doc-files/`). I've linked to the files in `scene/doc-files/`. - PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1628544278
Re: RFR: 8311895: CSS Transitions [v23]
> Implementation of [CSS Transitions](https://www.w3.org/TR/css-transitions-1/).
>
> ### Future enhancements
> CSS transition support for backgrounds and borders: #1471
>
> ### Limitations
> This implementation supports both shorthand and longhand notations for the
> `transition` property. However, due to limitations of JavaFX CSS, mixing both
> notations doesn't work:
>
> .button {
> transition: -fx-background-color 1s;
> transition-easing-function: linear;
> }
>
> This issue should be addressed in a follow-up enhancement.
Michael Strauß has updated the pull request incrementally with one additional
commit since the last revision:
link to easing function images in scene
-
Changes:
- all: https://git.openjdk.org/jfx/pull/870/files
- new: https://git.openjdk.org/jfx/pull/870/files/10c06a97..29f43ed6
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jfx=870=22
- incr: https://webrevs.openjdk.org/?repo=jfx=870=21-22
Stats: 92 lines in 5 files changed: 0 ins; 88 del; 4 mod
Patch: https://git.openjdk.org/jfx/pull/870.diff
Fetch: git fetch https://git.openjdk.org/jfx.git pull/870/head:pull/870
PR: https://git.openjdk.org/jfx/pull/870
Re: RFR: 8311895: CSS Transitions [v22]
On Wed, 5 Jun 2024 17:43:46 GMT, Kevin Rushforth wrote: >> Michael Strauß has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 62 commits: >> >> - Merge branch 'refs/heads/master' into feature/css-transitions >> - document css parser limitation >> - documentation change >> - added documentation >> - address review comments >> - Merge branch 'refs/heads/master' into feature/css-transitions >> - extract magic string to named constant >> - use existing property in test >> - fixed documentation >> - Merge branch 'master' into feature/css-transitions >> - ... and 52 more: https://git.openjdk.org/jfx/compare/cf09d6f1...10c06a97 > > modules/javafx.graphics/src/main/docs/javafx/scene/doc-files/cssref.html line > 704: > >> 702: >> 703: > scope="row">transitionproperty >> 704: [ none | all | customident# >> ] > > Why is there a literal `#` after `` (and others below)? It > shows up when viewing the doc. The `#` indicates that one or more of the preceding entity can appear in a comma-separated list. Contrast that with `*` (used in other places in cssref.html), which indicates that zero or more of the preceding entity can appear (but doesn't prescribe a comma-separated list). See: https://www.w3.org/TR/css-values-4/#component-multipliers - PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1628479243
Re: RFR: 8332895: Support interpolation for backgrounds and borders [v2]
> 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`
> - `ImagePattern`
> - `LinearGradient`
> - `RadialGradient`
> - `Stop`
>
> Note that this PR also changes the specification of `Interpolatable` to make
> users aware that they shouldn't assume any particular identity of the object
> returned from the `interpolate()` method. This allows the implementation to
> re-use objects and reduce the number of object allocations.
Michael Strauß has updated the pull request incrementally with two additional
commits since the last revision:
- clean up imports
- add since tags
-
Changes:
- all: https://git.openjdk.org/jfx/pull/1471/files
- new: https://git.openjdk.org/jfx/pull/1471/files/25bcb1df..bb84c57d
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jfx=1471=01
- incr: https://webrevs.openjdk.org/?repo=jfx=1471=00-01
Stats: 87 lines in 16 files changed: 79 ins; 7 del; 1 mod
Patch: https://git.openjdk.org/jfx/pull/1471.diff
Fetch: git fetch https://git.openjdk.org/jfx.git pull/1471/head:pull/1471
PR: https://git.openjdk.org/jfx/pull/1471
Re: RFR: 8332895: Support interpolation for backgrounds and borders
On Mon, 3 Jun 2024 09:48:40 GMT, Florian Kirmaier wrote:
> Can you elaborate on this? Are changes in the Region.background still trigger
> change events? If not, is there a mechanism to get them? Like
> Transform.onTransformChanged? Are the previous immutable objects like
> Background and Border now mutable?
I'm not entirely sure what you mean here, but nothing how backgrounds and
borders work has changed. Both are still deeply immutable, what's new is that
they implement `Interpolatable`.
For example, if you want to change the background color of a region, you can't
reassign `BackgroundFill.fill`. Instead, you will have to create a new
`Background` instance that contains a new `BackgroundFill`, which contains the
new color. This is what CSS does when it applies a new background color.
This PR allows the CSS system to also create intermediate backgrounds that
represent the transition from one color to the next.
The `interpolate()` method may or may not return a new instance of the
interpolatable object. For example, consider the current version of
`Color.interpolate()`, which returns `this` for t<=0 and `endValue` for t>=1
(in other words, a new instance is not returned in all cases):
@Override
public Color interpolate(Color endValue, double t) {
if (t <= 0.0) return this;
if (t >= 1.0) return endValue;
...
}
However, the current specification of `Interpolatable.interpolate()` is a bit
unclear on that:
/*
* The function calculates an interpolated value along the fraction
* {@code t} between {@code 0.0} and {@code 1.0}. When {@code t} = 1.0,
* {@code endVal} is returned.
*/
The updated specification is clearer:
/**
* Returns an intermediate value between the value of this {@code
Interpolatable} and the specified
* {@code endValue} using the linear interpolation factor {@code t},
ranging from 0 (inclusive)
* to 1 (inclusive).
*
* The returned value may not be a new instance; an implementation might
also return one of the
* two existing instances if the intermediate value would be equal to one
of the existing values.
* However, this is an optimization and applications should not assume any
particular identity
* of the returned value.
*
* An implementation is not required to reject interpolation factors less
than 0 or larger than 1,
* but this specification gives no meaning to values returned outside of
this range. For example,
* an implementation might clamp the interpolation factor to [0..1], or it
might continue the linear
* interpolation outside of this range.
*/
-
PR Comment: https://git.openjdk.org/jfx/pull/1471#issuecomment-2145035539
RFR: 8332895: Support interpolation for backgrounds and borders
This PR completes the CSS Transitions story (see #870) by adding interpolation support for backgrounds and borders, making them targetable by transitions. More specifically, the following types will now implement `Interpolatable`. - `Insets` - `Background` - `BackgroundFill` - `BackgroundImage` - `BackgroundPosition` - `BackgroundSize` - `Border` - `BorderImage` - `BorderStroke` - `BorderWidths` - `CornerRadii` - `ImagePattern` - `LinearGradient` - `RadialGradient` - `Stop` Note that this PR also changes the specification of `Interpolatable` to make users aware that they shouldn't assume any particular identity of the object returned from the `interpolate()` method. This allows the implementation to re-use objects and reduce the number of object allocations. - Commit messages: - Add interpolation support for backgrounds and borders Changes: https://git.openjdk.org/jfx/pull/1471/files Webrev: https://webrevs.openjdk.org/?repo=jfx=1471=00 Issue: https://bugs.openjdk.org/browse/JDK-8332895 Stats: 2867 lines in 42 files changed: 2470 ins; 72 del; 325 mod Patch: https://git.openjdk.org/jfx/pull/1471.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1471/head:pull/1471 PR: https://git.openjdk.org/jfx/pull/1471
Re: RFR: 8330304: MenuBar: Invisible Menu works incorrectly with keyboard arrows [v2]
On Fri, 31 May 2024 15:17:19 GMT, Andy Goryachev wrote: >> The root cause is that the skin used two fields to store one entity >> (`focusedMenu` and `focusedMenuIndex`), causing mismatch when invisible >> menu(s) are present. >> >> The fix involves using a single index variable. >> >> Also wanted to note that in theory, `openMenu` and `openMenuButton` can also >> be replaced with a single boolean, but I decided not to change these in >> order to keep the amount of diffs to a minimum. > > Andy Goryachev has updated the pull request incrementally with one additional > commit since the last revision: > > review comments LGTM - Marked as reviewed by mstrauss (Committer). PR Review: https://git.openjdk.org/jfx/pull/1467#pullrequestreview-2091162499
Re: RFR: 8311895: CSS Transitions [v22]
> Implementation of [CSS
> Transitions](https://gist.github.com/mstr2/c72f8c9faa87de14926978f517a6018a).
>
> ### Future enhancements
> CSS transitions requires all participating objects to implement the
> `Interpolatable` interface. For example, targeting `-fx-background-color`
> only works if all background-related objects are interpolatable: `Color`,
> `BackgroundFill`, and `Background`.
>
> In a follow-up PR, the following types will implement the `Interpolatable`
> interface:
> `LinearGradient`, `RadialGradient`, `Stop`, `Background`, `BackgroundFill`,
> `BackgroundImage`, `BackgroundPosition`, `BackgroundSize`,
> `BackgroundStroke`, `BorderWidths`, `CornerRadii`, `Insets`.
>
> ### Limitations
> This implementation supports both shorthand and longhand notations for the
> `transition` property. However, due to limitations of JavaFX CSS, mixing both
> notations doesn't work:
>
> .button {
> transition: -fx-background-color 1s;
> transition-easing-function: linear;
> }
>
> This issue should be addressed in a follow-up enhancement.
Michael Strauß has updated the pull request with a new target base due to a
merge or a rebase. The pull request now contains 62 commits:
- Merge branch 'refs/heads/master' into feature/css-transitions
- document css parser limitation
- documentation change
- added documentation
- address review comments
- Merge branch 'refs/heads/master' into feature/css-transitions
- extract magic string to named constant
- use existing property in test
- fixed documentation
- Merge branch 'master' into feature/css-transitions
- ... and 52 more: https://git.openjdk.org/jfx/compare/cf09d6f1...10c06a97
-
Changes: https://git.openjdk.org/jfx/pull/870/files
Webrev: https://webrevs.openjdk.org/?repo=jfx=870=21
Stats: 4786 lines in 47 files changed: 4743 ins; 4 del; 39 mod
Patch: https://git.openjdk.org/jfx/pull/870.diff
Fetch: git fetch https://git.openjdk.org/jfx.git pull/870/head:pull/870
PR: https://git.openjdk.org/jfx/pull/870
Re: RFR: 8330304: MenuBar: Invisible Menu works incorrectly with keyboard arrows
On Thu, 30 May 2024 15:36:37 GMT, Andy Goryachev wrote:
> The root cause is that the skin used two fields to store one entity
> (`focusedMenu` and `focusedMenuIndex`), causing mismatch when invisible
> menu(s) are present.
>
> The fix involves using a single index variable.
>
> Also wanted to note that in theory, `openMenu` and `openMenuButton` can also
> be replaced with a single boolean, but I decided not to change these in order
> to keep the amount of diffs to a minimum.
modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java
line 735:
> 733: // package protected for testing purposes
> 734: MenuBarButton menuBarButtonAt(int i) {
> 735: if (i < container.getChildren().size()) {
This method throws `IndexOutOfBoundsException` if the index is negative, but
returns `null` if the index is larger than the collection size. None of the
callers account for this, so we'd just get `NullPointerException` at the call
sites.
Since you touched this method, I suggest replacing the implementation with
MenuBarButton menuBarButtonAt(int i) {
return (MenuBarButton)container.getChildren().get(i);
}
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1467#discussion_r1622359643
Re: RFR: 8289115: Touch events is not dispatched after upgrade to JAVAFX17+
On Tue, 28 May 2024 08:09:41 GMT, Florian Kirmaier wrote: > I can confirm that this change fixes a Problem for more users. Actually, i > have a built with exactly the same change. So if this doesn't break anything, > it would be great to see this integrated. Feel free to approve if you think the patch is good to go. - PR Comment: https://git.openjdk.org/jfx/pull/1459#issuecomment-2137251075
Integrated: 8087444: CornerRadii with different horizontal and vertical values treated as uniform
On Tue, 28 May 2024 16:15:25 GMT, Michael Strauß wrote: > This PR fixes the incorrect computation of the `CornerRadii.uniform` flag for > the 16-argument constructor. > > I've also added tests for all other constructors. This pull request has now been integrated. Changeset: 2e1427e8 Author:Michael Strauß URL: https://git.openjdk.org/jfx/commit/2e1427e8c873c10c0929ffdf385bec305f13f41d Stats: 82 lines in 2 files changed: 69 ins; 1 del; 12 mod 8087444: CornerRadii with different horizontal and vertical values treated as uniform Reviewed-by: angorya, jhendrikx - PR: https://git.openjdk.org/jfx/pull/1465
Re: RFR: 8311895: CSS Transitions [v21]
> Implementation of [CSS
> Transitions](https://gist.github.com/mstr2/c72f8c9faa87de14926978f517a6018a).
>
> ### Future enhancements
> CSS transitions requires all participating objects to implement the
> `Interpolatable` interface. For example, targeting `-fx-background-color`
> only works if all background-related objects are interpolatable: `Color`,
> `BackgroundFill`, and `Background`.
>
> In a follow-up PR, the following types will implement the `Interpolatable`
> interface:
> `LinearGradient`, `RadialGradient`, `Stop`, `Background`, `BackgroundFill`,
> `BackgroundImage`, `BackgroundPosition`, `BackgroundSize`,
> `BackgroundStroke`, `BorderWidths`, `CornerRadii`, `Insets`.
>
> ### Limitations
> This implementation supports both shorthand and longhand notations for the
> `transition` property. However, due to limitations of JavaFX CSS, mixing both
> notations doesn't work:
>
> .button {
> transition: -fx-background-color 1s;
> transition-easing-function: linear;
> }
>
> This issue should be addressed in a follow-up enhancement.
Michael Strauß has updated the pull request incrementally with one additional
commit since the last revision:
document css parser limitation
-
Changes:
- all: https://git.openjdk.org/jfx/pull/870/files
- new: https://git.openjdk.org/jfx/pull/870/files/f64ad706..8a938dc8
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jfx=870=20
- incr: https://webrevs.openjdk.org/?repo=jfx=870=19-20
Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod
Patch: https://git.openjdk.org/jfx/pull/870.diff
Fetch: git fetch https://git.openjdk.org/jfx.git pull/870/head:pull/870
PR: https://git.openjdk.org/jfx/pull/870
Re: RFR: 8322964: Optimize performance of CSS selector matching [v13]
On Tue, 28 May 2024 21:44:36 GMT, John Hendrikx wrote: >> Improves performance of selector matching in the CSS subsystem. This is done >> by using custom set implementation which are highly optimized for the most >> common cases where the number of selectors is small (most commonly 1 or 2). >> It also should be more memory efficient for medium sized and large >> applications which have many style names defined in various CSS files. >> >> Due to the optimization, the concept of `StyleClass`, which was only >> introduced to assign a fixed bit index for each unique style class name >> encountered, is no longer needed. This is because style classes are no >> longer stored in a `BitSet` which required a fixed index per encountered >> style class. >> >> The performance improvements are the result of several factors: >> - Less memory use when only very few style class names are used in selectors >> and styles from a large pool of potential styles (a `BitSet` for potentially >> 1000 different style names needed 1000 bits (worst case) as it was not >> sparse). >> - Specialized sets for small number of elements (0, 1, 2, 3-9 and 10+) >> - Specialized sets are append only (reduces code paths) and can be made read >> only without requiring a wrapper >> - Iterator creation is avoided when doing `containsAll` check thanks to the >> inverse function `isSuperSetOf` >> - Avoids making a copy of the list of style class names to compare (to >> convert them to `StyleClass` and put them into a `Set`) -- this copy could >> not be cached and was always discarded immediately after... >> >> The overall performance was tested using the JFXCentral application which >> displays about 800 nodes on its start page with about 1000 styles in various >> style sheets (and which allows to refresh this page easily). >> >> On JavaFX 20, the fastest refresh speed was 121 ms on my machine. With the >> improvements in this PR, the fastest refresh had become 89 ms. The speed >> improvement is the result of a 30% faster `Scene#doCSSPass`, which takes up >> the bulk of the time to refresh the JFXCentral main page (about 100 ms >> before vs 70 ms after the change). > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Add copyright header Marked as reviewed by mstrauss (Committer). - PR Review: https://git.openjdk.org/jfx/pull/1316#pullrequestreview-2083924662
Re: RFR: 8087444: CornerRadii with different horizontal and vertical values treated as uniform
On Tue, 28 May 2024 21:15:47 GMT, John Hendrikx wrote: >> This PR fixes the incorrect computation of the `CornerRadii.uniform` flag >> for the 16-argument constructor. >> >> I've also added tests for all other constructors. > > modules/javafx.graphics/src/main/java/javafx/scene/layout/CornerRadii.java > line 315: > >> 313: bottomRightHorizontalRadiusAsPercent || >> bottomRightVerticalRadiusAsPercent || >> 314: bottomLeftVerticalRadiusAsPercent || >> bottomLeftHorizontalRadiusAsPercent; >> 315: uniform = topLeftHorizontalRadius == topLeftVerticalRadius && > > I found the diff a bit troubling to read, but from what I gather, all the > radii need to be equal to qualify for uniformity, and what you did here is > add a `topLeftHorizontalRadius == topLeftVerticalRadius` (and percent > version) to this check. > > You also already improved the readability by using `topLeftHorizontalRadius` > everywhere. It might be even better to extract this one to a new variable, > so it is even more clear that you're comparing all the other radii with the > same value. I tried that, but in my eyes it didn't clearly improve readability beyond the current version. - PR Review Comment: https://git.openjdk.org/jfx/pull/1465#discussion_r1617913798
Re: RFR: 8087444: CornerRadii with different horizontal and vertical values treated as uniform
On Tue, 28 May 2024 17:55:24 GMT, Andy Goryachev wrote:
>> This PR fixes the incorrect computation of the `CornerRadii.uniform` flag
>> for the 16-argument constructor.
>>
>> I've also added tests for all other constructors.
>
> modules/javafx.graphics/src/test/java/test/javafx/scene/layout/CornerRadiiTest.java
> line 65:
>
>> 63:
>> 64: @Nested
>> 65: class IsUniformTests {
>
> just curious - what was the rationale for `@Nested` here, as the number of
> the test cases is fairly small.
I will add some more tests with
[JDK-8332895](https://bugs.openjdk.org/browse/JDK-8332895) (adding
interpolation support), and the `@Nested` nicely groups all tests for a given
functionality.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1465#discussion_r1617700166
Re: RFR: 8332863: Crash in JPEG decoder if we enable MEM_STATS
On Fri, 24 May 2024 06:48:50 GMT, Jayathirth D V wrote: > In IJG library's jmemmgr.c file we can define MEM_STATS(by default this flag > is not defined and we don't see any issue) to enable printing of memory > statistics log. But if we enable it, we get crash while disposing IJG stored > objects in jmemmgr->free-pool() function. > > > # > # A fatal error has been detected by the Java Runtime Environment: > # > # SIGSEGV (0xb) at pc=0x0001269d5164, pid=47784, tid=259 > # > # JRE version: Java(TM) SE Runtime Environment (21.0+35) (build > 21+35-LTS-2513) > # Java VM: Java HotSpot(TM) 64-Bit Server VM (21+35-LTS-2513, mixed mode, > sharing, tiered, compressed oops, compressed class ptrs, g1 gc, bsd-aarch64) > # Problematic frame: > # C [libjavafx_iio.dylib+0x49164] free_pool+0x88 > # > # No core dump will be written. Core dumps have been disabled. To enable core > dumping, try "ulimit -c unlimited" before starting Java again > # > # If you would like to submit a bug report, please visit: > # https://bugreport.java.com/bugreport/crash.jsp > # The crash happened outside the Java Virtual Machine in native code. > # See problematic frame for where to report the bug. > > --- T H R E A D --- > > Current thread (0x000121a42c00): JavaThread "JavaFX Application Thread" > [_thread_in_native, id=259, stack(0x00016d11c000,0x00016d918000) > (8176K)] > > Stack: [0x00016d11c000,0x00016d918000], sp=0x00016d912780, free > space=8153k > Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native > code) > C [libjavafx_iio.dylib+0x49164] free_pool+0x88 > C [libjavafx_iio.dylib+0x49410] self_destruct+0x3c > C [libjavafx_iio.dylib+0xe888] jpeg_destroy+0x3c > C [libjavafx_iio.dylib+0x4bb1c] imageio_dispose+0x98 > C [libjavafx_iio.dylib+0x4b178] disposeIIO+0x2c > C [libjavafx_iio.dylib+0x4b140] > Java_com_sun_javafx_iio_jpeg_JPEGImageLoader_disposeNative+0x2c > > > This is happening because we delete the error handler before we actually > start deleting IJG stored objects and while freeing the IJG objects we try to > access cinfo->err->trace_level of error handler. This early deletion of error > handler is happening in jpegloader.c->imageio_dispose() function. > > I have moved deletion of error handler logic after we destroy IJG stored > objects in jpegloader.c->imageio_dispose(). This resolves this issue. > There is no regression test case because we need to enable MEM_STATS flag to > see this issue. > Ran graphics unit tests also and i don't see any issues with this change. This is a safe change, because `jpeg_destroy` doesn't change the `err` field that contains the application-provided error manager. - Marked as reviewed by mstrauss (Committer). PR Review: https://git.openjdk.org/jfx/pull/1463#pullrequestreview-2083527655
RFR: 8087444: CornerRadii with different horizontal and vertical values treated as uniform
This PR fixes the incorrect computation of the `CornerRadii.uniform` flag for the 16-argument constructor. I've also added tests for all other constructors. - Commit messages: - fixed computation of uniform flag - failing test Changes: https://git.openjdk.org/jfx/pull/1465/files Webrev: https://webrevs.openjdk.org/?repo=jfx=1465=00 Issue: https://bugs.openjdk.org/browse/JDK-8087444 Stats: 82 lines in 2 files changed: 69 ins; 1 del; 12 mod Patch: https://git.openjdk.org/jfx/pull/1465.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1465/head:pull/1465 PR: https://git.openjdk.org/jfx/pull/1465
Re: RFR: 8322964: Optimize performance of CSS selector matching [v12]
On Tue, 28 May 2024 04:48:32 GMT, John Hendrikx wrote: >> Improves performance of selector matching in the CSS subsystem. This is done >> by using custom set implementation which are highly optimized for the most >> common cases where the number of selectors is small (most commonly 1 or 2). >> It also should be more memory efficient for medium sized and large >> applications which have many style names defined in various CSS files. >> >> Due to the optimization, the concept of `StyleClass`, which was only >> introduced to assign a fixed bit index for each unique style class name >> encountered, is no longer needed. This is because style classes are no >> longer stored in a `BitSet` which required a fixed index per encountered >> style class. >> >> The performance improvements are the result of several factors: >> - Less memory use when only very few style class names are used in selectors >> and styles from a large pool of potential styles (a `BitSet` for potentially >> 1000 different style names needed 1000 bits (worst case) as it was not >> sparse). >> - Specialized sets for small number of elements (0, 1, 2, 3-9 and 10+) >> - Specialized sets are append only (reduces code paths) and can be made read >> only without requiring a wrapper >> - Iterator creation is avoided when doing `containsAll` check thanks to the >> inverse function `isSuperSetOf` >> - Avoids making a copy of the list of style class names to compare (to >> convert them to `StyleClass` and put them into a `Set`) -- this copy could >> not be cached and was always discarded immediately after... >> >> The overall performance was tested using the JFXCentral application which >> displays about 800 nodes on its start page with about 1000 styles in various >> style sheets (and which allows to refresh this page easily). >> >> On JavaFX 20, the fastest refresh speed was 121 ms on my machine. With the >> improvements in this PR, the fastest refresh had become 89 ms. The speed >> improvement is the result of a 30% faster `Scene#doCSSPass`, which takes up >> the bulk of the time to refresh the JFXCentral main page (about 100 ms >> before vs 70 ms after the change). > > John Hendrikx has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 21 commits: > > - Merge branch 'openjdk:master' into feature/selector-performance-improvement > - Merge remote-tracking branch 'upstream/master' into > feature/selector-performance-improvement > - Added 100% coverage test for FixedCapacitySet > - Move getStyleClassNames to location it was introduced to reduce diff > - Remove deprecations > - Add @Deprecated tag > - Remove commented code in BitSetTest > - Remove unnecessary addAll overrides > - Optimize performance of OpenAddressed Set just in case it is ever used > - Fix docs > - ... and 11 more: https://git.openjdk.org/jfx/compare/31fe622c...d72aefab Marked as reviewed by mstrauss (Committer). - PR Review: https://git.openjdk.org/jfx/pull/1316#pullrequestreview-2082501449
Re: RFR: 8322964: Optimize performance of CSS selector matching [v12]
On Tue, 28 May 2024 11:01:21 GMT, John Hendrikx wrote: >> modules/javafx.graphics/src/main/java/com/sun/javafx/css/FixedCapacitySet.java >> line 419: >> >>> 417: */ >>> 418: >>> 419: assert elements.length > requestedCapacity : "must have >>> more buckets than capacity"; >> >> This just asserts that the previous three lines of code were correct. Seems >> a bit excessive to me, if you really need this tested, I'd prefer a unit >> test. > > True, but I think it's a lot easier to follow than the logic before it. I see > it a bit more as a hint to future maintainers (which might be me) that there > are assumptions being made here that code relies on. I understand that, my objection is only with the `assert` keyword. I don't like test code in production code (which this is, it verifies that an assumption holds). I think only unit tests should contain test code, but it's a minor issue and I'll reapprove as-is. - PR Review Comment: https://git.openjdk.org/jfx/pull/1316#discussion_r1617055213
Re: RFR: 8322964: Optimize performance of CSS selector matching [v12]
On Tue, 28 May 2024 04:48:32 GMT, John Hendrikx wrote: >> Improves performance of selector matching in the CSS subsystem. This is done >> by using custom set implementation which are highly optimized for the most >> common cases where the number of selectors is small (most commonly 1 or 2). >> It also should be more memory efficient for medium sized and large >> applications which have many style names defined in various CSS files. >> >> Due to the optimization, the concept of `StyleClass`, which was only >> introduced to assign a fixed bit index for each unique style class name >> encountered, is no longer needed. This is because style classes are no >> longer stored in a `BitSet` which required a fixed index per encountered >> style class. >> >> The performance improvements are the result of several factors: >> - Less memory use when only very few style class names are used in selectors >> and styles from a large pool of potential styles (a `BitSet` for potentially >> 1000 different style names needed 1000 bits (worst case) as it was not >> sparse). >> - Specialized sets for small number of elements (0, 1, 2, 3-9 and 10+) >> - Specialized sets are append only (reduces code paths) and can be made read >> only without requiring a wrapper >> - Iterator creation is avoided when doing `containsAll` check thanks to the >> inverse function `isSuperSetOf` >> - Avoids making a copy of the list of style class names to compare (to >> convert them to `StyleClass` and put them into a `Set`) -- this copy could >> not be cached and was always discarded immediately after... >> >> The overall performance was tested using the JFXCentral application which >> displays about 800 nodes on its start page with about 1000 styles in various >> style sheets (and which allows to refresh this page easily). >> >> On JavaFX 20, the fastest refresh speed was 121 ms on my machine. With the >> improvements in this PR, the fastest refresh had become 89 ms. The speed >> improvement is the result of a 30% faster `Scene#doCSSPass`, which takes up >> the bulk of the time to refresh the JFXCentral main page (about 100 ms >> before vs 70 ms after the change). > > John Hendrikx has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 21 commits: > > - Merge branch 'openjdk:master' into feature/selector-performance-improvement > - Merge remote-tracking branch 'upstream/master' into > feature/selector-performance-improvement > - Added 100% coverage test for FixedCapacitySet > - Move getStyleClassNames to location it was introduced to reduce diff > - Remove deprecations > - Add @Deprecated tag > - Remove commented code in BitSetTest > - Remove unnecessary addAll overrides > - Optimize performance of OpenAddressed Set just in case it is ever used > - Fix docs > - ... and 11 more: https://git.openjdk.org/jfx/compare/31fe622c...d72aefab modules/javafx.graphics/src/main/java/com/sun/javafx/css/FixedCapacitySet.java line 419: > 417: */ > 418: > 419: assert elements.length > requestedCapacity : "must have more > buckets than capacity"; This just asserts that the previous three lines of code were correct. Seems a bit excessive to me, if you really need this tested, I'd prefer a unit test. - PR Review Comment: https://git.openjdk.org/jfx/pull/1316#discussion_r1616923824
Re: RFR: 8311895: CSS Transitions [v20]
> Implementation of [CSS
> Transitions](https://gist.github.com/mstr2/c72f8c9faa87de14926978f517a6018a).
>
> ### Future enhancements
> CSS transitions requires all participating objects to implement the
> `Interpolatable` interface. For example, targeting `-fx-background-color`
> only works if all background-related objects are interpolatable: `Color`,
> `BackgroundFill`, and `Background`.
>
> In a follow-up PR, the following types will implement the `Interpolatable`
> interface:
> `LinearGradient`, `RadialGradient`, `Stop`, `Background`, `BackgroundFill`,
> `BackgroundImage`, `BackgroundPosition`, `BackgroundSize`,
> `BackgroundStroke`, `BorderWidths`, `CornerRadii`, `Insets`.
>
> ### Limitations
> This implementation supports both shorthand and longhand notations for the
> `transition` property. However, due to limitations of JavaFX CSS, mixing both
> notations doesn't work:
>
> .button {
> transition: -fx-background-color 1s;
> transition-easing-function: linear;
> }
>
> This issue should be addressed in a follow-up enhancement.
Michael Strauß has updated the pull request incrementally with one additional
commit since the last revision:
documentation change
-
Changes:
- all: https://git.openjdk.org/jfx/pull/870/files
- new: https://git.openjdk.org/jfx/pull/870/files/4cb6c876..f64ad706
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jfx=870=19
- incr: https://webrevs.openjdk.org/?repo=jfx=870=18-19
Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jfx/pull/870.diff
Fetch: git fetch https://git.openjdk.org/jfx.git pull/870/head:pull/870
PR: https://git.openjdk.org/jfx/pull/870
Re: RFR: 8311895: CSS Transitions [v19]
On Sat, 25 May 2024 23:19:03 GMT, Nir Lisker wrote: >> Michael Strauß has updated the pull request incrementally with one >> additional commit since the last revision: >> >> added documentation > > modules/javafx.graphics/src/main/docs/javafx/scene/doc-files/cssref.html line > 749: > >> 747: The property value is set programmatically >> 748: The property is bound >> 749: The node becomes invisible > > I would mention that this relates to the `visible` property and not to the > `opacity` one (the node is invisible if opacity is 0). > > Other than that, looks good. I've changed it to `The node (or any of its parents) is invisible, as indicated by the Node.visible property`. - PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1615101199
Re: RFR: 8311895: CSS Transitions [v18]
On Sat, 25 May 2024 21:17:06 GMT, Nir Lisker wrote: > This is what I would expect, so looks good. Where is this mentioned to the > user? Good question. Since we don't have any suitable API elements for javadocs, I've added some documentation to the CSS reference. - PR Comment: https://git.openjdk.org/jfx/pull/870#issuecomment-2131488168
Re: RFR: 8311895: CSS Transitions [v19]
> Implementation of [CSS
> Transitions](https://gist.github.com/mstr2/c72f8c9faa87de14926978f517a6018a).
>
> ### Future enhancements
> CSS transitions requires all participating objects to implement the
> `Interpolatable` interface. For example, targeting `-fx-background-color`
> only works if all background-related objects are interpolatable: `Color`,
> `BackgroundFill`, and `Background`.
>
> In a follow-up PR, the following types will implement the `Interpolatable`
> interface:
> `LinearGradient`, `RadialGradient`, `Stop`, `Background`, `BackgroundFill`,
> `BackgroundImage`, `BackgroundPosition`, `BackgroundSize`,
> `BackgroundStroke`, `BorderWidths`, `CornerRadii`, `Insets`.
>
> ### Limitations
> This implementation supports both shorthand and longhand notations for the
> `transition` property. However, due to limitations of JavaFX CSS, mixing both
> notations doesn't work:
>
> .button {
> transition: -fx-background-color 1s;
> transition-easing-function: linear;
> }
>
> This issue should be addressed in a follow-up enhancement.
Michael Strauß has updated the pull request incrementally with one additional
commit since the last revision:
added documentation
-
Changes:
- all: https://git.openjdk.org/jfx/pull/870/files
- new: https://git.openjdk.org/jfx/pull/870/files/d3184e6c..4cb6c876
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jfx=870=18
- incr: https://webrevs.openjdk.org/?repo=jfx=870=17-18
Stats: 12 lines in 1 file changed: 10 ins; 0 del; 2 mod
Patch: https://git.openjdk.org/jfx/pull/870.diff
Fetch: git fetch https://git.openjdk.org/jfx.git pull/870/head:pull/870
PR: https://git.openjdk.org/jfx/pull/870
Re: RFR: 8311895: CSS Transitions [v17]
On Sat, 25 May 2024 19:39:12 GMT, Nir Lisker wrote: > I still don't know what happens when a value is programmatically set while a > css transition is in progress. What I understood is that binding the property > will not allow the transition to start/continue, but didn't see where setting > a value was mentioned. Any of the following actions will cancel a running transition: 1. Setting the property value 2. Binding the property 3. Making the node invisible (i.e. `isTreeVisible()` returns false) 4. Removing the node from the scene graph In effect, transitions are only active when "left alone", any programmatic interaction will cancel them. - PR Comment: https://git.openjdk.org/jfx/pull/870#issuecomment-2131459204
Re: RFR: 8311895: CSS Transitions [v17]
On Sat, 25 May 2024 19:35:56 GMT, Nir Lisker wrote: >> Michael Strauß has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 57 commits: >> >> - Merge branch 'refs/heads/master' into feature/css-transitions >> - extract magic string to named constant >> - use existing property in test >> - fixed documentation >> - Merge branch 'master' into feature/css-transitions >> - update 'since' tags >> - Fix javadoc error >> - Change javadoc comment >> - Merge branch 'master' into feature/css-transitions >> - Discard redundant transitions in StyleableProperty impls >> - ... and 47 more: https://git.openjdk.org/jfx/compare/94aa2b68...a43dee30 > > modules/javafx.graphics/src/main/docs/javafx/scene/doc-files/cssref.html line > 690: > >> 688: changed, it smoothly transitions to the new value over a period >> of time. Implicit transitions are supported >> 689: for all primitive types, as well as for types that implement >> javafx.animation.Interpolatable. >> 690: Transitions can be defined for any node in the JavaFX scene >> graph with the following properties: > > The way this is phrased makes it sound like the node has "the following > properties", not the transition. Maybe move that part: > "Transitions with the following properties can be defined for any node in the > JavaFX scene graph", or just add a comma. I understand that you're saying that `property`, `duration`, `timing-function`, and `delay` are all sub-properties of `transition`. However, from a CSS perspective, `transition-property`, `transition-duration`, `transition-timing-function` and `transition-delay` are properties of `Node`, in the same way as `-fx-background-color`, `-fx-background-insets`, etc. are properties of `Node`. `transition` is a short-hand property that combines all four properties into one (we don't have a short-hand property for backgrounds yet). I think that both mental models are basically correct (four properties of node, vs. four sub-properties of transition). - PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1614886767
Re: RFR: 8311895: CSS Transitions [v17]
On Sat, 25 May 2024 19:28:56 GMT, Nir Lisker wrote: >> Michael Strauß has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 57 commits: >> >> - Merge branch 'refs/heads/master' into feature/css-transitions >> - extract magic string to named constant >> - use existing property in test >> - fixed documentation >> - Merge branch 'master' into feature/css-transitions >> - update 'since' tags >> - Fix javadoc error >> - Change javadoc comment >> - Merge branch 'master' into feature/css-transitions >> - Discard redundant transitions in StyleableProperty impls >> - ... and 47 more: https://git.openjdk.org/jfx/compare/94aa2b68...a43dee30 > > modules/javafx.graphics/src/main/docs/javafx/scene/doc-files/cssref.html line > 688: > >> 686: Transitions >> 687: JavaFX supports implicit transitions for properties >> that are styled by CSS. When a property value is >> 688: changed, it smoothly transitions to the new value over a period >> of time. Implicit transitions are supported > > Maybe not so smoothly when using a step interpolator? I've changed the wording a bit. > modules/javafx.graphics/src/main/java/com/sun/javafx/css/TransitionDefinition.java > line 40: > >> 38: * @param duration duration of the transition >> 39: * @param delay delay after which the transition is started; if >> negative, the transition starts >> 40: * immediately, but will appear to have begun at an earlier >> point in time > > Why accept a negative delay? An > [`Animation`](https://openjfx.io/javadoc/22/javafx.graphics/javafx/animation/Animation.html#getDelay()) > doesn't accept it. The W3C specification for the [transition-delay](https://www.w3.org/TR/css-transitions-1/#transition-delay-property) property mandates it, and this PR is a complete implementation of the specification. You're right that `Animation` doesn't support negative delays, but at least for implicit transitions, negative delays have some utility. Maye we can revisit the choice to disallow negative delays for `Animation` later. - PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1614870557 PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1614870213
Re: RFR: 8311895: CSS Transitions [v17]
On Sat, 25 May 2024 17:40:01 GMT, Nir Lisker wrote:
>> Michael Strauß has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 57 commits:
>>
>> - Merge branch 'refs/heads/master' into feature/css-transitions
>> - extract magic string to named constant
>> - use existing property in test
>> - fixed documentation
>> - Merge branch 'master' into feature/css-transitions
>> - update 'since' tags
>> - Fix javadoc error
>> - Change javadoc comment
>> - Merge branch 'master' into feature/css-transitions
>> - Discard redundant transitions in StyleableProperty impls
>> - ... and 47 more: https://git.openjdk.org/jfx/compare/94aa2b68...a43dee30
>
> modules/javafx.graphics/src/main/java/javafx/animation/Interpolator.java line
> 328:
>
>> 326: * @since 23
>> 327: */
>> 328: public static Interpolator STEPS(int intervals, StepPosition
>> position) {
>
> Static method names shouldn't be named like constants, although
> `Interpolator` does this for other methods already. Not sure if this trend
> should continue.
Changed the parameter name to `intervalCount`.
I agree that the uppercase naming scheme in `Interpolator` is a bit
unfortunate, but at this point I think consistency is more important. We're
unlikely to add more built-in interpolators in any case.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1614865200
Re: RFR: 8311895: CSS Transitions [v17]
On Sat, 25 May 2024 10:40:05 GMT, Nir Lisker wrote:
>> Michael Strauß has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 57 commits:
>>
>> - Merge branch 'refs/heads/master' into feature/css-transitions
>> - extract magic string to named constant
>> - use existing property in test
>> - fixed documentation
>> - Merge branch 'master' into feature/css-transitions
>> - update 'since' tags
>> - Fix javadoc error
>> - Change javadoc comment
>> - Merge branch 'master' into feature/css-transitions
>> - Discard redundant transitions in StyleableProperty impls
>> - ... and 47 more: https://git.openjdk.org/jfx/compare/94aa2b68...a43dee30
>
> modules/javafx.graphics/src/main/java/javafx/animation/Interpolator.java line
> 277:
>
>> 275: * @since 23
>> 276: */
>> 277: public enum StepPosition {
>
> I think it would be helpful to include (or link to) images that show what the
> steps for each option looks like. The verbal description is a bit technical.
I've included the images that are also used in the CSS reference documentation.
Now there are two copies of these images in two different `doc-files` folders,
but I guess that's okay.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1614863775
Re: RFR: 8311895: CSS Transitions [v18]
> Implementation of [CSS
> Transitions](https://gist.github.com/mstr2/c72f8c9faa87de14926978f517a6018a).
>
> ### Future enhancements
> CSS transitions requires all participating objects to implement the
> `Interpolatable` interface. For example, targeting `-fx-background-color`
> only works if all background-related objects are interpolatable: `Color`,
> `BackgroundFill`, and `Background`.
>
> In a follow-up PR, the following types will implement the `Interpolatable`
> interface:
> `LinearGradient`, `RadialGradient`, `Stop`, `Background`, `BackgroundFill`,
> `BackgroundImage`, `BackgroundPosition`, `BackgroundSize`,
> `BackgroundStroke`, `BorderWidths`, `CornerRadii`, `Insets`.
>
> ### Limitations
> This implementation supports both shorthand and longhand notations for the
> `transition` property. However, due to limitations of JavaFX CSS, mixing both
> notations doesn't work:
>
> .button {
> transition: -fx-background-color 1s;
> transition-easing-function: linear;
> }
>
> This issue should be addressed in a follow-up enhancement.
Michael Strauß has updated the pull request incrementally with one additional
commit since the last revision:
address review comments
-
Changes:
- all: https://git.openjdk.org/jfx/pull/870/files
- new: https://git.openjdk.org/jfx/pull/870/files/a43dee30..d3184e6c
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jfx=870=17
- incr: https://webrevs.openjdk.org/?repo=jfx=870=16-17
Stats: 28 lines in 7 files changed: 8 ins; 0 del; 20 mod
Patch: https://git.openjdk.org/jfx/pull/870.diff
Fetch: git fetch https://git.openjdk.org/jfx.git pull/870/head:pull/870
PR: https://git.openjdk.org/jfx/pull/870
Re: RFR: 8311895: CSS Transitions [v16]
On Fri, 24 May 2024 10:15:37 GMT, drmarmac wrote: > * Attempting to do background-color transitions doesn't work, I presume > because it's not yet `Interpolatable` in this PR. Do we need to emit a > warning in such a case? Right now the `transition` CSS code just seems to be > ignored. CSS transitions work with styleable properties that are either primitives or `Interpolatable` objects. So if you apply a transition to the sub-property `-fx-background-color`, what really happens is that the CSS subsystem creates a new `Background` instance and applies it to the `Region.background` property. However, since `Background` is not interpolatable, you won't see an animation. This will start to work once all relevant classes are interpolatable, which will come with a subsequent PR. I'm not sure if we really need a warning. The unexpected non-animation is really only due to a few missing interpolatable classes, which will be fixed soon. > * Same with the current limitation of not being able to mix shorthand and > longhand notations, do we need a warning if it is attempted? This is out of scope for this PR, since the problem is not unique to the CSS `transition` property. We should improve the CSS parser to handle shorthand/longhand notations uniformly. - PR Comment: https://git.openjdk.org/jfx/pull/870#issuecomment-2129318991
Re: RFR: 8311895: CSS Transitions [v16]
On Fri, 24 May 2024 10:02:44 GMT, drmarmac wrote:
>> Michael Strauß has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 53 commits:
>>
>> - Merge branch 'master' into feature/css-transitions
>> - update 'since' tags
>> - Fix javadoc error
>> - Change javadoc comment
>> - Merge branch 'master' into feature/css-transitions
>> - Discard redundant transitions in StyleableProperty impls
>> - Changes per review
>> - Merge branch 'master' into feature/css-transitions
>> - Merge branch 'master' into feature/css-transitions
>> - Add TransitionMediator
>> - ... and 43 more: https://git.openjdk.org/jfx/compare/aa9907a5...6614abb9
>
> modules/javafx.graphics/src/main/java/javafx/animation/Interpolator.java line
> 291:
>
>> 289: * All rise points are within the open interval (0..1).
>> 290: */
>> 291: BOTH,
>
> Looks like the docs for BOTH and NONE are swapped? (this would also affect
> the CSR)
Good catch! I've also updated the CSR.
> modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 9081:
>
>> 9079: TransitionDefinition transition = get(i);
>> 9080:
>> 9081: boolean selected =
>> "all".equals(transition.propertyName())
>
> Is "all" the same as `TransitionDefinitionCssMetaData.PROPERTY_ALL`, so the
> magic string can be avoided?
I've replaced `"all"` with a named constant in all relevant places.
> modules/javafx.graphics/src/test/java/test/javafx/scene/Node_transition_Test.java
> line 162:
>
>> 160: List transitions =
>> NodeShim.getTransitionDefinitions(node);
>> 161: assertEquals(3, transitions.size());
>> 162: assertTransitionEquals("-fx-background-color",
>> Duration.seconds(1), Duration.seconds(0.5), CSS_EASE, transitions.get(0));
>
> `node` is a `Rectangle`, which doesn't have a `background`property. Doesn't
> hurt this specific test, but maybe it's better to use an existing property.
I've changed the test to use `-fx-fill` instead.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1613307416
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1613307906
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1613308024
Re: RFR: 8311895: CSS Transitions [v17]
> Implementation of [CSS
> Transitions](https://gist.github.com/mstr2/c72f8c9faa87de14926978f517a6018a).
>
> ### Future enhancements
> CSS transitions requires all participating objects to implement the
> `Interpolatable` interface. For example, targeting `-fx-background-color`
> only works if all background-related objects are interpolatable: `Color`,
> `BackgroundFill`, and `Background`.
>
> In a follow-up PR, the following types will implement the `Interpolatable`
> interface:
> `LinearGradient`, `RadialGradient`, `Stop`, `Background`, `BackgroundFill`,
> `BackgroundImage`, `BackgroundPosition`, `BackgroundSize`,
> `BackgroundStroke`, `BorderWidths`, `CornerRadii`, `Insets`.
>
> ### Limitations
> This implementation supports both shorthand and longhand notations for the
> `transition` property. However, due to limitations of JavaFX CSS, mixing both
> notations doesn't work:
>
> .button {
> transition: -fx-background-color 1s;
> transition-easing-function: linear;
> }
>
> This issue should be addressed in a follow-up enhancement.
Michael Strauß has updated the pull request with a new target base due to a
merge or a rebase. The pull request now contains 57 commits:
- Merge branch 'refs/heads/master' into feature/css-transitions
- extract magic string to named constant
- use existing property in test
- fixed documentation
- Merge branch 'master' into feature/css-transitions
- update 'since' tags
- Fix javadoc error
- Change javadoc comment
- Merge branch 'master' into feature/css-transitions
- Discard redundant transitions in StyleableProperty impls
- ... and 47 more: https://git.openjdk.org/jfx/compare/94aa2b68...a43dee30
-
Changes: https://git.openjdk.org/jfx/pull/870/files
Webrev: https://webrevs.openjdk.org/?repo=jfx=870=16
Stats: 4677 lines in 43 files changed: 4634 ins; 4 del; 39 mod
Patch: https://git.openjdk.org/jfx/pull/870.diff
Fetch: git fetch https://git.openjdk.org/jfx.git pull/870/head:pull/870
PR: https://git.openjdk.org/jfx/pull/870
Re: RFR: 8319555: [TestBug] Utility for creating instruction window for manual tests [v9]
On Thu, 23 May 2024 16:11:42 GMT, Andy Goryachev wrote: > > Short of using a build system, how would I run a manual test with this new > > class? > > I am running from within Eclipse. Here is the command line (remove newlines) > it launches the EmojiTest. I think the key is to add the location of compiled > classes via `-classpath`: Yes this will work, but it requires me to compile `ManualTestWindow.java` before. That's quite a lot of heavy-lifting without tooling support (like you get from Eclipse). Maybe we should integrate the manual tests into the Gradle build, so that running a manual test might be as easy as invoking the application plugin's `:run` task for the manual test. - PR Comment: https://git.openjdk.org/jfx/pull/1413#issuecomment-2127576658
Re: prism.maxvram limitation
I wonder why there's a software-defined limit in the first place, given that there doesn't seem to be a way for JavaFX to gracefully handle failed texture loads. You say that the limit is reasonable for GPUs, but why is this any different from system memory? GPUs come in all different kinds of sizes, why is 512 MB any more significant than 256 MB or 8 GB? It's an arbitrary value that might be too small for some systems, and too large for other systems. Right now, JavaFX simply bugs out when the limit is reached. Unless a graceful degradation capability is added, I suggest to remove the limit entirely and let JavaFX bug out when the actual hardware limits are reached. On Tue, May 14, 2024 at 4:14 PM Eduard Sedov wrote: > > Hi all, > > There is a long-lived bug in JavaFX: JDK-8089112: Need to handle the case of > a failed texture load when rendering large images > > JavaFX manages a pool of resources that is limited to 512Mbytes by default. > This limit can be increased by setting the 'prism.maxvram' system property. > > This limit is reasonable for the GraphicsPipelines (the D3DPipeline and the > ES2Pipeline) that are backed by a graphics device that has such a limitation. > > But it does not make sense for pipelines that use only the main memory: the > J2DPipeline and perhaps the SWPipeline. > > The J2DPipeline is currently used for printing. For example, printing an > image on Letter paper using 600PPI printer requires 134_640_000 bytes. When > the printed image is redirected to a PDF printer, even higher resolution is > needed to get adequate quality because the PDF can zoom in. This often > exceeds the limitation and ends in a NullPointerException if the allocated > textures exceed the specified maxvram value. > > There is no way to specify different values for each pipeline or to remove > the limitation for software pipelines. > > I would add this possibility? What do you think? > > Thanks, > Eduard > >
Re: RFR: 8319555: [TestBug] Utility for creating instruction window for manual tests [v9]
On Fri, 17 May 2024 19:22:39 GMT, Andy Goryachev wrote:
>> ## ManualTestWindow
>>
>> This facility provides the base class for manual tests which displays the
>> test instructions,
>> the UI under test, and the Pass/Fail buttons.
>>
>> Example:
>>
>>
>> public class ManualTestExample extends ManualTestWindow {
>> public ManualTestExample() {
>> super(
>> "Manual Test Example",
>> """
>> Instructions:
>> 1. you will see a button named "Test"
>> 2. press the button
>> 3. verify that the button can be pressed""",
>> 400, 250
>> );
>> }
>>
>> public static void main(String[] args) throws Exception {
>> launch(args);
>> }
>>
>> @Override
>> protected Node createContent() {
>> return new Button("Test");
>> }
>> }
>>
>>
>> Resulting application window:
>>
>> 
>>
>> Readme:
>>
>> https://github.com/andy-goryachev-oracle/jfx/blob/8319555.manual/tests/manual/util/README.md
>>
>> @prrace 's test EmojiTest has been converted to use the new test window as a
>> demonstration (also fixed the Eclipse project so it works now).
>>
>> Q: What other features can be added to the test window?
>>
>> Edit: the sources are left in their original place at the root of the
>> project.
>
> 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 15 additional
> commits since the last revision:
>
> - Merge branch 'master' into 8319555.manual
> - Merge remote-tracking branch 'origin/master' into 8319555.manual
> - Merge remote-tracking branch 'origin/8319555.manual' into 8319555.manual
> - removed example
> - cleanup
> - whitespace
> - extends application
> - works
> - .
> - sources moved back
> - ... and 5 more: https://git.openjdk.org/jfx/compare/681030f4...e4c8d29a
I run manual tests on the command line like so:
java --module-path=$env:JFX_SDK --add-modules=javafx.controls
./tests/manual/events/PlatformPreferencesChangedTest.java
This doesn't work with the new utility class, since it is outside of the class
file hierarchy.
Short of using a build system, how would I run a manual test with this new
class?
-
PR Comment: https://git.openjdk.org/jfx/pull/1413#issuecomment-2127511624
Re: RFR: 8289115: Touch events is not dispatched after upgrade to JAVAFX17+
On Tue, 21 May 2024 14:25:51 GMT, Michael Strauß wrote: > This PR fixes a bug > ([JDK-8289115](https://bugs.openjdk.org/browse/JDK-8289115)) that was > introduced with #394, which changed the following line in the > `NotifyTouchInput` function (ViewContainer.cpp): > > - const bool isDirect = true; > + const bool isDirect = IsTouchEvent(); > > > `IsTouchEvent` is a small utility function that uses the Windows SDK function > `GetMessageExtraInfo` to distinguish between mouse events and pen events as > described in this article: > https://learn.microsoft.com/en-us/windows/win32/tablet/system-events-and-mouse-messages > > I think that using this function to distinguish between touchscreen events > and pen events is erroneous. The linked article states: > _"When your application receives a **mouse message** (such as WM_LBUTTONDOWN) > [...]"_ > > But we are not receiving a mouse message in `NotifyTouchInput`, we are > receiving a `WM_TOUCH` message. > I couldn't find any information on whether this API usage is also supported > for `WM_TOUCH` messages, but my testing shows that that's probably not the > case (I tested this with a XPS 15 touchscreen laptop, where > `GetMessageExtraInfo` returns 0 for `WM_TOUCH` messages). > > My suggestion is to check the `TOUCHEVENTF_PEN` flag of the `TOUCHINPUT` > structure instead: > > const bool isDirect = (ti->dwFlags & TOUCHEVENTF_PEN) == 0; @jperedadnr Maybe you could test this PR with the pen digitizer that you used for #394 to ensure that this change doesn't introduce yet another regression. - PR Comment: https://git.openjdk.org/jfx/pull/1459#issuecomment-2124119824
RFR: 8289115: Touch events is not dispatched after upgrade to JAVAFX17+
This PR fixes a bug ([JDK-8289115](https://bugs.openjdk.org/browse/JDK-8289115)) that was introduced with #394, which changed the following line in the `NotifyTouchInput` function (ViewContainer.cpp): - const bool isDirect = true; + const bool isDirect = IsTouchEvent(); `IsTouchEvent` is a small utility function that uses the Windows SDK function `GetMessageExtraInfo` to distinguish between mouse events and pen events as described in this article: https://learn.microsoft.com/en-us/windows/win32/tablet/system-events-and-mouse-messages I think that using this function to distinguish between touchscreen events and pen events is erroneous. The linked article states: _"When your application receives a **mouse message** (such as WM_LBUTTONDOWN) [...]"_ But we are not receiving a mouse message in `NotifyTouchInput`, we are receiving a `WM_TOUCH` message. I couldn't find any information on whether this API usage is also supported for `WM_TOUCH` messages, but my testing shows that that's probably not the case (I tested this with a XPS 15 touchscreen laptop, where `GetMessageExtraInfo` returns 0 for `WM_TOUCH` messages). My suggestion is to check the `TOUCHEVENTF_PEN` flag of the `TOUCHINPUT` structure instead: const bool isDirect = (ti->dwFlags & TOUCHEVENTF_PEN) == 0; - Commit messages: - Fixed stylus detection Changes: https://git.openjdk.org/jfx/pull/1459/files Webrev: https://webrevs.openjdk.org/?repo=jfx=1459=00 Issue: https://bugs.openjdk.org/browse/JDK-8289115 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/1459.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1459/head:pull/1459 PR: https://git.openjdk.org/jfx/pull/1459
Re: RFR: 8322964: Optimize performance of CSS selector matching [v9]
On Mon, 11 Mar 2024 16:54:25 GMT, John Hendrikx wrote: >> Improves performance of selector matching in the CSS subsystem. This is done >> by using custom set implementation which are highly optimized for the most >> common cases where the number of selectors is small (most commonly 1 or 2). >> It also should be more memory efficient for medium sized and large >> applications which have many style names defined in various CSS files. >> >> Due to the optimization, the concept of `StyleClass`, which was only >> introduced to assign a fixed bit index for each unique style class name >> encountered, is no longer needed. This is because style classes are no >> longer stored in a `BitSet` which required a fixed index per encountered >> style class. >> >> The performance improvements are the result of several factors: >> - Less memory use when only very few style class names are used in selectors >> and styles from a large pool of potential styles (a `BitSet` for potentially >> 1000 different style names needed 1000 bits (worst case) as it was not >> sparse). >> - Specialized sets for small number of elements (0, 1, 2, 3-9 and 10+) >> - Specialized sets are append only (reduces code paths) and can be made read >> only without requiring a wrapper >> - Iterator creation is avoided when doing `containsAll` check thanks to the >> inverse function `isSuperSetOf` >> - Avoids making a copy of the list of style class names to compare (to >> convert them to `StyleClass` and put them into a `Set`) -- this copy could >> not be cached and was always discarded immediately after... >> >> The overall performance was tested using the JFXCentral application which >> displays about 800 nodes on its start page with about 1000 styles in various >> style sheets (and which allows to refresh this page easily). >> >> On JavaFX 20, the fastest refresh speed was 121 ms on my machine. With the >> improvements in this PR, the fastest refresh had become 89 ms. The speed >> improvement is the result of a 30% faster `Scene#doCSSPass`, which takes up >> the bulk of the time to refresh the JFXCentral main page (about 100 ms >> before vs 70 ms after the change). > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Move getStyleClassNames to location it was introduced to reduce diff Looks good to me. - Marked as reviewed by mstrauss (Committer). PR Review: https://git.openjdk.org/jfx/pull/1316#pullrequestreview-2051622575
Re: RFR: 8092102: Labeled: textTruncated property [v15]
On Mon, 6 May 2024 15:23:20 GMT, Andy Goryachev wrote: >> Adds **Labeled.textTruncated** property which indicates when the text is >> visually truncated (and the ellipsis string is inserted) in order to fit the >> available width. >> >> The new property is being set by the code which computes the actual text >> string to be displayed (and which inserts the ellipsis string) in >> `LabeledSkinBase.updateDisplayedText(double,double)`. >> >> >> **Alternative** >> >> None exists as this requires changes to the core (Utils). >> >> >> **See Also** >> >> * [JDK-8327483](https://bugs.openjdk.org/browse/JDK-8327483) TreeView: Allow >> for tooltip when cell text is truncated >> * [JDK-8205211](https://bugs.openjdk.org/browse/JDK-8205211) Ability to show >> Tooltip only when text is shown with ellipsis (...) > > Andy Goryachev has updated the pull request incrementally with one additional > commit since the last revision: > > comments modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/skin/Utils.java line 236: > 234: OverrunStyle type, > 235: String ellipsisString, > 236: AtomicBoolean textTruncated Instead of returning the clipped text with the return value, and the truncation flag with an argument, have you considered just using a record to return both at the same time? That would be a cleaner solution. - PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1592079493
Re: Proposal: Public InputMap (v2)
Hi Andy!
The updated proposal seems to be a slight refinement of the original
proposal, and I think most of the points raised in the previous
discussion still stand.
As it is, I still think that this is an exceptionally large API
surface for what the feature can actually bring to the table. It's
overly specific in trying to solve problems for which general-purpose
APIs like event handlers can be extended to work just as well (and
open up many more use cases than just those of a bespoke control API).
The fact that the API tries to fix a problem with the event system
itself (unpredictable invocation order) is a red flag. We should fix
this problem where it originates, which is the event API; crafting
workaround APIs in other parts of JavaFX doesn't seem to be the right
approach to me. This would also help to break the problem down into
more manageable chunks: improve the event system first, then see how
that influences the remaining problems that need to be solved.
You provide several examples of what the InputMap API can do:
1. Adding a new key mapping
// creates a new key binding mapped to an external function
control.getInputMap().registerKey(KeyBinding.shortcut(KeyCode.W), () -> {
externalFunction();
});
I don't see why this is needed. It doesn't seem to be easier than
using a plain old event handler, it's just different.
2. Redefine an existing function
// redefine function keeping existing key mappings
getInputMap().registerFunction(Tag.COPY, (control) -> customCopy());
If I want to change the meaning of the copy() method, can I not just
override the method and provide my own implementation? Plain old Java
seems to do the job.
3. Map an existing function to a new binding
// map a new key binding
control.getInputMap().registerKey(KeyBinding.shortcut(KeyCode.W), Tag.COPY);
Again, an event handler would do the job here.
4. Obtain the default function
// obtains the default function handler
FunctionHandler h = getInputMap().getDefaultFunction(Tag.COPY);
If I override the copy() method to provide my own implementation, I
could also add a defaultCopy() method that calls the base
implementation. Then I can call both copy() and defaultCopy().
(I don't know why I would want to do this, though.)
To summarize: the way I see it, your examples don't provide a
compelling reason why we need this new API. Given the size and
complexity of the proposal, I suggest to break the problem down into
more manageable parts, and solve the most fundamental issues first. At
the very least, the issue with event invocation ordering should be
solved in the event system.
On Mon, Mar 11, 2024 at 4:22 PM Andy Goryachev
wrote:
>
> Dear JavaFX developers:
>
>
>
> Thank you all for your feedback on my earlier Behavior/InputMap proposal [6],
> [7]. There was some positive reaction to the idea of allowing for easy
> customization of JavaFX controls behavior, as well as some push back. Some
> of the objections were:
>
>
>
> desire for some static behavior to avoid the per-instance penalty
> clearer separation of concerns between controls, skins, and behaviors
> desire to have some sort of public API for behaviors
> need for addressing an issue with the event handler execution order between
> application and skins
>
>
>
> I would like to restart the discussion by submitting the updated proposal [0]
> which addresses the issues raised earlier. The new proposal retains the idea
> of allowing wider customization of key mappings via the InputMap. The new
> features include:
>
>
>
> separation of SkinInputMap from the control's InputMap
> enabling static skin input maps for stateless behaviors
> explicit priority levels for event handlers and key mappings created by the
> application and by the skin
>
>
>
> The ideas put forth in this proposal have been validated with the
> proof-of-concept migration of some non-trivial controls:
>
>
>
> complex popup-like controls: ComboBox, ColorPicker, DatePicker
> complex text input controls: PasswordField, TextArea, TextField
> control with a stateless behavior: TabPane
>
>
>
> as well as a brand new RichTextArea control being incubated [8].
>
>
>
> Please take a look at the proposal [0], a list of discussion points [1], and
> the API Specification (javadoc) [2]. While the proposed API is ready for
> review, it isn't complete nor set in stone. We are looking for feedback, and
> will update the proposal based on the suggestions we receive from the
> community. We encourage you to comment either in the mailing list, or by
> leaving comments inline in a draft pull request [3].
>
>
>
> For context, the links to the original RFE [4] and the earlier proposals [6],
> [7] are provided below.
>
>
>
> What do you think?
>
> -andy
>
>
>
>
>
> References
>
>
>
> [0] Proposal:
> https://github.com/andy-goryachev-oracle/Test/blob/main/doc/InputMap/InputMapV2.md
>
> [1] Discussion points:
>
Re: RFR: 8331616: ChangeListener is not triggered when the InvalidationListener is removed
On Fri, 3 May 2024 14:45:24 GMT, John Hendrikx wrote: > This PR provides a fix for the linked issue. > > The issue was that when an invalidation listener is removed, and the > `ExpressionHelper` type changes from `Generic` to `SingleChange` that it > would copy the current value of the `Generic` instance before it was updated > (this is because invalidation listeners trigger before change listeners and > the current value would only be updated **after** invalidation listeners > notifications were completed). > > The code now will update the current value before sending out any > notifications **if** there are change listeners present to head off this > problem. > > Added a few test cases to verify this. > > Note: the PR which replaces `ExpressionHelper` does not have this problem: > https://github.com/openjdk/jfx/pull/1081 The fix works as expected. However, I hope that we can get #1081 in as a replacement. - Marked as reviewed by mstrauss (Committer). PR Review: https://git.openjdk.org/jfx/pull/1448#pullrequestreview-2042311784
Re: RFR: 8311895: CSS Transitions [v16]
> Implementation of [CSS
> Transitions](https://gist.github.com/mstr2/c72f8c9faa87de14926978f517a6018a).
>
> ### Example
>
> .button {
> -fx-background-color: dodgerblue;
> }
>
> .button:hover {
> -fx-background-color: red;
> -fx-scale-x: 1.1;
> -fx-scale-y: 1.1;
>
> transition: -fx-background-color 0.5s ease,
> -fx-scale-x 0.5s ease,
> -fx-scale-y 0.5s ease;
> }
>
> src="https://user-images.githubusercontent.com/43553916/184781143-0520fbfe-54bf-4b8d-93ac-834708e46500.gif;
> width="200"/>
>
> ### Limitations
> This implementation supports both shorthand and longhand notations for the
> `transition` property. However, due to limitations of JavaFX CSS, mixing both
> notations doesn't work:
>
> .button {
> transition: -fx-background-color 1s;
> transition-easing-function: linear;
> }
>
> This issue should be addressed in a follow-up enhancement.
Michael Strauß has updated the pull request with a new target base due to a
merge or a rebase. The pull request now contains 53 commits:
- Merge branch 'master' into feature/css-transitions
- update 'since' tags
- Fix javadoc error
- Change javadoc comment
- Merge branch 'master' into feature/css-transitions
- Discard redundant transitions in StyleableProperty impls
- Changes per review
- Merge branch 'master' into feature/css-transitions
- Merge branch 'master' into feature/css-transitions
- Add TransitionMediator
- ... and 43 more: https://git.openjdk.org/jfx/compare/aa9907a5...6614abb9
-
Changes: https://git.openjdk.org/jfx/pull/870/files
Webrev: https://webrevs.openjdk.org/?repo=jfx=870=15
Stats: 4673 lines in 43 files changed: 4630 ins; 4 del; 39 mod
Patch: https://git.openjdk.org/jfx/pull/870.diff
Fetch: git fetch https://git.openjdk.org/jfx.git pull/870/head:pull/870
PR: https://git.openjdk.org/jfx/pull/870
Re: RFR: 8320563: Remove D3D9 code paths in favor of D3D9Ex [v3]
On Fri, 26 Apr 2024 07:53:59 GMT, Lukasz Kostyra wrote: >> JFX minimum requirements guarantee 9Ex availability, so old non-Ex paths are >> no longer needed. >> >> In multiple parts (ex. Mesh, Graphics, etc.) where the Device is acquired I >> changed the type to explicitly use `IDirect3DDevice9Ex`. Technically it >> doesn't matter much (`IDirect3DDevice9Ex` inherits `IDirect3DDevice` - it >> was leveraged to transparently use the Ex device in the backend) but now we >> don't have the non-Ex device, so that keeps it a bit more consistent and >> clear IMO. >> >> Verified by running tests on Windows 11, did not notice any regressions. >> Unfortunately I have no way to test this on older systems. > > Lukasz Kostyra has updated the pull request incrementally with one additional > commit since the last revision: > > Change pd3dEx to pd3d9 LGTM - Marked as reviewed by mstrauss (Committer). PR Review: https://git.openjdk.org/jfx/pull/1445#pullrequestreview-2024655439
Re: RFR: 8311895: CSS Transitions [v15]
> Implementation of [CSS
> Transitions](https://gist.github.com/mstr2/c72f8c9faa87de14926978f517a6018a).
>
> ### Example
>
> .button {
> -fx-background-color: dodgerblue;
> }
>
> .button:hover {
> -fx-background-color: red;
> -fx-scale-x: 1.1;
> -fx-scale-y: 1.1;
>
> transition: -fx-background-color 0.5s ease,
> -fx-scale-x 0.5s ease,
> -fx-scale-y 0.5s ease;
> }
>
> src="https://user-images.githubusercontent.com/43553916/184781143-0520fbfe-54bf-4b8d-93ac-834708e46500.gif;
> width="200"/>
>
> ### Limitations
> This implementation supports both shorthand and longhand notations for the
> `transition` property. However, due to limitations of JavaFX CSS, mixing both
> notations doesn't work:
>
> .button {
> transition: -fx-background-color 1s;
> transition-easing-function: linear;
> }
>
> This issue should be addressed in a follow-up enhancement.
Michael Strauß has updated the pull request incrementally with two additional
commits since the last revision:
- update 'since' tags
- Fix javadoc error
-
Changes:
- all: https://git.openjdk.org/jfx/pull/870/files
- new: https://git.openjdk.org/jfx/pull/870/files/f7ab6db3..137a00c7
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jfx=870=14
- incr: https://webrevs.openjdk.org/?repo=jfx=870=13-14
Stats: 14 lines in 3 files changed: 8 ins; 0 del; 6 mod
Patch: https://git.openjdk.org/jfx/pull/870.diff
Fetch: git fetch https://git.openjdk.org/jfx.git pull/870/head:pull/870
PR: https://git.openjdk.org/jfx/pull/870
Re: RFR: 8311895: CSS Transitions [v14]
> Implementation of [CSS
> Transitions](https://gist.github.com/mstr2/c72f8c9faa87de14926978f517a6018a).
>
> ### Example
>
> .button {
> -fx-background-color: dodgerblue;
> }
>
> .button:hover {
> -fx-background-color: red;
> -fx-scale-x: 1.1;
> -fx-scale-y: 1.1;
>
> transition: -fx-background-color 0.5s ease,
> -fx-scale-x 0.5s ease,
> -fx-scale-y 0.5s ease;
> }
>
> src="https://user-images.githubusercontent.com/43553916/184781143-0520fbfe-54bf-4b8d-93ac-834708e46500.gif;
> width="200"/>
>
> ### Limitations
> This implementation supports both shorthand and longhand notations for the
> `transition` property. However, due to limitations of JavaFX CSS, mixing both
> notations doesn't work:
>
> .button {
> transition: -fx-background-color 1s;
> transition-easing-function: linear;
> }
>
> This issue should be addressed in a follow-up enhancement.
Michael Strauß has updated the pull request incrementally with one additional
commit since the last revision:
Change javadoc comment
-
Changes:
- all: https://git.openjdk.org/jfx/pull/870/files
- new: https://git.openjdk.org/jfx/pull/870/files/7edb374f..f7ab6db3
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jfx=870=13
- incr: https://webrevs.openjdk.org/?repo=jfx=870=12-13
Stats: 4 lines in 1 file changed: 0 ins; 3 del; 1 mod
Patch: https://git.openjdk.org/jfx/pull/870.diff
Fetch: git fetch https://git.openjdk.org/jfx.git pull/870/head:pull/870
PR: https://git.openjdk.org/jfx/pull/870
Re: RFR: 8311895: CSS Transitions [v13]
> Implementation of [CSS
> Transitions](https://gist.github.com/mstr2/c72f8c9faa87de14926978f517a6018a).
>
> ### Example
>
> .button {
> -fx-background-color: dodgerblue;
> }
>
> .button:hover {
> -fx-background-color: red;
> -fx-scale-x: 1.1;
> -fx-scale-y: 1.1;
>
> transition: -fx-background-color 0.5s ease,
> -fx-scale-x 0.5s ease,
> -fx-scale-y 0.5s ease;
> }
>
> src="https://user-images.githubusercontent.com/43553916/184781143-0520fbfe-54bf-4b8d-93ac-834708e46500.gif;
> width="200"/>
>
> ### Limitations
> This implementation supports both shorthand and longhand notations for the
> `transition` property. However, due to limitations of JavaFX CSS, mixing both
> notations doesn't work:
>
> .button {
> transition: -fx-background-color 1s;
> transition-easing-function: linear;
> }
>
> This issue should be addressed in a follow-up enhancement.
Michael Strauß has updated the pull request with a new target base due to a
merge or a rebase. The pull request now contains 49 commits:
- Merge branch 'master' into feature/css-transitions
- Discard redundant transitions in StyleableProperty impls
- Changes per review
- Merge branch 'master' into feature/css-transitions
- Merge branch 'master' into feature/css-transitions
- Add TransitionMediator
- Test whether two Interpolatable instances are compatible
- Merge branch 'master' into feature/css-transitions
- Added documentation
- Review changes
- ... and 39 more: https://git.openjdk.org/jfx/compare/eca32354...7edb374f
-
Changes: https://git.openjdk.org/jfx/pull/870/files
Webrev: https://webrevs.openjdk.org/?repo=jfx=870=12
Stats: 4661 lines in 43 files changed: 4622 ins; 1 del; 38 mod
Patch: https://git.openjdk.org/jfx/pull/870.diff
Fetch: git fetch https://git.openjdk.org/jfx.git pull/870/head:pull/870
PR: https://git.openjdk.org/jfx/pull/870
Re: RFR: 8311895: CSS Transitions [v9]
On Tue, 14 Nov 2023 13:04:35 GMT, John Hendrikx wrote:
>> Michael Strauß has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Test whether two Interpolatable instances are compatible
>
> modules/javafx.graphics/src/main/java/javafx/css/StyleableDoubleProperty.java
> line 78:
>
>> 76:
>> 77: if (transition != null) {
>> 78: timer = TransitionTimer.run(new TransitionTimerImpl(this,
>> v), transition);
>
> Would it be possible to check here if this timer is going to the same target?
> Also see other comment about a simplification.
>
> if (timer == null || timer.newValue.equals(v))) {
> timer = TransitionTimer.run(new TransitionTimerImpl(this, v),
> transition);
> }
I've moved the logic to discard redundant transitions to the
`StyleableProperty` implementation classes.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1545575965
Re: RFR: 8311895: CSS Transitions [v12]
> Implementation of [CSS
> Transitions](https://gist.github.com/mstr2/c72f8c9faa87de14926978f517a6018a).
>
> ### Example
>
> .button {
> -fx-background-color: dodgerblue;
> }
>
> .button:hover {
> -fx-background-color: red;
> -fx-scale-x: 1.1;
> -fx-scale-y: 1.1;
>
> transition: -fx-background-color 0.5s ease,
> -fx-scale-x 0.5s ease,
> -fx-scale-y 0.5s ease;
> }
>
> src="https://user-images.githubusercontent.com/43553916/184781143-0520fbfe-54bf-4b8d-93ac-834708e46500.gif;
> width="200"/>
>
> ### Limitations
> This implementation supports both shorthand and longhand notations for the
> `transition` property. However, due to limitations of JavaFX CSS, mixing both
> notations doesn't work:
>
> .button {
> transition: -fx-background-color 1s;
> transition-easing-function: linear;
> }
>
> This issue should be addressed in a follow-up enhancement.
Michael Strauß has updated the pull request incrementally with one additional
commit since the last revision:
Discard redundant transitions in StyleableProperty impls
-
Changes:
- all: https://git.openjdk.org/jfx/pull/870/files
- new: https://git.openjdk.org/jfx/pull/870/files/22aa4d64..1595a190
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jfx=870=11
- incr: https://webrevs.openjdk.org/?repo=jfx=870=10-11
Stats: 432 lines in 10 files changed: 320 ins; 93 del; 19 mod
Patch: https://git.openjdk.org/jfx/pull/870.diff
Fetch: git fetch https://git.openjdk.org/jfx.git pull/870/head:pull/870
PR: https://git.openjdk.org/jfx/pull/870
Integrated: 8325900: Emit a warning on macOS if AWT has set the NSAppearance
On Wed, 14 Feb 2024 21:09:39 GMT, Michael Strauß wrote: > Platform preferences detection doesn't pick up effective macOS system > preferences if AWT owns the NSApplication and has set its NSAppearance to a > fixed value. > > The workaround is to set the system property > "apple.awt.application.appearance=system". > > If this property is not set, the following warning will be emitted if a > JavaFX application attempts to use the platform preferences API: > > > WARNING: Reported preferences may not reflect the macOS system preferences > unless the sytem > property apple.awt.application.appearance=system is set. This warning can be > disabled by > setting javafx.preferences.suppressAppleAwtWarning=true. This pull request has now been integrated. Changeset: eca32354 Author:Michael Strauß URL: https://git.openjdk.org/jfx/commit/eca323547ec0e84b40bebb213350b6cea5385904 Stats: 71 lines in 5 files changed: 65 ins; 0 del; 6 mod 8325900: Emit a warning on macOS if AWT has set the NSAppearance Reviewed-by: kcr, mfox - PR: https://git.openjdk.org/jfx/pull/1367
Re: RFR: 8267565: Support "@3x" and greater high-density image naming convention [v2]
On Tue, 26 Mar 2024 21:27:49 GMT, drmarmac wrote: >> This PR extends the range of hi-res images that are loaded via naming >> convention, now including scale factors higher than `@2x`. >> Supporting these is already being >> [recommended](https://developer.apple.com/design/human-interface-guidelines/images#Best-practices) >> for some platforms. >> >> I tested this manually on Windows with "300%" UI scale factor, and verified >> `@2x` still works on macOS. > > drmarmac has updated the pull request incrementally with one additional > commit since the last revision: > > Change name2x to nameScaled Looks good to me. - Marked as reviewed by mstrauss (Committer). PR Review: https://git.openjdk.org/jfx/pull/1429#pullrequestreview-1962957040
Re: RFR: JDK-8290310: ChangeListener events are incorrect or misleading when a nested change occurs [v11]
On Fri, 9 Jun 2023 12:00:06 GMT, John Hendrikx wrote: >> This provides and uses a new implementation of `ExpressionHelper`, called >> `ListenerManager` with improved semantics. >> >> # Behavior >> >> |Listener...|ExpressionHelper|ListenerManager| >> |---|---|---| >> |Invocation Order|In order they were registered, invalidation listeners >> always before change listeners|(unchanged)| >> |Removal during Notification|All listeners present when notification started >> are notified, but excluded for any nested changes|Listeners are removed >> immediately regardless of nesting| >> |Addition during Notification|Only listeners present when notification >> started are notified, but included for any nested changes|New listeners are >> never called during the current notification regardless of nesting| >> >> ## Nested notifications: >> >> | |ExpressionHelper|ListenerManager| >> |---|---|---| >> |Type|Depth first (call stack increases for each nested level)|(same)| >> |# of Calls|Listeners * Depth (using incorrect old values)|Collapses nested >> changes, skipping non-changes| >> |Vetoing Possible?|No|Yes| >> |Old Value correctness|Only for listeners called before listeners making >> nested changes|Always| >> >> # Performance >> >> |Listener|ExpressionHelper|ListenerManager| >> |---|---|---| >> |Addition|Array based, append in empty slot, resize as needed|(same)| >> |Removal|Array based, shift array, resize as needed|(same)| >> |Addition during notification|Array is copied, removing collected >> WeakListeners in the process|Appended when notification finishes| >> |Removal during notification|As above|Entry is `null`ed (to avoid moving >> elements in array that is being iterated)| >> |Notification completion with changes|-|Null entries (and collected >> WeakListeners) are removed| >> |Notifying Invalidation Listeners|1 ns each|(same)| >> |Notifying Change Listeners|1 ns each (*)|2-3 ns each| >> >> (*) a simple for loop is close to optimal, but unfortunately does not >> provide correct old values >> >> # Memory Use >> >> Does not include alignment, and assumes a 32-bit VM or one that is using >> compressed oops. >> >> |Listener|ExpressionHelper|ListenerManager|OldValueCaching ListenerManager| >> |---|---|---|---| >> |No Listeners|none|none|none| >> |Single InvalidationListener|16 bytes overhead|none|none| >> |Single ChangeListener|20 bytes overhead|none|16 bytes overhead| >> |Multiple listeners|57 + 4 per listener (excluding unused slots)|57 + 4 per >> listener (excluding unused slots)|61 + 4 per listener (excluding unused >> slots)| >> >> # About nested changes >> >> Nested changes are simply changes... > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Fix generic warnings `ListenerManager` is an obvious improvement, as it fixes incorrect behavior and allows listeners to veto changes. However, the behavior of `ListenerManager` is also an implementation detail and not documented anywhere. This leads me to the following questions: 1. How will users know that they can now do all of the things that were previously broken? Do we need a specification for what is allowed and what's not allowed? 2. Should this behavior be required for all valid `ObservableValue` implementations? (This would render many existing implementations defective.) 3. If `ObservableValue` implementations are not required to replicate the `ListenerManager` behavior, we should probably make it easily discoverable whether any particular implementation (most of them are properties) supports nested changes/vetoing. In most of the public API, there's no obvious way to see (without looking at the source code) whether a property implementation extends one of the `*PropertyBase` classes. - PR Comment: https://git.openjdk.org/jfx/pull/1081#issuecomment-2018017130
Re: RFR: 8311895: CSS Transitions [v2]
On Mon, 31 Jul 2023 13:44:28 GMT, John Hendrikx wrote: >> Michael Strauß has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Make TransitionEvent final > > Some early feedback, it's a lot of code :) I've implemented @hjohn's suggestion of an interface between `TransitionTimer` and `StyleableProperty` with the new `TransitionMediator` class. This cleans up the code and separates the interacting components. This PR is now again ready for review. - PR Comment: https://git.openjdk.org/jfx/pull/870#issuecomment-2004638297
Re: RFR: 8311895: CSS Transitions [v11]
> Implementation of [CSS
> Transitions](https://gist.github.com/mstr2/c72f8c9faa87de14926978f517a6018a).
>
> ### Example
>
> .button {
> -fx-background-color: dodgerblue;
> }
>
> .button:hover {
> -fx-background-color: red;
> -fx-scale-x: 1.1;
> -fx-scale-y: 1.1;
>
> transition: -fx-background-color 0.5s ease,
> -fx-scale-x 0.5s ease,
> -fx-scale-y 0.5s ease;
> }
>
> src="https://user-images.githubusercontent.com/43553916/184781143-0520fbfe-54bf-4b8d-93ac-834708e46500.gif;
> width="200"/>
>
> ### Limitations
> This implementation supports both shorthand and longhand notations for the
> `transition` property. However, due to limitations of JavaFX CSS, mixing both
> notations doesn't work:
>
> .button {
> transition: -fx-background-color 1s;
> transition-easing-function: linear;
> }
>
> This issue should be addressed in a follow-up enhancement.
Michael Strauß has updated the pull request incrementally with one additional
commit since the last revision:
Changes per review
-
Changes:
- all: https://git.openjdk.org/jfx/pull/870/files
- new: https://git.openjdk.org/jfx/pull/870/files/b76568c3..22aa4d64
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jfx=870=10
- incr: https://webrevs.openjdk.org/?repo=jfx=870=09-10
Stats: 17 lines in 1 file changed: 4 ins; 4 del; 9 mod
Patch: https://git.openjdk.org/jfx/pull/870.diff
Fetch: git fetch https://git.openjdk.org/jfx.git pull/870/head:pull/870
PR: https://git.openjdk.org/jfx/pull/870
Re: RFR: 8311895: CSS Transitions [v9]
On Tue, 14 Nov 2023 09:41:08 GMT, John Hendrikx wrote:
>> Michael Strauß has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Test whether two Interpolatable instances are compatible
>
> modules/javafx.graphics/src/main/java/com/sun/javafx/css/TransitionTimer.java
> line 179:
>
>> 177: if (newTimer.delay < 0) {
>> 178: double adjustedDelay = nanosToMillis(newTimer.delay) *
>> newTimer.reversingShorteningFactor;
>> 179: newTimer.startTime = now + millisToNanos(adjustedDelay);
>
> I don't see the value of converting these back and forth to milliseconds.
> Why not just:
>
> Suggestion:
>
> newTimer.startTime = now + newTimer.delay *
> newTimer.reversingShorteningFactor;
>
> I checked the calculations, and it makes no difference (the millis aren't
> rounded or anything), so you're just moving the decimal point before/after
> doing a multiplication that doesn't care where the decimal point is.
>
> If there is a reason that I missed, then I think this really needs a comment.
I've removed the conversions.
> modules/javafx.graphics/src/main/java/com/sun/javafx/css/TransitionTimer.java
> line 196:
>
>> 194: double frac = (double)(nanos - (millis * 1_000_000L)) /
>> 1_000_000D;
>> 195: return (double)millis + frac;
>> 196: }
>
> This function seems to want to preserve extra decimals in some way. If the
> original long has a magnitude that is so large that some least significant
> digits get lost in the double conversion, then trying to add them later
> (`millis + frac`) will not restore them.
>
> In other words, you can just do:
>
> Suggestion:
>
> private static double nanosToMillis(long nanos) {
> return nanos / 1_000_000.0;
> }
>
>
> Or avoiding the (generally slower) division completely:
>
> Suggestion:
>
> private static double nanosToMillis(long nanos) {
> return nanos * 0.000_001;
> }
>
>
> All the extra operations are only likely to introduce small errors.
Changed as suggested.
> modules/javafx.graphics/src/main/java/com/sun/javafx/css/TransitionTimer.java
> line 207:
>
>> 205: long wholeMillis = (long)millis;
>> 206: double frac = millis - (double)wholeMillis;
>> 207: return wholeMillis * 1_000_000L + (long)(frac * 1_000_000D);
>
> I'd recommend keeping this simple (it seems to want to recover extra
> significant digits, but that's not possible):
>
> Suggestion:
>
> return ((long)(millis * 1_000_000.0);
Changed as suggested.
> modules/javafx.graphics/src/main/java/com/sun/javafx/css/TransitionTimer.java
> line 234:
>
>> 232:
>> 233: Node node = (Node)timer.getProperty().getBean();
>> 234: node.fireEvent(new TransitionEvent(eventType,
>> timer.getProperty(), elapsedTime));
>
> minor:
> Suggestion:
>
> long elapsedTime;
>
> // Elapsed time specification:
> https://www.w3.org/TR/css-transitions-1/#event-transitionevent
> if (eventType == TransitionEvent.RUN || eventType ==
> TransitionEvent.START) {
> elapsedTime = Math.min(Math.max(-timer.delay, 0),
> timer.duration);
> } else if (eventType == TransitionEvent.CANCEL) {
> elapsedTime = Math.max(0, timer.currentTime -
> timer.startTime);
> } else if (eventType == TransitionEvent.END) {
> elapsedTime = timer.duration;
> } else {
> throw new IllegalArgumentException("eventType");
> }
>
> Node node = (Node)timer.getProperty().getBean();
> node.fireEvent(new TransitionEvent(eventType,
> timer.getProperty(), Duration.millis(nanosToMillis(elapsedTime;
Changed as suggested.
> modules/javafx.graphics/src/main/java/javafx/css/StyleableDoubleProperty.java
> line 111:
>
>> 109: private TransitionTimer timer = null;
>> 110:
>> 111: private static class TransitionTimerImpl extends
>> TransitionTimer {
>
> I think you can simplify this. TransitionTimer does not need any of its
> generic parameters if you provide an abstract method to get the property
> (just the general interface), which is sufficient for the `getBean` call you
> do on it, and for passing it along as the source of events.
>
> The constructor, `onUpdate` and `onStop` don't need the property parameter at
> all if you make the `TransistionTimerImpl` non-static.
>
> I also have the feeling that instead of subclassing `
Re: RFR: 8311895: CSS Transitions [v10]
> Implementation of [CSS
> Transitions](https://gist.github.com/mstr2/c72f8c9faa87de14926978f517a6018a).
>
> ### Example
>
> .button {
> -fx-background-color: dodgerblue;
> }
>
> .button:hover {
> -fx-background-color: red;
> -fx-scale-x: 1.1;
> -fx-scale-y: 1.1;
>
> transition: -fx-background-color 0.5s ease,
> -fx-scale-x 0.5s ease,
> -fx-scale-y 0.5s ease;
> }
>
> src="https://user-images.githubusercontent.com/43553916/184781143-0520fbfe-54bf-4b8d-93ac-834708e46500.gif;
> width="200"/>
>
> ### Limitations
> This implementation supports both shorthand and longhand notations for the
> `transition` property. However, due to limitations of JavaFX CSS, mixing both
> notations doesn't work:
>
> .button {
> transition: -fx-background-color 1s;
> transition-easing-function: linear;
> }
>
> This issue should be addressed in a follow-up enhancement.
Michael Strauß has updated the pull request with a new target base due to a
merge or a rebase. The pull request now contains 46 commits:
- Merge branch 'master' into feature/css-transitions
- Merge branch 'master' into feature/css-transitions
- Add TransitionMediator
- Test whether two Interpolatable instances are compatible
- Merge branch 'master' into feature/css-transitions
- Added documentation
- Review changes
- Review changes
- Make interpolator fields final
- Review changes
- ... and 36 more: https://git.openjdk.org/jfx/compare/ad3d44e2...b76568c3
-
Changes: https://git.openjdk.org/jfx/pull/870/files
Webrev: https://webrevs.openjdk.org/?repo=jfx=870=09
Stats: 4434 lines in 42 files changed: 4395 ins; 1 del; 38 mod
Patch: https://git.openjdk.org/jfx/pull/870.diff
Fetch: git fetch https://git.openjdk.org/jfx.git pull/870/head:pull/870
PR: https://git.openjdk.org/jfx/pull/870
Re: Validation Support
I would not be in favor of adding any particular data validation framework to JavaFX. Data validation comes in all kinds of different shapes and sizes, which makes it a good fit for (opinionated) third-party libraries. However, I fully agree with John that JavaFX should provide more APIs that can only realistically be implemented in FX. I've proposed a "significant interaction" API, which is crucial for many data validation scenarios: https://mail.openjdk.org/pipermail/openjfx-dev/2023-March/039327.html On Fri, Mar 1, 2024 at 12:50 PM Dirk Lemmermann wrote: > > Hi everyone, > > I updated the validation framework ValidatorFX today in our project to the > latest release and I really like it a lot. It is a small compact API and > works with any observable as opposed to the validation support provided by > ControlsFX. > > Using it made me wonder whether it would make sense to bundle it or something > like it directly with JavaFX. Developers often mention missing validation > support as a drawback of using JavaFX. Adding this would take one item off > from the list of arguments against using JavaFX. > > Many UI frameworks do have built-in validation support, e.g. Vaadin [0], > Angular, [1], or QT [2] > > What do you think? > > —Dirk > > [0] https://vaadin.com/docs/latest/binding-data/components-binder-validation > [1] https://angular.io/guide/form-validation > [2] https://doc.qt.io/qt-6/qtquick-input-textinput.html >
Re: RFR: 8325900: Emit a warning on macOS if AWT has set the NSAppearance [v3]
On Mon, 19 Feb 2024 17:13:58 GMT, Anirvan Sarkar wrote: > What about SWT ? It looks like SWT also supports dark mode since version 4.12 > [1][2]. > > [1] : https://eclipse.dev/eclipse/news/4.12/platform_isv.php#dark-theme-mac > [2] : https://bugs.eclipse.org/bugs/show_bug.cgi?id=540357 I haven’t tested this in combination with SWT, have you? - PR Comment: https://git.openjdk.org/jfx/pull/1367#issuecomment-1962068535
Re: RFR: 8325900: Emit a warning on macOS if AWT has set the NSAppearance [v3]
On Fri, 23 Feb 2024 20:14:43 GMT, Kevin Rushforth wrote:
>> Michael Strauß has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> address review comments
>
> modules/javafx.graphics/src/main/java/com/sun/glass/ui/mac/MacApplication.java
> line 483:
>
>> 481:
>> 482: if (!AWT_SYSTEM_APPEARANCE.equals(awtAppearanceProperty)) {
>> 483: Logging.getJavaFXLogger().warning(String.format(
>
> Minor: Maybe add newlines to break this message into two or three separate
> lines?
Done. I've also moved the `checkSystemAppearance` assignment out of the if
branch (it should be unconditionally set to `false` after the first call).
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1367#discussion_r1501215285
Re: RFR: 8325900: Emit a warning on macOS if AWT has set the NSAppearance [v4]
> Platform preferences detection doesn't pick up effective macOS system > preferences if AWT owns the NSApplication and has set its NSAppearance to a > fixed value. > > The workaround is to set the system property > "apple.awt.application.appearance=system". > > If this property is not set, the following warning will be emitted if a > JavaFX application attempts to use the platform preferences API: > > > WARNING: Reported preferences may not reflect the macOS system preferences > unless the sytem > property apple.awt.application.appearance=system is set. This warning can be > disabled by > setting javafx.preferences.suppressAppleAwtWarning=true. Michael Strauß has updated the pull request incrementally with one additional commit since the last revision: Break warning message into separate lines - Changes: - all: https://git.openjdk.org/jfx/pull/1367/files - new: https://git.openjdk.org/jfx/pull/1367/files/5c743df7..8fdbc624 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1367=03 - incr: https://webrevs.openjdk.org/?repo=jfx=1367=02-03 Stats: 7 lines in 1 file changed: 3 ins; 2 del; 2 mod Patch: https://git.openjdk.org/jfx/pull/1367.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1367/head:pull/1367 PR: https://git.openjdk.org/jfx/pull/1367
Re: RFR: 8089373: Translation from character to key code is not sufficient [v2]
On Fri, 9 Feb 2024 03:37:17 GMT, Martin Fox wrote: >> On Windows a common shortcut like Ctrl+'+' could only be invoked from the >> main keyboard and not the numeric keypad. Toolkit.getKeyCodeForChar did not >> have enough context to know whether it should return a result from the main >> keyboard or the keypad. >> >> This PR alters getKeyCodeForChar to pass in the code of the key the system >> is trying to match against. Only the Windows platform actually uses this >> additional information. >> >> On the Mac the numeric keypad has always worked due to the odd way >> getKeyCodeForChar is implemented (until PR #1209 the keypad worked more >> reliably than the main keyboard). On Linux getKeyCodeForChar is a mess; >> neither the main keyboard or the numeric keypad work reliably. I have an >> upcoming PR which should make both work correctly. > > Martin Fox 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 seven additional commits since > the last revision: > > - Consistent terminology and more details in comments. > - Merge remote-tracking branch 'upstream/master' into keypadcombo > - Added SEPARATOR to list of keypad keys > - CharacterCombinations now work on the numeric keypad > - Fixed Monocle > - Merge remote-tracking branch 'upstream/master' into keypadcombo > - Added hint to getKeyCodeForChar to enable numeric keypad Looks good to me. - Marked as reviewed by mstrauss (Committer). PR Review: https://git.openjdk.org/jfx/pull/1289#pullrequestreview-1888180397
Re: RFR: 8325900: Emit a warning on macOS if AWT has set the NSAppearance [v3]
> Platform preferences detection doesn't pick up effective macOS system > preferences if AWT owns the NSApplication and has set its NSAppearance to a > fixed value. > > The workaround is to set the system property > "apple.awt.application.appearance=system". > > If this property is not set, the following warning will be emitted if a > JavaFX application attempts to use the platform preferences API: > > > WARNING: Reported preferences may not reflect the macOS system preferences > unless the sytem > property apple.awt.application.appearance=system is set. This warning can be > disabled by > setting javafx.preferences.suppressAppleAwtWarning=true. 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/1367/files - new: https://git.openjdk.org/jfx/pull/1367/files/688b2f80..5c743df7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1367=02 - incr: https://webrevs.openjdk.org/?repo=jfx=1367=01-02 Stats: 15 lines in 2 files changed: 10 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jfx/pull/1367.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1367/head:pull/1367 PR: https://git.openjdk.org/jfx/pull/1367
Re: RFR: 8325900: Emit a warning on macOS if AWT has set the NSAppearance [v2]
> Platform preferences detection doesn't pick up effective macOS system > preferences if AWT owns the NSApplication and has set its NSAppearance to a > fixed value. > > The workaround is to set the system property > "apple.awt.application.appearance=system". > > If this property is not set, the following warning will be emitted if a > JavaFX application attempts to use the platform preferences API: > > > WARNING: Reported preferences may not reflect the macOS system preferences > unless the sytem > property apple.awt.application.appearance=system is set. This warning can be > disabled by > setting javafx.preferences.suppressAppleAwtWarning=true. Michael Strauß has updated the pull request incrementally with one additional commit since the last revision: Don't call into the native toolkit from Platform.getPreferences() - Changes: - all: https://git.openjdk.org/jfx/pull/1367/files - new: https://git.openjdk.org/jfx/pull/1367/files/27ae6a7d..688b2f80 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1367=01 - incr: https://webrevs.openjdk.org/?repo=jfx=1367=00-01 Stats: 25 lines in 3 files changed: 14 ins; 8 del; 3 mod Patch: https://git.openjdk.org/jfx/pull/1367.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1367/head:pull/1367 PR: https://git.openjdk.org/jfx/pull/1367
RFR: 8325900: Emit a warning on macOS if AWT has set the NSAppearance
Platform preferences detection doesn't pick up effective macOS system preferences if AWT owns the NSApplication and has set its NSAppearance to a fixed value. The workaround is to set the system property "apple.awt.application.appearance=system". If this property is not set, the following warning will be emitted if a JavaFX application attempts to use the platform preferences API: WARNING: Reported preferences may not reflect the macOS system preferences unless the sytem property apple.awt.application.appearance=system is set. This warning can be disabled by setting javafx.preferences.suppressAppleAwtWarning=true. - Commit messages: - Emit a warning on macOS if AWT has set the NSAppearance Changes: https://git.openjdk.org/jfx/pull/1367/files Webrev: https://webrevs.openjdk.org/?repo=jfx=1367=00 Issue: https://bugs.openjdk.org/browse/JDK-8325900 Stats: 53 lines in 4 files changed: 48 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jfx/pull/1367.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1367/head:pull/1367 PR: https://git.openjdk.org/jfx/pull/1367
Re: RFR: JDK-8325798: Spinner throws uncatchable exception on tab out from garbled text
On Tue, 13 Feb 2024 20:42:08 GMT, Marius Hanl wrote:
> When a `Spinner` is configured with e.g. Integers
> (`IntegerSpinnerValueFactory`) and the user types in a `String`, e.g. 'abc'
> and focuses another `Node`, an exception is thrown (`NumberFormatException`).
> This exception is literally uncatchable, as it happens on focus lost.
>
> The issue is the same as for the `DatePicker` component described in
> [JDK-8303478](https://bugs.openjdk.org/browse/JDK-8303478), which was fixed
> in PR: https://github.com/openjdk/jfx/pull/1274.
>
> I did the exact same fix in this PR. Also added the same test.
LGTM
modules/javafx.controls/src/test/java/test/javafx/scene/control/SpinnerTest.java
line 1588:
> 1586:
> 1587: /**
> 1588: * When Spinner looses focus with misformatted text in the editor,
typo: loses
modules/javafx.controls/src/test/java/test/javafx/scene/control/SpinnerTest.java
line 1609:
> 1607: spinner.getEditor().setText("2abc");
> 1608:
> 1609: // loosing focus triggers cancelEdit() because the text cannot
> be parsed
typo: losing
-
Marked as reviewed by mstrauss (Committer).
PR Review: https://git.openjdk.org/jfx/pull/1365#pullrequestreview-1879077791
PR Review Comment: https://git.openjdk.org/jfx/pull/1365#discussion_r1488678419
PR Review Comment: https://git.openjdk.org/jfx/pull/1365#discussion_r1488678809
Re: [External] : Re: Platform preferences theme detection
The reason why the values are wrong might be that in PlatformSupport.m L105, we query [NSApp effectiveApperance], but this always seems to return the light appearance if JavaFX doesn't own the NSApplication. On Mon, Feb 12, 2024 at 6:26 PM Kevin Rushforth wrote: > > Actually, it's worse than not detecting changes, it's simply not getting the > right values at all. If I run the program when the system appearance is > already in Dark mode, it doesn't get the correct values at start up. > > -- Kevin
Re: Platform preferences theme detection
Since I can't reproduce the error on macOS, it's hard to say what could be causing it. If you have the chance, maybe you could run the PlatformPreferencesChangedTest application: https://github.com/openjdk/jfx/blob/master/tests/manual/events/PlatformPreferencesChangedTest.java That might rule out that the differences we're seeing have anything to do with the way we're using the API. On Sat, Feb 10, 2024 at 6:23 PM Christopher Schnick wrote: > > I checked again, restarted everything, but still the same problem: > > Furthermore, I also tested it on my Asahi Fedora Linux with the latest KDE > Plasma. The preferences values there are all detected correctly, but are > never updated at runtime when changed in the settings menu.
Re: Platform preferences theme detection
Hi Christopher, the relevant preferences for color scheme detection are textColor=0x00ff and textBackgroundColor=0x on your system. That's pretty strange, I'm getting the following values on macOS Sonoma 14.3.1 M1: in light mode: textColor=0x00ff, textBackgroundColor=0x in dark mode: textColor=0x, textBackgroundColor=0x1e1e1eff Consequently, I'm also getting the correct color scheme when I'm switching between light and dark mode on macOS (verified with the PlatformPreferencesChangedTest application). On Sat, Feb 10, 2024 at 5:55 AM Christopher Schnick wrote: > Lastly I also tried it on macOS. The accent color detection works there, > but the color scheme detection does not, i.e. it always returns light mode. > I'm using macOS Sonoma 14.3 with aarch64. > > Here are the preferences when dark mode is set in the settings: > > > > So since you are thinking about making this a preview-only feature, I > don't think there is that much to do to properly handle systems where some > properties are not supported. Just a few methods to set the default value > of foreground, background, and accent color would be enough. In theory I > don't need to know exactly what color observation is supported, I only want > to have the ability to use my own colors as defaults in that case. > > Best > Christopher Schnick > >
Preview features proposal
I've created a proposal that defines preview features [0]. The definition very closely matches the language used in JEP-12, but is adapted for JavaFX in some places. Additionally, I've created the internal helper class `PreviewFeature` that can be used to verify that applications have opted into using preview features at runtime. If the system property "javafx.enablePreview" system property has not been set, the `PreviewFeature.checkEnabled()` method will throw an exception similar to the following: is a preview feature of JavaFX 23. Preview features may be removed in a future release, or upgraded to permanent features of JavaFX. Programs can only use preview features when the following system property is set: -Djavafx.enablePreview=true If the system property has been set, the method will log a one-time warning to the error output stream, similar to the following: Note: This program uses the following preview feature of JavaFX 23: Preview features may be removed in a future release, or upgraded to permanent features of JavaFX. This warning can be disabled with the following system property: -Djavafx.suppressPreviewBanner=true I think that we should use the @Depcreated annotation to document API elements of preview features, with a @deprecated javadoc tag similar to the following: @deprecated This is a preview feature which may be changed or removed in a future release. The specification of @Deprecated specifically mentions this usage: "An element may be deprecated for any of several reasons, for example, its usage is likely to lead to errors; it may be changed incompatibly or removed in a future version; it has been superseded by a newer, usually preferable alternative; or it is obsolete." Finally, all preview features of a JavaFX release should be listed in the release notes. [0] https://github.com/openjdk/jfx/pull/1359
Re: Preview features for JavaFX
Hi Kevin, my suggestion would be to annotate and document the preview API (at least annotations do show up by default in most IDEs), and emit a one-time runtime warning when the API is used (this works for methods and constructors). This would make it quite visible to developers that they are using a preview feature, or that a third-party library uses a preview feature. The runtime warning can be suppressed with a command line parameter such as "javafx.enablePreviewFeatures". A more drastic approach would be to throw an exception from new APIs when the parameter is not specified. Given that there are very tangible benefits to previewing new API, this would seem to me like a good enough solution. On Wed, Feb 7, 2024 at 12:59 AM Kevin Rushforth wrote: > > In order for preview features and incubating features to not cause more > problems than they solve, there needs to be a robust way to ensure that > applications and libraries don't use them without knowing that they are > doing so. We know how to do that for a feature that lives in its own > module (an incubating feature), but not how to do that for something > like a preview feature. > > For incubating features, this is relatively straight-forward, since they > are delivered in a separate module that has "incubator" in the name, > isn't resolved by default, and warns you at runtime when those modules > are resolved. Adapting what the JDK does for JavaFX should be pretty > easy, and retain the benefit that an app knows when they are using > incubating features. > > I don't think it is feasible to do the same thing for preview features. > The way the JDK preview features work is that a command line option is > needed both at compile time and at runtime to opt into preview features > for a specific release. This prevents using a preview API from an > existing module and package without knowing that it is subject to > change. Without a clear "opt in" mechanism to be able to use an API, an > app would be able to accidentally use a feature whose API is unstable > and quite possible might change. An annotation isn't good enough (and > documentation certainly isn't sufficient). IDEs will still autocomplete > and show the API, and once an app uses it -- accidentally or otherwise > -- there is no indication at runtime that you are using a feature that > will likely stop working without any notice in the next version. > > I don't see a good way to do this for JavaFX given the limitations. > > -- Kevin
Preview features for JavaFX
The discussion around the new Platform Preferences API has brought up a potential area where the API may lack a way to detect whether a particular preference is supported on a particular operating system [0]. Discussions like these will invariably come up when new API is released, and some of the real-world insights may prove to be very valuable. However, with the current development process, we specify and implement new features largely without feedback from application developers. I know that, in principle, developers can join in on the discussion on this mailing list. But the reality is that GA is the first time that a new feature gets wider exposure. All of this makes it very hard for us to ship new features, since we must be extremely careful to get it right the first time. The JDK uses incubator modules and preview features to address these challenges. It seems that OpenJFX will also potentially use an incubator module to introduce new controls [1]. This is great for modular features, but not so great for new API that is added to existing infrastructure. Maybe we could add something akin to preview features to OpenJFX: this could be as easy as documenting the new API to be in preview, or decorate the new API with a @PreviewFeature annotation. I don't think that it is necessary to go beyond simple documentation; in particular, we don't need this to be integrated with the Java compiler. Documenting new features to be preview features will enable us to ship features quicker, and ensure that what we're building is actually useful in the real world because we can actually go back and improve aspects of a feature without worrying as much about backwards compatibility. In particular, my suggestion is to ship the new Platform Preferences API as a preview feature for jfx22. [0] https://mail.openjdk.org/pipermail/openjfx-dev/2024-February/045176.html [1] https://bugs.openjdk.org/browse/JDK-8309381
Re: Platform preferences theme detection
Hi Christopher! 1) Accent color detection is not yet implemented for Linux/GTK. I think this is a fairly recent addition for Ubuntu, probably in version 22. 2) I don't see this behavior on my Ubuntu 20.04 system (need to upgrade to 23 and test again). If you have acess to JBS, feel free to open a bug ticket to track this issue. On Mon, Feb 5, 2024 at 10:56 PM Christopher Schnick wrote: > I finally found some time to test the latest ea build and can confirm that > it works now on Windows as expected. > > Next, I moved to Linux, more specifically Ubuntu 23.04, and encountered > some issues: > - The accent color detection does not work, i.e. it does not initially > detect the value set in the settings and also does not change when I modify > the accent color in the Ubuntu Settings. I can see some values changing in > the map, however the end result is still the same > - When opening the Ubuntu Settings application, the entire platform > preferences map is updated with weird values but somehow changes back > immediately after. Maybe it reports the default light mode colors for a > split second when opening the settings? > > Once again I do not know whether this is intended or not as there is no > definitive list of which operating systems are supported. Here is the > obligatory preferences map on that system: > > >
[jfx22] Integrated: 8324879: Platform-specific preferences keys are incorrect for Windows toolkit
On Tue, 30 Jan 2024 19:21:50 GMT, Michael Strauß wrote: > This pull request contains a backport of commit > [af7e0571](https://github.com/openjdk/jfx/commit/af7e05716711d942df20eb1f807b384810a4a839) > from the [openjdk/jfx](https://git.openjdk.org/jfx) repository. > > The commit being backported was authored by Michael Strauß on 30 Jan 2024 and > was reviewed by Kevin Rushforth and Andy Goryachev. This pull request has now been integrated. Changeset: 34914c46 Author:Michael Strauß URL: https://git.openjdk.org/jfx/commit/34914c465ac71bd2ab3ba2a5ccae5f353bc59264 Stats: 40 lines in 3 files changed: 33 ins; 0 del; 7 mod 8324879: Platform-specific preferences keys are incorrect for Windows toolkit Reviewed-by: kcr Backport-of: af7e05716711d942df20eb1f807b384810a4a839 - PR: https://git.openjdk.org/jfx/pull/1355
[jfx22] RFR: 8324879: Platform-specific preferences keys are incorrect for Windows toolkit
This pull request contains a backport of commit [af7e0571](https://github.com/openjdk/jfx/commit/af7e05716711d942df20eb1f807b384810a4a839) from the [openjdk/jfx](https://git.openjdk.org/jfx) repository. The commit being backported was authored by Michael Strauß on 30 Jan 2024 and was reviewed by Kevin Rushforth and Andy Goryachev. - Commit messages: - Backport af7e05716711d942df20eb1f807b384810a4a839 Changes: https://git.openjdk.org/jfx/pull/1355/files Webrev: https://webrevs.openjdk.org/?repo=jfx=1355=00 Issue: https://bugs.openjdk.org/browse/JDK-8324879 Stats: 40 lines in 3 files changed: 33 ins; 0 del; 7 mod Patch: https://git.openjdk.org/jfx/pull/1355.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1355/head:pull/1355 PR: https://git.openjdk.org/jfx/pull/1355
Integrated: 8324879: Platform-specific preferences keys are incorrect for Windows toolkit
On Tue, 30 Jan 2024 00:53:51 GMT, Michael Strauß wrote: > `WinApplication` contains mappings of platform-specific preference keys to > platform-independent keys. > These keys are incorrect (for example, `Windows.UIColor.ForegroundColor` > instead of `Windows.UIColor.Foreground`). > > This was not discovered during testing because the manual test application > doesn't show the values of platform-independent properties (it only shows the > platform-specific mappings). I've exended the test application to also show > the platform-independent property values. This pull request has now been integrated. Changeset: af7e0571 Author:Michael Strauß URL: https://git.openjdk.org/jfx/commit/af7e05716711d942df20eb1f807b384810a4a839 Stats: 40 lines in 3 files changed: 33 ins; 0 del; 7 mod 8324879: Platform-specific preferences keys are incorrect for Windows toolkit Reviewed-by: kcr, angorya - PR: https://git.openjdk.org/jfx/pull/1353
Re: Binding properties to constant values
Hi John, the rule that values set from code are less specific than values set in author and inline styles is probably lifted from the CSS specification, which says [0]: "The UA may choose to honor presentational attributes in an HTML source document. If so, these attributes are translated to the corresponding CSS rules with specificity equal to 0, and are treated as if they were inserted at the start of the author style sheet." However, it seems like an arbitrary fact that attributes in an FXML document are equivalent to calling setters from code. Attributes in FXML documents could just as well be their own thing, couldn't they? Let's assume for a moment that we could change that (which is a breaking change), and make a distinction between values that come from FXML attributes and values that come from code. This would introduce a new StyleOrigin.ATTRIBUTE, which would be the origin for all attribute values set by FXMLLoader (or by anyone, this is a public API in StyleableProperty after all). However, bindings in FXML documents would probably continue to use the USER origin. The CSS system would then never change values with USER origin, since they definitely don't come from attribute values. With regards to your idea of only allowing cssProperty.setValue() if the current value doesn't come from an AUTHOR or INLINE style: it seems like this *will* work some of the time, namely before CSS is applied for the first time. [0] https://www.w3.org/TR/CSS22/cascade.html#preshint On Tue, Jan 30, 2024 at 3:22 PM John Hendrikx wrote: > > Hi Michael, > > I think we first need to decide what the correct behavior is for CSS > properties, as the "bind" solution IMHO is a bug. > > The StyleOrigin enum encodes the relative priorities of styles and user > set values, but it is incomplete and not fully enforced. There is > (currently) actually a 5th level (next to USER_AGENT, USER, AUTHOR and > INLINE) where it checks the binding state (it has no choice, as it will > get an exception otherwise, or has to call `unbind` first). Whether > that's a bug or should more formally be accepted as the correct behavior > remains to be seen. Also the AUTHOR and INLINE levels are only best > effort, as setting a value in code (USER level) will override AUTHOR and > INLINE values **temporarily** until the next CSS pass... > > So first questions to answer IMHO are: > > 1) Does it make sense to treat values set in code (which IMHO should > always be the last word on anything) as less important than values from > AUTHOR and INLINE styles? This is specified in the CSS document, but > not fully enforced. > > 2) Should bound values (which IMHO fall under "values set from code") be > able to override AUTHOR and INLINE styles? Why are they being treated > differently at all? Which StyleOrigin level do they fall under? > "USER"? A 5th level with higher priority than "INLINE"? > > 3) Should properties which hold an AUTHOR or INLINE value reject calls > to `setValue` (to make it clear they are temporary and that your code is > probably wrong)? This currently is not in line with the CSS document. > Note that this will be slightly annoying for the CSS engine as it may > not be able to "reset" such values as easily as before (which is > probably the reason it currently works the way it does, but that's no > excuse IMHO). > > As for your potential solution, if you introduce a constant binding > system (to solve a CSS problem), does that make sense for properties as > a whole? What can be achieved with `bindConstant` that can't be done > with `setValue`? `bindConstant` will become the "setter" that always > works (never throws an exception...), but probably at a higher cost than > using `setValue`. Would it not make more sense to only have such > methods on the Styleable properties (which can then also signal this by > using an even higher precedence StyleOrigin instead of relying on > bound/unbound) once there is agreement on the above questions? > > In other words, I'd look more in the direction of providing users with a > better "setter" only for CSS properties, that also uses a different > StyleOrigin, and to bring both binding and setting in line with the CSS > document's specification (or alternatively, to change that > specification). This means that the normal setter provided next to the > property method (ie. setXXX) would have to default to some standard > behavior, while a more specific setter provided on the property itself > can have an overriding behavior, something like: > > setX() -> calls cssProperty.setValue() > cssProperty.setValue() -> sets values if not originated from an > AUTHOR or INLINE stylesheet, otherwise throws exception (as if bound) > cssProperty.forceValue() -> sets value unconditionally, setting > StyleOrigin to some new to introduce 5th level > (StyleOrigin.FORCED/DEVELOPER/DEBUG/CONSTANT/FINAL) > > Binding can then either be categorized as the StyleOrigin.FORCED or if > it is
Binding properties to constant values
Hi everyone,
I'm interested in hearing your thoughts on the following proposal,
which could increase the expressiveness of the JavaFX Property API:
Problem
---
The JavaFX CSS system applies the following order of precedence to
determine the value of a styleable property (in ascending
specificity):
1. user-agent stylesheets
2. values set from code
3. Scene stylesheets
4. Parent stylesheets
5. inline styles
While this system works quite well in general, applications sometimes
need to override individual property values from code. However, this
doesn't work reliably in the presence of Scene or Parent stylesheets.
There are two usual workarounds to solve this problem:
A) Use an inline style instead of setting the property value directly.
This is obviously not a good solution, as you'll lose the strong
typing afforded by the Java language. Additionally, the value must be
a true constant, it can't be an expression or the result of a
computation.
B) Create an `ObservableValue` instance that holds the desired value,
and bind the property to it.
This is a much better solution. However, what we really want is just
the binding semantics: the bound property becomes unmodifiable and has
the highest precedence in the CSS cascade. But the API only gives us
binding semantics if we give it an `ObservableValue`.
Solution
I'm proposing to separate the toggles "binding semantics" and
"observability". While observability requires you to provide an
`ObservableValue`, binding semantics should work with both observable
and regular values.
This is a powerful addition to the Property API, since it increases
the expressiveness of the API in a natural way:
// instead of:
rect.setStyle("-fx-fill: red; -fx-width: 200");
// you can use strongly-typed Java code:
rect.fillProperty().bindConstant(Color.RED);
rect.widthProperty().bindConstant(200);
Since the `bindConstant` method accepts a value of the property type,
all features of the Java language can be used to fill in the value.
Implementation
--
The following method will be added to the `Property` interface:
default void bindConstant(T value) {
bind(ObjectConstant.valueOf(value));
}
Specialized methods will be added to `BooleanProperty`,
`DoubleProperty`, `FloatProperty`, `IntegerProperty`, and
`LongProperty`, each with one of the preexisting constant wrappers
that are already in the framework.
Some wrappers can be deduplicated (we only ever need two wrapper
instances for boolean values).
Re: RFR: 8260013: Snapshot does not work for nodes in a subscene [v4]
On Mon, 29 Jan 2024 09:32:06 GMT, Lukasz Kostyra wrote: >> Originally this issue showed the problem of Node being incorrectly rendered >> (clipped) when snapshotting, compared to a snapshot of the whole Scene. >> Later on there was another problem added - lights not being taken into >> account if they are added to a SubScene. >> >> As it later turned out, the original problem from this bug report is a >> problem with ParallelCamera incorrectly estimating near/far clipping planes, >> which just happened to reveal itself while snapshotting a Node. During >> testing I found out you can make the Node clip regardless of snapshot >> mechanism. Clipping issue was moved to a separate bug report and this PR >> only fixes the inconsistency in lights being gathered for a snapshot. >> >> `Scene.doSnapshot()` was expanded to also check if SubScene provided to it >> is non-null and to fetch lights assigned to it. Scenario was tested with >> added SnapshotLightsTest. >> >> Rest of the tests were checked and don't produce any noticeable regressions. > > Lukasz Kostyra has updated the pull request incrementally with one additional > commit since the last revision: > > Minor review adjustments Looks good! - Marked as reviewed by mstrauss (Committer). PR Review: https://git.openjdk.org/jfx/pull/1332#pullrequestreview-1850271783
Withdrawn: 8324219: Remove incorrect documentation from Animation methods
On Fri, 19 Jan 2024 16:07:31 GMT, Michael Strauß wrote:
> The `Animation` class states in the documentation of various methods that the
> method call would be asynchronous, using language similar to:
>
>
> {@code stop()} is an asynchronous call, the {@code Animation} may not stop
> immediately.
>
>
> This is factually wrong, there are no asynchronous calls. In fact, the
> methods in question can only be called synchronously on the JavaFX
> Application thread, and the state of the animation is changed immediately.
> For example, when `stop()` is called, the animation transitions to the
> stopped state instantly, without waiting for the next animation pulse.
This pull request has been closed without being integrated.
-
PR: https://git.openjdk.org/jfx/pull/1342
Re: RFR: JDK-8323615: PopupControl.skin.setSkin(Skin) fails to call dispose() on discarded Skin
On Thu, 11 Jan 2024 20:13:09 GMT, Marius Hanl wrote:
> For some reason the `skinProperty` did not allow to set a new skin when it is
> the same class as the previous one.
> This leads to multiple issues:
> 1. When creating a new skin (same class as previous), the skin will likely
> install listener but is then rejected when setting it due to the
> `skinProperty` class constraint
> -> `PopupControl` is in a weird state as the current skin was not disposed
> since it is still set, although we already created and 'set' a new skin
> 2. A skin of the same class can behave differently, so it is not really valid
> to reject a skin just because it is the same class as the previous
> -> Just imagine we have the following skin class
>
> class MyCustomSkin extends Skin {
> public MyCustomSkin(C skinnable, boolean someFlag) { ... }
> }
>
> Now if we would change the skin of the `PopupControl` two times like this:
>
> popup.setSkin(new MyCustomSkin(popup, true));
> popup.setSkin(new MyCustomSkin(popup, false));
>
> The second time the skin will be rejected as it is the same class, although I
> may changed the skin behaviour, in this case demonstrated by a boolean flag
> for showing purposes.
>
> This is the same issue and fix as done for `Control` in
> [JDK-8276056](https://bugs.openjdk.org/browse/JDK-8276056) (PR:
> https://github.com/openjdk/jfx/pull/806)
Looks good, and works as I expect it.
-
Marked as reviewed by mstrauss (Committer).
PR Review: https://git.openjdk.org/jfx/pull/1331#pullrequestreview-1850261757
RFR: 8324879: Platform-specific preferences keys are incorrect for Windows toolkit
`WinApplication` contains mappings of platform-specific preference keys to platform-independent keys. These keys are incorrect (for example, `Windows.UIColor.ForegroundColor` instead of `Windows.UIColor.Foreground`). This was not discovered during testing because the manual test application doesn't show the values of platform-independent properties (it only shows the platform-specific mappings). I've exended the test application to also show the platform-independent property values. - Commit messages: - fix incorrect key mappings for windows Changes: https://git.openjdk.org/jfx/pull/1353/files Webrev: https://webrevs.openjdk.org/?repo=jfx=1353=00 Issue: https://bugs.openjdk.org/browse/JDK-8324879 Stats: 40 lines in 3 files changed: 33 ins; 0 del; 7 mod Patch: https://git.openjdk.org/jfx/pull/1353.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1353/head:pull/1353 PR: https://git.openjdk.org/jfx/pull/1353
Re: Platform preferences theme detection
I see that the names of the platform mappings defined in
WinApplication::getPlatformKeyMappings() are simply wrong
("Windows.UIColor.ForegroundColor" instead of
"Windows.UIColor.Foreground"), so the platform mappings are not applied to
the properties.
That's quite surprising, and it's a change that must have slipped into the
feature at a very late stage during development, so that it went unnoticed
by all reviewers.
I'll file a bug and prepare a fix for this issue.
On Mon, Jan 29, 2024 at 10:45 PM Christopher Schnick
wrote:
> Hello Michael,
>
> I took a look at the implementation and tried to find the issue. From what
> I can see, the mappings returned are correct:
>
>
>
> but it seems like they are somehow not applied the PreferencesProperties:
>
> Let me know whether I can help with anything to debug this issue.
>
>
Re: Platform preferences theme detection
Hi Christopher! > - Should this feature work in that ea version? Yes. > - Is Windows 10 supported by the color scheme detection? Color scheme detection should be supported on Windows 10 beginning with build 10240. > - The documentation says that LIGHT is returned in case theme the > detection is not supported. But I guess there is no way to find out > whether querying that property is supported on the current system? It > might be useful in some circumstances to adapt the behavior when we know > that this is not supported. There is no way to directly query whether any of the platform-independent properties (colorScheme, backgroundColor, foregroundColor, accentColor) are supported by the current OS. You can check whether the platform-dependent mappings (for example, "Windows.UIColor.Background") are contained in the map, which will only be the case if supported by the OS. > - The documentation mentions that some preferences might not be > available on the current system. Does that mean that if they are > available, they are also observable? Or might it be possible that a > certain setting is available but does not support dynamically updating > at runtime and cannot be observed? If you see a mapping for a preference, it should also be updated automatically if the corresponding OS setting is changed. Do you see the mappings for "Windows.UIColor.Background" and "Windows.UIColor.Foreground" in the map returned by Platform.getPreferences()? The color scheme value is derived from these colors. If the mappings are not present, something unexpected is happening. On Mon, Jan 29, 2024 at 9:23 PM Christopher Schnick wrote: > > Hello, > > I just tried out the new 22-ea+27 build to see whether we can utilize > some of the new features, particularly the platform preferences API, to > replace the library https://github.com/Dansoftowner/jSystemThemeDetector > that we are currently using for theme detection. > > I'm running this on Windows 10 with the latest updates and the color > scheme always reports LIGHT, even when dark mode is enabled in the > settings. The observable value also does not update when the value > changes in the settings. Obviously I have a few questions here: > - Should this feature work in that ea version? > - Is Windows 10 supported by the color scheme detection? > - The documentation says that LIGHT is returned in case theme the > detection is not supported. But I guess there is no way to find out > whether querying that property is supported on the current system? It > might be useful in some circumstances to adapt the behavior when we know > that this is not supported. > - The documentation mentions that some preferences might not be > available on the current system. Does that mean that if they are > available, they are also observable? Or might it be possible that a > certain setting is available but does not support dynamically updating > at runtime and cannot be observed? > > Best regards > Christopher >
Re: HEADS-UP: Threading restriction for Animation play, pause, stop now enforced USE CASE
Hi Kevin, have you considered my proposal, which is basically the same approach: it uses runLater internally to dispatch the interaction with AbstractPrimaryTimer (instead of trying to make AbstractPrimaryTimer work under concurrent access). But from the point of view of the caller, the API works just as it worked before: all state changes of the Animation class are instantly visible, there is no surprising change as it would be with what you're currently proposing. With my proposal, play() can be called on a background thread, and the behavior of the Animation class remains the same. If you're reading getStatus() after calling play(), you will always see that the animation is currently running (even if internally, the animation hasn't yet been added to the primary timer). On Thu, Jan 25, 2024 at 6:30 PM Kevin Rushforth wrote: > > And that's why the "be careful" option is not a satisfying solution. > > Let's proceed with the option to change the implementation of > play/pause/stop/start to do the work in a runLater, and change the docs > accordingly. It's the only feasible option for JavaFX 22 (other than "do > nothing"), and I also think it is a good choice. If we later want to evolve > it for thread-safety, which I'm not convinced that we do, we can consider > that for a future release. > > -- Kevin
