Subject: Re: [EGIT PATCH 01/11] Add support code to handle plugin property changes

From: "Shawn O. Pearce" <[email-masked]>
Date: Thu, 5 Feb 2009 08:40:22 -0800

Tor Arne Vestbø <[email-masked]> wrote:
> Shawn O. Pearce wrote:
> > Tor Arne Vestbø <[email-masked]> wrote:
> >> diff --git a/org.spearce.egit.ui/src/org/spearce/egit/ui/Activator.java b/org.spearce.egit.ui/src/org/spearce/egit/ui/Activator.java
> >> index fced643..d4a9e8e 100644
> >> --- a/org.spearce.egit.ui/src/org/spearce/egit/ui/Activator.java
> >> +++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/Activator.java
> >> @@ -44,9 +48,24 @@
> >>   * This is a plugin singleton mostly controlling logging.
> >>   */
> >>  public class Activator extends AbstractUIPlugin {
> >> +
> >> +	/**
> >> +	 *  The one and only instance
> >> +	 */
> >>  	private static Activator plugin;
> > 
> > What does this field do?  I don't see it referenced anywhere...
> 
> It's the singleton instance, referenced from Activator.getDefault(). I
> just documented it :)

*sigh*.  I haven't had enough caffiene yet this morning.  Yes,
I see what you mean, its only the Javadoc that was added.  OK,
forget I said anything here.  :-)
 
> >>  	/**
> >> +	 * Property listeners for plugin specific events
> >> +	 */
> >> +	private static List<IPropertyChangeListener> propertyChangeListeners =
> >> +		new ArrayList<IPropertyChangeListener>(5);
> > 
> > None of these list accesses are thread-safe.  Are we certain they
> > call will come from a single thread, e.g. the SWT event thread?
> > Or do we need to put synchronized protection in here?
> 
> The addPropertyChangeListener method is called at startup from the
> GitLightweightDecorator constructor, in one of the worker threads, and
> same thing with removePropertyChangeListener from dispose(). The
> broadcastPropertyChange method is called in the main thread every time
> the Git decorator preference page is closes.
> 
> This is the sync model used by other Eclipse plugins for keeping track
> of propertyChangeListeners, for example in the TeamUIPlugin, that's why
> I assumed it was OK. I'm perfectly fine with syncrhronizing it though,
> similar to repositoryChangeListeners in GitProjectData?

OK, I see.  I'd perhaps prefer to make this thread-safe just in case.
If its always coming off the SWT event thread, then just tossing
a synchronized keyword on all 3 methods should be Good Enough(tm).

-- 
Shawn.