Re: RFR: 8268642: Improve property system to facilitate correct usage [v3]
> Based on previous discussions, this PR attempts to improve the JavaFX
> property system by enforcing correct API usage in several cases that are
> outlined below. It also streamlines the API by deprecating untyped APIs in
> favor of typed APIs that better express intent.
>
> ### 1. Behavioral changes for regular bindings
>
> var target = new SimpleObjectProperty(bean, "target");
> var source = new SimpleObjectProperty(bean, "source");
> target.bind(source);
> target.bindBidirectional(source);
>
> _Before:_ `RuntimeException` --> "bean.target: A bound value cannot be set."
> _After:_ `IllegalStateException` --> "bean.target: Bidirectional binding
> cannot target a bound property."
>
>
> var target = new SimpleObjectProperty(bean, "target");
> var source = new SimpleObjectProperty(bean, "source");
> var other = new SimpleObjectProperty(bean, "other");
> source.bind(other);
> target.bindBidirectional(source);
>
> _Before:_ no error
> _After:_ `IllegalArgumentException` --> "bean.source: Bidirectional binding
> cannot target a bound property."
>
>
> var target = new SimpleObjectProperty(bean, "target");
> var source = new SimpleObjectProperty(bean, "source");
> target.bindBidirectional(source);
> target.bind(source);
>
> _Before:_ no error
> _After:_ `IllegalStateException` --> "bean.target: Cannot bind a property
> that is targeted by a bidirectional binding."
>
>
> ### 2. Behavioral changes for content bindings
>
> var target = new SimpleListProperty(bean, "target");
> var source = new SimpleListProperty(bean, "source");
> target.bindContent(source);
> target.bindContentBidirectional(source);
>
> _Before:_ no error
> _After:_ `IllegalStateException` --> "bean.target: Bidirectional content
> binding cannot target a bound collection."
>
>
> var target = new SimpleListProperty(bean, "target");
> var source = new SimpleListProperty(bean, "source");
> var other = new SimpleListProperty();
> source.bindContent(other);
> target.bindContentBidirectional(source);
>
> _Before:_ no error
> _After:_ `IllegalArgumentException` --> "bean.source: Bidirectional content
> binding cannot target a bound collection."
>
>
> var target = new SimpleListProperty(bean, "target");
> var source = new SimpleListProperty(bean, "source");
> target.bindContentBidirectional(source);
> target.bindContent(source);
>
> _Before:_ no error
> _After:_ `IllegalStateException` --> "bean.target: Cannot bind a collection
> that is targeted by a bidirectional content binding."
>
>
> var target = new SimpleListProperty(bean, "target");
> var source = new SimpleListProperty(bean, "source");
> target.bindContent(source);
> target.set(FXCollections.observableArrayList());
>
> _Before:_ no error
> _After:_ `IllegalStateException` --> "bean.target: Cannot set the value of a
> content-bound property."
>
>
> var target = new SimpleListProperty(bean, "target");
> var source = new SimpleListProperty(bean, "source");
> target.bindContent(source);
> target.add("foo");
>
> _Before_: no error, but binding is broken because the lists are in an
> inconsistent state
> _After:_ `RuntimeExeption` via `Thread.UncaughtExceptionHandler` --> "Illegal
> list modification: Content binding was removed because the lists are
> out-of-sync."
>
>
> var target = new SimpleListProperty(bean, "target");
> var source = new SimpleListProperty(bean, "source");
> target.bindContentBidirectional(source);
> target.bindContentBidirectional(source);
> target.add("foo");
>
> _Before_: no error, but `target` contains `[foo, foo, foo, foo]`, while
> `source` contains `[foo, foo, foo]`
> _After:_ no error, both lists contain `[foo]` (the second binding replaces
> the first)
>
>
> ### 3. Align un-binding of unidirectional content bindings with regular
> unidirectional bindings
> The API of unidirectional content bindings is aligned with regular
> unidirectional bindings because the semantics are similar. Like `unbind()`,
> `unbindContent(Object)` should not need an argument because there can only be
> a single content binding. For this reason, `unbindContent(Object)` will be
> deprecated in favor of `unbindContent()`:
>
> void bindContent(ObservableList source);
> + void unbindContent();
> + boolean isContentBound();
>
> + @Deprecated(since = "18", forRemoval = true)
> void unbindContent(Object object);
>
>
> ### 4. Replace untyped binding APIs with typed APIs
> The following untyped APIs will be marked for removal in favor of typed
> replacements:
> In `javafx.beans.binding.Bindings`:
>
> @Deprecated(since = "18", forRemoval = true)
> void unbindBidirectional(Object property1, Object property2)
>
> @Deprecated(since = "18", forRemoval = true)
> void unbindContentBidirectional(Object obj1, Object obj2)
>
> @Deprecated(since = "18", forRemoval = true)
> void unbindContent(Object obj1, Object obj2)
>
>
> In `ReadOnlyListProperty`, `ReadOnlySetProperty`, `ReadOnlyMapProperty`:
>
> @Deprecated(since = "18", forRemoval = true)
> void
Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages [v2]
On Sun, 1 Jan 2023 16:08:15 GMT, John Hendrikx wrote:
>> Packages fixed:
>> - com.sun.javafx.binding
>> - com.sun.javafx.collections
>> - javafx.beans
>> - javafx.beans.binding
>> - javafx.collections
>> - javafx.collections.transformation
>
> John Hendrikx has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Clean up expression classes repeated null checks
> - Use instanceof with pattern matching in a few spots
I took a closer look at the sorting-related classes. I think that the design
there is not ideal. Fixing it might be out of scope for this PR. I wouldn't
mind fixing it myself, by the way.
The way the JDK treats soring on lists is with a single `sort(Comparable c)`
method that, when receiving `null`, assumes that the elements are `Comparable`
and throws if not. `Collections` has 2 static sorting methods that help enforce
this condition, where the one that doesn't take a `Comparator` passes `null` to
the one that does.
JavaFX tries to emulate this. `FXCollections` has the same 2 methods for
`ObservableList`, but one doesn't call the other. The asymmetry comes from
`SortableList` (which doesn't live up to its name - non-sortable lists can also
implement it). It defines a 0-argument method and a `Comparator`-argument
method as different behaviors, instead of one calling the other. This is
deferred to its implementations, `ObservableListWrapper` and
`ObservableSequentialListWrapper`, which make the differentiation by, again,
calling 2 different methods on `SortHelper`.
I suggest that the argument check be made at the lowest level, like in the JDK
(inside `Arrays` sort method). First, `FXCollections` can change to:
public static > void sort(ObservableList
list) {
sort(list, null); // or Comparator.naturalOrder() instead of null
}
public static void sort(ObservableList list, Comparator
c) {
if (list instanceof SortableList) {
list.sort(c);
} else {
List newContent = new ArrayList<>(list);
newContent.sort(c);
list.setAll(newContent);
}
}
`SortableList` then removes its `sort()` method, and now it's just overriding
`List::sort(Comparator)` without doing anything with it, so it can be removed
too, and it's left as a marker interface for the special sorting in
`SortHelper` (I haven't understood yet why it's better , I need to dig there
more):
public interface SortableList {}
Now `ObservableListWrapper` and `ObservableSequentialListWrapper` also remove
`sort()`, and override `List::sort(Comparator)` directly and in accordance with
its contract, passing the (possibly `null`) comparator to `SortHelper`, which
is akin to the JDK's `Arrays::sort` method.
Now I need to look into `SortHelper` to see what it does exactly and how to
change it. I think it doesn't need the `sort(List list)` method too now
because it doesn't really enforce that the elements are `Comparable`, it will
naturally throw if not.
---
In my opinion, this is the design flaw: `ObservableList` should have overridden
`List`'s sort method to specify that its sorting is done as one change:
public interface ObservableList extends List, Observable {
@Override
default void sort(Comparator c) {
var newContent = new ArrayList<>(this);
newContent.sort(c);
setAll(newContent);
}
Then we wouldn't need the `SortableList` marker because `FXCollections` could
just call this method directly (like `Collections` does, which means that
`FXCollections`'s methods are not needed anymore, though we can't remove them),
and whichever implementation wants to do a more efficient sorting, like
`ObservableListWrapper`, can override again.
This will be a behavioral change, though, because the generated change events
differ.
-
PR: https://git.openjdk.org/jfx/pull/972
Re: RFR: 8268642: Improve property system to facilitate correct usage [v2]
> Based on previous discussions, this PR attempts to improve the JavaFX
> property system by enforcing correct API usage in several cases that are
> outlined below. It also streamlines the API by deprecating untyped APIs in
> favor of typed APIs that better express intent.
>
> ### 1. Behavioral changes for regular bindings
>
> var target = new SimpleObjectProperty(bean, "target");
> var source = new SimpleObjectProperty(bean, "source");
> target.bind(source);
> target.bindBidirectional(source);
>
> _Before:_ `RuntimeException` --> "bean.target: A bound value cannot be set."
> _After:_ `IllegalStateException` --> "bean.target: Bidirectional binding
> cannot target a bound property."
>
>
> var target = new SimpleObjectProperty(bean, "target");
> var source = new SimpleObjectProperty(bean, "source");
> var other = new SimpleObjectProperty(bean, "other");
> source.bind(other);
> target.bindBidirectional(source);
>
> _Before:_ no error
> _After:_ `IllegalArgumentException` --> "bean.source: Bidirectional binding
> cannot target a bound property."
>
>
> var target = new SimpleObjectProperty(bean, "target");
> var source = new SimpleObjectProperty(bean, "source");
> target.bindBidirectional(source);
> target.bind(source);
>
> _Before:_ no error
> _After:_ `IllegalStateException` --> "bean.target: Cannot bind a property
> that is targeted by a bidirectional binding."
>
>
> ### 2. Behavioral changes for content bindings
>
> var target = new SimpleListProperty(bean, "target");
> var source = new SimpleListProperty(bean, "source");
> target.bindContent(source);
> target.bindContentBidirectional(source);
>
> _Before:_ no error
> _After:_ `IllegalStateException` --> "bean.target: Bidirectional content
> binding cannot target a bound collection."
>
>
> var target = new SimpleListProperty(bean, "target");
> var source = new SimpleListProperty(bean, "source");
> var other = new SimpleListProperty();
> source.bindContent(other);
> target.bindContentBidirectional(source);
>
> _Before:_ no error
> _After:_ `IllegalArgumentException` --> "bean.source: Bidirectional content
> binding cannot target a bound collection."
>
>
> var target = new SimpleListProperty(bean, "target");
> var source = new SimpleListProperty(bean, "source");
> target.bindContentBidirectional(source);
> target.bindContent(source);
>
> _Before:_ no error
> _After:_ `IllegalStateException` --> "bean.target: Cannot bind a collection
> that is targeted by a bidirectional content binding."
>
>
> var target = new SimpleListProperty(bean, "target");
> var source = new SimpleListProperty(bean, "source");
> target.bindContent(source);
> target.set(FXCollections.observableArrayList());
>
> _Before:_ no error
> _After:_ `IllegalStateException` --> "bean.target: Cannot set the value of a
> content-bound property."
>
>
> var target = new SimpleListProperty(bean, "target");
> var source = new SimpleListProperty(bean, "source");
> target.bindContent(source);
> target.add("foo");
>
> _Before_: no error, but binding is broken because the lists are in an
> inconsistent state
> _After:_ `RuntimeExeption` via `Thread.UncaughtExceptionHandler` --> "Illegal
> list modification: Content binding was removed because the lists are
> out-of-sync."
>
>
> ### 3. Align un-binding of unidirectional content bindings with regular
> unidirectional bindings
> The API of unidirectional content bindings is aligned with regular
> unidirectional bindings because the semantics are similar. Like `unbind()`,
> `unbindContent(Object)` should not need an argument because there can only be
> a single content binding. For this reason, `unbindContent(Object)` will be
> deprecated in favor of `unbindContent()`:
>
> void bindContent(ObservableList source);
> + void unbindContent();
> + boolean isContentBound();
>
> + @Deprecated(since = "18", forRemoval = true)
> void unbindContent(Object object);
>
>
> ### 4. Replace untyped binding APIs with typed APIs
> The following untyped APIs will be marked for removal in favor of typed
> replacements:
> In `javafx.beans.binding.Bindings`:
>
> @Deprecated(since = "18", forRemoval = true)
> void unbindBidirectional(Object property1, Object property2)
>
> @Deprecated(since = "18", forRemoval = true)
> void unbindContentBidirectional(Object obj1, Object obj2)
>
> @Deprecated(since = "18", forRemoval = true)
> void unbindContent(Object obj1, Object obj2)
>
>
> In `ReadOnlyListProperty`, `ReadOnlySetProperty`, `ReadOnlyMapProperty`:
>
> @Deprecated(since = "18", forRemoval = true)
> void unbindContentBidirectional(Object object)
>
>
> ~~5. Support un-binding bidirectional conversion bindings with typed API
> At this moment, `StringProperty` is the only property implementation that
> offers bidirectional conversion bindings, where the `StringProperty` is bound
> to a property of another type:~~
>
> void bindBidirectional(Property other, StringConverter converter)
>
> ~~The inherited `Property.unbindBidir
Re: RFR: 8267546: Add CSS themes as a first-class concept [v17]
> This PR adds style themes as a first-class concept to OpenJFX. A style theme
> is a collection of stylesheets and the logic that governs them. Style themes
> can respond to OS notifications and update their stylesheets dynamically.
> This PR also re-implements Caspian and Modena as style themes.
>
> ### New APIs in `javafx.graphics`
> The new theming-related APIs in `javafx.graphics` provide a basic framework
> to support application-wide style themes. Higher-level theming concepts (for
> example, "dark mode" detection or accent coloring) are not a part of this
> basic framework, because any API invented here might soon be out of date.
> Implementations can build on top of this framework to add useful higher-level
> features.
> 1. StyleTheme
> A style theme is an implementation of the `javafx.css.StyleTheme` interface:
>
> /**
> * {@code StyleTheme} is a collection of stylesheets that specify the
> appearance of UI controls and other
> * nodes in the application. Like a user-agent stylesheet, a {@code
> StyleTheme} is implicitly used by all
> * JavaFX nodes in the scene graph.
> *
> * The list of stylesheets that comprise a {@code StyleTheme} can be modified
> while the application is running,
> * enabling applications to create dynamic themes that respond to changing
> user preferences.
> *
> * In the CSS subsystem, stylesheets that comprise a {@code StyleTheme} are
> classified as
> * {@link StyleOrigin#USER_AGENT} stylesheets, but have a higher precedence
> in the CSS cascade
> * than a stylesheet referenced by {@link
> Application#userAgentStylesheetProperty()}.
> */
> public interface StyleTheme {
> /**
> * Gets the list of stylesheet URLs that comprise this {@code StyleTheme}.
> *
> * If the list of stylesheets that comprise this {@code StyleTheme} is
> changed at runtime, this
> * method must return an {@link ObservableList} to allow the CSS
> subsystem to subscribe to list
> * change notifications.
> *
> * @implSpec Implementations of this method that return an {@link
> ObservableList} must emit all
> * change notifications on the JavaFX application thread.
> *
> * @implNote Implementations of this method that return an {@link
> ObservableList} are encouraged
> * to minimize the number of subsequent list change
> notifications that are fired by the
> * list, as each change notification causes the CSS subsystem
> to re-apply the referenced
> * stylesheets.
> */
> List getStylesheets();
> }
>
>
> A new `styleTheme` property is added to `javafx.application.Application`, and
> `userAgentStylesheet` is promoted to a JavaFX property (currently, this is
> just a getter/setter pair):
>
> public class Application {
> ...
> /**
> * Specifies the user-agent stylesheet of the application.
> *
> * A user-agent stylesheet is a global stylesheet that can be specified
> in addition to a
> * {@link StyleTheme} and that is implicitly used by all JavaFX nodes in
> the scene graph.
> * It can be used to provide default styling for UI controls and other
> nodes.
> * A user-agent stylesheets has the lowest precedence in the CSS cascade.
> *
> * Before JavaFX 20, built-in themes were selectable using the special
> user-agent stylesheet constants
> * {@link #STYLESHEET_CASPIAN} and {@link #STYLESHEET_MODENA}. For
> backwards compatibility, the meaning
> * of these special constants is retained: setting the user-agent
> stylesheet to either {@code STYLESHEET_CASPIAN}
> * or {@code STYLESHEET_MODENA} will also set the value of the {@link
> #styleThemeProperty() styleTheme}
> * property to a new instance of the corresponding theme class.
> *
> * Note: this property can be modified on any thread, but it is not
> thread-safe and must
> * not be concurrently modified with {@link #styleThemeProperty()
> styleTheme}.
> */
> public static StringProperty userAgentStylesheetProperty();
> public static String getUserAgentStylesheet();
> public static void setUserAgentStylesheet(String url);
>
> /**
> * Specifies the {@link StyleTheme} of the application.
> *
> * {@code StyleTheme} is a collection of stylesheets that define the
> appearance of the application.
> * Like a user-agent stylesheet, a {@code StyleTheme} is implicitly used
> by all JavaFX nodes in the
> * scene graph.
> *
> * Stylesheets that comprise a {@code StyleTheme} have a higher
> precedence in the CSS cascade than a
> * stylesheet referenced by the {@link #userAgentStylesheetProperty()
> userAgentStylesheet} property.
> *
> * Note: this property can be modified on any thread, but it is not
> thread-safe and must not be
> * concurrently modified with {@link #userAgentStylesheetProperty()
> userAgentStylesheet}.
>
Re: RFR: 8267546: Add CSS themes as a first-class concept [v16]
> This PR adds style themes as a first-class concept to OpenJFX. A style theme
> is a collection of stylesheets and the logic that governs them. Style themes
> can respond to OS notifications and update their stylesheets dynamically.
> This PR also re-implements Caspian and Modena as style themes.
>
> ### New APIs in `javafx.graphics`
> The new theming-related APIs in `javafx.graphics` provide a basic framework
> to support application-wide style themes. Higher-level theming concepts (for
> example, "dark mode" detection or accent coloring) are not a part of this
> basic framework, because any API invented here might soon be out of date.
> Implementations can build on top of this framework to add useful higher-level
> features.
> 1. StyleTheme
> A style theme is an implementation of the `javafx.css.StyleTheme` interface:
>
> /**
> * {@code StyleTheme} is a collection of stylesheets that specify the
> appearance of UI controls and other
> * nodes in the application. Like a user-agent stylesheet, a {@code
> StyleTheme} is implicitly used by all
> * JavaFX nodes in the scene graph.
> *
> * The list of stylesheets that comprise a {@code StyleTheme} can be modified
> while the application is running,
> * enabling applications to create dynamic themes that respond to changing
> user preferences.
> *
> * In the CSS subsystem, stylesheets that comprise a {@code StyleTheme} are
> classified as
> * {@link StyleOrigin#USER_AGENT} stylesheets, but have a higher precedence
> in the CSS cascade
> * than a stylesheet referenced by {@link
> Application#userAgentStylesheetProperty()}.
> */
> public interface StyleTheme {
> /**
> * Gets the list of stylesheet URLs that comprise this {@code StyleTheme}.
> *
> * If the list of stylesheets that comprise this {@code StyleTheme} is
> changed at runtime, this
> * method must return an {@link ObservableList} to allow the CSS
> subsystem to subscribe to list
> * change notifications.
> *
> * @implSpec Implementations of this method that return an {@link
> ObservableList} must emit all
> * change notifications on the JavaFX application thread.
> *
> * @implNote Implementations of this method that return an {@link
> ObservableList} are encouraged
> * to minimize the number of subsequent list change
> notifications that are fired by the
> * list, as each change notification causes the CSS subsystem
> to re-apply the referenced
> * stylesheets.
> */
> List getStylesheets();
> }
>
>
> A new `styleTheme` property is added to `javafx.application.Application`, and
> `userAgentStylesheet` is promoted to a JavaFX property (currently, this is
> just a getter/setter pair):
>
> public class Application {
> ...
> /**
> * Specifies the user-agent stylesheet of the application.
> *
> * A user-agent stylesheet is a global stylesheet that can be specified
> in addition to a
> * {@link StyleTheme} and that is implicitly used by all JavaFX nodes in
> the scene graph.
> * It can be used to provide default styling for UI controls and other
> nodes.
> * A user-agent stylesheets has the lowest precedence in the CSS cascade.
> *
> * Before JavaFX 20, built-in themes were selectable using the special
> user-agent stylesheet constants
> * {@link #STYLESHEET_CASPIAN} and {@link #STYLESHEET_MODENA}. For
> backwards compatibility, the meaning
> * of these special constants is retained: setting the user-agent
> stylesheet to either {@code STYLESHEET_CASPIAN}
> * or {@code STYLESHEET_MODENA} will also set the value of the {@link
> #styleThemeProperty() styleTheme}
> * property to a new instance of the corresponding theme class.
> *
> * Note: this property can be modified on any thread, but it is not
> thread-safe and must
> * not be concurrently modified with {@link #styleThemeProperty()
> styleTheme}.
> */
> public static StringProperty userAgentStylesheetProperty();
> public static String getUserAgentStylesheet();
> public static void setUserAgentStylesheet(String url);
>
> /**
> * Specifies the {@link StyleTheme} of the application.
> *
> * {@code StyleTheme} is a collection of stylesheets that define the
> appearance of the application.
> * Like a user-agent stylesheet, a {@code StyleTheme} is implicitly used
> by all JavaFX nodes in the
> * scene graph.
> *
> * Stylesheets that comprise a {@code StyleTheme} have a higher
> precedence in the CSS cascade than a
> * stylesheet referenced by the {@link #userAgentStylesheetProperty()
> userAgentStylesheet} property.
> *
> * Note: this property can be modified on any thread, but it is not
> thread-safe and must not be
> * concurrently modified with {@link #userAgentStylesheetProperty()
> userAgentStylesheet}.
>
Re: RFR: 8299423: JavaFX Mac system menubar leaks [v2]
On Mon, 2 Jan 2023 09:26:14 GMT, Florian Kirmaier wrote: >> This PR fixes the leak in the mac system menu bar. >> >> Inside the native code, NewGlobalRef is called for the callable. >> Which makes it into a "GC-Root" until DeleteGlobalRef is called. >> >> The DeleteGlobalRef is never called for the MenuEntry, if it's removed from >> the menu without removing it's callable. >> This PR adds logic, whether the Menu is inserted. If it's not inserted in a >> Menu anymore, then DeleteGlobalRef is called, by calling `_setCallback` with >> the callable "null". >> >> The unit test verifies, that this bug happened without this change, but no >> longer happens with this change. > > Florian Kirmaier has updated the pull request incrementally with one > additional commit since the last revision: > > JDK-8299423 > Simplified the fix for the mac-menu-bar based on code review If you're going forward with weak global references, you need to be aware that objects only referred to by weak references can be collected at any time. In order to do anything meaningful with a weak global reference, you need to create a new local reference to it and check whether the local reference is null (and don't forget to delete the local reference after using it). Note that a local reference is automatically created for any object passed to a JNI method, so you only need to do that manually when the callback is used in other places. - PR: https://git.openjdk.org/jfx/pull/987
Re: RFR: 8273379 - GTK3 stops sending key events during drag and drop [v10]
> This PR fixes 8273379. > > I reverted back to use GDK (from > [8225571](https://bugs.openjdk.org/browse/JDK-8225571)) to handle the events. > > It may also fix [8280383](https://bugs.openjdk.org/browse/JDK-8280383). > > There's also some cleaup. > > The most changed part is that I had to move `process_events` to > `glass_evloop` because it's reused in glass_dnd. > > To do general testing: > `java @build/run.args -jar apps/toys/DragDrop/dist/DragDrop.jar` Thiago Milczarek Sayao has updated the pull request incrementally with one additional commit since the last revision: Gtk2 fixes - Changes: - all: https://git.openjdk.org/jfx/pull/986/files - new: https://git.openjdk.org/jfx/pull/986/files/c827893a..665656a4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=986&range=09 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=986&range=08-09 Stats: 26 lines in 1 file changed: 14 ins; 4 del; 8 mod Patch: https://git.openjdk.org/jfx/pull/986.diff Fetch: git fetch https://git.openjdk.org/jfx pull/986/head:pull/986 PR: https://git.openjdk.org/jfx/pull/986
Re: RFR: 8273379 - GTK3 stops sending key events during drag and drop [v9]
On Tue, 3 Jan 2023 23:29:13 GMT, Thiago Milczarek Sayao wrote: >> This PR fixes 8273379. >> >> I reverted back to use GDK (from >> [8225571](https://bugs.openjdk.org/browse/JDK-8225571)) to handle the >> events. >> >> It may also fix [8280383](https://bugs.openjdk.org/browse/JDK-8280383). >> >> There's also some cleaup. >> >> The most changed part is that I had to move `process_events` to >> `glass_evloop` because it's reused in glass_dnd. >> >> To do general testing: >> `java @build/run.args -jar apps/toys/DragDrop/dist/DragDrop.jar` > > Thiago Milczarek Sayao has updated the pull request incrementally with one > additional commit since the last revision: > > Simplify DragView paint This will need a lot of careful testing. Can you provide more information about why undoing the changes from [JDK-8225571](https://bugs.openjdk.org/browse/JDK-8225571) is the best approach? Also, have you tested that the various issues that were fixed by JDK-8225571 won't regress as a result? - PR: https://git.openjdk.org/jfx/pull/986
Re: RFR: 8299423: JavaFX Mac system menubar leaks [v2]
On Mon, 2 Jan 2023 09:26:14 GMT, Florian Kirmaier wrote:
>> This PR fixes the leak in the mac system menu bar.
>>
>> Inside the native code, NewGlobalRef is called for the callable.
>> Which makes it into a "GC-Root" until DeleteGlobalRef is called.
>>
>> The DeleteGlobalRef is never called for the MenuEntry, if it's removed from
>> the menu without removing it's callable.
>> This PR adds logic, whether the Menu is inserted. If it's not inserted in a
>> Menu anymore, then DeleteGlobalRef is called, by calling `_setCallback` with
>> the callable "null".
>>
>> The unit test verifies, that this bug happened without this change, but no
>> longer happens with this change.
>
> Florian Kirmaier has updated the pull request incrementally with one
> additional commit since the last revision:
>
> JDK-8299423
> Simplified the fix for the mac-menu-bar based on code review
> This PR adds logic, whether the Menu is inserted. If it's not inserted in a
> Menu anymore, then DeleteGlobalRef is called, by calling _setCallback with
> the callable "null".
That would be one approach, but it's not what this PR currently does. Instead,
I see that you changed the JNI code to use weak global references. Unless there
is code on the Java side that holds a strong reference, using a weak reference
for the callback can cause the opposite problem of premature GC, such that the
callback stops working in some cases. How certain are you that this can't
happen here?
modules/javafx.graphics/src/main/java/com/sun/glass/ui/mac/MacMenuDelegate.java
line 66:
> 64: boolean enabled, boolean
> checked) {
> 65: ptr = _createMenuItem(title, (char)shortcutKey, shortcutModifiers,
> 66: pixels, enabled, checked, callback);
This file now has only white-space changes, which should be reverted.
tests/system/src/test/java/test/javafx/scene/control/SystemMenuBarTest.java
line 1:
> 1: package test.javafx.scene.control;
This file needs a copyright header.
-
PR: https://git.openjdk.org/jfx/pull/987
Re: RFR: 8209017: CheckBoxTreeCell: graphic on TreeItem not always showing [v3]
On Tue, 3 Jan 2023 23:56:12 GMT, Nir Lisker wrote: >> A simple fix for the graphics of a `CheckBoxTreeCell` not showing after >> expand/collapse events. The graphics node needs to be released from the >> previous checkbox so that it doesn't serve as the graphics of more than one >> checkbox at once. If it does, only one checkbox (its skin) can actually use >> it as a scenegraph child, and it's not always the correct one. >> >> Added also a manual test app as I'm unsure about how to do an automatic test >> for this. The test app will be useful for later fixes, so I suggest adding >> it anyway. To test the fix, simply expand and collapse the root note >> repeatedly while it has a child. Before the fix the graphic will disappear >> after a few tries, after the fix it won't. You can test other hierarchies as >> well by adding children to the tree. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Removed thrown exception Marked as reviewed by angorya (Committer). - PR: https://git.openjdk.org/jfx/pull/970
Re: RFR: 8209017: CheckBoxTreeCell: graphic on TreeItem not always showing [v3]
On Tue, 3 Jan 2023 23:56:12 GMT, Nir Lisker wrote: >> A simple fix for the graphics of a `CheckBoxTreeCell` not showing after >> expand/collapse events. The graphics node needs to be released from the >> previous checkbox so that it doesn't serve as the graphics of more than one >> checkbox at once. If it does, only one checkbox (its skin) can actually use >> it as a scenegraph child, and it's not always the correct one. >> >> Added also a manual test app as I'm unsure about how to do an automatic test >> for this. The test app will be useful for later fixes, so I suggest adding >> it anyway. To test the fix, simply expand and collapse the root note >> repeatedly while it has a child. Before the fix the graphic will disappear >> after a few tries, after the fix it won't. You can test other hierarchies as >> well by adding children to the tree. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Removed thrown exception Marked as reviewed by jhendrikx (Author). - PR: https://git.openjdk.org/jfx/pull/970
Re: RFR: 8209017: CheckBoxTreeCell: graphic on TreeItem not always showing [v3]
> A simple fix for the graphics of a `CheckBoxTreeCell` not showing after > expand/collapse events. The graphics node needs to be released from the > previous checkbox so that it doesn't serve as the graphics of more than one > checkbox at once. If it does, only one checkbox (its skin) can actually use > it as a scenegraph child, and it's not always the correct one. > > Added also a manual test app as I'm unsure about how to do an automatic test > for this. The test app will be useful for later fixes, so I suggest adding it > anyway. To test the fix, simply expand and collapse the root note repeatedly > while it has a child. Before the fix the graphic will disappear after a few > tries, after the fix it won't. You can test other hierarchies as well by > adding children to the tree. Nir Lisker has updated the pull request incrementally with one additional commit since the last revision: Removed thrown exception - Changes: - all: https://git.openjdk.org/jfx/pull/970/files - new: https://git.openjdk.org/jfx/pull/970/files/4d89b0da..abb71bd4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=970&range=02 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=970&range=01-02 Stats: 3 lines in 1 file changed: 0 ins; 2 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/970.diff Fetch: git fetch https://git.openjdk.org/jfx pull/970/head:pull/970 PR: https://git.openjdk.org/jfx/pull/970
Re: RFR: 8209017: CheckBoxTreeCell: graphic on TreeItem not always showing [v2]
On Tue, 3 Jan 2023 23:39:36 GMT, Andy Goryachev wrote: >> Nir Lisker has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Added copyright header and changed package name > > tests/performance/checkboxTreeView/src/main/java/main/CheckBoxTreeEditor.java > line 26: > >> 24: */ >> 25: >> 26: package main; > > I think the package name "main" is ok here, considering this code will be > folded into the tester. There is only one package with 1 class. I didn't see any meaningful name for the package. - PR: https://git.openjdk.org/jfx/pull/970
Re: RFR: 8209017: CheckBoxTreeCell: graphic on TreeItem not always showing [v2]
On Tue, 3 Jan 2023 23:38:10 GMT, Nir Lisker wrote:
>> A simple fix for the graphics of a `CheckBoxTreeCell` not showing after
>> expand/collapse events. The graphics node needs to be released from the
>> previous checkbox so that it doesn't serve as the graphics of more than one
>> checkbox at once. If it does, only one checkbox (its skin) can actually use
>> it as a scenegraph child, and it's not always the correct one.
>>
>> Added also a manual test app as I'm unsure about how to do an automatic test
>> for this. The test app will be useful for later fixes, so I suggest adding
>> it anyway. To test the fix, simply expand and collapse the root note
>> repeatedly while it has a child. Before the fix the graphic will disappear
>> after a few tries, after the fix it won't. You can test other hierarchies as
>> well by adding children to the tree.
>
> Nir Lisker has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Added copyright header and changed package name
I verified that the test shows the problem without the fix and it looks good
with the fix.
I left one more minor comment on the test.
tests/performance/checkboxTreeView/src/main/java/main/CheckBoxTreeEditor.java
line 49:
> 47:
> 48: @Override
> 49: public void start(Stage stage) throws IOException {
Minor: nothing in this program can throw IOE, so this could be removed (along
with the import).
-
PR: https://git.openjdk.org/jfx/pull/970
Re: RFR: 8209017: CheckBoxTreeCell: graphic on TreeItem not always showing [v2]
On Tue, 3 Jan 2023 23:34:06 GMT, Nir Lisker wrote: >> A simple fix for the graphics of a `CheckBoxTreeCell` not showing after >> expand/collapse events. The graphics node needs to be released from the >> previous checkbox so that it doesn't serve as the graphics of more than one >> checkbox at once. If it does, only one checkbox (its skin) can actually use >> it as a scenegraph child, and it's not always the correct one. >> >> Added also a manual test app as I'm unsure about how to do an automatic test >> for this. The test app will be useful for later fixes, so I suggest adding >> it anyway. To test the fix, simply expand and collapse the root note >> repeatedly while it has a child. Before the fix the graphic will disappear >> after a few tries, after the fix it won't. You can test other hierarchies as >> well by adding children to the tree. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Added copyright header and changed package name Marked as reviewed by jhendrikx (Author). - PR: https://git.openjdk.org/jfx/pull/970
Re: RFR: 8209017: CheckBoxTreeCell: graphic on TreeItem not always showing [v2]
On Tue, 3 Jan 2023 23:38:10 GMT, Nir Lisker wrote: >> A simple fix for the graphics of a `CheckBoxTreeCell` not showing after >> expand/collapse events. The graphics node needs to be released from the >> previous checkbox so that it doesn't serve as the graphics of more than one >> checkbox at once. If it does, only one checkbox (its skin) can actually use >> it as a scenegraph child, and it's not always the correct one. >> >> Added also a manual test app as I'm unsure about how to do an automatic test >> for this. The test app will be useful for later fixes, so I suggest adding >> it anyway. To test the fix, simply expand and collapse the root note >> repeatedly while it has a child. Before the fix the graphic will disappear >> after a few tries, after the fix it won't. You can test other hierarchies as >> well by adding children to the tree. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Added copyright header and changed package name lgtm tests/performance/checkboxTreeView/src/main/java/main/CheckBoxTreeEditor.java line 26: > 24: */ > 25: > 26: package main; I think the package name "main" is ok here, considering this code will be folded into the tester. - Marked as reviewed by angorya (Committer). PR: https://git.openjdk.org/jfx/pull/970
Re: RFR: 8209017: CheckBoxTreeCell: graphic on TreeItem not always showing [v2]
> A simple fix for the graphics of a `CheckBoxTreeCell` not showing after > expand/collapse events. The graphics node needs to be released from the > previous checkbox so that it doesn't serve as the graphics of more than one > checkbox at once. If it does, only one checkbox (its skin) can actually use > it as a scenegraph child, and it's not always the correct one. > > Added also a manual test app as I'm unsure about how to do an automatic test > for this. The test app will be useful for later fixes, so I suggest adding it > anyway. To test the fix, simply expand and collapse the root note repeatedly > while it has a child. Before the fix the graphic will disappear after a few > tries, after the fix it won't. You can test other hierarchies as well by > adding children to the tree. Nir Lisker has updated the pull request incrementally with one additional commit since the last revision: Added copyright header and changed package name - Changes: - all: https://git.openjdk.org/jfx/pull/970/files - new: https://git.openjdk.org/jfx/pull/970/files/b1808afe..4d89b0da Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=970&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=970&range=00-01 Stats: 203 lines in 2 files changed: 114 ins; 89 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/970.diff Fetch: git fetch https://git.openjdk.org/jfx pull/970/head:pull/970 PR: https://git.openjdk.org/jfx/pull/970
Re: RFR: 8209017: CheckBoxTreeCell: graphic on TreeItem not always showing
On Tue, 3 Jan 2023 23:19:16 GMT, Andy Goryachev wrote: >> I wasn't sure exactly where to put it, but it's going to be used in a fix >> for a performance issue I found when a node has many children. >> >> If it will fit into a larger app, I don't mind. > > I'll move it there as a part of JDK-8299335 I think it's fine as a separate test program for now. Once we have a tester application, we can consider which existing tests should go there. - PR: https://git.openjdk.org/jfx/pull/970
Re: RFR: 8209017: CheckBoxTreeCell: graphic on TreeItem not always showing
On Tue, 6 Dec 2022 20:10:16 GMT, Nir Lisker wrote: > A simple fix for the graphics of a `CheckBoxTreeCell` not showing after > expand/collapse events. The graphics node needs to be released from the > previous checkbox so that it doesn't serve as the graphics of more than one > checkbox at once. If it does, only one checkbox (its skin) can actually use > it as a scenegraph child, and it's not always the correct one. > > Added also a manual test app as I'm unsure about how to do an automatic test > for this. The test app will be useful for later fixes, so I suggest adding it > anyway. To test the fix, simply expand and collapse the root note repeatedly > while it has a child. Before the fix the graphic will disappear after a few > tries, after the fix it won't. You can test other hierarchies as well by > adding children to the tree. The fix looks correct to me. I left a couple comments on the test program. tests/performance/checkboxTreeView/src/main/java/checkboxTreeView/CheckBoxTreeEditor.java line 1: > 1: package checkboxTreeView; This needs a copyright header. Also, the convention for package names is to use all lower case. - PR: https://git.openjdk.org/jfx/pull/970
Re: RFR: 8273379 - GTK3 stops sending key events during drag and drop [v9]
> This PR fixes 8273379. > > I reverted back to use GDK (from > [8225571](https://bugs.openjdk.org/browse/JDK-8225571)) to handle the events. > > It may also fix [8280383](https://bugs.openjdk.org/browse/JDK-8280383). > > There's also some cleaup. > > The most changed part is that I had to move `process_events` to > `glass_evloop` because it's reused in glass_dnd. > > To do general testing: > `java @build/run.args -jar apps/toys/DragDrop/dist/DragDrop.jar` Thiago Milczarek Sayao has updated the pull request incrementally with one additional commit since the last revision: Simplify DragView paint - Changes: - all: https://git.openjdk.org/jfx/pull/986/files - new: https://git.openjdk.org/jfx/pull/986/files/5150b5ce..c827893a Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=986&range=08 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=986&range=07-08 Stats: 33 lines in 1 file changed: 7 ins; 22 del; 4 mod Patch: https://git.openjdk.org/jfx/pull/986.diff Fetch: git fetch https://git.openjdk.org/jfx pull/986/head:pull/986 PR: https://git.openjdk.org/jfx/pull/986
Re: RFR: 8209017: CheckBoxTreeCell: graphic on TreeItem not always showing
On Tue, 3 Jan 2023 23:13:52 GMT, Nir Lisker wrote: >> tests/performance/checkboxTreeView/.classpath line 1: >> >>> 1: >> >> minor: should this project be moved to manual tests hierarchy? >> >> BTW, I am planning to create a monkey tester application which will include >> many tests, to simplify the testing/verification of fixes JDK-8299335 >> This code be added to the tester, either as a separate page, or as a data >> source option for one of the TreeTableView page. > > I wasn't sure exactly where to put it, but it's going to be used in a fix for > a performance issue I found when a node has many children. > > If it will fit into a larger app, I don't mind. I'll move it there as a part of JDK-8299335 - PR: https://git.openjdk.org/jfx/pull/970
Re: RFR: 8209017: CheckBoxTreeCell: graphic on TreeItem not always showing
On Tue, 3 Jan 2023 23:11:08 GMT, John Hendrikx wrote: > Looks good to me, releasing the graphic for empty cells. I'm left wondering > if it should also be releasing the bindings it makes in that case, although > that's less likely to cause (visible) issues. I could make that change. I don't see any issues either way. - PR: https://git.openjdk.org/jfx/pull/970
Re: RFR: 8209017: CheckBoxTreeCell: graphic on TreeItem not always showing
On Tue, 6 Dec 2022 20:10:16 GMT, Nir Lisker wrote: > A simple fix for the graphics of a `CheckBoxTreeCell` not showing after > expand/collapse events. The graphics node needs to be released from the > previous checkbox so that it doesn't serve as the graphics of more than one > checkbox at once. If it does, only one checkbox (its skin) can actually use > it as a scenegraph child, and it's not always the correct one. > > Added also a manual test app as I'm unsure about how to do an automatic test > for this. The test app will be useful for later fixes, so I suggest adding it > anyway. To test the fix, simply expand and collapse the root note repeatedly > while it has a child. Before the fix the graphic will disappear after a few > tries, after the fix it won't. You can test other hierarchies as well by > adding children to the tree. Looks good. - Marked as reviewed by angorya (Committer). PR: https://git.openjdk.org/jfx/pull/970
Re: RFR: 8209017: CheckBoxTreeCell: graphic on TreeItem not always showing
On Tue, 3 Jan 2023 23:07:19 GMT, Andy Goryachev wrote: >> A simple fix for the graphics of a `CheckBoxTreeCell` not showing after >> expand/collapse events. The graphics node needs to be released from the >> previous checkbox so that it doesn't serve as the graphics of more than one >> checkbox at once. If it does, only one checkbox (its skin) can actually use >> it as a scenegraph child, and it's not always the correct one. >> >> Added also a manual test app as I'm unsure about how to do an automatic test >> for this. The test app will be useful for later fixes, so I suggest adding >> it anyway. To test the fix, simply expand and collapse the root note >> repeatedly while it has a child. Before the fix the graphic will disappear >> after a few tries, after the fix it won't. You can test other hierarchies as >> well by adding children to the tree. > > tests/performance/checkboxTreeView/.classpath line 1: > >> 1: > > minor: should this project be moved to manual tests hierarchy? > > BTW, I am planning to create a monkey tester application which will include > many tests, to simplify the testing/verification of fixes JDK-8299335 > This code be added to the tester, either as a separate page, or as a data > source option for one of the TreeTableView page. I wasn't sure exactly where to put it, but it's going to be used in a fix for performance issue I found when a node has many children. If it will fit into a grater app I don't mind. - PR: https://git.openjdk.org/jfx/pull/970
Re: RFR: 8209017: CheckBoxTreeCell: graphic on TreeItem not always showing
On Tue, 6 Dec 2022 20:10:16 GMT, Nir Lisker wrote: > A simple fix for the graphics of a `CheckBoxTreeCell` not showing after > expand/collapse events. The graphics node needs to be released from the > previous checkbox so that it doesn't serve as the graphics of more than one > checkbox at once. If it does, only one checkbox (its skin) can actually use > it as a scenegraph child, and it's not always the correct one. > > Added also a manual test app as I'm unsure about how to do an automatic test > for this. The test app will be useful for later fixes, so I suggest adding it > anyway. To test the fix, simply expand and collapse the root note repeatedly > while it has a child. Before the fix the graphic will disappear after a few > tries, after the fix it won't. You can test other hierarchies as well by > adding children to the tree. Looks good to me, releasing the graphic for empty cells. I'm left wondering if it should also be releasing the bindings it makes in that case, although that's less likely to cause (visible) issues. - Marked as reviewed by jhendrikx (Author). PR: https://git.openjdk.org/jfx/pull/970
Re: RFR: 8209017: CheckBoxTreeCell: graphic on TreeItem not always showing
On Tue, 6 Dec 2022 20:10:16 GMT, Nir Lisker wrote: > A simple fix for the graphics of a `CheckBoxTreeCell` not showing after > expand/collapse events. The graphics node needs to be released from the > previous checkbox so that it doesn't serve as the graphics of more than one > checkbox at once. If it does, only one checkbox (its skin) can actually use > it as a scenegraph child, and it's not always the correct one. > > Added also a manual test app as I'm unsure about how to do an automatic test > for this. The test app will be useful for later fixes, so I suggest adding it > anyway. To test the fix, simply expand and collapse the root note repeatedly > while it has a child. Before the fix the graphic will disappear after a few > tries, after the fix it won't. You can test other hierarchies as well by > adding children to the tree. tests/performance/checkboxTreeView/.classpath line 1: > 1: minor: should this project be moved to manual tests hierarchy? BTW, I am planning to create a monkey tester application which will include many tests, to simplify the testing/verification of fixes JDK-8299335 This code be added to the tester, either as a separate page, or as a data source option for one of the TreeTableView page. - PR: https://git.openjdk.org/jfx/pull/970
Re: RFR: 8293119: Additional constrained resize policies for Tree/TableView [v29]
On Tue, 3 Jan 2023 22:56:22 GMT, Kevin Rushforth wrote: > I am seeing a new behavior, could you check the latest version please? I think the latest version works as expected, except for one scenario with fractional size, where both AUTO_RESIZE_FLEX_LAST_COLUMN and AUTO_RESIZE_FLEX_NEXT_COLUMN result in the adjustment of the columns to the left of the one being resized (data=many columns, same pref) - PR: https://git.openjdk.org/jfx/pull/897
Re: RFR: 8293119: Additional constrained resize policies for Tree/TableView [v29]
On Tue, 3 Jan 2023 19:02:20 GMT, Andy Goryachev wrote: >> The current CONSTRAINED_RESIZE_POLICY has a number of issues as explained in >> [JDK-8292810](https://bugs.openjdk.org/browse/JDK-8292810). >> >> We propose to address all these issues by replacing the old column resize >> algorithm with a different one, which not only honors all the constraints >> when resizing, but also provides 4 different resize modes similar to >> JTable's. The new implementation brings changes to the public API for >> Tree/TableView and ResizeFeaturesBase classes. Specifically: >> >> - create a public abstract javafx.scene.control.ConstrainedColumnResizeBase >> class >> - provide an out-of-the box implementation via >> javafx.scene.control.ConstrainedColumnResize class, offeting 4 resize modes: >> AUTO_RESIZE_NEXT_COLUMN, AUTO_RESIZE_SUBSEQUENT_COLUMNS, >> AUTO_RESIZE_LAST_COLUMN, AUTO_RESIZE_ALL_COLUMNS >> - add corresponding public static constants to Tree/TableView >> - make Tree/TableView.CONSTRAINED_RESIZE_POLICY an alias to >> AUTO_RESIZE_SUBSEQUENT_COLUMNS (a slight behavioral change - discuss) >> - add getContentWidth() and setColumnWidth(TableColumnBase col, double >> width) methods to ResizeFeatureBase >> - suppress the horizontal scroll bar when resize policy is instanceof >> ConstrainedColumnResizeBase >> - update javadoc >> >> >> Notes >> >> 1. The current resize policies' toString() methods return >> "unconstrained-resize" and "constrained-resize", used by the skin base to >> set a pseudostate. All constrained policies that extend >> ConstrainedColumnResizeBase will return "constrained-resize" value. >> 2. The reason an abstract class ( ConstrainedColumnResizeBase) was chosen >> instead of a marker interface is exactly for its toString() method which >> supplies "constrained-resize" value. The implementors might choose to use a >> different value, however they must ensure the stylesheet contains the same >> adjustments for the new policy as those made in modena.css for >> "constrained-resize" value. > > Andy Goryachev has updated the pull request incrementally with two additional > commits since the last revision: > > - 2023 > - 2023 The Windows HiDPI behavior (on my system with a screen scale of 1.25) is _much_ better with the latest version. With a policy of NEXT or LAST, the divider tracks the mouse cursor as I would expect. With a policy of FLEX_NEXT or FLEX_LAST, it tracks until the first (or last) column being resized hits its minimum and it switches to another column. Similarly, with a policy of SUBSEQUENT, it doesn't quite track as expected when one of the columns gets small (i.e., close to its minimum). This might just be error accumulation. It seems like a minor problem. I am seeing a new behavior, unrelated to HiDPI, that I don't recall seeing before. The "flex" policies will start out correctly taking the extra space from the next (or last) column, then unexpectedly switch to a different column, then later switch back to the next (or last) column. As far as I can tell, it only happens when the window is resized from its initial size. The easiest way to see this is: 1. Resize the window such that the width is larger 2. Resize the FX split pane making the table wider 3. Select "Data" -> "pref only" 4. Select "Policy" -> "AUTO_RESIZE_FLEX_LAST_COLUMN" 5. Drag the divider between "C1" and "C2" to the right It will start out taking the space needed to make C1 larger from C4 (as expected), then switch to one of C2 or C3 before C4 has resized to its minimum, then after a while, it will switch back to C4 until it has hit its minimum. This happens on both Mac and Windows. - PR: https://git.openjdk.org/jfx/pull/897
Re: RFR: 8209017: CheckBoxTreeCell: graphic on TreeItem not always showing
On Tue, 6 Dec 2022 20:10:16 GMT, Nir Lisker wrote: > A simple fix for the graphics of a `CheckBoxTreeCell` not showing after > expand/collapse events. The graphics node needs to be released from the > previous checkbox so that it doesn't serve as the graphics of more than one > checkbox at once. If it does, only one checkbox (its skin) can actually use > it as a scenegraph child, and it's not always the correct one. > > Added also a manual test app as I'm unsure about how to do an automatic test > for this. The test app will be useful for later fixes, so I suggest adding it > anyway. To test the fix, simply expand and collapse the root note repeatedly > while it has a child. Before the fix the graphic will disappear after a few > tries, after the fix it won't. You can test other hierarchies as well by > adding children to the tree. @hjohn @kevinrushforth Can you review this in time for RDP1? It's a simple fix. - PR: https://git.openjdk.org/jfx/pull/970
Re: CFV: New OpenJFX Committer: John Hendrikx
Vote: YES Op di 3 jan. 2023 om 16:17 schreef Kevin Rushforth < kevin.rushfo...@oracle.com>: > I hereby nominate John Hendrikx [1] to OpenJFX Committer. > > John is an OpenJFX community member, who has contributed 16 commits [2] > to OpenJFX. > > Votes are due by January 17, 2023 at 16:00 UTC. > > Only current OpenJFX Committers [3] are eligible to vote on this > nomination. Votes must be cast in the open by replying to this mailing > list. > > For Lazy Consensus voting instructions, see [4]. Nomination to a project > Committer is described in [5]. > > Thanks. > > -- Kevin > > > [1] https://openjdk.java.net/census#jhendrikx > > [2] > > https://github.com/openjdk/jfx/search?q=author-name%3A%22John+Hendrikx%22&s=author-date&type=commits > > [3] https://openjdk.java.net/census#openjfx > > [4] https://openjdk.java.net/bylaws#lazy-consensus > > [5] https://openjdk.java.net/projects#project-committer > >
Re: RFR: 8296413: Tree/TableView with null focus model throws NPE in queryAccessibleAttribute() [v2]
> - added test to ensure no exception is thrown from > Control.queryAccessibleAttribute() for all accessible attributes values > - fixed null focus model case in Tree/TableView Andy Goryachev has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 29 commits: - 8296413: 2023 - Merge remote-tracking branch 'origin/master' into 8296413.query.accessible - Merge remote-tracking branch 'origin/master' into 8296413.query.accessible - Merge branch '8187145.null.selection.model' into 8296413.query.accessible - Merge remote-tracking branch 'origin/master' into 8187145.null.selection.model - Merge branch '8187145.null.selection.model' into 8296413.query.accessible - Merge remote-tracking branch 'origin/master' into 8187145.null.selection.model - 8296413: cleanup - Merge branch '8187145.null.selection.model' into 8296413.query.accessible - Merge remote-tracking branch 'origin/master' into 8187145.null.selection.model - ... and 19 more: https://git.openjdk.org/jfx/compare/a35c3bf7...6111660b - Changes: https://git.openjdk.org/jfx/pull/938/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=938&range=01 Stats: 235 lines in 8 files changed: 181 ins; 26 del; 28 mod Patch: https://git.openjdk.org/jfx/pull/938.diff Fetch: git fetch https://git.openjdk.org/jfx pull/938/head:pull/938 PR: https://git.openjdk.org/jfx/pull/938
Re: RFR: 8296413: Tree/TableView with null focus model throws NPE in queryAccessibleAttribute() [v2]
On Mon, 2 Jan 2023 11:42:47 GMT, Ambarish Rapte wrote: >> Andy Goryachev has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 29 commits: >> >> - 8296413: 2023 >> - Merge remote-tracking branch 'origin/master' into 8296413.query.accessible >> - Merge remote-tracking branch 'origin/master' into >>8296413.query.accessible >> - Merge branch '8187145.null.selection.model' into 8296413.query.accessible >> - Merge remote-tracking branch 'origin/master' into >> 8187145.null.selection.model >> - Merge branch '8187145.null.selection.model' into 8296413.query.accessible >> - Merge remote-tracking branch 'origin/master' into >> 8187145.null.selection.model >> - 8296413: cleanup >> - Merge branch '8187145.null.selection.model' into 8296413.query.accessible >> - Merge remote-tracking branch 'origin/master' into >> 8187145.null.selection.model >> - ... and 19 more: https://git.openjdk.org/jfx/compare/a35c3bf7...6111660b > > modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/QueryAccessibleAttributeTest.java > line 2: > >> 1: /* >> 2: * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved. > > Please change year to 2023 done - PR: https://git.openjdk.org/jfx/pull/938
Re: RFR: 8293119: Additional constrained resize policies for Tree/TableView [v29]
> The current CONSTRAINED_RESIZE_POLICY has a number of issues as explained in > [JDK-8292810](https://bugs.openjdk.org/browse/JDK-8292810). > > We propose to address all these issues by replacing the old column resize > algorithm with a different one, which not only honors all the constraints > when resizing, but also provides 4 different resize modes similar to > JTable's. The new implementation brings changes to the public API for > Tree/TableView and ResizeFeaturesBase classes. Specifically: > > - create a public abstract javafx.scene.control.ConstrainedColumnResizeBase > class > - provide an out-of-the box implementation via > javafx.scene.control.ConstrainedColumnResize class, offeting 4 resize modes: > AUTO_RESIZE_NEXT_COLUMN, AUTO_RESIZE_SUBSEQUENT_COLUMNS, > AUTO_RESIZE_LAST_COLUMN, AUTO_RESIZE_ALL_COLUMNS > - add corresponding public static constants to Tree/TableView > - make Tree/TableView.CONSTRAINED_RESIZE_POLICY an alias to > AUTO_RESIZE_SUBSEQUENT_COLUMNS (a slight behavioral change - discuss) > - add getContentWidth() and setColumnWidth(TableColumnBase col, double > width) methods to ResizeFeatureBase > - suppress the horizontal scroll bar when resize policy is instanceof > ConstrainedColumnResizeBase > - update javadoc > > > Notes > > 1. The current resize policies' toString() methods return > "unconstrained-resize" and "constrained-resize", used by the skin base to set > a pseudostate. All constrained policies that extend > ConstrainedColumnResizeBase will return "constrained-resize" value. > 2. The reason an abstract class ( ConstrainedColumnResizeBase) was chosen > instead of a marker interface is exactly for its toString() method which > supplies "constrained-resize" value. The implementors might choose to use a > different value, however they must ensure the stylesheet contains the same > adjustments for the new policy as those made in modena.css for > "constrained-resize" value. Andy Goryachev has updated the pull request incrementally with two additional commits since the last revision: - 2023 - 2023 - Changes: - all: https://git.openjdk.org/jfx/pull/897/files - new: https://git.openjdk.org/jfx/pull/897/files/645407ef..d7741c46 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=897&range=28 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=897&range=27-28 Stats: 11 lines in 11 files changed: 0 ins; 0 del; 11 mod Patch: https://git.openjdk.org/jfx/pull/897.diff Fetch: git fetch https://git.openjdk.org/jfx pull/897/head:pull/897 PR: https://git.openjdk.org/jfx/pull/897
Re: RFR: 8293119: Additional constrained resize policies for Tree/TableView [v28]
> The current CONSTRAINED_RESIZE_POLICY has a number of issues as explained in > [JDK-8292810](https://bugs.openjdk.org/browse/JDK-8292810). > > We propose to address all these issues by replacing the old column resize > algorithm with a different one, which not only honors all the constraints > when resizing, but also provides 4 different resize modes similar to > JTable's. The new implementation brings changes to the public API for > Tree/TableView and ResizeFeaturesBase classes. Specifically: > > - create a public abstract javafx.scene.control.ConstrainedColumnResizeBase > class > - provide an out-of-the box implementation via > javafx.scene.control.ConstrainedColumnResize class, offeting 4 resize modes: > AUTO_RESIZE_NEXT_COLUMN, AUTO_RESIZE_SUBSEQUENT_COLUMNS, > AUTO_RESIZE_LAST_COLUMN, AUTO_RESIZE_ALL_COLUMNS > - add corresponding public static constants to Tree/TableView > - make Tree/TableView.CONSTRAINED_RESIZE_POLICY an alias to > AUTO_RESIZE_SUBSEQUENT_COLUMNS (a slight behavioral change - discuss) > - add getContentWidth() and setColumnWidth(TableColumnBase col, double > width) methods to ResizeFeatureBase > - suppress the horizontal scroll bar when resize policy is instanceof > ConstrainedColumnResizeBase > - update javadoc > > > Notes > > 1. The current resize policies' toString() methods return > "unconstrained-resize" and "constrained-resize", used by the skin base to set > a pseudostate. All constrained policies that extend > ConstrainedColumnResizeBase will return "constrained-resize" value. > 2. The reason an abstract class ( ConstrainedColumnResizeBase) was chosen > instead of a marker interface is exactly for its toString() method which > supplies "constrained-resize" value. The implementors might choose to use a > different value, however they must ensure the stylesheet contains the same > adjustments for the new policy as those made in modena.css for > "constrained-resize" value. Andy Goryachev has updated the pull request incrementally with one additional commit since the last revision: small delta - Changes: - all: https://git.openjdk.org/jfx/pull/897/files - new: https://git.openjdk.org/jfx/pull/897/files/9b8fd1f0..645407ef Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=897&range=27 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=897&range=26-27 Stats: 5 lines in 1 file changed: 5 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/897.diff Fetch: git fetch https://git.openjdk.org/jfx pull/897/head:pull/897 PR: https://git.openjdk.org/jfx/pull/897
Re: CFV: New OpenJFX Committer: John Hendrikx
Vote: YES Em ter., 3 de jan. de 2023 às 12:17, Kevin Rushforth < kevin.rushfo...@oracle.com> escreveu: > I hereby nominate John Hendrikx [1] to OpenJFX Committer. > > John is an OpenJFX community member, who has contributed 16 commits [2] > to OpenJFX. > > Votes are due by January 17, 2023 at 16:00 UTC. > > Only current OpenJFX Committers [3] are eligible to vote on this > nomination. Votes must be cast in the open by replying to this mailing > list. > > For Lazy Consensus voting instructions, see [4]. Nomination to a project > Committer is described in [5]. > > Thanks. > > -- Kevin > > > [1] https://openjdk.java.net/census#jhendrikx > > [2] > > https://github.com/openjdk/jfx/search?q=author-name%3A%22John+Hendrikx%22&s=author-date&type=commits > > [3] https://openjdk.java.net/census#openjfx > > [4] https://openjdk.java.net/bylaws#lazy-consensus > > [5] https://openjdk.java.net/projects#project-committer > >
Re: CFV: New OpenJFX Committer: John Hendrikx
Vote: YES On Tue, Jan 3, 2023 at 4:17 PM Kevin Rushforth wrote: > > I hereby nominate John Hendrikx [1] to OpenJFX Committer. > > John is an OpenJFX community member, who has contributed 16 commits [2] > to OpenJFX. > > Votes are due by January 17, 2023 at 16:00 UTC. > > Only current OpenJFX Committers [3] are eligible to vote on this > nomination. Votes must be cast in the open by replying to this mailing list. > > For Lazy Consensus voting instructions, see [4]. Nomination to a project > Committer is described in [5]. > > Thanks. > > -- Kevin > > > [1] https://openjdk.java.net/census#jhendrikx > > [2] > https://github.com/openjdk/jfx/search?q=author-name%3A%22John+Hendrikx%22&s=author-date&type=commits > > [3] https://openjdk.java.net/census#openjfx > > [4] https://openjdk.java.net/bylaws#lazy-consensus > > [5] https://openjdk.java.net/projects#project-committer >
Re: CFV: New OpenJFX Committer: John Hendrikx
Vote: YES -andy From: openjfx-dev on behalf of Kevin Rushforth Date: Tuesday, 2023/01/03 at 07:17 To: John Hendrikx , openjfx-dev Subject: CFV: New OpenJFX Committer: John Hendrikx I hereby nominate John Hendrikx [1] to OpenJFX Committer. John is an OpenJFX community member, who has contributed 16 commits [2] to OpenJFX. Votes are due by January 17, 2023 at 16:00 UTC. Only current OpenJFX Committers [3] are eligible to vote on this nomination. Votes must be cast in the open by replying to this mailing list. For Lazy Consensus voting instructions, see [4]. Nomination to a project Committer is described in [5]. Thanks. -- Kevin [1] https://openjdk.java.net/census#jhendrikx [2] https://github.com/openjdk/jfx/search?q=author-name%3A%22John+Hendrikx%22&s=author-date&type=commits [3] https://openjdk.java.net/census#openjfx [4] https://openjdk.java.net/bylaws#lazy-consensus [5] https://openjdk.java.net/projects#project-committer
Re: CFV: New OpenJFX Committer: John Hendrikx
Vote: Yes Regards, Ajit > On 03-Jan-2023, at 8:47 PM, Kevin Rushforth > wrote: > > I hereby nominate John Hendrikx [1] to OpenJFX Committer. > > John is an OpenJFX community member, who has contributed 16 commits [2] to > OpenJFX. > > Votes are due by January 17, 2023 at 16:00 UTC. > > Only current OpenJFX Committers [3] are eligible to vote on this nomination. > Votes must be cast in the open by replying to this mailing list. > > For Lazy Consensus voting instructions, see [4]. Nomination to a project > Committer is described in [5]. > > Thanks. > > -- Kevin > > > [1] https://openjdk.java.net/census#jhendrikx > > [2] > https://github.com/openjdk/jfx/search?q=author-name%3A%22John+Hendrikx%22&s=author-date&type=commits > > [3] https://openjdk.java.net/census#openjfx > > [4] https://openjdk.java.net/bylaws#lazy-consensus > > [5] https://openjdk.java.net/projects#project-committer >
Re: CFV: New OpenJFX Committer: John Hendrikx
Vote: YES On 1/3/23 7:17 AM, Kevin Rushforth wrote: I hereby nominate John Hendrikx [1] to OpenJFX Committer.
Re: CFV: New OpenJFX Committer: John Hendrikx
Vote: YES On Tue, Jan 3, 2023 at 5:18 PM Kevin Rushforth wrote: > Vote: YES > > On 1/3/2023 7:17 AM, Kevin Rushforth wrote: > > I hereby nominate John Hendrikx [1] to OpenJFX Committer. > >
Re: CFV: New OpenJFX Committer: John Hendrikx
Vote: YES On 1/3/2023 7:17 AM, Kevin Rushforth wrote: I hereby nominate John Hendrikx [1] to OpenJFX Committer.
CFV: New OpenJFX Committer: John Hendrikx
I hereby nominate John Hendrikx [1] to OpenJFX Committer. John is an OpenJFX community member, who has contributed 16 commits [2] to OpenJFX. Votes are due by January 17, 2023 at 16:00 UTC. Only current OpenJFX Committers [3] are eligible to vote on this nomination. Votes must be cast in the open by replying to this mailing list. For Lazy Consensus voting instructions, see [4]. Nomination to a project Committer is described in [5]. Thanks. -- Kevin [1] https://openjdk.java.net/census#jhendrikx [2] https://github.com/openjdk/jfx/search?q=author-name%3A%22John+Hendrikx%22&s=author-date&type=commits [3] https://openjdk.java.net/census#openjfx [4] https://openjdk.java.net/bylaws#lazy-consensus [5] https://openjdk.java.net/projects#project-committer
Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages [v2]
On Tue, 3 Jan 2023 00:24:25 GMT, Nir Lisker wrote:
>> What would go wrong if they're not the same type? `ListContentBinding`
>> doesn't (and can't) enforce it and doesn't care either way. The whole
>> function fails silently if types don't match. Also `ListContentBinding` is
>> a private class and so I'd expect this code to be aware of how it works and
>> what is/isn't safe to do.
>>
>> I personally think this entire class is unfinished. It fails miserably in
>> edge cases without so much as a warning to the user. Take this for example:
>>
>> public static void main(String[] args) {
>> ObservableList a = FXCollections.observableArrayList();
>> ObservableList b = FXCollections.observableArrayList();
>> Bindings.bindContentBidirectional(a, b);
>> Bindings.bindContentBidirectional(a, b);
>> a.add("A");
>> System.out.println(a + " : " + b);
>> }
>>
>> Prints:
>>
>> [A, A, A, A] : [A, A, A]
>>
>> No mention about this in the API docs at all. It breaks even worse when you
>> make circular bindings `[a,b], [b,c], [c,a]` (stack traces get logged to the
>> console complaining about `IndexOutOfBoundsException`).
>>
>> I've created a solution that rejects double bindings and circular bindings,
>> but it's a bit out of scope for this. I think however that it is worth
>> adding, and considering that the current behavior is broken when doing any
>> of such things, not a big API break if instead we throw an exception.
>
> You are right, nothing would go wrong.
>
> I agree that the behavior is undesired and should be fixed in another issue.
> I was thinking of adding more specific overloads that make sense to the
> public API and deprecating the method that takes everything, making it throw.
Adding more specific unbind functions may be a good idea, but I don't think we
can change `unbind` to throw :)
I've added concept code here (https://github.com/openjdk/jfx/pull/988) that
rejects situations that would lead to lists becoming unsynced or behave badly
in other ways.
-
PR: https://git.openjdk.org/jfx/pull/972
Re: RFR: 8298728: Cells in VirtualFlow jump after resizing [v2]
On Tue, 20 Dec 2022 17:49:13 GMT, Johan Vos wrote:
>> When recalculating sizes, we often don't want the current index and/or
>> offset to change.
>>
>> Allow to fix the index/offset when doing recalculations.
>>
>> Fix JDK-8298728
>
> Johan Vos has updated the pull request incrementally with one additional
> commit since the last revision:
>
> move statements to new lines.
> Add another failing test, and a fix: when the cell that is positioned at
> the "current index"
> is resized, we also need to modify the offset (wich is calculated from the
> top of that cell
> to the start of the viewport).
Overall fix looks good to me.
I have identified two very minor typos.
modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java
line 2329:
> 2327: getOrCreateCellSize(index - 1);
> 2328: }
> 2329: if (index < getCellCount() -1) {
Minor : Need a space after `-`
modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/VirtualFlowTest.java
line 1473:
> 1471:
> 1472:
> 1473: for (int i =0 ; i < heights.length; i++) {
Minor : Need a space after `=`
-
PR: https://git.openjdk.org/jfx/pull/974
Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages [v2]
On Tue, 3 Jan 2023 00:26:21 GMT, Nir Lisker wrote: >> Yes, looks like this is quite broken. This wouldn't have gone unnoticed so >> long if unbind would just throw an exception when nothing could be unbound; >> silently failing is never a good idea. > > Can you file an issue for this if it's not filed already? I've filed https://bugs.openjdk.org/browse/JDK-8299521 - PR: https://git.openjdk.org/jfx/pull/972
Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages [v2]
On Wed, 28 Dec 2022 20:16:51 GMT, Nir Lisker wrote:
>> John Hendrikx has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Clean up expression classes repeated null checks
>> - Use instanceof with pattern matching in a few spots
>
> modules/javafx.base/src/main/java/com/sun/javafx/collections/MappingChange.java
> line 38:
>
>> 36: private List removed;
>> 37:
>> 38: private static final Map NOOP_MAP = new Map<>() {
>
> I think that we can do better with a bit of refactoring. The `Map` interface
> here is just `java.util.Function`. We can get rid of it and use `Function`
> instead. The `map` method call in `getRemoved` will be replaced with `apply`.
> The call sites needs just a little adjustment:
> * In `TableView::TableViewArrayListSelectionModel` the `cellToIndicesMap`
> needs to change its type to `Function`, but it is also unused (didn't look
> what it was supposed to be for), so no issue there.
> * In `TableView`, the method `fireCustomSelectedCellsListChangeEvent` needs
> to change its 2nd argument in the `MappingChange` constructor to
> `Function.indentity()` or something like `f -> f`.
> * Same changes for `TreeTableView`.
>
> I think that we can also use JavaFX's `Callback`, though I never use it if I
> can use `Function`.
Changed as suggested; I removed the unused fields.
-
PR: https://git.openjdk.org/jfx/pull/972
Re: RFR: 8243115: Spurious invalidations due to bug in IntegerBinding and other classes
On Tue, 3 Jan 2023 09:46:51 GMT, Ambarish Rapte wrote: > Looks good to me. Tested on Windows10 and verified that not setting > `observer` to `null` does not lead to any leak. Please merge with latest > master to trigger a GitHub build and test. Thanks, I've merged in master. > Under a different bug, should we implement the `dispose()` method? Track all > observables in a Weak list and remove `observer` from them in `dispose()` Perhaps, currently only more specific implementations (provided mainly by `Bindings`) do this. I think it was left up to the subclass to decide whether this would be worth it in order to keep bindings as light weight as possible. `dispose` certainly makes no promises in that regard (it basically makes no promises at all after reading the docs). - PR: https://git.openjdk.org/jfx/pull/198
Re: RFR: 8243115: Spurious invalidations due to bug in IntegerBinding and other classes
On Tue, 12 May 2020 22:41:14 GMT, Nir Lisker wrote:
> > I'm fine with doing a fix, but I need to know which one. Avoiding NPE's and
> > silently doing nothing is IMHO not very desirable as this will give the
> > user of the API no feedback that something went wrong.
> > So I would prefer to fix this by documenting that these cases will result
> > in a NPE.
> > The `bind` method has a similar issue -- it doesn't check its array
> > elements for `null`, and will throw a NPE when attempting to add a listener
> > to `null`. Again, I would just document the NPE so what is clearly a
> > mistake doesn't go unnoticed.
>
> I think that checking the array elements doesn't help much because by the
> time you can check them they will already be used, and that will throw an NPE
> implicitly.
(I must have missed this comment)
What I meant here was to document this behavior, not to add a null check. So
for bind:
/**
* Start observing the dependencies for changes. If the value of one of the
* dependencies changes, the binding is marked as invalid.
*
* @param dependencies
*the dependencies to observe
+* @throws NullPointerException when dependencies is null, or any of its
elements was null
*/
protected final void bind(Observable... dependencies) {
- if ((dependencies != null) && (dependencies.length > 0)) {
+ if (dependencies.length > 0) {
if (observer == null) {
observer = new BindingHelperObserver(this);
}
for (final Observable dep : dependencies) {
dep.addListener(observer);
}
}
}
And if you want to be even more specific, we could add that when a NPE is
thrown, the result is undefined (as some dependencies may have been added
already). I don't think we want to specify that case.
> `bind` is no-op for `null` or 0-length arrays, but should have probably throw
> an NPE on the `null` case. The 0-length check saves creating the `observer`,
> so I think it makes sense. `unbind` should probably only throw an NPE on
> `null` arrays, but that happens implicitly anyway, so maybe there is no
> change needed unless it's for clarity because we can add a custom error
> message.
I don't think we should throw a specific exception (or add a message), only
documentation. IMHO, passing `null` anywhere in any form, is always incorrect
and doesn't require further explanation unless explicitly allowed in the
documentation.
-
PR: https://git.openjdk.org/jfx/pull/198
Re: RFR: 8243115: Spurious invalidations due to bug in IntegerBinding and other classes [v2]
> This fixes a bug where the first call to unbind would clear the internal > invalidation listener used, resulting in subsequent unbind calls to be > no-ops, unless bind was called again first. > > I had to rewrite the parameterized test slightly as Parameterized will only > call the parameters method once, and my new test modifies the internal state > of the bindings used as parameters (by doing some unbind calls) which was > making other tests fail. John Hendrikx 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 two additional commits since the last revision: - Merge branch 'master' of https://git.openjdk.org/jfx into feature/unbind - 8243115: Unregister bindings when unbind called multiple times This fixes a bug where the first call to unbind would clear the internal invalidation listener used, resulting in subsequent unbind calls to be no-ops, unless bind was called again first. - Changes: - all: https://git.openjdk.org/jfx/pull/198/files - new: https://git.openjdk.org/jfx/pull/198/files/e3095db3..011017b7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=198&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=198&range=00-01 Stats: 2030609 lines in 18328 files changed: 1087981 ins; 691043 del; 251585 mod Patch: https://git.openjdk.org/jfx/pull/198.diff Fetch: git fetch https://git.openjdk.org/jfx pull/198/head:pull/198 PR: https://git.openjdk.org/jfx/pull/198
Re: RFR: 8243115: Spurious invalidations due to bug in IntegerBinding and other classes
On Mon, 27 Apr 2020 11:43:28 GMT, John Hendrikx wrote: > This fixes a bug where the first call to unbind would clear the internal > invalidation listener used, resulting in subsequent unbind calls to be > no-ops, unless bind was called again first. > > I had to rewrite the parameterized test slightly as Parameterized will only > call the parameters method once, and my new test modifies the internal state > of the bindings used as parameters (by doing some unbind calls) which was > making other tests fail. Looks good to me. Tested on Windows10 and verified that not setting `observer` to `null` does not lead to any leak. Please merge with latest master to trigger a GitHub build and test. Under a different bug, should we implement the `dispose()` method? Track all observables in a Weak list and remove `observer` from them in `dispose()` - PR: https://git.openjdk.org/jfx/pull/198
Re: RFR: 8290310: ChangeListener events are incorrect or misleading when a nested change occurs [v2]
> This contains the following: > - Nested changes or invalidations using ExpressionHelper are delayed until > the current emission completes > - This fixes odd change events being produced (with incorrect oldValue) > - Also fixes a bug in ExpressionHelper where a nested change would unlock > the listener list early, which could cause a > `ConcurrentModificationException` if a nested change was combined with a > remove/add listener call > - A test for ExpressionHelper to verify the new behavior > - A test for all *Property and *Binding classes that verifies correct > listener behavior at the API level (this tests gets 85% coverage on > ExpressionHelper on its own, the only thing it is not testing is the locking > behavior, which is not relevant at the API level). > - A fix for `WebColorFieldSkin` which triggered a nested change which used a > flag to prevent an event loop (I've changed it now to match how > `DoubleFieldSkin` and `IntegerFieldSkin` do it John Hendrikx has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains two commits: - Merge branch 'master' of https://git.openjdk.org/jfx into feature/delayed-nested-emission - Delay nested event emission - Changes: https://git.openjdk.org/jfx/pull/837/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=837&range=01 Stats: 505 lines in 4 files changed: 466 ins; 12 del; 27 mod Patch: https://git.openjdk.org/jfx/pull/837.diff Fetch: git fetch https://git.openjdk.org/jfx pull/837/head:pull/837 PR: https://git.openjdk.org/jfx/pull/837
