Changeset 18842 in josm

Timestamp:

2023-09-25T17:34:00+02:00 (16 months ago)

Author:

taylor.smock

Message:

Fix #23191: NPE in AddTagsDialog

This is caused when a new layer is added via Remote Control while the add tag
dialog is open.

Location:

trunk

Files:

• src/org/openstreetmap/josm/gui/dialogs/properties/TagEditHelper.java (modified) (12 diffs)
• test/unit/org/openstreetmap/josm/gui/dialogs/properties/TagEditHelperTest.java (modified) (5 diffs)

trunk/src/org/openstreetmap/josm/gui/dialogs/properties/TagEditHelper.java

@@ -73 +73 @@
 import org.openstreetmap.josm.data.osm.OsmPrimitiveType;
 import org.openstreetmap.josm.data.osm.Tag;
+import org.openstreetmap.josm.data.osm.Tagged;
 import org.openstreetmap.josm.data.osm.search.SearchCompiler;
 import org.openstreetmap.josm.data.osm.search.SearchParseError;
@@ -82 +83 @@
 import org.openstreetmap.josm.data.preferences.StringProperty;
 import org.openstreetmap.josm.data.tagging.ac.AutoCompletionItem;
+import org.openstreetmap.josm.gui.ConditionalOptionPaneUtil;
 import org.openstreetmap.josm.gui.ExtendedDialog;
 import org.openstreetmap.josm.gui.IExtendedDialog;
