Changeset 18683 in josm for trunk

Timestamp:

2023-03-06T23:28:55+01:00 (2 years ago)

Author:

taylor.smock

Message:

Fix #22783: Presets menu should not sort the first action block

The specific problem was that when the preset menu was sorted, the order of
the Preset menu (prior to the first separator) was Search preset...,
Preset preferences..., Search for objects by preset... instead of
Search preset..., Search for objects by preset..., Preset Preferences...

The fix for this was changing the TaggingPresetMenu.PresetTextComparator.compare
method to have the same behavior for the ordered presets. A non-regression
test was also added.

Location:

trunk

Files:

• src/org/openstreetmap/josm/gui/tagging/presets/TaggingPresetMenu.java (modified) (3 diffs)
• test/unit/org/openstreetmap/josm/gui/tagging/presets/TaggingPresetsTest.java (modified) (4 diffs)

trunk/src/org/openstreetmap/josm/gui/tagging/presets/TaggingPresetMenu.java

@@ -11 +11 @@
 import java.io.Serializable;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Comparator;
 import java.util.List;
@@ -21 +22 @@
 import javax.swing.JSeparator;
 
+import org.openstreetmap.josm.actions.JosmAction;
 import org.openstreetmap.josm.gui.MainApplication;
 import org.openstreetmap.josm.gui.MainFrame;
+import org.openstreetmap.josm.gui.MainMenu;
 import org.openstreetmap.josm.tools.AlphanumComparator;
 import org.openstreetmap.josm.tools.Logging;
@@ -38 +41 @@
         @Override
         public int compare(JMenuItem o1, JMenuItem o2) {
-            if (MainApplication.getMenu().presetSearchAction.equals(o1.getAction()))
-                return -1;
-            else if (MainApplication.getMenu().presetSearchAction.equals(o2.getAction()))
-                return 1;
-            else
-                return AlphanumComparator.getInstance().compare(o1.getText(), o2.getText());
+            final MainMenu menu = MainApplication.getMenu();
+            // This is needed to keep the order of the search actions -> preferences
+            for (JosmAction action : Arrays.asList(menu.presetSearchAction, menu.presetSearchPrimitiveAction)) {
+                if (action.equals(o1.getAction())) {
+                    return -1;
+                } else if (action.equals(o2.getAction())) {
+                    return 1;
+                }
+            }
+            return AlphanumComparator.getInstance().compare(o1.getText(), o2.getText());
         }
     }

trunk/test/unit/org/openstreetmap/josm/gui/tagging/presets/TaggingPresetsTest.java

@@ -1 +1 @@
 // License: GPL. For details, see LICENSE file.
 package org.openstreetmap.josm.gui.tagging.presets;
+
+import static org.junit.jupiter.api.Assertions.assertAll;
+import static org.junit.jupiter.api.Assertions.assertSame;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 
 import java.util.Collection;
@@ -8 +12 @@
 import java.util.concurrent.TimeoutException;
 
-import org.junit.jupiter.api.Test;
-import org.junit.jupiter.api.extension.RegisterExtension;
-import org.openstreetmap.josm.testutils.JOSMTestRules;
-import org.openstreetmap.josm.tools.Logging;
+import javax.swing.JMenu;
+import javax.swing.JSeparator;
 
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import net.trajano.commons.testing.UtilityClassTestUtil;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+import org.openstreetmap.josm.actions.PreferencesAction;
+import org.openstreetmap.josm.gui.MainApplication;
+import org.openstreetmap.josm.gui.MainMenu;
+import org.openstreetmap.josm.testutils.JOSMTestRules;
+import org.openstreetmap.josm.tools.Logging;
 
 /**
@@ -26 +37 @@
     @RegisterExtension
     @SuppressFBWarnings(value = "URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD")
-    public JOSMTestRules test = new JOSMTestRules();
+    public JOSMTestRules test = new JOSMTestRules().main();
 
     /**
@@ -35 +46 @@
     void testUtilityClass() throws ReflectiveOperationException {
         UtilityClassTestUtil.assertUtilityClassWellDefined(TaggingPresets.class);
+    }
+
+    /**
+     * Ensure that sorting the menu does <em>not</em> change the order of the first 3 actions.
+     * See {@link TaggingPresetMenu.PresetTextComparator} for the comparator to fix if this test
+     * fails <i>after</i> adding a new menu item.
+     */
+    @ParameterizedTest
+    @ValueSource(booleans = {true, false})
+    void testNonRegression22783(boolean sort) {
+        final MainMenu menu = MainApplication.getMenu();
+        TaggingPresets.SORT_MENU.put(sort);
+        TaggingPresets.initialize();
+        final JMenu presetsMenu = menu.presetsMenu;
+        assertAll(() -> assertSame(menu.presetSearchAction, presetsMenu.getItem(0).getAction()),
+                () -> assertSame(menu.presetSearchPrimitiveAction, presetsMenu.getItem(1).getAction()),
+                () -> assertTrue(presetsMenu.getItem(2).getAction() instanceof PreferencesAction),
+                () -> assertTrue(presetsMenu.getMenuComponent(3) instanceof JSeparator));
     }