Ticket #21825: 21825.5.patch

src/org/openstreetmap/josm/data/osm/IRelation.java

@@ -2 +2 @@
 package org.openstreetmap.josm.data.osm;
 
 import java.util.Collection;
+import java.util.Collections;
 import java.util.List;
 import java.util.stream.Collectors;
 
+import org.openstreetmap.josm.data.validation.TestError;
 import org.openstreetmap.josm.tools.Utils;
 
 /**
@@ -138 +140 @@
         return getMembers().stream().filter(rmv -> role.equals(rmv.getRole()))
                 .map(IRelationMember::getMember).collect(Collectors.toList());
     }
+
+    /**
+     * Check if this relation is useful
+     * @return {@code true} if this relation is useful
+     */
+    default boolean isUseful() {
+        return !this.isEmpty() && this.hasKey("type");
+    }
+
+    /**
+     * Check if this relation is valid
+     * @return A collection of errors, if any
+     */
+    default Collection<TestError> validate() {
+        return Collections.emptyList();
+    }
 }

src/org/openstreetmap/josm/data/osm/Relation.java

@@ -14 +14 @@
 
 import org.openstreetmap.josm.data.osm.visitor.OsmPrimitiveVisitor;
 import org.openstreetmap.josm.data.osm.visitor.PrimitiveVisitor;
+import org.openstreetmap.josm.data.validation.OsmValidator;
+import org.openstreetmap.josm.data.validation.TestError;
+import org.openstreetmap.josm.data.validation.tests.MultipolygonTest;
+import org.openstreetmap.josm.data.validation.tests.RelationChecker;
+import org.openstreetmap.josm.data.validation.tests.TurnrestrictionTest;
+import org.openstreetmap.josm.gui.progress.NullProgressMonitor;
 import org.openstreetmap.josm.spi.preferences.Config;
 import org.openstreetmap.josm.tools.CopyList;
 import org.openstreetmap.josm.tools.SubclassFilteredCollection;
@@ -567 +573 @@
     public UniqueIdGenerator getIdGenerator() {
         return idGenerator;
     }
+
+    @Override
+    public Collection<TestError> validate() {
+        return Stream.of(MultipolygonTest.class, TurnrestrictionTest.class, RelationChecker.class)
+                .map(OsmValidator::getTest).filter(test -> test.enabled || test.testBeforeUpload).flatMap(test -> {
+            test.startTest(NullProgressMonitor.INSTANCE);
+            test.visit(this);
+            test.endTest();
+            return test.getErrors().stream();
+        }).collect(Collectors.toList());
+    }
 }

src/org/openstreetmap/josm/data/validation/tests/RelationChecker.java

@@ -154 +154 @@
                     .message(tr("Route scheme is unspecified. Add {0} ({1}=public_transport; {2}=legacy)", "public_transport:version", "2", "1"))
                     .primitives(n)
                     .build());
