fix #4591: refreshing without progress leads to ANR

6 files changed, 83 insertions, 94 deletions

diff --git a/main/src/cgeo/geocaching/DataStore.java b/main/src/cgeo/geocaching/DataStore.java
index 4e067ec..0f9f6bf 100644
--- a/main/src/cgeo/geocaching/DataStore.java
+++ b/main/src/cgeo/geocaching/DataStore.java
@@ -1226,15 +1226,13 @@ public class DataStore {
             return;
         }
         final SQLiteStatement statement = PreparedStatement.INSERT_ATTRIBUTE.getStatement();
-        synchronized (statement) {
-            final long timestamp = System.currentTimeMillis();
-            for (final String attribute : attributes) {
-                statement.bindString(1, geocode);
-                statement.bindLong(2, timestamp);
-                statement.bindString(3, attribute);
+        final long timestamp = System.currentTimeMillis();
+        for (final String attribute : attributes) {
+            statement.bindString(1, geocode);
+            statement.bindLong(2, timestamp);
+            statement.bindString(3, attribute);
 
-                statement.executeInsert();
-            }
+            statement.executeInsert();
         }
     }
 
@@ -1248,17 +1246,13 @@ public class DataStore {
         init();
 
         database.beginTransaction();
-
         try {
             final SQLiteStatement insertDestination = PreparedStatement.INSERT_SEARCH_DESTINATION.getStatement();
-            synchronized (insertDestination) {
-                insertDestination.bindLong(1, destination.getDate());
-                final Geopoint coords = destination.getCoords();
-                insertDestination.bindDouble(2, coords.getLatitude());
-                insertDestination.bindDouble(3, coords.getLongitude());
-
-                insertDestination.executeInsert();
-            }
+            insertDestination.bindLong(1, destination.getDate());
+            final Geopoint coords = destination.getCoords();
+            insertDestination.bindDouble(2, coords.getLatitude());
+            insertDestination.bindDouble(3, coords.getLongitude());
+            insertDestination.executeInsert();
             database.setTransactionSuccessful();
         } catch (final Exception e) {
             Log.e("Updating searchedDestinations db failed", e);
@@ -1270,7 +1264,6 @@ public class DataStore {
     public static boolean saveWaypoints(final Geocache cache) {
         init();
         database.beginTransaction();
-
         try {
             saveWaypointsWithoutTransaction(cache);
             database.setTransactionSuccessful();
@@ -1416,51 +1409,56 @@ public class DataStore {
         final List<Image> spoilers = cache.getSpoilers();
         if (CollectionUtils.isNotEmpty(spoilers)) {
             final SQLiteStatement insertSpoiler = PreparedStatement.INSERT_SPOILER.getStatement();
-            synchronized (insertSpoiler) {
-                final long timestamp = System.currentTimeMillis();
-                for (final Image spoiler : spoilers) {
-                    insertSpoiler.bindString(1, geocode);
-                    insertSpoiler.bindLong(2, timestamp);
-                    insertSpoiler.bindString(3, spoiler.getUrl());
-                    insertSpoiler.bindString(4, spoiler.getTitle());
-                    final String description = spoiler.getDescription();
-                    if (description != null) {
-                        insertSpoiler.bindString(5, description);
-                    } else {
-                        insertSpoiler.bindNull(5);
-                    }
-                    insertSpoiler.executeInsert();
+            final long timestamp = System.currentTimeMillis();
+            for (final Image spoiler : spoilers) {
+                insertSpoiler.bindString(1, geocode);
+                insertSpoiler.bindLong(2, timestamp);
+                insertSpoiler.bindString(3, spoiler.getUrl());
+                insertSpoiler.bindString(4, spoiler.getTitle());
+                final String description = spoiler.getDescription();
+                if (description != null) {
+                    insertSpoiler.bindString(5, description);
+                } else {
+                    insertSpoiler.bindNull(5);
                 }
+                insertSpoiler.executeInsert();
             }
         }
     }
 
-    public static void saveLogsWithoutTransaction(final String geocode, final Iterable<LogEntry> logs) {
+    public static void saveLogs(final String geocode, final Iterable<LogEntry> logs) {
+        database.beginTransaction();
+        try {
+            saveLogsWithoutTransaction(geocode, logs);
+            database.setTransactionSuccessful();
+        } finally {
+            database.endTransaction();
+        }
+    }
+
+    private static void saveLogsWithoutTransaction(final String geocode, final Iterable<LogEntry> logs) {
         // TODO delete logimages referring these logs
         database.delete(dbTableLogs, "geocode = ?", new String[]{geocode});
 
         final SQLiteStatement insertLog = PreparedStatement.INSERT_LOG.getStatement();
-        synchronized (insertLog) {
-            final long timestamp = System.currentTimeMillis();
-            for (final LogEntry log : logs) {
-                insertLog.bindString(1, geocode);
-                insertLog.bindLong(2, timestamp);
-                insertLog.bindLong(3, log.type.id);
-                insertLog.bindString(4, log.author);
-                insertLog.bindString(5, log.log);
-                insertLog.bindLong(6, log.date);
-                insertLog.bindLong(7, log.found);
-                insertLog.bindLong(8, log.friend ? 1 : 0);
-                final long logId = insertLog.executeInsert();
-                if (log.hasLogImages()) {
-                    // INSERT_LOG_IMAGE is used only here and is already protected by the synchronization on INSERT_LOG
-                    final SQLiteStatement insertImage = PreparedStatement.INSERT_LOG_IMAGE.getStatement();
-                    for (final Image img : log.getLogImages()) {
-                        insertImage.bindLong(1, logId);
-                        insertImage.bindString(2, img.getTitle());
-                        insertImage.bindString(3, img.getUrl());
-                        insertImage.executeInsert();
-                    }
+        final long timestamp = System.currentTimeMillis();
+        for (final LogEntry log : logs) {
+            insertLog.bindString(1, geocode);
+            insertLog.bindLong(2, timestamp);
+            insertLog.bindLong(3, log.type.id);
+            insertLog.bindString(4, log.author);
+            insertLog.bindString(5, log.log);
+            insertLog.bindLong(6, log.date);
+            insertLog.bindLong(7, log.found);
+            insertLog.bindLong(8, log.friend ? 1 : 0);
+            final long logId = insertLog.executeInsert();
+            if (log.hasLogImages()) {
+                final SQLiteStatement insertImage = PreparedStatement.INSERT_LOG_IMAGE.getStatement();
+                for (final Image img : log.getLogImages()) {
+                    insertImage.bindLong(1, logId);
+                    insertImage.bindString(2, img.getTitle());
+                    insertImage.bindString(3, img.getUrl());
+                    insertImage.executeInsert();
                 }
             }
         }
@@ -1474,16 +1472,14 @@ public class DataStore {
         if (MapUtils.isNotEmpty(logCounts)) {
             final Set<Entry<LogType, Integer>> logCountsItems = logCounts.entrySet();
             final SQLiteStatement insertLogCounts = PreparedStatement.INSERT_LOG_COUNTS.getStatement();
-            synchronized (insertLogCounts) {
-                final long timestamp = System.currentTimeMillis();
-                for (final Entry<LogType, Integer> pair : logCountsItems) {
-                    insertLogCounts.bindString(1, geocode);
-                    insertLogCounts.bindLong(2, timestamp);
-                    insertLogCounts.bindLong(3, pair.getKey().id);
-                    insertLogCounts.bindLong(4, pair.getValue());
-
-                    insertLogCounts.executeInsert();
-                }
+            final long timestamp = System.currentTimeMillis();
+            for (final Entry<LogType, Integer> pair : logCountsItems) {
+                insertLogCounts.bindString(1, geocode);
+                insertLogCounts.bindLong(2, timestamp);
+                insertLogCounts.bindLong(3, pair.getKey().id);
+                insertLogCounts.bindLong(4, pair.getValue());
+
+                insertLogCounts.executeInsert();
             }
         }
     }
@@ -2593,13 +2589,10 @@ public class DataStore {
         database.beginTransaction();
         try {
             final SQLiteStatement setVisit = PreparedStatement.UPDATE_VISIT_DATE.getStatement();
-
-            synchronized (setVisit) {
-                for (final String geocode : geocodes) {
-                    setVisit.bindLong(1, visitedDate);
-                    setVisit.bindString(2, geocode);
-                    setVisit.execute();
-                }
+            for (final String geocode : geocodes) {
+                setVisit.bindLong(1, visitedDate);
+                setVisit.bindString(2, geocode);
+                setVisit.execute();
             }
             database.setTransactionSuccessful();
         } finally {
@@ -2764,10 +2757,8 @@ public class DataStore {
             if (cnt > 0) {
                 // move caches from deleted list to standard list
                 final SQLiteStatement moveToStandard = PreparedStatement.MOVE_TO_STANDARD_LIST.getStatement();
-                synchronized (moveToStandard) {
-                    moveToStandard.bindLong(1, listId);
-                    moveToStandard.execute();
-                }
+                moveToStandard.bindLong(1, listId);
+                moveToStandard.execute();
 
                 status = true;
             }
@@ -2800,18 +2791,16 @@ public class DataStore {
         final SQLiteStatement move = PreparedStatement.MOVE_TO_LIST.getStatement();
 
         database.beginTransaction();
-        synchronized (move) {
-            try {
-                for (final Geocache cache : caches) {
-                    move.bindLong(1, listId);
-                    move.bindString(2, cache.getGeocode());
-                    move.execute();
-                    cache.setListId(listId);
-                }
-                database.setTransactionSuccessful();
-            } finally {
-                database.endTransaction();
+        try {
+            for (final Geocache cache : caches) {
+                move.bindLong(1, listId);
+                move.bindString(2, cache.getGeocode());
+                move.execute();
+                cache.setListId(listId);
             }
+            database.setTransactionSuccessful();
+        } finally {
+            database.endTransaction();
         }
     }
 
@@ -2966,7 +2955,7 @@ public class DataStore {
         MOVE_TO_STANDARD_LIST("UPDATE " + dbTableCaches + " SET reason = " + StoredList.STANDARD_LIST_ID + " WHERE reason = ?"),
         MOVE_TO_LIST("UPDATE " + dbTableCaches + " SET reason = ? WHERE geocode = ?"),
         UPDATE_VISIT_DATE("UPDATE " + dbTableCaches + " SET visiteddate = ? WHERE geocode = ?"),
-        INSERT_LOG_IMAGE("INSERT INTO " + dbTableLogImages + " (log_id, title, url) VALUES (?, ?, ?)"), // See use of INSERT_LOG for synchronization
+        INSERT_LOG_IMAGE("INSERT INTO " + dbTableLogImages + " (log_id, title, url) VALUES (?, ?, ?)"),
         INSERT_LOG_COUNTS("INSERT INTO " + dbTableLogCount + " (geocode, updated, type, count) VALUES (?, ?, ?, ?)"),
         INSERT_SPOILER("INSERT INTO " + dbTableSpoilers + " (geocode, updated, url, title, description) VALUES (?, ?, ?, ?, ?)"),
         LOG_COUNT_OF_GEOCODE("SELECT count(_id) FROM " + DataStore.dbTableLogsOffline + " WHERE geocode = ?"),
diff --git a/main/src/cgeo/geocaching/LogCacheActivity.java b/main/src/cgeo/geocaching/LogCacheActivity.java
index 118c47b..1d31e27 100644
--- a/main/src/cgeo/geocaching/LogCacheActivity.java
+++ b/main/src/cgeo/geocaching/LogCacheActivity.java
@@ -419,7 +419,7 @@ public class LogCacheActivity extends AbstractLoggingActivity implements DateDia
                     final LogEntry logNow = new LogEntry(date.getTimeInMillis(), typeSelected, log);
                     logNow.friend = true;
                     newLogs.add(0, logNow);
-                    DataStore.saveLogsWithoutTransaction(cache.getGeocode(), newLogs);
+                    DataStore.saveLogs(cache.getGeocode(), newLogs);
 
                     // update offline log in DB
                     cache.clearOfflineLog();
@@ -443,7 +443,7 @@ public class LogCacheActivity extends AbstractLoggingActivity implements DateDia
                         final String uploadedImageUrl = imageResult.getImageUri();
                         if (StringUtils.isNotEmpty(uploadedImageUrl)) {
                             logNow.addLogImage(new Image(uploadedImageUrl, imageCaption, imageDescription));
-                            DataStore.saveLogsWithoutTransaction(cache.getGeocode(), newLogs);
+                            DataStore.saveLogs(cache.getGeocode(), newLogs);
                         }
                         return imageResult.getPostResult();
                     }
diff --git a/main/src/cgeo/geocaching/connector/gc/GCParser.java b/main/src/cgeo/geocaching/connector/gc/GCParser.java
index a25cba8..014acf7 100644
--- a/main/src/cgeo/geocaching/connector/gc/GCParser.java
+++ b/main/src/cgeo/geocaching/connector/gc/GCParser.java
@@ -393,7 +393,7 @@ public abstract class GCParser {
         final SearchResult result = new SearchResult(parsed.left);
         if (parsed.left == StatusCode.NO_ERROR) {
             result.addAndPutInCache(Collections.singletonList(parsed.right));
-            DataStore.saveLogsWithoutTransaction(parsed.right.getGeocode(), getLogs(parseUserToken(page), Logs.ALL).toBlocking().toIterable());
+            DataStore.saveLogs(parsed.right.getGeocode(), getLogs(parseUserToken(page), Logs.ALL).toBlocking().toIterable());
         }
         return result;
     }
@@ -1883,7 +1883,7 @@ public abstract class GCParser {
         mergedLogs.subscribe(new Action1<List<LogEntry>>() {
                                  @Override
                                  public void call(final List<LogEntry> logEntries) {
-                                     DataStore.saveLogsWithoutTransaction(cache.getGeocode(), logEntries);
+                                     DataStore.saveLogs(cache.getGeocode(), logEntries);
                                  }
                              });
         if (cache.isFound() && cache.getVisitedDate() == 0) {
diff --git a/main/src/cgeo/geocaching/connector/oc/OkapiClient.java b/main/src/cgeo/geocaching/connector/oc/OkapiClient.java
index ee095e3..275a103 100644
--- a/main/src/cgeo/geocaching/connector/oc/OkapiClient.java
+++ b/main/src/cgeo/geocaching/connector/oc/OkapiClient.java
@@ -403,7 +403,7 @@ final class OkapiClient {
             cache.setDetailedUpdatedNow();
             // save full detailed caches
             DataStore.saveCache(cache, EnumSet.of(SaveFlag.DB));
-            DataStore.saveLogsWithoutTransaction(cache.getGeocode(), parseLogs((ArrayNode) response.path(CACHE_LATEST_LOGS)));
+            DataStore.saveLogs(cache.getGeocode(), parseLogs((ArrayNode) response.path(CACHE_LATEST_LOGS)));
         } catch (ClassCastException | NullPointerException e) {
             Log.e("OkapiClient.parseCache", e);
         }
diff --git a/main/src/cgeo/geocaching/files/GPXParser.java b/main/src/cgeo/geocaching/files/GPXParser.java
index 7dd5c4a..4f8979b 100644
--- a/main/src/cgeo/geocaching/files/GPXParser.java
+++ b/main/src/cgeo/geocaching/files/GPXParser.java
@@ -344,7 +344,7 @@ public abstract class GPXParser extends FileParser {
                     // finally store the cache in the database
                     result.add(geocode);
                     DataStore.saveCache(cache, EnumSet.of(SaveFlag.DB));
-                    DataStore.saveLogsWithoutTransaction(cache.getGeocode(), logs);
+                    DataStore.saveLogs(cache.getGeocode(), logs);
 
                     // avoid the cachecache using lots of memory for caches which the user did not actually look at
                     DataStore.removeCache(geocode, EnumSet.of(RemoveFlag.CACHE));
diff --git a/tests/src/cgeo/geocaching/export/ExportTest.java b/tests/src/cgeo/geocaching/export/ExportTest.java
index 7dc4ed9..965abdd 100644
--- a/tests/src/cgeo/geocaching/export/ExportTest.java
+++ b/tests/src/cgeo/geocaching/export/ExportTest.java
@@ -34,7 +34,7 @@ public class ExportTest extends CGeoTestCase {
         cache.setCoords(new Geopoint("N 49 44.000 E 8 37.000"));
         final LogEntry log = new LogEntry(1353244820000L, LogType.FOUND_IT, "Smile: \ud83d\ude0a");
         DataStore.saveCache(cache, LoadFlags.SAVE_ALL);
-        DataStore.saveLogsWithoutTransaction(cache.getGeocode(), Collections.singletonList(log));
+        DataStore.saveLogs(cache.getGeocode(), Collections.singletonList(log));
         ArrayList<Geocache> exportList = new ArrayList<Geocache>();
         exportList.add(cache);
         GpxExportTester gpxExport = new GpxExportTester();