Ticket #13429: patch-validator-cleanup-and-use-listener.patch

src/org/openstreetmap/josm/actions/ValidateAction.java

@@ -168 +168 @@
                 errors.addAll(test.getErrors());
             }
             tests = null;
-            if (Main.pref.getBoolean(ValidatorPreference.PREF_USE_IGNORE, true)) {
+            if (ValidatorPreference.PREF_USE_IGNORE.get()) {
                 getProgressMonitor().subTask(tr("Updating ignored errors ..."));
                 for (TestError error : errors) {
                     if (canceled) return;

src/org/openstreetmap/josm/actions/upload/ValidateUploadHook.java

@@ -60 +60 @@
             test.startTest(null);
             test.visit(selection);
             test.endTest();
-            if (ValidatorPreference.PREF_OTHER.get() &&
-                Main.pref.getBoolean(ValidatorPreference.PREF_OTHER_UPLOAD, false)) {
+            if (ValidatorPreference.PREF_OTHER.get() && ValidatorPreference.PREF_OTHER_UPLOAD.get()) {
                 errors.addAll(test.getErrors());
             } else {
                 for (TestError e : test.getErrors()) {
@@ -82 +81 @@
         if (errors.isEmpty())
             return true;
 
-        if (Main.pref.getBoolean(ValidatorPreference.PREF_USE_IGNORE, true)) {
+        if (ValidatorPreference.PREF_USE_IGNORE.get()) {
             int nume = 0;
             for (TestError error : errors) {
                 List<String> s = new ArrayList<>();

src/org/openstreetmap/josm/data/validation/OsmValidator.java

@@ -182 +182 @@
 
     private static void loadIgnoredErrors() {
         ignoredErrors.clear();
-        if (Main.pref.getBoolean(ValidatorPreference.PREF_USE_IGNORE, true)) {
+        if (ValidatorPreference.PREF_USE_IGNORE.get()) {
             Path path = Paths.get(getValidatorDir()).resolve("ignorederrors");
             if (Files.exists(path)) {
                 try {
@@ -216 +216 @@
     }
 
     public static synchronized void initializeErrorLayer() {
-        if (!Main.pref.getBoolean(ValidatorPreference.PREF_LAYER, true))
+        if (!ValidatorPreference.PREF_LAYER.get())
             return;
         if (errorLayer == null) {
             errorLayer = new ValidatorLayer();

src/org/openstreetmap/josm/data/validation/TestError.java

@@ -268 +268 @@
         return tester;
     }
 
+    /**
+     * Set the tester that raised the error.
+     * @param tester te tester
+     */
     public void setTester(Test tester) {
         this.tester = tester;
     }

src/org/openstreetmap/josm/gui/dialogs/ValidatorDialog.java

@@ -150 +150 @@
         fixButton.setEnabled(false);
         buttons.add(fixButton);
 
-        if (Main.pref.getBoolean(ValidatorPreference.PREF_USE_IGNORE, true)) {
+        if (ValidatorPreference.PREF_USE_IGNORE.get()) {
             ignoreButton = new SideButton(new AbstractAction() {
                 {
                     putValue(NAME, tr("Ignore"));
@@ -192 +192 @@
             tree.setVisible(v);
         }
         super.setVisible(v);
-        Main.map.repaint();
     }
 
     /**

src/org/openstreetmap/josm/gui/dialogs/validator/ValidatorTreePanel.java

@@ -10 +10 @@
 import java.util.Collections;
 import java.util.EnumMap;
 import java.util.Enumeration;
-import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
 import java.util.Set;
 import java.util.function.Predicate;
+import java.util.stream.Collectors;
 
 import javax.swing.JTree;
 import javax.swing.ToolTipManager;
@@ -35 +35 @@
 import org.openstreetmap.josm.gui.preferences.validator.ValidatorPreference;
 import org.openstreetmap.josm.gui.util.GuiHelper;
 import org.openstreetmap.josm.tools.Destroyable;
-import org.openstreetmap.josm.tools.MultiMap;
+import org.openstreetmap.josm.tools.ListenerList;
 
 /**
  * A panel that displays the error tree. The selection manager
@@ -72 +72 @@
      */
     private transient Set<? extends OsmPrimitive> filter;
 
-    /** a counter to check if tree has been rebuild */
-    private int updateCount;
+    private final ListenerList<Runnable> invalidationListeners = ListenerList.create();
 
     /**
      * Constructor
@@ -134 +133 @@
             valTreeModel.setRoot(new DefaultMutableTreeNode());
         }
         super.setVisible(v);
+        invalidationListeners.fireEvent(Runnable::run);
     }
 
     /**
      * Builds the errors tree
      */
     public void buildTree() {
-        updateCount++;
         final DefaultMutableTreeNode rootNode = new DefaultMutableTreeNode();
 
         if (errors == null || errors.isEmpty()) {
@@ -171 +170 @@
             }
         }
 
-        Map<Severity, MultiMap<String, TestError>> errorTree = new EnumMap<>(Severity.class);
-        Map<Severity, HashMap<String, MultiMap<String, TestError>>> errorTreeDeep = new EnumMap<>(Severity.class);
-        for (Severity s : Severity.values()) {
-            errorTree.put(s, new MultiMap<String, TestError>(20));
-            errorTreeDeep.put(s, new HashMap<String, MultiMap<String, TestError>>());
+        Predicate<TestError> filterToUse = e -> !e.isIgnored();
+        if (!ValidatorPreference.PREF_OTHER.get()) {
+            filterToUse = filterToUse.and(e -> e.getSeverity() != Severity.OTHER);
         }
-
-        final Boolean other = ValidatorPreference.PREF_OTHER.get();
-        for (TestError e : errors) {
-            if (e.isIgnored()) {
-                continue;
-            }
-            Severity s = e.getSeverity();
-            if (!other && s == Severity.OTHER) {
-                continue;
-            }
-            String d = e.getDescription();
-            String m = e.getMessage();
-            if (filter != null) {
-                boolean found = false;
-                for (OsmPrimitive p : e.getPrimitives()) {
-                    if (filter.contains(p)) {
-                        found = true;
-                        break;
-                    }
-                }
-                if (!found) {
-                    continue;
-                }
-            }
-            if (d != null) {
-                MultiMap<String, TestError> b = errorTreeDeep.get(s).get(m);
-                if (b == null) {
-                    b = new MultiMap<>(20);
-                    errorTreeDeep.get(s).put(m, b);
-                }
-                b.put(d, e);
-            } else {
-                errorTree.get(s).put(m, e);
-            }
+        if (filter != null) {
+            filterToUse = filterToUse.and(e -> e.getPrimitives().stream().anyMatch(filter::contains));
         }
+        Map<Severity, Map<String, Map<String, List<TestError>>>> errorTreeDeep
+            = errors.stream().filter(filterToUse).collect(
+                    Collectors.groupingBy(e -> e.getSeverity(), () -> new EnumMap<>(Severity.class),
+                            Collectors.groupingBy(e -> e.getDescription() == null ? "" : e.getDescription(),
+                                    Collectors.groupingBy(e -> e.getMessage()))));
 
         List<TreePath> expandedPaths = new ArrayList<>();
-        for (Severity s : Severity.values()) {
-            MultiMap<String, TestError> severityErrors = errorTree.get(s);
-            Map<String, MultiMap<String, TestError>> severityErrorsDeep = errorTreeDeep.get(s);
-            if (severityErrors.isEmpty() && severityErrorsDeep.isEmpty()) {
-                continue;
-            }
-
+        errorTreeDeep.forEach((s, severityErrorsDeep) -> {
             // Severity node
             DefaultMutableTreeNode severityNode = new GroupTreeNode(s);
             rootNode.add(severityNode);
@@ -229 +193 @@
                 expandedPaths.add(new TreePath(new Object[] {rootNode, severityNode}));
             }
 
-            for (Entry<String, Set<TestError>> msgErrors : severityErrors.entrySet()) {
-                // Message node
-                Set<TestError> errs = msgErrors.getValue();
-                String msg = tr("{0} ({1})", msgErrors.getKey(), errs.size());
-                DefaultMutableTreeNode messageNode = new DefaultMutableTreeNode(msg);
-                severityNode.add(messageNode);
+            Map<String, List<TestError>> severityErrors = severityErrorsDeep.get("");
+            if (severityErrors != null) {
+                for (Entry<String, List<TestError>> msgErrors : severityErrors.entrySet()) {
+                    // Message node
+                    List<TestError> errs = msgErrors.getValue();
+                    String msg = tr("{0} ({1})", msgErrors.getKey(), errs.size());
+                    DefaultMutableTreeNode messageNode = new DefaultMutableTreeNode(msg);
+                    severityNode.add(messageNode);
 
-                if (oldSelectedRows.contains(msgErrors.getKey())) {
-                    expandedPaths.add(new TreePath(new Object[] {rootNode, severityNode, messageNode}));
-                }
+                    if (oldSelectedRows.contains(msgErrors.getKey())) {
+                        expandedPaths.add(new TreePath(new Object[] {rootNode, severityNode, messageNode}));
+                    }
 
-                for (TestError error : errs) {
-                    // Error node
-                    DefaultMutableTreeNode errorNode = new DefaultMutableTreeNode(error);
-                    messageNode.add(errorNode);
+                    errs.stream().map(DefaultMutableTreeNode::new).forEach(messageNode::add);
                 }
             }
-            for (Entry<String, MultiMap<String, TestError>> bag : severityErrorsDeep.entrySet()) {
+
+            severityErrorsDeep.forEach((description, errorlist) -> {
+                if (description.isEmpty()) {
+                    return;
+                }
                 // Group node
-                MultiMap<String, TestError> errorlist = bag.getValue();
-                DefaultMutableTreeNode groupNode = null;
+                DefaultMutableTreeNode groupNode;
                 if (errorlist.size() > 1) {
-                    groupNode = new GroupTreeNode(bag.getKey());
+                    groupNode = new GroupTreeNode(description);
                     severityNode.add(groupNode);
-                    if (oldSelectedRows.contains(bag.getKey())) {
+                    if (oldSelectedRows.contains(description)) {
                         expandedPaths.add(new TreePath(new Object[] {rootNode, severityNode, groupNode}));
                     }
+                } else {
+                    groupNode = null;
                 }
 
-                for (Entry<String, Set<TestError>> msgErrors : errorlist.entrySet()) {
+                errorlist.forEach((message, errs) -> {
                     // Message node
-                    Set<TestError> errs = msgErrors.getValue();
                     String msg;
                     if (groupNode != null) {
-                        msg = tr("{0} ({1})", msgErrors.getKey(), errs.size());
+                        msg = tr("{0} ({1})", message, errs.size());
                     } else {
-                        msg = tr("{0} - {1} ({2})", msgErrors.getKey(), bag.getKey(), errs.size());
+                        msg = tr("{0} - {1} ({2})", message, description, errs.size());
                     }
                     DefaultMutableTreeNode messageNode = new DefaultMutableTreeNode(msg);
                     if (groupNode != null) {
@@ -274 +241 @@
                         severityNode.add(messageNode);
                     }
 
-                    if (oldSelectedRows.contains(msgErrors.getKey())) {
+                    if (oldSelectedRows.contains(message)) {
                         if (groupNode != null) {
                             expandedPaths.add(new TreePath(new Object[] {rootNode, severityNode, groupNode, messageNode}));
                         } else {
@@ -282 +249 @@
                         }
                     }
 
-                    for (TestError error : errs) {
-                        // Error node
-                        DefaultMutableTreeNode errorNode = new DefaultMutableTreeNode(error);
-                        messageNode.add(errorNode);
-                    }
-                }
-            }
-        }
+                    errs.stream().map(DefaultMutableTreeNode::new).forEach(messageNode::add);
+                });
+            });
+        });
 
         valTreeModel.setRoot(rootNode);
         for (TreePath path : expandedPaths) {
             this.expandPath(path);
         }
+
+        invalidationListeners.fireEvent(Runnable::run);
+    }
+
+    /**
+     * Add a new invalidation listener
+     * @param listener The listener
+     */
+    public void addInvalidationListener(Runnable listener) {
+        invalidationListeners.addListener(listener);
+    }
+
+    /**
+     * Remove an invalidation listener
+     * @param listener The listener
+     */
+    public void removeInvalidationListener(Runnable listener) {
+        invalidationListeners.removeListener(listener);
     }
 
     /**
@@ -425 +406 @@
         return (DefaultMutableTreeNode) valTreeModel.getRoot();
     }
 
-    /**
-     * Returns a value to check if tree has been rebuild
-     * @return the current counter
-     */
-    public int getUpdateCount() {
-        return updateCount;
-    }
-
     private void clearErrors() {
         if (errors != null) {
             DataSet ds = Main.getLayerManager().getEditDataSet();

src/org/openstreetmap/josm/gui/layer/ValidatorLayer.java

@@ -40 +40 @@
  * @since 10386 (new LayerChangeListener interface)
  */
 public class ValidatorLayer extends Layer implements LayerChangeListener {
-
-    private int updateCount = -1;
+    private final Runnable invalidator = this::invalidate;
 
     /**
      * Constructs a new Validator layer
@@ -49 +48 @@
     public ValidatorLayer() {
         super(tr("Validation errors"));
         Main.getLayerManager().addLayerChangeListener(this);
+        Main.map.validatorDialog.tree.addInvalidationListener(invalidator);
     }
 
     /**
@@ -67 +67 @@
     @SuppressWarnings("unchecked")
     @Override
     public void paint(final Graphics2D g, final MapView mv, Bounds bounds) {
-        updateCount = Main.map.validatorDialog.tree.getUpdateCount();
         DefaultMutableTreeNode root = Main.map.validatorDialog.tree.getRoot();
         if (root == null || root.getChildCount() == 0)
             return;
@@ -123 +122 @@
     }
 
     @Override
-    public boolean isChanged() {
-        return updateCount != Main.map.validatorDialog.tree.getUpdateCount();
-    }
-
-    @Override
     public void visitBoundingBox(BoundingXYVisitor v) {
         // Do nothing
     }
@@ -167 +161 @@
         if (e.getRemovedLayer() instanceof OsmDataLayer && e.getSource().getLayersOfType(OsmDataLayer.class).size() <= 1) {
             e.scheduleRemoval(Collections.singleton(this));
         } else if (e.getRemovedLayer() == this) {
-            Main.getLayerManager().removeLayerChangeListener(this);
             OsmValidator.errorLayer = null;
         }
     }
@@ -176 +169 @@
     public LayerPositionStrategy getDefaultLayerPosition() {
         return LayerPositionStrategy.IN_FRONT;
     }
+
+    @Override
+    public void destroy() {
+        Main.map.validatorDialog.tree.removeInvalidationListener(invalidator);
+        Main.getLayerManager().removeLayerChangeListener(this);
+        super.destroy();
+    }
 }

src/org/openstreetmap/josm/gui/preferences/validator/ValidatorPreference.java

@@ -39 +39 @@
     public static final String PREFIX = "validator";
 
     /** The preferences key for error layer */
-    public static final String PREF_LAYER = PREFIX + ".layer";
+    public static final BooleanProperty PREF_LAYER = new BooleanProperty(PREFIX + ".layer", true);
 
     /** The preferences key for enabled tests */
     public static final String PREF_SKIP_TESTS = PREFIX + ".skip";
 
     /** The preferences key for enabled tests */
-    public static final String PREF_USE_IGNORE = PREFIX + ".ignore";
+    public static final BooleanProperty PREF_USE_IGNORE = new BooleanProperty(PREFIX + ".ignore", true);
 
     /** The preferences key for enabled tests before upload*/
     public static final String PREF_SKIP_TESTS_BEFORE_UPLOAD = PREFIX + ".skipBeforeUpload";
 
     /** The preferences key for ignored severity other on upload */
-    public static final String PREF_OTHER_UPLOAD = PREFIX + ".otherUpload";
+    public static final BooleanProperty PREF_OTHER_UPLOAD = new BooleanProperty(PREFIX + ".otherUpload", false);
 
     /** The preferences for ignored severity other */
     public static final BooleanProperty PREF_OTHER = new BooleanProperty(PREFIX + ".other", false);

src/org/openstreetmap/josm/gui/preferences/validator/ValidatorTestsPreference.java

@@ -57 +57 @@
         JPanel testPanel = new VerticallyScrollablePanel(new GridBagLayout());
         testPanel.setBorder(BorderFactory.createEmptyBorder(5, 5, 5, 5));
 
-        prefUseIgnore = new JCheckBox(tr("Use ignore list."), Main.pref.getBoolean(ValidatorPreference.PREF_USE_IGNORE, true));
+        prefUseIgnore = new JCheckBox(tr("Use ignore list."), ValidatorPreference.PREF_USE_IGNORE.get());
         prefUseIgnore.setToolTipText(tr("Use the ignore list to suppress warnings."));
         testPanel.add(prefUseIgnore, GBC.eol());
 
-        prefUseLayer = new JCheckBox(tr("Use error layer."), Main.pref.getBoolean(ValidatorPreference.PREF_LAYER, true));
+        prefUseLayer = new JCheckBox(tr("Use error layer."), ValidatorPreference.PREF_LAYER.get());
         prefUseLayer.setToolTipText(tr("Use the error layer to display problematic elements."));
         testPanel.add(prefUseLayer, GBC.eol());
 
@@ -70 +70 @@
         testPanel.add(prefOther, GBC.eol());
 
         prefOtherUpload = new JCheckBox(tr("Show informational level on upload."),
-                Main.pref.getBoolean(ValidatorPreference.PREF_OTHER_UPLOAD, false));
+                ValidatorPreference.PREF_OTHER_UPLOAD.get());
         prefOtherUpload.setToolTipText(tr("Show the informational tests in the upload check windows."));
         testPanel.add(prefOtherUpload, GBC.eol());
 
@@ -116 +116 @@
 
         Main.pref.putCollection(ValidatorPreference.PREF_SKIP_TESTS, tests);
         Main.pref.putCollection(ValidatorPreference.PREF_SKIP_TESTS_BEFORE_UPLOAD, testsBeforeUpload);
-        Main.pref.put(ValidatorPreference.PREF_USE_IGNORE, prefUseIgnore.isSelected());
+        ValidatorPreference.PREF_USE_IGNORE.put(prefUseIgnore.isSelected());
         ValidatorPreference.PREF_OTHER.put(prefOther.isSelected());
-        Main.pref.put(ValidatorPreference.PREF_OTHER_UPLOAD, prefOtherUpload.isSelected());
-        Main.pref.put(ValidatorPreference.PREF_LAYER, prefUseLayer.isSelected());
+        ValidatorPreference.PREF_OTHER_UPLOAD.put(prefOtherUpload.isSelected());
+        ValidatorPreference.PREF_LAYER.put(prefUseLayer.isSelected());
         return false;
     }
 

test/unit/org/openstreetmap/josm/gui/dialogs/validator/ValidatorTreePanelTest.java

@@ -43 +43 @@
                 new TestError(null, Severity.ERROR, "err", 0, new Node(1)),
                 new TestError(null, Severity.WARNING, "warn", 0, new Node(2)))));
         assertNotNull(vtp);
-        assertEquals(1, vtp.getUpdateCount());
         assertEquals(2, vtp.getErrors().size());
         vtp.setVisible(true);
         vtp.setVisible(false);