Ticket #23399: 23399.patch

File 23399.patch, 7.5 KB (added by taylor.smock, 14 months ago)

  • core/src/org/openstreetmap/josm/actions/SimplifyWayAction.java 

    Subject: [PATCH] Fix #23399: Simplify way crashes

This was primarily caused by selection updates occurring while the commands were generated. The fixes could be any of the following:
* Synchronized collections
* New list
* Avoiding selection updates

The last option was taken for the following reasons:
* Avoiding a StackOverflow exception in 1k way test
* Reduced memory allocations in 1k way test (>11GB to <250MB)
* Reduced CPU cycles in 1k way test (16s to 2.7s)
---
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
diff --git a/core/src/org/openstreetmap/josm/actions/SimplifyWayAction.java b/core/src/org/openstreetmap/josm/actions/SimplifyWayAction.java
    a b  
    5656import org.openstreetmap.josm.tools.GBC;
    5757import org.openstreetmap.josm.tools.ImageProvider;
    5858import org.openstreetmap.josm.tools.Shortcut;
    59 import org.openstreetmap.josm.tools.StreamUtils;
    6059
    6160/**
    6261 * Delete unnecessary nodes from a way
    … …  
    340339     * @since 16566 (private)
    341340     */
    342341    private static SequenceCommand buildSimplifyWaysCommand(List<Way> ways, double threshold) {
    343         Collection<Command> allCommands = ways.stream()
    344                 .map(way -> createSimplifyCommand(way, threshold))
    345                 .filter(Objects::nonNull)
    346                 .collect(StreamUtils.toUnmodifiableList());
     342        List<Command> allCommands = new ArrayList<>(ways.size());
     343        for (Way way : ways) {
     344            Command command = createSimplifyCommand(way, threshold, false);
     345            if (command != null) {
     346                allCommands.add(command);
     347            }
     348        }
    347349        if (allCommands.isEmpty())
    348350            return null;
     351        final List<OsmPrimitive> deletedPrimitives = allCommands.stream()
     352                .map(Command::getChildren)
     353                .flatMap(Collection::stream)
     354                .filter(DeleteCommand.class::isInstance)
     355                .map(DeleteCommand.class::cast)
     356                .map(DeleteCommand::getParticipatingPrimitives)
     357                .flatMap(Collection::stream)
     358                .collect(Collectors.toList());
     359        allCommands.get(0).getAffectedDataSet().clearSelection(deletedPrimitives);
    349360        return new SequenceCommand(
    350361                trn("Simplify {0} way", "Simplify {0} ways", allCommands.size(), allCommands.size()),
    351                 allCommands);
     362                Collections.unmodifiableList(allCommands));
    352363    }
    353364
    354365    /**
    … …  
    371382     * @since 15419
    372383     */
    373384    public static SequenceCommand createSimplifyCommand(Way w, double threshold) {
     385        return createSimplifyCommand(w, threshold, true);
     386    }
     387
     388    /**
     389     * Creates the SequenceCommand to simplify a way with a given threshold.
     390     *
     391     * @param w the way to simplify
     392     * @param threshold the max error threshold
     393     * @param deselect {@code true} if we want to deselect the deleted nodes
     394     * @return The sequence of commands to run
     395     */
     396    private static SequenceCommand createSimplifyCommand(Way w, double threshold, boolean deselect) {
    374397        int lower = 0;
    375398        int i = 0;
    376399
    … …  
    417440        Collection<Command> cmds = new LinkedList<>();
    418441        cmds.add(new ChangeNodesCommand(w, newNodes));
    419442        cmds.add(new DeleteCommand(w.getDataSet(), delNodes));
    420         w.getDataSet().clearSelection(delNodes);
     443        if (deselect) {
     444            w.getDataSet().clearSelection(delNodes);
     445        }
    421446        return new SequenceCommand(
    422447                trn("Simplify Way (remove {0} node)", "Simplify Way (remove {0} nodes)", delNodes.size(), delNodes.size()), cmds);
    423448    }
    … …  
    571596            final List<Way> newWays = dataSet.getSelectedWays().stream()
    572597                    .filter(p -> !p.isIncomplete())
    573598                    .collect(Collectors.toList());
     599            if (Objects.deepEquals(newWays, this.wayList)) {
     600                return;
     601            }
    574602            this.wayList.clear();
    575603            this.wayList.addAll(newWays);
    576604            if (this.consumer != null) {

  • core/test/unit/org/openstreetmap/josm/actions/SimplifyWayActionTest.java 

    IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
diff --git a/core/test/unit/org/openstreetmap/josm/actions/SimplifyWayActionTest.java b/core/test/unit/org/openstreetmap/josm/actions/SimplifyWayActionTest.java
    a b  
    11// License: GPL. For details, see LICENSE file.
    22package org.openstreetmap.josm.actions;
    33
     4import static org.junit.jupiter.api.Assertions.assertAll;
     5import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
    46import static org.junit.jupiter.api.Assertions.assertEquals;
     7import static org.junit.jupiter.api.Assertions.assertFalse;
    58import static org.junit.jupiter.api.Assertions.assertNotNull;
    69
    710import java.io.IOException;
    … …  
    2427import org.openstreetmap.josm.data.osm.DataSet;
    2528import org.openstreetmap.josm.data.osm.Node;
    2629import org.openstreetmap.josm.data.osm.Way;
     30import org.openstreetmap.josm.gui.ExtendedDialog;
    2731import org.openstreetmap.josm.gui.MainApplication;
     32import org.openstreetmap.josm.gui.layer.OsmDataLayer;
     33import org.openstreetmap.josm.gui.util.GuiHelper;
    2834import org.openstreetmap.josm.io.IllegalDataException;
    2935import org.openstreetmap.josm.io.OsmReader;
    3036import org.openstreetmap.josm.testutils.annotations.Main;
    3137import org.openstreetmap.josm.testutils.annotations.Projection;
     38import org.openstreetmap.josm.testutils.mockers.ExtendedDialogMocker;
    3239import org.openstreetmap.josm.tools.Utils;
    3340
    3441/**
    … …  
    99106        assertEquals(1, deleteCommands.size());
    100107        assertEquals(Collections.singleton(n1), deleteCommands.iterator().next().getParticipatingPrimitives());
    101108    }
     109
     110    /**
     111     * Non-regression test for #23399
     112     */
     113    @Test
     114    void testNonRegression23399() {
     115        new ExtendedDialogMocker(Collections.singletonMap("Simplify way", "Simplify")) {
     116            @Override
     117            protected String getString(ExtendedDialog instance) {
     118                return instance.getTitle();
     119            }
     120        };
     121        final ArrayList<Way> ways = new ArrayList<>(1000);
     122        final DataSet ds = new DataSet();
     123        for (int i = 0; i < 1000; i++) {
     124            final Way way = TestUtils.newWay("", new Node(new LatLon(0, 0)), new Node(new LatLon(0, 0.001)),
     125                    new Node(new LatLon(0, 0.002)));
     126            ways.add(way);
     127            ds.addPrimitiveRecursive(way);
     128        }
     129        MainApplication.getLayerManager().addLayer(new OsmDataLayer(ds, "SimplifyWayActionTest#testNonRegression23399", null));
     130        GuiHelper.runInEDTAndWait(() -> ds.setSelected(ds.allPrimitives()));
     131        assertEquals(ds.allPrimitives().size(), ds.getAllSelected().size());
     132        assertDoesNotThrow(() -> GuiHelper.runInEDTAndWaitWithException(() -> action.actionPerformed(null)));
     133        assertAll(ways.stream().map(way -> () -> assertEquals(2, way.getNodesCount())));
     134        assertAll(ds.getAllSelected().stream().map(p -> () -> assertFalse(p.isDeleted())));
     135        assertEquals(3000, ds.getAllSelected().size());
     136    }
    102137}