diff --git a/src/org/openstreetmap/josm/data/osm/IRelation.java b/src/org/openstreetmap/josm/data/osm/IRelation.java index 6d59bd427a..9a7897d36f 100644 --- a/src/org/openstreetmap/josm/data/osm/IRelation.java +++ b/src/org/openstreetmap/josm/data/osm/IRelation.java @@ -2,9 +2,11 @@ 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,4 +140,20 @@ public interface IRelation> extends IPrimitive { 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 validate() { + return Collections.emptyList(); + } } diff --git a/src/org/openstreetmap/josm/data/osm/Relation.java b/src/org/openstreetmap/josm/data/osm/Relation.java index 3b12c48c80..1653b30f2e 100644 --- a/src/org/openstreetmap/josm/data/osm/Relation.java +++ b/src/org/openstreetmap/josm/data/osm/Relation.java @@ -14,6 +14,12 @@ import java.util.stream.Stream; 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,4 +573,15 @@ public final class Relation extends OsmPrimitive implements IRelation 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()); + } } diff --git a/src/org/openstreetmap/josm/data/validation/tests/RelationChecker.java b/src/org/openstreetmap/josm/data/validation/tests/RelationChecker.java index 959f73d1d0..1c10a7c575 100644 --- a/src/org/openstreetmap/josm/data/validation/tests/RelationChecker.java +++ b/src/org/openstreetmap/josm/data/validation/tests/RelationChecker.java @@ -154,7 +154,7 @@ public class RelationChecker extends Test implements TaggingPresetListener { .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) diff --git a/src/org/openstreetmap/josm/gui/dialogs/relation/GenericRelationEditor.java b/src/org/openstreetmap/josm/gui/dialogs/relation/GenericRelationEditor.java index 743e05f4e0..ccd3e41ad4 100644 --- a/src/org/openstreetmap/josm/gui/dialogs/relation/GenericRelationEditor.java +++ b/src/org/openstreetmap/josm/gui/dialogs/relation/GenericRelationEditor.java @@ -3,6 +3,7 @@ package org.openstreetmap.josm.gui.dialogs.relation; 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,9 +27,12 @@ import java.util.ArrayList; 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,6 +51,7 @@ import javax.swing.JTabbedPane; 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,10 +59,12 @@ import org.openstreetmap.josm.command.Command; 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,9 +112,12 @@ import org.openstreetmap.josm.gui.tagging.presets.TaggingPreset; 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,6 +142,7 @@ public class GenericRelationEditor extends RelationEditor implements CommandQueu 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,13 +273,17 @@ public class GenericRelationEditor extends RelationEditor implements CommandQueu 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,7 +291,7 @@ public class GenericRelationEditor extends RelationEditor implements CommandQueu 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,12 +422,82 @@ public class GenericRelationEditor extends RelationEditor implements CommandQueu * * @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> 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 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 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,7 +1086,7 @@ public class GenericRelationEditor extends RelationEditor implements CommandQueu @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); } } } diff --git a/src/org/openstreetmap/josm/gui/dialogs/relation/actions/ApplyAction.java b/src/org/openstreetmap/josm/gui/dialogs/relation/actions/ApplyAction.java index 7ddfae63ac..2a51047659 100644 --- a/src/org/openstreetmap/josm/gui/dialogs/relation/actions/ApplyAction.java +++ b/src/org/openstreetmap/josm/gui/dialogs/relation/actions/ApplyAction.java @@ -5,6 +5,7 @@ import static org.openstreetmap.josm.tools.I18n.tr; import java.awt.event.ActionEvent; +import org.openstreetmap.josm.data.osm.IRelation; import org.openstreetmap.josm.tools.ImageProvider; /** @@ -35,6 +36,7 @@ public class ApplyAction extends SavingAction { @Override public void updateEnabledState() { - setEnabled(isEditorDirty()); + final IRelation currentRelation = editorAccess.getChangedRelation(); + setEnabled(currentRelation.isUseful() && isEditorDirty()); } } diff --git a/src/org/openstreetmap/josm/gui/dialogs/relation/actions/CancelAction.java b/src/org/openstreetmap/josm/gui/dialogs/relation/actions/CancelAction.java index 6efdaeabdf..a51be40834 100644 --- a/src/org/openstreetmap/josm/gui/dialogs/relation/actions/CancelAction.java +++ b/src/org/openstreetmap/josm/gui/dialogs/relation/actions/CancelAction.java @@ -8,6 +8,7 @@ import java.awt.event.ActionEvent; 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,7 +48,7 @@ public class CancelAction extends SavingAction { 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,7 +61,7 @@ public class CancelAction extends SavingAction { hideEditor(); } - protected int confirmClosingByCancel() { + protected int confirmClosingByCancel(final IRelation newRelation) { ButtonSpec[] options = { new ButtonSpec( tr("Yes, save the changes and close"), @@ -82,6 +83,10 @@ public class CancelAction extends SavingAction { ) }; + // 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("The relation has been changed.

Do you want to save your changes?"), diff --git a/src/org/openstreetmap/josm/gui/dialogs/relation/actions/IRelationEditorActionAccess.java b/src/org/openstreetmap/josm/gui/dialogs/relation/actions/IRelationEditorActionAccess.java index cddedaf165..b1b79a3128 100644 --- a/src/org/openstreetmap/josm/gui/dialogs/relation/actions/IRelationEditorActionAccess.java +++ b/src/org/openstreetmap/josm/gui/dialogs/relation/actions/IRelationEditorActionAccess.java @@ -3,6 +3,8 @@ package org.openstreetmap.josm.gui.dialogs.relation.actions; 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,6 +67,23 @@ public interface IRelationEditorActionAccess { */ 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. diff --git a/src/org/openstreetmap/josm/gui/dialogs/relation/actions/SavingAction.java b/src/org/openstreetmap/josm/gui/dialogs/relation/actions/SavingAction.java index 26813f23c1..35ca1dcfb7 100644 --- a/src/org/openstreetmap/josm/gui/dialogs/relation/actions/SavingAction.java +++ b/src/org/openstreetmap/josm/gui/dialogs/relation/actions/SavingAction.java @@ -54,8 +54,8 @@ abstract class SavingAction extends AbstractRelationEditorAction { 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)); diff --git a/src/org/openstreetmap/josm/io/session/MarkerSessionImporter.java b/src/org/openstreetmap/josm/io/session/MarkerSessionImporter.java index 3a05dea2e0..e7d008324e 100644 --- a/src/org/openstreetmap/josm/io/session/MarkerSessionImporter.java +++ b/src/org/openstreetmap/josm/io/session/MarkerSessionImporter.java @@ -6,11 +6,13 @@ import static org.openstreetmap.josm.tools.I18n.tr; 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,6 +37,7 @@ public class MarkerSessionImporter implements SessionLayerImporter { } 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,7 +53,7 @@ public class MarkerSessionImporter implements SessionLayerImporter { return importData.getMarkerLayer(); } - } catch (XPathExpressionException e) { + } catch (XPathExpressionException | XPathFactoryConfigurationException e) { throw new IllegalDataException(e); } }