2
\$\begingroup\$

I've been playing with an idea for simplifying management of event listeners on custom Swing components.

Swing provides the EventListenerList class for maintaining events, and every JComponent has a protected EventListenerList listenerList = new EventListenerList(); available for use by subclasses.

The thing is, I think EventListenerList is poorly designed. They tried to minimize memory usage by packing all of a component's listener types together. But it doesn't save much memory, and it reallocates the array on every add/remove, and it has a bothersome bunch of boilerplate code needed. For each event type, components using it have the following methods (using ActionListener and a hypothetical button component as an example):

/**
 * Adds an {@code ActionListener} to the button.
 * 
 * @param l the listener to be added
 */
public void addActionListener(ActionListener l) {
    listenerList.add(ActionListener.class, l);
}


/**
 * Removes an {@code ActionListener} from the button.
 * 
 * @param l the listener to be removed
 */ 
public void removeActionListener(ActionListener l) {
    listenerList.remove(ActionListener.class, l);
}


/**
 * Returns an array of all the {@code ActionListener}s added
 * to this button with {@link #addActionListener}.
 * 
 * @return all of the {@code ActionListener}s added or an empty
 *         array if no listeners have been added
 */
public ActionListener[] getActionListeners() {
    return listenerList.getListeners(ActionListener.class);
}


/**
 * Notifies all listeners that have registered interest for
 * notification on this event type.
 * 
 * @param event the {@code ActionEvent} object
 */
protected void fireActionPerformed(ActionEvent event) {
    // Guaranteed to return a non-null array
    Object[] listeners = listenerList.getListenerList();
    // Process the listeners last to first, notifying
    // those that are interested in this event
    for (int i = listeners.length-2; i>=0; i-=2) {
        if (listeners[i]==ActionListener.class) {
            ((ActionListener)listeners[i+1]).actionPerformed(event);
        }
    }
}

The boilerplate loop for the fire event method is rather ugly. The other methods are trivial, but still come with a burden of implementing the dull documentation in order to satisfy coding requirements, so you end up with about 50 lines of boilerplate crap per event type per component. It has always irritated me also, that they do not appear in order together in the javadoc because it sorts them in alphabetical order. Altogether I hate it.

My idea was to manage all of a component's listeners of a particular type with an ordinary List:

/** A list of {@code ActionListener}s registered for this component. */
public final List<ActionListener> actionListeners = newListenerList(ActionListener.class);

Where newListenerList is the following reusable utility method:

public static <E extends EventListener> List<E> newListenerList(Class<E> klass) {
    return Collections.checkedList(new ArrayList<>(), klass);
}

Users of the component would add/remove/manage listeners by:

someThing.actionListeners.add(new ActionListener() { ... });
someThing.actionListeners.remove(...);
for (ActionListener l : someThing.actionListeners) { ... }

I thought it was brilliant when I thought of it. Certainly it solves my main complaint which is the amount of code. And it allows easier management of the list. I've already begun using it, merrily tearing out the fussy old EventListenerList methods, but mulling it over over the last few days I've begun to realize cracks in the idea:

  1. I can't prevent null being accidentally added to the list, which will not throw an exception until the time of firing the event.
  2. When there are multiple listeners the order of event dispatch is reversed compared to the normal pattern.
  3. It's not safe for manipulation during iteration. If an event is dispatched this way:

    protected void fireActionPerformed(ActionEvent e) {
        for (ActionListener l : actionListeners)
            l.actionPerformed(e);
    }
    

    Then an event handler is not allowed to do this:

    class MyListener implements ActionListener {
        @Override
        public void actionPerformed(ActionEvent e) {
            // run callFoo() only the first time
            callFoo();
            someThing.actionListeners.remove(this);
        }
    }
    

    Because that will upset the Iterator in fireActionPerformed, likely resulting in lost or duplicate events or a ConcurrentModificationException. Not good!

  4. Anything else?

My questions: Do you think using a list like this is a good way of managing Swing event listeners? How would you improve it?

\$\endgroup\$
2
\$\begingroup\$

Certainly it solves my main complaint which is the amount of code.

But it raises another issue, namely the amount of Lists floating around. In the class Component, there are 11 different event types and I don't want to know how many others exist elsewhere. So you'd have to add 20 lists to every component. Note that most components have no listeners.

Even if using nullable references, it'd be 80 more bytes per component. With real lists, you add 3 objects ("checked", "List", and its array).

