Changeset 32659 in osm for applications/editors

Timestamp:

2016-07-15T15:25:20+02:00 (8 years ago)

Author:

floscher

Message:

[mapillary] Rework join and unjoin command

Location:

applications/editors/josm/plugins/mapillary

Files:

• src/org/openstreetmap/josm/plugins/mapillary/history/commands/CommandJoin.java (modified) (2 diffs)
• src/org/openstreetmap/josm/plugins/mapillary/mode/JoinMode.java (modified) (1 diff)
• src/org/openstreetmap/josm/plugins/mapillary/utils/MapillaryUtils.java (modified) (2 diffs)
• test/unit/org/openstreetmap/josm/plugins/mapillary/history/MapillaryRecordTest.java (modified) (4 diffs)

applications/editors/josm/plugins/mapillary/src/org/openstreetmap/josm/plugins/mapillary/history/commands/CommandJoin.java

@@ -4 +4 @@
 import static org.openstreetmap.josm.tools.I18n.tr;
 
-import java.util.List;
+import java.util.Arrays;
 import java.util.concurrent.ConcurrentSkipListSet;
 
@@ -30 +30 @@
    *           if the List size is different from 2.
    */
-  public CommandJoin(List<MapillaryAbstractImage> images) {
-    super(new ConcurrentSkipListSet<>(images));
-    a = images.get(0);
-    b = images.get(1);
-    if (images.size() != 2)
-      throw new IllegalArgumentException();
+  public CommandJoin(final MapillaryAbstractImage a, final MapillaryAbstractImage b) {
+    super(new ConcurrentSkipListSet<>(Arrays.asList(new MapillaryAbstractImage[]{a, b}))); // throws NPE if a or b is null
+    if (a.getSequence() == b.getSequence()) {
+      throw new IllegalArgumentException("Both images must be in different sequences for joining.");
+    }
+    this.a = a;
+    this.b = b;
   }
 
   @Override
   public void execute() {
-    this.redo();
+    redo();
   }
 

applications/editors/josm/plugins/mapillary/src/org/openstreetmap/josm/plugins/mapillary/mode/JoinMode.java

@@ -51 +51 @@
         (this.data.getHighlightedImage().previous() == null && this.lastClick.next() == null
           || this.data.getHighlightedImage().next() == null && this.lastClick.previous() == null)
-        && (this.data.getHighlightedImage().getSequence() != this.lastClick.getSequence() || this.lastClick.getSequence() == null)
+        && this.data.getHighlightedImage().getSequence() != this.lastClick.getSequence()
       ) {
-
-        MapillaryRecord.getInstance().addCommand(
-            new CommandJoin(Arrays.asList(new MapillaryAbstractImage[] {
-                this.lastClick, this.data.getHighlightedImage() })));
+        MapillaryRecord.getInstance().addCommand(new CommandJoin(this.lastClick, this.data.getHighlightedImage()));
       } else if (this.lastClick.next() == this.data.getHighlightedImage()
           || this.lastClick.previous() == this.data.getHighlightedImage()) {

applications/editors/josm/plugins/mapillary/src/org/openstreetmap/josm/plugins/mapillary/utils/MapillaryUtils.java

@@ -155 +155 @@
    * Joins two images into the same sequence. One of them must be the last image of a sequence, the other one the beginning of a different one.
    *
-   * @param mapillaryAbstractImage the first image, into whose sequence the images from the sequence of the second image are merged
-   * @param mapillaryAbstractImage2 the second image, whose sequence is merged into the sequence of the first image
-   */
-  public static synchronized void join(
-          MapillaryAbstractImage mapillaryAbstractImage,
-          MapillaryAbstractImage mapillaryAbstractImage2) {
-    MapillaryAbstractImage firstImage = mapillaryAbstractImage;
-    MapillaryAbstractImage secondImage = mapillaryAbstractImage2;
-
-    if (mapillaryAbstractImage.next() != null) {
-      firstImage = mapillaryAbstractImage2;
-      secondImage = mapillaryAbstractImage;
-    }
-    if (firstImage.getSequence() == null) {
-      MapillarySequence seq = new MapillarySequence();
-      seq.add(firstImage);
-      firstImage.setSequence(seq);
-    }
-    if (secondImage.getSequence() == null) {
-      MapillarySequence seq = new MapillarySequence();
-      seq.add(secondImage);
-      mapillaryAbstractImage2.setSequence(seq);
-    }
-
-    for (MapillaryAbstractImage img : secondImage.getSequence().getImages()) {
-      firstImage.getSequence().add(img);
-      img.setSequence(firstImage.getSequence());
-    }
-    if (Main.main != null)
-      MapillaryData.dataUpdated();
+   * @param imgA the first image, into whose sequence the images from the sequence of the second image are merged
+   * @param imgB the second image, whose sequence is merged into the sequence of the first image
+   */
+  public static synchronized void join(MapillaryAbstractImage imgA, MapillaryAbstractImage imgB) {
+    if (imgA == null || imgB == null) {
+      throw new IllegalArgumentException("Both images must be non-null for joining.");
+    }
+    if (imgA.getSequence() == imgB.getSequence()) {
+      throw new IllegalArgumentException("You can only join images of different sequences.");
+    }
+    if ((imgA.next() != null || imgB.previous() != null) && (imgB.next() != null || imgA.previous() != null)) {
+      throw new IllegalArgumentException("You can only join an image at the end of a sequence with one at the beginning of another sequence.");
+    }
+    if (imgA.next() != null || imgB.previous() != null) {
+      join(imgB, imgA);
+    } else {
+      for (MapillaryAbstractImage img : imgB.getSequence().getImages()) {
+        imgA.getSequence().add(img);
+        img.setSequence(imgA.getSequence());
+      }
+      if (Main.main != null) {
+        MapillaryData.dataUpdated();
+      }
+    }
   }
 
@@ -240 +234 @@
    * All others are put into the second new sequence.
    *
-   * @param mapillaryAbstractImage one of the images marking where to split the sequence
-   * @param mapillaryAbstractImage2 the other image marking where to split the sequence, needs to be a direct neighbour of {@code mapillaryAbstractImage} in the sequence.
-   */
-  public static synchronized void unjoin(
-          MapillaryAbstractImage mapillaryAbstractImage,
-          MapillaryAbstractImage mapillaryAbstractImage2) {
-    MapillaryAbstractImage firstImage = mapillaryAbstractImage;
-    MapillaryAbstractImage secondImage = mapillaryAbstractImage2;
-
-    if (mapillaryAbstractImage.next() != mapillaryAbstractImage2) {
-      firstImage = mapillaryAbstractImage2;
-      secondImage = mapillaryAbstractImage;
-    }
-
-    ArrayList<MapillaryAbstractImage> firstHalf = new ArrayList<>(
-            firstImage.getSequence().getImages().subList(0,
-                    firstImage.getSequence().getImages().indexOf(secondImage)));
-    ArrayList<MapillaryAbstractImage> secondHalf = new ArrayList<>(
-            firstImage.getSequence().getImages().subList(
-                    firstImage.getSequence().getImages().indexOf(secondImage),
-                    firstImage.getSequence().getImages().size()));
-
-    MapillarySequence seq1 = new MapillarySequence();
-    MapillarySequence seq2 = new MapillarySequence();
-
-    for (MapillaryAbstractImage img : firstHalf) {
-      img.setSequence(seq1);
-      seq1.add(img);
-    }
-    for (MapillaryAbstractImage img : secondHalf) {
-      img.setSequence(seq2);
-      seq2.add(img);
-    }
-    if (Main.main != null)
-      MapillaryData.dataUpdated();
+   * @param imgA one of the images marking where to split the sequence
+   * @param imgB the other image marking where to split the sequence, needs to be a direct neighbour of {@code imgA} in the sequence.
+   */
+  public static synchronized void unjoin(MapillaryAbstractImage imgA, MapillaryAbstractImage imgB) {
+    if (imgA == null || imgB == null) {
+      throw new IllegalArgumentException("Both images must be non-null for unjoining.");
+    }
+    if (imgA.getSequence() != imgB.getSequence()) {
+      throw new IllegalArgumentException("You can only unjoin with two images from the same sequence.");
+    }
+    if (imgB.equals(imgA.next()) && imgA.equals(imgB.next())) {
+      throw new IllegalArgumentException("When unjoining with two images these must be consecutive in one sequence.");
+    }
+
+    if (imgA.equals(imgB.next())) {
+      unjoin(imgB, imgA);
+    } else {
+      MapillarySequence seqA = new MapillarySequence();
+      MapillarySequence seqB = new MapillarySequence();
+      boolean insideFirstHalf = true;
+      for (MapillaryAbstractImage img : imgA.getSequence().getImages()) {
+        if (insideFirstHalf) {
+          img.setSequence(seqA);
+          seqA.add(img);
+        } else {
+          img.setSequence(seqB);
+          seqB.add(img);
+        }
+        if (img.equals(imgA)) {
+          insideFirstHalf = false;
+        }
+      }
+      if (Main.main != null) {
+        MapillaryData.dataUpdated();
+      }
+    }
   }
 

applications/editors/josm/plugins/mapillary/test/unit/org/openstreetmap/josm/plugins/mapillary/history/MapillaryRecordTest.java

@@ -177 +177 @@
   @Test
   public void commandJoinClass() {
-    CommandJoin cmd1 = new CommandJoin(Arrays.asList(new MapillaryAbstractImage[]{img1, img2}));
-    CommandJoin cmd2 = new CommandJoin(Arrays.asList(new MapillaryAbstractImage[]{img2, img3}));
+    CommandJoin cmd1 = new CommandJoin(img1, img2);
+    CommandJoin cmd2 = new CommandJoin(img2, img3);
 
     this.record.addCommand(cmd1);
@@ -189 +189 @@
     assertEquals(3, img1.getSequence().getImages().size());
     assertEquals(img3, img1.next().next());
-
-    try {
-      this.record.addCommand(new CommandJoin(Arrays.asList(new MapillaryAbstractImage[]{img1, img2, img3})));
-      fail();
-    } catch (IllegalArgumentException e) {
-      // Expected output.
-    }
+  }
+  
+  @Test(expected=NullPointerException.class)
+  public void commandJoinNull1() {
+    new CommandJoin(img1, null);
+  }
+  
+  @Test(expected=NullPointerException.class)
+  public void commandJoinNull2() {
+    new CommandJoin(null, img1);
   }
 
@@ -203 +206 @@
   @Test
   public void commandUnjoinClass() {
-    CommandJoin join1 = new CommandJoin(
-            Arrays.asList(new MapillaryAbstractImage[]{this.img1, this.img2}));
-    CommandJoin join2 = new CommandJoin(
-            Arrays.asList(new MapillaryAbstractImage[]{this.img2, this.img3}));
+    CommandJoin join1 = new CommandJoin(this.img1, this.img2);
+    CommandJoin join2 = new CommandJoin(this.img2, this.img3);
 
     CommandUnjoin cmd1 = new CommandUnjoin(
@@ -238 +239 @@
   @Test
   public void commandDeleteTest() {
-    CommandJoin join1 = new CommandJoin(Arrays.asList(new MapillaryAbstractImage[]{img1, img2}));
-    CommandJoin join2 = new CommandJoin(Arrays.asList(new MapillaryAbstractImage[]{img2, img3}));
+    CommandJoin join1 = new CommandJoin(img1, img2);
+    CommandJoin join2 = new CommandJoin(img2, img3);
 
     CommandDelete cmd1 = new CommandDelete(