Undo: implement rudimentary support for undo of dive-merging

For this, an output-parameter was added to the backend merge_dives()
function. When non-zero, instead of adding the merged dive to
the preferred trip, the preferred trip is returned to the caller.

Since the new UndoObject, just like the delete-dives UndoObject,
needs to remove/readd a set of dives, the corresponding functionality
was split-off in a helper function.

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

core/dive.c

@@ -2459,7 +2459,7 @@ static void pick_trip(struct dive *res, const struct dive *pick)
 /*
  * Pick a trip for a dive
  */
-static void merge_trip(struct dive *res, struct dive *a, struct dive *b)
+static struct dive *get_preferred_trip(struct dive *a, struct dive *b)
 {
 	dive_trip_t *atrip, *btrip;
 
@@ -2469,39 +2469,34 @@ static void merge_trip(struct dive *res, struct dive *a, struct dive *b)
 	 * ones.
 	 */
 	if (a->tripflag > b->tripflag)
-		goto pick_a;
+		return a;
 
 	if (a->tripflag < b->tripflag)
-		goto pick_b;
+		return b;
 
 	/* Otherwise, look at the trip data and pick the "better" one */
 	atrip = a->divetrip;
 	btrip = b->divetrip;
 	if (!atrip)
-		goto pick_b;
+		return b;
 	if (!btrip)
-		goto pick_a;
+		return a;
 	if (!atrip->location)
-		goto pick_b;
+		return b;
 	if (!btrip->location)
-		goto pick_a;
+		return a;
 	if (!atrip->notes)
-		goto pick_b;
+		return b;
 	if (!btrip->notes)
-		goto pick_a;
+		return 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:
-	b = a;
-pick_b:
-	pick_trip(res, b);
+		return a;
+	return b;
 }
 
 #if CURRENTLY_NOT_USED
@@ -2844,7 +2839,7 @@ static int likely_same_dive(const struct dive *a, const struct dive *b)
 struct dive *try_to_merge(struct dive *a, struct dive *b, bool prefer_downloaded)
 {
 	if (likely_same_dive(a, b))
-		return merge_dives(a, b, 0, prefer_downloaded);
+		return merge_dives(a, b, 0, prefer_downloaded, NULL);
 	return NULL;
 }
 
@@ -3335,10 +3330,15 @@ int count_dives_with_suit(const char *suit)
  * be the old dive and dive b is supposed to be the newly imported
  * 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 paramater. If "trip" is NULL, then the dive will
+ * instead be added to this trip.
  */
