Created attachment 38644 [details]
Proposed change

Generally looks good. Do you plan to update any of the numerous existing classes
which could use it? 


[JG01] I think just a public constructor would be more straightforward than a
factory method. There are good reasons to use factory methods, but I don't think
any of them apply here.


[JG02] Does the fireChange(ChangeEvent) method need to be public? I can't think
of any situation where I would call it instead of simply fireChange().


[JG03] Is there any particular reason why null is accepted for a listener in
add/removeChangeListener? I can't think of any situation where that would not
indicate a bug in some other code.


[JG04] The getChangeListeners() method does not look thread-safe. If addCL is
called between the call to size() and toArray(...) then no big deal - the
provided array will be discarded and a different one created. But if removeCL is
called in between, you may be passing a larger array than needed, which means it
may contain nulls.

Also I am unsure what the intended use case for this method is. I cannot think
of any code I have written which would ever call it. Perhaps a boolean
hasChangeListeners() would be used in a few cases.

JG01: I don't see a reason to use a factory method either, but I would use it
even if only because it's more flexible than the constructor. It is also quite
visible in the Javadoc, so I wouldn't be afraid that users won't find it.
Actually for me the strongest reason to use a constructor would be consistency
with PropertyChangeSupport.

JG02: No, I was just attempting to be consistent with PropertyChangeSupport. No
strong opinion, probably it can be made private. I just noticed some code in
j2ee which could use it -- it passes received ChangeEvent's to its own
registered listeners, but that is probably wrong, as those listeners will
receive a ChangeEvent from a source other that the one they have registered to.

JG03: Consistency with PCS again (and do I wonder why that class allows nulls).
No strong opinion, but I wonder how implementations of addChangeListener() and
fireChange() through the code base handle nulls (although the general pattern
seems to be to skip any checks for null in both aCL() and fireChange(), so any
nulls passed to aCL() should have already resulted in NPEs).

JG04: Ah yes, thanks for catching it. You have probably already guessed the
reason for this method -- yes, PCS. I also have a test where I need to check the
number of listeners, and this method seemed more flexible than
getListenerCount(). However, I just realized I am testing for zero in that test
anyway, so I will replace this method with hasListeners().

As for updating existing code, I plan to change a few usages in the Java EE
area, but I'm not comfortable with changing other people's code without being
asked for it. I could e.g. do projects and ant/project if you agree.

Y01 I do not want to treat openide/util as place that can contain everything. 
It is one of the modules that is on classpath - e.g. visible to every other 
module's classloader and I really thing it should be kept as small as 
possible. Preferably create core/util and dump these utilities there.

Y02 If you really want to create such support and keep it in openide/util, 
then please make it generally useful, e.g. think about generic support for 
listeners. Something like:
<Listener extends java.util.EventListener, Support extends Listener & 
java.util.Set<Listener>> Support createSupport(Class<Listener> listenerClass)
I mean try to create something like
http://java.sun.com/j2se/1.4.2/docs/api/java/awt/AWTEventMulticaster.html
but dynamically typed.

Y03 Possibly make the support for "weak" listeners. E.g. allow the listeners 
to be held via weak references. I know Hrebejk always thought that this should 
be the default. Moreover it could indicate where to put the createSupport 
method - it could be in WeakListeners.createSupport(...., boolean weak).

Y02-3: this would indeed be more generic, but it would require much more time
than I intended to invest in this. I will have a look at it, but I make no promises.

I'm not sure the returned support should be a Set. I would prefer to return my
own class containing just needed methods such as addListener() and removeListener().

