core: fix undo of dive merging

When merging two dives, if a divesite is chosen that doesn't
have a GPS location, but another divesite has a GPS location,
then the GPS location of the former is set to that of the
latter.

However, that was done outside of the undo system, so that
it is not undone and the frontend is not made aware of the
change.

Fix this. To simplify things, move the code from the undo
machinery to the core.

Signed-off-by: Berthold Stoeger <bstoeger@mail.tuwien.ac.at>

commands/command_base.h

@@ -149,6 +149,12 @@ QVector<T> stdToQt(const std::vector<T> &v)
 #endif
 }
 
+template<typename T>
+std::vector<T> qtToStd(const QVector<T> &v)
+{
+	return std::vector<T>(v.begin(), v.end());
+}
+
 // We put everything in a namespace, so that we can shorten names without polluting the global namespace
 namespace Command {
 

commands/command_divelist.cpp

@@ -1,13 +1,14 @@
 // SPDX-License-Identifier: GPL-2.0
 
 #include "command_divelist.h"
+#include "core/divefilter.h"
 #include "core/divelist.h"
 #include "core/divelog.h"
 #include "core/qthelper.h"
 #include "core/selection.h"
 #include "core/subsurface-qt/divelistnotifier.h"
 #include "qt-models/filtermodels.h"
-#include "core/divefilter.h"
+#include "qt-models/divelocationmodel.h"
 
 namespace Command {
 
@@ -911,7 +912,7 @@ DeleteDiveComputer::DeleteDiveComputer(dive *d, int dc_num)
 	setText(Command::Base::tr("delete dive computer"));
 }
 
-MergeDives::MergeDives(const QVector <dive *> &dives)
+MergeDives::MergeDives(const QVector <dive *> &dives) : site(nullptr)
 {
 	setText(Command::Base::tr("merge dive"));
 
@@ -922,21 +923,7 @@ MergeDives::MergeDives(const QVector <dive *> &dives)
 		return;
 	}
 
-	auto [d, trip] = divelog.dives.merge_dives(*dives[0], *dives[1], dives[1]->when - dives[0]->when, false);
-
-	// Currently, the core code selects the dive -> this is not what we want, as
-	// we manually manage the selection post-command.
-	// TODO: Remove selection code from core.
-	d->selected = false;
-
-	// Set the preferred dive trip, so that for subsequent merges the better trip can be selected
-	d->divetrip = trip;
-	for (int i = 2; i < dives.count(); ++i) {
-		auto [d2, trip] = divelog.dives.merge_dives(*d, *dives[i], dives[i]->when - d->when, false);
-		d = std::move(d2);
-		// Set the preferred dive trip and site, so that for subsequent merges the better trip and site can be selected
-		d->divetrip = trip;
-	}
+	auto [d, set_location] = divelog.dives.merge_dives(qtToStd(dives));
 
 	// The merged dive gets the number of the first dive with a non-zero number
 	for (const dive *dive: dives) {
@@ -993,6 +980,11 @@ MergeDives::MergeDives(const QVector <dive *> &dives)
 	mergedDive.dives[0].site = d->dive_site;
 	divesToMerge.dives = std::vector<dive *>(dives.begin(), dives.end());
 
+	if (set_location.has_value() && d->dive_site) {
+		location = *set_location;
+		site = d->dive_site;
+	}
+
 	// We got our preferred trip and site, so now the references can be deleted from the newly generated dive
 	d->divetrip = nullptr;
 	d->dive_site = nullptr;
@@ -1005,11 +997,20 @@ bool MergeDives::workToBeDone()
 	return !mergedDive.dives.empty();
 }
 
+void MergeDives::swapDivesite()
+{
+	if (!site)
+		return;
+	std::swap(location, site->location);
+	emit diveListNotifier.diveSiteChanged(site, LocationInformationModel::LOCATION); // Inform frontend of changed dive site.
+}
+
 void MergeDives::redoit()
 {
 	renumberDives(divesToRenumber);
 	diveToUnmerge = addDives(mergedDive);
 	unmergedDives = removeDives(divesToMerge);
+	swapDivesite();
 
 	// Select merged dive and make it current
 	setSelection(diveToUnmerge.dives, diveToUnmerge.dives[0], -1);
@@ -1020,6 +1021,7 @@ void MergeDives::undoit()
 	divesToMerge = addDives(unmergedDives);
 	mergedDive = removeDives(diveToUnmerge);
 	renumberDives(divesToRenumber);
+	swapDivesite();
 
 	// Select unmerged dives and make first one current
 	setSelection(divesToMerge.dives, divesToMerge.dives[0], -1);

commands/command_divelist.h

@@ -267,6 +267,7 @@ private:
 	void undoit() override;
 	void redoit() override;
 	bool workToBeDone() override;
+	void swapDivesite(); // Common code for undo and redo.
 
 	// For redo
 	// Add one and remove a batch of dives
@@ -284,6 +285,8 @@ private:
 
 	// For undo and redo
 	QVector<QPair<dive *, int>> divesToRenumber;
+	dive_site		*site;
+	location_t		location;
 };
 
 } // namespace Command