For your own listener type or two, it may be OK, for AWT/Swing it would be too bad.

I can't prevent null being accidentally added to the list

What's the problem? Make an own null-forbidding list or check when iterating.

When there are multiple listeners the order of event dispatch is reversed compared to the normal pattern.

The normal pattern is not guaranteed. It mostly works, but not always. You can't even rely on Swing handlers firing before yours. You could use ListIterator.prev(), but I wouldn't care (I might even try to randomize the order instead, so I get a chance to catch depending on order early).

With your own Listener, you could rely on the order, but probably you should not, as in a complex program, listeners may get added and removed and added again and whatever...

It's not safe for manipulation during iteration.

Ditch the iterator and use an index. Or write an own List.

Anything else?

The memory problem may be a problem of yours, too.

I personally would reduce the Javadoc to something like

/** @see Button.addActionListener. */

if any. Or, if you need it many times, implement an interface and document it there (no need to re-document anything in the class as it adds absolutely nothing new).

A simple helper would reduce fireActionPerformed to a single line, too.


I actually consider the idea of add/remove/get/fire rather strange. Looking at any AWT/Swing class reveals tons of such methods with no chance to quickly find what you want. Something like

something.listeners(ActionListener.class).add(myFancyActionListener);

would be longer and probably slower, but IMHO much nicer. But it's a different story and it comes 20 years too late.


"Ditch the iterator and use an index." While that would prevent throwing the CME, that just hides the problem.

No, you can do it in a consistent way, so that after the removal, all remaining listeners get iterated.

And it complicates the boilerplate, which I specifically want to avoid unless it does something really worthwhile.

You can extract the all the boilerplate into a single helper method and then maybe 10 lines are no problem.

(4) "Or write an own List." I don't know how to make that safe for modification during iteration either though.

The simplest solution would be a CopyOnWriteArrayList, which guarantees to iterate a consistent snapshot and never throw a CME.

Looking at the sources of ArrayList and just dropping the co-modification test should do, too, but then I'd suggest to implement only the needed methods rather than the whole List interface.

(5) "A simple helper would reduce fireActionPerformed to a single line" Yes but ActionListener is only one interface; I can't make a generic helper for all listener interfaces.

I see that the fireXXX events are a problem, but you could use

<L extends EventListener> List<L> getListeners(Class<L> listenerClass, Object[] listeners);

and reduce it to a simple loop. If you need fireMouseMoved often, you could write

void helpFireMouseMoved(MouseEvent e, Object[] listeners) {
    for (MouseEventListener l : getListeners(MouseMotionListener.class, listeners) {
        l.mouseMoved(e);
    }
}

You could use reflection instead and write a universal method accepting the method name, but that's ugly.

in which case it's better not to do anything too weird.

I'm afraid so.

The problems are that the listener have no common class, are not parameterized with the event type, and have multiple differently named methods instead of overriding a single one. I could imagine something like

interface MouseMovedListener extends EventListener<MouseMovedEvent e> {
    @Override react(e);
}

be the best for Java 8, but it's not exactly nice and it's too late.

\$\endgroup\$
  • 1
    \$\begingroup\$ Concerning your javadoc... are you sure you didn't mean @see? \$\endgroup\$ – Vogel612 Nov 29 '14 at 23:52
  • \$\begingroup\$ (1) I'm not really worried about the memory usage. (2) "Make an own null-forbidding list" Perhaps I will have to. (3) "Ditch the iterator and use an index." While that would prevent throwing the CME, that just hides the problem. And it complicates the boilerplate, which I specifically want to avoid unless it does something really worthwhile. (4) "Or write an own List." I don't know how to make that safe for modification during iteration either though. \$\endgroup\$ – Boann Nov 30 '14 at 23:26
  • \$\begingroup\$ (5) "A simple helper would reduce fireActionPerformed to a single line" Yes but ActionListener is only one interface; I can't make a generic helper for all listener interfaces. (6) I like your something.listeners(ActionListener.class).add idea, and it has prompted me to ponder other ideas too, but as you say it's a bit late for Swing itself. Unless there's a solution so beautifully compelling as to be worth hacking on to existing Swing classes, its use is limited to a handful of events on a few custom classes, in which case it's better not to do anything too weird. Thanks for replying btw. \$\endgroup\$ – Boann Nov 30 '14 at 23:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.