@@ -278 +280 @@
         try {
             activeDataSet.beginUpdate();
-            sel = OsmDataManager.getInstance().getInProgressSelection();
-            if (Utils.isEmpty(sel))
+            Collection<OsmPrimitive> selection = OsmDataManager.getInstance().getInProgressSelection();
+            this.sel = selection;
+            if (Utils.isEmpty(selection))
                 return;
 
@@ -287 +290 @@
 
             addDialog.destroyActions();
-            if (addDialog.getValue() == 1)
-                addDialog.performTagAdding();
-            else
+            // Remote control can cause the selection to change, see #23191.
+            if (addDialog.getValue() == 1 && (selection == sel || warnSelectionChanged())) {
+                addDialog.performTagAdding(selection);
+            } else {
                 addDialog.undoAllTagsAdding();
+            }
         } finally {
             activeDataSet.endUpdate();
@@ -373 +378 @@
     public void loadTagsIfNeeded() {
         loadTagsToIgnore();
-        if (PROPERTY_REMEMBER_TAGS.get() && recentTags.isEmpty()) {
+        if (Boolean.TRUE.equals(PROPERTY_REMEMBER_TAGS.get()) && recentTags.isEmpty()) {
             recentTags.loadFromPreference(PROPERTY_RECENT_TAGS);
         }
@@ -407 +412 @@
      */
     public void saveTagsIfNeeded() {
-        if (PROPERTY_REMEMBER_TAGS.get() && !recentTags.isEmpty()) {
+        if (Boolean.TRUE.equals(PROPERTY_REMEMBER_TAGS.get()) && !recentTags.isEmpty()) {
             recentTags.saveToPreference(PROPERTY_RECENT_TAGS);
         }
@@ -449 +454 @@
             return selectedItem.toString();
         return getEditItem(cb);
+    }
+
+    /**
+     * Warn user about a selection change
+     * @return {@code true} if the user wants to apply the tag change to the old selection
+     */
+    private static boolean warnSelectionChanged() {
+        return ConditionalOptionPaneUtil.showConfirmationDialog("properties.selection-changed",
+                MainApplication.getMainFrame(),
+                tr("Data selection has changed since the dialog was opened"),
+                tr("Apply tag change to old selection?"),
+                JOptionPane.YES_NO_OPTION, JOptionPane.WARNING_MESSAGE, JOptionPane.YES_OPTION);
     }
 
@@ -506 +523 @@
                 /**
                  * This hack allows the comboboxes to have their own orientation.
-                 *
+                 * <p>
                  * The problem is that
                  * {@link org.openstreetmap.josm.gui.ExtendedDialog#showDialog ExtendedDialog} calls
                  * {@code applyComponentOrientation} very late in the dialog construction process
                  * thus overwriting the orientation the components have chosen for themselves.
-                 *
+                 * <p>
                  * This stops the propagation of {@code applyComponentOrientation}, thus all
                  * components may (and have to) set their own orientation.
@@ -794 +811 @@
                 /**
                  * This hack allows the comboboxes to have their own orientation.
-                 *
+                 * <p>
                  * The problem is that
                  * {@link org.openstreetmap.josm.gui.ExtendedDialog#showDialog ExtendedDialog} calls
                  * {@code applyComponentOrientation} very late in the dialog construction process
                  * thus overwriting the orientation the components have chosen for themselves.
-                 *
+                 * <p>
                  * This stops the propagation of {@code applyComponentOrientation}, thus all
                  * components may (and have to) set their own orientation.
@@ -1187 +1204 @@
          */
         public final void performTagAdding() {
+            Collection<OsmPrimitive> selection = sel;
+            if (!Utils.isEmpty(selection)) {
+                performTagAdding(selection);
+            }
+        }
+
+        /**
+         * Read tags from comboboxes and add it to all selected objects
+         * @param selection The selection to perform tag adding on
+         * @since 18842
+         */
+        private void performTagAdding(Collection<OsmPrimitive> selection) {
             String key = getEditItem(keys);
             String value = getEditItem(values);
             if (key.isEmpty() || value.isEmpty())
                 return;
-            for (OsmPrimitive osm : sel) {
+            for (Tagged osm : selection) {
                 String val = osm.get(key);
                 if (val != null && !val.equals(value)) {
@@ -1203 +1232 @@
             }
             recentTags.add(new Tag(key, value));
-            valueCount.put(key, new TreeMap<String, Integer>());
+            valueCount.put(key, new TreeMap<>());
             AutoCompletionManager.rememberUserInput(key, value, false);
             commandCount++;
-            UndoRedoHandler.getInstance().add(new ChangePropertyCommand(sel, key, value));
+            UndoRedoHandler.getInstance().add(new ChangePropertyCommand(selection, key, value));
             changedKey = key;
             clearEntries();
@@ -1216 +1245 @@
         }
 
+        /**
+         * Undo all tag add commands that this dialog has created
+         */
         public void undoAllTagsAdding() {
             UndoRedoHandler.getInstance().undo(commandCount);

trunk/test/unit/org/openstreetmap/josm/gui/dialogs/properties/TagEditHelperTest.java

@@ -2 +2 @@
 package org.openstreetmap.josm.gui.dialogs.properties;
 
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 
 import java.awt.GraphicsEnvironment;
@@ -15 +17 @@
 import java.util.List;
 import java.util.Map;
+import java.util.concurrent.Future;
+import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.function.Function;
 import java.util.stream.Collectors;
 
+import javax.swing.JOptionPane;
 import javax.swing.JTable;
 import javax.swing.table.DefaultTableModel;
 
+import org.awaitility.Awaitility;
+import org.awaitility.Durations;
 import org.junit.jupiter.api.Test;
 import org.openstreetmap.josm.TestUtils;
@@ -30 +37 @@
 import org.openstreetmap.josm.data.osm.Way;
 import org.openstreetmap.josm.data.tagging.ac.AutoCompletionItem;
+import org.openstreetmap.josm.gui.ExtendedDialog;
 import org.openstreetmap.josm.gui.MainApplication;
 import org.openstreetmap.josm.gui.dialogs.properties.TagEditHelper.AddTagsDialog;
@@ -35 +43 @@
 import org.openstreetmap.josm.gui.mappaint.MapPaintStyles;
 import org.openstreetmap.josm.gui.mappaint.mapcss.MapCSSStyleSource;
+import org.openstreetmap.josm.spi.preferences.Config;
 import org.openstreetmap.josm.testutils.annotations.Projection;
 import org.openstreetmap.josm.testutils.annotations.Territories;
 import org.openstreetmap.josm.testutils.mockers.WindowMocker;
+import org.openstreetmap.josm.tools.JosmRuntimeException;
+
+import mockit.Mock;
+import mockit.MockUp;
 
 /**
@@ -104 +117 @@
             return node;
         }, "junction", "roundabout");
+    }
+
+    /**
+     * Non-regression test for <a href="https://josm.openstreetmap.de/ticket/23191>#23191</a>
+     */
+    @Test
+    void testTicket23191() {
+        TestUtils.assumeWorkingJMockit();
+        final TagEditHelper tagEditHelper = newTagEditHelper();
+        final DataSet original = new DataSet();
+        MainApplication.getLayerManager().addLayer(new OsmDataLayer(original, "TagEditHelperTest.testTicket23191_1", null));
+        final Node toSelect = TestUtils.newNode("");
+        original.addPrimitive(toSelect);
+        original.setSelected(toSelect);
+        assertEquals(1, OsmDataManager.getInstance().getInProgressISelection().size());
+        assertTrue(OsmDataManager.getInstance().getInProgressISelection().contains(toSelect));
+
+        final AtomicBoolean canContinue = new AtomicBoolean();
+        final AtomicBoolean showingDialog = new AtomicBoolean();
+
+        // Instantiate the AddTagsDialog where we don't have to worry about race conditions
+        tagEditHelper.sel = OsmDataManager.getInstance().getInProgressSelection();
+        final AddTagsDialog addTagsDialog = tagEditHelper.getAddTagsDialog();
+        tagEditHelper.resetSelection();
+        new MockUp<TagEditHelper>() {
+            @Mock
+            public AddTagsDialog getAddTagsDialog() {
+                return addTagsDialog;
+            }
+        };
+
+        new MockUp<AddTagsDialog>() {
+            @Mock
+            public ExtendedDialog showDialog() {
+                showingDialog.set(true);
+                while (!canContinue.get()) {
+                    synchronized (canContinue) {
+                        try {
+                            canContinue.wait();
+                        } catch (InterruptedException e) {
+                            throw new JosmRuntimeException(e);
+                        }
+                    }
+                }
+                return null;
+            }
+
+            @Mock
+            public int getValue() {
+                return 1;
+            }
+        };
+
+        // Avoid showing the JOption pane
+        Config.getPref().putBoolean("message.properties.selection-changed", false);
+        Config.getPref().putInt("message.properties.selection-changed.value", JOptionPane.YES_OPTION);
+
+        // "Open" the tag edit dialog -- this should technically be in the EDT, but we are mocking the UI parts out,
+        // since the EDT does allow new EDT runnables when showing the add tag dialog
+        Future<?> tagFuture = MainApplication.worker.submit(tagEditHelper::addTag);
+
+        Awaitility.await().atMost(Durations.ONE_SECOND).untilTrue(showingDialog);
+        // This is what remote control will effectively do
+        MainApplication.getLayerManager().addLayer(new OsmDataLayer(new DataSet(), "TagEditHelperTest.testTicket23191_2", null));
+        tagEditHelper.resetSelection();
+
+        // Enter key=value
+        addTagsDialog.keys.setText("building");
+        addTagsDialog.values.setText("yes");
+
+        // Close the tag edit dialog
+        synchronized (canContinue) {
+            canContinue.set(true);
+            canContinue.notifyAll();
+        }
+
+        assertDoesNotThrow(() -> tagFuture.get());
     }