Changeset 19078 in josm for trunk/src

Timestamp:

2024-05-13T20:34:55+02:00 (6 months ago)

Author:

taylor.smock

Message:

Fix #4142: Track fully downloaded objects (patch by stoecker, GerdP, and myself)

The serialization move from PrimitiveData to AbstractPrimitive should be
reverted prior to 24.05 (see #23677).

The serialization move was required since we want to ensure that all downstream
users of AbstractPrimitive were not using the flags field, which was done by
making the field private instead of protected. They may still be using that
field (via updateFlags) which would not be caught by compile-time or runtime
errors.

Additionally, a good chunk of common functionality was moved up from
OsmPrimitive, even though much of it wasn't useful for PrimitiveData.

Location:

trunk/src/org/openstreetmap/josm

Files:

• actions/AlignInCircleAction.java (modified) (2 diffs)
• actions/CombineWayAction.java (modified) (1 diff)
• actions/DeleteAction.java (modified) (2 diffs)
• actions/JoinAreasAction.java (modified) (1 diff)
• actions/SplitWayAction.java (modified) (1 diff)
• actions/downloadtasks/DownloadReferrersTask.java (modified) (1 diff)
• command/Command.java (modified) (1 diff)
• command/DeleteCommand.java (modified) (1 diff)
• data/osm/AbstractPrimitive.java (modified) (5 diffs)
• data/osm/DataSetMerger.java (modified) (1 diff)
• data/osm/IPrimitive.java (modified) (2 diffs)
• data/osm/OsmPrimitive.java (modified) (5 diffs)
• data/osm/PrimitiveData.java (modified) (2 diffs)
• data/validation/tests/BarriersEntrances.java (modified) (1 diff)
• data/vector/VectorPrimitive.java (modified) (1 diff)
• gui/dialogs/InspectPrimitiveDataText.java (modified) (1 diff)
• io/BoundingBoxDownloader.java (modified) (3 diffs)
• io/OverpassDownloadReader.java (modified) (4 diffs)

trunk/src/org/openstreetmap/josm/actions/AlignInCircleAction.java

@@ -2 +2 @@
 package org.openstreetmap.josm.actions;
 
+import static java.util.function.Predicate.not;
 import static org.openstreetmap.josm.gui.help.HelpUtil.ht;
 import static org.openstreetmap.josm.tools.I18n.tr;
@@ -272 +273 @@
         fixNodes.addAll(collectNodesWithExternReferrers(ways));
 
-        // Check if one or more nodes are outside of download area
-        if (nodes.stream().anyMatch(Node::isOutsideDownloadArea))
-            throw new InvalidSelection(tr("One or more nodes involved in this action is outside of the downloaded area."));
+        // Check if one or more nodes does not have all parents available
+        if (nodes.stream().anyMatch(not(Node::isReferrersDownloaded)))
+            throw new InvalidSelection(tr("One or more nodes involved in this action may have additional referrers."));
 
 

trunk/src/org/openstreetmap/josm/actions/CombineWayAction.java

@@ -285 +285 @@
             final Node[] endnodes = {w.firstNode(), w.lastNode()};
             for (Node n : endnodes) {
-                if (!n.isNew() && n.isOutsideDownloadArea() && !endNodesOutside.add(n)) {
-                    new Notification(tr("Combine ways refused<br>" + "(A shared node is outside of the download area)"))
+                if (!n.isNew() && !n.isReferrersDownloaded() && !endNodesOutside.add(n)) {
+                    new Notification(tr("Combine ways refused<br>" + "(A shared node may have additional referrers)"))
                             .setIcon(JOptionPane.INFORMATION_MESSAGE).show();
                     return;

trunk/src/org/openstreetmap/josm/actions/DeleteAction.java

@@ -17 +17 @@
 import org.openstreetmap.josm.command.DeleteCommand.DeletionCallback;
 import org.openstreetmap.josm.data.osm.DefaultNameFormatter;
+import org.openstreetmap.josm.data.osm.INode;
+import org.openstreetmap.josm.data.osm.IRelation;
+import org.openstreetmap.josm.data.osm.IWay;
 import org.openstreetmap.josm.data.osm.OsmPrimitive;
 import org.openstreetmap.josm.data.osm.Relation;
@@ -104 +107 @@
     public static boolean checkAndConfirmOutlyingDelete(Collection<? extends OsmPrimitive> primitives,
             Collection<? extends OsmPrimitive> ignore) {
+        final boolean nodes = primitives.stream().anyMatch(INode.class::isInstance);
+        final boolean ways = primitives.stream().anyMatch(IWay.class::isInstance);
+        final boolean relations = primitives.stream().anyMatch(IRelation.class::isInstance);
+        final String type;
+        if (nodes && !ways && !relations) {
+            type = tr("You are about to delete nodes which can have other referrers not yet downloaded.");
+        } else if (!nodes && ways && !relations) {
+            type = tr("You are about to delete ways which can have other referrers not yet downloaded.");
+        } else if (!nodes && !ways && relations) {
+            type = tr("You are about to delete relations which can have other referrers not yet downloaded.");
+        } else {
+            // OK. We have multiple types being deleted.
+            type = tr("You are about to delete primitives which can have other referrers not yet downloaded.");
+        }
         return Boolean.TRUE.equals(GuiHelper.runInEDTAndWaitAndReturn(() -> checkAndConfirmOutlyingOperation("delete",
                 tr("Delete confirmation"),
-                tr("You are about to delete nodes which can have other referrers not yet downloaded."
+                tr("{0}"
                         + "<br>"
                         + "This can cause problems because other objects (that you do not see) might use them."
                         + "<br>"
-                        + "Do you really want to delete?"),
+                        + "Do you really want to delete?", type),
                 tr("You are about to delete incomplete objects."
                         + "<br>"

trunk/src/org/openstreetmap/josm/actions/JoinAreasAction.java

@@ -673 +673 @@
             // remove now unconnected nodes without tags
             List<Node> toRemove = oldNodes.stream().filter(
-                    n -> (n.isNew() || !n.isOutsideDownloadArea()) && !n.hasKeys() && n.getReferrers().isEmpty())
+                    n -> n.isReferrersDownloaded() && !n.hasKeys() && n.getReferrers().isEmpty())
                     .collect(Collectors.toList());
             if (!toRemove.isEmpty()) {

trunk/src/org/openstreetmap/josm/actions/SplitWayAction.java

@@ -144 +144 @@
                     .show();
             return;
+        } else if (!checkAndConfirmOutlyingOperation("splitway", tr("Split way confirmation"),
+                tr("You are about to split a way that may have referrers that are not yet downloaded.")
+                        + "<br/>"
+                        + tr("This can lead to broken relations.") + "<br/>"
+                        + tr("Do you really want to split?"),
+                tr("The selected area is incomplete. Continue?"),
+                applicableWays, null)) {
+            return;
         }
 

trunk/src/org/openstreetmap/josm/actions/downloadtasks/DownloadReferrersTask.java

@@ -124 +124 @@
         DataSetMerger visitor = new DataSetMerger(targetLayer.getDataSet(), parents);
         visitor.merge();
+        this.children.stream().map(p -> targetLayer.getDataSet().getPrimitiveById(p))
+                .forEach(p -> p.setReferrersDownloaded(true));
+
         SwingUtilities.invokeLater(targetLayer::onPostDownloadFromServer);
         if (visitor.getConflicts().isEmpty())

trunk/src/org/openstreetmap/josm/command/Command.java

@@ -228 +228 @@
             if (osm.isIncomplete()) {
                 res |= IS_INCOMPLETE;
-            } else if ((res & IS_OUTSIDE) == 0 && (osm.isOutsideDownloadArea()
-                    || (osm instanceof Node && !osm.isNew() && osm.getDataSet() != null && osm.getDataSet().getDataSourceBounds().isEmpty()))
-                            && (ignore == null || !ignore.contains(osm))) {
+            } else if ((res & IS_OUTSIDE) == 0 && !osm.isReferrersDownloaded() && (ignore == null || !ignore.contains(osm))) {
                 res |= IS_OUTSIDE;
             }

trunk/src/org/openstreetmap/josm/command/DeleteCommand.java

@@ -434 +434 @@
         }
 
-        if (!silent && !callback.checkAndConfirmOutlyingDelete(
-                primitivesToDelete, Utils.filteredCollection(primitivesToDelete, Way.class)))
+        if (!silent && !callback.checkAndConfirmOutlyingDelete(primitivesToDelete, null))
             return null;
 

trunk/src/org/openstreetmap/josm/data/osm/AbstractPrimitive.java

@@ -4 +4 @@
 import static org.openstreetmap.josm.tools.I18n.tr;
 
+import java.io.IOException;
+import java.io.ObjectInputStream;
+import java.io.ObjectOutputStream;
 import java.text.MessageFormat;
 import java.time.Instant;
@@ -132 +135 @@
 
     /**
+     * Determines if the primitive has all of its referrers
+     */
+    protected static final short FLAG_ALL_REFERRERS_DOWNLOADED = 1 << 14;
+
+    /**
      * Put several boolean flags to one short int field to save memory.
      * Other bits of this field are used in subclasses.
      */
-    protected volatile short flags = FLAG_VISIBLE;   // visible per default
+    private volatile short flags = FLAG_VISIBLE;   // visible per default
 
     /**
@@ -366 +374 @@
     }
 
+    /**
+     * Write common data to a serialization stream. At time of writing, this should <i>only</i> be used by {@link PrimitiveData}.
+     * @param oos The output stream to write to
+     * @throws IOException see {@link ObjectOutputStream#write}
+     */
+    protected void writeObjectCommon(ObjectOutputStream oos) throws IOException {
+        oos.writeLong(id);
+        oos.writeLong(user == null ? -1 : user.getId());
+        oos.writeInt(version);
+        oos.writeInt(changesetId);
+        oos.writeInt(timestamp);
+        oos.writeObject(keys);
+        oos.writeShort(flags);
+    }
+
+    /**
+     * Read common data from a serialization stream. At time of writing, this should <i>only</i> be used by {@link PrimitiveData}.
+     * @param ois The serialization stream to read from
+     * @throws ClassNotFoundException see {@link ObjectInputStream#readObject()}
+     * @throws IOException see {@link ObjectInputStream#read}
+     */
+    protected void readObjectCommon(ObjectInputStream ois) throws ClassNotFoundException, IOException {
+        id = ois.readLong();
+        final long userId = ois.readLong();
+        user = userId == -1 ? null : User.getById(userId);
+        version = ois.readInt();
+        changesetId = ois.readInt();
+        timestamp = ois.readInt();
+        keys = (String[]) ois.readObject();
+        flags = ois.readShort();
+    }
+
     @Override
     public void setModified(boolean modified) {
@@ -379 +419 @@
     public boolean isDeleted() {
         return (flags & FLAG_DELETED) != 0;
+    }
+
+    @Override
+    public void setReferrersDownloaded(boolean referrersDownloaded) {
+        this.updateFlags(FLAG_ALL_REFERRERS_DOWNLOADED, referrersDownloaded);
     }
 
@@ -407 +452 @@
         updateFlags(FLAG_DELETED, deleted);
         setModified(deleted ^ !isVisible());
+    }
+
+    @Override
+    public boolean hasDirectionKeys() {
+        return (flags & FLAG_HAS_DIRECTIONS) != 0;
+    }
+
+    @Override
+    public boolean reversedDirection() {
+        return (flags & FLAG_DIRECTION_REVERSED) != 0;
+    }
+
+    @Override
+    public boolean isTagged() {
+        return (flags & FLAG_TAGGED) != 0;
+    }
+
+    @Override
+    public boolean isAnnotated() {
+        return (flags & FLAG_ANNOTATED) != 0;
+    }
+
+    @Override
+    public boolean isHighlighted() {
+        return (flags & FLAG_HIGHLIGHTED) != 0;
+    }
+
+    @Override
+    public boolean isDisabled() {
+        return (flags & FLAG_DISABLED) != 0;
+    }
+
+    @Override
+    public boolean isDisabledAndHidden() {
+        return ((flags & FLAG_DISABLED) != 0) && ((flags & FLAG_HIDE_IF_DISABLED) != 0);
+    }
+
+    @Override
+    public boolean isPreserved() {
+        return (flags & FLAG_PRESERVED) != 0;
+    }
+
+    @Override
+    public boolean isReferrersDownloaded() {
+        return isNew() || (flags & FLAG_ALL_REFERRERS_DOWNLOADED) != 0;
     }
 

trunk/src/org/openstreetmap/josm/data/osm/DataSetMerger.java

@@ -370 +370 @@
         }
         if (mergeFromSource) {
+            boolean backupReferrersDownloadedStatus = target.isReferrersDownloaded() && haveSameVersion;
             target.mergeFrom(source);
+            if (backupReferrersDownloadedStatus && !target.isReferrersDownloaded()) {
+                target.setReferrersDownloaded(true);
+            }
             objectsWithChildrenToMerge.add(source.getPrimitiveId());
         }

trunk/src/org/openstreetmap/josm/data/osm/IPrimitive.java

@@ -38 +38 @@
 
     /**
+     * Set the status of the referrers
+     * @param referrersDownloaded {@code true} if all referrers for this object have been downloaded
+     * @since xxx
+     */
+    default void setReferrersDownloaded(boolean referrersDownloaded) {
+        throw new UnsupportedOperationException(this.getClass().getName() + " does not support referrers status");
+    }
+
+    /**
      * Checks if object is known to the server.
      * Replies true if this primitive is either unknown to the server (i.e. its id
@@ -75 +84 @@
      */
     void setDeleted(boolean deleted);
+
+    /**
+     * Determines if this primitive is fully downloaded
+     * @return {@code true} if the primitive is fully downloaded and all parents and children should be available.
+     * {@code false} otherwise.
+     * @since xxx
+     */
+    default boolean isReferrersDownloaded() {
+        return false;
+    }
 
     /**

trunk/src/org/openstreetmap/josm/data/osm/OsmPrimitive.java

@@ -365 +365 @@
 
     @Override
-    public boolean isDisabled() {
-        return (flags & FLAG_DISABLED) != 0;
-    }
-
-    @Override
-    public boolean isDisabledAndHidden() {
-        return ((flags & FLAG_DISABLED) != 0) && ((flags & FLAG_HIDE_IF_DISABLED) != 0);
-    }
-
-    @Override
-    public boolean isPreserved() {
-        return (flags & FLAG_PRESERVED) != 0;
-    }
-
-    @Override
     public boolean isSelectable() {
         // not synchronized -> check disabled twice just to be sure we did not have a race condition.
@@ -493 +478 @@
     }
 
-    @Override
-    public boolean isHighlighted() {
-        return (flags & FLAG_HIGHLIGHTED) != 0;
-    }
-
     /*---------------
      * DIRECTION KEYS
@@ -525 +505 @@
     private void updateAnnotated() {
         updateFlagsNoLock(FLAG_ANNOTATED, hasKeys() && getWorkInProgressKeys().stream().anyMatch(this::hasKey));
-    }
-
-    @Override
-    public boolean isTagged() {
-        return (flags & FLAG_TAGGED) != 0;
-    }
-
-    @Override
-    public boolean isAnnotated() {
-        return (flags & FLAG_ANNOTATED) != 0;
     }
 
@@ -550 +520 @@
         updateFlagsNoLock(FLAG_DIRECTION_REVERSED, directionReversed);
         updateFlagsNoLock(FLAG_HAS_DIRECTIONS, hasDirections);
-    }
-
-    @Override
-    public boolean hasDirectionKeys() {
-        return (flags & FLAG_HAS_DIRECTIONS) != 0;
-    }
-
-    @Override
-    public boolean reversedDirection() {
-        return (flags & FLAG_DIRECTION_REVERSED) != 0;
     }
 
@@ -852 +812 @@
                         tr("Cannot merge primitives with different ids. This id is {0}, the other is {1}", id, other.getId()));
 
+            setIncomplete(other.isIncomplete());
+            super.cloneFrom(other);
             setKeys(other.hasKeys() ? other.getKeys() : null);
-            timestamp = other.timestamp;
             version = other.version;
-            setIncomplete(other.isIncomplete());
-            flags = other.flags;
-            user = other.user;
             changesetId = other.changesetId;
         } finally {

trunk/src/org/openstreetmap/josm/data/osm/PrimitiveData.java

@@ -85 +85 @@
     private void writeObject(ObjectOutputStream oos) throws IOException {
         // since super class is not Serializable
-        oos.writeLong(id);
-        oos.writeLong(user == null ? -1 : user.getId());
-        oos.writeInt(version);
-        oos.writeInt(changesetId);
-        oos.writeInt(timestamp);
-        oos.writeObject(keys);
-        oos.writeShort(flags);
+        super.writeObjectCommon(oos);
         oos.defaultWriteObject();
     }
@@ -97 +91 @@
     private void readObject(ObjectInputStream ois) throws ClassNotFoundException, IOException {
         // since super class is not Serializable
-        id = ois.readLong();
-        final long userId = ois.readLong();
-        user = userId == -1 ? null : User.getById(userId);
-        version = ois.readInt();
-        changesetId = ois.readInt();
-        timestamp = ois.readInt();
-        keys = (String[]) ois.readObject();
-        flags = ois.readShort();
+        super.readObjectCommon(ois);
         ois.defaultReadObject();
     }

trunk/src/org/openstreetmap/josm/data/validation/tests/BarriersEntrances.java

@@ -27 +27 @@
     @Override
     public void visit(Node n) {
-        if (n.hasTag("barrier", "entrance") && !n.isOutsideDownloadArea()) {
+        if (n.hasTag("barrier", "entrance") && n.isReferrersDownloaded()) {
             for (OsmPrimitive p : n.getReferrers()) {
                 if (p.hasKey("barrier")) {

trunk/src/org/openstreetmap/josm/data/vector/VectorPrimitive.java

@@ -60 +60 @@
 
     @Override
-    public boolean isTagged() {
-        return (flags & FLAG_TAGGED) != 0;
-    }
-
-    @Override
     public boolean isAnnotated() {
         return this.getInterestingTags().size() - this.getKeys().size() > 0;

trunk/src/org/openstreetmap/josm/gui/dialogs/InspectPrimitiveDataText.java

@@ -130 +130 @@
             sb.append(tr("deleted-on-server")).append(INDENT);
         }
+        if (o.isReferrersDownloaded()) {
+            sb.append(tr("all-referrers-downloaded")).append(INDENT);
+        } else {
+            sb.append(tr("referrers-not-all-downloaded")).append(INDENT);
+        }
         if (o.isModified()) {
             sb.append(tr("modified")).append(INDENT);

trunk/src/org/openstreetmap/josm/io/BoundingBoxDownloader.java

@@ -7 +7 @@
 import java.io.InputStream;
 import java.net.SocketException;
+import java.util.Collection;
+import java.util.Collections;
 import java.util.List;
 
@@ -216 +218 @@
                 }
             }
+            // From https://wiki.openstreetmap.org/wiki/API_v0.6#Retrieving_map_data_by_bounding_box:_GET_/api/0.6/map,
+            // relations are not recursed up, so they *may* have parent relations.
+            // Nodes inside the download area should have all relations and ways that refer to them.
+            // Ways should have all relations that refer to them and all child nodes, but those child nodes may not
+            //    have their parent referrers.
+            // Relations will have the *first* parent relations downloaded, but those are not split out in the returns.
+            // So we always assume that a relation has referrers that need to be downloaded unless it has no child relations.
+            // Our "full" overpass query doesn't return the same data as a standard download, so we cannot
+            // mark relations with no child relations as fully downloaded *yet*.
+            if (this.considerAsFullDownload()) {
+                final Collection<Bounds> bounds = this.getBounds();
+                // We cannot use OsmPrimitive#isOutsideDownloadArea yet since some download methods haven't added
+                // the download bounds to the dataset yet. This is specifically the case for overpass downloads.
+                ds.getNodes().stream().filter(n -> bounds.stream().anyMatch(b -> b.contains(n)))
+                        .forEach(i -> i.setReferrersDownloaded(true));
+                ds.getWays().forEach(i -> i.setReferrersDownloaded(true));
+            }
             return ds;
         } catch (OsmTransferException e) {
@@ -280 +299 @@
     }
 
+    /**
+     * Get the bounds for this downloader
+     * @return The bounds for this downloader
+     * @since xxx
+     */
+    protected Collection<Bounds> getBounds() {
+        return Collections.singleton(new Bounds(this.lat1, this.lon1, this.lat2, this.lon2));
+    }
 }

trunk/src/org/openstreetmap/josm/io/OverpassDownloadReader.java

@@ -14 +14 @@
 import java.time.format.DateTimeParseException;
 import java.util.Arrays;
+import java.util.Collection;
+import java.util.Collections;
 import java.util.EnumMap;
 import java.util.List;
@@ -20 +22 @@
 import java.util.NoSuchElementException;
 import java.util.Objects;
+import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.TimeUnit;
@@ -407 +410 @@
             // add bounds if necessary (note that Overpass API does not return bounds in the response XML)
             if (ds != null && ds.getDataSources().isEmpty() && overpassQuery.contains("{{bbox}}")) {
-                if (crosses180th) {
-                    Bounds bounds = new Bounds(lat1, lon1, lat2, 180.0);
-                    DataSource src = new DataSource(bounds, getBaseUrl());
-                    ds.addDataSource(src);
-
-                    bounds = new Bounds(lat1, -180.0, lat2, lon2);
-                    src = new DataSource(bounds, getBaseUrl());
-                    ds.addDataSource(src);
-                } else {
-                    Bounds bounds = new Bounds(lat1, lon1, lat2, lon2);
-                    DataSource src = new DataSource(bounds, getBaseUrl());
-                    ds.addDataSource(src);
-                }
+                getBounds().forEach(bounds -> ds.addDataSource(new DataSource(bounds, getBaseUrl())));
             }
             return ds;
@@ -441 +432 @@
         return overpassQuery.equals(OverpassDownloadSource.FULL_DOWNLOAD_QUERY);
     }
+
+    @Override
+    protected Collection<Bounds> getBounds() {
+        if (this.overpassQuery.contains("{{bbox}}")) {
+            if (crosses180th) {
+                return Set.of(new Bounds(lat1, lon1, lat2, 180.0),
+                        new Bounds(lat1, -180.0, lat2, lon2));
+            } else {
+                return Collections.singleton(new Bounds(lat1, lon1, lat2, lon2));
+            }
+        }
+        return Collections.emptySet();
+    }
 }