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

From: Shawn O. Pearce <spearce@spearce.org>
Date: Feb 5, 2009, 16:40 UTC
Message-ID: <20090205164022.GK26880@spearce.org>
In-Reply-To: <498B1563.4070905@gmail.com>

Tor Arne Vestbø <torarnv@gmail.com> wrote:

Show 20 quoted lines

> Shawn O. Pearce wrote:
> > Tor Arne Vestbø <torarnv@gmail.com> 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.  :-)

Show 20 quoted lines

> >>  	/**
> >> +	 * 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.