Re: RFR: 8294809: ListenerHelper for managing and disconnecting listeners [v8]
On Fri, 4 Nov 2022 19:04:03 GMT, Kevin Rushforth wrote: > we would need more descriptive names for `ChLi`, `MaChLi`, and so forth > (presuming they need to be exposed). they don't need to be exposed, will convert them to `private`. - PR: https://git.openjdk.org/jfx/pull/908
Re: RFR: 8294809: ListenerHelper for managing and disconnecting listeners [v8]
On Mon, 31 Oct 2022 16:45:51 GMT, Andy Goryachev wrote: >> Introduction >> >> There is a number of places where various listeners (strong as well as weak) >> are added, to be later disconnected in one go. For example, Skin >> implementations use dispose() method to clean up the listeners installed in >> the corresponding Control (sometimes using >> LambdaMultiplePropertyChangeListenerHandler class). >> >> This pattern is also used by app developers, but there is no public utility >> class to do that, so every one must invent their own, see for instance >> https://github.com/andy-goryachev/FxTextEditor/blob/master/src/goryachev/fx/FxDisconnector.java >> >> Proposal >> >> It might be beneficial to create a class (ListenerHelper, feel free to >> suggest a better name) which: >> >> - provides convenient methods like addChangeListener, >> addInvalidationListener, addWeakChangeListener, etc. >> - keeps track of the listeners and the corresponding ObservableValues >> - provides a single disconnect() method to remove all the listeners in one >> go. >> - optionally, it should be possible to add a lambda (Runnable) to a group of >> properties >> - optionally, there should be a boolean flag to fire the lambda immediately >> - strongly suggest implementing an IDisconnectable functional interface with >> a single disconnect() method >> >> Make it Public Later >> >> Initially, I think we could place the new class in package >> com.sun.javafx.scene.control; to be made publicly accessible later. >> >> Where It Will Be Useful >> >> [JDK-8294589](https://bugs.openjdk.org/browse/JDK-8294589) "MenuBarSkin: >> memory leak when changing skin" >> and other skins, as a replacement for >> LambdaMultiplePropertyChangeListenerHandler. >> >> https://github.com/openjdk/jfx/pull/908#:~:text=19%20hours%20ago-,8295175%3A%20SplitPaneSkinSkin%3A%20memory%20leak%20when%20changing%20skin%20%23911,-Draft >> >> https://github.com/openjdk/jfx/pull/914 > > Andy Goryachev has updated the pull request incrementally with one additional > commit since the last revision: > > 8294809: whitespace I fully agree with the need for a wider discussion. Any suggestions for changing of names etc. are always welcomed. The use case for a public ListenerHelper is to provide a convenient way to disconnect any listeners when a certain application-level UI (panel, window) is closed and gets discarded. For instance, a financial application might have a short-lived popup with a chart, multiple subscriptions to real time data, and so on; and there is a single moment when this popup gets closed and discarded. In this situation, it is better to disconnect all data subscriptions as well as all listeners that has been added to various application components, at that exact moment. For this PR, the goal is not to extend the public API surface and keep things as implementation detail. ChLi and other can be made package private or private. - PR: https://git.openjdk.org/jfx/pull/908
Re: RFR: 8294809: ListenerHelper for managing and disconnecting listeners [v8]
On Fri, 4 Nov 2022 17:58:16 GMT, Kevin Rushforth wrote:
>> Andy Goryachev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8294809: whitespace
>
> modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/IDisconnectable.java
> line 35:
>
>> 33: */
>> 34: @FunctionalInterface
>> 35: public interface IDisconnectable {
>
> We don't use the pattern of naming interfaces with a leading `I` in JavaFX
> (or in the JDK), but since this isn't public API, you can leave it for now.
leaving as is for now.
-
PR: https://git.openjdk.org/jfx/pull/908
Re: RFR: 8294809: ListenerHelper for managing and disconnecting listeners [v8]
On Mon, 31 Oct 2022 16:45:51 GMT, Andy Goryachev wrote: >> Introduction >> >> There is a number of places where various listeners (strong as well as weak) >> are added, to be later disconnected in one go. For example, Skin >> implementations use dispose() method to clean up the listeners installed in >> the corresponding Control (sometimes using >> LambdaMultiplePropertyChangeListenerHandler class). >> >> This pattern is also used by app developers, but there is no public utility >> class to do that, so every one must invent their own, see for instance >> https://github.com/andy-goryachev/FxTextEditor/blob/master/src/goryachev/fx/FxDisconnector.java >> >> Proposal >> >> It might be beneficial to create a class (ListenerHelper, feel free to >> suggest a better name) which: >> >> - provides convenient methods like addChangeListener, >> addInvalidationListener, addWeakChangeListener, etc. >> - keeps track of the listeners and the corresponding ObservableValues >> - provides a single disconnect() method to remove all the listeners in one >> go. >> - optionally, it should be possible to add a lambda (Runnable) to a group of >> properties >> - optionally, there should be a boolean flag to fire the lambda immediately >> - strongly suggest implementing an IDisconnectable functional interface with >> a single disconnect() method >> >> Make it Public Later >> >> Initially, I think we could place the new class in package >> com.sun.javafx.scene.control; to be made publicly accessible later. >> >> Where It Will Be Useful >> >> [JDK-8294589](https://bugs.openjdk.org/browse/JDK-8294589) "MenuBarSkin: >> memory leak when changing skin" >> and other skins, as a replacement for >> LambdaMultiplePropertyChangeListenerHandler. >> >> https://github.com/openjdk/jfx/pull/908#:~:text=19%20hours%20ago-,8295175%3A%20SplitPaneSkinSkin%3A%20memory%20leak%20when%20changing%20skin%20%23911,-Draft >> >> https://github.com/openjdk/jfx/pull/914 > > Andy Goryachev has updated the pull request incrementally with one additional > commit since the last revision: > > 8294809: whitespace @arapte and @aghaisas Can you also review this? It really needs to be done in connection with one of the existing Draft PRs that will take advantage of it, such as PR #925 , since the purpose is to make it easier for skins to avoid memory leaks. - PR: https://git.openjdk.org/jfx/pull/908
Re: RFR: 8294809: ListenerHelper for managing and disconnecting listeners [v8]
On Mon, 31 Oct 2022 16:45:51 GMT, Andy Goryachev wrote:
>> Introduction
>>
>> There is a number of places where various listeners (strong as well as weak)
>> are added, to be later disconnected in one go. For example, Skin
>> implementations use dispose() method to clean up the listeners installed in
>> the corresponding Control (sometimes using
>> LambdaMultiplePropertyChangeListenerHandler class).
>>
>> This pattern is also used by app developers, but there is no public utility
>> class to do that, so every one must invent their own, see for instance
>> https://github.com/andy-goryachev/FxTextEditor/blob/master/src/goryachev/fx/FxDisconnector.java
>>
>> Proposal
>>
>> It might be beneficial to create a class (ListenerHelper, feel free to
>> suggest a better name) which:
>>
>> - provides convenient methods like addChangeListener,
>> addInvalidationListener, addWeakChangeListener, etc.
>> - keeps track of the listeners and the corresponding ObservableValues
>> - provides a single disconnect() method to remove all the listeners in one
>> go.
>> - optionally, it should be possible to add a lambda (Runnable) to a group of
>> properties
>> - optionally, there should be a boolean flag to fire the lambda immediately
>> - strongly suggest implementing an IDisconnectable functional interface with
>> a single disconnect() method
>>
>> Make it Public Later
>>
>> Initially, I think we could place the new class in package
>> com.sun.javafx.scene.control; to be made publicly accessible later.
>>
>> Where It Will Be Useful
>>
>> [JDK-8294589](https://bugs.openjdk.org/browse/JDK-8294589) "MenuBarSkin:
>> memory leak when changing skin"
>> and other skins, as a replacement for
>> LambdaMultiplePropertyChangeListenerHandler.
>>
>> https://github.com/openjdk/jfx/pull/908#:~:text=19%20hours%20ago-,8295175%3A%20SplitPaneSkinSkin%3A%20memory%20leak%20when%20changing%20skin%20%23911,-Draft
>>
>> https://github.com/openjdk/jfx/pull/914
>
> Andy Goryachev has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8294809: whitespace
Before reviewing the code itself, I have a couple global comments and a small
number of inline comments.
Since `ListenerHelper` is now an internal implementation detail, we can defer
the longer discussion of whether and how to expose this as public API. In its
current incarnation, it seems like a helpful utility to fix a number of skin
memory leaks, and will likely be an eventual replacement for the (also
internal) `LambdaMultiplePropertyChangeListenerHandler` utility class.
If this ever does become public API in the future, we will need a naming
discussion as part of the larger discussion of what form the API should take.
For example, we wouldn't use `IDisconnectable` as the name of a public
interface in the JavaFX API (we don't use the pattern a leading `I` for
interfaces anywhere in JavaFX or in the JDK); also, we would need more
descriptive names for `ChLi`, `MaChLi`, and so forth (presuming they need to be
exposed).
The larger discussion, though, will first need to clearly identify what the
purpose of this utility is. If it is specific to control skins, then whatever
minimal public API surface that needs to be exposed to do that job so
third-party controls could use it is what we would want. If it is to be more
general, then it doesn't belong in the `javafx.controls` module at all, but
belongs in `javafx.base`. In the either case, but especially for an API in
`javafx.base`, we need to ensure that this is generally useful to application
developers, and not just for our own internal UI controls. Also, we would need
to see how it fits with the existing APIs in `javafx.base`, and the newly
proposed APIs in PR #830. Btw, the existing implementation is not suitable to
be generic, since it references classes like `Task`, `Node`, `Scene`, `Label`,
and `TableColumn`.
As I mentioned above, we don't need to have this discussion unless and until
there is a desire to make this public API some future release.
modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/IDisconnectable.java
line 32:
> 30: * Original code is re-licensed to Oracle by the author.
> 31: *
> https://github.com/andy-goryachev/FxTextEditor/blob/master/src/goryachev/common/util/Disconnectable.java
> 32: * Copyright © 2021-2022 Andy Goryachev
Copyright and licensing information doesn't belong in the javadocs. I recommend
putting this in a separate ordinary comment block (`/*` rather than `/**`)
right above the class docs.
modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/IDisconnectable.java
line 35:
> 33: */
> 34: @FunctionalInterface
> 35: public interface IDisconnectable {
We don't use the pattern of naming interfaces with a leading `I` in JavaFX (or
in the JDK), but since this isn't public API, you can leave it for now.
Re: RFR: 8294809: ListenerHelper for managing and disconnecting listeners [v8]
> Introduction > > There is a number of places where various listeners (strong as well as weak) > are added, to be later disconnected in one go. For example, Skin > implementations use dispose() method to clean up the listeners installed in > the corresponding Control (sometimes using > LambdaMultiplePropertyChangeListenerHandler class). > > This pattern is also used by app developers, but there is no public utility > class to do that, so every one must invent their own, see for instance > https://github.com/andy-goryachev/FxTextEditor/blob/master/src/goryachev/fx/FxDisconnector.java > > Proposal > > It might be beneficial to create a class (ListenerHelper, feel free to > suggest a better name) which: > > - provides convenient methods like addChangeListener, > addInvalidationListener, addWeakChangeListener, etc. > - keeps track of the listeners and the corresponding ObservableValues > - provides a single disconnect() method to remove all the listeners in one go. > - optionally, it should be possible to add a lambda (Runnable) to a group of > properties > - optionally, there should be a boolean flag to fire the lambda immediately > - strongly suggest implementing an IDisconnectable functional interface with > a single disconnect() method > > Make it Public Later > > Initially, I think we could place the new class in package > com.sun.javafx.scene.control; to be made publicly accessible later. > > Where It Will Be Useful > > [JDK-8294589](https://bugs.openjdk.org/browse/JDK-8294589) "MenuBarSkin: > memory leak when changing skin" > and other skins, as a replacement for > LambdaMultiplePropertyChangeListenerHandler. > > https://github.com/openjdk/jfx/pull/908#:~:text=19%20hours%20ago-,8295175%3A%20SplitPaneSkinSkin%3A%20memory%20leak%20when%20changing%20skin%20%23911,-Draft > > https://github.com/openjdk/jfx/pull/914 Andy Goryachev has updated the pull request incrementally with one additional commit since the last revision: 8294809: whitespace - Changes: - all: https://git.openjdk.org/jfx/pull/908/files - new: https://git.openjdk.org/jfx/pull/908/files/e78ed6db..e0e0aabb Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=908=07 - incr: https://webrevs.openjdk.org/?repo=jfx=908=06-07 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/908.diff Fetch: git fetch https://git.openjdk.org/jfx pull/908/head:pull/908 PR: https://git.openjdk.org/jfx/pull/908
