On Mon, 10 Jul 2023 00:17:44 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> Okay, that makes sense for the InvalidationListener. >> >> However, for the ChangeListener, as I see it, this PR gives two options, >> over the same listener: you can care only about the newValue, or you can >> care about both oldValue and newValue, both from the observableValue. These >> are then passed to the consumer/biConsumer that defines the ChangeListener, >> but I don't see any reason why this consumer/biconsumer needs to get >> initialized with any value at all, does it? >> >> Only bindings apply the initial value: when you bind two properties >> (unidirectionally), one property will get any future values of the other, >> starting from the initial value. But I don't see that happening to >> changeListeners: when you add a changeListener to a property, I don't see >> the action being invoked with the current value of the property at that >> moment, only when the property does change from the current value. >> >> In fact, if you run this test: >> >> >> private final StringProperty value = new SimpleStringProperty("Initial"); >> >> @Test >> void >> subscribeConsumerShouldCallSubscriberImmediatelyAndAfterEachChange() { >> value.addListener((obs, ov, nv) -> >> System.out.println("changeListener, nv: " + nv)); >> value.subscribe(nv -> System.out.println("consumer, nv = " + nv)); >> value.subscribe((ov, nv) -> System.out.println("biconsumer, nv = " + >> nv)); >> value.set("A"); >> } >> >> >> you'll get: >> >> >> consumer, nv = Initial >> changeListener, nv: A >> consumer, nv = A >> biconsumer, nv = A >> >> >> It feels wrong to see the first printout to me. >> >> My point is that I don't see why we need to call the consumer at all before >> there is any actual change in the property. > > I think it would be better to see value listeners as the listener alternative > to what `bind` offers, except not limited to properties. The main purpose of > these subscriptions is to keep things in sync, not so much to know when > something changed. When you can use `bind` that would be ideal, but not all > properties can be bound, and not everything you may want to keep in sync is a > property. `SelectionModel` is a good example; its properties can't be bound, > but it does have methods to modify the selection: > > myModelSelection.subscribe(selectionModel::select); // sync right away > thanks to initial call > > The same goes for other state that you may want to sync, like the example I > gave in another reply: > > > node.sceneProperty() > .flatMap(Scene::windowProperty) > .flatMap(Window::showingProperty) > .orElse(false) > .subscribe(this::subscribeOrUnsubscribeDependingOnVisibility); > > It's much nicer if this gets called immediately, to avoid having to duplicate > this code to register the initial subscribers (and this code may need to > check the visibility first still... was this UI part added to an existing > visible UI? Then register immediately, if it wasn't then do nothing and let > the subscriber above handle it... ) > > I think calling subscribers immediately when it is possible and makes sense > to do so should be the default. For change subscribers it makes no sense as > there is no change to report; for invalidation subscribers, you could again > make the case that it may be good to call them immediately if the property > isn't currently valid, if it weren't for the fact that `ExpressionHelper` > will make a property always valid when you register an `InvalidationListener` > (undocumented) -- and since it will always be valid immediately after > subscription, there is no need to do that initial call. (The > `ExpressionHelper` making the property valid when a listener gets added may > even have been a quick hack, as it will unnecessarily make something valid, > which may result in **all** invalidation listeners getting called down the > line, while the intention maybe was to only call the newly registered one if > the property was currently invalid... too late to change this now though > without breaking lots of code.) > > So I realize this may seem inconsistent, but each subscriber type IMHO can > stand on its own and can have a different purpose and different semantics, > and I think that in the case of a value subscriber, there is a lot more > benefit from calling the subscriber immediately than th... Thanks for the clarifications. If I get it right, you talk about `invalidation subscriber` for `subscribe(Runnable)` API, `value subscriber` for `subscribe(Consumer)` API and `change subscriber` for `subscribe(BiConsumer)` API, and that's fine, but for a developer that approaches this new API, there is no indication of that (API naming, javadoc), so that's why I'm a little bit concerned about this different in behaviour of the value vs change subscribers. As commented before, probably the javadoc for the three `subscribe` methods should be extended a little bit more with a mention of the listener involved, and with a small sample. ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1069#discussion_r1257872604