Who will be the clients of listener types other than PropertyChangeListener and
ChangeListener, and also of the weak listener support? o.n.editor.Registry could
probably have a use for the weak ChangeListener support. What else? (I'm not
saying there are no such usages, I just don't know of any.)

Y01: as you probably expect, I'm not going to create a new module with a single
class. Does anyone have any other candidates for such a module?

Y01 - should not be dealt with in this issue. openide/util is the only
appropriate *existing* place we have for such a class. If someone wants to file
a separate API review to split openide/util into a "core" and an "optional"
piece, great; that will involve some effort to arbitrarily divide up its classes
and create an appropriate ModuleAutoDeps, I guess, plus a couple days' work to
upgrade nb.org modules to use the right deps (running fix-dependencies on each).

Y02 - I also don't see any compelling use case for a generified support. For
less common event listener types there are typically only one or a handful of
classes which serve as event sources, anyway; PropertyChangeListener and
ChangeListener are the only generic listener interfaces we use. I also do not
see a straightforward way to generify a fireChange() method to handle other
kinds of listeners, which generally require specialized means of constructing
event objects.

Y03 - might be useful to future code, though AFAIK there would be no users of it
at the moment; all our code follows the normal pattern of holding listeners
strongly, with clients bearing the responsibility of using WeakListeners if they
need it. Certainly it would be dangerous (probably incompatible) to suddenly
change the semantics of existing event sources. Generally I am suspicious of
using weak listeners transparently, as they can introduce nondeterministic
behavior - probably better if their usage is clearly visible in the code which
might be using them incorrectly. Adding a method to the support class such as

public void addWeakChangeListener(ChangeListener l);

(optionally also a remove method) would be fine, and probably it is fine for
event sources to have similarly named methods. IMHO such a feature should not be
in the WeakListeners class because (a) it would not use the same implementation
at all; (b) a factory method is useless because you need to define the signature
of its return value anyway; (c) the normal (non-weak) case is clearly and
directly analogous to PropertyChangeSupport, which is a well-known JRE utility
class whose style we should follow.

Created attachment 38700 [details]
Generic support is imho more useful and actually cleaner in its API

I don't like Yarda's proposed interface much. It is less convenient and
intuitive to use and doesn't buy me anything if I am just using ChangeListener,
which is the only common use case for this API that I know of. Compare:

class Source1 {
  final ChangeSupport cs = new ChangeSupport(this);
  public void addChangeListener(ChangeListener l) {
    cs.addChangeListener(l);
  }
  public void removeChangeListener(ChangeListener l) {
    cs.removeChangeListener(l);
  }
  void stuffHappened() {
    cs.fireChange();
  }
}

class Source2 {
  final Collection<ChangeListener> listeners = new
CopyOnWriteArrayList<ChangeListener>();
  public void addChangeListener(ChangeListener l) {
    listeners.addChangeListener(l);
  }
  public void removeChangeListener(ChangeListener l) {
    listeners.removeChangeListener(l);
  }
  void stuffHappened() {
    if (!listeners.isEmpty()) {
      ChangeEvent ev = new ChangeEvent(this);
      for (ChangeListener l : listeners) {
        l.stateChanged(ev);
      }
    }
  }
}

class Source3 {
  final Collection<ChangeListener> listeners = new
CopyOnWriteArrayList<ChangeListener>();
  final ChangeListener cs = WeakListeners.createSupport(ChangeListener.class,
listeners);
  public void addChangeListener(ChangeListener l) {
    listeners.addChangeListener(l);
  }
  public void removeChangeListener(ChangeListener l) {
    listeners.removeChangeListener(l);
  }
  void stuffHappened() {
    if (!listeners.isEmpty()) {
      ChangeEvent ev = new ChangeEvent(this);
      cs.stateChanged(ev);
    }
  }
}

Using the "support" under Yarda's proposal is actually just as much code as
doing without it. Further, rather than using a simple idiom every knows - same
as PropertyChangeSupport - you have to know to look in WeakListeners, then
basically copy the sample from the Javadoc to use the unfamiliar idiom. Even in
the uncommon case where you need to support a different event interface (e.g.
FileChangeListener), it would be simpler to write it directly using
CopyOnWriteArrayList.

I can't but agree.

Yarda's proposal is perhaps cleaner, but its weakness is its genericity -- it
can't support custom operations such as fireChange(), which is exactly what is
needed from a ChangeListener support. I can't imagine an user comparing e.g.
PropertyChangeSupport and the support returned by
WeakListeners.createSupport(PropertyChangeListener.class, listeners) and
choosing the latter. I believe the same would be true for the ChangeSupport I
proposed.

Created attachment 39184 [details]
Proposed change (revised)

I attached a new version of ChangeSupport that addresses Jesse's comments. It
removes the factory method, makes fireChange(ChangeEvent) private and introduces
hasListeners() instead of getChangeListeners(). Null parameters to
add/removeChangeListener() are still allowed.

I also considered adding the following constructor:

ChangeSupport(boolean unique);

to allow or disallow duplicate registrations of the same listener. There are
places in NetBeans where listeners are held in a Set, so this would come in
useful. But I think it would be better if all listener sources behaved the same
way, so I don't plan to add this constructor.

I will modify a few existing modules to use this class and attach the diffs here
in a few days. No plans to do whole top-level directories though--I am viewing
these changes mainly as a way to notify the readers of the respective CVS
mailing lists that this class is available.

Revised version looks fine to me.

I don't see any good reason to treat listeners as Set<ChangeListener> rather
than List<ChangeListener>; that just makes add and remove behave less
symmetrically, as add followed by add followed by remove is different from add
followed by remove followed by add. (Even with List they are not quite symmetric
since remove followed by add is different from add followed by remove.)

Created attachment 39657 [details]
Using ChangeSupport in projects

Created attachment 39660 [details]
Using ChangeSupport in java/api

Created attachment 39661 [details]
Using ChangeSupport in java/j2seproject

Created attachment 39662 [details]
Using ChangeSupport in j2ee/earproject, j2ee/ejbjarproject, j2ee/persistence

Created attachment 39663 [details]
Using ChangeSupport in web/project

I plan to commit the attached changes after getting a review for projects,
java/api and java/j2seproject. Hopefully by the end of the week.

projects, java/api, and java/j2seproject diffs looks good to me. I could use it
in apisupport/project too. (Including in templates, though that is a bit more
subtle as you need to check for users of an older NB version.)

Created attachment 39723 [details]
Using ChangeSupport in apisupport/project (w/o templates)

I did apisupport/project (please review), but without the templates. It would
take me more time to do them than to you or whoever owns the module, and I'd
prefer to spend that time scrathing one of my other itches. And they perhaps
deserve a separate issue anyway.

apisupport/project looks good again. (You got to use hasListeners()!) And yes,
any changes to templates need to be done by me or someone else working on
apisupport. It is not a high priority anyway.

I didn't reassign to myself timely and now I have to wait a working day before
committing, so I plan to commit Monday evening. Hope I'm not blocking anyone.

Thanks to reviewers for all comments and to people on CC for moral support :-)

So I guess FIXED, thanks.

I am working on using it in some additional core-team modules.

Commit log. Late because I spent some time ensuring the broken commit validation
was not caused by this.

Checking in openide/util/apichanges.xml;
/cvs/openide/util/apichanges.xml,v  <--  apichanges.xml
new revision: 1.23; previous revision: 1.22
done
Checking in openide/util/nbproject/project.properties;
/cvs/openide/util/nbproject/project.properties,v  <--  project.properties
new revision: 1.26; previous revision: 1.25
done
RCS file: /cvs/openide/util/src/org/openide/util/ChangeSupport.java,v
done
Checking in openide/util/src/org/openide/util/ChangeSupport.java;
/cvs/openide/util/src/org/openide/util/ChangeSupport.java,v  <--  ChangeSupport.java
initial revision: 1.1
done
RCS file: /cvs/openide/util/test/unit/src/org/openide/util/ChangeSupportTest.java,v
done
Checking in openide/util/test/unit/src/org/openide/util/ChangeSupportTest.java;
/cvs/openide/util/test/unit/src/org/openide/util/ChangeSupportTest.java,v  <-- 
ChangeSupportTest.java
initial revision: 1.1
done

Created attachment 39992 [details]
Commit log of the update of other modules to use ChangeSupport