Re: RFR: 8138842: TableViewSelectionModel.selectIndices does not select index 0 [v2]
On Mon, 6 Feb 2023 06:28:39 GMT, Ajit Ghaisas wrote: >> Karthik P K has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fix first index selection issue in TreeTableView > > Looks good to me. @aghaisas please sponsor this PR. - PR: https://git.openjdk.org/jfx/pull/1018
Re: Feedback requested for infrastructure for properties that wish to delay registering listeners
On 06/02/2023 06:00, Michael Strauß wrote:
I like your proposal in general, since it may result in a performance
improvement in some cases. If we can avoid to eagerly registering
listeners that aren't needed, that's great.
Thanks. It would probably simplify some cases that now require custom
property implementations.
However, it is not an alternative to using weak listeners. Consider
the following test, which is taken from Florian's PR
(https://github.com/openjdk/jfx/pull/689):
@Test
public void testListPropertyLeak2() {
JMemoryBuddy.memoryTest(checker -> {
ObservableList list = FXCollections.observableArrayList();
ListProperty listProperty = new SimpleListProperty<>(list);
checker.setAsReferenced(list);
checker.assertCollectable(listProperty);
});
}
This test only passes with your PR because `listProperty` doesn't
register a listener on `list` (that's what you are proposing, after
all).
Yes, when creating a new instance of something (in this case
`SimpleListProperty`) there shouldn't be any weird side effects.
Registering a listener to something else in the constructor is passing
on a reference to the instance (SimpleListProperty) before it is fully
constructed, which is a bad practice. Because it is bad practice, it is
very rare to see an object creation have side effects, like it being
registered somewhere, that would block its immediate garbage collection.
A call to `new` without an assignment to a variable, will in almost all
cases result in that object being immediately eligible for GC. This is
what users expect, the only one that has a reference to the object
resulting from a `new` operator is the caller. The cases where this
doesn't happen are likely bugs or not idiomatic Java. This escapes are
problematic for various reasons (see recent discussions in amber-dev and
the new this-escape warning). In the strictly threaded environment of
JavaFX you can get away with it usually, but not always. It is still a
huge surprise for users that expect the `new` operator to be side effect
free.
This is why I applaud the effort which we're doing for Skins at the
moment, moving all listener registration to the `install` method,
preventing the instance from escaping before it is fully constructed.
It means that just constructing the Skin does not tie it to a control
already. Similarly, just constructing a ListProperty should not tie it
to an ObservableList; it doesn't matter if that tie is strong or weak.
As soon as you do that, for example by adding the following line, the
test fails:
listProperty.addListener((ListChangeListener) change ->
{});
But the test shouldn't ever fail, because the ObservableList that is
wrapped by ListProperty should never keep a strong reference to the
ListProperty, even if the ListProperty is itself observed.
Why should it work that way? The alternative is that my listener is
discarded without my knowledge. There is no warning in my IDE, in my
logs, or anywhere. It just seems to work, and then breaks at some point
in the future. The fact that it seems to work, but then doesn't is a
huge problem for debugging, testing and reproducing user problems. That
last one is nasty, because it may work on my machine; when weak
references get cleared can depend on JVM used, JVM parameters, memory
available, hardware platform, etc.. -- they're fine for low level work
like caching, but using them in a high level mechanism like a UI is
making JavaFX far more complex than it should be.
That's a bug in the current implementation, and hiding it by making it
contingent upon the presence of observers doesn't fix the underlying
issue.
I don't think it's a bug. I took an action to register a listener. It
is on me to take action to unregister it. And until that time it should
keep working.
I mean, what if Java were to decide that a `Thread` which I started is
not doing relevant work because it determined that the objects the
thread is using are not referenced by any other threads? Should it be
allowed to kill this `Thread`? And if that's allowed, should it do this
silently without warning?
What if I added an important listener like this:
permissionsListProperty.addListener(AuditingService::auditLogPermissionChanges);
Do you think JavaFX should decide that this is not relevant because only
the property still has a reference to this listener, but nothing else has?
Also, do you think it should keep working until a garbage collection
occurs, but then stop working after one occurs? When will that be?
I feel there is fundamental misunderstanding of how listeners are
normally working in Java. JavaFX is the only system that decides that a
listener is irrelevant based on how it is being referenced. Nothing
else does this. Beans don't do this (PropertyChangeListener), other UI
frameworks don't do this (Swing, AWT, SWT), event frameworks I worked
Re: RFR: 8138842: TableViewSelectionModel.selectIndices does not select index 0 [v2]
On Wed, 1 Feb 2023 06:37:21 GMT, Karthik P K wrote: >> In `selectIndices` method, zero length array is not considered while >> ignoring row number given as parameter. >> >> Updated the code to consider both null and zero length array in the >> condition before ignoring the row value given as parameter. >> >> Added unit test to validate the fix > > Karthik P K has updated the pull request incrementally with one additional > commit since the last revision: > > Fix first index selection issue in TreeTableView Looks good to me. - Marked as reviewed by aghaisas (Reviewer). PR: https://git.openjdk.org/jfx/pull/1018
Re: Feedback requested for infrastructure for properties that wish to delay registering listeners
I like your proposal in general, since it may result in a performance
improvement in some cases. If we can avoid to eagerly registering
listeners that aren't needed, that's great.
However, it is not an alternative to using weak listeners. Consider
the following test, which is taken from Florian's PR
(https://github.com/openjdk/jfx/pull/689):
@Test
public void testListPropertyLeak2() {
JMemoryBuddy.memoryTest(checker -> {
ObservableList list = FXCollections.observableArrayList();
ListProperty listProperty = new SimpleListProperty<>(list);
checker.setAsReferenced(list);
checker.assertCollectable(listProperty);
});
}
This test only passes with your PR because `listProperty` doesn't
register a listener on `list` (that's what you are proposing, after
all).
As soon as you do that, for example by adding the following line, the
test fails:
listProperty.addListener((ListChangeListener) change -> {});
But the test shouldn't ever fail, because the ObservableList that is
wrapped by ListProperty should never keep a strong reference to the
ListProperty, even if the ListProperty is itself observed.
That's a bug in the current implementation, and hiding it by making it
contingent upon the presence of observers doesn't fix the underlying
issue.
On Sun, Feb 5, 2023 at 10:46 AM John Hendrikx wrote:
>
> Any comments on this?
>
> I think having API to make it easier for both JavaFX and users to create
> listeners that can delay registration to their own dependencies. This
> would be an alternative to using weak listeners for these cases.
>
> --John
Re: RFR: 8290310: ChangeListener events are incorrect or misleading when a nested change occurs [v2]
On Tue, 3 Jan 2023 09:42:05 GMT, John Hendrikx wrote: >> 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 > The fix for the bug with nested events releasing the lock seems fine. I'm > still not sure if the behavioral change is what we want the result to be even > if it's better than what we have now. I have mentioned this in [#108 > (comment)](https://github.com/openjdk/jfx/pull/108#issuecomment-590878643). > we need to decide on a spec first, and there are several facets to it as > discussed in the mailing list link above ([#837 > (review)](https://github.com/openjdk/jfx/pull/837#pullrequestreview-1233910264)). Is there any doubt as to what the old value should be? I think this is a bug fix only, and although I think it is good to discuss the whole listener system, I think we shouldn't delay this fix too much. After all, it is a very common pattern to use the old value for unregistering listeners when listening to a property chain (like node -> scene), and if the "old property" is ever incorrect, then the `removeListener` call will silently fail (shame we can't change that any more). > I will re-summarize what I think needs to be defined before implementing a > behavior change like the one proposed here: I don't think this is a behavior change, it is a bug fix, unless we document that `ChangeListener` old value is a "best effort value" that cannot be trusted to contain anything useful when nested changes occur. Nested changes can easily occur in the huge web of properties that are sometimes woven (take the layout code for example). > ### Listener order > When listeners are registered, they are added to some collection (an array, > currently). The add/remove methods hint at an order, but don't guarantee one. > Do we specify an order, or say it's unspecified? I think it is too late to change anything significant here. Also the primary reason we've been pushing for a change here is I think the poor (remove) performance when there are many listeners. However, I think having many listeners on a single property is always an unwanted situation. The remove performance is simply the first problem you'll encounter. The next problem you'll encounter is when 10.000+ listeners need to be notified of a change... it will quickly become unworkable. Furthermore, there are data structures that have `O(1)` add(end) + remove(T) performance and `O(n)` iterate performance, while still being sequential and allowing duplicates. The remove "problem" could be solved that way without breaking existing code, but that will not speed up notification (which will be your next problem). I've mostly abandoned fixes in this area as I think it is **never** a good idea to have so many listeners that remove performance becomes an issue. So, if it were up to me, and I'm pretty convinced this is going to break quite some code out there, I'd say it should be: - listeners are called in the order they're registered - duplicates are allowed - removing a listener removes the first instance of that listener (not the last or a random one) Since there are data structures that can handle the requirements we need (we only need add-at-end, remove(T) and iteration) I think we're not locking ourselves into too much problems (the cost of such a data structure is slower get(index) and remove(index) performance, but we don't need these). > ### Event dispatch order > The order in which listeners are triggered is not necessarily the order of > the listeners above. Do we specify a dispatch order, or say it's unspecified? > If it's specified, the listener order is probably also specified. I'll consider this the same problem, with the same risks, see above. > We are also dispatching invalidation events be
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 looked at it again. I think it would be best to do the sorting-related changes separately. The rest looks good. - PR: https://git.openjdk.org/jfx/pull/972
Re: RFR: 8290310: ChangeListener events are incorrect or misleading when a nested change occurs [v2]
On Tue, 3 Jan 2023 09:42:05 GMT, John Hendrikx wrote: >> 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 modules/javafx.base/src/test/java/test/javafx/beans/ObservableValueTest.java line 1: > 1: package test.javafx.beans; You will need to add a license header for a new file. - PR: https://git.openjdk.org/jfx/pull/837
Re: RFR: 8290310: ChangeListener events are incorrect or misleading when a nested change occurs [v2]
On Tue, 3 Jan 2023 09:42:05 GMT, John Hendrikx wrote:
>> 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
The fix for the bug with nested events releasing the lock seems fine. I'm still
not sure if the behavioral change is what we want the result to be even if it's
better than what we have now. I have mentioned this in
https://github.com/openjdk/jfx/pull/108#issuecomment-590878643. we need to
decide on a spec first, and there are several facets to it as discussed in the
mailing list link above
(https://github.com/openjdk/jfx/pull/837#pullrequestreview-1233910264).
I will re-summarize what I think needs to be defined before implementing a
behavior change like the one proposed here:
### Listener order
When listeners are registered, they are added to some collection (an array,
currently). The add/remove methods hint at an order, but don't guarantee one.
Do we specify an order, or say it's unspecified?
### Event dispatch order
The order in which listeners are triggered is not necessarily the order of the
listeners above. Do we specify a dispatch order, or say it's unspecified? If
it's specified, the listener order is probably also specified.
We are also dispatching invalidation events before change events arbitrarily.
Do we dispatch the event to all listeners of one type and then to the other, or
do we want to combine them according to a joint dispatch order? We either say
that they are dispatched separately, together in some order (like
registration), or that it's unspecified.
### Nested listener modifications
If a listener is added/removed during an event dispatch, do we specify it will
affect ("be in time for") the nested event chain, the outer event chain, or
that it's unspecified?
### Nested value modifications
If a listener changes the value of its property during an event dispatch, do we
specify that the new event will trigger first, halting the current event
dispatch (depth-first approach), or that the new event waits until current one
finishes (breadth-first approach), or that it is unspecified? Do we guarantee
that when a listener is invoked it sees the new value that was set by the
latest nested change, or will it see the value that existed at trigger time, or
is it unspecified?
If listeners affect each other with nested events, then the dispatch order
matters.
---
If we answer "unspecified" to the questions above, it allows us more
implementation freedom. It will also mean that listeners should be thought of
as "lone wolves" - they are not intended to talk to each other or depend on
each other; each should do something regardless of what the others are doing
and assume its code "territory" is not affected by other listeners. The user
takes responsibility for coupling them (nested modifications).
On the other hand, no guarantees bring limited usage. The question is, what is
the intended usage?
-
PR: https://git.openjdk.org/jfx/pull/837
Re: RFR: 8173321: TableView: Click on right trough has no effect when cell height is higher than viewport height [v5]
> This is my (joachim.schr...@gmx.de) first contribution to openjfx. My > Contributor Agreement is signed but still in review. > So please be patient with an absolute beginner as contributor ... . > The work of this pull request was fully done in my spare time. > > I first filed the bug myself in 2017. I had begun working with JavaFX in 2014. > > The two changes address the two problems mentioned in JDK-8173321: > - Using a JavaFX TableView, a click on the right trough has no effect when > the cell height of the cell currently displayed is higher than viewport height > - The ScrollBar ist displayed with a minimal height. > > The changes were tested and ran well with Java 17 and the current master > branch of openjfx. JoachimSchriek has updated the pull request incrementally with one additional commit since the last revision: Delete VirtualFlow.java Revert unintentional changes to VirtualFlow - Changes: - all: https://git.openjdk.org/jfx/pull/985/files - new: https://git.openjdk.org/jfx/pull/985/files/c35424d3..e7e9bc99 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=985&range=04 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=985&range=03-04 Stats: 3392 lines in 1 file changed: 0 ins; 3392 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/985.diff Fetch: git fetch https://git.openjdk.org/jfx pull/985/head:pull/985 PR: https://git.openjdk.org/jfx/pull/985
Re: RFR: 8173321: TableView: Click on right trough has no effect when cell height is higher than viewport height [v4]
> This is my (joachim.schr...@gmx.de) first contribution to openjfx. My > Contributor Agreement is signed but still in review. > So please be patient with an absolute beginner as contributor ... . > The work of this pull request was fully done in my spare time. > > I first filed the bug myself in 2017. I had begun working with JavaFX in 2014. > > The two changes address the two problems mentioned in JDK-8173321: > - Using a JavaFX TableView, a click on the right trough has no effect when > the cell height of the cell currently displayed is higher than viewport height > - The ScrollBar ist displayed with a minimal height. > > The changes were tested and ran well with Java 17 and the current master > branch of openjfx. JoachimSchriek has updated the pull request incrementally with one additional commit since the last revision: Changes made following findings from review - Changes: - all: https://git.openjdk.org/jfx/pull/985/files - new: https://git.openjdk.org/jfx/pull/985/files/7ad99bde..c35424d3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=985&range=03 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=985&range=02-03 Stats: 83 lines in 3 files changed: 39 ins; 23 del; 21 mod Patch: https://git.openjdk.org/jfx/pull/985.diff Fetch: git fetch https://git.openjdk.org/jfx pull/985/head:pull/985 PR: https://git.openjdk.org/jfx/pull/985
Re: RFR: 8173321: TableView: Click on right trough has no effect when cell height is higher than viewport height [v3]
On Sat, 4 Feb 2023 15:27:13 GMT, Kevin Rushforth wrote: >> JoachimSchriek has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Deleted trailing whitespace > > modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java > line 2611: > >> 2609: >> 2610: lengthBar.setMax(1); >> 2611: lengthBar.setVisibleAmount( numCellsVisibleOnScreen / >> (float) cellCount); > > I'm concerned with the change to no longer use the estimated size. This will > produce very different results for tables with variable row hights. @johanvos > will very likely want to comment on this. > > Also, The cast is unnecessary. > > Minor: remove the space after the `(` I am very sorry at this point. I have made a mistake here because my changes were based on the code I had seen in 2017 and reported in JDK-8173321. At that time, the size calculation of the ScrollBar was based on the integer value of numCellsVisibleOnScreen. I will revert the changes to VirtualFlow. - PR: https://git.openjdk.org/jfx/pull/985
Re: RFR: 8173321: TableView: Click on right trough has no effect when cell height is higher than viewport height [v3]
On Sat, 4 Feb 2023 15:16:28 GMT, Kevin Rushforth wrote:
>> JoachimSchriek has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Deleted trailing whitespace
>
> modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/VirtualScrollBar.java
> line 135:
>
>> 133: /**
>> 134: * JDK-8173321: Click on trough has no effect when cell
>> height > viewport height:
>> 135: * Solution: Scroll one cell further up resp. down if only
>> one cell is shown.
>
> We generally do not refer to specific bug IDs in source code comments.
> Rather, describe the purpose of this block. Also, please spell out `resp.` --
> I can't figure out what it is supposed to mean.
I will change the comment in my next commit to:
Scroll one cell further in the direction the user has clicked if only one cell
is shown.
Otherwise, a click on the trough would have no effect when cell height >
viewport height.
> modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/VirtualScrollBar.java
> line 151:
>
>> 149: IndexedCell cell = firstVisibleCell;
>> 150: if (cell == null)
>> 151: return;
>
> Unless the if statement, including its target, is entirely on one line, you
> _must_ enclose the target of the `if` with curly braces, like this:
>
>
> if (cell == null) {
> return;
> }
>
>
> Since it fits on one line, the prior code is also acceptable:
>
>
> if (cell == null) return;
I will change this to the first alternative in my next commit.
> modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/VirtualScrollBar.java
> line 156:
>
>> 154: IndexedCell cell = lastVisibleCell;
>> 155: if (cell == null)
>> 156: return;
>
> Either revert this or enclose in curly braces.
I will change this to the first alternative in my next commit.
-
PR: https://git.openjdk.org/jfx/pull/985
Re: Feedback requested for infrastructure for properties that wish to delay registering listeners
Any comments on this?
I think having API to make it easier for both JavaFX and users to create
listeners that can delay registration to their own dependencies. This
would be an alternative to using weak listeners for these cases.
--John
On 19/01/2023 16:49, John Hendrikx wrote:
Hi list,
I've been looking into what it would take to make the design of
properties which only register listeners when needed easier to
implement with JavaFX, both for JavaFX itself as well as for the
user. This is the result of my investigation into ListProperty (and
other collection properties) and how hard it would be to change their
semantics to only register listeners when they're absolutely required
for the correct functioning of the class.
Currently, JavaFX doesn't offer the tools one would need to register
listeners in a just-in-time fashion. This is because there is no
notification mechanism available in its observable values when they
become observed or unobserved. The closest thing there currently
exists is what ObjectBinding and LazyObjectBinding offer:
From ObjectBinding:
/**
* Checks if the binding has at least one listener registered on
it. This
* is useful for subclasses which want to conserve resources when
not observed.
*
* @return {@code true} if this binding currently has one or more
* listeners registered on it, otherwise {@code false}
* @since 19
*/
protected final boolean isObserved() {
return helper != null;
}
From LazyObjectBinding:
/**
* Called after a listener was added to start observing inputs if
they're not observed already.
*/
private void updateSubscriptionAfterAdd() { ... }
/**
* Called after a listener was removed to stop observing inputs if
this was the last listener
* observing this binding.
*/
private void updateSubscriptionAfterRemove() { ... }
I'm proposing to extend the JavaFX with new methods that can be used
to stay informed about the observable's current observed status. These
methods will facilitate implementations of properties that wish to
delay registering listeners until they are themselves observed. This
can save memory, save unnecessary notifications via events and reduces
the need for weak listener constructs.
The proposal does not require any new fields, and would default to
reporting observable values as being observed. Most JavaFX observable
values should be retrofitted to support this functionality -- the ones
relying on a form of ExpressionHelper will only require minimal
changes with minimal impact.
The methods that I would like to see added are the following:
In ObservableValue:
/**
* Checks if this ObservableValue is currently observed. If
unknown or unsupported,
* {@code true} is returned.
*
* @return {@code true} if this ObservableValue currently has one
or more
* listeners registered on it, otherwise {@code false}
*/
default boolean isObserved() { return true; }
The above method is useful for debugging, but its primary use is for
complex properties which observed status is determined by not only its
direct listeners but also any child properties it may offer.
ListProperty is such an example, which offers child properties size
and empty. Its observed status would be determined like this:
helper != null || (size0 != null && size0.isObserved()) ||
(empty0 != null && empty0.isObserved())
Further, we need two protected callback methods. These should be
added in all Binding and Property Base classes (basically all classes
that implement addListener/removeListener methods). These are called
when the observed status of the property changes:
/**
* Called when this property transitions from unobserved to observed.
*/
protected void observed() {}
/**
* Called when this property transitions from observed to unobserved.
*/
protected void unobserved() {}
These methods are implemented by child properties in order to inform
the parent that their observed status has changed, which may require
the parent property to change its status as well.
When implemented, LazyObjectBinding can be simplified as some or all
of its functionality will be part of ObjectBinding (LazyObjectBinding
is not a public class at the moment). The isObserved method in
ObjectBinding would go from protected to public (it's already final).
Implementation for classes that rely on a form of ExpressionHelper is
simple; they can check if the helper is null or not (for isObserved)
and check if the nullity changed during addListener/removeListener
calls to make their call to "observed" or "unobserved". No additional
fields are required.
I've added a proof of concept here
(https://github.com/openjdk/jfx/pull/1004) where `ListPropertyBase`
was altered to use these new methods to delay its listener
registration and to remove its listener when no longer obs
Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages [v2]
On Wed, 4 Jan 2023 05:28:10 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
>
> 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.
@nlisker I think I made all modifications requested, is there anything else
that should be done here? Do you want the sort changes to be made or an issue
be filed for it?
-
PR: https://git.openjdk.org/jfx/pull/972
Re: RFR: 8290310: ChangeListener events are incorrect or misleading when a nested change occurs [v2]
On Tue, 3 Jan 2023 09:42:05 GMT, John Hendrikx wrote: >> 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 @arapte @nlisker what do you think? - PR: https://git.openjdk.org/jfx/pull/837
Re: RFR: 8290765: Remove parent disabled/treeVisible listeners [v5]
On Fri, 3 Feb 2023 23:31:24 GMT, Michael Strauß wrote: >> `Node` adds InvalidationListeners to its parent's `disabled` and >> `treeVisible` properties and calls its own `updateDisabled()` and >> `updateTreeVisible(boolean)` methods when the property values change. >> >> These listeners are not required, since `Node` can easily call the >> `updateDisabled()` and `updateTreeVisible(boolean)` methods on its children, >> saving the memory cost of maintaining listeners and bindings. > > Michael Strauß has updated the pull request incrementally with one additional > commit since the last revision: > > improved tests, changed foreach loop Marked as reviewed by jhendrikx (Committer). - PR: https://git.openjdk.org/jfx/pull/841
