Index: trunk/src/org/openstreetmap/josm/actions/UploadAction.java =================================================================== --- trunk/src/org/openstreetmap/josm/actions/UploadAction.java (revision 18751) +++ trunk/src/org/openstreetmap/josm/actions/UploadAction.java (revision 18752) @@ -12,4 +12,8 @@ import java.util.Map; import java.util.Optional; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.Future; +import java.util.function.Consumer; import javax.swing.JOptionPane; @@ -37,4 +41,5 @@ import org.openstreetmap.josm.spi.preferences.Config; import org.openstreetmap.josm.tools.ImageProvider; +import org.openstreetmap.josm.tools.JosmRuntimeException; import org.openstreetmap.josm.tools.Logging; import org.openstreetmap.josm.tools.Shortcut; @@ -43,9 +48,9 @@ /** * Action that opens a connection to the osm server and uploads all changes. - * + *

* A dialog is displayed asking the user to specify a rectangle to grab. * The url and account settings from the preferences are used. - * - * If the upload fails this action offers various options to resolve conflicts. + *

+ * If the upload fails, this action offers various options to resolve conflicts. * * @author imi @@ -56,5 +61,5 @@ * when the user wants to upload data. Plugins can insert their own hooks here * if they want to be able to veto an upload. - * + *

* Be default, the standard upload dialog is the only element in the list. * Plugins should normally insert their code before that, so that the upload @@ -195,6 +200,6 @@ /** - * Check whether the preconditions are met to upload data in apiData. - * Makes sure upload is allowed, primitives in apiData don't participate in conflicts and + * Check whether the preconditions are met to upload data in {@code apiData}. + * Makes sure upload is allowed, primitives in {@code apiData} don't participate in conflicts and * runs the installed {@link UploadHook}s. * @@ -204,23 +209,51 @@ */ public static boolean checkPreUploadConditions(AbstractModifiableLayer layer, APIDataSet apiData) { + try { + return checkPreUploadConditionsAsync(layer, apiData, null).get(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new JosmRuntimeException(e); + } catch (ExecutionException e) { + throw new JosmRuntimeException(e); + } + } + + /** + * Check whether the preconditions are met to upload data in {@code apiData}. + * Makes sure upload is allowed, primitives in {@code apiData} don't participate in conflicts and + * runs the installed {@link UploadHook}s. + * + * @param layer the source layer of the data to be uploaded + * @param apiData the data to be uploaded + * @param onFinish {@code true} if the preconditions are met; {@code false}, otherwise + * @return A future that completes when the checks are finished + * @since 18752 + */ + private static Future checkPreUploadConditionsAsync(AbstractModifiableLayer layer, APIDataSet apiData, Consumer onFinish) { + final CompletableFuture future = new CompletableFuture<>(); + if (onFinish != null) { + future.thenAccept(onFinish); + } if (layer.isUploadDiscouraged() && warnUploadDiscouraged(layer)) { - return false; - } - if (layer instanceof OsmDataLayer) { + future.complete(false); + } else if (layer instanceof OsmDataLayer) { OsmDataLayer osmLayer = (OsmDataLayer) layer; ConflictCollection conflicts = osmLayer.getConflicts(); if (apiData.participatesInConflict(conflicts)) { alertUnresolvedConflicts(osmLayer); - return false; + future.complete(false); } } // Call all upload hooks in sequence. - // FIXME: this should become an asynchronous task - // - if (apiData != null) { - return UPLOAD_HOOKS.stream().allMatch(hook -> hook.checkUpload(apiData)); - } - - return true; + if (!future.isDone()) { + MainApplication.worker.execute(() -> { + boolean hooks = true; + if (apiData != null) { + hooks = UPLOAD_HOOKS.stream().allMatch(hook -> hook.checkUpload(apiData)); + } + future.complete(hooks); + }); + } + return future; } @@ -236,6 +269,20 @@ return; } - if (!checkPreUploadConditions(layer, apiData)) - return; + checkPreUploadConditionsAsync(layer, apiData, passed -> GuiHelper.runInEDT(() -> { + if (Boolean.TRUE.equals(passed)) { + realUploadData(layer, apiData); + } else { + new Notification(tr("One of the upload verification processes failed")).show(); + } + })); + } + + /** + * Uploads data to the OSM API. + * + * @param layer the source layer for the data to upload + * @param apiData the primitives to be added, updated, or deleted + */ + private static void realUploadData(final OsmDataLayer layer, final APIDataSet apiData) { ChangesetUpdater.check(); @@ -284,7 +331,5 @@ uploadStrategySpecification, layer, apiData, cs); - if (asyncUploadTask.isPresent()) { - MainApplication.worker.execute(asyncUploadTask.get()); - } + asyncUploadTask.ifPresent(MainApplication.worker::execute); } else { MainApplication.worker.execute(new UploadPrimitivesTask(uploadStrategySpecification, layer, apiData, cs)); Index: trunk/src/org/openstreetmap/josm/actions/upload/ValidateUploadHook.java =================================================================== --- trunk/src/org/openstreetmap/josm/actions/upload/ValidateUploadHook.java (revision 18751) +++ trunk/src/org/openstreetmap/josm/actions/upload/ValidateUploadHook.java (revision 18752) @@ -6,7 +6,7 @@ import java.awt.Dimension; import java.awt.GridBagLayout; -import java.util.ArrayList; import java.util.Collection; import java.util.List; +import java.util.concurrent.atomic.AtomicBoolean; import javax.swing.JPanel; @@ -15,15 +15,11 @@ import org.openstreetmap.josm.data.APIDataSet; import org.openstreetmap.josm.data.osm.OsmPrimitive; -import org.openstreetmap.josm.data.preferences.sources.ValidatorPrefHelper; import org.openstreetmap.josm.data.validation.OsmValidator; -import org.openstreetmap.josm.data.validation.Severity; -import org.openstreetmap.josm.data.validation.Test; import org.openstreetmap.josm.data.validation.TestError; +import org.openstreetmap.josm.data.validation.ValidationTask; import org.openstreetmap.josm.data.validation.util.AggregatePrimitivesVisitor; import org.openstreetmap.josm.gui.ExtendedDialog; import org.openstreetmap.josm.gui.MainApplication; -import org.openstreetmap.josm.gui.MapFrame; import org.openstreetmap.josm.gui.dialogs.validator.ValidatorTreePanel; -import org.openstreetmap.josm.gui.layer.OsmDataLayer; import org.openstreetmap.josm.gui.layer.ValidatorLayer; import org.openstreetmap.josm.gui.util.GuiHelper; @@ -34,5 +30,5 @@ * The action that does the validate thing. *

- * This action iterates through all active tests and give them the data, so that + * This action iterates through all active tests and gives them the data, so that * each one can test it. * @@ -45,55 +41,25 @@ * Validate the modified data before uploading * @param apiDataSet contains primitives to be uploaded - * @return true if upload should continue, else false + * @return {@code true} if upload should continue, else false */ @Override public boolean checkUpload(APIDataSet apiDataSet) { - - OsmValidator.initializeTests(); - Collection tests = OsmValidator.getEnabledTests(true); - if (tests.isEmpty()) - return true; - + AtomicBoolean returnCode = new AtomicBoolean(); AggregatePrimitivesVisitor v = new AggregatePrimitivesVisitor(); v.visit(apiDataSet.getPrimitivesToAdd()); - Collection selection = v.visit(apiDataSet.getPrimitivesToUpdate()); + Collection visited = v.visit(apiDataSet.getPrimitivesToUpdate()); + OsmValidator.initializeTests(); + new ValidationTask(errors -> { + if (errors.stream().allMatch(TestError::isIgnored)) { + returnCode.set(true); + } else { + // Unfortunately, the progress monitor is not "finished" until after `finish` is called, so we will + // have a ProgressMonitor open behind the error screen. Fortunately, the error screen appears in front + // of the progress monitor. + GuiHelper.runInEDTAndWait(() -> returnCode.set(displayErrorScreen(errors))); + } + }, null, OsmValidator.getEnabledTests(true), visited, null, true).run(); - List errors = new ArrayList<>(30); - for (Test test : tests) { - test.setBeforeUpload(true); - test.setPartialSelection(true); - test.startTest(null); - test.visit(selection); - test.endTest(); - if (ValidatorPrefHelper.PREF_OTHER.get() && ValidatorPrefHelper.PREF_OTHER_UPLOAD.get()) { - errors.addAll(test.getErrors()); - } else { - for (TestError e : test.getErrors()) { - if (e.getSeverity() != Severity.OTHER) { - errors.add(e); - } - } - } - test.clear(); - test.setBeforeUpload(false); - } - - if (Boolean.TRUE.equals(ValidatorPrefHelper.PREF_USE_IGNORE.get())) { - errors.forEach(TestError::updateIgnored); - } - - OsmDataLayer editLayer = MainApplication.getLayerManager().getEditLayer(); - if (editLayer != null) { - editLayer.validationErrors.clear(); - editLayer.validationErrors.addAll(errors); - } - MapFrame map = MainApplication.getMap(); - if (map != null) { - map.validatorDialog.tree.setErrors(errors); - } - if (errors.stream().allMatch(TestError::isIgnored)) - return true; - - return displayErrorScreen(errors); + return returnCode.get(); } @@ -102,6 +68,6 @@ * give the user the possibility to cancel the upload. * @param errors The errors displayed in the screen - * @return true, if the upload should continue. false - * if the user requested cancel. + * @return {@code true}, if the upload should continue.
+ * {@code false}, if the user requested cancel. */ private static boolean displayErrorScreen(List errors) { Index: trunk/src/org/openstreetmap/josm/data/validation/ValidationTask.java =================================================================== --- trunk/src/org/openstreetmap/josm/data/validation/ValidationTask.java (revision 18751) +++ trunk/src/org/openstreetmap/josm/data/validation/ValidationTask.java (revision 18752) @@ -4,7 +4,10 @@ import static org.openstreetmap.josm.tools.I18n.tr; +import java.awt.GraphicsEnvironment; import java.util.ArrayList; import java.util.Collection; import java.util.List; +import java.util.function.BiConsumer; +import java.util.function.Consumer; import javax.swing.JOptionPane; @@ -26,9 +29,12 @@ */ public class ValidationTask extends PleaseWaitRunnable { + private final Consumer> onFinish; private Collection tests; private final Collection validatedPrimitives; private final Collection formerValidatedPrimitives; + private final boolean beforeUpload; private boolean canceled; private List errors; + private BiConsumer testConsumer; /** @@ -45,12 +51,36 @@ } - protected ValidationTask(ProgressMonitor progressMonitor, - Collection tests, - Collection validatedPrimitives, - Collection formerValidatedPrimitives) { - super(tr("Validating"), progressMonitor, false /*don't ignore exceptions */); + /** + * Constructs a new {@code ValidationTask} + * + * @param onFinish called when the tests are finished + * @param progressMonitor the progress monitor to update with test progress + * @param tests the tests to run + * @param validatedPrimitives the collection of primitives to validate. + * @param formerValidatedPrimitives the last collection of primitives being validates. May be null. + * @param beforeUpload {@code true} if this is being run prior to upload + * @since 18752 + */ + public ValidationTask(Consumer> onFinish, + ProgressMonitor progressMonitor, + Collection tests, + Collection validatedPrimitives, + Collection formerValidatedPrimitives, + boolean beforeUpload) { + super(tr("Validating"), + progressMonitor != null ? progressMonitor : new PleaseWaitProgressMonitor(tr("Validating")), + false /*don't ignore exceptions */); + this.onFinish = onFinish; this.validatedPrimitives = validatedPrimitives; this.formerValidatedPrimitives = formerValidatedPrimitives; this.tests = tests; + this.beforeUpload = beforeUpload; + } + + protected ValidationTask(ProgressMonitor progressMonitor, + Collection tests, + Collection validatedPrimitives, + Collection formerValidatedPrimitives) { + this(null, progressMonitor, tests, validatedPrimitives, formerValidatedPrimitives, false); } @@ -64,15 +94,20 @@ if (canceled) return; - // update GUI on Swing EDT - GuiHelper.runInEDT(() -> { - MapFrame map = MainApplication.getMap(); - map.validatorDialog.unfurlDialog(); - map.validatorDialog.tree.setErrors(errors); - //FIXME: nicer way to find / invalidate the corresponding error layer - MainApplication.getLayerManager().getLayersOfType(ValidatorLayer.class).forEach(ValidatorLayer::invalidate); - if (!errors.isEmpty()) { - OsmValidator.initializeErrorLayer(); - } - }); + if (!GraphicsEnvironment.isHeadless() && MainApplication.getMap() != null) { + // update GUI on Swing EDT + GuiHelper.runInEDT(() -> { + MapFrame map = MainApplication.getMap(); + map.validatorDialog.unfurlDialog(); + map.validatorDialog.tree.setErrors(errors); + //FIXME: nicer way to find / invalidate the corresponding error layer + MainApplication.getLayerManager().getLayersOfType(ValidatorLayer.class).forEach(ValidatorLayer::invalidate); + if (!errors.isEmpty()) { + OsmValidator.initializeErrorLayer(); + } + }); + } + if (this.onFinish != null) { + this.onFinish.accept(this.errors); + } } @@ -89,5 +124,5 @@ testCounter++; getProgressMonitor().setCustomText(tr("Test {0}/{1}: Starting {2}", testCounter, tests.size(), test.getName())); - test.setBeforeUpload(false); + test.setBeforeUpload(this.beforeUpload); test.setPartialSelection(formerValidatedPrimitives != null); test.startTest(getProgressMonitor().createSubTaskMonitor(validatedPrimitives.size(), false)); @@ -95,5 +130,9 @@ test.endTest(); errors.addAll(test.getErrors()); + if (this.testConsumer != null) { + this.testConsumer.accept(this, test); + } test.clear(); + test.setBeforeUpload(false); } tests = null; @@ -125,3 +164,12 @@ return errors; } + + /** + * A test consumer to avoid filling up memory. A test consumer may remove tests it has consumed. + * @param testConsumer The consumer which takes a {@link ValidationTask} ({@code this}) and the test that finished. + * @since 18752 + */ + public void setTestConsumer(BiConsumer testConsumer) { + this.testConsumer = testConsumer; + } } Index: trunk/src/org/openstreetmap/josm/data/validation/ValidatorCLI.java =================================================================== --- trunk/src/org/openstreetmap/josm/data/validation/ValidatorCLI.java (revision 18751) +++ trunk/src/org/openstreetmap/josm/data/validation/ValidatorCLI.java (revision 18752) @@ -11,4 +11,5 @@ import java.nio.charset.StandardCharsets; import java.nio.file.Files; +import java.nio.file.Path; import java.nio.file.Paths; import java.util.ArrayList; @@ -26,4 +27,5 @@ import java.util.stream.Collectors; +import jakarta.json.JsonObject; import org.apache.commons.compress.utils.FileNameUtils; import org.openstreetmap.josm.actions.ExtensionFileFilter; @@ -177,7 +179,7 @@ for (String inputFile : this.input) { if (inputFile.endsWith(".validator.mapcss")) { - this.processValidatorFile(inputFile); + processValidatorFile(inputFile); } else if (inputFile.endsWith(".mapcss")) { - this.processMapcssFile(inputFile); + processMapcssFile(inputFile); } else { this.processFile(inputFile); @@ -198,5 +200,5 @@ * @throws ParseException if the file does not match the mapcss syntax */ - private void processMapcssFile(final String inputFile) throws ParseException { + private static void processMapcssFile(final String inputFile) throws ParseException { final MapCSSStyleSource styleSource = new MapCSSStyleSource(new File(inputFile).toURI().getPath(), inputFile, inputFile); styleSource.loadStyleSource(); @@ -215,5 +217,5 @@ * @throws ParseException if the file does not match the validator mapcss syntax */ - private void processValidatorFile(final String inputFile) throws ParseException, IOException { + private static void processValidatorFile(final String inputFile) throws ParseException, IOException { // Check asserts Config.getPref().putBoolean("validator.check_assert_local_rules", true); @@ -257,5 +259,4 @@ try { Logging.info(task); - OsmValidator.initializeTests(); dataLayer = MainApplication.getLayerManager().getLayersOfType(OsmDataLayer.class) .stream().filter(layer -> inputFileFile.equals(layer.getAssociatedFile())) @@ -270,11 +271,23 @@ } } - Collection tests = OsmValidator.getEnabledTests(false); - if (Files.isRegularFile(Paths.get(outputFile)) && !Files.deleteIfExists(Paths.get(outputFile))) { + Path path = Paths.get(outputFile); + if (path.toFile().isFile() && !Files.deleteIfExists(path)) { Logging.error("Could not delete {0}, attempting to append", outputFile); } GeoJSONMapRouletteWriter geoJSONMapRouletteWriter = new GeoJSONMapRouletteWriter(dataSet); - try (OutputStream fileOutputStream = Files.newOutputStream(Paths.get(outputFile))) { - tests.parallelStream().forEach(test -> runTest(test, geoJSONMapRouletteWriter, fileOutputStream, dataSet)); + OsmValidator.initializeTests(); + + try (OutputStream fileOutputStream = Files.newOutputStream(path)) { + // The first writeErrors catches anything that was written, for whatever reason. This is probably never + // going to be called. + ValidationTask validationTask = new ValidationTask(errors -> writeErrors(geoJSONMapRouletteWriter, fileOutputStream, errors), + progressMonitorFactory.get(), OsmValidator.getEnabledTests(false), + dataSet.allPrimitives(), Collections.emptyList(), false); + // This avoids keeping errors in memory + validationTask.setTestConsumer((t, test) -> { + writeErrors(geoJSONMapRouletteWriter, fileOutputStream, test.getErrors()); + t.getErrors().removeIf(test.getErrors()::contains); + }); + validationTask.run(); } } finally { @@ -283,4 +296,18 @@ } Logging.info(stopwatch.toString(task)); + } + } + + private void writeErrors(GeoJSONMapRouletteWriter geoJSONMapRouletteWriter, OutputStream fileOutputStream, + Collection errors) { + for (TestError error : errors) { + Optional object = geoJSONMapRouletteWriter.write(error); + if (object.isPresent()) { + try { + writeToFile(fileOutputStream, object.get().toString().getBytes(StandardCharsets.UTF_8)); + } catch (IOException e) { + throw new JosmRuntimeException(e); + } + } } } @@ -300,28 +327,4 @@ } return FileNameUtils.getBaseName(FileNameUtils.getBaseName(inputString)) + ".geojson"; - } - - /** - * Run a test - * @param test The test to run - * @param geoJSONMapRouletteWriter The object to use to create challenges - * @param fileOutputStream The location to write data to - * @param dataSet The dataset to check - */ - private void runTest(final Test test, final GeoJSONMapRouletteWriter geoJSONMapRouletteWriter, - final OutputStream fileOutputStream, DataSet dataSet) { - test.startTest(progressMonitorFactory.get()); - test.visit(dataSet.allPrimitives()); - test.endTest(); - test.getErrors().stream().map(geoJSONMapRouletteWriter::write) - .filter(Optional::isPresent).map(Optional::get) - .map(jsonObject -> jsonObject.toString().getBytes(StandardCharsets.UTF_8)).forEach(bytes -> { - try { - writeToFile(fileOutputStream, bytes); - } catch (IOException e) { - throw new JosmRuntimeException(e); - } - }); - test.clear(); } Index: trunk/src/org/openstreetmap/josm/gui/io/SaveLayersDialog.java =================================================================== --- trunk/src/org/openstreetmap/josm/gui/io/SaveLayersDialog.java (revision 18751) +++ trunk/src/org/openstreetmap/josm/gui/io/SaveLayersDialog.java (revision 18752) @@ -64,6 +64,6 @@ /** * Dialog that pops up when the user closes a layer with modified data. - * - * It asks for confirmation that all modification should be discarded and offers + *

+ * It asks for confirmation that all modifications should be discarded and offer * to save the layers to file or upload to server, depending on the type of layer. */ @@ -450,6 +450,6 @@ closeDialog(); } - } catch (UserCancelException ignore) { - Logging.trace(ignore); + } catch (UserCancelException userCancelException) { + Logging.trace(userCancelException); } } @@ -557,51 +557,65 @@ AbstractModifiableLayer layer = layerInfo.getLayer(); if (canceled) { + GuiHelper.runInEDTAndWait(() -> model.setUploadState(layer, UploadOrSaveState.CANCELED)); + continue; + } + GuiHelper.runInEDTAndWait(() -> monitor.subTask(tr("Preparing layer ''{0}'' for upload ...", layerInfo.getName()))); + + // checkPreUploadConditions must not be run in the EDT to avoid deadlocks + if (!UploadAction.checkPreUploadConditions(layer)) { + GuiHelper.runInEDTAndWait(() -> model.setUploadState(layer, UploadOrSaveState.FAILED)); + continue; + } + + GuiHelper.runInEDTAndWait(() -> uploadLayersUploadModelStateOnFinish(layer)); + currentTask = null; + } + } + + /** + * Update the {@link #model} state on upload finish + * @param layer The layer that has been saved + */ + private void uploadLayersUploadModelStateOnFinish(AbstractModifiableLayer layer) { + AbstractUploadDialog dialog = layer.getUploadDialog(); + if (dialog != null) { + dialog.setVisible(true); + if (dialog.isCanceled()) { model.setUploadState(layer, UploadOrSaveState.CANCELED); - continue; - } - monitor.subTask(tr("Preparing layer ''{0}'' for upload ...", layerInfo.getName())); - - if (!UploadAction.checkPreUploadConditions(layer)) { - model.setUploadState(layer, UploadOrSaveState.FAILED); - continue; - } - - AbstractUploadDialog dialog = layer.getUploadDialog(); - if (dialog != null) { - dialog.setVisible(true); - if (dialog.isCanceled()) { - model.setUploadState(layer, UploadOrSaveState.CANCELED); - continue; - } - dialog.rememberUserInput(); - } - - currentTask = layer.createUploadTask(monitor); - if (currentTask == null) { - model.setUploadState(layer, UploadOrSaveState.FAILED); - continue; - } - Future currentFuture = worker.submit(currentTask); - try { - // wait for the asynchronous task to complete - currentFuture.get(); - } catch (CancellationException e) { - Logging.trace(e); - model.setUploadState(layer, UploadOrSaveState.CANCELED); - } catch (InterruptedException | ExecutionException e) { - Logging.error(e); - model.setUploadState(layer, UploadOrSaveState.FAILED); - ExceptionDialogUtil.explainException(e); - } - if (currentTask.isCanceled()) { - model.setUploadState(layer, UploadOrSaveState.CANCELED); - } else if (currentTask.isFailed()) { - Logging.error(currentTask.getLastException()); - ExceptionDialogUtil.explainException(currentTask.getLastException()); - model.setUploadState(layer, UploadOrSaveState.FAILED); - } else { - model.setUploadState(layer, UploadOrSaveState.OK); - } - currentTask = null; + return; + } + dialog.rememberUserInput(); + } + + currentTask = layer.createUploadTask(monitor); + if (currentTask == null) { + model.setUploadState(layer, UploadOrSaveState.FAILED); + return; + } + Future currentFuture = worker.submit(currentTask); + try { + // wait for the asynchronous task to complete + currentFuture.get(); + } catch (CancellationException e) { + Logging.trace(e); + model.setUploadState(layer, UploadOrSaveState.CANCELED); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + Logging.error(e); + model.setUploadState(layer, UploadOrSaveState.FAILED); + ExceptionDialogUtil.explainException(e); + } catch (ExecutionException e) { + Logging.error(e); + model.setUploadState(layer, UploadOrSaveState.FAILED); + ExceptionDialogUtil.explainException(e); + } + if (currentTask.isCanceled()) { + model.setUploadState(layer, UploadOrSaveState.CANCELED); + } else if (currentTask.isFailed()) { + Logging.error(currentTask.getLastException()); + ExceptionDialogUtil.explainException(currentTask.getLastException()); + model.setUploadState(layer, UploadOrSaveState.FAILED); + } else { + model.setUploadState(layer, UploadOrSaveState.OK); } } @@ -673,10 +687,11 @@ @Override public void run() { + GuiHelper.runInEDTAndWait(() -> model.setMode(SaveLayersModel.Mode.UPLOADING_AND_SAVING)); + // We very specifically do not want to block the EDT or the worker thread when validating + List toUpload = model.getLayersToUpload(); + if (!toUpload.isEmpty()) { + uploadLayers(toUpload); + } GuiHelper.runInEDTAndWait(() -> { - model.setMode(SaveLayersModel.Mode.UPLOADING_AND_SAVING); - List toUpload = model.getLayersToUpload(); - if (!toUpload.isEmpty()) { - uploadLayers(toUpload); - } List toSave = model.getLayersToSave(); if (!toSave.isEmpty()) {