fix #3791: have logs separated from caches

8 files changed, 58 insertions, 55 deletions

diff --git a/main/src/cgeo/geocaching/DataStore.java b/main/src/cgeo/geocaching/DataStore.java
index 302bfc3..0adefb8 100644
--- a/main/src/cgeo/geocaching/DataStore.java
+++ b/main/src/cgeo/geocaching/DataStore.java
@@ -25,6 +25,7 @@ import org.apache.commons.collections4.CollectionUtils;
 import org.apache.commons.collections4.MapUtils;
 import org.apache.commons.lang3.StringUtils;
 import org.eclipse.jdt.annotation.NonNull;
+
 import rx.android.observables.AndroidObservable;
 import rx.functions.Action1;
 import rx.functions.Func0;
@@ -1188,7 +1189,6 @@ public class DataStore {
             saveAttributesWithoutTransaction(cache);
             saveWaypointsWithoutTransaction(cache);
             saveSpoilersWithoutTransaction(cache);
-            saveLogsWithoutTransaction(cache.getGeocode(), cache.getLogs());
             saveLogCountsWithoutTransaction(cache);
             saveInventoryWithoutTransaction(cache.getGeocode(), cache.getInventory());
 
@@ -1421,7 +1421,7 @@ public class DataStore {
         }
     }
 
