Fix dive trip merging logic

We used to have very spotty logic for picking the dive trip when
merging two dives. It turns out that that spotty logic almost never
really matters, because in practice you'll never hit the situation of
merging two dives with different dive trips, but it *can* happen.

In particular, it happens when you use multiple dive computers, and
end up loading the dives from one computer on top of the dives of your
other computer. If the clocks of the dive computers was set
sufficiently close to each other, the dive merging logic will kick in
and you may now have slightly different times for the dives that get
merged, and the trip merging logic got *really* confused.

The trip management also depends on the trip dates being updated
correctly when the dives associated with a trip are updated (whether
added or removed), and the trip merging code did none of that.

This fixes it all up. Hopefully correctly.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Dirk Hohndel <dirk@hohndel.org>

4 files changed, 106 insertions, 19 deletions

diff --git a/dive.c b/dive.c
index c855acef8..ca6fda649 100644
--- a/dive.c
+++ b/dive.c
@@ -713,11 +713,94 @@ static void merge_equipment(struct dive *res, struct dive *a, struct dive *b)
 }
 
 /*
+ * When merging two dives, this picks the trip from one, and removes it
+ * from the other.
+ *
+ * The 'next' dive is not involved in the dive merging, but is the dive
+ * that will be the next dive after the merged dive.
+ */
+static void pick_and_delete_trip(struct dive *res, struct dive *pick, struct dive *remove, struct dive *next)
+{
+	tripflag_t tripflag = pick->tripflag;
+	dive_trip_t *trip = pick->divetrip;
+
+	res->tripflag = tripflag;
+	res->divetrip = trip;
+
+	/*
+	 * We may have to change the trip date if we picked an earlier
+	 * date for the dive that now uses it.
+	 */
+	if (res->when < trip->when)
+		trip->when = res->when;
+
+	/* Was it the same trip as the removed dive? All good*/
+	if (trip == remove->divetrip)
+		return;
+
+	/* Ok, we're dropping a dive. We may need to fix up the date on it */
+	trip = remove->divetrip;
+	if (trip->when != remove->when)
+		return;
+
+	if (next && next->divetrip == trip) {
+		trip->when = next->when;
+		return;
+	}
+
+	delete_trip(trip);
+}
+
+/*
+ * Pick a trip for a dive
+ */
+static void merge_trip(struct dive *res, struct dive *a, struct dive *b, struct dive *next)
+{
+	/*
+	 * The larger tripflag is more relevant: we prefer
+	 * take manually assigned trips over auto-generated
+	 * ones.
+	 */
+	if (a->tripflag > b->tripflag)
+		goto pick_a;
+
+	if (a->tripflag < b->tripflag)
+		goto pick_b;
+
+	/*
+	 * Ok, so the divetrips are equally "important".
+	 * Pick the one with the better description.
+	 */
+	if (!a->location)
+		goto pick_b;
+	if (!b->location)
+		goto pick_a;
+	if (!a->notes)
+		goto pick_b;
+	if (!b->notes)
+		goto pick_a;
+
+	/*
+	 * Ok, so both have location and notes.
+	 * Pick the earlier one.
+	 */
+	if (a->when < b->when)
+		goto pick_a;
+	goto pick_b;
+
+pick_a:
+	pick_and_delete_trip(res, a, b, next);
+	return;
+pick_b:
+	pick_and_delete_trip(res, b, a, next);
+}
+
+/*
  * This could do a lot more merging. Right now it really only
  * merges almost exact duplicates - something that happens easily
  * with overlapping dive downloads.
  */
