From 92c57cc436a60daaf5a7ae85661e1ce58a740622 Mon Sep 17 00:00:00 2001 From: Taylor Smock Date: Tue, 17 May 2022 15:22:02 -0600 Subject: [PATCH] Fix JOSM #21902: IAE: Listener was not registered before This is caused by (a) sharing an action between a menu and (b) a mapframe icon. This adds a non-regression test, and additionally modifies the AreaSelectorAction to override `readPreferences`, which is called when the map mode is entered. Furthermore, the MapMode class implements PreferenceChangedListener, which removes the need to notify the action manually about modified preferences. Signed-off-by: Taylor Smock --- .gitignore | 1 - .../areaselector/AreaSelectorAction.java | 34 ++++---------- .../areaselector/AreaSelectorPlugin.java | 10 ++-- .../preferences/AreaSelectorPreference.java | 9 +--- .../areaselector/AreaSelectorPluginTest.java | 47 +++++++++++++++++++ 5 files changed, 65 insertions(+), 36 deletions(-) create mode 100644 test/unit/org/openstreetmap/josm/plugins/areaselector/AreaSelectorPluginTest.java diff --git a/.gitignore b/.gitignore index db6bc47..15ea0fa 100644 --- a/.gitignore +++ b/.gitignore @@ -3,7 +3,6 @@ bin .svn build out -test lib/jar lib/javadoc lib/source diff --git a/src/org/openstreetmap/josm/plugins/areaselector/AreaSelectorAction.java b/src/org/openstreetmap/josm/plugins/areaselector/AreaSelectorAction.java index 6ee4bd5..f9a081a 100644 --- a/src/org/openstreetmap/josm/plugins/areaselector/AreaSelectorAction.java +++ b/src/org/openstreetmap/josm/plugins/areaselector/AreaSelectorAction.java @@ -49,9 +49,7 @@ import org.openstreetmap.josm.data.preferences.BooleanProperty; import org.openstreetmap.josm.data.preferences.DoubleProperty; import org.openstreetmap.josm.data.preferences.StringProperty; -import org.openstreetmap.josm.gui.IconToggleButton; import org.openstreetmap.josm.gui.MainApplication; -import org.openstreetmap.josm.gui.MapFrame; import org.openstreetmap.josm.gui.MapView; import org.openstreetmap.josm.gui.Notification; import org.openstreetmap.josm.gui.layer.Layer; @@ -100,17 +98,18 @@ public class AreaSelectorAction extends MapMode implements MouseListener { protected Point clickPoint = null; - public AreaSelectorAction(MapFrame mapFrame) { + public AreaSelectorAction() { super(tr("Area Selection"), "areaselector", tr("Select an area (e.g. building) from an underlying image."), Shortcut.registerShortcut("tools:areaselector", tr("Tools: {0}", tr("Area Selector")), KeyEvent.VK_A, Shortcut.ALT_CTRL), getCursor()); // load prefs - this.readPrefs(); + this.readPreferences(); } - protected void readPrefs() { + @Override + protected void readPreferences() { this.mergeNodes = new BooleanProperty(KEY_MERGENODES, true).get(); this.showAddressDialog = new BooleanProperty(KEY_SHOWADDRESSDIALOG, true).get(); this.taggingStyle = new StringProperty(KEY_TAGGINGSTYLE, "none").get(); @@ -141,12 +140,6 @@ public void exitMode() { MainApplication.getMap().mapView.removeMouseListener(this); } - public void updateMapFrame(MapFrame oldFrame, MapFrame newFrame) { - if (oldFrame == null && newFrame != null) { - MainApplication.getMap().addMapMode(new IconToggleButton(this)); - } - } - /** * Invoked when the mouse button has been clicked (pressed and released) on * a component. @@ -167,16 +160,12 @@ public void mouseClicked(MouseEvent e) { new Notification("" + tr("Area Selector") + "
" + tr("Trying to detect an area at:") + "
" + coordinates.getX() + ", " + coordinates.getY()).setIcon(JOptionPane.INFORMATION_MESSAGE) .show(); - SwingUtilities.invokeLater(new Runnable() { - - @Override - public void run() { - try { - createArea(); - } catch (Exception ex) { - log.error("failed to add area", ex); - new BugReportDialog(ex); - } + SwingUtilities.invokeLater(() -> { + try { + createArea(); + } catch (Exception ex) { + log.error("failed to add area", ex); + new BugReportDialog(ex); } }); @@ -552,7 +541,4 @@ public Command mergeNode(Node node) { return MergeNodesAction.mergeNodes(selectedNodes, targetNode, targetLocationNode); } - public void setPrefs() { - this.readPrefs(); - } } diff --git a/src/org/openstreetmap/josm/plugins/areaselector/AreaSelectorPlugin.java b/src/org/openstreetmap/josm/plugins/areaselector/AreaSelectorPlugin.java index 016b3bb..e42ec5a 100644 --- a/src/org/openstreetmap/josm/plugins/areaselector/AreaSelectorPlugin.java +++ b/src/org/openstreetmap/josm/plugins/areaselector/AreaSelectorPlugin.java @@ -10,6 +10,7 @@ import org.apache.logging.log4j.core.config.builder.api.ConfigurationBuilder; import org.apache.logging.log4j.core.config.builder.api.ConfigurationBuilderFactory; import org.apache.logging.log4j.core.config.builder.impl.BuiltConfiguration; +import org.openstreetmap.josm.gui.IconToggleButton; import org.openstreetmap.josm.gui.MainApplication; import org.openstreetmap.josm.gui.MainMenu; import org.openstreetmap.josm.gui.MapFrame; @@ -33,7 +34,7 @@ public AreaSelectorPlugin(PluginInformation info) { setupLogging(); - areaSelectorAction = new AreaSelectorAction(MainApplication.getMap()); + areaSelectorAction = new AreaSelectorAction(); MainMenu.add(MainApplication.getMenu().toolsMenu, areaSelectorAction); addressDialogAction = new AddressDialogAction(MainApplication.getMap()); @@ -45,12 +46,15 @@ public AreaSelectorPlugin(PluginInformation info) { */ @Override public void mapFrameInitialized(MapFrame oldFrame, MapFrame newFrame) { - areaSelectorAction.updateMapFrame(oldFrame, newFrame); + // Do not share AreaSelectorActions -- the action added to the mapmode is destroyed when the mapmode is destroyed + if (oldFrame == null && newFrame != null) { + MainApplication.getMap().addMapMode(new IconToggleButton(new AreaSelectorAction())); + } } @Override public PreferenceSetting getPreferenceSetting() { - return new AreaSelectorPreference(this); + return new AreaSelectorPreference(); } /** diff --git a/src/org/openstreetmap/josm/plugins/areaselector/preferences/AreaSelectorPreference.java b/src/org/openstreetmap/josm/plugins/areaselector/preferences/AreaSelectorPreference.java index 3b45e05..8a0fce2 100644 --- a/src/org/openstreetmap/josm/plugins/areaselector/preferences/AreaSelectorPreference.java +++ b/src/org/openstreetmap/josm/plugins/areaselector/preferences/AreaSelectorPreference.java @@ -5,20 +5,15 @@ import org.openstreetmap.josm.gui.preferences.DefaultTabPreferenceSetting; import org.openstreetmap.josm.gui.preferences.PreferenceTabbedPane; -import org.openstreetmap.josm.plugins.areaselector.AreaSelectorAction; -import org.openstreetmap.josm.plugins.areaselector.AreaSelectorPlugin; public class AreaSelectorPreference extends DefaultTabPreferenceSetting { - AreaSelectorPlugin areaSelectorPlugin; PreferencesPanel prefPanel; - public AreaSelectorPreference(AreaSelectorPlugin plugin) { + public AreaSelectorPreference() { super("areaselector", "

