Marker Bugfixes: markers deletion didn't work and track import didn't include marker's photos. Fixes #514.

Marker edition bugfixed: orphan photos are deleted.

src/main/java/de/dennisguse/opentracks/MarkerEditActivity.java

@@ -37,6 +37,8 @@ import androidx.annotation.NonNull;
 import java.io.File;
 import java.io.FileDescriptor;
 import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
 
 import de.dennisguse.opentracks.content.data.Marker;
 import de.dennisguse.opentracks.content.data.Track;
@@ -70,8 +72,12 @@ public class MarkerEditActivity extends AbstractActivity {
     private MenuItem insertGalleryImgMenuItem;
 
     private Uri photoUri;
+    private Uri photoUriOriginal;
+    private List<Uri> photoUriDeleteList = new ArrayList<>();
     private boolean hasCamera;
 
+    private boolean isNewMarker;
+
     // UI elements
     private MarkerEditBinding viewBinding;
 
@@ -92,13 +98,41 @@ public class MarkerEditActivity extends AbstractActivity {
                 marker.setPhotoUrl(null);
             }
             viewBinding.markerEditPhoto.setImageBitmap(null);
+            photoUriDeleteList.add(photoUri);
+            photoUriOriginal = photoUriOriginal == null ? photoUri : photoUriOriginal;
             photoUri = null;
             hideAndShowOptions();
         });
 
-        viewBinding.markerEditCancel.setOnClickListener(v -> finish());
+        viewBinding.markerEditCancel.setOnClickListener(v -> {
+            Track.Id trackId = getTrackId();
+            if (isNewMarker && trackId != null) {
+                // new marker and user cancel -> delete all photos from track directory
+                for (Uri photoUriDelete : photoUriDeleteList) {
+                    File photoFile = FileUtils.getPhotoFileIfExists(this, trackId, photoUriDelete);
+                    FileUtils.deleteDirectoryRecurse(photoFile);
+                }
+                if (photoUri != null) {
+                    File photoFile = FileUtils.getPhotoFileIfExists(this, trackId, photoUri);
+                    FileUtils.deleteDirectoryRecurse(photoFile);
+                }
+            } else if (!isNewMarker && trackId != null) {
+                // no new marker, user cancel and photo was changed -> delete all photos but original one
+                for (Uri photoUri : photoUriDeleteList) {
+                    if (photoUriOriginal != photoUri) {
+                        File photoFile = FileUtils.getPhotoFileIfExists(this, trackId, photoUri);
+                        FileUtils.deleteDirectoryRecurse(photoFile);
+                    }
+                }
+                if (photoUri != null && photoUri != photoUriOriginal) {
+                    File photoFile = FileUtils.getPhotoFileIfExists(this, trackId, photoUri);
+                    FileUtils.deleteDirectoryRecurse(photoFile);
+                }
+            }
+            finish();
+        });
 
-        final boolean isNewMarker = markerId == null;
+        isNewMarker = markerId == null;
 
         setTitle(isNewMarker ? R.string.menu_insert_marker : R.string.menu_edit);
         viewBinding.markerEditDone.setText(isNewMarker ? R.string.generic_add : R.string.generic_save);
@@ -108,6 +142,13 @@ public class MarkerEditActivity extends AbstractActivity {
             } else {
                 saveMarker();
             }
+            Track.Id trackId = getTrackId();
+            if (trackId == null) {
+                for (Uri photoUri : photoUriDeleteList) {
+                    File photoFile = FileUtils.getPhotoFileIfExists(this, trackId, photoUri);
+                    FileUtils.deleteDirectoryRecurse(photoFile);
+                }
+            }
             finish();
         });
 

src/main/java/de/dennisguse/opentracks/MarkerListActivity.java

@@ -237,9 +237,6 @@ public class MarkerListActivity extends AbstractActivity implements DeleteMarker
                 }
                 return true;
             case R.id.list_context_menu_delete:
