This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.

Bug 246030 - Enhanced reflective rules for WeakListener removal
Summary: Enhanced reflective rules for WeakListener removal
Status: NEW
Alias: None
Product: utilities
Classification: Unclassified
Component: Code (show other bugs)
Version: 8.0.1
Hardware: PC Linux
: P3 normal (vote)
Assignee: Jaroslav Havlin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-07-27 09:10 UTC by elotter
Modified: 2014-10-30 22:49 UTC (History)
0 users

See Also:
Issue Type: ENHANCEMENT
Exception Reporter:


Attachments
Code with workaround (1.68 KB, text/x-java-source)
2014-08-08 14:14 UTC, Jaroslav Havlin
Details

Note You need to log in before you can comment on or make changes to this bug.
Description elotter 2014-07-27 09:10:53 UTC
For a custom XYZListener (extending EventListener), the reflective listener removal rules as in the javadoc for WeakListeners involves calling .removeXYZListener(...) if available.

What I propose is an enhancement to this behaviour, but remaining dependent on the reflective nature. Libraries such as JFreeChart has listener types and addition/removal methods that do not quite follow the convention that would make it compatible with WeakListeners.* in platform. For example, consider the 

addChangeListener(AxisChangeListener listener)

and

removeChangeListener(AxisChangeListener listener)

from

http://www.jfree.org/jfreechart/api/javadoc/org/jfree/chart/axis/Axis.html

Here the unfortunate naming makes this appear as AxisChangeListener slotted into ChangeListener spot, and NB WeakListeners won't handle this case.

What about the weak listener removal code for weak listeners also opting to call 
removeChangeListener(XYZListener listener) in this case, if the normal cases did not provide any options? This would make the removal a bit more aggressive, but make this immediately useful for examples such as the case described above.

and 

The weak listener is notified that the reference to the listener was cleared.
It tries to unregister itself from the source. This is why it needs the reference to the source for the registration. The unregistration is done using reflection, usually looking up the method remove<listenerType> of the source and calling it.

Alternatively, provide an exposed way in API to allow the user to provide the correct method to call for listener removal. Perhaps this is more generic, and will allow a call something like

WeakListeners.create(AxisChangeListener.class, axisChangeListener, Axis.this, "removeChangeListener")

from within an Axis subclass in order to ensure that the listener removal will kick in.
Comment 1 elotter 2014-07-27 09:13:39 UTC
Apologies, I may have messed up the formatting, next comment should be actual enhancement description
Comment 2 elotter 2014-07-27 09:13:50 UTC
For a custom XYZListener (extending EventListener), the reflective listener removal rules as in the javadoc for WeakListeners involves calling .removeXYZListener(...) if available.

What I propose is an enhancement to this behaviour, but remaining dependent on the reflective nature. Libraries such as JFreeChart has listener types and addition/removal methods that do not quite follow the convention that would make it compatible with WeakListeners.* in platform. For example, consider the 

addChangeListener(AxisChangeListener listener)

and

removeChangeListener(AxisChangeListener listener)

from

http://www.jfree.org/jfreechart/api/javadoc/org/jfree/chart/axis/Axis.html

Here the unfortunate naming makes this appear as AxisChangeListener slotted into ChangeListener spot, and NB WeakListeners won't handle this case.

What about the weak listener removal code for weak listeners also opting to call 
removeChangeListener(XYZListener listener) in this case, if the normal cases did not provide any options? This would make the removal a bit more aggressive, but make this immediately useful for examples such as the case described above.

Alternatively, provide an exposed way in API to allow the user to provide the correct method to call for listener removal. Perhaps this is more generic, and will allow a call something like

WeakListeners.create(AxisChangeListener.class, axisChangeListener, Axis.this, "removeChangeListener")

from within an Axis subclass in order to ensure that the listener removal will kick in.
Comment 3 Jaroslav Havlin 2014-08-08 14:14:56 UTC
Created attachment 148607 [details]
Code with workaround

> Here the unfortunate naming makes this appear as AxisChangeListener slotted 
> into ChangeListener spot, and NB WeakListeners won't handle this case.
The naming is indeed unfortunate. I'm not sure we should support it.

> What about the weak listener removal code for weak listeners also opting to
> call removeChangeListener(XYZListener listener) in this case, if the normal
> cases did not provide any options?
I'm afraid this can be quite risky in some cases, and there can be some ambiguity.

> Alternatively, provide an exposed way in API to allow the user to provide
> the correct method to call for listener removal.Perhaps this is more generic,
> and will allow a call something like WeakListeners.create(
> AxisChangeListener.class, axisChangeListener, Axis.this,
> "removeChangeListener") from within an Axis subclass in order to ensure that
> the listener removal will kick in.
This solution seems better to me.

I'm attaching example code with workaround that can be used in the current NetBeans version.
We can use a proxy object for the source that contains the correct method name for listener removal and delegates to the real source. (Note that the source object is referenced weakly in the weak listener, so the proxy should be stored somewhere so that it isn't garbage collected immediately.)
I haven't tested the code very carefully, so please use it with caution.
Comment 4 elotter 2014-10-30 22:49:11 UTC
Thanks Jaroslav, your workaround idea sounds good. To take that further and towards integration, what about introducing something like the following variation of your code:

public interface ListenerRemover<S,L extends EventListener> {
    void removeListener(S object, L listener);
}

and then adding the following to org-openide-util's WeakListeners:

public static <S,L extends EventListener> L create(Class<L> listenerClass, L listener, S source, ListenerRemover<S,L> listenerRemover) {
    ProxySource<S,L> proxySource = new ProxySource<>(source, listenerRemover);
    L weakListener = WeakListeners.create(listenerClass, listener, proxySource);
    return weakListener;
}
    
static class ProxySource<S,L extends EventListener> {
    private final ListenerRemover<S,L> remover;
    private final S source;

    public ProxySource(S source, ListenerRemover<S,L> remover) {
        this.source = source;
        this.remover = remover;
    }

    public void removeListener(L l) {
        remover.removeListener(source, l);
    }
}

which will allow the AxisChangeListener case to be generically handled using a trivial ListenerRemover<Axis,AxisChangeListener> .