Changeset 7204 in josm

Timestamp:

2014-06-01T14:07:18+02:00 (10 years ago)

Author:

Don-vip

Message:

fix #10086 - fix EDT violation causing freeze of Save/upload dialog for empty or conflicting layers

Location:

trunk/src/org/openstreetmap/josm

Files:

• actions/SaveActionBase.java (modified) (1 diff)
• gui/io/SaveLayerInfo.java (modified) (4 diffs)
• gui/io/SaveLayerTask.java (modified) (1 diff)
• gui/io/SaveLayersDialog.java (modified) (1 diff)
• gui/layer/OsmDataLayer.java (modified) (4 diffs)
• gui/util/GuiHelper.java (modified) (4 diffs)

trunk/src/org/openstreetmap/josm/actions/SaveActionBase.java

@@ -56 +56 @@
     }
 
-    public static boolean doSave(Layer layer, File file) {
-        if(!layer.checkSaveConditions())
+    /**
+     * Saves a layer to a given file.
+     * @param layer The layer to save
+     * @param file The destination file
+     * @param checkSaveConditions if {@code true}, checks preconditions before saving. Set it to {@code false} to skip it
+     * if preconditions have already been checked (as this check can prompt UI dialog in EDT it may be best in some cases
+     * to do it earlier).
+     * @return {@code true} if the layer has been successfully saved, {@code false} otherwise
+     * @since 7204
+     */
+    public static boolean doSave(Layer layer, File file, boolean checkSaveConditions) {
+        if (checkSaveConditions && !layer.checkSaveConditions())
             return false;
         return doInternalSave(layer, file);

trunk/src/org/openstreetmap/josm/gui/io/SaveLayerInfo.java

@@ -16 +16 @@
     /** the osm data layer */
     private OsmDataLayer layer;
+    private boolean doCheckSaveConditions;
     private boolean doSaveToFile;
     private boolean doUploadToServer;
@@ -23 +24 @@
 
     /**
-     *
+     * Constructs a new {@code SaveLayerInfo}.
      * @param layer the layer. Must not be null.
      * @throws IllegalArgumentException thrown if layer is null
@@ -30 +31 @@
         CheckParameterUtil.ensureParameterNotNull(layer, "layer");
         this.layer = layer;
+        this.doCheckSaveConditions = true;
         this.doSaveToFile = layer.requiresSaveToFile();
         this.doUploadToServer = layer.requiresUploadToServer() && !layer.isUploadDiscouraged();
@@ -42 +44 @@
     public OsmDataLayer getLayer() {
         return layer;
+    }
+
+    /**
+     * Replies true if preconditions should be checked before saving; false, otherwise
+     *
+     * @return true if preconditions should be checked before saving; false, otherwise
+     * @since 7204
+     */
+    public boolean isDoCheckSaveConditions() {
+        return doCheckSaveConditions;
+    }
+
+    /**
+     * Sets whether preconditions should be checked before saving
+     *
+     * @param doCheckSaveConditions true to check save preconditions; false, to skip checking
+     * @since 7204
+     */
+    public void setDoCheckSaveConditions(boolean doCheckSaveConditions) {
+        this.doCheckSaveConditions = doCheckSaveConditions;
     }
 

trunk/src/org/openstreetmap/josm/gui/io/SaveLayerTask.java

@@ -49 +49 @@
         try {
             parentMonitor.subTask(tr("Saving layer to ''{0}'' ...", layerInfo.getFile().toString()));
-            if (!SaveAction.doSave(layerInfo.getLayer(), layerInfo.getFile())) {
+            if (!SaveAction.doSave(layerInfo.getLayer(), layerInfo.getFile(), layerInfo.isDoCheckSaveConditions())) {
                 setFailed(true);
                 return;

trunk/src/org/openstreetmap/josm/gui/io/SaveLayersDialog.java

@@ -506 +506 @@
                     continue;
                 }
-                currentTask= new SaveLayerTask(layerInfo, monitor);
+                // Check save preconditions earlier to avoid a blocking reentring call to EDT (see #10086)
+                if (layerInfo.isDoCheckSaveConditions()) {
+                    if (!layerInfo.getLayer().checkSaveConditions()) {
+                        continue;
+                    }
+                    layerInfo.setDoCheckSaveConditions(false);
+                }
+                currentTask = new SaveLayerTask(layerInfo, monitor);
                 currentFuture = worker.submit(currentTask);
 

trunk/src/org/openstreetmap/josm/gui/layer/OsmDataLayer.java

@@ -1 +1 @@
 // License: GPL. See LICENSE file for details.
-
 package org.openstreetmap.josm.gui.layer;
 
@@ -28 +27 @@
 import java.util.List;
 import java.util.Map;
+import java.util.concurrent.Callable;
 import java.util.concurrent.CopyOnWriteArrayList;
 
@@ -78 +78 @@
 import org.openstreetmap.josm.gui.progress.PleaseWaitProgressMonitor;
 import org.openstreetmap.josm.gui.progress.ProgressMonitor;
+import org.openstreetmap.josm.gui.util.GuiHelper;
 import org.openstreetmap.josm.gui.widgets.JosmTextArea;
 import org.openstreetmap.josm.tools.DateUtils;
@@ -750 +751 @@
     public boolean checkSaveConditions() {
         if (isDataSetEmpty()) {
-            ExtendedDialog dialog = new ExtendedDialog(
-                    Main.parent,
-                    tr("Empty document"),
-                    new String[] {tr("Save anyway"), tr("Cancel")}
-            );
-            dialog.setContent(tr("The document contains no data."));
-            dialog.setButtonIcons(new String[] {"save.png", "cancel.png"});
-            dialog.showDialog();
-            if (dialog.getValue() != 1) return false;
+            if (1 != GuiHelper.runInEDTAndWaitAndReturn(new Callable<Integer>() {
+                @Override
+                public Integer call() {
+                    ExtendedDialog dialog = new ExtendedDialog(
+                            Main.parent,
+                            tr("Empty document"),
+                            new String[] {tr("Save anyway"), tr("Cancel")}
+                    );
+                    dialog.setContent(tr("The document contains no data."));
+                    dialog.setButtonIcons(new String[] {"save.png", "cancel.png"});
+                    return dialog.showDialog().getValue();
+                }
+            })) {
+                return false;
+            }
         }
 
         ConflictCollection conflicts = getConflicts();
         if (conflicts != null && !conflicts.isEmpty()) {
-            ExtendedDialog dialog = new ExtendedDialog(
-                    Main.parent,
-                    /* I18N: Display title of the window showing conflicts */
-                    tr("Conflicts"),
-                    new String[] {tr("Reject Conflicts and Save"), tr("Cancel")}
-            );
-            dialog.setContent(tr("There are unresolved conflicts. Conflicts will not be saved and handled as if you rejected all. Continue?"));
-            dialog.setButtonIcons(new String[] {"save.png", "cancel.png"});
-            dialog.showDialog();
-            if (dialog.getValue() != 1) return false;
+            if (1 != GuiHelper.runInEDTAndWaitAndReturn(new Callable<Integer>() {
+                @Override
+                public Integer call() {
+                    ExtendedDialog dialog = new ExtendedDialog(
+                            Main.parent,
+                            /* I18N: Display title of the window showing conflicts */
+                            tr("Conflicts"),
+                            new String[] {tr("Reject Conflicts and Save"), tr("Cancel")}
+                    );
+                    dialog.setContent(tr("There are unresolved conflicts. Conflicts will not be saved and handled as if you rejected all. Continue?"));
+                    dialog.setButtonIcons(new String[] {"save.png", "cancel.png"});
+                    return dialog.showDialog().getValue();
+                }
+            })) {
+                return false;
+            }
         }
         return true;

trunk/src/org/openstreetmap/josm/gui/util/GuiHelper.java

@@ -22 +22 @@
 import java.util.Arrays;
 import java.util.List;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.FutureTask;
 
 import javax.swing.GrayFilter;
@@ -68 +71 @@
     }
 
+    /**
+     * Executes asynchronously a runnable in
+     * <a href="http://docs.oracle.com/javase/tutorial/uiswing/concurrency/dispatch.html">Event Dispatch Thread</a>.
+     * @param task The runnable to execute
+     * @see SwingUtilities#invokeLater
+     */
     public static void runInEDT(Runnable task) {
         if (SwingUtilities.isEventDispatchThread()) {
@@ -76 +85 @@
     }
 
+    /**
+     * Executes synchronously a runnable in
+     * <a href="http://docs.oracle.com/javase/tutorial/uiswing/concurrency/dispatch.html">Event Dispatch Thread</a>.
+     * @param task The runnable to execute
+     * @see SwingUtilities#invokeAndWait
+     */
     public static void runInEDTAndWait(Runnable task) {
         if (SwingUtilities.isEventDispatchThread()) {
@@ -84 +99 @@
             } catch (InterruptedException | InvocationTargetException e) {
                 Main.error(e);
+            }
+        }
+    }
+
+    /**
+     * Executes synchronously a callable in
+     * <a href="http://docs.oracle.com/javase/tutorial/uiswing/concurrency/dispatch.html">Event Dispatch Thread</a>
+     * and return a value.
+     * @param callable The callable to execute
+     * @return The computed result
+     * @since 7204
+     */
+    public static <V> V runInEDTAndWaitAndReturn(Callable<V> callable) {
+        if (SwingUtilities.isEventDispatchThread()) {
+            try {
+                return callable.call();
+            } catch (Exception e) {
+                Main.error(e);
+                return null;
+            }
+        } else {
+            FutureTask<V> task = new FutureTask<V>(callable);
+            SwingUtilities.invokeLater(task);
+            try {
+                return task.get();
+            } catch (InterruptedException | ExecutionException e) {
+                Main.error(e);
+                return null;
             }
         }