-struct dive *merge_dives(struct dive *a, struct dive *b, int offset, bool prefer_downloaded)
+struct dive *merge_dives(struct dive *a, struct dive *b, int offset, bool prefer_downloaded, struct dive_trip **trip)
 {
 	struct dive *res = alloc_dive();
+	struct dive *preferred_trip;
 
 	if (offset) {
 		/*
@@ -3358,7 +3358,11 @@ struct dive *merge_dives(struct dive *a, struct dive *b, int offset, bool prefer
 	}
 	res->when = prefer_downloaded ? b->when : a->when;
 	res->selected = a->selected || b->selected;
-	merge_trip(res, a, b);
+	preferred_trip = get_preferred_trip(a, b);
+	if (trip)
+		*trip = preferred_trip->divetrip;
+	else
+		pick_trip(res, preferred_trip);
 	MERGE_TXT(res, a, b, notes, "\n--\n");
 	MERGE_TXT(res, a, b, buddy, ", ");
 	MERGE_TXT(res, a, b, divemaster, ", ");

core/dive.h

@@ -573,7 +573,7 @@ extern int split_dive_dont_insert(const struct dive *dive, struct dive **new1, s
 extern void split_dive(struct dive *);
 extern int split_dive_at_time_dont_insert(const struct dive *dive, duration_t time, struct dive **new1, struct dive **new2);
 extern void split_dive_at_time(struct dive *dive, duration_t time);
-extern struct dive *merge_dives(struct dive *a, struct dive *b, int offset, bool prefer_downloaded);
+extern struct dive *merge_dives(struct dive *a, struct dive *b, int offset, bool prefer_downloaded, struct dive_trip **trip);
 extern struct dive *try_to_merge(struct dive *a, struct dive *b, bool prefer_downloaded);
 extern struct event *clone_event(const struct event *src_ev);
 extern void copy_events(const struct divecomputer *s, struct divecomputer *d);

core/divelist.c

@@ -26,7 +26,7 @@
  * struct dive *unregister_dive(int idx)
  * void delete_single_dive(int idx)
  * void add_single_dive(int idx, struct dive *dive)
- * void merge_two_dives(struct dive *a, struct dive *b)
+ * struct dive *merge_two_dives(struct dive *a, struct dive *b)
  * void select_dive(int idx)
  * void deselect_dive(int idx)
  * void mark_divelist_changed(int changed)
@@ -1060,7 +1060,7 @@ struct dive *merge_two_dives(struct dive *a, struct dive *b)
 	if (i < 0 || j < 0)
 		// something is wrong with those dives. Bail
 		return NULL;
-	res = merge_dives(a, b, b->when - a->when, false);
+	res = merge_dives(a, b, b->when - a->when, false, NULL);
 	if (!res)
 		return NULL;
 

desktop-widgets/divelistview.cpp

@@ -611,21 +611,32 @@ static bool can_merge(const struct dive *a, const struct dive *b, enum asked_use
 void DiveListView::mergeDives()
 {
 	int i;
-	struct dive *dive, *maindive = NULL;
+	struct dive *d;
 	enum asked_user have_asked = NOTYET;
 
-	for_each_dive (i, dive) {
-		if (dive->selected) {
-			if (!can_merge(maindive, dive, &have_asked)) {
-				maindive = dive;
-			} else {
-				maindive = merge_two_dives(maindive, dive);
-				i--; // otherwise we skip a dive in the freshly changed list
-			}
+	// Collect a vector of batches of dives to merge (i.e. a vector of vector of dives)
+	QVector<QVector<dive *>> merge_batches;
+	QVector<dive *> current_batch;
+	for_each_dive (i, d) {
+		if (!d->selected)
+			continue;
+		if (current_batch.empty()) {
+			current_batch.append(d);
+		} else if (can_merge(current_batch.back(), d, &have_asked)) {
+			current_batch.append(d);
+		} else {
+			if (current_batch.count() > 1)
+				merge_batches.append(current_batch);
+			current_batch.clear();
 		}
 	}
-	MainWindow::instance()->refreshProfile();
-	MainWindow::instance()->refreshDisplay();
+	if (current_batch.count() > 1)
+		merge_batches.append(current_batch);
+
+	for (const QVector<dive *> &batch: merge_batches) {
+		UndoMergeDives *undoCommand = new UndoMergeDives(batch);
+		MainWindow::instance()->undoStack->push(undoCommand);
+	}
 }
 
 void DiveListView::splitDives()

desktop-widgets/tab-widgets/maintab.cpp

@@ -809,9 +809,8 @@ void MainTab::acceptChanges()
 		MainWindow::instance()->dive_list()->verticalScrollBar()->setSliderPosition(scrolledBy);
 		MainWindow::instance()->dive_list()->setFocus();
 		resetPallete();
-		saveTags(QVector<dive *>{ &displayed_dive });
+		saveTags();
 		displayed_dive.divetrip = nullptr; // Should not be necessary, just in case!
-		Command::addDive(&displayed_dive, autogroup, true);
 		return;
 	} else if (MainWindow::instance() && MainWindow::instance()->dive_list()->selectedTrips().count() == 1) {
 		/* now figure out if things have changed */

desktop-widgets/undocommands.cpp

@@ -44,6 +44,50 @@ static dive *addDive(DiveToAdd &d)
 	return res;
 }
 
+// This helper function calls removeDive() on a list of dives to be removed and
+// returns a vector of corresponding DiveToAdd objects, which can later be readded.
+// The passed in vector is cleared.
+static std::vector<DiveToAdd> removeDives(std::vector<dive *> &divesToDelete)
+{
+	std::vector<DiveToAdd> res;
+	res.reserve(divesToDelete.size());
+
+	for (dive *d: divesToDelete)
+		res.push_back(removeDive(d));
+	divesToDelete.clear();
+
+	return res;
+}
+
+// This helper function is the counterpart fo removeDives(): it calls addDive() on a list
+// of dives to be (re)added and returns a vector of the added dives. It does this in reverse
+// order, so that trips are created appropriately and indexing is correct.
+// The passed in vector is cleared.
+static std::vector<dive *> addDives(std::vector<DiveToAdd> &divesToAdd)
+{
+	std::vector<dive *> res;
+	res.reserve(divesToAdd.size());
+
+	for (auto it = divesToAdd.rbegin(); it != divesToAdd.rend(); ++it)
+		res.push_back(addDive(*it));
+	divesToAdd.clear();
+
+	return res;
+}
+
+// This helper function renumbers dives according to an array of id/number pairs.
+// The old numbers are stored in the array, thus calling this function twice has no effect.
+// TODO: switch from uniq-id to indexes once all divelist-actions are controlled by "UndoCommands".
+static void renumberDives(QVector<QPair<int, int>> &divesToRenumber)
+{
+	for (auto &pair: divesToRenumber) {
+		dive *d = get_dive_by_uniq_id(pair.first);
+		if (!d)
+			continue;
+		std::swap(d->number, pair.second);
+	}
+}
+
 UndoAddDive::UndoAddDive(dive *d)
 {
 	setText(gettextFromC::tr("add dive"));
@@ -78,18 +122,16 @@ void UndoAddDive::undo()
 	MainWindow::instance()->refreshDisplay();
 }
 
-UndoDeleteDive::UndoDeleteDive(const QVector<struct dive*> &divesToDeleteIn) : divesToDelete(divesToDeleteIn)
+UndoDeleteDive::UndoDeleteDive(const QVector<struct dive*> &divesToDeleteIn) : divesToDelete(divesToDeleteIn.toStdVector())
 {
 	setText(tr("delete %n dive(s)", "", divesToDelete.size()));
 }
 
 void UndoDeleteDive::undo()
 {
-	for (auto it = divesToAdd.rbegin(); it != divesToAdd.rend(); ++it)
-		divesToDelete.append(addDive(*it));
+	divesToDelete = addDives(divesToAdd);
 
 	mark_divelist_changed(true);
-	divesToAdd.clear();
 
 	// Finally, do the UI stuff:
 	MainWindow::instance()->refreshDisplay();
@@ -97,10 +139,7 @@ void UndoDeleteDive::undo()
 
 void UndoDeleteDive::redo()
 {
-	for (dive *d: divesToDelete)
-		divesToAdd.push_back(removeDive(d));
-
-	divesToDelete.clear();
+	divesToAdd = removeDives(divesToDelete);
 	mark_divelist_changed(true);
 
 	// Finally, do the UI stuff:
@@ -145,12 +184,7 @@ UndoRenumberDives::UndoRenumberDives(const QVector<QPair<int, int>> &divesToRenu
 
 void UndoRenumberDives::undo()
 {
-	for (auto &pair: divesToRenumber) {
-		dive *d = get_dive_by_uniq_id(pair.first);
-		if (!d)
-			continue;
-		std::swap(d->number, pair.second);
-	}
+	renumberDives(divesToRenumber);
 	mark_divelist_changed(true);
 
 	// Finally, do the UI stuff:
@@ -260,3 +294,104 @@ void UndoSplitDives::undo()
 	MainWindow::instance()->refreshDisplay();
 	MainWindow::instance()->refreshProfile();
 }
+
+UndoMergeDives::UndoMergeDives(const QVector <dive *> &dives)
+{
+	setText(gettextFromC::tr("merge dive"));
+
+	// We start in redo mode
+	diveToUnmerge = nullptr;
+
+	// Just a safety check - if there's not two or more dives - do nothing
+	// The caller should have made sure that this doesn't happen.
+	if (dives.count() < 2) {
+		qWarning() << "Merging less than two dives";
+		return;
+	}
+
+	dive_trip *preferred_trip;
+	OwningDivePtr d(merge_dives(dives[0], dives[1], dives[1]->when - dives[0]->when, false, &preferred_trip));
+
+	// Set the preferred dive trip, so that for subsequent merges the better trip can be selected
+	d->divetrip = preferred_trip;
+	for (int i = 2; i < dives.count(); ++i) {
+		d.reset(merge_dives(d.get(), dives[i], dives[i]->when - d->when, false, &preferred_trip));
+		// Set the preferred dive trip, so that for subsequent merges the better trip can be selected
+		d->divetrip = preferred_trip;
+	}
+
+	// We got our preferred trip, so now the reference can be deleted from the newly generated dive
+	d->divetrip = nullptr;
+
+	// The merged dive gets the number of the first dive
+	d->number = dives[0]->number;
+
+	// We will only renumber the remaining dives if the joined dives are consecutive.
+	// Otherwise all bets are off concerning what the user wanted and doing nothing seems
+	// like the best option.
+	int idx = get_divenr(dives[0]);
+	int num = dives.count();
+	if (idx < 0 || idx + num > dive_table.nr) {
+		// It was the callers responsibility to pass only known dives.
+		// Something is seriously wrong - give up.
+		qWarning() << "Merging unknown dives";
+		return;
+	}
+	// std::equal compares two ranges. The parameters are (begin_range1, end_range1, begin_range2).
+	// Here, we can compare C-arrays, because QVector guarantees contiguous storage.
+	if (std::equal(&dives[0], &dives[0] + num, &dive_table.dives[idx]) &&
+	    dives[0]->number && dives.last()->number && dives[0]->number < dives.last()->number) {
+		// We have a consecutive set of dives. Rename all following dives according to the
+		// number of erased dives. This considers that there might be missing numbers.
+		// Comment copied from core/divelist.c:
+		// So if you had a dive list  1 3 6 7 8, and you
+		// merge 1 and 3, the resulting numbered list will
+		// be 1 4 5 6, because we assume that there were
+		// some missing dives (originally dives 4 and 5),
+		// that now will still be missing (dives 2 and 3
+		// in the renumbered world).
+		//
+		// Obviously the normal case is that everything is
+		// consecutive, and the difference will be 1, so the
+		// above example is not supposed to be normal.
+		int diff = dives.last()->number - dives[0]->number;
+		divesToRenumber.reserve(dive_table.nr - idx - num);
+		int previousnr = dives[0]->number;
+		for (int i = idx + num; i < dive_table.nr; ++i) {
+			int newnr = dive_table.dives[i]->number - diff;
+
+			// Stop renumbering if stuff isn't in order (see also core/divelist.c)
+			if (newnr <= previousnr)
+				break;
+			divesToRenumber.append(QPair<int,int>(dive_table.dives[i]->id, newnr));
+			previousnr = newnr;
+		}
+	}
+
+	mergedDive.dive = std::move(d);
+	mergedDive.idx = get_divenr(dives[0]);
+	mergedDive.trip = preferred_trip;
+	divesToMerge = dives.toStdVector();
+}
+
+void UndoMergeDives::redo()
+{
+	renumberDives(divesToRenumber);
+	diveToUnmerge = addDive(mergedDive);
+	unmergedDives = removeDives(divesToMerge);
+
+	// Finally, do the UI stuff:
+	MainWindow::instance()->refreshDisplay();
+	MainWindow::instance()->refreshProfile();
+}
+
+void UndoMergeDives::undo()
+{
+	divesToMerge = addDives(unmergedDives);
+	mergedDive = removeDive(diveToUnmerge);
+	renumberDives(divesToRenumber);
+
+	// Finally, do the UI stuff:
+	MainWindow::instance()->refreshDisplay();
+	MainWindow::instance()->refreshProfile();
+}

desktop-widgets/undocommands.h

@@ -180,7 +180,7 @@ private:
 	void redo() override;
 
 	// For redo
-	QVector<struct dive*> divesToDelete;
+	std::vector<struct dive*> divesToDelete;
 
 	std::vector<OwningTripPtr> tripsToAdd;
 	std::vector<DiveToAdd> divesToAdd;
@@ -246,4 +246,25 @@ private:
 	dive		*divesToUnsplit[2];
 };
 
+class UndoMergeDives : public QUndoCommand {
+public:
+	UndoMergeDives(const QVector<dive *> &dives);
+private:
+	void undo() override;
+	void redo() override;
+
+	// For redo
+	// Add one and remove a batch of dives
+	DiveToAdd		 mergedDive;
+	std::vector<dive *>	 divesToMerge;
+
+	// For undo
+	// Remove one and add a batch of dives
+	dive			*diveToUnmerge;
+	std::vector<DiveToAdd>	 unmergedDives;
+
+	// For undo and redo
+	QVector<QPair<int, int>> divesToRenumber;
+};
+
 #endif // UNDOCOMMANDS_H