core/dive.cpp

@@ -198,7 +198,7 @@ void dive::clear()
  * any impact on the source */
 void copy_dive(const struct dive *s, struct dive *d)
 {
-	/* simply copy things over, but then the dive cache. */
+	/* simply copy things over, but then clear the dive cache. */
 	*d = *s;
 	d->invalidate_cache();
 }

core/divelist.cpp

@@ -1092,13 +1092,13 @@ std::array<std::unique_ptr<dive>, 2> dive_table::split_dive_at_time(const struct
 /*
  * Pick a trip for a dive
  */
-static struct dive_trip *get_preferred_trip(const struct dive *a, const struct dive *b)
+static struct dive_trip *get_preferred_trip(const struct dive &a, const struct dive &b)
 {
 	dive_trip *atrip, *btrip;
 
 	/* If only one dive has a trip, choose that */
-	atrip = a->divetrip;
-	btrip = b->divetrip;
+	atrip = a.divetrip;
+	btrip = b.divetrip;
 	if (!atrip)
 		return btrip;
 	if (!btrip)
@@ -1124,7 +1124,7 @@ static struct dive_trip *get_preferred_trip(const struct dive *a, const struct d
 	 * Ok, so both have location and notes.
 	 * Pick the earlier one.
 	 */
-	if (a->when < b->when)
+	if (a.when < b.when)
 		return atrip;
 	return btrip;
 }
@@ -1152,38 +1152,63 @@ static struct dive_trip *get_preferred_trip(const struct dive *a, const struct d
  * dive. If the flag "prefer_downloaded" is set, data of the latter
  * will take priority over the former.
  *
- * The trip the new dive should be associated with (if any) is returned
- * in the "trip" output parameter.
+ * The dive site is set, but the caller still has to add it to the
+ * divelog's dive site manually.
  *
- * The dive site the new dive should be added to (if any) is returned
- * in the "dive_site" output parameter.
  */
-merge_result dive_table::merge_dives(const struct dive &a_in, const struct dive &b_in, int offset, bool prefer_downloaded) const
+std::unique_ptr<dive> dive_table::merge_two_dives(const struct dive &a_in, const struct dive &b_in, int offset, bool prefer_downloaded) const
 {
-	merge_result res = { };
-
 	const dive *a = &a_in;
 	const dive *b = &b_in;
 	if (is_dc_planner(&a->dcs[0]))
 		std::swap(a, b);
 
-	res.dive = dive::create_merged_dive(*a, *b, offset, prefer_downloaded);
+	auto d = dive::create_merged_dive(*a, *b, offset, prefer_downloaded);
 
 	/* The CNS values will be recalculated from the sample in fixup_dive() */
-	res.dive->cns = res.dive->maxcns = 0;
+	d->cns = d->maxcns = 0;
 
-	res.trip = get_preferred_trip(a, b);
+	/* Unselect the new dive if the original dive was selected. */
+	d->selected = false;
 
 	/* we take the first dive site, unless it's empty */
-	res.dive->dive_site = a->dive_site && !a->dive_site->is_empty() ? a->dive_site : b->dive_site;
-	if (res.dive->dive_site && !res.dive->dive_site->has_gps_location() && b->dive_site && b->dive_site->has_gps_location()) {
-		/* we picked the first dive site and that didn't have GPS data, but the new dive has
-		 * GPS data (that could be a download from a GPS enabled dive computer).
-		 * Keep the dive site, but add the GPS data */
-		res.dive->dive_site->location = b->dive_site->location;
-	}
-	fixup_dive(*res.dive);
+	d->dive_site = a->dive_site && !a->dive_site->is_empty() ? a->dive_site : b->dive_site;
 
+	fixup_dive(*d);
+
+	return d;
+}
+
+merge_result dive_table::merge_dives(const std::vector<dive *> &dives) const
+{
+	merge_result res;
+
+	// We don't support merging of less than two dives, but
+	// let's try to treat this gracefully.
+	if (dives.empty())
+		return res;
+	if (dives.size() == 1) {
+		res.dive = std::make_unique<dive>(*dives[0]);
+		return res;
+	}
+
+	auto d = merge_two_dives(*dives[0], *dives[1], dives[1]->when - dives[0]->when, false);
+	d->divetrip = get_preferred_trip(*dives[0], *dives[1]);
+
+	for (size_t i = 2; i < dives.size(); ++i) {
+		auto d2 = divelog.dives.merge_two_dives(*d, *dives[i], dives[i]->when - d->when, false);
+		d2->divetrip = get_preferred_trip(*d, *dives[i]);
+		d = std::move(d2);
+	}
+
+	// If the new dive site has no gps location, try to find the first dive with a gps location
+	if (d->dive_site && !d->dive_site->has_gps_location()) {
+		auto it = std::find_if(dives.begin(), dives.end(), [](const dive *d)
+				       { return d->dive_site && d->dive_site->has_gps_location(); } );
+		if (it != dives.end())
+			res.set_location = (*it)->dive_site->location;
+	}
+	res.dive = std::move(d);
 	return res;
 }
 
@@ -1205,8 +1230,7 @@ struct std::unique_ptr<dive> dive_table::try_to_merge(const struct dive &a, cons
 	if (!a.likely_same(b))
 		return {};
 
-	auto [res, trip] = merge_dives(a, b, 0, prefer_downloaded);
-	return std::move(res);
+	return merge_two_dives(a, b, 0, prefer_downloaded);
 }
 
 /* Clone a dive and delete given dive computer */

core/divelist.h

@@ -6,6 +6,7 @@
 #include "divesitetable.h"
 #include "units.h"
 #include <memory>
+#include <optional>
 
 struct dive;
 struct divelog;
@@ -17,7 +18,7 @@ int comp_dives_ptr(const struct dive *a, const struct dive *b);
 
 struct merge_result {
 	std::unique_ptr<struct dive> dive;
-	dive_trip *trip;
+	std::optional<location_t> set_location; // change location of dive site
 };
 
 struct dive_table : public sorted_owning_table<dive, &comp_dives> {
@@ -40,13 +41,14 @@ struct dive_table : public sorted_owning_table<dive, &comp_dives> {
 	std::array<std::unique_ptr<dive>, 2> split_divecomputer(const struct dive &src, int num) const;
 	std::array<std::unique_ptr<dive>, 2> split_dive(const struct dive &dive) const;
 	std::array<std::unique_ptr<dive>, 2> split_dive_at_time(const struct dive &dive, duration_t time) const;
-	merge_result merge_dives(const struct dive &a_in, const struct dive &b_in, int offset, bool prefer_downloaded) const;
+	merge_result merge_dives(const std::vector<dive *> &dives) const;
 	std::unique_ptr<dive> try_to_merge(const struct dive &a, const struct dive &b, bool prefer_downloaded) const;
 	bool has_dive(unsigned int deviceid, unsigned int diveid) const;
 	std::unique_ptr<dive> clone_delete_divecomputer(const struct dive &d, int dc_number);
 private:
 	int calculate_cns(struct dive &dive) const; // Note: writes into dive->cns
 	std::array<std::unique_ptr<dive>, 2> split_dive_at(const struct dive &dive, int a, int b) const;
+	std::unique_ptr<dive> merge_two_dives(const struct dive &a_in, const struct dive &b_in, int offset, bool prefer_downloaded) const;
 };
 
 void clear_dive_file_data();