Skip to content

Commit

Permalink
Fix JOSM #21902: IAE: Listener was not registered before
Browse files Browse the repository at this point in the history
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 <[email protected]>
  • Loading branch information
tsmock committed Aug 24, 2022
1 parent 773aef4 commit 92c57cc
Show file tree
Hide file tree
Showing 5 changed files with 65 additions and 36 deletions.
1 change: 0 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ bin
.svn
build
out
test
lib/jar
lib/javadoc
lib/source
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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.
Expand All @@ -167,16 +160,12 @@ public void mouseClicked(MouseEvent e) {
new Notification("<strong>" + tr("Area Selector") + "</strong><br />" + tr("Trying to detect an area at:")
+ "<br>" + 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);
}
});

Expand Down Expand Up @@ -552,7 +541,4 @@ public Command mergeNode(Node node) {
return MergeNodesAction.mergeNodes(selectedNodes, targetNode, targetLocationNode);
}

public void setPrefs() {
this.readPrefs();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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());
Expand All @@ -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();
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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",
"<html><p><b>"+tr("Area Selector - Preferences") + "</b></p></html>",
tr("Settings for the area detection algorithm."));

areaSelectorPlugin = plugin;
}

@Override
Expand All @@ -33,8 +28,6 @@ public void addGui(PreferenceTabbedPane gui) {
@Override
public boolean ok() {
prefPanel.savePreferences();
AreaSelectorAction areaSelectorAction = areaSelectorPlugin.getAreaSelectorAction();
areaSelectorAction.setPrefs();
return false;
}
}
Original file line number Diff line number Diff line change
@@ -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));
}
}

0 comments on commit 92c57cc

Please sign in to comment.