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..d0405bd938 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((Relation) 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..26184c7725 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..52f17e72e7 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,6 +27,7 @@ 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; @@ -47,6 +49,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; @@ -58,6 +61,7 @@ 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; @@ -108,6 +112,8 @@ import org.openstreetmap.josm.gui.tagging.presets.TaggingPresets; 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; @@ -276,7 +282,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 +413,78 @@ 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 TableModelListener listener = l -> { + final Relation newRelation = new Relation(); + this.tagEditorPanel.getModel().applyToPrimitive(newRelation); + this.memberTableModel.applyToRelation(newRelation); + updateOkPanel(newRelation, okButton, deleteButton); + updateErrorMessage(newRelation, errorLabel); + }; + 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(Relation 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 errorPane The pane (used for background colour) + * @param errorLabel The label to update + */ + private void updateErrorMessage(Relation newRelation, JLabel errorLabel) { + Collection errors = newRelation.validate(); + 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()); + } } /** 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..e36ae38182 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,15 @@ 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(); + final Relation newRelation; + if (getEditor().getRelation() != null) { + newRelation = getEditor().getRelation(); + } else { + newRelation = new Relation(); + getTagModel().applyToPrimitive(newRelation); + getMemberTableModel().applyToRelation(newRelation); + } + int ret = confirmClosingByCancel(newRelation); if (ret == 0) { //Yes, save the changes //copied from OKAction.run() Config.getPref().put("relation.editor.generic.lastrole", Utils.strip(tfRole.getText())); @@ -60,7 +69,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 +91,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/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));