"+tr("Area Selector - Preferences") + "

", tr("Settings for the area detection algorithm.")); - - areaSelectorPlugin = plugin; } @Override @@ -33,8 +28,6 @@ public void addGui(PreferenceTabbedPane gui) { @Override public boolean ok() { prefPanel.savePreferences(); - AreaSelectorAction areaSelectorAction = areaSelectorPlugin.getAreaSelectorAction(); - areaSelectorAction.setPrefs(); return false; } } diff --git a/test/unit/org/openstreetmap/josm/plugins/areaselector/AreaSelectorPluginTest.java b/test/unit/org/openstreetmap/josm/plugins/areaselector/AreaSelectorPluginTest.java new file mode 100644 index 0000000..371e436 --- /dev/null +++ b/test/unit/org/openstreetmap/josm/plugins/areaselector/AreaSelectorPluginTest.java @@ -0,0 +1,47 @@ +package org.openstreetmap.josm.plugins.areaselector; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; + +import java.util.jar.Attributes; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; +import org.openstreetmap.josm.data.osm.DataSet; +import org.openstreetmap.josm.gui.MainApplication; +import org.openstreetmap.josm.gui.MapFrame; +import org.openstreetmap.josm.gui.layer.Layer; +import org.openstreetmap.josm.gui.layer.OsmDataLayer; +import org.openstreetmap.josm.plugins.PluginException; +import org.openstreetmap.josm.plugins.PluginInformation; +import org.openstreetmap.josm.testutils.JOSMTestRules; +import org.openstreetmap.josm.testutils.annotations.BasicPreferences; + +/** + * Test class for {@link AreaSelectorPlugin} + * @author Taylor Smock + */ +@BasicPreferences +class AreaSelectorPluginTest { + @RegisterExtension + static JOSMTestRules josmTestRules = new JOSMTestRules().main().projection(); + + /** + * Non-regression test for JOSM #21902. This occurs when someone removes all layers, adds a new layer, and then + * removes all layers again. + */ + @Test + void testNonRegression21902() throws PluginException { + assertDoesNotThrow(() -> AreaSelectorPlugin.class.getDeclaredMethod("mapFrameInitialized", MapFrame.class, MapFrame.class), "AreaSelector used to implement mapFrameInitialized"); + final Attributes attributes = new Attributes(); + attributes.put(new Attributes.Name("Plugin-Mainversion"), Integer.toString(10_000)); + AreaSelectorPlugin plugin = new AreaSelectorPlugin(new PluginInformation(attributes, getClass().getSimpleName(), null)); + MainApplication.getMainPanel().addMapFrameListener(plugin); + // We have to go through this twice. So let us do it a few extra times, just in case. + for (int i = 1; i <= 5; i++) { + final Layer testLayer = new OsmDataLayer(new DataSet(), getClass().getSimpleName(), null); + assertDoesNotThrow(() -> MainApplication.getLayerManager().addLayer(testLayer), "Failed on cycle " + i); + assertDoesNotThrow(() -> MainApplication.getLayerManager().removeLayer(testLayer), "Failed on cycle " + i); + } + assertDoesNotThrow(() -> plugin.mapFrameInitialized(null, null)); + } +}