Index: /trunk/src/org/openstreetmap/josm/actions/ExpertToggleAction.java =================================================================== --- /trunk/src/org/openstreetmap/josm/actions/ExpertToggleAction.java (revision 11223) +++ /trunk/src/org/openstreetmap/josm/actions/ExpertToggleAction.java (revision 11224) @@ -6,10 +6,8 @@ import java.awt.Component; import java.awt.event.ActionEvent; -import java.lang.ref.WeakReference; -import java.util.ArrayList; -import java.util.Iterator; -import java.util.List; import org.openstreetmap.josm.Main; +import org.openstreetmap.josm.data.preferences.BooleanProperty; +import org.openstreetmap.josm.tools.ListenerList; /** @@ -19,35 +17,27 @@ public class ExpertToggleAction extends ToggleAction { + /** + * This listener is notified whenever the expert mode setting changed. + */ @FunctionalInterface public interface ExpertModeChangeListener { + /** + * The expert mode changed. + * @param isExpert true if expert mode was enabled, false otherwise. + */ void expertChanged(boolean isExpert); } - private static final List> listeners = new ArrayList<>(); - private static final List> visibilityToggleListeners = new ArrayList<>(); + // TODO: Switch to checked list. We can do this as soon as we do not see any more warnings. + private static final ListenerList listeners = ListenerList.createUnchecked(); + private static final ListenerList visibilityToggleListeners = ListenerList.createUnchecked(); + + private static final BooleanProperty PREF_EXPERT = new BooleanProperty("expert", false); private static final ExpertToggleAction INSTANCE = new ExpertToggleAction(); private static synchronized void fireExpertModeChanged(boolean isExpert) { - Iterator> it1 = listeners.iterator(); - while (it1.hasNext()) { - WeakReference wr = it1.next(); - ExpertModeChangeListener listener = wr.get(); - if (listener == null) { - it1.remove(); - continue; - } - listener.expertChanged(isExpert); - } - Iterator> it2 = visibilityToggleListeners.iterator(); - while (it2.hasNext()) { - WeakReference wr = it2.next(); - Component c = wr.get(); - if (c == null) { - it2.remove(); - continue; - } - c.setVisible(isExpert); - } + listeners.fireEvent(listener -> listener.expertChanged(isExpert)); + visibilityToggleListeners.fireEvent(c -> c.setVisible(isExpert)); } @@ -63,9 +53,5 @@ public static synchronized void addExpertModeChangeListener(ExpertModeChangeListener listener, boolean fireWhenAdding) { if (listener == null) return; - for (WeakReference wr : listeners) { - // already registered ? => abort - if (wr.get() == listener) return; - } - listeners.add(new WeakReference<>(listener)); + listeners.addWeakListener(listener); if (fireWhenAdding) { listener.expertChanged(isExpert()); @@ -80,36 +66,25 @@ public static synchronized void removeExpertModeChangeListener(ExpertModeChangeListener listener) { if (listener == null) return; - Iterator> it = listeners.iterator(); - while (it.hasNext()) { - WeakReference wr = it.next(); - // remove the listener - and any other listener which god garbage - // collected in the meantime - if (wr.get() == null || wr.get() == listener) { - it.remove(); - } - } + listeners.removeListener(listener); } + /** + * Marks a component to be only visible when expert mode is enabled. The visibility of the component is changed automatically. + * @param c The component. + */ public static synchronized void addVisibilitySwitcher(Component c) { if (c == null) return; - for (WeakReference wr : visibilityToggleListeners) { - // already registered ? => abort - if (wr.get() == c) return; - } - visibilityToggleListeners.add(new WeakReference<>(c)); + visibilityToggleListeners.addWeakListener(c); c.setVisible(isExpert()); } + /** + * Stops tracking visibility changes for the given component. + * @param c The component. + * @see #addVisibilitySwitcher(Component) + */ public static synchronized void removeVisibilitySwitcher(Component c) { if (c == null) return; - Iterator> it = visibilityToggleListeners.iterator(); - while (it.hasNext()) { - WeakReference wr = it.next(); - // remove the listener - and any other listener which god garbage - // collected in the meantime - if (wr.get() == null || wr.get() == c) { - it.remove(); - } - } + visibilityToggleListeners.removeListener(c); } @@ -128,5 +103,5 @@ Main.toolbar.register(this); } - setSelected(Main.pref.getBoolean("expert", false)); + setSelected(PREF_EXPERT.get()); notifySelectedState(); } @@ -135,5 +110,18 @@ protected final void notifySelectedState() { super.notifySelectedState(); + PREF_EXPERT.put(isSelected()); fireExpertModeChanged(isSelected()); + } + + /** + * Forces the expert mode state to the given state. + * @param isExpert if expert mode should be used. + * @since 11224 + */ + public void setExpert(boolean isExpert) { + if (isSelected() != isExpert) { + setSelected(isExpert); + notifySelectedState(); + } } @@ -141,5 +129,4 @@ public void actionPerformed(ActionEvent e) { toggleSelectedState(e); - Main.pref.put("expert", isSelected()); notifySelectedState(); } Index: /trunk/src/org/openstreetmap/josm/tools/ListenerList.java =================================================================== --- /trunk/src/org/openstreetmap/josm/tools/ListenerList.java (revision 11223) +++ /trunk/src/org/openstreetmap/josm/tools/ListenerList.java (revision 11224) @@ -80,10 +80,11 @@ */ public synchronized void addWeakListener(T listener) { - ensureNotInList(listener); - // clean the weak listeners, just to be sure... - while (weakListeners.remove(new WeakListener(null))) { - // continue - } - weakListeners.add(new WeakListener<>(listener)); + if (ensureNotInList(listener)) { + // clean the weak listeners, just to be sure... + while (weakListeners.remove(new WeakListener(null))) { + // continue + } + weakListeners.add(new WeakListener<>(listener)); + } } @@ -93,12 +94,16 @@ */ public synchronized void addListener(T listener) { - ensureNotInList(listener); - listeners.add(listener); - } - - private void ensureNotInList(T listener) { + if (ensureNotInList(listener)) { + listeners.add(listener); + } + } + + private boolean ensureNotInList(T listener) { CheckParameterUtil.ensureParameterNotNull(listener, "listener"); if (containsListener(listener)) { failAdd(listener); + return false; + } else { + return true; } } @@ -217,4 +222,18 @@ } + private static class UncheckedListenerList extends ListenerList { + @Override + protected void failAdd(T listener) { + Logging.warn("Listener was alreaady added: {0}", listener); + // ignore + } + + @Override + protected void failRemove(T listener) { + Logging.warn("Listener was removed twice or not added: {0}", listener); + // ignore + } + } + /** * Create a new listener list @@ -229,3 +248,15 @@ } } + + /** + * Creates a new listener list that does not fail if listeners are added ore removed twice. + *

