josm-undelete-explaination.txt

org.openstreetmap.josm.data.osm.OsmPrimitive:
    Added isUndeleted() and isNewOrUndeleted() methods. Removed isVisible() check from
    "nonDeleted" predicates. "Invisible" means that object is deleted on the server,
    but it shoudn't be treated as deleted in JOSM

org.openstreetmap.josm.data.osm.OsmPrimitive.setDeleted(boolean deleted)
    I think it's really good idea to add XOR operation with !isVisible() to setModified() 
    call. If object is visible, behavor doesn't change. But is object is invisible,
    setDeleted(true) implies setModified(false). Primitive is deleted on the server and
    deleted in JOSM, it's not modified. And setDeleted(false) implies setModified(true):
    to make invisible object not deleted, we need to modify it.

org.openstreetmap.josm.actions.search.SearchCompiler.Modified.match(OsmPrimitive osm),
org.openstreetmap.josm.gui.MapStatus.popupBuildPrimitiveLabels(final OsmPrimitive osm),
org.openstreetmap.josm.actions.UploadSelectionAction:
    Both isNew() and isNewOrUndeleted() can be used, but isNewOrUndeleted() is better.
    In normal work all undeleted objects should be marked as "modified" anyway.
    However there are some cases(for example when you load a file that has unmodified
    "invisible" objects) where they aren't.

org.openstreetmap.josm.data.osm.DatasetConsistencyTest.checkZeroNodesWays():
    Removed isVisible() check. If "invisible" primitive was fetched from server without
    creating a conflict it's an error, there should be a warining in Dataset consistency test.

org.openstreetmap.josm.command.DeleteCommand.checkAndConfirmOutlyingDeletes(OsmDataLayer layer, Collection<OsmPrimitive> primitivesToDelete)
    Chenged isNew() to isNewOrUndeleted(). It doesn't matter for "invisible" objects wheather
    they are inside the download area or not as for new.

org.openstreetmap.josm.data.osm.DataSetMerger.mergeById(OsmPrimitive source):
    If target with lower version is "invisible" and source is "visible", it just means
    that object was undeleted. There's nothing weird there, we should use "source" version.
    It's weird if target and source have equal version. It means that one of them has wrong
    "visible" attribute. Create a conflict in this case.

org.openstreetmap.josm.data.osm.DataSetMerger.mergeNodeList(Way source),
org.openstreetmap.josm.data.osm.DataSetMerger.mergeRelationMembers(Relation source):    
    Removed isVisible() check. Why "invisible" primitives are silently ignored there?

org.openstreetmap.josm.actions.SaveActionBase.isDataSetEmpty(OsmDataLayer layer),
org.openstreetmap.josm.io.OsmWriter.shouldWrite(OsmPrimitive osm):
    Primitives that was undeleted and deleted again by user shouldn't be saved in file.

org.openstreetmap.josm.io.DiffResultProcessor.postProcess(Changeset cs, ProgressMonitor monitor),
org.openstreetmap.josm.io.OsmApi.modifyPrimitive(OsmPrimitive osm, ProgressMonitor monitor):
    Added setVisible(true) for all processed primitives. Note that the OSM server ignores the
    "visible" attribute for PUT and <modify> requests.

org.openstreetmap.josm.data.APIDataSet:
    Changed isNew() to isNewOrUndeleted() so undeleted objects are now added to toAdd array,
    not to toUpdate array. It is needed because undeleted and created should be sorted together.
    It doesn't matter for JOSM OsmApi implementation in which APIDataSet array primitives are,
    it's just needed to upload primitives in correct order.
    Also added safety isVisible() check for deleted primitives. Delete command shoudn't be sent
    for "invisible" primitives.

undelete.diff

Index: src/org/openstreetmap/josm/actions/search/SearchCompiler.java
===================================================================
--- src/org/openstreetmap/josm/actions/search/SearchCompiler.java	(revision 3334)
+++ src/org/openstreetmap/josm/actions/search/SearchCompiler.java	(working copy)
@@ -561,7 +561,7 @@
 
     private static class Modified extends Match {
         @Override public boolean match(OsmPrimitive osm) {
-            return osm.isModified() || osm.isNew();
+            return osm.isModified() || osm.isNewOrUndeleted();
         }
         @Override public String toString() {return "modified";}
     }
