RFR: 8290310: ChangeListener events are incorrect or misleading when a nested change occurs [v6]
John Hendrikx
jhendrikx at openjdk.org
Sun Apr 16 12:37:45 UTC 2023
On Sun, 16 Apr 2023 07:43:05 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:
>> modules/javafx.base/src/main/java/com/sun/javafx/binding/OldValueCachingListenerList.java line 101:
>>
>>> 99: * notification otherwise {@code false}
>>> 100: */
>>> 101: public boolean notifyListeners(ObservableValue<T> observableValue) {
>>
>> The code in this method is _almost_ identical to `ListenerList.notifyListeners(ObservableValue<T>, T)`.
>> Given that this method is somewhat complex, I think it would be good to use a common implementation.
>> This will help with code review, and decrease the chance that both methods diverge further with future modifications.
>
> I agree with you there, and I've been looking what would be a good way to achieve this. I will take another look soon. My primary concern is that this is a somewhat critical path, and I would want to ensure that it doesn't cause too much performance regressions (I've already been optimizing all of this code with the help of a JMH test)
While looking that code over to see if it could be merged without impacting the general case, I discovered a small bug in the OldValueCaching version. After I fixed it, the code was even more similar than it was already. The only different still is the fact that the latest value must be kept track of whenever ObservableValue#getValue is called.
I've now added an extra parameter to the generic version to allow for storing the latest value when it is queried (and not storing it if it's not needed). This seems to have a minimal performance impact only, so I think the trade off is acceptable.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1167895136
More information about the openjfx-dev
mailing list