-struct dive *try_to_merge(struct dive *a, struct dive *b)
+struct dive *try_to_merge(struct dive *a, struct dive *b, struct dive *next)
 {
 	struct dive *res;
 
@@ -727,14 +810,7 @@ struct dive *try_to_merge(struct dive *a, struct dive *b)
 	res = alloc_dive();
 
 	res->when = a->when;
-	/* the larger tripflag is more relevant */
-	if(a->tripflag > b->tripflag) {
-		res->tripflag = a->tripflag;
-		res->divetrip = a->divetrip;
-	} else {
-		res->tripflag = b->tripflag;
-		res->divetrip = b->divetrip;
-	}
+	merge_trip(res, a, b, next);
 	MERGE_NONZERO(res, a, b, latitude);
 	MERGE_NONZERO(res, a, b, longitude);
 	MERGE_TXT(res, a, b, location);
diff --git a/dive.h b/dive.h
index d9f587a79..a1520a1a6 100644
--- a/dive.h
+++ b/dive.h
@@ -242,7 +242,7 @@ struct event {
 #define W_IDX_SECONDARY 1
 
 typedef gint64 timestamp_t;
-typedef enum { TF_NONE, NO_TRIP, IN_TRIP, ASSIGNED_TRIP, AUTOGEN_TRIP, NUM_TRIPFLAGS } tripflag_t;
+typedef enum { TF_NONE, NO_TRIP, IN_TRIP, AUTOGEN_TRIP, ASSIGNED_TRIP, NUM_TRIPFLAGS } tripflag_t;
 extern const char *tripflag_names[NUM_TRIPFLAGS];
 
 typedef struct dive_trip {
@@ -298,6 +298,7 @@ extern gboolean autogroup;
 #define DIVE_FITS_TRIP(_dive, _dive_trip) ((_dive_trip)->when - TRIP_THRESHOLD <= (_dive)->when)
 
 extern void insert_trip(dive_trip_t **trip);
+extern void delete_trip(dive_trip_t *trip);
 
 /*
  * We keep our internal data in well-specified units, but
@@ -390,7 +391,7 @@ extern void finish_sample(struct dive *dive);
 
 extern void report_dives(gboolean imported);
 extern struct dive *fixup_dive(struct dive *dive);
-extern struct dive *try_to_merge(struct dive *a, struct dive *b);
+extern struct dive *try_to_merge(struct dive *a, struct dive *b, struct dive *next);
 
 extern void renumber_dives(int nr);
 
diff --git a/divelist.c b/divelist.c
index 498210e1f..8819f4ab1 100644
--- a/divelist.c
+++ b/divelist.c
@@ -1025,7 +1025,7 @@ void insert_trip(dive_trip_t **dive_trip_p)
 #endif
 }
 
-static inline void delete_trip(GList *trip)
+static inline void delete_trip_list_entry(GList *trip)
 {
 	dive_trip_t *dive_trip = (dive_trip_t *)g_list_nth_data(trip, 0);
 	if (dive_trip->location)
@@ -1035,6 +1035,12 @@ static inline void delete_trip(GList *trip)
 	dump_trip_list();
 #endif
 }
+
+void delete_trip(dive_trip_t *trip)
+{
+	delete_trip_list_entry(find_trip(trip->when));
+}
+
 static dive_trip_t *create_and_hookup_trip_from_dive(struct dive *dive)
 {
 	dive_trip_t *dive_trip = calloc(sizeof(dive_trip_t),1);
@@ -1693,7 +1699,7 @@ static void remove_from_trip(GtkTreePath *path)
 	/* if this was the last dive on the trip, remove the trip */
 	if (! gtk_tree_model_iter_has_child(MODEL(dive_list), &parent)) {
 		gtk_tree_store_remove(STORE(dive_list), &parent);
-		delete_trip(find_trip(dive->divetrip->when));
+		delete_trip(dive->divetrip);
 		free(dive->divetrip);
 	}
 	/* mark the dive as intentionally at the top level */
@@ -1803,7 +1809,7 @@ void remove_trip(GtkTreePath *trippath, gboolean force_no_trip)
 	}
 	/* finally, remove the trip */
 	gtk_tree_store_remove(STORE(dive_list), &parent);
-	delete_trip(find_trip(dive_trip->when));
+	delete_trip(dive_trip);
 	free(dive_trip);
 #ifdef DEBUG_TRIP
 	dump_trip_list();
@@ -1858,7 +1864,7 @@ void merge_trips_cb(GtkWidget *menuitem, GtkTreePath *trippath)
 	}
 	update_trip_timestamp(&prevtripiter, DIVE_TRIP(prevtrip));
 	free(DIVE_TRIP(trip));
-	delete_trip(trip);
+	delete_trip_list_entry(trip);
 	gtk_tree_store_remove(STORE(dive_list), &thistripiter);
 	mark_divelist_changed(TRUE);
 }
@@ -1975,7 +1981,7 @@ static void delete_selected_dives_cb(GtkWidget *menuitem, GtkTreePath *path)
 				if (dive->divetrip == divetrip_to_update)
 					divetrip_to_update->when = dive->when;
 				else
-					delete_trip(find_trip(divetrip_to_update->when));
+					delete_trip(divetrip_to_update);
 			}
 			continue;
 		}
@@ -1984,7 +1990,7 @@ static void delete_selected_dives_cb(GtkWidget *menuitem, GtkTreePath *path)
 		 * that needs to be updated, check if this dive is still in that trip;
 		 * if not, delete the trip */
 		if (divetrip_needs_update && dive->divetrip != divetrip_to_update) {
-			delete_trip(find_trip(divetrip_to_update->when));
+			delete_trip(divetrip_to_update);
 			divetrip_needs_update = FALSE;
 		}
 		/* if this dive is part of a divetrip and is the first dive that
@@ -2044,7 +2050,7 @@ static void delete_dive_cb(GtkWidget *menuitem, GtkTreePath *path)
 	if (dive->divetrip && dive->when == dive->divetrip->when) {
 		struct dive *next_dive = get_dive(idx + 1);
 		if (!next_dive || next_dive->divetrip != dive->divetrip)
-			delete_trip(find_trip(dive->when));
+			delete_trip(dive->divetrip);
 		else
 			next_dive->divetrip->when = next_dive->when;
 	}
diff --git a/main.c b/main.c
index ca3722383..ff874834c 100644
--- a/main.c
+++ b/main.c
@@ -126,12 +126,16 @@ void report_dives(gboolean is_imported)
 		struct dive **pp = &dive_table.dives[i-1];
 		struct dive *prev = pp[0];
 		struct dive *dive = pp[1];
+		struct dive *next;
 		struct dive *merged;
 
 		if (prev->when + prev->duration.seconds < dive->when)
 			continue;
 
-		merged = try_to_merge(prev, dive);
+		next = NULL;
+		if (i < dive_table.nr-1)
+			next = pp[2];
+		merged = try_to_merge(prev, dive, next);
 		if (!merged)
 			continue;