On Mon, 10 Jul 2023 08:01:46 GMT, Jose Pereda <jper...@openjdk.org> wrote:
>> 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 callin... > > 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. Yes, it's true they map like that: Runnable -> invalidations, Consumer -> values, BiConsumer -> changes. I will extend the docs as this indeed is a bit unclear now (the methods were called "invalidations", "values" and "changes" before). ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1069#discussion_r1257889214
