diff --git a/src/org/openstreetmap/josm/actions/SimplifyWayAction.java b/src/org/openstreetmap/josm/actions/SimplifyWayAction.java index de0c78790..0999785a7 100644 --- a/src/org/openstreetmap/josm/actions/SimplifyWayAction.java +++ b/src/org/openstreetmap/josm/actions/SimplifyWayAction.java @@ -17,7 +17,11 @@ import java.util.LinkedList; 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,10 +50,12 @@ import org.openstreetmap.josm.gui.HelpAwareOptionPane; 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,6 +64,7 @@ import org.openstreetmap.josm.tools.StreamUtils; * @since 2575 */ public class SimplifyWayAction extends JosmAction { + private static final int MAX_WAYS_NO_ASK = 10; /** * Constructs a new {@code SimplifyWayAction}. @@ -120,14 +127,56 @@ public class SimplifyWayAction extends JosmAction { } /** - * 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 ways, String text, boolean auto) { + AtomicReference returnVar = new AtomicReference<>((double) -1); + AtomicBoolean hasRun = new AtomicBoolean(); + askSimplifyWays(ways, text, auto, (result, command) -> { + 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 waysCollection, String text, boolean auto, SimplifyWayActionListener... listeners) { + final Collection realListeners = Stream.of(listeners).filter(Objects::nonNull).collect(Collectors.toList()); + final List ways; + if (waysCollection instanceof List) { + ways = (List) waysCollection; + } else { + ways = new ArrayList<>(waysCollection); + } IPreferences s = Config.getPref(); String key = "simplify-way." + (auto ? "auto." : ""); String keyRemember = key + "remember"; @@ -135,9 +184,11 @@ public class SimplifyWayAction extends JosmAction { String r = s.get(keyRemember, "ask"); if (auto && "no".equals(r)) { - return -1; + realListeners.forEach(listener -> listener.maxErrorListener(-1, null)); + return; } else if ("yes".equals(r)) { - return s.getDouble(keyError, 3.0); + realListeners.forEach(listener -> listener.maxErrorListener(s.getDouble(keyError, 3.0), null)); + return; } JPanel p = new JPanel(new GridBagLayout()); @@ -178,53 +229,61 @@ public class SimplifyWayAction extends JosmAction { } 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(); + final Command command = l.lastCommand; + 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, command)); + }); + GuiHelper.runInEDT(ed::showDialog); } @Override public void actionPerformed(ActionEvent e) { DataSet ds = getLayerManager().getEditDataSet(); - ds.update(() -> { - List 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; - } - - 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); + List 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; + } - if (err > 0) { - simplifyWays(ways, err); + String lengthstr = SystemOfMeasurement.getSystemOfMeasurement().getDistText( + ways.stream().mapToDouble(Way::getLength).sum()); + + 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, (maxErr, simplifyCommand) -> { + if (maxErr > 0) { + if (simplifyCommand == null) { + simplifyWays(ways, maxErr); + } else { + GuiHelper.runInEDT(() -> UndoRedoHandler.getInstance().add(simplifyCommand)); + } + } } - }); + ); + } /** @@ -308,7 +367,8 @@ public class SimplifyWayAction extends JosmAction { * @since 16566 (private) */ private static SequenceCommand buildSimplifyWaysCommand(List ways, double threshold) { - Collection allCommands = ways.stream() + // Use `parallelStream` since this can significantly decrease the amount of time taken for large numbers of ways + Collection allCommands = ways.parallelStream() .map(way -> createSimplifyCommand(way, threshold)) .filter(Objects::nonNull) .collect(StreamUtils.toUnmodifiableList()); @@ -499,6 +559,7 @@ public class SimplifyWayAction extends JosmAction { private final JLabel nodesToRemove; private final SpinnerNumberModel errorModel; private final List ways; + private ForkJoinTask futureNodesToRemove; SimplifyChangeListener(JLabel nodesToRemove, SpinnerNumberModel errorModel, List ways) { this.nodesToRemove = nodesToRemove; @@ -508,18 +569,80 @@ public class SimplifyWayAction extends JosmAction { @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() { + 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()) { + GuiHelper.runInEDT(() -> nodesToRemove.setText(tr("calculating..."))); + result = updateNodesToRemove(ways, threshold); + return true; + } + } + return false; + } + }); + } + }); + } + + private Command updateNodesToRemove(List ways, double threshold) { + // This must come first in order for everything else to be accurate + if (lastCommand != null && lastCommand.equals(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 + * @param simplify The command that was used to generate stats for the user with the maxError. May be null. + */ + void maxErrorListener(double maxError, Command simplify); + } } diff --git a/src/org/openstreetmap/josm/gui/ExtendedDialog.java b/src/org/openstreetmap/josm/gui/ExtendedDialog.java index a11295f58..fcc616a4d 100644 --- a/src/org/openstreetmap/josm/gui/ExtendedDialog.java +++ b/src/org/openstreetmap/josm/gui/ExtendedDialog.java @@ -42,6 +42,7 @@ import org.openstreetmap.josm.tools.GBC; 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,6 +96,7 @@ public class ExtendedDialog extends JDialog implements IExtendedDialog { private transient Icon icon; private final boolean modal; private boolean focusOnDefaultButton; + private final ListenerList resultListeners = ListenerList.create(); /** true, if the dialog should include a help button */ private boolean showHelpButton; @@ -241,6 +243,7 @@ public class ExtendedDialog extends JDialog implements IExtendedDialog { // Check if the user has set the dialog to not be shown again if (toggleCheckState()) { result = toggleValue; + this.finishResult(); return this; } @@ -388,6 +391,7 @@ public class ExtendedDialog extends JDialog implements IExtendedDialog { protected void buttonAction(int buttonIndex, ActionEvent evt) { result = buttonIndex+1; setVisible(false); + this.finishResult(); } /** @@ -419,6 +423,7 @@ public class ExtendedDialog extends JDialog implements IExtendedDialog { getClass().getName(), actionEvent, new Exception().getStackTrace()[1]); } setVisible(false); + ExtendedDialog.this.finishResult(); } }; @@ -561,4 +566,52 @@ public class ExtendedDialog extends JDialog implements IExtendedDialog { 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 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); + } } diff --git a/src/org/openstreetmap/josm/gui/MainApplication.java b/src/org/openstreetmap/josm/gui/MainApplication.java index 744d8878c..a3ab8ff65 100644 --- a/src/org/openstreetmap/josm/gui/MainApplication.java +++ b/src/org/openstreetmap/josm/gui/MainApplication.java @@ -44,6 +44,7 @@ import java.util.Set; 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,6 +213,42 @@ public class MainApplication { */ 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 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,8 +320,8 @@ public class MainApplication { * 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)); }; /**