Skip to content

Commit 3e2d807

Browse files
author
taylor.smock
committed
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`. git-svn-id: https://josm.openstreetmap.de/svn/trunk@19078 0c6e7542-c601-0410-84e7-c038aed88b3b
1 parent 503a1b6 commit 3e2d807

22 files changed

+221
-92
lines changed

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

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// License: GPL. For details, see LICENSE file.
22
package org.openstreetmap.josm.actions;
33

4+
import static java.util.function.Predicate.not;
45
import static org.openstreetmap.josm.gui.help.HelpUtil.ht;
56
import static org.openstreetmap.josm.tools.I18n.tr;
67

@@ -271,9 +272,9 @@ public static Command buildCommand(DataSet ds) throws InvalidSelection {
271272
}
272273
fixNodes.addAll(collectNodesWithExternReferrers(ways));
273274

274-
// Check if one or more nodes are outside of download area
275-
if (nodes.stream().anyMatch(Node::isOutsideDownloadArea))
276-
throw new InvalidSelection(tr("One or more nodes involved in this action is outside of the downloaded area."));
275+
// Check if one or more nodes does not have all parents available
276+
if (nodes.stream().anyMatch(not(Node::isReferrersDownloaded)))
277+
throw new InvalidSelection(tr("One or more nodes involved in this action may have additional referrers."));
277278

278279

279280
if (center == null) {

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -284,8 +284,8 @@ public void actionPerformed(ActionEvent event) {
284284
for (Way w : selectedWays) {
285285
final Node[] endnodes = {w.firstNode(), w.lastNode()};
286286
for (Node n : endnodes) {
287-
if (!n.isNew() && n.isOutsideDownloadArea() && !endNodesOutside.add(n)) {
288-
new Notification(tr("Combine ways refused<br>" + "(A shared node is outside of the download area)"))
287+
if (!n.isNew() && !n.isReferrersDownloaded() && !endNodesOutside.add(n)) {
288+
new Notification(tr("Combine ways refused<br>" + "(A shared node may have additional referrers)"))
289289
.setIcon(JOptionPane.INFORMATION_MESSAGE).show();
290290
return;
291291

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

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,9 @@
1616

1717
import org.openstreetmap.josm.command.DeleteCommand.DeletionCallback;
1818
import org.openstreetmap.josm.data.osm.DefaultNameFormatter;
19+
import org.openstreetmap.josm.data.osm.INode;
20+
import org.openstreetmap.josm.data.osm.IRelation;
21+
import org.openstreetmap.josm.data.osm.IWay;
1922
import org.openstreetmap.josm.data.osm.OsmPrimitive;
2023
import org.openstreetmap.josm.data.osm.Relation;
2124
import org.openstreetmap.josm.data.osm.RelationToChildReference;
@@ -103,13 +106,27 @@ protected void updateEnabledState(Collection<? extends OsmPrimitive> selection)
103106
*/
104107
public static boolean checkAndConfirmOutlyingDelete(Collection<? extends OsmPrimitive> primitives,
105108
Collection<? extends OsmPrimitive> ignore) {
109+
final boolean nodes = primitives.stream().anyMatch(INode.class::isInstance);
110+
final boolean ways = primitives.stream().anyMatch(IWay.class::isInstance);
111+
final boolean relations = primitives.stream().anyMatch(IRelation.class::isInstance);
112+
final String type;
113+
if (nodes && !ways && !relations) {
114+
type = tr("You are about to delete nodes which can have other referrers not yet downloaded.");
115+
} else if (!nodes && ways && !relations) {
116+
type = tr("You are about to delete ways which can have other referrers not yet downloaded.");
117+
} else if (!nodes && !ways && relations) {
118+
type = tr("You are about to delete relations which can have other referrers not yet downloaded.");
119+
} else {
120+
// OK. We have multiple types being deleted.
121+
type = tr("You are about to delete primitives which can have other referrers not yet downloaded.");
122+
}
106123
return Boolean.TRUE.equals(GuiHelper.runInEDTAndWaitAndReturn(() -> checkAndConfirmOutlyingOperation("delete",
107124
tr("Delete confirmation"),
108-
tr("You are about to delete nodes which can have other referrers not yet downloaded."
125+
tr("{0}"
109126
+ "<br>"
110127
+ "This can cause problems because other objects (that you do not see) might use them."
111128
+ "<br>"
112-
+ "Do you really want to delete?"),
129+
+ "Do you really want to delete?", type),
113130
tr("You are about to delete incomplete objects."
114131
+ "<br>"
115132
+ "This will cause problems because you don''t see the real object."

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -672,7 +672,7 @@ private JoinAreasResult joinAreas(List<Multipolygon> areas) throws UserCancelExc
672672
commitCommands(marktr("Removed duplicate nodes"));
673673
// remove now unconnected nodes without tags
674674
List<Node> toRemove = oldNodes.stream().filter(
675-
n -> (n.isNew() || !n.isOutsideDownloadArea()) && !n.hasKeys() && n.getReferrers().isEmpty())
675+
n -> n.isReferrersDownloaded() && !n.hasKeys() && n.getReferrers().isEmpty())
676676
.collect(Collectors.toList());
677677
if (!toRemove.isEmpty()) {
678678
cmds.add(new DeleteCommand(toRemove));

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,14 @@ public static void runOn(DataSet ds) {
143143
.setIcon(JOptionPane.WARNING_MESSAGE)
144144
.show();
145145
return;
146+
} else if (!checkAndConfirmOutlyingOperation("splitway", tr("Split way confirmation"),
147+
tr("You are about to split a way that may have referrers that are not yet downloaded.")
148+
+ "<br/>"
149+
+ tr("This can lead to broken relations.") + "<br/>"
150+
+ tr("Do you really want to split?"),
151+
tr("The selected area is incomplete. Continue?"),
152+
applicableWays, null)) {
153+
return;
146154
}
147155

148156
// Finally, applicableWays contains only one perfect way

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,9 @@ protected void finish() {
123123

124124
DataSetMerger visitor = new DataSetMerger(targetLayer.getDataSet(), parents);
125125
visitor.merge();
126+
this.children.stream().map(p -> targetLayer.getDataSet().getPrimitiveById(p))
127+
.forEach(p -> p.setReferrersDownloaded(true));
128+
126129
SwingUtilities.invokeLater(targetLayer::onPostDownloadFromServer);
127130
if (visitor.getConflicts().isEmpty())
128131
return;

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

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -227,9 +227,7 @@ public static int checkOutlyingOrIncompleteOperation(
227227
for (OsmPrimitive osm : primitives) {
228228
if (osm.isIncomplete()) {
229229
res |= IS_INCOMPLETE;
230-
} else if ((res & IS_OUTSIDE) == 0 && (osm.isOutsideDownloadArea()
231-
|| (osm instanceof Node && !osm.isNew() && osm.getDataSet() != null && osm.getDataSet().getDataSourceBounds().isEmpty()))
232-
&& (ignore == null || !ignore.contains(osm))) {
230+
} else if ((res & IS_OUTSIDE) == 0 && !osm.isReferrersDownloaded() && (ignore == null || !ignore.contains(osm))) {
233231
res |= IS_OUTSIDE;
234232
}
235233
}

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -433,8 +433,7 @@ public static Command delete(Collection<? extends OsmPrimitive> selection, boole
433433
primitivesToDelete.addAll(nodesToDelete);
434434
}
435435

436-
if (!silent && !callback.checkAndConfirmOutlyingDelete(
437-
primitivesToDelete, Utils.filteredCollection(primitivesToDelete, Way.class)))
436+
if (!silent && !callback.checkAndConfirmOutlyingDelete(primitivesToDelete, null))
438437
return null;
439438

440439
Collection<Way> waysToBeChanged = primitivesToDelete.stream()

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

Lines changed: 91 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,9 @@
33

44
import static org.openstreetmap.josm.tools.I18n.tr;
55

6+
import java.io.IOException;
7+
import java.io.ObjectInputStream;
8+
import java.io.ObjectOutputStream;
69
import java.text.MessageFormat;
710
import java.time.Instant;
811
import java.util.ArrayList;
@@ -130,11 +133,16 @@ public abstract class AbstractPrimitive implements IPrimitive, IFilterablePrimit
130133
*/
131134
protected static final short FLAG_PRESERVED = 1 << 13;
132135

136+
/**
137+
* Determines if the primitive has all of its referrers
138+
*/
139+
protected static final short FLAG_ALL_REFERRERS_DOWNLOADED = 1 << 14;
140+
133141
/**
134142
* Put several boolean flags to one short int field to save memory.
135143
* Other bits of this field are used in subclasses.
136144
*/
137-
protected volatile short flags = FLAG_VISIBLE; // visible per default
145+
private volatile short flags = FLAG_VISIBLE; // visible per default
138146

139147
/**
140148
* The mappaint cache index for this primitive.
@@ -365,6 +373,38 @@ protected boolean updateFlagsChanged(short flag, boolean value) {
365373
return oldFlags != flags;
366374
}
367375

376+
/**
377+
* Write common data to a serialization stream. At time of writing, this should <i>only</i> be used by {@link PrimitiveData}.
378+
* @param oos The output stream to write to
379+
* @throws IOException see {@link ObjectOutputStream#write}
380+
*/
381+
protected void writeObjectCommon(ObjectOutputStream oos) throws IOException {
382+
oos.writeLong(id);
383+
oos.writeLong(user == null ? -1 : user.getId());
384+
oos.writeInt(version);
385+
oos.writeInt(changesetId);
386+
oos.writeInt(timestamp);
387+
oos.writeObject(keys);
388+
oos.writeShort(flags);
389+
}
390+
391+
/**
392+
* Read common data from a serialization stream. At time of writing, this should <i>only</i> be used by {@link PrimitiveData}.
393+
* @param ois The serialization stream to read from
394+
* @throws ClassNotFoundException see {@link ObjectInputStream#readObject()}
395+
* @throws IOException see {@link ObjectInputStream#read}
396+
*/
397+
protected void readObjectCommon(ObjectInputStream ois) throws ClassNotFoundException, IOException {
398+
id = ois.readLong();
399+
final long userId = ois.readLong();
400+
user = userId == -1 ? null : User.getById(userId);
401+
version = ois.readInt();
402+
changesetId = ois.readInt();
403+
timestamp = ois.readInt();
404+
keys = (String[]) ois.readObject();
405+
flags = ois.readShort();
406+
}
407+
368408
@Override
369409
public void setModified(boolean modified) {
370410
updateFlags(FLAG_MODIFIED, modified);
@@ -380,6 +420,11 @@ public boolean isDeleted() {
380420
return (flags & FLAG_DELETED) != 0;
381421
}
382422

423+
@Override
424+
public void setReferrersDownloaded(boolean referrersDownloaded) {
425+
this.updateFlags(FLAG_ALL_REFERRERS_DOWNLOADED, referrersDownloaded);
426+
}
427+
383428
@Override
384429
public boolean isUndeleted() {
385430
return (flags & (FLAG_VISIBLE + FLAG_DELETED)) == 0;
@@ -408,6 +453,51 @@ public void setDeleted(boolean deleted) {
408453
setModified(deleted ^ !isVisible());
409454
}
410455

456+
@Override
457+
public boolean hasDirectionKeys() {
458+
return (flags & FLAG_HAS_DIRECTIONS) != 0;
459+
}
460+
461+
@Override
462+
public boolean reversedDirection() {
463+
return (flags & FLAG_DIRECTION_REVERSED) != 0;
464+
}
465+
466+
@Override
467+
public boolean isTagged() {
468+
return (flags & FLAG_TAGGED) != 0;
469+
}
470+
471+
@Override
472+
public boolean isAnnotated() {
473+
return (flags & FLAG_ANNOTATED) != 0;
474+
}
475+
476+
@Override
477+
public boolean isHighlighted() {
478+
return (flags & FLAG_HIGHLIGHTED) != 0;
479+
}
480+
481+
@Override
482+
public boolean isDisabled() {
483+
return (flags & FLAG_DISABLED) != 0;
484+
}
485+
486+
@Override
487+
public boolean isDisabledAndHidden() {
488+
return ((flags & FLAG_DISABLED) != 0) && ((flags & FLAG_HIDE_IF_DISABLED) != 0);
489+
}
490+
491+
@Override
492+
public boolean isPreserved() {
493+
return (flags & FLAG_PRESERVED) != 0;
494+
}
495+
496+
@Override
497+
public boolean isReferrersDownloaded() {
498+
return isNew() || (flags & FLAG_ALL_REFERRERS_DOWNLOADED) != 0;
499+
}
500+
411501
/**
412502
* If set to true, this object is incomplete, which means only the id
413503
* and type is known (type is the objects instance class)

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -369,7 +369,11 @@ private boolean mergeById(OsmPrimitive source) {
369369
mergeFromSource = true;
370370
}
371371
if (mergeFromSource) {
372+
boolean backupReferrersDownloadedStatus = target.isReferrersDownloaded() && haveSameVersion;
372373
target.mergeFrom(source);
374+
if (backupReferrersDownloadedStatus && !target.isReferrersDownloaded()) {
375+
target.setReferrersDownloaded(true);
376+
}
373377
objectsWithChildrenToMerge.add(source.getPrimitiveId());
374378
}
375379
return true;

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

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,15 @@ public interface IPrimitive extends IQuadBucketType, Tagged, PrimitiveId, Stylab
3636
*/
3737
void setModified(boolean modified);
3838

39+
/**
40+
* Set the status of the referrers
41+
* @param referrersDownloaded {@code true} if all referrers for this object have been downloaded
42+
* @since xxx
43+
*/
44+
default void setReferrersDownloaded(boolean referrersDownloaded) {
45+
throw new UnsupportedOperationException(this.getClass().getName() + " does not support referrers status");
46+
}
47+
3948
/**
4049
* Checks if object is known to the server.
4150
* Replies true if this primitive is either unknown to the server (i.e. its id
@@ -75,6 +84,16 @@ public interface IPrimitive extends IQuadBucketType, Tagged, PrimitiveId, Stylab
7584
*/
7685
void setDeleted(boolean deleted);
7786

87+
/**
88+
* Determines if this primitive is fully downloaded
89+
* @return {@code true} if the primitive is fully downloaded and all parents and children should be available.
90+
* {@code false} otherwise.
91+
* @since xxx
92+
*/
93+
default boolean isReferrersDownloaded() {
94+
return false;
95+
}
96+
7897
/**
7998
* Determines if this primitive is incomplete.
8099
* @return {@code true} if this primitive is incomplete, {@code false} otherwise

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

Lines changed: 2 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -363,21 +363,6 @@ public void setPreserved(boolean isPreserved) {
363363
updateFlags(FLAG_PRESERVED, isPreserved);
364364
}
365365

366-
@Override
367-
public boolean isDisabled() {
368-
return (flags & FLAG_DISABLED) != 0;
369-
}
370-
371-
@Override
372-
public boolean isDisabledAndHidden() {
373-
return ((flags & FLAG_DISABLED) != 0) && ((flags & FLAG_HIDE_IF_DISABLED) != 0);
374-
}
375-
376-
@Override
377-
public boolean isPreserved() {
378-
return (flags & FLAG_PRESERVED) != 0;
379-
}
380-
381366
@Override
382367
public boolean isSelectable() {
383368
// not synchronized -> check disabled twice just to be sure we did not have a race condition.
@@ -492,11 +477,6 @@ public void setHighlighted(boolean highlighted) {
492477
}
493478
}
494479

495-
@Override
496-
public boolean isHighlighted() {
497-
return (flags & FLAG_HIGHLIGHTED) != 0;
498-
}
499-
500480
/*---------------
501481
* DIRECTION KEYS
502482
*---------------*/
@@ -526,16 +506,6 @@ private void updateAnnotated() {
526506
updateFlagsNoLock(FLAG_ANNOTATED, hasKeys() && getWorkInProgressKeys().stream().anyMatch(this::hasKey));
527507
}
528508

529-
@Override
530-
public boolean isTagged() {
531-
return (flags & FLAG_TAGGED) != 0;
532-
}
533-
534-
@Override
535-
public boolean isAnnotated() {
536-
return (flags & FLAG_ANNOTATED) != 0;
537-
}
538-
539509
protected void updateDirectionFlags() {
540510
boolean hasDirections = false;
541511
boolean directionReversed = false;
@@ -551,16 +521,6 @@ protected void updateDirectionFlags() {
551521
updateFlagsNoLock(FLAG_HAS_DIRECTIONS, hasDirections);
552522
}
553523

554-
@Override
555-
public boolean hasDirectionKeys() {
556-
return (flags & FLAG_HAS_DIRECTIONS) != 0;
557-
}
558-
559-
@Override
560-
public boolean reversedDirection() {
561-
return (flags & FLAG_DIRECTION_REVERSED) != 0;
562-
}
563-
564524
/*------------
565525
* Keys handling
566526
------------*/
@@ -851,12 +811,10 @@ public void mergeFrom(OsmPrimitive other) {
851811
throw new DataIntegrityProblemException(
852812
tr("Cannot merge primitives with different ids. This id is {0}, the other is {1}", id, other.getId()));
853813

814+
setIncomplete(other.isIncomplete());
815+
super.cloneFrom(other);
854816
setKeys(other.hasKeys() ? other.getKeys() : null);
855-
timestamp = other.timestamp;
856817
version = other.version;
857-
setIncomplete(other.isIncomplete());
858-
flags = other.flags;
859-
user = other.user;
860818
changesetId = other.changesetId;
861819
} finally {
862820
writeUnlock(locked);

0 commit comments

Comments
 (0)