[WIP Patch] Josm remove geoimages from the layer even if this image is unsuccessfully deleted

[StephaneP]

What steps will reproduce the problem?

1. Load some not too small images
2. Try do delete these images from disk with Josm (Ctrl+Shift+Del), as fast as possible.

What is the expected result?

Josm should delete these images from the disk and from the image layer.

What happens instead?

If you delete the images too fast, Josm will show you an error Image file could not be deleted. The file isn't deleted (Why?) and the image is removed from the image layer

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

I can see the error in the logs : - 00740.978 W: Unable to delete file H:\bug_gpx_josm_1\2021-04-15_11H59mn42s-Cam_avant-G0052614.JPG

URL:https://josm.openstreetmap.de/svn/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2021-04-23 21:32:22 +0200 (Fri, 23 Apr 2021)
Build-Date:2021-04-24 01:30:58
Revision:17814
Relative:URL: ^/trunk

Identification: JOSM/1.5 (17814 en) Windows 10 64-Bit
OS Build number: Windows 10 Pro 2009 (19042)
Memory Usage: 3290 MB / 7218 MB (2643 MB allocated, but free)
Java version: 1.8.0_191-b12, Oracle Corporation, Java HotSpot(TM) 64-Bit Server VM
Look and Feel: com.sun.java.swing.plaf.windows.WindowsLookAndFeel
Screen: \Display0 1920×1080 (scaling 1.00×1.00)
Maximum Screen Size: 1920×1080
Best cursor sizes: 16×16→32×32, 32×32→32×32
System property file.encoding: Cp1252
System property sun.jnu.encoding: Cp1252

Plugins:
+ Mapillary (1.5.37.6)
+ OpeningHoursEditor (35640)
+ PicLayer (2a9aa7a)
+ apache-commons (35524)
+ apache-http (35589)
+ areaselector (368)
+ austriaaddresshelper (1597341117)
+ cadastre-fr (35727)
+ continuosDownload (91)
+ contourmerge (v0.1.6)
+ ejml (35458)
+ geotools (35458)
+ javafx-windows (35655)
+ jaxb (35543)
+ jna (35662)
+ jts (35458)
+ log4j (35458)
+ opendata (35640)
+ photo_geotagging (35715)
+ photoadjust (35640)
+ reverter (35732)
+ tageditor (35640)
+ todo (30306)
+ turnlanes-tagging (288)
+ turnrestrictions (35640)
+ undelete (35640)
+ utilsplugin2 (35691)

Tagging presets:
+ %UserProfile%\Documents\Gitlab\preset-josm-collectivites\Presets-collectivités.xml

Map paint styles:
- https://josm.openstreetmap.de/josmfile?page=Styles/Enhanced_Lane_and_Road_Attributes&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/Lane_and_Road_Attributes&zip=1
- https://raw.githubusercontent.com/species/josm-preset-traffic_sign_direction/master/direction.mapcss
- %UserProfile%\Documents\GitHub\MapCSS-JOSM-Bicycle\cycleway.mapcss

Last errors/warnings:
- 00041.368 E: Failed to locate image 'regulatory--dual-lanes-cyclists-and-pedestrians--g1'
- 00041.852 E: Failed to locate image 'regulatory--texts--g1'
- 00041.853 E: Failed to locate image 'regulatory--texts--g2'
- 00041.942 E: Failed to locate image 'void--car-mount'
- 00041.943 E: Failed to locate image 'void--dynamic'
- 00041.944 E: Failed to locate image 'void--ego-vehicle'
- 00041.944 E: Failed to locate image 'void--ground'
- 00041.945 E: Failed to locate image 'void--static'
- 00042.066 E: Failed to locate image 'warning--kangaroo-crossing--g1'
- 00740.978 W: Unable to delete file H:\bug_gpx_josm_1\2021-04-15_11H59mn42s-Cam_avant-G0052614.JPG

[GerdP]

Probably caused by Windows. Sometimes it doesn't allow to delete a file which was just closed. Trying again a bit later works fine. Unix based systems don't show this problem.

[StephaneP]

Replying to GerdP:

Probably caused by Windows. Sometimes it doesn't allow to delete a file which was just closed. Trying again a bit later works fine. Unix based systems don't show this problem.

If you're right, and if there is no other way to manage this problem, I think Josm should not remove the image from the image layer if it failed to delete it.

[StephaneP]

