Changeset 18706 in josm

Timestamp:

2023-04-20T22:13:25+02:00 (19 months ago)

Author:

taylor.smock

Message:

Fix #22817: Don't delete layer with changes and a non-normal upload policy (patch by gaben, modified)

Patch modifications are as follows:

• A non-regression test
• Account for UploadPolicy.BLOCKED causing requiresUploadToServer to return false.

Location:

trunk

Files:

• src/org/openstreetmap/josm/gui/io/SaveLayersDialog.java (modified) (3 diffs)
• test/unit/org/openstreetmap/josm/gui/io/SaveLayersDialogTest.java (modified) (3 diffs)

trunk/src/org/openstreetmap/josm/gui/io/SaveLayersDialog.java

@@ -82 +82 @@
     }
 
-    private enum UserAction {
+    /**
+     * The action a user decided to take with respect to an operation
+     */
+    enum UserAction {
         /** save/upload layers was successful, proceed with operation */
         PROCEED,
@@ -113 +116 @@
         if (!GraphicsEnvironment.isHeadless()) {
             SaveLayersDialog dialog = new SaveLayersDialog(MainApplication.getMainFrame());
-            List<AbstractModifiableLayer> layersWithUnmodifiedChanges = new ArrayList<>();
+            List<AbstractModifiableLayer> layersWithUnsavedChanges = new ArrayList<>();
             for (Layer l: selectedLayers) {
                 if (!(l instanceof AbstractModifiableLayer)) {
@@ -122 +125 @@
                         ((!odl.isSavable() && !odl.isUploadable()) ||
                                 odl.requiresSaveToFile() ||
-                                (odl.requiresUploadToServer() && !odl.isUploadDiscouraged()))) {
-                    layersWithUnmodifiedChanges.add(odl);
+                                odl.requiresUploadToServer())) {
+                    layersWithUnsavedChanges.add(odl);
                 }
             }
             dialog.prepareForSavingAndUpdatingLayers(reason);
-            if (!layersWithUnmodifiedChanges.isEmpty()) {
-                dialog.getModel().populate(layersWithUnmodifiedChanges);
+            if (!layersWithUnsavedChanges.isEmpty()) {
+                dialog.getModel().populate(layersWithUnsavedChanges);
                 dialog.setVisible(true);
                 switch(dialog.getUserAction()) {

trunk/test/unit/org/openstreetmap/josm/gui/io/SaveLayersDialogTest.java

@@ -6 +6 @@
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
+import java.awt.Component;
+import java.awt.GraphicsEnvironment;
+import java.io.File;
 import java.util.Collections;
 import java.util.List;
+
 import javax.swing.JComponent;
 import javax.swing.JLabel;
@@ -13 +17 @@
 import javax.swing.JOptionPane;
 
-import org.junit.jupiter.api.extension.RegisterExtension;
+import mockit.Invocation;
+import mockit.Mock;
+import mockit.MockUp;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.EnumSource;
+import org.openstreetmap.josm.command.AddPrimitivesCommand;
+import org.openstreetmap.josm.data.coor.LatLon;
 import org.openstreetmap.josm.data.osm.DataSet;
+import org.openstreetmap.josm.data.osm.Node;
+import org.openstreetmap.josm.data.osm.UploadPolicy;
 import org.openstreetmap.josm.gui.layer.OsmDataLayer;
-import org.openstreetmap.josm.testutils.JOSMTestRules;
+import org.openstreetmap.josm.testutils.annotations.BasicPreferences;
 import org.openstreetmap.josm.testutils.mockers.JOptionPaneSimpleMocker;
-
-import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+import org.openstreetmap.josm.testutils.mockers.WindowMocker;
 
 /**
  * Unit tests of {@link SaveLayersDialog} class.
  */
+@BasicPreferences
 class SaveLayersDialogTest {
-
-    /**
-     * Setup tests
-     */
-    @RegisterExtension
-    @SuppressFBWarnings(value = "URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD")
-    public JOSMTestRules test = new JOSMTestRules();
-
     /**
      * Test of {@link SaveLayersDialog#confirmSaveLayerInfosOK}.
@@ -118 +122 @@
         assertTrue(SaveLayersDialog.confirmSaveLayerInfosOK(new SaveLayersModel()));
     }
+
+    /**
+     * Non-regression test for #22817: No warning when deleting a layer with changes and discourages upload
+     * @param policy The upload policy to test
+     */
+    @ParameterizedTest
+    @EnumSource(value = UploadPolicy.class)
+    void testNonRegression22817(UploadPolicy policy) {
+        final OsmDataLayer osmDataLayer = new OsmDataLayer(new DataSet(), null, null);
+        osmDataLayer.getDataSet().setUploadPolicy(policy);
+        // BLOCKED files don't have a way to become blocked via the UI, so they must be loaded from disk.
+        if (policy == UploadPolicy.BLOCKED) {
+            osmDataLayer.setAssociatedFile(new File("/dev/null"));
+        }
+        new AddPrimitivesCommand(Collections.singletonList(new Node(LatLon.ZERO).save()), Collections.emptyList(), osmDataLayer.getDataSet())
+                .executeCommand();
+        assertTrue(osmDataLayer.getDataSet().isModified());
+        new WindowMocker();
+        // Needed since the *first call* is to check whether we are in a headless environment
+        new GraphicsEnvironmentMock();
+        // Needed since we need to mock out the UI
+        SaveLayersDialogMock saveLayersDialogMock = new SaveLayersDialogMock();
+
+        assertTrue(SaveLayersDialog.saveUnsavedModifications(Collections.singleton(osmDataLayer), SaveLayersDialog.Reason.DELETE));
+        assertEquals(1, saveLayersDialogMock.getUserActionCalled, "The user should have been asked for an action on the layer");
+    }
+
+    private static class GraphicsEnvironmentMock extends MockUp<GraphicsEnvironment> {
+        @Mock
+        public static boolean isHeadless(Invocation invocation) {
+            return false;
+        }
+    }
+
+    private static class SaveLayersDialogMock extends MockUp<SaveLayersDialog> {
+        private final SaveLayersModel model = new SaveLayersModel();
+        private int getUserActionCalled = 0;
+        @Mock
+        public void $init(Component parent) {
+            // Do nothing
+        }
+
+        @Mock
+        public void prepareForSavingAndUpdatingLayers(final SaveLayersDialog.Reason reason) {
+            // Do nothing
+        }
+
+        @Mock
+        public SaveLayersModel getModel() {
+            return this.model;
+        }
+
+        @Mock
+        public void setVisible(boolean b) {
+            // Do nothing
+        }
+
+        @Mock
+        public SaveLayersDialog.UserAction getUserAction() {
+            this.getUserActionCalled++;
+            return SaveLayersDialog.UserAction.PROCEED;
+        }
+
+        @Mock
+        public void closeDialog() {
+            // Do nothing
+        }
+    }
 }