Fix bug on compute methods that prevented them from recomputing old values

app/src/androidTest/java/org/isoron/uhabits/activities/common/views/ScoreChartTest.java

@@ -49,7 +49,7 @@ public class ScoreChartTest extends BaseViewTest
         habit = fixtures.createLongHabit();
 
         view = new ScoreChart(targetContext);
-        view.setScores(habit.getScores().getAll());
+        view.setScores(habit.getScores().toList());
         view.setColor(ColorUtils.getColor(targetContext, habit.getColor()));
         view.setBucketSize(7);
         measureView(view, dpToPixels(300), dpToPixels(200));

app/src/androidTest/java/org/isoron/uhabits/models/sqlite/SQLiteCheckmarkListTest.java

@@ -92,6 +92,16 @@ public class SQLiteCheckmarkListTest extends BaseAndroidTest
         assertThat(list.get(7).getTimestamp(), equalTo(today - 10 * day));
     }
 
+    @Test
+    public void testGetByInterval_withLongInterval()
+    {
+        long from = today - 200 * day;
+        long to = today;
+
+        List<Checkmark> list = checkmarks.getByInterval(from, to);
+        assertThat(list.size(), equalTo(201));
+    }
+
     @Test
     public void testInvalidateNewerThan()
     {

app/src/androidTest/java/org/isoron/uhabits/models/sqlite/SQLiteScoreListTest.java

@@ -33,8 +33,6 @@ import org.junit.runner.*;
 
 import java.util.*;
 
-import static junit.framework.Assert.assertNotNull;
-import static junit.framework.Assert.assertNull;
 import static org.hamcrest.MatcherAssert.*;
 import static org.hamcrest.Matchers.*;
 
@@ -66,7 +64,7 @@ public class SQLiteScoreListTest extends BaseAndroidTest
     @Test
     public void testGetAll()
     {
-        List<Score> list = scores.getAll();
+        List<Score> list = scores.toList();
         assertThat(list.size(), equalTo(121));
         assertThat(list.get(0).getTimestamp(), equalTo(today));
         assertThat(list.get(10).getTimestamp(), equalTo(today - 10 * day));
@@ -103,17 +101,6 @@ public class SQLiteScoreListTest extends BaseAndroidTest
         assertThat(records.get(0).timestamp, equalTo(today));
     }
 
-    @Test
-    public void testGetByTimestamp()
-    {
-        Score s = scores.getByTimestamp(today);
-        assertNotNull(s);
-        assertThat(s.getTimestamp(), equalTo(today));
-
-        s = scores.getByTimestamp(today - 200 * day);
-        assertNull(s);
-    }
-
     private List<ScoreRecord> getAllRecords()
     {
         return new Select()

app/src/main/java/org/isoron/uhabits/activities/habits/show/views/ScoreCard.java

@@ -146,7 +146,7 @@ public class ScoreCard extends HabitCard
             List<Score> scores;
             ScoreList scoreList = getHabit().getScores();
 
-            if (bucketSize == 1) scores = scoreList.getAll();
+            if (bucketSize == 1) scores = scoreList.toList();
             else scores = scoreList.groupBy(getTruncateField(bucketSize));
 
             chart.setScores(scores);

app/src/main/java/org/isoron/uhabits/models/CheckmarkList.java

@@ -176,19 +176,54 @@ public abstract class CheckmarkList
      * specified interval of time. Days that already have a corresponding
      * checkmark are skipped.
      *
+     * This method assumes the list of computed checkmarks has no holes. That
+     * is, if there is a checkmark computed at time t1 and another at time t2,
+     * then every checkmark between t1 and t2 is also computed.
+     *
      * @param from timestamp for the beginning of the interval
      * @param to   timestamp for the end of the interval
      */
-    protected final synchronized void compute(long from, final long to)
+    protected final synchronized void compute(long from, long to)
     {
         final long day = DateUtils.millisecondsInOneDay;
 
-        Checkmark newestCheckmark = getNewestComputed();
-        if (newestCheckmark != null)
-            from = newestCheckmark.getTimestamp() + day;
+        Checkmark newest = getNewestComputed();
+        Checkmark oldest = getOldestComputed();
+
+        if (newest == null)
+        {
+            forceRecompute(from, to);
+        }
+        else
+        {
+            forceRecompute(from, oldest.getTimestamp() - day);
+            forceRecompute(newest.getTimestamp() + day, to);
+        }
+    }
+
+    /**
+     * Returns oldest checkmark that has already been computed.
+     *
+     * @return oldest checkmark already computed
+     */
+    protected abstract Checkmark getOldestComputed();
 
+    /**
+     * Computes and stores one checkmark for each day that falls inside the
+     * specified interval of time.
+     *
+     * This method does not check if the checkmarks have already been
+     * computed or not. If they have, then duplicate checkmarks will
+     * be stored, which is a bad thing.
+     *
+     * @param from timestamp for the beginning of the interval
+     * @param to   timestamp for the end of the interval
+     */
+    private synchronized void forceRecompute(long from, long to)
+    {
         if (from > to) return;
 
+        final long day = DateUtils.millisecondsInOneDay;
         Frequency freq = habit.getFrequency();
 
         long fromExtended = from - (long) (freq.getDenominator()) * day;
@@ -231,8 +266,8 @@ public abstract class CheckmarkList
 
     /**
      * Computes and stores one checkmark for each day, since the first
-     * repetition until today. Days that already have a corresponding checkmark
-     * are skipped.
+     * repetition of the habit until today. Days that already have a
+     * corresponding checkmark are skipped.
      */
     protected final void computeAll()
     {
@@ -245,8 +280,6 @@ public abstract class CheckmarkList
 
     /**
      * Returns newest checkmark that has already been computed.
-     * <p>
-     * Ignores any checkmark that has timestamp in the future.
      *
      * @return newest checkmark already computed
      */

app/src/main/java/org/isoron/uhabits/models/ScoreList.java

@@ -57,19 +57,6 @@ public abstract class ScoreList implements Iterable<Score>
      */
     public abstract void add(List<Score> scores);
 
-    public abstract List<Score> getAll();
-
-    /**
-     * Returns the score that has the given timestamp.
-     * <p>
-     * If no such score exists, returns null.
-     *
-     * @param timestamp the timestamp to find.
-     * @return the score with given timestamp, or null if none exists.
-     */
-    @Nullable
-    public abstract Score getByTimestamp(long timestamp);
-
     public ModelObservable getObservable()
     {
         return observable;
@@ -88,22 +75,23 @@ public abstract class ScoreList implements Iterable<Score>
     /**
      * Returns the value of the score for a given day.
      * <p>
-     * If there is no score at the given timestamp (for example, if the
-     * timestamp given happens before the first repetition of the habit) then
-     * returns zero.
+     * If the timestamp given happens before the first repetition of the habit
+     * then returns zero.
      *
      * @param timestamp the timestamp of a day
      * @return score value for that day
      */
     public final int getValue(long timestamp)
     {
-        Score s = getByTimestamp(timestamp);
-        if (s != null) return s.getValue();
-        return 0;
+        compute(timestamp, timestamp);
+        Score s = getComputedByTimestamp(timestamp);
+        if(s == null) throw new IllegalStateException();
+        return s.getValue();
     }
 
     public List<Score> groupBy(DateUtils.TruncateField field)
     {
+        computeAll();
         HashMap<Long, ArrayList<Long>> groups = getGroupedValues(field);
         List<Score> scores = groupsToAvgScores(groups);
         Collections.sort(scores, (s1, s2) -> s2.compareNewer(s1));
@@ -122,9 +110,20 @@ public abstract class ScoreList implements Iterable<Score>
     @Override
     public Iterator<Score> iterator()
     {
-        return getAll().iterator();
+        return toList().iterator();
     }
 
+    /**
+     * Returns a Java list of scores, containing one score for each day, from
+     * the first repetition of the habit until today.
+     * <p>
+     * The scores are sorted by decreasing timestamp. The first score
+     * corresponds to today.
+     *
+     * @return list of scores
+     */
+    public abstract List<Score> toList();
+
     public void writeCSV(Writer out) throws IOException
     {
         computeAll();
@@ -140,16 +139,15 @@ public abstract class ScoreList implements Iterable<Score>
     }
 
     /**
-     * Computes and saves the scores that are missing inside a given time
-     * interval.
+     * Computes and stores one score for each day inside the given interval.
      * <p>
      * Scores that have already been computed are skipped, therefore there is no
      * harm in calling this function more times, or with larger intervals, than
      * strictly needed. The endpoints of the interval are included.
      * <p>
-     * This function assumes that there are no gaps on the scores. That is, if
-     * the newest score has timestamp t, then every score with timestamp lower
-     * than t has already been computed.
+     * This method assumes the list of computed scores has no holes. That is, if
+     * there is a score computed at time t1 and another at time t2, then every
+     * score between t1 and t2 is also computed.
      *
      * @param from timestamp of the beginning of the interval
      * @param to   timestamp of the end of the time interval
@@ -157,34 +155,24 @@ public abstract class ScoreList implements Iterable<Score>
     protected synchronized void compute(long from, long to)
     {
         final long day = DateUtils.millisecondsInOneDay;
-        final double freq = habit.getFrequency().toDouble();
-
-        int newestValue = 0;
-        long newestTimestamp = 0;
 
         Score newest = getNewestComputed();
-        if (newest != null)
+        Score oldest = getOldestComputed();
+
+        if (newest == null)
         {
-            newestValue = newest.getValue();
-            newestTimestamp = newest.getTimestamp();
+            Repetition oldestRep = habit.getRepetitions().getOldest();
+            if (oldestRep != null)
+                from = Math.min(from, oldestRep.getTimestamp());
+            forceRecompute(from, to, 0);
         }
-
-        if (newestTimestamp > 0) from = newestTimestamp + day;
-
-        final int checkmarkValues[] = habit.getCheckmarks().getValues(from, to);
-        final long beginning = from;
-
-        int lastScore = newestValue;
-        List<Score> scores = new LinkedList<>();
-
-        for (int i = 0; i < checkmarkValues.length; i++)
+        else
         {
-            int value = checkmarkValues[checkmarkValues.length - i - 1];
-            lastScore = Score.compute(freq, lastScore, value);
-            scores.add(new Score(beginning + day * i, lastScore));
+            if (oldest == null) throw new IllegalStateException();
+            forceRecompute(from, oldest.getTimestamp() - day, 0);
+            forceRecompute(newest.getTimestamp() + day, to,
+                newest.getValue());
         }
-
-        add(scores);
     }
 
     /**
@@ -196,20 +184,66 @@ public abstract class ScoreList implements Iterable<Score>
         Repetition oldestRep = habit.getRepetitions().getOldest();
         if (oldestRep == null) return;
 
-        long toTimestamp = DateUtils.getStartOfToday();
-        compute(oldestRep.getTimestamp(), toTimestamp);
+        long today = DateUtils.getStartOfToday();
+        compute(oldestRep.getTimestamp(), today);
     }
 
     /**
-     * Returns the most recent score that has already been computed.
-     * <p>
-     * If no score has been computed yet, returns null.
+     * Returns the score that has the given timestamp, if it has already been
+     * computed. If that score has not been computed yet, returns null.
      *
-     * @return the newest score computed, or null if none exist
+     * @param timestamp the timestamp of the score
+     * @return the score with given timestamp, or null not yet computed.
+     */
+    @Nullable
+    protected abstract Score getComputedByTimestamp(long timestamp);
+
+    /**
+     * Returns the most recent score that has already been computed. If no score
+     * has been computed yet, returns null.
      */
     @Nullable
     protected abstract Score getNewestComputed();
 
+    /**
+     * Returns oldest score already computed. If no score has been computed yet,
+     * returns null.
+     */
+    @Nullable
+    protected abstract Score getOldestComputed();
+
+    /**
+     * Computes and stores one score for each day inside the given interval.
+     * <p>
+     * This function does not check if the scores have already been computed. If
+     * they have, then it stores duplicate scores, which is a bad thing.
+     *
+     * @param from          timestamp of the beginning of the interval
+     * @param to            timestamp of the end of the interval
+     * @param previousValue value of the score on the day immediately before the
+     *                      interval begins
+     */
+    private void forceRecompute(long from, long to, int previousValue)
+    {
+        if(from > to) return;
+
+        final long day = DateUtils.millisecondsInOneDay;
+
+        final double freq = habit.getFrequency().toDouble();
+        final int checkmarkValues[] = habit.getCheckmarks().getValues(from, to);
+
+        List<Score> scores = new LinkedList<>();
+
+        for (int i = 0; i < checkmarkValues.length; i++)
+        {
+            int value = checkmarkValues[checkmarkValues.length - i - 1];
+            previousValue = Score.compute(freq, previousValue, value);
+            scores.add(new Score(from + day * i, previousValue));
+        }
+
+        add(scores);
+    }
+
     @NonNull
     private HashMap<Long, ArrayList<Long>> getGroupedValues(DateUtils.TruncateField field)
     {

app/src/main/java/org/isoron/uhabits/models/memory/MemoryCheckmarkList.java

@@ -72,21 +72,17 @@ public class MemoryCheckmarkList extends CheckmarkList
     }
 
     @Override
-    protected Checkmark getNewestComputed()
-    {
-        long newestTimestamp = 0;
-        Checkmark newestCheck = null;
-
-        for (Checkmark c : list)
-        {
-            if (c.getTimestamp() > newestTimestamp)
+    protected Checkmark getOldestComputed()
     {
-                newestCheck = c;
-                newestTimestamp = c.getTimestamp();
-            }
+        if(list.isEmpty()) return null;
+        return list.getLast();
     }
 
-        return newestCheck;
+    @Override
+    protected Checkmark getNewestComputed()
+    {
+        if(list.isEmpty()) return null;
+        return list.getFirst();
     }
 
 }

app/src/main/java/org/isoron/uhabits/models/memory/MemoryScoreList.java

@@ -27,7 +27,7 @@ import java.util.*;
 
 public class MemoryScoreList extends ScoreList
 {
-    List<Score> list;
+    LinkedList<Score> list;
 
     public MemoryScoreList(Habit habit)
     {
@@ -35,6 +35,24 @@ public class MemoryScoreList extends ScoreList
         list = new LinkedList<>();
     }
 
+    @Override
+    public void add(List<Score> scores)
+    {
+        list.addAll(scores);
+        Collections.sort(list,
+            (s1, s2) -> Long.signum(s2.getTimestamp() - s1.getTimestamp()));
+    }
+
+    @Nullable
+    @Override
+    public Score getComputedByTimestamp(long timestamp)
+    {
+        for (Score s : list)
+            if (s.getTimestamp() == timestamp) return s;
+
+        return null;
+    }
+
     @Override
     public void invalidateNewerThan(long timestamp)
     {
@@ -49,7 +67,7 @@ public class MemoryScoreList extends ScoreList
 
     @Override
     @NonNull
-    public List<Score> getAll()
+    public List<Score> toList()
     {
         computeAll();
         return new LinkedList<>(list);
@@ -57,28 +75,17 @@ public class MemoryScoreList extends ScoreList
 
     @Nullable
     @Override
-    public Score getByTimestamp(long timestamp)
-    {
-        computeAll();
-        for (Score s : list)
-            if (s.getTimestamp() == timestamp) return s;
-
-        return null;
-    }
-
-    @Override
-    public void add(List<Score> scores)
+    protected Score getNewestComputed()
     {
-        list.addAll(scores);
-        Collections.sort(list,
-            (s1, s2) -> Long.signum(s2.getTimestamp() - s1.getTimestamp()));
+        if (list.isEmpty()) return null;
+        return list.getFirst();
     }
 
     @Nullable
     @Override
-    protected Score getNewestComputed()
+    protected Score getOldestComputed()
     {
         if (list.isEmpty()) return null;
-        return list.get(0);
+        return list.getLast();
     }
 }

app/src/main/java/org/isoron/uhabits/models/sqlite/SQLiteCheckmarkList.java

@@ -84,7 +84,7 @@ public class SQLiteCheckmarkList extends CheckmarkList
     public List<Checkmark> getByInterval(long fromTimestamp, long toTimestamp)
     {
         check(habit.getId());
-        computeAll();
+        compute(fromTimestamp, toTimestamp);
 
         String query = "select habit, timestamp, value " +
                        "from checkmarks " +
@@ -119,7 +119,6 @@ public class SQLiteCheckmarkList extends CheckmarkList
     protected Checkmark getNewestComputed()
     {
         check(habit.getId());
-
         String query = "select habit, timestamp, value " +
                        "from checkmarks " +
                        "where habit = ? " +
@@ -127,24 +126,42 @@ public class SQLiteCheckmarkList extends CheckmarkList
                        "limit 1";
 
         String params[] = { Long.toString(habit.getId()) };
+        return getSingleCheckmarkFromQuery(query, params);
+    }
 
-        CheckmarkRecord record = sqlite.querySingle(query, params);
-        if (record == null) return null;
-        record.habit = habitRecord;
-        return record.toCheckmark();
+    @Override
+    protected Checkmark getOldestComputed()
+    {
+        check(habit.getId());
+        String query = "select habit, timestamp, value " +
+                       "from checkmarks " +
+                       "where habit = ? " +
+                       "order by timestamp asc " +
+                       "limit 1";
+
+        String params[] = { Long.toString(habit.getId()) };
+        return getSingleCheckmarkFromQuery(query, params);
     }
 
     @Contract("null -> fail")
     private void check(Long id)
     {
         if (id == null) throw new RuntimeException("habit is not saved");
-
         if (habitRecord != null) return;
 
         habitRecord = HabitRecord.get(id);
         if (habitRecord == null) throw new RuntimeException("habit not found");
     }
 
+    @Nullable
+    private Checkmark getSingleCheckmarkFromQuery(String query, String params[])
+    {
+        CheckmarkRecord record = sqlite.querySingle(query, params);
+        if (record == null) return null;
+        record.habit = habitRecord;
+        return record.toCheckmark();
+    }
+
     @NonNull
     private List<Checkmark> toCheckmarks(@NonNull List<CheckmarkRecord> records)
     {

app/src/main/java/org/isoron/uhabits/models/sqlite/SQLiteScoreList.java

@@ -84,46 +84,20 @@ public class SQLiteScoreList extends ScoreList
         }
     }
 
-    @Override
-    @NonNull
-    public List<Score> getAll()
-    {
-        check(habit.getId());
-        computeAll();
-
-        String query = "select habit, timestamp, score from Score " +
-                       "where habit = ? order by timestamp desc";
-
-        String params[] = {Long.toString(habit.getId())};
-
-        List<ScoreRecord> records = sqlite.query(query, params);
-        for (ScoreRecord record : records) record.habit = habitRecord;
-
-        List<Score> scores = new LinkedList<>();
-        for (ScoreRecord rec : records)
-            scores.add(rec.toScore());
-
-        return scores;
-    }
-
     @Override
     @Nullable
-    public Score getByTimestamp(long timestamp)
+    public Score getComputedByTimestamp(long timestamp)
     {
         check(habit.getId());
-        computeAll();
 
         String query = "select habit, timestamp, score from Score " +
                        "where habit = ? and timestamp = ? " +
                        "order by timestamp desc";
 
         String params[] =
-            {Long.toString(habit.getId()), Long.toString(timestamp)};
+            { Long.toString(habit.getId()), Long.toString(timestamp) };
 
-        ScoreRecord record = sqlite.querySingle(query, params);
-        if (record == null) return null;
-        record.habit = habitRecord;
-        return record.toScore();
+        return getScoreFromQuery(query, params);
     }
 
     @Override
@@ -138,6 +112,28 @@ public class SQLiteScoreList extends ScoreList
         getObservable().notifyListeners();
     }
 
+    @Override
+    @NonNull
+    public List<Score> toList()
+    {
+        check(habit.getId());
+        computeAll();
+
+        String query = "select habit, timestamp, score from Score " +
+                       "where habit = ? order by timestamp desc";
+
+        String params[] = { Long.toString(habit.getId()) };
+
+        List<ScoreRecord> records = sqlite.query(query, params);
+        for (ScoreRecord record : records) record.habit = habitRecord;
+
+        List<Score> scores = new LinkedList<>();
+        for (ScoreRecord rec : records)
+            scores.add(rec.toScore());
+
+        return scores;
+    }
+
     @Nullable
     @Override
     protected Score getNewestComputed()
@@ -147,22 +143,38 @@ public class SQLiteScoreList extends ScoreList
                        "where habit = ? order by timestamp desc " +
                        "limit 1";
 
-        String params[] = {Long.toString(habit.getId())};
+        String params[] = { Long.toString(habit.getId()) };
+        return getScoreFromQuery(query, params);
+    }
 
-        ScoreRecord record = sqlite.querySingle(query, params);
-        if (record == null) return null;
-        record.habit = habitRecord;
-        return record.toScore();
+    @Nullable
+    @Override
+    protected Score getOldestComputed()
+    {
+        check(habit.getId());
+        String query = "select habit, timestamp, score from Score " +
+                       "where habit = ? order by timestamp asc " +
+                       "limit 1";
+
+        String params[] = { Long.toString(habit.getId()) };
+        return getScoreFromQuery(query, params);
     }
 
     @Contract("null -> fail")
     private void check(Long id)
     {
         if (id == null) throw new RuntimeException("habit is not saved");
-
-        if(habitRecord != null) return;
-
+        if (habitRecord != null) return;
         habitRecord = HabitRecord.get(id);
         if (habitRecord == null) throw new RuntimeException("habit not found");
     }
+
+    @Nullable
+    private Score getScoreFromQuery(String query, String[] params)
+    {
+        ScoreRecord record = sqlite.querySingle(query, params);
+        if (record == null) return null;
+        record.habit = habitRecord;
+        return record.toScore();
+    }
 }

app/src/main/java/org/isoron/uhabits/widgets/ScoreWidget.java

@@ -69,7 +69,7 @@ public class ScoreWidget extends BaseWidget
         List<Score> scores;
         ScoreList scoreList = habit.getScores();
 
-        if (size == 1) scores = scoreList.getAll();
+        if (size == 1) scores = scoreList.toList();
         else scores = scoreList.groupBy(ScoreCard.getTruncateField(size));
 
         int color = ColorUtils.getColor(getContext(), habit.getColor());

app/src/test/java/org/isoron/uhabits/models/ScoreListTest.java

@@ -110,14 +110,17 @@ public class ScoreListTest extends BaseUnitTest
             3699107,
             2846927,
             1948077,
-            1000000
+            1000000,
+            0,
+            0,
+            0
         };
 
+        ScoreList scores = habit.getScores();
         long current = DateUtils.getStartOfToday();
         for (int expectedValue : expectedValues)
         {
-            assertThat(habit.getScores().getValue(current),
-                equalTo(expectedValue));
+            assertThat(scores.getValue(current), equalTo(expectedValue));
             current -= DateUtils.millisecondsInOneDay;
         }
     }