Index: /applications/editors/josm/plugins/mapillary/src/org/openstreetmap/josm/plugins/mapillary/history/commands/CommandJoin.java =================================================================== --- /applications/editors/josm/plugins/mapillary/src/org/openstreetmap/josm/plugins/mapillary/history/commands/CommandJoin.java (revision 32658) +++ /applications/editors/josm/plugins/mapillary/src/org/openstreetmap/josm/plugins/mapillary/history/commands/CommandJoin.java (revision 32659) @@ -4,5 +4,5 @@ import static org.openstreetmap.josm.tools.I18n.tr; -import java.util.List; +import java.util.Arrays; import java.util.concurrent.ConcurrentSkipListSet; @@ -30,15 +30,16 @@ * if the List size is different from 2. */ - public CommandJoin(List 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(); } Index: /applications/editors/josm/plugins/mapillary/src/org/openstreetmap/josm/plugins/mapillary/mode/JoinMode.java =================================================================== --- /applications/editors/josm/plugins/mapillary/src/org/openstreetmap/josm/plugins/mapillary/mode/JoinMode.java (revision 32658) +++ /applications/editors/josm/plugins/mapillary/src/org/openstreetmap/josm/plugins/mapillary/mode/JoinMode.java (revision 32659) @@ -51,10 +51,7 @@ (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()) { Index: /applications/editors/josm/plugins/mapillary/src/org/openstreetmap/josm/plugins/mapillary/utils/MapillaryUtils.java =================================================================== --- /applications/editors/josm/plugins/mapillary/src/org/openstreetmap/josm/plugins/mapillary/utils/MapillaryUtils.java (revision 32658) +++ /applications/editors/josm/plugins/mapillary/src/org/openstreetmap/josm/plugins/mapillary/utils/MapillaryUtils.java (revision 32659) @@ -155,34 +155,28 @@ * 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,39 +234,40 @@ * 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 firstHalf = new ArrayList<>( - firstImage.getSequence().getImages().subList(0, - firstImage.getSequence().getImages().indexOf(secondImage))); - ArrayList 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(); + } + } } Index: /applications/editors/josm/plugins/mapillary/test/unit/org/openstreetmap/josm/plugins/mapillary/history/MapillaryRecordTest.java =================================================================== --- /applications/editors/josm/plugins/mapillary/test/unit/org/openstreetmap/josm/plugins/mapillary/history/MapillaryRecordTest.java (revision 32658) +++ /applications/editors/josm/plugins/mapillary/test/unit/org/openstreetmap/josm/plugins/mapillary/history/MapillaryRecordTest.java (revision 32659) @@ -177,6 +177,6 @@ @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,11 +189,14 @@ 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,8 +206,6 @@ @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,6 +239,6 @@ @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(