Ticket #19199: 19199.threading.2.patch

.project

@@ -1 +1 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <projectDescription>
-        <name>JOSM</name>
+        <name>JOSM_clean</name>
         <comment></comment>
         <projects>
         </projects>

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

@@ -17 +17 @@
 import java.util.List;
 import java.util.Objects;
 import java.util.Set;
+import java.util.concurrent.ForkJoinTask;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicReference;
 import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 import javax.swing.BorderFactory;
 import javax.swing.JCheckBox;
@@ -46 +50 @@
 import org.openstreetmap.josm.gui.HelpAwareOptionPane.ButtonSpec;
 import org.openstreetmap.josm.gui.MainApplication;
 import org.openstreetmap.josm.gui.Notification;
+import org.openstreetmap.josm.gui.util.GuiHelper;
 import org.openstreetmap.josm.spi.preferences.Config;
 import org.openstreetmap.josm.spi.preferences.IPreferences;
 import org.openstreetmap.josm.tools.GBC;
 import org.openstreetmap.josm.tools.ImageProvider;
+import org.openstreetmap.josm.tools.Logging;
 import org.openstreetmap.josm.tools.Shortcut;
 import org.openstreetmap.josm.tools.StreamUtils;
 
@@ -58 +64 @@
  * @since 2575
  */
 public class SimplifyWayAction extends JosmAction {
+    private static final int MAX_WAYS_NO_ASK = 10;
 
     /**
      * Constructs a new {@code SimplifyWayAction}.
@@ -120 +127 @@
     }
 
     /**
-     * Asks the user for max-err value used to simplify ways, if not remembered before
+     * Asks the user for max-err value used to simplify ways, if not remembered before. This is not asynchronous.
      * @param ways the ways that are being simplified (to show estimated number of nodes to be removed)
      * @param text the text being shown
      * @param auto whether it's called automatically (conversion) or by the user
      * @return the max-err value or -1 if canceled
+     * @see #askSimplifyWays(Collection, String, boolean, SimplifyWayActionListener...)
      * @since 16566
      */
     public static double askSimplifyWays(List<Way> ways, String text, boolean auto) {
+        AtomicReference<Double> returnVar = new AtomicReference<>((double) -1);
+        AtomicBoolean hasRun = new AtomicBoolean();
+        askSimplifyWays(ways, text, auto, result -> {
+            synchronized (returnVar) {
+                returnVar.set(result);
+                returnVar.notifyAll();
+                hasRun.set(true);
+            }
+        });
+        if (!SwingUtilities.isEventDispatchThread()) {
+            synchronized (returnVar) {
+                while (!hasRun.get()) {
+                    try {
+                        // Wait 2 seconds
+                        returnVar.wait(2000);
+                    } catch (InterruptedException e) {
+                        Logging.error(e);
+                        Thread.currentThread().interrupt();
+                    }
+                }
+            }
+        }
+        return returnVar.get();
+    }
+
+    /**
+     * Asks the user for max-err value used to simplify ways, if not remembered before
+     * @param waysCollection the ways that are being simplified (to show estimated number of nodes to be removed)
+     * @param text the text being shown
+     * @param auto whether it's called automatically (conversion) or by the user
+     * @param listeners Listeners to call when the max error is calculated
+     * @since xxx
+     */
+    public static void askSimplifyWays(Collection<Way> waysCollection, String text, boolean auto, SimplifyWayActionListener... listeners) {
+        final Collection<SimplifyWayActionListener> realListeners = Stream.of(listeners).filter(Objects::nonNull).collect(Collectors.toList());
+        final List<Way> ways;
+        if (waysCollection instanceof List) {
+            ways = (List<Way>) waysCollection;
+        } else {
+            ways = new ArrayList<>(waysCollection);
+        }
         IPreferences s = Config.getPref();
         String key = "simplify-way." + (auto ? "auto." : "");
         String keyRemember = key + "remember";
@@ -135 +184 @@
 
         String r = s.get(keyRemember, "ask");
         if (auto && "no".equals(r)) {
-            return -1;
+            realListeners.forEach(listener -> listener.maxErrorListener(-1));
+            return;
         } else if ("yes".equals(r)) {
-            return s.getDouble(keyError, 3.0);
+            realListeners.forEach(listener -> listener.maxErrorListener(s.getDouble(keyError, 3.0)));
+            return;
         }
 
         JPanel p = new JPanel(new GridBagLayout());
@@ -178 +229 @@
         } else {
             ed.setButtonIcons("ok", "cancel");
         }
-
-        int ret = ed.showDialog().getValue();
-        double val = (double) n.getValue();
-        if (l.lastCommand != null && l.lastCommand.equals(UndoRedoHandler.getInstance().getLastCommand())) {
-            UndoRedoHandler.getInstance().undo();
-            l.lastCommand = null;
-        }
-        if (ret == 1) {
-            s.putDouble(keyError, val);
-            if (c.isSelected()) {
-                s.put(keyRemember, "yes");
+        ed.addResultListener(ret -> {
+            final double result;
+            double val = (double) n.getValue();
+            if (l.lastCommand != null && l.lastCommand.equals(UndoRedoHandler.getInstance().getLastCommand())) {
+                UndoRedoHandler.getInstance().undo();
+                l.lastCommand = null;
             }
-            return val;
-        } else {
-            if (auto && c.isSelected()) { //do not remember cancel for manual simplify, otherwise nothing would happen
-                s.put(keyRemember, "no");
+            if (ret == 1) {
+                s.putDouble(keyError, val);
+                if (c.isSelected()) {
+                    s.put(keyRemember, "yes");
+                }
+                result = val;
+            } else {
+                if (auto && c.isSelected()) { //do not remember cancel for manual simplify, otherwise nothing would happen
+                    s.put(keyRemember, "no");
+                }
+                result = -1;
             }
-            return -1;
-        }
+            realListeners.forEach(listener -> listener.maxErrorListener(result));
+        });
+        GuiHelper.runInEDT(ed::showDialog);
     }
 
     @Override
     public void actionPerformed(ActionEvent e) {
         DataSet ds = getLayerManager().getEditDataSet();
-        ds.update(() -> {
-            List<Way> ways = ds.getSelectedWays().stream()
-                    .filter(p -> !p.isIncomplete())
-                    .collect(Collectors.toList());
-            if (ways.isEmpty()) {
-                alertSelectAtLeastOneWay();
-                return;
-            } else if (!confirmWayWithNodesOutsideBoundingBox(ways) || (ways.size() > 10 && !confirmSimplifyManyWays(ways.size()))) {
-                return;
-            }
+        List<Way> ways = ds.getSelectedWays().stream()
+                .filter(p -> !p.isIncomplete())
+                .collect(Collectors.toList());
+        if (ways.isEmpty()) {
+            alertSelectAtLeastOneWay();
+            return;
+        } else if (!confirmWayWithNodesOutsideBoundingBox(ways) || (ways.size() > MAX_WAYS_NO_ASK && !confirmSimplifyManyWays(ways.size()))) {
+            return;
+        }
 
-            String lengthstr = SystemOfMeasurement.getSystemOfMeasurement().getDistText(
-                    ways.stream().mapToDouble(Way::getLength).sum());
+        String lengthstr = SystemOfMeasurement.getSystemOfMeasurement().getDistText(
+                ways.stream().mapToDouble(Way::getLength).sum());
 
-            double err = askSimplifyWays(ways, trn(
-                    "You are about to simplify {0} way with a total length of {1}.",
-                    "You are about to simplify {0} ways with a total length of {1}.",
-                    ways.size(), ways.size(), lengthstr), false);
+        double err = askSimplifyWays(ways, trn(
+                "You are about to simplify {0} way with a total length of {1}.",
+                "You are about to simplify {0} ways with a total length of {1}.",
+                ways.size(), ways.size(), lengthstr), false);
 
-            if (err > 0) {
-                simplifyWays(ways, err);
-            }
-        });
+        if (err > 0) {
+            simplifyWays(ways, err);
+        }
     }
 
     /**
@@ -308 +360 @@
      * @since 16566 (private)
      */
     private static SequenceCommand buildSimplifyWaysCommand(List<Way> ways, double threshold) {
-        Collection<Command> allCommands = ways.stream()
+        // Use `parallelStream` since this can significantly decrease the amount of time taken for large numbers of ways
+        Collection<Command> allCommands = ways.parallelStream()
                 .map(way -> createSimplifyCommand(way, threshold))
                 .filter(Objects::nonNull)
                 .collect(StreamUtils.toUnmodifiableList());
@@ -499 +552 @@
         private final JLabel nodesToRemove;
         private final SpinnerNumberModel errorModel;
         private final List<Way> ways;
+        private ForkJoinTask<?> futureNodesToRemove;
 
         SimplifyChangeListener(JLabel nodesToRemove, SpinnerNumberModel errorModel, List<Way> ways) {
             this.nodesToRemove = nodesToRemove;
@@ -508 +562 @@
 
         @Override
         public void stateChanged(ChangeEvent e) {
-            if (Objects.equals(UndoRedoHandler.getInstance().getLastCommand(), lastCommand)) {
-                UndoRedoHandler.getInstance().undo();
+            MainApplication.worker.execute(() -> {
+                synchronized (errorModel) {
+                    double threshold = errorModel.getNumber().doubleValue();
+                    if (futureNodesToRemove != null) {
+                        futureNodesToRemove.cancel(false);
+                    }
+                    futureNodesToRemove = MainApplication.getForkJoinPool().submit(new ForkJoinTask<Command>() {
+                        private Command result;
+
+                        @Override
+                        public Command getRawResult() {
+                            return result;
+                        }
+
+                        @Override
+                        protected void setRawResult(Command value) {
+                            this.result = value;
+                        }
+
+                        @Override
+                        protected boolean exec() {
+                            synchronized (SimplifyChangeListener.this) {
+                                if (!this.isCancelled()) {
+                                    result = updateNodesToRemove(ways, threshold);
+                                    return true;
+                                }
+                            }
+                            return false;
+                        }
+                    });
+                }
+            });
+        }
+
+        private Command updateNodesToRemove(List<Way> ways, double threshold) {
+            // This must come first in order for everything else to be accurate
+            if (lastCommand != null && Objects.equals(lastCommand, UndoRedoHandler.getInstance().getLastCommand())) {
+                // We need to wait to avoid cases where we cannot undo due threading issues.
+                GuiHelper.runInEDTAndWait(() -> UndoRedoHandler.getInstance().undo());
             }
-            double threshold = errorModel.getNumber().doubleValue();
-            int removeNodes = simplifyWaysCountNodesRemoved(ways, threshold);
-            nodesToRemove.setText(trn(
-                    "(about {0} node to remove)",
-                    "(about {0} nodes to remove)", removeNodes, removeNodes));
-            lastCommand = SimplifyWayAction.buildSimplifyWaysCommand(ways, threshold);
-            if (lastCommand != null) {
-                UndoRedoHandler.getInstance().add(lastCommand);
+            final Command command = buildSimplifyWaysCommand(ways, threshold);
+            // This is the same code from simplifyWaysCountNodesRemoved, but is duplicated for performance reasons
+            // (this avoids running the code to calculate the command twice).
+            int removeNodes = command == null ? 0 : (int) command.getParticipatingPrimitives().stream()
+                    .filter(Node.class::isInstance)
+                    .count();
+            int totalNodes = ways.parallelStream().mapToInt(Way::getNodesCount).sum();
+            int percent = (int) Math.round(100 * removeNodes / (double) totalNodes);
+            GuiHelper.runInEDTAndWait(() ->
+                    nodesToRemove.setText(trn(
+                            "(about {0} node to remove ({1}%))",
+                            "(about {0} nodes to remove ({1}%))", removeNodes, removeNodes, percent))
+            );
+            lastCommand = command;
+            if (lastCommand != null && ways.size() < MAX_WAYS_NO_ASK) {
+                GuiHelper.runInEDTAndWait(() -> UndoRedoHandler.getInstance().add(lastCommand));
             }
+            return lastCommand;
         }
     }
+
+
+    /**
+     * A listener that is called when the {@link #askSimplifyWays} method is called.
+     * @since xxx
+     */
+    @FunctionalInterface
+    public interface SimplifyWayActionListener {
+        /**
+         * Called when the max error is set
+         * @param maxError The max error
+         */
+        void maxErrorListener(double maxError);
+    }
 }

src/org/openstreetmap/josm/gui/ExtendedDialog.java

@@ -42 +42 @@
 import org.openstreetmap.josm.tools.ImageProvider;
 import org.openstreetmap.josm.tools.ImageProvider.ImageSizes;
 import org.openstreetmap.josm.tools.InputMapUtils;
+import org.openstreetmap.josm.tools.ListenerList;
 import org.openstreetmap.josm.tools.Logging;
 import org.openstreetmap.josm.tools.Utils;
 
@@ -95 +96 @@
     private transient Icon icon;
     private final boolean modal;
     private boolean focusOnDefaultButton;
+    private final ListenerList<ExtendedDialogResultListener> resultListeners = ListenerList.create();
 
     /** true, if the dialog should include a help button */
     private boolean showHelpButton;
@@ -241 +243 @@
         // Check if the user has set the dialog to not be shown again
         if (toggleCheckState()) {
             result = toggleValue;
+            this.finishResult();
             return this;
         }
 
@@ -388 +391 @@
     protected void buttonAction(int buttonIndex, ActionEvent evt) {
         result = buttonIndex+1;
         setVisible(false);
+        this.finishResult();
     }
 
     /**
@@ -419 +423 @@
                             getClass().getName(), actionEvent, new Exception().getStackTrace()[1]);
                 }
                 setVisible(false);
+                ExtendedDialog.this.finishResult();
             }
         };
 
@@ -561 +566 @@
             HelpBrowser.setUrlForHelpTopic(helpTopic);
         }
     }
+
+    /**
+     * Add a listener for the results. This makes this dialog non-modal, and stops blocking the EDT.
+     * @param listener The listener to add
+     * @since xxx
+     */
+    public void addResultListener(ExtendedDialogResultListener listener) {
+        if (this.isModal()) {
+            this.setModal(false);
+        }
+        this.resultListeners.addListener(listener);
+    }
+
+    /**
+     * Remove a result listener. If there are no more result listeners, this dialog will become modal, and will block the EDT.
+     * @param listener The listener to remove
+     * @since xxx
+     */
+    public void removeResultListener(ExtendedDialogResultListener listener) {
+        this.resultListeners.removeListener(listener);
+        if (!this.resultListeners.hasListeners()) {
+            this.setModal(true);
+        }
+    }
+
+    /**
+     * Call when the result has been calculated. There is no point in keeping the listeners, so remove them all.
+     */
+    private void finishResult() {
+        this.resultListeners.fireEvent(listener -> listener.parseResult(result));
+        final List<ExtendedDialogResultListener> listeners = new ArrayList<>();
+        this.resultListeners.fireEvent(listeners::add);
+        for (ExtendedDialogResultListener listener : listeners) {
+            this.removeResultListener(listener);
+        }
+    }
+
+    /**
+     * A listener for results
+     * @since xxx
+     */
+    public interface ExtendedDialogResultListener {
+        /**
+         * Do something with a result
+         * @param result The result when the dialog is finished
+         */
+        void parseResult(int result);
+    }
 }

src/org/openstreetmap/josm/gui/MainApplication.java

@@ -44 +44 @@
 import java.util.TreeSet;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
+import java.util.concurrent.ForkJoinPool;
 import java.util.concurrent.Future;
 import java.util.logging.Level;
 import java.util.stream.Collectors;
@@ -212 +213 @@
      */
     public static final ExecutorService worker = new ProgressMonitorExecutor("main-worker-%d", Thread.NORM_PRIORITY);
 
+    /**
+     * This class literally exists to ensure that consumers cannot shut the pool down.
+     * @since xxx
+     */
+    private static class ForkJoinPoolAlways extends ForkJoinPool {
+        ForkJoinPoolAlways() {
+            // Leave a processor for the UI.
+            super(Math.max(1, Runtime.getRuntime().availableProcessors() - 1), ForkJoinPool.defaultForkJoinWorkerThreadFactory,
+                    new BugReportExceptionHandler(), false);
+        }
+
+        @Override
+        public void shutdown() {
+            // Do nothing. This is functionally the same as for the commonPool.
+        }
+
+        @Override
+        public List<Runnable> shutdownNow() {
+            // Do nothing. This is functionally the same as for the commonPool.
+            return Collections.emptyList();
+        }
+    }
+
+    // If there is a security manager, ForkJoinPool.commonPool uses an "InnocuousForkJoinWorkerThreadFactory", which can kill stuff calling
+    // repaint. Otherwise, the common pool is better from (most) perspectives.
+    private static final ForkJoinPool forkJoinPool = System.getSecurityManager() != null ? new ForkJoinPoolAlways() : ForkJoinPool.commonPool();
+
+    /**
+     * Get a generic {@link ForkJoinPool}.
+     * @return A ForkJoinPool to use. Calling {@link ForkJoinPool#shutdown()} or {@link ForkJoinPool#shutdownNow()} has no effect.
+     * @since xxx
+     */
+    public static final ForkJoinPool getForkJoinPool() {
+        return forkJoinPool;
+    }
+
     /**
      * Provides access to the layers displayed in the main view.
      */
@@ -283 +320 @@
      * Listener that sets the enabled state of undo/redo menu entries.
      */
     final CommandQueueListener redoUndoListener = (queueSize, redoSize) -> {
-            menu.undo.setEnabled(queueSize > 0);
-            menu.redo.setEnabled(redoSize > 0);
+            GuiHelper.runInEDT(() -> menu.undo.setEnabled(queueSize > 0));
+            GuiHelper.runInEDT(() -> menu.redo.setEnabled(redoSize > 0));
         };
 
     /**