[patch] No warning deleting a data layer with changes and discouraged upload set

[skyper]

What steps will reproduce the problem?

1. Download new data
2. Make some changes
3. Set "discourage upload" on the layer
4. Delete layer

What is the expected result?

Upload/Save dialog with upload disabled

What happens instead?

Layer is deleted without any warning

Please provide any additional information below. Attach a screenshot if possible.

Did not try upload=never but the dialog should still be displayed.

Relative:URL: ^/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2023-03-13 21:59:27 +0100 (Mon, 13 Mar 2023)
Revision:18690
Build-Date:2023-03-14 02:30:58
URL:https://josm.openstreetmap.de/svn/trunk

[skyper]

With current behavior, you can loose all you work, e.g. a private file of all you mushroom spots.

[gaben]

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

@@ -112 +112 @@
     public static boolean saveUnsavedModifications(Iterable<? extends Layer> selectedLayers, Reason reason) {
         if (!GraphicsEnvironment.isHeadless()) {
             SaveLayersDialog dialog = new SaveLayersDialog(MainApplication.getMainFrame());
-            List<AbstractModifiableLayer> layersWithUnmodifiedChanges = new ArrayList<>();
+            List<AbstractModifiableLayer> layersWithUnsavedChanges = new ArrayList<>();
             for (Layer l: selectedLayers) {
                 if (!(l instanceof AbstractModifiableLayer)) {
                     continue;
@@ -121 +121 @@
                 if (odl.isModified() &&
                         ((!odl.isSavable() && !odl.isUploadable()) ||
                                 odl.requiresSaveToFile() ||
-                                (odl.requiresUploadToServer() && !odl.isUploadDiscouraged()))) {
-                    layersWithUnmodifiedChanges.add(odl);
+                                odl.requiresUploadToServer())) {
+                    layersWithUnsavedChanges.add(odl);
                 }
             }
             dialog.prepareForSavingAndUpdatingLayers(reason);
-            if (!layersWithUnmodifiedChanges.isEmpty()) {
-                dialog.getModel().populate(layersWithUnmodifiedChanges);
+            if (!layersWithUnsavedChanges.isEmpty()) {
+                dialog.getModel().populate(layersWithUnsavedChanges);
                 dialog.setVisible(true);
                 switch(dialog.getUserAction()) {
                     case PROCEED: return true;

The upload=never option is not affected as in that case the isUploadable() returns false anyway.

I renamed the layersWithUnmodifiedChanges local variable to something more meaningful because the current naming contradicts.

Also whoever commits, the !odl.isSavable() && !odl.isUploadable() seems suspicious. When does it happen?

[taylor.smock]

Ticket retargeted after milestone closed

[taylor.smock]

Replying to gaben:

The upload=never option is not affected as in that case the isUploadable() returns false anyway.

I renamed the layersWithUnmodifiedChanges local variable to something more meaningful because the current naming contradicts.

Also whoever commits, the !odl.isSavable() && !odl.isUploadable() seems suspicious. When does it happen?

isSavable() is used to check whether or not the layer can be saved. This will return true in most cases, while isUploadable() checks to see if the data can be uploaded (false for BLOCKED, other data sources can modify this while still being savable, or not be savable while being uploadable).

upload=never is affected since requiresUploadToServer checks to see if upload is blocked.

Anyway, I've got a test case for all three upload policy states, so we can be relatively certain that the bug in this section of code is "fixed".

[taylor.smock]

In 18706/josm:

Fix #22817: Don't delete layer with changes and a non-normal upload policy (patch by gaben, modified)

Patch modifications are as follows:

• A non-regression test
• Account for UploadPolicy.BLOCKED causing requiresUploadToServer to return false.