On Tue, 14 Feb 2023 14:35:23 GMT, Ambarish Rapte <ara...@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
