RFR: 8290310: ChangeListener events are incorrect or misleading when a nested change occurs [v3]

[John Hendrikx]

On Tue, 14 Feb 2023 14:35:23 GMT, Ambarish Rapte <arapte at openjdk.org> wrote:

>> John Hendrikx has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains four commits:
>> 
>>  - Add copyright header
>>  - Merge branch 'master' of https://git.openjdk.org/jfx into feature/delayed-nested-emission
>>  - Merge branch 'master' of https://git.openjdk.org/jfx into feature/delayed-nested-emission
>>  - Delay nested event emission
>
> I did test this well and looks good to me. I tested changing the property value in more than one listener and combination of Invalidation and Change Listener. The behavior looks good and did not seem like it will cause regression(of course depends on specific scenarios)
> I think the change may need a doc update, to explain this behavior.
> 
> Coming to the broad level of discussion, I think this change can still go in and other aspects can be worked on later whenever we get to them.

@arapte and others

> I did test this well and looks good to me. I tested changing the property value in more than one listener and combination of Invalidation and Change Listener. The behavior looks good and did not seem like it will cause regression(of course depends on specific scenarios) I think the change may need a doc update, to explain this behavior.

Did you have a specific place in mind for a documentation update? I just re-read the documentation for `ObservableValue#addListener(ChangeListener)` and `ChangeListener` and the only thing I found that may need updating is this comment on `ChangeListener#changed`:

> In general, it is considered bad practice to modify the observed value in this method.

Note that JavaFX itself does this sometimes to restore values if they don't match requirements.

It's a bit of a tough requirement.  If you do any work in a `ChangeListener` that touches other properties, those may trigger more changes, and those may touch the original property again (the original change call would still be higher up on the call stack) without realizing it would be a nested change.

Perhaps it could be reworded to:

> Changing the observed value in this method will result in all listeners being notified of this latest change after the initial change notification (with the original old and values) has completed. The listeners that still needed to be notified may see a new value that differs from a call to {@link ObservableValue#get}. All listeners are then notified again with an old value equal to the initial new value, and a new value with the latest value.

I added it in already.  If there are any other places where you think documentation should be update, please let me know.

> modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/skin/WebColorFieldSkin.java line 112:
> 
>> 110: 
>> 111:                 if (!newText.equals(text)) {
>> 112:                     getTextField().setText(newText);
> 
> I would recommend to keep only listener specific change in this PR. If the pattern matching can be skipped please revert those. I would be glad to review in a separate PR.

Removed

-------------

PR: https://git.openjdk.org/jfx/pull/837