-        } else if (n.hasKey("type") && allroles.isEmpty()) {
+        } else if ((n.hasKey("type") && allroles.isEmpty()) || !n.hasKey("type")) {
             errors.add(TestError.builder(this, Severity.OTHER, RELATION_UNKNOWN)
                     .message(tr("Relation type is unknown"))
                     .primitives(n)

src/org/openstreetmap/josm/gui/dialogs/relation/GenericRelationEditor.java

@@ -3 +3 @@
 
 import static org.openstreetmap.josm.gui.help.HelpUtil.ht;
 import static org.openstreetmap.josm.tools.I18n.tr;
+import static org.openstreetmap.josm.tools.I18n.trn;
 
 import java.awt.BorderLayout;
 import java.awt.Component;
@@ -26 +27 @@
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.Comparator;
 import java.util.EnumSet;
 import java.util.List;
 import java.util.Set;
+import java.util.concurrent.Future;
+import java.util.concurrent.atomic.AtomicReference;
 import java.util.stream.Collectors;
 
 import javax.swing.AbstractAction;
@@ -47 +51 @@
 import javax.swing.JTable;
 import javax.swing.JToolBar;
 import javax.swing.KeyStroke;
+import javax.swing.event.TableModelListener;
 
 import org.openstreetmap.josm.actions.JosmAction;
 import org.openstreetmap.josm.command.ChangeMembersCommand;
@@ -54 +59 @@
 import org.openstreetmap.josm.data.UndoRedoHandler;
 import org.openstreetmap.josm.data.UndoRedoHandler.CommandQueueListener;
 import org.openstreetmap.josm.data.osm.DefaultNameFormatter;
+import org.openstreetmap.josm.data.osm.IRelation;
 import org.openstreetmap.josm.data.osm.OsmPrimitive;
 import org.openstreetmap.josm.data.osm.Relation;
 import org.openstreetmap.josm.data.osm.RelationMember;
 import org.openstreetmap.josm.data.osm.Tag;
+import org.openstreetmap.josm.data.validation.TestError;
 import org.openstreetmap.josm.data.validation.tests.RelationChecker;
 import org.openstreetmap.josm.gui.ConditionalOptionPaneUtil;
 import org.openstreetmap.josm.gui.MainApplication;
@@ -105 +112 @@
 import org.openstreetmap.josm.gui.tagging.presets.TaggingPresetHandler;
 import org.openstreetmap.josm.gui.tagging.presets.TaggingPresetType;
 import org.openstreetmap.josm.gui.tagging.presets.TaggingPresets;
+import org.openstreetmap.josm.gui.util.GuiHelper;
 import org.openstreetmap.josm.gui.util.WindowGeometry;
 import org.openstreetmap.josm.spi.preferences.Config;
 import org.openstreetmap.josm.tools.CheckParameterUtil;
+import org.openstreetmap.josm.tools.GBC;
+import org.openstreetmap.josm.tools.ImageProvider;
 import org.openstreetmap.josm.tools.InputMapUtils;
 import org.openstreetmap.josm.tools.Logging;
 import org.openstreetmap.josm.tools.Shortcut;
@@ -132 +142 @@
     private final SelectionTableModel selectionTableModel;
 
     private final AutoCompletingTextField tfRole;
+    private final RelationEditorActionAccess actionAccess;
 
     /**
      * the menu item in the windows menu. Required to properly hide on dialog close.
@@ -262 +273 @@
             selectedTabPane = sourceTabbedPane.getSelectedComponent();
         });
 
-        IRelationEditorActionAccess actionAccess = new RelationEditorActionAccess();
+        actionAccess = new RelationEditorActionAccess();
 
         refreshAction = new RefreshAction(actionAccess);
         applyAction = new ApplyAction(actionAccess);
         selectAction = new SelectAction(actionAccess);
         duplicateAction = new DuplicateRelationAction(actionAccess);
         deleteAction = new DeleteCurrentRelationAction(actionAccess);
+
+        this.memberTableModel.addTableModelListener(applyAction);
+        this.tagEditorPanel.getModel().addTableModelListener(applyAction);
+
         addPropertyChangeListener(deleteAction);
 
         okAction = new OKAction(actionAccess);
@@ -276 +291 @@
 
         getContentPane().add(buildToolBar(refreshAction, applyAction, selectAction, duplicateAction, deleteAction), BorderLayout.NORTH);
         getContentPane().add(tabbedPane, BorderLayout.CENTER);
-        getContentPane().add(buildOkCancelButtonPanel(okAction, cancelAction), BorderLayout.SOUTH);
+        getContentPane().add(buildOkCancelButtonPanel(okAction, deleteAction, cancelAction), BorderLayout.SOUTH);
 
         setSize(findMaxDialogSize());
 
@@ -407 +422 @@
      *
      * @return the panel with the OK and the Cancel button
      */
-    protected static JPanel buildOkCancelButtonPanel(OKAction okAction, CancelAction cancelAction) {
-        JPanel pnl = new JPanel(new FlowLayout(FlowLayout.CENTER));
-        pnl.add(new JButton(okAction));
+    protected JPanel buildOkCancelButtonPanel(OKAction okAction, DeleteCurrentRelationAction deleteAction,
+            CancelAction cancelAction) {
+        JPanel mainPanel = new JPanel(new GridBagLayout());
+        final JLabel errorLabel = new JLabel(" ");
+        final JPanel pnl = new JPanel(new FlowLayout(FlowLayout.CENTER));
+        mainPanel.add(errorLabel, GBC.eol().anchor(GridBagConstraints.CENTER));
+        mainPanel.add(pnl, GBC.eol().anchor(GridBagConstraints.CENTER));
+        final JButton okButton = new JButton(okAction);
+        final JButton deleteButton = new JButton(deleteAction);
+        okButton.setPreferredSize(deleteButton.getPreferredSize());
+        pnl.add(okButton);
+        pnl.add(deleteButton);
         pnl.add(new JButton(cancelAction));
         pnl.add(new JButton(new ContextSensitiveHelpAction(ht("/Dialog/RelationEditor"))));
-        return pnl;
+        // Keep users from saving invalid relations -- a relation MUST have at least a tag with the key "type"
+        // AND must contain at least one other OSM object.
+        final AtomicReference<Future<?>> testErrorsUpdate = new AtomicReference<>();
+        final TableModelListener listener = l -> {
+            final IRelation<?> newRelation = this.actionAccess.getChangedRelation();
+            updateOkPanel(newRelation, okButton, deleteButton);
+            final Future<?> oldFuture = testErrorsUpdate.getAndSet(MainApplication.worker.submit(() -> {
+                GuiHelper.runInEDTAndWait(() -> errorLabel.setText(tr("Validation in progress")));
+                final Collection<TestError> testErrors = newRelation.validate();
+                GuiHelper.runInEDTAndWait(() -> updateErrorMessage(testErrors, errorLabel));
+            }));
+            if (oldFuture != null) {
+                oldFuture.cancel(true);
+            }
+        };
+        listener.tableChanged(null);
+        this.memberTableModel.addTableModelListener(listener);
+        this.tagEditorPanel.getModel().addTableModelListener(listener);
+        return mainPanel;
+    }
+
+    /**
+     * Update the OK panel area
+     * @param newRelation What the new relation would "look" like if it were to be saved now
+     * @param okButton The OK button
+     * @param deleteButton The delete button
+     */
+    private void updateOkPanel(IRelation<?> newRelation, JButton okButton, JButton deleteButton) {
+        okButton.setVisible(newRelation.isUseful() || this.getRelationSnapshot() == null);
+        deleteButton.setVisible(!newRelation.isUseful() && this.getRelationSnapshot() != null);
+        if (this.getRelationSnapshot() == null && !newRelation.isUseful()) {
+            okButton.setText(tr("Delete"));
+        } else {
+            okButton.setText(tr("OK"));
+        }
+    }
+
+    /**
+     * Update the error message
+     * @param newRelation What the new relation would "look" like if it were to be saved now
+     * @param errorLabel The label to update
+     */
+    private void updateErrorMessage(Collection<TestError> errors, JLabel errorLabel) {
+        if (errors.isEmpty()) {
+            // Setting " " helps keep the label in place for layout calculations
+            errorLabel.setText(" ");
+            errorLabel.setIcon(null);
+        } else {
+            final TestError error = errors.stream()
+                    .min(Comparator.comparingInt(testError -> testError.getSeverity().getLevel()))
+                    .orElse(errors.iterator().next());
+            final StringBuilder sb = new StringBuilder();
+            if (error.getDescription() != null) {
+                sb.append(error.getDescription()).append(": ");
+            }
+            sb.append(error.getMessage());
+            if (errors.size() > 1) {
+                sb.append(trn(" with {0} more problem", " with {0} more problems", errors.size() - 1, errors.size() - 1));
+            }
+            errorLabel.setIcon(ImageProvider.get("data", error.getSeverity().getIcon()));
+            errorLabel.setText(sb.toString());
+        }
     }
 
     /**
@@ -1001 +1086 @@
         @Override
         public void mouseClicked(MouseEvent e) {
             if (e.getButton() == MouseEvent.BUTTON1 && e.getClickCount() == 2) {
-                new EditAction(new RelationEditorActionAccess()).actionPerformed(null);
+                new EditAction(actionAccess).actionPerformed(null);
             }
         }
     }

src/org/openstreetmap/josm/gui/dialogs/relation/actions/ApplyAction.java

@@ -5 +5 @@
 
 import java.awt.event.ActionEvent;
 
+import org.openstreetmap.josm.data.osm.IRelation;
 import org.openstreetmap.josm.tools.ImageProvider;
 
 /**
@@ -35 +36 @@
 
     @Override
     public void updateEnabledState() {
-        setEnabled(isEditorDirty());
+        final IRelation<?> currentRelation = editorAccess.getChangedRelation();
+        setEnabled(currentRelation.isUseful() && isEditorDirty());
     }
 }

src/org/openstreetmap/josm/gui/dialogs/relation/actions/CancelAction.java

@@ -8 +8 @@
 import javax.swing.JOptionPane;
 import javax.swing.RootPaneContainer;
 
+import org.openstreetmap.josm.data.osm.IRelation;
 import org.openstreetmap.josm.data.osm.Relation;
 import org.openstreetmap.josm.gui.HelpAwareOptionPane;
 import org.openstreetmap.josm.gui.HelpAwareOptionPane.ButtonSpec;
@@ -47 +48 @@
         if ((!getMemberTableModel().hasSameMembersAs(snapshot) || getTagModel().isDirty())
          && !(snapshot == null && getTagModel().getTags().isEmpty())) {
             //give the user a chance to save the changes
-            int ret = confirmClosingByCancel();
+            int ret = confirmClosingByCancel(this.editorAccess.getChangedRelation());
             if (ret == 0) { //Yes, save the changes
                 //copied from OKAction.run()
                 Config.getPref().put("relation.editor.generic.lastrole", Utils.strip(tfRole.getText()));
@@ -60 +61 @@
         hideEditor();
     }
 
-    protected int confirmClosingByCancel() {
+    protected int confirmClosingByCancel(final IRelation<?> newRelation) {
         ButtonSpec[] options = {
                 new ButtonSpec(
                         tr("Yes, save the changes and close"),
@@ -82 +83 @@
                 )
         };
 
+        // Keep users from saving invalid relations -- a relation MUST have at least a tag with the key "type"
+        // AND must contain at least one other OSM object.
+        options[0].setEnabled(newRelation.isUseful());
+
         return HelpAwareOptionPane.showOptionDialog(
                 MainApplication.getMainFrame(),
                 tr("<html>The relation has been changed.<br><br>Do you want to save your changes?</html>"),

src/org/openstreetmap/josm/gui/dialogs/relation/actions/IRelationEditorActionAccess.java

@@ -3 +3 @@
 
 import javax.swing.Action;
 
+import org.openstreetmap.josm.data.osm.IRelation;
+import org.openstreetmap.josm.data.osm.Relation;
 import org.openstreetmap.josm.gui.dialogs.relation.IRelationEditor;
 import org.openstreetmap.josm.gui.dialogs.relation.MemberTable;
 import org.openstreetmap.josm.gui.dialogs.relation.MemberTableModel;
@@ -65 +67 @@
      */
     TagEditorModel getTagModel();
 
+    /**
+     * Get the changed relation
+     * @return The changed relation (note: will not be part of a dataset). This should never be {@code null}.
+     * @since xxx
+     */
+    default IRelation<?> getChangedRelation() {
+        final Relation newRelation;
+        if (getEditor().getRelation() != null) {
+            newRelation = new Relation(getEditor().getRelation());
+        } else {
+            newRelation = new Relation();
+        }
+        getTagModel().applyToPrimitive(newRelation);
+        getMemberTableModel().applyToRelation(newRelation);
+        return newRelation;
+    }
+
     /**
      * Get the text field that is used to edit the role.
      * @return The role text field.

src/org/openstreetmap/josm/gui/dialogs/relation/actions/SavingAction.java

@@ -54 +54 @@
         tagEditorModel.applyToPrimitive(newRelation);
         getMemberTableModel().applyToRelation(newRelation);
         // If the user wanted to create a new relation, but hasn't added any members or
-        // tags, don't add an empty relation
-        if (newRelation.isEmpty() && !newRelation.hasKeys())
+        // tags (specifically the "type" tag), don't add the relation
+        if (!newRelation.isUseful())
             return;
         UndoRedoHandler.getInstance().add(new AddCommand(getLayer().getDataSet(), newRelation));
 

src/org/openstreetmap/josm/io/session/MarkerSessionImporter.java

@@ -6 +6 @@
 import java.io.IOException;
 import java.io.InputStream;
 
+import javax.xml.XMLConstants;
 import javax.xml.xpath.XPath;
 import javax.xml.xpath.XPathConstants;
 import javax.xml.xpath.XPathExpression;
 import javax.xml.xpath.XPathExpressionException;
 import javax.xml.xpath.XPathFactory;
+import javax.xml.xpath.XPathFactoryConfigurationException;
 
 import org.openstreetmap.josm.gui.io.importexport.GpxImporter;
 import org.openstreetmap.josm.gui.layer.Layer;
@@ -35 +37 @@
         }
         try {
             XPathFactory xPathFactory = XPathFactory.newInstance();
+            xPathFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
             XPath xpath = xPathFactory.newXPath();
             XPathExpression fileExp = xpath.compile("file/text()");
             String fileStr = (String) fileExp.evaluate(elem, XPathConstants.STRING);
@@ -50 +53 @@
 
                 return importData.getMarkerLayer();
             }
-        } catch (XPathExpressionException e) {
+        } catch (XPathExpressionException | XPathFactoryConfigurationException e) {
             throw new IllegalDataException(e);
         }
     }