I'm not a Java developper, but if I understand the code correctly, doesn't this ​line currentData.removeSelectedImages();should be inside the
if (Utils.deleteFile(delete.getFile()) section?

[StephaneP]

Perhaps it could help:
​https://stackoverflow.com/questions/991489/file-delete-returns-false-even-though-file-exists-file-canread-file-canw

[StephaneP]

Remove geoimage only after a successful delete

[StephaneP]

Here is a small patch to remove the image from the layer only if it is successfully deleted.

[GerdP]

I think you want to remove only the image that was successfully deleted? Your patch will not help when the 2nd image file was not deleted.

[StephaneP]

Why it will not help for the 2nd image file?
I did test this patch on my computer, trying to delete image file as fast as possible, and it was ok: I had a lot of "Could not be deleted", but at least the not deleted images were not removed from the layer.

[GerdP]

I did not try it yet. What happens if you select 2 or more two files and delete them and only the first deleteFile() is successfull? My understanding of the patched code is that you remove all images from the layer if the first deleteFile() is successfull.

[StephaneP]

Ok, I didn't think of the multiple images selection.

As selecting multiple images is a slow process (no selection tool available) I can't reproduce the "could not be deleted" warning.
But there is another problem: If I select->"Delete from disk" 4 images, it deletes 4 images files and remove 7 images from the layer (2x - 1).
I'm afraid I don't have the skill to fix that.

[StephaneP]

It works better if I call currentData.removeImage(delete) but when I delete 5 images files, then the next 5 images are selected. It could be useful, but it's not intended to behave like this. It seems there is something to do with setSelectedImageIndex in ImageData.java, but I don't really understand. It's hard to dig in an unknown software with and unknown language.

[GerdP]

Hmm, this function is really dangerous. r17833 deletes files which have the "read only" attribute. I think should not do that.
How do you see that the next 5 images are selected?

[GerdP]

Ah, ok, when I apply the patch with currentData.removeImage(delete) I see that multiple images are selected.
Maybe use currentData.selectFirstImage(); like this?

src/org/openstreetmap/josm/gui/layer/geoimage/ImageViewerDialog.java

@@ -343 +343 @@
                         .getValue();
 
                 if (result == 2) {
-                    currentData.removeSelectedImages();
                     for (ImageEntry delete : toDelete) {
                         if (Utils.deleteFile(delete.getFile())) {
+                            currentData.removeImage(delete);
                             Logging.info("File " + delete.getFile() + " deleted.");
                         } else {
                             JOptionPane.showMessageDialog(
@@ -356 +356 @@
                                     );
                         }
                     }
+                    currentData.selectFirstImage();
                 }
             }
         }

[StephaneP]

Hi!
I have something which works. I think the new code doesn't integrate very well with the existing one. Please give me your opinion:

src/org/openstreetmap/josm/data/ImageData.java

@@ -275 +275 @@
         removeImages(selectedImages);
     }
 
+    public void removeImageInSelectedImages(ImageEntry image) {
+        int imgIndex = data.indexOf(image);
+        int imgIndexSelected = selectedImagesIndex.indexOf(imgIndex);
+        // remove deleted image from selectedImagesIndex
+        selectedImagesIndex.remove(imgIndexSelected);
+        // updating remaining images index
+        for (int i = 0; i < selectedImagesIndex.size(); i++) {
+            if (selectedImagesIndex.get(i) > imgIndex) {
+                selectedImagesIndex.set(i, selectedImagesIndex.get(i) - 1);
+            }
+        }
+        data.remove(image);
+        this.geoImages.remove(image);
+    }
+
+    public void SetSelectedImagesAfterDelete(int index) {
+        // update selected image only if there is no selected images anymore
+        if (selectedImagesIndex.size() == 0) {
+            selectedImagesIndex.add(-1);
+        if (index > data.size() -1) {
+                index--;
+            }
+            setSelectedImageIndex(index, true);
+        } else {
+            listeners.fireEvent(l -> l.selectedImageChanged(this));
+        }
+    }
+
+    public int getMaxSelectedImagesIndex() {
+        int maxIndex = Collections.max(selectedImagesIndex);
+        return maxIndex;
+    }
+    
     /**
      * Remove the current selected image from the list
      * @since 15348

src/org/openstreetmap/josm/gui/layer/geoimage/ImageViewerDialog.java

@@ -343 +343 @@
                         .getValue();
 
                 if (result == 2) {
-                    currentData.removeSelectedImages();
+                    int nextImage = currentData.getMaxSelectedImagesIndex() - toDelete.size() + 1;
                     for (ImageEntry delete : toDelete) {
                         if (Utils.deleteFile(delete.getFile())) {
                             Logging.info("File " + delete.getFile() + " deleted.");
+                            currentData.removeImageInSelectedImages(delete);
                         } else {
                             JOptionPane.showMessageDialog(
                                     MainApplication.getMainFrame(),
@@ -356 +357 @@
                                     );
                         }
                     }
+                    currentData.SetSelectedImagesAfterDelete(nextImage);
                 }
             }
         }

[GerdP]

@Simon: What do you think?

[Don-vip]

In 18049/josm:

fix #20796 - Delete image file from disk : only remove images from geoimage layer after successful deletion