Index: src/org/openstreetmap/josm/actions/UploadSelectionAction.java
===================================================================
--- src/org/openstreetmap/josm/actions/UploadSelectionAction.java	(revision 3334)
+++ src/org/openstreetmap/josm/actions/UploadSelectionAction.java	(working copy)
@@ -77,9 +77,9 @@
     protected Set<OsmPrimitive> getModifiedPrimitives(Collection<OsmPrimitive> primitives) {
         HashSet<OsmPrimitive> ret = new HashSet<OsmPrimitive>();
         for (OsmPrimitive p: primitives) {
-            if (p.isNew()) {
+            if (p.isNewOrUndeleted()) {
                 ret.add(p);
-            } else if (p.isVisible() && p.isModified() && !p.isIncomplete()) {
+            } else if (p.isModified() && !p.isIncomplete()) {
                 ret.add(p);
             }
         }
@@ -170,8 +170,7 @@
     /**
      * Computes the collection of primitives to upload, given a collection of candidate
      * primitives.
-     * Some of the candidates are excluded, i.e. if they aren't modified or if they
-     * aren't visible.
+     * Some of the candidates are excluded, i.e. if they aren't modified.
      * Other primitives are added. A typical case is a primitive which is new and and
      * which is referred by a modified relation. In order to upload the relation the
      * new primitive has to be uploaded as well, even if it isn't included in the
@@ -186,14 +185,14 @@
         }
 
         public void visit(Node n) {
-            if (n.isNew() || ((n.isModified() || n.isDeleted()) && n.isVisible())) {
+            if (n.isNewOrUndeleted() || n.isModified() || n.isDeleted()) {
                 // upload new nodes as well as modified and deleted ones
                 hull.add(n);
             }
         }
 
         public void visit(Way w) {
-            if (w.isNew() || ((w.isModified() || w.isDeleted()) && w.isVisible())) {
+            if (w.isNewOrUndeleted() || w.isModified() || w.isDeleted()) {
                 // upload new ways as well as modified and deleted ones
                 hull.add(w);
                 for (Node n: w.getNodes()) {
@@ -205,14 +204,14 @@
         }
 
         public void visit(Relation r) {
-            if (r.isNew() || ((r.isModified() || r.isDeleted()) && r.isVisible())) {
+            if (r.isNewOrUndeleted() || r.isModified() || r.isDeleted()) {
                 hull.add(r);
                 for (OsmPrimitive p : r.getMemberPrimitives()) {
                     // add new relation members. Don't include modified
                     // relation members. r shouldn't refer to deleted primitives,
                     // so wont check here for deleted primitives here
                     //
-                    if (p.isNew()) {
+                    if (p.isNewOrUndeleted()) {
                         p.visit(this);
                     }
                 }
@@ -294,7 +293,7 @@
         protected Set<OsmPrimitive> getPrimitivesToCheckForParents() {
             HashSet<OsmPrimitive> ret = new HashSet<OsmPrimitive>();
             for (OsmPrimitive p: toUpload) {
-                if (p.isDeleted() && !p.isNew()) {
+                if (p.isDeleted() && !p.isNewOrUndeleted()) {
                     ret.add(p);
                 }
             }
Index: src/org/openstreetmap/josm/actions/SaveActionBase.java
===================================================================
--- src/org/openstreetmap/josm/actions/SaveActionBase.java	(revision 3334)
+++ src/org/openstreetmap/josm/actions/SaveActionBase.java	(working copy)
@@ -138,7 +138,7 @@
      */
     private boolean isDataSetEmpty(OsmDataLayer layer) {
         for (OsmPrimitive osm : layer.data.allNonDeletedPrimitives())
-            if (!osm.isDeleted() || !osm.isNew())
+            if (!osm.isDeleted() || !osm.isNewOrUndeleted())
                 return false;
         return true;
     }
Index: src/org/openstreetmap/josm/gui/MapStatus.java
===================================================================
--- src/org/openstreetmap/josm/gui/MapStatus.java	(revision 3334)
+++ src/org/openstreetmap/josm/gui/MapStatus.java	(working copy)
@@ -419,7 +419,7 @@
         private final JLabel popupBuildPrimitiveLabels(final OsmPrimitive osm) {
             final StringBuilder text = new StringBuilder();
             String name = osm.getDisplayName(DefaultNameFormatter.getInstance());
-            if (osm.isNew() || osm.isModified()) {
+            if (osm.isNewOrUndeleted() || osm.isModified()) {
                 name = "<i><b>"+ name + "*</b></i>";
             }
             text.append(name);
Index: src/org/openstreetmap/josm/io/DiffResultProcessor.java
===================================================================
--- src/org/openstreetmap/josm/io/DiffResultProcessor.java	(revision 3334)
+++ src/org/openstreetmap/josm/io/DiffResultProcessor.java	(working copy)
@@ -124,6 +124,7 @@
                 processed.add(p);
                 if (!p.isDeleted()) {
                     p.setOsmId(entry.new_id, entry.new_version);
+                    p.setVisible(true);
                 }
                 if (cs != null && !cs.isNew()) {
                     p.setChangesetId(cs.getId());
Index: src/org/openstreetmap/josm/io/OsmApi.java
===================================================================
--- src/org/openstreetmap/josm/io/OsmApi.java	(revision 3334)
+++ src/org/openstreetmap/josm/io/OsmApi.java	(working copy)
@@ -274,6 +274,7 @@
             ret = sendRequest("PUT", OsmPrimitiveType.from(osm).getAPIName()+"/" + osm.getId(), toXml(osm, true), monitor);
             osm.setOsmId(osm.getId(), Integer.parseInt(ret.trim()));
             osm.setChangesetId(getChangeset().getId());
+            osm.setVisible(true);
         } catch(NumberFormatException e) {
             throw new OsmTransferException(tr("Unexpected format of new version of modified primitive ''{0}''. Got ''{1}''.", osm.getId(), ret));
         }
Index: src/org/openstreetmap/josm/io/OsmWriter.java
===================================================================
--- src/org/openstreetmap/josm/io/OsmWriter.java	(revision 3334)
+++ src/org/openstreetmap/josm/io/OsmWriter.java	(working copy)
@@ -97,7 +97,7 @@
     }
 
     private boolean shouldWrite(OsmPrimitive osm) {
-        return !osm.isNew() || !osm.isDeleted();
+        return !osm.isNewOrUndeleted() || !osm.isDeleted();
     }
 
     public void writeDataSources(DataSet ds) {
Index: src/org/openstreetmap/josm/data/osm/OsmPrimitive.java
===================================================================
--- src/org/openstreetmap/josm/data/osm/OsmPrimitive.java	(revision 3334)
+++ src/org/openstreetmap/josm/data/osm/OsmPrimitive.java	(working copy)
@@ -391,6 +391,14 @@
     }
 
     /**
+     * Replies <code>true</code> if the object has been deleted on the server and was undeleted by the user.
+     * @return <code>true</code> if the object has been undeleted
+     */
+    public boolean isUndeleted() {
+        return (flags & (FLAG_VISIBLE + FLAG_DELETED)) == 0;
+    }
+
+    /**
      * Replies <code>true</code>, if the object is usable (i.e. complete
      * and not deleted).
      *
@@ -426,25 +434,25 @@
 
     public static Predicate<OsmPrimitive> nonDeletedPredicate = new Predicate<OsmPrimitive>() {
         public boolean evaluate(OsmPrimitive primitive) {
-            return primitive.isVisible() && !primitive.isDeleted();
+            return !primitive.isDeleted();
         }
     };
 
     public static Predicate<OsmPrimitive> nonDeletedCompletePredicate = new Predicate<OsmPrimitive>() {
         public boolean evaluate(OsmPrimitive primitive) {
-            return primitive.isVisible() && !primitive.isDeleted() && !primitive.isIncomplete();
+            return !primitive.isDeleted() && !primitive.isIncomplete();
         }
     };
 
     public static Predicate<OsmPrimitive> nonDeletedPhysicalPredicate = new Predicate<OsmPrimitive>() {
         public boolean evaluate(OsmPrimitive primitive) {
-            return primitive.isVisible() && !primitive.isDeleted() && !primitive.isIncomplete() && !(primitive instanceof Relation);
+            return !primitive.isDeleted() && !primitive.isIncomplete() && !(primitive instanceof Relation);
         }
     };
 
     public static Predicate<OsmPrimitive> modifiedPredicate = new Predicate<OsmPrimitive>() {
         public boolean evaluate(OsmPrimitive primitive) {
-            return primitive.isVisible() && primitive.isModified();
+            return primitive.isModified();
         }
     };
 
@@ -539,6 +547,14 @@
     }
 
     /**
+     *
+     * @return True if primitive is new or undeleted
+     */
+    public boolean isNewOrUndeleted() {
+        return (id <= 0) || ((flags & (FLAG_VISIBLE + FLAG_DELETED)) == 0);
+    }
+
+    /**
      * Sets the id and the version of this primitive if it is known to the OSM API.
      *
      * Since we know the id and its version it can't be incomplete anymore. incomplete
@@ -721,7 +737,7 @@
         } else {
             flags &= ~FLAG_DELETED;
         }
-        setModified(deleted);
+        setModified(deleted ^ !isVisible());
         if (dataSet != null) {
             if (deleted) {
                 dataSet.firePrimitivesRemoved(Collections.singleton(this), false);
Index: src/org/openstreetmap/josm/data/osm/DataSetMerger.java
===================================================================
--- src/org/openstreetmap/josm/data/osm/DataSetMerger.java	(revision 3334)
+++ src/org/openstreetmap/josm/data/osm/DataSetMerger.java	(working copy)
@@ -188,14 +188,10 @@
         for (Node sourceNode : source.getNodes()) {
             Node targetNode = (Node)getMergeTarget(sourceNode);
             if (targetNode != null) {
-                if (targetNode.isVisible()) {
-                    newNodes.add(targetNode);
-                    if (targetNode.isDeleted() && !conflicts.hasConflictForMy(targetNode)) {
-                        conflicts.add(new Conflict<OsmPrimitive>(targetNode, sourceNode, true));
-                        targetNode.setDeleted(false);
-                    }
-                } else {
-                    target.setModified(true);
+                newNodes.add(targetNode);
+                if (targetNode.isDeleted() && !conflicts.hasConflictForMy(targetNode)) {
+                    conflicts.add(new Conflict<OsmPrimitive>(targetNode, sourceNode, true));
+                    targetNode.setDeleted(false);
                 }
             } else
                 throw new IllegalStateException(tr("Missing merge target for node with id {0}", sourceNode.getUniqueId()));
@@ -219,15 +215,11 @@
             OsmPrimitive targetMember = getMergeTarget(sourceMember.getMember());
             if (targetMember == null)
                 throw new IllegalStateException(tr("Missing merge target of type {0} with id {1}", sourceMember.getType(), sourceMember.getUniqueId()));
-            if (targetMember.isVisible()) {
-                RelationMember newMember = new RelationMember(sourceMember.getRole(), targetMember);
-                newMembers.add(newMember);
-                if (targetMember.isDeleted() && !conflicts.hasConflictForMy(targetMember)) {
-                    conflicts.add(new Conflict<OsmPrimitive>(targetMember, sourceMember.getMember(), true));
-                    targetMember.setDeleted(false);
-                }
-            } else {
-                target.setModified(true);
+            RelationMember newMember = new RelationMember(sourceMember.getRole(), targetMember);
+            newMembers.add(newMember);
+            if (targetMember.isDeleted() && !conflicts.hasConflictForMy(targetMember)) {
+                conflicts.add(new Conflict<OsmPrimitive>(targetMember, sourceMember.getMember(), true));
+                targetMember.setDeleted(false);
             }
         }
         target.setMembers(newMembers);
@@ -251,15 +243,9 @@
         if (target.getVersion() > source.getVersion())
             // target.version > source.version => keep target version
             return true;
-        if (! target.isVisible() && source.isVisible()) {
+        if (! target.isVisible() && source.isVisible() && target.getVersion() == source.getVersion()) {
             // should not happen
-            // FIXME: this message does not make sense, source version can not be lower than
-            //        target version at this point
-            logger.warning(tr("Target object with id {0} and version {1} is visible although "
-                    + "source object with lower version {2} is not visible. "
-                    + "Cannot deal with this inconsistency. Keeping target object. ",
-                    Long.toString(target.getId()),Long.toString(target.getVersion()), Long.toString(source.getVersion())
-            ));
+            conflicts.add(target,source);
         } else if (target.isVisible() && ! source.isVisible()) {
             // this is always a conflict because the user has to decide whether
             // he wants to create a clone of its target primitive or whether he
Index: src/org/openstreetmap/josm/data/APIDataSet.java
===================================================================
--- src/org/openstreetmap/josm/data/APIDataSet.java	(revision 3334)
+++ src/org/openstreetmap/josm/data/APIDataSet.java	(working copy)
@@ -60,11 +60,11 @@
             if (osm.get("josm/ignore") != null) {
                 continue;
             }
-            if (osm.isNew() && !osm.isDeleted()) {
+            if (osm.isNewOrUndeleted() && !osm.isDeleted()) {
                 toAdd.add(osm);
             } else if (osm.isModified() && !osm.isDeleted()) {
                 toUpdate.add(osm);
-            } else if (osm.isDeleted() && !osm.isNew() && osm.isModified()) {
+            } else if (osm.isDeleted() && !osm.isNew() && osm.isModified() && osm.isVisible()) {
                 toDelete.add(osm);
             }
         }
@@ -197,11 +197,11 @@
         toUpdate.clear();
         toDelete.clear();
         for (OsmPrimitive osm: primitives) {
-            if (osm.isNew() && !osm.isDeleted()) {
+            if (osm.isNewOrUndeleted() && !osm.isDeleted()) {
                 toAdd.addLast(osm);
             } else if (osm.isModified() && !osm.isDeleted()) {
                 toUpdate.addLast(osm);
-            } else if (osm.isDeleted() && !osm.isNew() && osm.isModified()) {
+            } else if (osm.isDeleted() && !osm.isNew() && osm.isModified() && osm.isVisible()) {
                 toDelete.addFirst(osm);
             }
         }
@@ -311,7 +311,7 @@
         for (Relation relation: relations) {
             boolean refersToNewRelation = false;
             for (RelationMember m : relation.getMembers()) {
-                if (m.isRelation() && m.getMember().isNew()) {
+                if (m.isRelation() && m.getMember().isNewOrUndeleted()) {
                     refersToNewRelation = true;
                     break;
                 }
@@ -349,12 +349,12 @@
         public void build(Collection<Relation> relations) {
             this.relations = new HashSet<Relation>();
             for(Relation relation: relations) {
-                if (!relation.isNew() ) {
+                if (!relation.isNewOrUndeleted() ) {
                     continue;
                 }
                 this.relations.add(relation);
                 for (RelationMember m: relation.getMembers()) {
-                    if (m.isRelation() && m.getMember().isNew()) {
+                    if (m.isRelation() && m.getMember().isNewOrUndeleted()) {
                         addDependency(relation, (Relation)m.getMember());
                     }
                 }
Index: src/org/openstreetmap/josm/command/DeleteCommand.java
===================================================================
--- src/org/openstreetmap/josm/command/DeleteCommand.java	(revision 3334)
+++ src/org/openstreetmap/josm/command/DeleteCommand.java	(working copy)
@@ -444,7 +444,7 @@
             for (OsmPrimitive osm : primitivesToDelete) {
                 if (osm.isIncomplete())
                     incomplete = true;
-                else if (osm instanceof Node && !osm.isNew()
+                else if (osm instanceof Node && !osm.isNewOrUndeleted()
                 && !a.contains(((Node) osm).getCoor()))
                     outside = true;
             }