+ * Use of this list is discouraged. You should always use {@link #create()} in new implementations and check your listeners. + * @param The listener type + * @return A new list. + * @since 11224 + */ + public static ListenerList createUnchecked() { + return new UncheckedListenerList<>(); + } } Index: /trunk/test/unit/org/openstreetmap/josm/actions/ExpertToggleActionTest.java =================================================================== --- /trunk/test/unit/org/openstreetmap/josm/actions/ExpertToggleActionTest.java (revision 11224) +++ /trunk/test/unit/org/openstreetmap/josm/actions/ExpertToggleActionTest.java (revision 11224) @@ -0,0 +1,87 @@ +// License: GPL. For details, see LICENSE file. +package org.openstreetmap.josm.actions; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import java.util.concurrent.atomic.AtomicBoolean; + +import javax.swing.JPanel; + +import org.junit.Rule; +import org.junit.Test; +import org.openstreetmap.josm.actions.ExpertToggleAction.ExpertModeChangeListener; +import org.openstreetmap.josm.testutils.JOSMTestRules; + +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; + +/** + * Test {@link ExpertToggleAction} + * @author Michael Zangl + * @since 11224 + */ +public class ExpertToggleActionTest { + /** + * We need prefs to store expert mode state. + */ + @Rule + @SuppressFBWarnings(value = "URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD") + public JOSMTestRules test = new JOSMTestRules().preferences().platform(); + + /** + * Test {@link ExpertToggleAction#addVisibilitySwitcher(java.awt.Component)} + * and {@link ExpertToggleAction#removeVisibilitySwitcher(java.awt.Component)} + */ + @Test + public void testVisibilitySwitcher() { + ExpertToggleAction.getInstance().setExpert(false); + JPanel c = new JPanel(); + + ExpertToggleAction.addVisibilitySwitcher(c); + assertFalse(c.isVisible()); + + ExpertToggleAction.getInstance().setExpert(true); + assertTrue(c.isVisible()); + + ExpertToggleAction.removeVisibilitySwitcher(c); + ExpertToggleAction.getInstance().setExpert(false); + assertTrue(c.isVisible()); + + // null should not be a problem + ExpertToggleAction.addVisibilitySwitcher(null); + ExpertToggleAction.removeVisibilitySwitcher(null); + } + + /** + * Test {@link ExpertToggleAction#addExpertModeChangeListener(ExpertModeChangeListener)} + * and {@link ExpertToggleAction#removeExpertModeChangeListener(ExpertModeChangeListener)} + */ + @Test + public void testExpertModeListener() { + AtomicBoolean value = new AtomicBoolean(false); + ExpertToggleAction.getInstance().setExpert(true); + ExpertModeChangeListener listener = value::set; + + ExpertToggleAction.addExpertModeChangeListener(listener); + assertFalse(value.get()); + + ExpertToggleAction.getInstance().setExpert(false); + ExpertToggleAction.getInstance().setExpert(true); + assertTrue(value.get()); + + ExpertToggleAction.getInstance().setExpert(false); + assertFalse(value.get()); + + ExpertToggleAction.removeExpertModeChangeListener(listener); + ExpertToggleAction.getInstance().setExpert(true); + assertFalse(value.get()); + + ExpertToggleAction.addExpertModeChangeListener(listener, true); + assertTrue(value.get()); + + // null should not be a problem + ExpertToggleAction.addExpertModeChangeListener(null); + ExpertToggleAction.removeExpertModeChangeListener(null); + } + +} Index: /trunk/test/unit/org/openstreetmap/josm/gui/layer/OsmDataLayerTest.java =================================================================== --- /trunk/test/unit/org/openstreetmap/josm/gui/layer/OsmDataLayerTest.java (revision 11223) +++ /trunk/test/unit/org/openstreetmap/josm/gui/layer/OsmDataLayerTest.java (revision 11224) @@ -4,5 +4,4 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; @@ -192,11 +191,9 @@ public void testGetMenuEntries() { OsmDataLayer layer = new OsmDataLayer(new DataSet(), "", null); - boolean mode = ExpertToggleAction.isExpert(); - ExpertToggleAction.getInstance().actionPerformed(null); - assertNotEquals(mode, ExpertToggleAction.isExpert()); - assertEquals(ExpertToggleAction.isExpert() ? 16 : 13, layer.getMenuEntries().length); - ExpertToggleAction.getInstance().actionPerformed(null); - assertEquals(mode, ExpertToggleAction.isExpert()); - assertEquals(ExpertToggleAction.isExpert() ? 16 : 13, layer.getMenuEntries().length); + ExpertToggleAction.getInstance().setExpert(true); + assertEquals(16, layer.getMenuEntries().length); + + ExpertToggleAction.getInstance().setExpert(false); + assertEquals(13, layer.getMenuEntries().length); }