-                if (markerIds.length > 1 && markerIds.length == viewBinding.markerList.getCount()) {
-                    markerIds = null;
-                }
                 DeleteMarkerDialogFragment.showDialog(getSupportFragmentManager(), markerIds);
                 return true;
             case R.id.list_context_menu_select_all:

src/main/java/de/dennisguse/opentracks/fragments/DeleteMarkerDialogFragment.java

@@ -65,22 +65,16 @@ public class DeleteMarkerDialogFragment extends DialogFragment {
     @NonNull
     public Dialog onCreateDialog(Bundle savedInstanceState) {
         final Marker.Id[] markerIds = (Marker.Id[]) getArguments().getParcelableArray(KEY_MARKER_IDS);
+        final Context context = getContext();
 
         final FragmentActivity fragmentActivity = getActivity();
-        int titleId;
+        int titleId = markerIds.length > 1 ? R.string.generic_delete_selected_confirm_title : R.string.marker_delete_one_confirm_title;
-        int messageId;
+        int messageId = markerIds.length > 1 ? R.string.marker_delete_multiple_confirm_message : R.string.marker_delete_one_confirm_message;
-        if (markerIds == null) {
-            titleId = R.string.generic_delete_all_confirm_title;
-            messageId = R.string.marker_delete_all_confirm_message;
-        } else {
-            titleId = markerIds.length > 1 ? R.string.generic_delete_selected_confirm_title : R.string.marker_delete_one_confirm_title;
-            messageId = markerIds.length > 1 ? R.string.marker_delete_multiple_confirm_message : R.string.marker_delete_one_confirm_message;
-        }
         return DialogUtils.createConfirmationDialog(
                 fragmentActivity, titleId, getString(messageId), (dialog, which) -> new Thread(() -> {
                     ContentProviderUtils contentProviderUtils = new ContentProviderUtils(fragmentActivity);
                     for (Marker.Id markerId : markerIds) {
-                        contentProviderUtils.deleteMarker(getContext(), markerId);
+                        contentProviderUtils.deleteMarker(context, markerId);
                     }
                     caller.onMarkerDeleted();
                 }).start());

src/main/java/de/dennisguse/opentracks/io/file/importer/AbstractFileTrackImporter.java

@@ -68,7 +68,6 @@ abstract class AbstractFileTrackImporter extends DefaultHandler implements Track
     private final ContentProviderUtils contentProviderUtils;
     private final int recordingDistanceInterval;
 
-    private Track.Id importTrackId;
     private final List<Track.Id> trackIds = new ArrayList<>();
     private final List<Marker> markers = new ArrayList<>();
 
@@ -104,11 +103,6 @@ abstract class AbstractFileTrackImporter extends DefaultHandler implements Track
         this.recordingDistanceInterval = PreferencesUtils.getRecordingDistanceInterval(context);
     }
 
-    AbstractFileTrackImporter(Context context, ContentProviderUtils contentProviderUtils, Track.Id importTrackId) {
-        this(context, contentProviderUtils);
-        this.importTrackId = importTrackId;
-    }
-
     @Override
     public void setDocumentLocator(Locator locator) {
         this.locator = locator;
@@ -181,6 +175,10 @@ abstract class AbstractFileTrackImporter extends DefaultHandler implements Track
                         // No more markers
                         return;
                     }
+                    // If marker had photo it must be translated to internal photo url (depend on track id)
+                    if (marker.hasPhoto()) {
+                        marker.setPhotoUrl(getInternalPhotoUrl(marker.getPhotoUrl()));
+                    }
                 }
 
                 if (trackPoint == null) {
@@ -230,17 +228,8 @@ abstract class AbstractFileTrackImporter extends DefaultHandler implements Track
      */
     protected void onTrackStart() throws SAXException {
         trackData = new TrackData();
-        Track.Id trackId;
+        Uri uri = contentProviderUtils.insertTrack(trackData.track);
-        if (importTrackId == null) {
+        Track.Id trackId = new Track.Id(Long.parseLong(uri.getLastPathSegment()));
-            Uri uri = contentProviderUtils.insertTrack(trackData.track);
-            trackId = new Track.Id(Long.parseLong(uri.getLastPathSegment()));
-        } else {
-            if (trackIds.size() > 0) {
-                throw new SAXException(createErrorMessage("Cannot import more than one track to an existing track " + importTrackId.getId()));
-            }
-            trackId = importTrackId;
-            contentProviderUtils.clearTrack(trackId);
-        }
         trackIds.add(trackId);
         trackData.track.setId(trackId);
     }
@@ -401,7 +390,7 @@ abstract class AbstractFileTrackImporter extends DefaultHandler implements Track
      * @param externalPhotoUrl the file name
      */
     protected String getInternalPhotoUrl(String externalPhotoUrl) {
-        if (importTrackId == null) {
+        if (trackData.track.getId() == null) {
             Log.e(TAG, "Track id is invalid.");
             return null;
         }
@@ -412,7 +401,7 @@ abstract class AbstractFileTrackImporter extends DefaultHandler implements Track
         }
 
         String importFileName = KmzTrackImporter.importNameForFilename(externalPhotoUrl);
-        File file = FileUtils.getPhotoFileIfExists(context, importTrackId, Uri.parse(importFileName));
+        File file = FileUtils.buildInternalPhotoFile(context, trackData.track.getId(), Uri.parse(importFileName));
         if (file != null) {
             Uri photoUri = FileUtils.getUriForFile(context, file);
             return "" + photoUri;

src/main/java/de/dennisguse/opentracks/io/file/importer/KmlFileTrackImporter.java

@@ -193,10 +193,6 @@ public class KmlFileTrackImporter extends AbstractFileTrackImporter {
         if (!MARKER_STYLE.equals(markerType)) {
             return;
         }
-
-        // If there is photoUrl it has to be changed because that url in kml file is a relative path to the internal kmz file.
-        photoUrl = getInternalPhotoUrl(photoUrl);
-
         addMarker();
     }
 

src/main/java/de/dennisguse/opentracks/io/file/importer/KmzTrackImporter.java

@@ -32,6 +32,7 @@ import java.util.List;
 import java.util.zip.ZipEntry;
 import java.util.zip.ZipInputStream;
 
+import de.dennisguse.opentracks.R;
 import de.dennisguse.opentracks.content.data.Marker;
 import de.dennisguse.opentracks.content.data.Track;
 import de.dennisguse.opentracks.content.provider.ContentProviderUtils;
@@ -53,7 +54,6 @@ public class KmzTrackImporter implements TrackImporter {
     private static final int BUFFER_SIZE = 4096;
 
     private final Context context;
-    private Track.Id importTrackId; //TODO needed?
     private final Uri uriKmzFile;
 
     /**
@@ -67,22 +67,10 @@ public class KmzTrackImporter implements TrackImporter {
 
     @Override
     public Track.Id importFile(InputStream inputStream) {
-        Track.Id trackId;
+        Track.Id trackId = findAndParseKmlFile(inputStream);
 
-        if (!copyKmzImages()) {
+        if (!copyKmzImages(trackId)) {
-            cleanImport(context, importTrackId);
+            cleanImport(context, trackId);
-            return null;
-        }
-
-        try {
-            trackId = findAndParseKmlFile(inputStream);
-        } catch (Exception e) {
-            cleanImport(context, importTrackId);
-            throw e;
-        }
-
-        if (trackId == null) {
-            cleanImport(context, importTrackId);
             return null;
         }
 
@@ -96,7 +84,7 @@ public class KmzTrackImporter implements TrackImporter {
      *
      * @return false if there are errors or true otherwise.
      */
-    private boolean copyKmzImages() {
+    private boolean copyKmzImages(Track.Id trackId) {
         try (InputStream inputStream = context.getContentResolver().openInputStream(uriKmzFile);
              ZipInputStream zipInputStream = new ZipInputStream(inputStream)) {
             ZipEntry zipEntry;
@@ -109,7 +97,7 @@ public class KmzTrackImporter implements TrackImporter {
 
                 String fileName = zipEntry.getName();
                 if (hasImageExtension(fileName)) {
-                    readAndSaveImageFile(zipInputStream, importNameForFilename(fileName));
+                    readAndSaveImageFile(zipInputStream, trackId, importNameForFilename(fileName));
                 }
 
                 zipInputStream.closeEntry();
@@ -175,7 +163,7 @@ public class KmzTrackImporter implements TrackImporter {
             while ((zipEntry = zipInputStream.getNextEntry()) != null) {
                 if (Thread.interrupted()) {
                     Log.d(TAG, "Thread interrupted");
-                    return null;
+                    throw new RuntimeException(context.getString(R.string.import_thread_interrupted));
                 }
 
                 String fileName = zipEntry.getName();
@@ -183,7 +171,7 @@ public class KmzTrackImporter implements TrackImporter {
                     trackId = parseKml(zipInputStream);
                     if (trackId == null) {
                         Log.d(TAG, "Unable to parse kml in kmz");
-                        return null;
+                        throw new RuntimeException(context.getString(R.string.import_unable_to_import_file, fileName));
                     }
                 }
 
@@ -285,12 +273,12 @@ public class KmzTrackImporter implements TrackImporter {
      * @param zipInputStream the zip input stream
      * @param fileName       the file name
      */
-    private void readAndSaveImageFile(ZipInputStream zipInputStream, String fileName) throws IOException {
+    private void readAndSaveImageFile(ZipInputStream zipInputStream, Track.Id trackId, String fileName) throws IOException {
-        if (importTrackId == null || fileName.equals("")) {
+        if (trackId == null || fileName.equals("")) {
             return;
         }
 
-        File dir = FileUtils.getPhotoDir(context, importTrackId);
+        File dir = FileUtils.getPhotoDir(context, trackId);
         File file = new File(dir, fileName);
 
         try (FileOutputStream fileOutputStream = new FileOutputStream(file)) {

src/main/java/de/dennisguse/opentracks/util/FileUtils.java

@@ -20,6 +20,7 @@ import android.net.Uri;
 import android.os.Environment;
 import android.util.Log;
 
+import androidx.annotation.NonNull;
 import androidx.core.content.FileProvider;
 import androidx.documentfile.provider.DocumentFile;
 
@@ -289,6 +290,30 @@ public class FileUtils {
         return file;
     }
 
+    /**
+     * Builds interval photo file object for fileNameUri Uri.
+     *
+     * @param context     the Context.
+     * @param trackId     the id of the Track.
+     * @param fileNameUri the file name uri.
+     * @return            file object or null.
+     */
+    public static File buildInternalPhotoFile(Context context, Track.Id trackId, @NonNull Uri fileNameUri) {
+        if (fileNameUri == null) {
+            Log.w(TAG, "URI object is null.");
+            return null;
+        }
+
+        String filename = fileNameUri.getLastPathSegment();
+        if (filename == null) {
+            Log.w(TAG, "External photo contains no filename.");
+            return null;
+        }
+
+        File dir = FileUtils.getPhotoDir(context, trackId);
+        return new File(dir, filename);
+    }
+
     /**
      * Delete the directory recursively.
      *

src/main/res/values/strings.xml

@@ -285,6 +285,7 @@ limitations under the License.
     <string name="image_stop">Stop</string>
     <string name="image_track">Track</string>
     <!-- Import -->
+    <string name="import_thread_interrupted">Thread interrupted</string>
     <string name="import_unsupported_format">Unsupported file format</string>
     <string name="import_parser_error">Parser error: %1$s</string>
     <string name="import_unable_to_import_file">Unable to import file: %1$s</string>