-    private static void saveLogsWithoutTransaction(final String geocode, final List<LogEntry> logs) {
+    public static void saveLogsWithoutTransaction(final String geocode, final List<LogEntry> logs) {
         // TODO delete logimages referring these logs
         database.delete(dbTableLogs, "geocode = ?", new String[]{geocode});
 
@@ -1655,7 +1655,6 @@ public class DataStore {
                 }
 
                 if (loadFlags.contains(LoadFlag.LOAD_LOGS)) {
-                    cache.setLogs(loadLogs(cache.getGeocode()));
                     final Map<LogType, Integer> logCounts = loadLogCounts(cache.getGeocode());
                     if (MapUtils.isNotEmpty(logCounts)) {
                         cache.getLogCounts().clear();
@@ -1917,6 +1916,11 @@ public class DataStore {
         return false;
     }
 
+    /**
+     * @param geocode
+     * @return an immutable, non null list of logs
+     */
+    @NonNull
     public static List<LogEntry> loadLogs(String geocode) {
         List<LogEntry> logs = new ArrayList<LogEntry>();
 
@@ -1952,7 +1956,7 @@ public class DataStore {
 
         cursor.close();
 
-        return logs;
+        return Collections.unmodifiableList(logs);
     }
 
     public static Map<LogType, Integer> loadLogCounts(String geocode) {
diff --git a/main/src/cgeo/geocaching/Geocache.java b/main/src/cgeo/geocaching/Geocache.java
index ea3c7b5..bd1d8bf 100644
--- a/main/src/cgeo/geocaching/Geocache.java
+++ b/main/src/cgeo/geocaching/Geocache.java
@@ -42,6 +42,7 @@ import org.apache.commons.collections4.Predicate;
 import org.apache.commons.lang3.BooleanUtils;
 import org.apache.commons.lang3.ObjectUtils;
 import org.apache.commons.lang3.StringUtils;
+import org.eclipse.jdt.annotation.NonNull;
 import org.eclipse.jdt.annotation.Nullable;
 
 import rx.Scheduler;
@@ -79,6 +80,10 @@ import java.util.regex.Pattern;
 /**
  * Internal c:geo representation of a "cache"
  */
+/**
+ * @author kep9fe
+ *
+ */
 public class Geocache implements ICache, IWaypoint {
 
     private static final int OWN_WP_PREFIX_OFFSET = 17;
@@ -144,12 +149,7 @@ public class Geocache implements ICache, IWaypoint {
         }
     };
     private List<Image> spoilers = null;
-    private final LazyInitializedList<LogEntry> logs = new LazyInitializedList<LogEntry>() {
-        @Override
-        public List<LogEntry> call() {
-            return DataStore.loadLogs(geocode);
-        }
-    };
+
     private List<Trackable> inventory = null;
     private Map<LogType, Integer> logCounts = new HashMap<LogType, Integer>();
     private boolean userModifiedCoords = false;
@@ -187,7 +187,6 @@ public class Geocache implements ICache, IWaypoint {
         setReliableLatLon(true);
         setAttributes(Collections.<String> emptyList());
         setWaypoints(Collections.<Waypoint> emptyList(), false);
-        setLogs(Collections.<LogEntry> emptyList());
     }
 
     public void setChangeNotificationHandler(Handler newNotificationHandler) {
@@ -355,12 +354,6 @@ public class Geocache implements ICache, IWaypoint {
             inventory = other.inventory;
             inventoryItems = other.inventoryItems;
         }
-        if (logs.isEmpty()) { // keep last known logs if none
-            logs.clear();
-            if (other.logs != null) {
-                logs.addAll(other.logs);
-            }
-        }
         if (logCounts.isEmpty()) {
             logCounts = other.logCounts;
         }
@@ -427,7 +420,6 @@ public class Geocache implements ICache, IWaypoint {
                 attributes == other.attributes &&
                 waypoints == other.waypoints &&
                 spoilers == other.spoilers &&
-                logs == other.logs &&
                 inventory == other.inventory &&
                 logCounts == other.logCounts &&
                 ObjectUtils.equals(logOffline, other.logOffline) &&
@@ -1008,21 +1000,24 @@ public class Geocache implements ICache, IWaypoint {
     }
 
     /**
-     * @return never <code>null</code>
+     * The list of logs is immutable, because it is directly fetched from the database on demand, and not stored at this
+     * object. If you want to modify logs, you have to load all logs of the cache, create a new list from the existing
+     * list and store that new list in the database.
+     *
+     * @return immutable list of logs
      */
+    @NonNull
     public List<LogEntry> getLogs() {
-        // It is important to return the underlying list here and not the lazily initialized one,
-        // because database manipulation may erase the existing logs before methods are called
-        // on the previous logs, when updating the saved logs for example.
-        return logs.getUnderlyingList();
+        return DataStore.loadLogs(geocode);
     }
 
     /**
-     * @return only the logs of friends, never <code>null</code>
+     * @return only the logs of friends
      */
+    @NonNull
     public List<LogEntry> getFriendsLogs() {
         final ArrayList<LogEntry> friendLogs = new ArrayList<LogEntry>();
-        for (final LogEntry log : logs) {
+        for (final LogEntry log : getLogs()) {
             if (log.friend) {
                 friendLogs.add(log);
             }
@@ -1030,17 +1025,6 @@ public class Geocache implements ICache, IWaypoint {
         return Collections.unmodifiableList(friendLogs);
     }
 
-    /**
-     * @param logs
-     *            the log entries
-     */
-    public void setLogs(List<LogEntry> logs) {
-        this.logs.clear();
-        if (logs != null) {
-            this.logs.addAll(logs);
-        }
-    }
-
     public boolean isLogOffline() {
         return BooleanUtils.isTrue(logOffline);
     }
diff --git a/main/src/cgeo/geocaching/LogCacheActivity.java b/main/src/cgeo/geocaching/LogCacheActivity.java
index a1368cc..2b05263 100644
--- a/main/src/cgeo/geocaching/LogCacheActivity.java
+++ b/main/src/cgeo/geocaching/LogCacheActivity.java
@@ -515,16 +515,20 @@ public class LogCacheActivity extends AbstractLoggingActivity implements DateDia
                 final LogResult logResult = loggingManager.postLog(cache, typeSelected, date, log, logPwd, trackables);
 
                 if (logResult.getPostLogResult() == StatusCode.NO_ERROR) {
-                    final LogEntry logNow = new LogEntry(date.getTimeInMillis(), typeSelected, log);
-
-                    cache.getLogs().add(0, logNow);
-
+                    // update geocache in DB
                     if (typeSelected == LogType.FOUND_IT || typeSelected == LogType.ATTENDED) {
                         cache.setFound(true);
                         cache.setVisitedDate(new Date().getTime());
                     }
-
                     DataStore.saveChangedCache(cache);
+
+                    // update logs in DB
+                    ArrayList<LogEntry> newLogs = new ArrayList<LogEntry>(cache.getLogs());
+                    final LogEntry logNow = new LogEntry(date.getTimeInMillis(), typeSelected, log);
+                    newLogs.add(0, logNow);
+                    DataStore.saveLogsWithoutTransaction(cache.getGeocode(), newLogs);
+
+                    // update offline log in DB
                     cache.clearOfflineLog();
 
                     if (typeSelected == LogType.FOUND_IT) {
@@ -541,7 +545,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.saveChangedCache(cache);
+                            DataStore.saveLogsWithoutTransaction(cache.getGeocode(), newLogs);
                         }
                         return imageResult.getPostResult();
                     }
diff --git a/main/src/cgeo/geocaching/LogEntry.java b/main/src/cgeo/geocaching/LogEntry.java
index fde0564..ca4a3d1 100644
--- a/main/src/cgeo/geocaching/LogEntry.java
+++ b/main/src/cgeo/geocaching/LogEntry.java
@@ -14,6 +14,11 @@ import java.util.Collections;
 import java.util.List;
 import java.util.regex.Pattern;
 
+/**
+ * Entry in a log book. This object should not be referenced directly from a Geocache object to reduce the memory usage
+ * of the Geocache objects.
+ * 
+ */
 public final class LogEntry {
 
     private static final Pattern PATTERN_REMOVE_COLORS = Pattern.compile("</?font.*?>", Pattern.CASE_INSENSITIVE);
diff --git a/main/src/cgeo/geocaching/connector/gc/GCParser.java b/main/src/cgeo/geocaching/connector/gc/GCParser.java
index 56d434d..0549b3f 100644
--- a/main/src/cgeo/geocaching/connector/gc/GCParser.java
+++ b/main/src/cgeo/geocaching/connector/gc/GCParser.java
@@ -36,6 +36,7 @@ import cgeo.geocaching.utils.SynchronizedDateFormat;
 import cgeo.geocaching.utils.TextUtils;
 
 import ch.boye.httpclientandroidlib.HttpResponse;
+
 import org.apache.commons.collections4.CollectionUtils;
 import org.apache.commons.lang3.StringEscapeUtils;
 import org.apache.commons.lang3.StringUtils;
@@ -723,9 +724,6 @@ public abstract class GCParser {
 
         cache.parseWaypointsFromNote();
 
-        // logs
-        cache.setLogs(getLogsFromDetails(page, false));
-
         // last check for necessary cache conditions
         if (StringUtils.isBlank(cache.getGeocode())) {
             searchResult.setError(StatusCode.UNKNOWN_ERROR);
@@ -734,6 +732,7 @@ public abstract class GCParser {
 
         cache.setDetailedUpdatedNow();
         searchResult.addAndPutInCache(Collections.singletonList(cache));
+        DataStore.saveLogsWithoutTransaction(cache.getGeocode(), getLogsFromDetails(page, false));
         return searchResult;
     }
 
@@ -1831,16 +1830,18 @@ public abstract class GCParser {
         //cache.setLogs(loadLogsFromDetails(page, cache, false));
         if (Settings.isFriendLogsWanted()) {
             CancellableHandler.sendLoadProgressDetail(handler, R.string.cache_dialog_loading_details_status_logs);
-            final List<LogEntry> allLogs = cache.getLogs();
             final List<LogEntry> friendLogs = getLogsFromDetails(page, true);
-            if (friendLogs != null) {
+            if (friendLogs != null && !friendLogs.isEmpty()) {
+                // create new list, as the existing log list is immutable
+                ArrayList<LogEntry> mergedLogs = new ArrayList<LogEntry>(cache.getLogs());
                 for (final LogEntry log : friendLogs) {
-                    if (allLogs.contains(log)) {
-                        allLogs.get(allLogs.indexOf(log)).friend = true;
+                    if (mergedLogs.contains(log)) {
+                        mergedLogs.get(mergedLogs.indexOf(log)).friend = true;
                     } else {
-                        cache.getLogs().add(log);
+                        mergedLogs.add(log);
                     }
                 }
+                DataStore.saveLogsWithoutTransaction(cache.getGeocode(), mergedLogs);
             }
         }
 
diff --git a/main/src/cgeo/geocaching/connector/oc/OkapiClient.java b/main/src/cgeo/geocaching/connector/oc/OkapiClient.java
index 712bb26..3c93488 100644
--- a/main/src/cgeo/geocaching/connector/oc/OkapiClient.java
+++ b/main/src/cgeo/geocaching/connector/oc/OkapiClient.java
@@ -372,7 +372,6 @@ final class OkapiClient {
             }
 
             cache.setAttributes(parseAttributes(response.getJSONArray(CACHE_ATTRNAMES), response.optJSONArray(CACHE_ATTR_ACODES)));
-            cache.setLogs(parseLogs(response.getJSONArray(CACHE_LATEST_LOGS)));
             //TODO: Store license per cache
             //cache.setLicense(response.getString("attribution_note"));
             cache.setWaypoints(parseWaypoints(response.getJSONArray(CACHE_WPTS)), false);
@@ -388,6 +387,7 @@ final class OkapiClient {
             cache.setDetailedUpdatedNow();
             // save full detailed caches
             DataStore.saveCache(cache, EnumSet.of(SaveFlag.SAVE_DB));
+            DataStore.saveLogsWithoutTransaction(cache.getGeocode(), parseLogs(response.getJSONArray(CACHE_LATEST_LOGS)));
         } catch (final JSONException e) {
             Log.e("OkapiClient.parseCache", e);
         }
diff --git a/main/src/cgeo/geocaching/export/GpxSerializer.java b/main/src/cgeo/geocaching/export/GpxSerializer.java
index fef91cf..2da1638 100644
--- a/main/src/cgeo/geocaching/export/GpxSerializer.java
+++ b/main/src/cgeo/geocaching/export/GpxSerializer.java
@@ -239,12 +239,13 @@ public final class GpxSerializer {
     }
 
     private void writeLogs(final Geocache cache) throws IOException {
-        if (cache.getLogs().isEmpty()) {
+        List<LogEntry> logs = cache.getLogs();
+        if (logs.isEmpty()) {
             return;
         }
         gpx.startTag(PREFIX_GROUNDSPEAK, "logs");
 
-        for (final LogEntry log : cache.getLogs()) {
+        for (final LogEntry log : logs) {
             gpx.startTag(PREFIX_GROUNDSPEAK, "log");
             gpx.attribute("", "id", Integer.toString(log.id));
 
diff --git a/main/src/cgeo/geocaching/files/GPXParser.java b/main/src/cgeo/geocaching/files/GPXParser.java
index f5380be..8d328e4 100644
--- a/main/src/cgeo/geocaching/files/GPXParser.java
+++ b/main/src/cgeo/geocaching/files/GPXParser.java
@@ -42,6 +42,7 @@ import java.util.Collection;
 import java.util.Date;
 import java.util.EnumSet;
 import java.util.HashSet;
+import java.util.List;
 import java.util.Locale;
 import java.util.Set;
 import java.util.regex.Pattern;
@@ -99,6 +100,7 @@ public abstract class GPXParser extends FileParser {
     private String parentCacheCode = null;
     private boolean wptVisited = false;
     private boolean wptUserDefined = false;
+    private List<LogEntry> logs = new ArrayList<LogEntry>();
 
     /**
      * Parser result. Maps geocode to cache.
@@ -334,6 +336,7 @@ public abstract class GPXParser extends FileParser {
                     // finally store the cache in the database
                     result.add(geocode);
                     DataStore.saveCache(cache, EnumSet.of(SaveFlag.SAVE_DB));
+                    DataStore.saveLogsWithoutTransaction(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.REMOVE_CACHE));
@@ -757,7 +760,7 @@ public abstract class GPXParser extends FileParser {
                 @Override
                 public void end() {
                     if (log.type != LogType.UNKNOWN) {
-                        cache.getLogs().add(log);
+                        logs.add(log);
                     }
                 }
             });
@@ -876,7 +879,7 @@ public abstract class GPXParser extends FileParser {
 
     /**
      * Add listeners for c:geo extensions
-     * 
+     *
      * @param cacheParent
      */
     private void registerCgeoExtensions(final Element cacheParent) {
@@ -985,6 +988,7 @@ public abstract class GPXParser extends FileParser {
         parentCacheCode = null;
         wptVisited = false;
         wptUserDefined = false;
+        logs = new ArrayList<LogEntry>();
 
         cache = new Geocache(this);