Undo commands: refactor dive-trip handling

Trips were added to the core with the first dive of that trip.
With the recent changes that keep trips ordered by first dive,
this became counter-productive. Keeping a consistent state at
all times would mean resorting the trip table for every dive
that is added.

Instead, add all dives to a trip and *then* add the trip to the
core. Change the data-structures to not register trips-to-be-added
with individual dives, but keep them in a separate vector for
each undo command.

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

desktop-widgets/command_divelist.cpp

@@ -42,9 +42,10 @@ void processByTrip(std::vector<std::pair<dive_trip *, dive *>> &dives, Function
 
 
 // This helper function removes a dive, takes ownership of the dive and adds it to a DiveToAdd structure.
+// If the trip the dive belongs to becomes empty, it is removed and added to the tripsToAdd vector.
 // It is crucial that dives are added in reverse order of deletion, so the the indices are correctly
 // set and that the trips are added before they are used!
-DiveToAdd DiveListBase::removeDive(struct dive *d)
+DiveToAdd DiveListBase::removeDive(struct dive *d, std::vector<OwningTripPtr> &tripsToAdd)
 {
 	// If the dive to be removed is selected, we will inform the frontend
 	// later via a signal that the dive changed.
@@ -68,7 +69,7 @@ DiveToAdd DiveListBase::removeDive(struct dive *d)
 	res.trip = unregister_dive_from_trip(d);
 	if (res.trip && res.trip->dives.nr == 0) {
 		unregister_trip(res.trip);		// Remove trip from backend
-		res.tripToAdd.reset(res.trip);		// Take ownership of trip
+		tripsToAdd.emplace_back(res.trip);	// Take ownership of trip
 	}
 
 	res.dive.reset(unregister_dive(res.idx));	// Remove dive from backend
@@ -83,8 +84,6 @@ dive *DiveListBase::addDive(DiveToAdd &d)
 {
 	if (d.trip)
 		add_dive_to_trip(d.dive.get(), d.trip);
-	if (d.tripToAdd)
-		insert_trip(d.tripToAdd.release()); // Return ownership to backend
 	dive *res = d.dive.release();		// Give up ownership of dive
 
 	// Set the filter flag according to current filter settings
@@ -104,67 +103,70 @@ dive *DiveListBase::addDive(DiveToAdd &d)
 
 // 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.
+// Moreover, a vector of deleted trips is returned, if trips became empty.
 // The passed in vector is cleared.
-std::vector<DiveToAdd> DiveListBase::removeDives(std::vector<dive *> &divesToDelete)
+DivesAndTripsToAdd DiveListBase::removeDives(std::vector<dive *> &divesToDelete)
 {
-	std::vector<DiveToAdd> res;
+	std::vector<DiveToAdd> divesToAdd;
-	res.reserve(divesToDelete.size());
+	std::vector<OwningTripPtr> tripsToAdd;
+	divesToAdd.reserve(divesToDelete.size());
 
 	for (dive *d: divesToDelete)
-		res.push_back(removeDive(d));
+		divesToAdd.push_back(removeDive(d, tripsToAdd));
 	divesToDelete.clear();
 
 	// We send one dives-deleted signal per trip (see comments in DiveListNotifier.h).
 	// Therefore, collect all dives in an array and sort by trip.
 	std::vector<std::pair<dive_trip *, dive *>> dives;
-	dives.reserve(res.size());
+	dives.reserve(divesToAdd.size());
-	for (const DiveToAdd &entry: res)
+	for (const DiveToAdd &entry: divesToAdd)
 		dives.push_back({ entry.trip, entry.dive.get() });
 
 	// Send signals.
 	processByTrip(dives, [&](dive_trip *trip, const QVector<dive *> &divesInTrip) {
-		// Now, let's check if this trip is supposed to be deleted, by checking if it was marked
+		// Check if this trip is supposed to be deleted, by checking if it was marked as "add it".
-		// as "add it". We could be smarter here, but let's just check the whole array for brevity.
 		bool deleteTrip = trip &&
-				  std::find_if(res.begin(), res.end(), [trip](const DiveToAdd &entry)
+				  std::find_if(tripsToAdd.begin(), tripsToAdd.end(), [trip](const OwningTripPtr &ptr)
-					       { return entry.tripToAdd.get() == trip; }) != res.end();
+					       { return ptr.get() == trip; }) != tripsToAdd.end();
 		emit diveListNotifier.divesDeleted(trip, deleteTrip, divesInTrip);
 	});
-	return res;
+	return { std::move(divesToAdd), std::move(tripsToAdd) };
 }
 
 // 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.
-std::vector<dive *> DiveListBase::addDives(std::vector<DiveToAdd> &divesToAdd)
+std::vector<dive *> DiveListBase::addDives(DivesAndTripsToAdd &toAdd)
 {
 	std::vector<dive *> res;
-	res.resize(divesToAdd.size());
+	res.resize(toAdd.dives.size());
 
 	// First, tell the filters that new dives are added. We do this here
 	// instead of later by signals, so that the filter can set the
 	// checkboxes of the new rows to its liking. The added dives will
 	// then appear in the correct shown/hidden state.
 	QVector<dive *> divesForFilter;
-	for (const DiveToAdd &entry: divesToAdd)
+	for (const DiveToAdd &entry: toAdd.dives)
 		divesForFilter.push_back(entry.dive.get());
 
-	// At the end of the function, to send the proper dives-added signals,
-	// we the the list of added trips. Create this list now.
-	std::vector<dive_trip *> addedTrips;
-	for (const DiveToAdd &entry: divesToAdd) {
-		if (entry.tripToAdd)
-			addedTrips.push_back(entry.tripToAdd.get());
-	}
-
 	// Now, add the dives
 	// Note: the idiomatic STL-way would be std::transform, but let's use a loop since
 	// that is closer to classical C-style.
 	auto it2 = res.rbegin();
-	for (auto it = divesToAdd.rbegin(); it != divesToAdd.rend(); ++it, ++it2)
+	for (auto it = toAdd.dives.rbegin(); it != toAdd.dives.rend(); ++it, ++it2)
 		*it2 = addDive(*it);
-	divesToAdd.clear();
+	toAdd.dives.clear();
+
+	// If the dives belong to new trips, add these as well.
+	// Remember the pointers so that we can later check if a trip was newly added
+	std::vector<dive_trip *> addedTrips;
+	addedTrips.reserve(toAdd.trips.size());
+	for (OwningTripPtr &trip: toAdd.trips) {
+		addedTrips.push_back(trip.get());
+		insert_trip(trip.release()); // Return ownership to backend
+	}
+	toAdd.trips.clear();
 
 	// We send one dives-deleted signal per trip (see comments in DiveListNotifier.h).
 	// Therefore, collect all dives in a array and sort by trip.
@@ -175,8 +177,7 @@ std::vector<dive *> DiveListBase::addDives(std::vector<DiveToAdd> &divesToAdd)
 
 	// Send signals.
 	processByTrip(dives, [&](dive_trip *trip, const QVector<dive *> &divesInTrip) {
-		// Now, let's check if this trip is supposed to be created, by checking if it was marked
+		// Now, let's check if this trip is supposed to be created, by checking if it was marked as "add it".
-		// as "add it". We could be smarter here, but let's just check the whole array for brevity.
 		bool createTrip = trip && std::find(addedTrips.begin(), addedTrips.end(), trip) != addedTrips.end();
 		// Finally, emit the signal
 		emit diveListNotifier.divesAdded(trip, createTrip, divesInTrip);
@@ -515,7 +516,9 @@ AddDive::AddDive(dive *d, bool autogroup, bool newNumber)
 	if (newNumber)
 		divePtr->number = get_dive_nr_at_idx(idx);
 
-	divesToAdd.push_back({ std::move(divePtr), std::move(allocTrip), trip, idx });
+	divesToAdd.dives.push_back({ std::move(divePtr), trip, idx });
+	if (allocTrip)
+		divesToAdd.trips.push_back(std::move(allocTrip));
 }
 
 bool AddDive::workToBeDone()
@@ -584,8 +587,8 @@ void DeleteDive::redoit()
 
 	// Deselect all dives and select dive that was close to the first deleted dive
 	dive *newCurrent = nullptr;
-	if (!divesToAdd.empty()) {
+	if (!divesToAdd.dives.empty()) {
-		timestamp_t when = divesToAdd[0].dive->when;
+		timestamp_t when = divesToAdd.dives[0].dive->when;
 		newCurrent = find_next_visible_dive(when);
 	}
 	if (newCurrent)
@@ -772,13 +775,13 @@ SplitDives::SplitDives(dive *d, duration_t time)
 	new2->selected = false;
 
 	diveToSplit.push_back(d);
-	splitDives.resize(2);
+	splitDives.dives.resize(2);
-	splitDives[0].dive.reset(new1);
+	splitDives.dives[0].dive.reset(new1);
-	splitDives[0].trip = d->divetrip;
+	splitDives.dives[0].trip = d->divetrip;
-	splitDives[0].idx = idx;
+	splitDives.dives[0].idx = idx;
-	splitDives[1].dive.reset(new2);
+	splitDives.dives[1].dive.reset(new2);
-	splitDives[1].trip = d->divetrip;
+	splitDives.dives[1].trip = d->divetrip;
-	splitDives[1].idx = idx + 1;
+	splitDives.dives[1].idx = idx + 1;
 }
 
 bool SplitDives::workToBeDone()
@@ -882,16 +885,16 @@ MergeDives::MergeDives(const QVector <dive *> &dives)
 		}
 	}
 
-	mergedDive.resize(1);
+	mergedDive.dives.resize(1);
-	mergedDive[0].dive = std::move(d);
+	mergedDive.dives[0].dive = std::move(d);
-	mergedDive[0].idx = get_divenr(dives[0]);
+	mergedDive.dives[0].idx = get_divenr(dives[0]);
-	mergedDive[0].trip = preferred_trip;
+	mergedDive.dives[0].trip = preferred_trip;
 	divesToMerge = dives.toStdVector();
 }
 
 bool MergeDives::workToBeDone()
 {
-	return !mergedDive.empty();
+	return !mergedDive.dives.empty();
 }
 
 void MergeDives::redoit()

desktop-widgets/command_divelist.h

@@ -15,11 +15,16 @@ namespace Command {
 // Potentially it also adds a trip (if deletion of the dive resulted in deletion of the trip)
 struct DiveToAdd {
 	OwningDivePtr	 dive;		// Dive to add
-	OwningTripPtr	 tripToAdd;	// Not-null if we also have to add a dive
 	dive_trip	*trip;		// Trip the dive belongs to, may be null
 	int		 idx;		// Position in divelist
 };
 
+// Multiple trips and dives that have to be added for a command
+struct DivesAndTripsToAdd {
+	std::vector<DiveToAdd> dives;
+	std::vector<OwningTripPtr> trips;
+};
+
 // This helper structure describes a dive that should be moved to / removed from
 // a trip. If the "trip" member is null, the dive is removed from its trip (if
 // it is in a trip, that is)
@@ -29,7 +34,7 @@ struct DiveToTrip
 	dive_trip	*trip;
 };
 
-// This helper structure describes a number of dives to add to /remove from /
+// This helper structure describes a number of dives to add to / remove from /
 // move between trips.
 // It has ownership of the trips (if any) that have to be added before hand.
 struct DivesToTrip
@@ -51,10 +56,10 @@ protected:
 
 	// These are helper functions to add / remove dive from the C-core structures,
 	// which set the selectionChanged flag if the added / removed dive was selected.
-	DiveToAdd removeDive(struct dive *d);
+	DiveToAdd removeDive(struct dive *d, std::vector<OwningTripPtr> &tripsToAdd);
 	dive *addDive(DiveToAdd &d);
-	std::vector<DiveToAdd> removeDives(std::vector<dive *> &divesToDelete);
+	DivesAndTripsToAdd removeDives(std::vector<dive *> &divesToDelete);
-	std::vector<dive *> addDives(std::vector<DiveToAdd> &divesToAdd);
+	std::vector<dive *> addDives(DivesAndTripsToAdd &toAdd);
 
 	// Set the selection to a given state. Set the selectionChanged flag if anything changed.
 	void restoreSelection(const std::vector<dive *> &selection, dive *currentDive);
@@ -89,9 +94,9 @@ private:
 	bool workToBeDone() override;
 
 	// For redo
-	// Note: we use a vector even though we add only a single dive, so
+	// Note: we a multi-dive structure even though we add only a single dive, so
 	// that we can reuse the multi-dive functions of the other commands.
-	std::vector<DiveToAdd>	divesToAdd;
+	DivesAndTripsToAdd	divesToAdd;
 
 	// For undo
 	std::vector<dive *>	divesToRemove;
@@ -111,7 +116,7 @@ private:
 	std::vector<struct dive*> divesToDelete;
 
 	std::vector<OwningTripPtr> tripsToAdd;
-	std::vector<DiveToAdd> divesToAdd;
+	DivesAndTripsToAdd divesToAdd;
 };
 
 class ShiftTime : public DiveListBase {
@@ -185,13 +190,13 @@ private:
 	// Note: we use a vector even though we split only a single dive, so
 	// that we can reuse the multi-dive functions of the other commands.
 	std::vector<dive *>	diveToSplit;
-	std::vector<DiveToAdd>	splitDives;
+	DivesAndTripsToAdd	splitDives;
 
 	// For undo
 	// For each dive to unsplit, we remove two dives from and add one into the backend
-	// Note: we use a vector even though we unsplit only a single dive, so
+	// Note: we use a multi-dive structure even though we unsplit only a single dive, so
 	// that we can reuse the multi-dive functions of the other commands.
-	std::vector<DiveToAdd>	unsplitDive;
+	DivesAndTripsToAdd	unsplitDive;
 	std::vector<dive *>	divesToUnsplit;
 };
 
@@ -205,9 +210,9 @@ private:
 
 	// For redo
 	// Add one and remove a batch of dives
-	// Note: we use a vector even though we add only a single dive, so
+	// Note: we use a multi-dives structure even though we add only a single dive, so
 	// that we can reuse the multi-dive functions of the other commands.
-	std::vector<DiveToAdd>	mergedDive;
+	DivesAndTripsToAdd	mergedDive;
 	std::vector<dive *>	divesToMerge;
 
 	// For undo
@@ -215,7 +220,7 @@ private:
 	// Note: we use a vector even though we remove only a single dive, so
 	// that we can reuse the multi-dive functions of the other commands.
 	std::vector<dive *>	diveToUnmerge;
-	std::vector<DiveToAdd>	unmergedDives;
+	DivesAndTripsToAdd	unmergedDives;
 
 	// For undo and redo
 	QVector<QPair<dive *, int>> divesToRenumber;