Be much more careful about merging dives

This patch changes the dive merging to be much more careful about
things, because it turns out that we had several small oddities that
caused big merge issues.

The oddities are:

 - the dive "duration" is actually how long we spend under water.

   But that means that when we do "dive->when + dive.duration.seconds"
   to calculate the end of the dive, that is nonsensical if you came up
   to the surface in the middle of a dive.

   Now, normally you don't see profiles like that, but once you start
   merging dives together, it can go from "small detail" to "dominant
   factor".

 - We have two different cases of merging: the automatic "merge new dive
   computer download if it looks like the same dive" (which always has a
   merge offset of 0, since we merge it as a new dive computer) and the
   "merge two different dives into one longer dive.

   The code assumed that it could look at the "downloaded" flag for the
   dive to check one or the other, but that doesn't really work.
   Reading a dive from an XML file isn't any different from downloading
   it.

   So we need to change the logic to determine what kind of merge it is
   to actually check the passed-in time offset.

With this, Stuart Vernon's test-case of eight dives with short surface
intervals in between end up merging correctly into one dive.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Reported-by: Stuart Vernon <stuartv@force2.net>
Signed-off-by: Dirk Hohndel <dirk@hohndel.org>

1 files changed, 94 insertions, 20 deletions

diff --git a/dive.c b/dive.c
index 10ce63859..0248888a3 100644
--- a/dive.c
+++ b/dive.c
@@ -2743,28 +2743,48 @@ int count_dives_with_suit(const char *suit)
 	return counter;
 }
 
-
+/*
+ * Merging two dives can be subtle, because there's two different ways
+ * of merging:
+ *
+ * (a) two distinctly _different_ dives that have the same dive computer
+ *     are merged into one longer dive, because the user asked for it
+ *     in the divelist.
+ *
+ *     Because this case is with teh same dive computer, we *know* the
+ *     two must have a different start time, and "offset" is the relative
+ *     time difference between the two.
+ *
+ * (a) two different dive computers that we migth want to merge into
+ *     one single dive with multiple dive computers.
+ *
+ *     This is the "try_to_merge()" case, which will have offset == 0,
+ *     even if the dive times migth be different.
+ */
 struct dive *merge_dives(struct dive *a, struct dive *b, int offset, bool prefer_downloaded)
 {
 	struct dive *res = alloc_dive();
 	struct dive *dl = NULL;
 
-	/* Aim for newly downloaded dives to be 'b' (keep old dive data first) */
-	if (a->downloaded && !b->downloaded) {
-		struct dive *tmp = a;
-		a = b;
-		b = tmp;
+	if (offset) {
+		/*
+		 * If "likely_same_dive()" returns true, that means that
+		 * it is *not* the same dive computer, and we do not want
+		 * to try to turn it into a single longer dive. So we'd
+		 * join them as two separate dive computers at zero offset.
+		 */
+		if (likely_same_dive(a, b))
+			offset = 0;
+	} else {
+		/* Aim for newly downloaded dives to be 'b' (keep old dive data first) */
+		if (a->downloaded && !b->downloaded) {
+			struct dive *tmp = a;
+			a = b;
+			b = tmp;
+		}
+		if (prefer_downloaded && b->downloaded)
+			dl = b;
 	}
-	if (prefer_downloaded && b->downloaded)
-		dl = b;
-
-	/*
-	 * Did the user ask us to merge dives in the dive list?
-	 * We may want to just join the dive computers, not try to
-	 * interleave them at some offset.
-	 */
-	if (offset && likely_same_dive(a, b))
-		offset = 0;
 
 	res->when = dl ? dl->when : a->when;
 	res->selected = a->selected || b->selected;
@@ -2793,6 +2813,56 @@ struct dive *merge_dives(struct dive *a, struct dive *b, int offset, bool prefer
 	return res;
 }
 
+/*
+ * "dc_maxtime()" is how much total time this dive computer
+ * has for this dive. Note that it can differ from "duration"
+ * if there are surface events in the middle.
+ *
+ * Still, we do ignore all but the last surface samples from the
+ * end, because some divecomputers just generate lots of them.
+ */
+static inline int dc_totaltime(const struct divecomputer *dc)
+{
+	int time = dc->duration.seconds;
+	int nr = dc->samples;
+
+	while (nr--) {
+		struct sample *s = dc->sample + nr;
+		time = s->time.seconds;
+		if (s->depth.mm >= SURFACE_THRESHOLD)
+			break;
+	}
+	return time;
+}
+
+/*
+ * The end of a dive is actually not trivial, because "duration"
+ * is not the duration until the end, but the time we spend under
+ * water, which can be very different if there are surface events
+ * during the dive.
+ *
+ * So walk the dive computers, looking for the longest actual
+ * time in the samples (and just default to the dive duration if
+ * there are no samples).
+ */
+static inline int dive_totaltime(const struct dive *dive)
+{
+	int time =  dive->duration.seconds;
+	const struct divecomputer *dc;
+
+	for_each_dc(dive, dc) {
+		int dc_time = dc_totaltime(dc);
+		if (dc_time > time)
+			time = dc_time;
+	}
+	return time;
+}
+
+timestamp_t dive_endtime(const struct dive *dive)
+{
+	return dive->when + dive_totaltime(dive);
+}
+
 struct dive *find_dive_including(timestamp_t when)
 {
 	int i;
@@ -2802,7 +2872,7 @@ struct dive *find_dive_including(timestamp_t when)
 	 * also we always use the duration from the first divecomputer
 	 *     could this ever be a problem? */
 	for_each_dive (i, dive) {
-		if (dive->when <= when && when <= dive->when + dive->duration.seconds)
+		if (dive->when <= when && when <= dive_endtime(dive))
 			return dive;
 	}
 	return NULL;
@@ -2810,12 +2880,16 @@ struct dive *find_dive_including(timestamp_t when)
 
 bool time_during_dive_with_offset(struct dive *dive, timestamp_t when, timestamp_t offset)
 {
-	return dive->when - offset <= when && when <= dive->when + dive->duration.seconds + offset;
+	timestamp_t start = dive->when;
+	timestamp_t end = dive_endtime(dive);
+	return start - offset <= when && when <= end + offset;
 }
 
 bool dive_within_time_range(struct dive *dive, timestamp_t when, timestamp_t offset)
 {
-	return when - offset <= dive->when && dive->when + dive->duration.seconds <= when + offset;
+	timestamp_t start = dive->when;
+	timestamp_t end = dive_endtime(dive);
+	return when - offset <= start && end <= when + offset;
 }
 
 /* find the n-th dive that is part of a group of dives within the offset around 'when'.
@@ -2970,7 +3044,7 @@ bool dive_check_picture_time(struct dive *d, int shift_time, timestamp_t timesta
 	offset_t offset;
 	if (timestamp) {
 		offset.seconds = timestamp - d->when + shift_time;
-		if (offset.seconds > -D30MIN && offset.seconds < (int)d->duration.seconds + D30MIN) {
+		if (offset.seconds > -D30MIN && offset.seconds < dive_totaltime(d) + D30MIN) {
 			// this picture belongs to this dive
 			return true;
 		}