Changeset 18686 in josm

Timestamp:

2023-03-08T20:00:36+01:00 (21 months ago)

Author:

taylor.smock

Message:

See #22767, fix failing unit tests

The fix for #22767 included a new unit test, which caused a cascading failure.
The first step of the fix included removing the destroyed dialog from the
MapFrame. Unfortunately, MapFrame#removeToggleDialog did not properly
remove the ToggleDialog -- it called DialogsPanel#remove(Component) with
the ToggleDialog as an argument. Since the DialogsPanel never adds the
ToggleDialog as a direct subcomponent, this was effectively a no-op.

Location:

trunk

Files:

• src/org/openstreetmap/josm/gui/dialogs/DialogsPanel.java (modified) (2 diffs)
• src/org/openstreetmap/josm/gui/dialogs/ToggleDialog.java (modified) (1 diff)
• src/org/openstreetmap/josm/gui/layer/geoimage/ImageViewerDialog.java (modified) (1 diff)
• test/unit/org/openstreetmap/josm/gui/layer/geoimage/ImageViewerDialogTest.java (modified) (2 diffs)
• test/unit/org/openstreetmap/josm/gui/layer/geoimage/ImagesLoaderTest.java (modified) (1 diff)
• test/unit/org/openstreetmap/josm/gui/layer/geoimage/WikimediaCommonsLoaderTest.java (modified) (2 diffs)

trunk/src/org/openstreetmap/josm/gui/dialogs/DialogsPanel.java

@@ -105 +105 @@
         } else {
             dlg.hideDialog();
+        }
+    }
+
+    /**
+     * Remove a {@link ToggleDialog} from the list of known dialogs and trigger reconstruct.
+     * @param toggleDialog The dialog to remove
+     * @since 18686
+     */
+    public void remove(ToggleDialog toggleDialog) {
+        remove(toggleDialog, true);
+    }
+
+    /**
+     * Remove a {@link ToggleDialog} from the list of known dialogs.
+     * @param toggleDialog The dialog to remove
+     * @param doReconstruct <code>true</code> if reconstruction should be triggered.
+     * @since 18686
+     */
+    public void remove(ToggleDialog toggleDialog, boolean doReconstruct) {
+        toggleDialog.setDialogsPanel(null);
+        final JPanel oldPanel = panels.get(allDialogs.indexOf(toggleDialog));
+        allDialogs.remove(toggleDialog);
+        panels.remove(oldPanel);
+        mSpltPane.remove(oldPanel);
+        if (doReconstruct && !allDialogs.isEmpty()) {
+            reconstruct(Action.ELEMENT_SHRINKS, toggleDialog);
         }
     }
@@ -326 +352 @@
     @Override
     public void destroy() {
-        for (ToggleDialog t : allDialogs) {
+        for (ToggleDialog t : new ArrayList<>(allDialogs)) {
             try {
                 t.destroy();

trunk/src/org/openstreetmap/josm/gui/dialogs/ToggleDialog.java

@@ -478 +478 @@
     @Override
     public void destroy() {
-        dialogsPanel = null;
+        if (dialogsPanel != null) {
+            dialogsPanel.remove(this);
+            dialogsPanel = null;
+        }
         rememberHeight();
         closeDetachedDialog();

trunk/src/org/openstreetmap/josm/gui/layer/geoimage/ImageViewerDialog.java

@@ -145 +145 @@
      */
     private static void destroyInstance() {
+        MapFrame map = MainApplication.getMap();
+        synchronized (ImageViewerDialog.class) {
+            if (dialog != null && map != null && map.getToggleDialog(ImageViewerDialog.class) != null) {
+                map.removeToggleDialog(dialog);
+            }
+        }
         dialog = null;
     }

trunk/test/unit/org/openstreetmap/josm/gui/layer/geoimage/ImageViewerDialogTest.java

@@ -21 +21 @@
 import javax.swing.JPanel;
 
+import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.extension.RegisterExtension;
@@ -44 +45 @@
 
     private ImageViewerDialog dialog;
+
     @BeforeEach
     void setup() {
         this.dialog = ImageViewerDialog.getInstance();
         this.dialog.displayImages(null);
+    }
+
+    @AfterEach
+    void tearDown() {
+        this.dialog.destroy();
     }
 

trunk/test/unit/org/openstreetmap/josm/gui/layer/geoimage/ImagesLoaderTest.java

@@ -10 +10 @@
 import java.util.List;
 
+import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.Test;
-import org.junit.jupiter.api.extension.RegisterExtension;
 import org.openstreetmap.josm.TestUtils;
 import org.openstreetmap.josm.gui.MainApplication;
 import org.openstreetmap.josm.gui.layer.GpxLayer;
 import org.openstreetmap.josm.io.GpxReader;
-import org.openstreetmap.josm.testutils.JOSMTestRules;
-
-import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+import org.openstreetmap.josm.testutils.annotations.BasicPreferences;
 
 /**
  * Unit tests of {@link ImagesLoader} class.
  */
+@BasicPreferences
 class ImagesLoaderTest {
 
-    /**
-     * We need prefs for this.
-     */
-    @RegisterExtension
-    @SuppressFBWarnings(value = "URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD")
-    public JOSMTestRules test = new JOSMTestRules().preferences();
+    @AfterEach
+    void tearDown() {
+        if (ImageViewerDialog.hasInstance()) {
+            ImageViewerDialog.getInstance().destroy();
+        }
+    }
 
     /**

trunk/test/unit/org/openstreetmap/josm/gui/layer/geoimage/WikimediaCommonsLoaderTest.java

@@ -11 +11 @@
 import java.util.List;
 
+import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.Test;
@@ -29 +30 @@
     static void beforeAll() {
         HttpClient.setFactory(Http1Client::new);
+    }
+
+    @AfterEach
+    void tearDown() {
+        if (ImageViewerDialog.hasInstance()) {
+            ImageViewerDialog.getInstance().destroy();
+        }
     }