Repository: GothenburgBitFactory/timewarrior (cpp)

Issue:
CmdSummary does not show empty intervals at midnight
```text
$ timew track 2018-08-10 - 2018-08-10
  Recorded 
    Started 2018-08-10T00:00:00
    Ended                    00
    Total               0:00:00

$ timew summary :year :ids
  (shows an empty list)
```
[Question] Overlapping zero-width intervals
While working on a fix for #101 and a related issue I noticed that a zero-width interval `Z = [p, p)` is **not** considered to overlap an interval `[p, q)`, where `q ≥ p`.

Currently, the following intervals overlap, as they should:

```text
a  p  q
[-----)
   Z
```

However, these two intervals do not overlap, even though the first interval is half-closed.

```text
p     q
[-----)
Z
```

This behavior is easy to change and would allow for zero-width intervals to be stored at the beginning of a datafile segment. Currently, attempting to add a zero-width interval to the beginning of a segment results in an assertion failure because there is no overlap and therefore nothing to store in the datafile. (#101)

```text
    Jan        Feb
[---------)[---------)
           Z

=> overlap(Feb, Z) = false
=> Feb ∩ Z = [NULL, NULL)
```

However, this change would make it impossible for users to store multiple intervals with the same start time. For example:

```text
[---)                 # Unrelated interval A
               [---)  # Unrelated interval B
        Z             # Zero-width interval at point p
[------------------)  # Interval computed by Timewarrior for overlap checking
        [---)         # Interval the user attempts to add
```

In this case, the zero-width interval overlaps the computed interval, so it is used for overlap checking. The zero-width interval would then (with this change) overlap the new interval the user wanted to add, because both intervals cover point `p`, and Timewarrior would cancel the transaction.

Two tests currently rely on being able to start an interval `[p, ∞)` when there already is a zero-width interval `Z = [p, p)` in the database:

* [`TestClock.test_start_additional`](https://github.com/GothenburgBitFactory/timewarrior/blob/1.1.1/test/clock.t#L96)
* [`TestClock.test_start_subtract`](https://github.com/GothenburgBitFactory/timewarrior/blob/1.1.1/test/clock.t#L120)

Worse, this change would cause problems with existing databases. For this reason it is necessary to leave the overlap checking in [`validate.cpp`](https://github.com/GothenburgBitFactory/timewarrior/blob/1.1.1/src/validate.cpp#L106) unchanged.

Proposed solutions, in order of preference:

* Create a `lenientOverlap()` method with the current behavior for use during overlap checking, and change the current [`overlap()`](https://github.com/GothenburgBitFactory/timewarrior/blob/1.1.1/src/Range.cpp#L133) method to use the more consistent and logically correct behavior. This is the best solution in my opinion.
* Do the opposite, creating a `strictOverlap()` method instead.
* Make the `overlap()` method asymmetric, so that `A` may overlap `Z` without `Z` overlapping `A`. I find this solution counter-intuitive and would not recommend it.

Any thoughts? @lauft


Discussion:

After pondering this a while I think we have to separate whether we want to check
- if an interval *encloses* another interval or
- if an interval *overlaps with* another interval.

Your zero-width interval `Z` lies within the segment but the overlap (with other intervals) is zero. There already is the function `Range::encloses(const Range& other)` which serves this purpose. So can we use this to determine in which datafile the interval should be stored?

Thanks! `encloses()` is *almost* perfect; the only issue is that it doesn't handle intervals without an end time (i.e. `[p, ∞)`, where `socm ≤ p < eocm`, is not enclosed in any segment.)

While working on a patch I found issues #165 and #166, both closely related to this one. Unfortunately I think it may be necessary that we introduce one or two additional methods if we want to handle zero-width intervals correctly. I'll submit a pull request with more detailed explanations soon and would appreciate your feedback on those changes!

Patch:
diff --git a/src/Database.cpp b/src/Database.cpp
--- a/src/Database.cpp
+++ b/src/Database.cpp
@@ -351,7 +351,10 @@ std::vector <Range> Database::segmentRange (const Range& range)
 
     // Capture date after incrementing month.
     Datetime segmentEnd (start_y, start_m, 1);
-    segments.push_back (Range (segmentStart, segmentEnd));
+    auto segment = Range (segmentStart, segmentEnd);
+    if (range.intersects (segment)) {
+      segments.push_back (segment);
+    }
   }
 
   return segments;
diff --git a/src/Datafile.cpp b/src/Datafile.cpp
--- a/src/Datafile.cpp
+++ b/src/Datafile.cpp
@@ -90,7 +90,7 @@ std::vector <std::string> Datafile::allLines ()
 void Datafile::addInterval (const Interval& interval)
 {
   // Note: end date might be zero.
-  assert (_range.overlap (interval.range));
+  assert (_range.segmentContains (interval.range));
 
   if (! _lines_loaded)
     load_lines ();
@@ -108,7 +108,7 @@ void Datafile::addInterval (const Interval& interval)
 void Datafile::deleteInterval (const Interval& interval)
 {
   // Note: end date might be zero.
-  assert (_range.overlap (interval.range));
+  assert (_range.segmentContains (interval.range));
 
   if (! _lines_loaded)
     load_lines ();
diff --git a/src/Range.cpp b/src/Range.cpp
--- a/src/Range.cpp
+++ b/src/Range.cpp
@@ -179,6 +179,41 @@ bool Range::encloses (const Range& other) const
   return false;
 }
 
+
+////////////////////////////////////////////////////////////////////////////////
+// This is a standard enclosure check, except that the other range
+// need not have an end time, even if this one does.
+//
+// Detect the following enclosure cases:
+//
+// this                     [--------)
+//   A                        [----)
+//   B                          [...
+//
+// this                     [...
+//   A                        [----)
+//   B                           [--------)
+//   C                                    [--------)
+//   D                         [...
+//   E                                 [...
+//
+bool Range::segmentContains (const Range& other) const
+{
+  if (is_started ()) {
+    if (is_ended ()) {
+      if (other.is_started () && other.start >= start) {
+        return ! other.is_ended () || other.end <= end;
+      }
+    } else {
+      if (other.is_started () && other.start >= start) {
+        return true;
+      }
+    }
+  }
+
+  return false;
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // Calculate the following intersection cases:
 //
@@ -216,7 +251,7 @@ Range Range::intersect (const Range& other) const
     if (is_ended ())
     {
       if (other.is_ended ())
-        result.end  = end < other.end ? end : other.end;
+        result.end = end < other.end ? end : other.end;
       else
         result.end = end;
     }
@@ -229,9 +264,28 @@ Range Range::intersect (const Range& other) const
     return result;
   }
 
+  // If there is an intersection but no overlap, we have a zero-width
+  // interval [p, p) and another interval [p, q), where q >= p.
+  if (intersects (other)) {
+    return Range {start, start};
+  }
+
   return Range {};
 }
 
+////////////////////////////////////////////////////////////////////////////////
+bool Range::intersects (const Range &other) const
+{
+  if (overlap (other)) {
+    return true;
+  }
+
+  // A half-closed zero-width interval [p, p) may have the same
+  // starting point as another interval without overlapping it.
+  // We consider p to be an element of a range [p, p).
+  return (is_started () && other.is_started () && start == other.start);
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // If the ranges do not overlap, the result is *this.
 //
diff --git a/src/Range.h b/src/Range.h
--- a/src/Range.h
+++ b/src/Range.h
@@ -51,7 +51,9 @@ class Range
 
   bool overlap (const Range&) const;
   bool encloses (const Range&) const;
+  bool segmentContains (const Range&) const;
   Range intersect (const Range&) const;
+  bool intersects (const Range&) const;
   Range combine (const Range&) const;
   std::vector <Range> subtract (const Range&) const;
   time_t total () const;
diff --git a/src/data.cpp b/src/data.cpp
--- a/src/data.cpp
+++ b/src/data.cpp
@@ -343,9 +343,11 @@ std::vector <Range> subset (
   const std::vector <Range>& ranges)
 {
   std::vector <Range> all;
-  for (auto& r : ranges)
-    if (range.overlap (r))
+  for (auto& r : ranges) {
+    if (range.intersects (r)) {
       all.push_back (r);
+    }
+  }
 
   return all;
 }
@@ -356,9 +358,11 @@ std::vector <Interval> subset (
   const std::vector <Interval>& intervals)
 {
   std::vector <Interval> all;
-  for (auto& interval : intervals)
-    if (range.overlap (interval.range))
+  for (auto& interval : intervals) {
+    if (range.intersects (interval.range)) {
       all.push_back (interval);
+    }
+  }
 
   return all;
 }
@@ -549,12 +553,8 @@ Range outerRange (const std::vector <Interval>& intervals)
 bool matchesFilter (const Interval& interval, const Interval& filter)
 {
   if ((filter.range.start.toEpoch () == 0 &&
-       filter.range.end.toEpoch () == 0)
-
-      ||
-
-      ((interval.range.end.toEpoch () == 0 || interval.range.end   > filter.range.start) &&
-       (filter.range.end.toEpoch ()   == 0 || interval.range.start < filter.range.end)))
+       filter.range.end.toEpoch () == 0
+     ) || interval.range.intersects (filter.range))
   {
     for (auto& tag : filter.tags ())
       if (! interval.hasTag (tag))
@@ -608,7 +608,7 @@ std::vector <Interval> getTracked (
 
     if (! inclusions.empty ()) {
       auto latest = inclusions.back();
-      if (latest.range.is_open()) {;
+      if (latest.range.is_open()) {
         filter.range.end = 0;
       }
     }


Test patch:
diff --git a/test/resize.t b/test/resize.t
--- a/test/resize.t
+++ b/test/resize.t
@@ -53,6 +53,13 @@ class TestResize(TestCase):
         code, out, err = self.t.runError("resize @1 10mins")
         self.assertIn('Cannot resize open interval @1', err)
 
+    def test_resize_full_month_interval(self):
+        """Resize an interval to cover a full month"""
+        self.t("track now - now")
+        self.t("resize @1 1month")
+        code, out, err = self.t("resize @1 1month")
+        self.assertIn('Resized @1 to 720:00:00', out)
+
 
 if __name__ == "__main__":
     from simpletap import TAPTestRunner
diff --git a/test/summary.t b/test/summary.t
--- a/test/summary.t
+++ b/test/summary.t
@@ -193,6 +193,18 @@ W10 2017-03-11 Sat @3 FOO  10:00:00 11:00:00 1:00:00
                                                      6:00:00
 """, out)
 
+    def test_with_empty_interval_at_start_of_day(self):
+        """Summary should display empty intervals at midnight"""
+        self.t("track sod - sod")
+        code, out, err = self.t("summary :year")
+        self.assertRegexpMatches(out, """
+Wk  ?Date       Day Tags    ?Start      ?End    Time   Total
+[ -]+
+W\d{1,2} \d{4}-\d{2}-\d{2} .{3}       ?0:00:00 0:00:00 0:00:00 0:00:00
+
+[ ]+0:00:00
+""")
+
 
 if __name__ == "__main__":
     from simpletap import TAPTestRunner