Undo: don't send signals batched by trip

Since the default view is batched by trips, signals were sent trip-wise.
This seemed like a good idea at first, but when more and more parts used
these signals, it became a burden. Therefore push the batching to the
part of the code where it is needed: the trip view.

The divesAdded and divesDeleted are not yet converted, because these
are combined with trip addition/deletion. This should also be detangled,
but not now.

Since the dive-lists were sorted in the processByTrip function, the
dive-list model now does its own sorting. This will have to be
audited.

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

desktop-widgets/command_divelist.cpp

@@ -89,6 +89,37 @@ dive *DiveListBase::addDive(DiveToAdd &d)
 	return res;
 }
 
+// Some signals are sent in batches per trip. To avoid writing the same loop
+// twice, this template takes a vector of trip / dive pairs, sorts it
+// by trip and then calls a function-object with trip and a QVector of dives in that trip.
+// The dives are sorted by the dive_less_than() function defined in the core.
+// Input parameters:
+//	- dives: a vector of trip,dive pairs, which will be sorted and processed in batches by trip.
+//	- action: a function object, taking a trip-pointer and a QVector of dives, which will be called for each batch.
+template<typename Function>
+void processByTrip(std::vector<std::pair<dive_trip *, dive *>> &dives, Function action)
+{
+	// Sort lexicographically by trip then according to the dive_less_than() function.
+	std::sort(dives.begin(), dives.end(),
+		  [](const std::pair<dive_trip *, dive *> &e1, const std::pair<dive_trip *, dive *> &e2)
+		  { return e1.first == e2.first ? dive_less_than(e1.second, e2.second) : e1.first < e2.first; });
+
+	// Then, process the dives in batches by trip
+	size_t i, j; // Begin and end of batch
+	for (i = 0; i < dives.size(); i = j) {
+		dive_trip *trip = dives[i].first;
+		for (j = i + 1; j < dives.size() && dives[j].first == trip; ++j)
+			; // pass
+		// Copy dives into a QVector. Some sort of "range_view" would be ideal, but Qt doesn't work this way.
+		QVector<dive *> divesInTrip(j - i);
+		for (size_t k = i; k < j; ++k)
+			divesInTrip[k - i] = dives[k].second;
+
+		// Finally, emit the signal
+		action(trip, divesInTrip);
+	}
+}
+
 // 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.
@@ -143,8 +174,10 @@ DivesAndSitesToRemove DiveListBase::addDives(DivesAndTripsToAdd &toAdd)
 {
 	std::vector<dive *> res;
 	std::vector<dive_site *> sites;
+	std::vector<std::pair<dive_trip *, dive *>> dives;
 	res.resize(toAdd.dives.size());
 	sites.reserve(toAdd.sites.size());
+	dives.reserve(toAdd.sites.size());
 
 	// Make sure that the dive list is sorted. The added dives will be sent in a signal
 	// and the recipients assume that the dives are sorted the same way as they are
@@ -157,8 +190,10 @@ DivesAndSitesToRemove DiveListBase::addDives(DivesAndTripsToAdd &toAdd)
 	// 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 = toAdd.dives.rbegin(); it != toAdd.dives.rend(); ++it, ++it2)
+	for (auto it = toAdd.dives.rbegin(); it != toAdd.dives.rend(); ++it, ++it2) {
 		*it2 = addDive(*it);
+		dives.push_back({ (*it2)->divetrip, *it2 });
+	}
 	toAdd.dives.clear();
 
 	// If the dives belong to new trips, add these as well.
@@ -180,7 +215,7 @@ DivesAndSitesToRemove DiveListBase::addDives(DivesAndTripsToAdd &toAdd)
 	toAdd.sites.clear();
 
 	// Send signals by trip.
-	processByTrip(res, [&](dive_trip *trip, const QVector<dive *> &divesInTrip) {
+	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 as "add it".
 		bool createTrip = trip && std::find(addedTrips.begin(), addedTrips.end(), trip) != addedTrips.end();
 		// Finally, emit the signal
@@ -191,30 +226,21 @@ DivesAndSitesToRemove DiveListBase::addDives(DivesAndTripsToAdd &toAdd)
 
 // 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 indices once all divelist-actions are controlled by undo-able commands
 static void renumberDives(QVector<QPair<dive *, int>> &divesToRenumber)
 {
+	QVector<dive *> dives;
+	dives.reserve(divesToRenumber.size());
 	for (auto &pair: divesToRenumber) {
 		dive *d = pair.first;
 		if (!d)
 			continue;
 		std::swap(d->number, pair.second);
+		dives.push_back(d);
 		invalidate_dive_cache(d);
 	}
 
-	// Emit changed signals per trip.
-	// First, collect all dives and sort by trip
-	std::vector<std::pair<dive_trip *, dive *>> dives;
-	dives.reserve(divesToRenumber.size());
-	for (const auto &pair: divesToRenumber) {
-		dive *d = pair.first;
-		dives.push_back({ d->divetrip, d });
-	}
-
 	// Send signals.
-	processByTrip(dives, [&](dive_trip *trip, const QVector<dive *> &divesInTrip) {
-		emit diveListNotifier.divesChanged(trip, divesInTrip, DiveField::NR);
-	});
+	emit diveListNotifier.divesChanged(dives, DiveField::NR);
 }
 
 // This helper function moves a dive to a trip. The old trip is recorded in the
@@ -571,20 +597,22 @@ ShiftTime::ShiftTime(const QVector<dive *> &changedDives, int amount)
 
 void ShiftTime::redoit()
 {
-	for (dive *d: diveList)
+	std::vector<dive_trip *> trips;
+	for (dive *d: diveList) {
 		d->when += timeChanged;
+		if (d->divetrip && std::find(trips.begin(), trips.end(), d->divetrip) == trips.end())
+			trips.push_back(d->divetrip);
+	}
 
-	// Changing times may have unsorted the dive table
+	// Changing times may have unsorted the dive and trip tables
 	sort_dive_table(&dive_table);
 	sort_trip_table(&trip_table);
+	for (dive_trip *trip: trips)
+		sort_dive_table(&trip->dives); // Keep the trip-table in order
 
-	// Send signals per trip (see comments in DiveListNotifier.h) and sort tables.
-	processByTrip(diveList, [&](dive_trip *trip, const QVector<dive *> &divesInTrip) {
-		if (trip)
-			sort_dive_table(&trip->dives); // Keep the trip-table in order
-		emit diveListNotifier.divesTimeChanged(trip, timeChanged, divesInTrip);
-		emit diveListNotifier.divesChanged(trip, divesInTrip, DiveField::DATETIME);
-	});
+	// Send signals
+	emit diveListNotifier.divesTimeChanged(timeChanged, diveList);
+	emit diveListNotifier.divesChanged(diveList, DiveField::DATETIME);
 
 	// Negate the time-shift so that the next call does the reverse
 	timeChanged = -timeChanged;

desktop-widgets/command_divesite.cpp

@@ -18,7 +18,7 @@ namespace Command {
 static std::vector<dive_site *> addDiveSites(std::vector<OwningDiveSitePtr> &sites)
 {
 	std::vector<dive_site *> res;
-	std::vector<dive *> changedDives;
+	QVector<dive *> changedDives;
 	res.reserve(sites.size());
 
 	for (OwningDiveSitePtr &ds: sites) {
@@ -36,9 +36,7 @@ static std::vector<dive_site *> addDiveSites(std::vector<OwningDiveSitePtr> &sit
 		emit diveListNotifier.diveSiteAdded(res.back(), idx); // Inform frontend of new dive site.
 	}
 
-	processByTrip(changedDives, [&](dive_trip *trip, const QVector<dive *> &divesInTrip) {
-		emit diveListNotifier.divesChanged(trip, divesInTrip, DiveField::DIVESITE);
-	});
+	emit diveListNotifier.divesChanged(changedDives, DiveField::DIVESITE);
 
 	// Clear vector of unused owning pointers
 	sites.clear();
@@ -52,7 +50,7 @@ static std::vector<dive_site *> addDiveSites(std::vector<OwningDiveSitePtr> &sit
 static std::vector<OwningDiveSitePtr> removeDiveSites(std::vector<dive_site *> &sites)
 {
 	std::vector<OwningDiveSitePtr> res;
-	std::vector<dive *> changedDives;
+	QVector<dive *> changedDives;
 	res.reserve(sites.size());
 
 	for (dive_site *ds: sites) {
@@ -69,9 +67,7 @@ static std::vector<OwningDiveSitePtr> removeDiveSites(std::vector<dive_site *> &
 		emit diveListNotifier.diveSiteDeleted(ds, idx); // Inform frontend of removed dive site.
 	}
 
-	processByTrip(changedDives, [&](dive_trip *trip, const QVector<dive *> &divesInTrip) {
-		emit diveListNotifier.divesChanged(trip, divesInTrip, DiveField::DIVESITE);
-	});
+	emit diveListNotifier.divesChanged(changedDives, DiveField::DIVESITE);
 
 	sites.clear();
 
@@ -363,7 +359,7 @@ void MergeDiveSites::redo()
 	sitesToAdd = std::move(removeDiveSites(sitesToRemove));
 
 	// Remember which dives changed so that we can send a single dives-edited signal
-	std::vector<dive *> divesChanged;
+	QVector<dive *> divesChanged;
 
 	// The dives of the above dive sites were reset to no dive sites.
 	// Add them to the merged-into dive site. Thankfully, we remember
@@ -374,15 +370,13 @@ void MergeDiveSites::redo()
 			divesChanged.push_back(site->dives.dives[i]);
 		}
 	}
-	processByTrip(divesChanged, [&](dive_trip *trip, const QVector<dive *> &divesInTrip) {
-		emit diveListNotifier.divesChanged(trip, divesInTrip, DiveField::DIVESITE);
-	});
+	emit diveListNotifier.divesChanged(divesChanged, DiveField::DIVESITE);
 }
 
 void MergeDiveSites::undo()
 {
 	// Remember which dives changed so that we can send a single dives-edited signal
-	std::vector<dive *> divesChanged;
+	QVector<dive *> divesChanged;
 
 	// Before readding the dive sites, unregister the corresponding dives so that they can be
 	// readded to their old dive sites.
@@ -395,9 +389,7 @@ void MergeDiveSites::undo()
 
 	sitesToRemove = std::move(addDiveSites(sitesToAdd));
 
-	processByTrip(divesChanged, [&](dive_trip *trip, const QVector<dive *> &divesInTrip) {
-		emit diveListNotifier.divesChanged(trip, divesInTrip, DiveField::DIVESITE);
-	});
+	emit diveListNotifier.divesChanged(divesChanged, DiveField::DIVESITE);
 }
 
 } // namespace Command

desktop-widgets/command_edit.cpp

@@ -97,9 +97,7 @@ void EditBase<T>::undo()
 
 	// Send signals.
 	DiveField id = fieldId();
-	processByTrip(dives, [&](dive_trip *trip, const QVector<dive *> &divesInTrip) {
-		emit diveListNotifier.divesChanged(trip, divesInTrip, id);
-	});
+	emit diveListNotifier.divesChanged(QVector<dive *>::fromStdVector(dives), id);
 
 	if (setSelection(selectedDives, current))
 		emit diveListNotifier.selectionChanged(); // If the selection changed -> tell the frontend
@@ -539,9 +537,7 @@ void EditTagsBase::undo()
 
 	// Send signals.
 	DiveField id = fieldId();
-	processByTrip(dives, [&](dive_trip *trip, const QVector<dive *> &divesInTrip) {
-		emit diveListNotifier.divesChanged(trip, divesInTrip, id);
-	});
+	emit diveListNotifier.divesChanged(QVector<dive *>::fromStdVector(dives), id);
 
 	if (setSelection(selectedDives, current))
 		emit diveListNotifier.selectionChanged(); // If the selection changed -> tell the frontend
@@ -750,7 +746,7 @@ void PasteDives::undo()
 	}
 	ownedDiveSites.clear();
 
-	std::vector<dive *> divesToNotify; // Remember dives so that we can send signals later
+	QVector<dive *> divesToNotify; // Remember dives so that we can send signals later
 	divesToNotify.reserve(dives.size());
 	for (PasteState &state: dives) {
 		divesToNotify.push_back(state.d);
@@ -780,28 +776,26 @@ void PasteDives::undo()
 	// TODO: We send one signal per changed field. This means that the dive list may
 	// update the entry numerous times. Perhaps change the field-id into flags?
 	// There seems to be a number of enums / flags describing dive fields. Perhaps unify them all?
-	processByTrip(divesToNotify, [&](dive_trip *trip, const QVector<dive *> &divesInTrip) {
-		if (what.notes)
-			emit diveListNotifier.divesChanged(trip, divesInTrip, DiveField::NOTES);
-		if (what.divemaster)
-			emit diveListNotifier.divesChanged(trip, divesInTrip, DiveField::DIVEMASTER);
-		if (what.buddy)
-			emit diveListNotifier.divesChanged(trip, divesInTrip, DiveField::BUDDY);
-		if (what.suit)
-			emit diveListNotifier.divesChanged(trip, divesInTrip, DiveField::SUIT);
-		if (what.rating)
-			emit diveListNotifier.divesChanged(trip, divesInTrip, DiveField::RATING);
-		if (what.visibility)
-			emit diveListNotifier.divesChanged(trip, divesInTrip, DiveField::VISIBILITY);
-		if (what.divesite)
-			emit diveListNotifier.divesChanged(trip, divesInTrip, DiveField::DIVESITE);
-		if (what.tags)
-			emit diveListNotifier.divesChanged(trip, divesInTrip, DiveField::TAGS);
-		if (what.cylinders)
-			emit diveListNotifier.cylindersReset(trip, divesInTrip);
-		if (what.weights)
-			emit diveListNotifier.weightsystemsReset(trip, divesInTrip);
-	});
+	if (what.notes)
+		emit diveListNotifier.divesChanged(divesToNotify, DiveField::NOTES);
+	if (what.divemaster)
+		emit diveListNotifier.divesChanged(divesToNotify, DiveField::DIVEMASTER);
+	if (what.buddy)
+		emit diveListNotifier.divesChanged(divesToNotify, DiveField::BUDDY);
+	if (what.suit)
+		emit diveListNotifier.divesChanged(divesToNotify, DiveField::SUIT);
+	if (what.rating)
+		emit diveListNotifier.divesChanged(divesToNotify, DiveField::RATING);
+	if (what.visibility)
+		emit diveListNotifier.divesChanged(divesToNotify, DiveField::VISIBILITY);
+	if (what.divesite)
+		emit diveListNotifier.divesChanged(divesToNotify, DiveField::DIVESITE);
+	if (what.tags)
+		emit diveListNotifier.divesChanged(divesToNotify, DiveField::TAGS);
+	if (what.cylinders)
+		emit diveListNotifier.cylindersReset(divesToNotify);
+	if (what.weights)
+		emit diveListNotifier.weightsystemsReset(divesToNotify);
 
 	if (diveSiteListChanged)
 		MapWidget::instance()->reload();

desktop-widgets/command_private.cpp

@@ -46,8 +46,8 @@ bool setSelection(const std::vector<dive *> &selection, dive *currentDive)
 {
 	// To do so, generate vectors of dives to be selected and deselected.
 	// We send signals batched by trip, so keep track of trip/dive pairs.
-	std::vector<std::pair<dive_trip *, dive *>> divesToSelect;
-	std::vector<std::pair<dive_trip *, dive *>> divesToDeselect;
+	QVector<dive *> divesToSelect;
+	QVector<dive *> divesToDeselect;
 
 	// TODO: We might want to keep track of selected dives in a more efficient way!
 	int i;
@@ -73,20 +73,16 @@ bool setSelection(const std::vector<dive *> &selection, dive *currentDive)
 		if (newState && !d->selected) {
 			d->selected = true;
 			++amount_selected;
-			divesToSelect.push_back({ d->divetrip, d });
+			divesToSelect.push_back(d);
 		} else if (!newState && d->selected) {
 			d->selected = false;
-			divesToDeselect.push_back({ d->divetrip, d });
+			divesToDeselect.push_back(d);
 		}
 	}
 
 	// Send the select and deselect signals
-	processByTrip(divesToSelect, [&](dive_trip *trip, const QVector<dive *> &divesInTrip) {
-		emit diveListNotifier.divesSelected(trip, divesInTrip);
-	});
-	processByTrip(divesToDeselect, [&](dive_trip *trip, const QVector<dive *> &divesInTrip) {
-		emit diveListNotifier.divesDeselected(trip, divesInTrip);
-	});
+	emit diveListNotifier.divesSelected(divesToSelect);
+	emit diveListNotifier.divesDeselected(divesToDeselect);
 
 	bool currentDiveChanged = false;
 	if (!currentDive) {

desktop-widgets/command_private.h

@@ -12,50 +12,6 @@
 
 namespace Command {
 
-// Generally, signals are sent in batches per trip. To avoid writing the same loop
-// again and again, this template takes a vector of trip / dive pairs, sorts it
-// by trip and then calls a function-object with trip and a QVector of dives in that trip.
-// The dives are sorted by the dive_less_than() function defined in the core.
-// Input parameters:
-//	- dives: a vector of trip,dive pairs, which will be sorted and processed in batches by trip.
-//	- action: a function object, taking a trip-pointer and a QVector of dives, which will be called for each batch.
-template<typename Function>
-void processByTrip(std::vector<std::pair<dive_trip *, dive *>> &dives, Function action)
-{
-	// Use std::tie for lexicographical sorting of trip, then start-time
-	std::sort(dives.begin(), dives.end(),
-		  [](const std::pair<dive_trip *, dive *> &e1, const std::pair<dive_trip *, dive *> &e2)
-		  { return e1.first == e2.first ? dive_less_than(e1.second, e2.second) : e1.first < e2.first; });
-
-	// Then, process the dives in batches by trip
-	size_t i, j; // Begin and end of batch
-	for (i = 0; i < dives.size(); i = j) {
-		dive_trip *trip = dives[i].first;
-		for (j = i + 1; j < dives.size() && dives[j].first == trip; ++j)
-			; // pass
-		// Copy dives into a QVector. Some sort of "range_view" would be ideal, but Qt doesn't work this way.
-		QVector<dive *> divesInTrip(j - i);
-		for (size_t k = i; k < j; ++k)
-			divesInTrip[k - i] = dives[k].second;
-
-		// Finally, emit the signal
-		action(trip, divesInTrip);
-	}
-}
-
-// The processByTrip() function above takes a vector if (trip, dive) pairs.
-// This overload takes a vector of dives.
-template<typename Vector, typename Function>
-void processByTrip(Vector &divesIn, Function action)
-{
-	std::vector<std::pair<dive_trip *, dive *>> dives;
-	dives.reserve(divesIn.size());
-	for (dive *d: divesIn)
-		dives.push_back({ d->divetrip, d });
-	processByTrip(dives, action);
-
-}
-
 // Reset the selection to the dives of the "selection" vector and send the appropriate signals.
 // Set the current dive to "currentDive". "currentDive" must be an element of "selection" (or
 // null if "seletion" is empty). Return true if the selection or current dive changed.

desktop-widgets/mapwidget.cpp

@@ -12,7 +12,6 @@
 #include "mainwindow.h"
 #include "divelistview.h"
 #include "command.h"
-#include "core/trip.h" // TODO: Needed because divesChanged uses a trip parameter -> remove that!
 
 static const QUrl urlMapWidget = QUrl(QStringLiteral("qrc:/qml/MapWidget.qml"));
 static const QUrl urlMapWidgetError = QUrl(QStringLiteral("qrc:/qml/MapWidgetError.qml"));
@@ -91,7 +90,7 @@ void MapWidget::coordinatesChanged(struct dive_site *ds, const location_t &locat
 	Command::editDiveSiteLocation(ds, location);
 }
 
-void MapWidget::divesChanged(dive_trip *, const QVector<dive *> &, DiveField field)
+void MapWidget::divesChanged(const QVector<dive *> &, DiveField field)
 {
 	if (field == DiveField::DIVESITE)
 		reload();

desktop-widgets/mapwidget.h

@@ -31,7 +31,7 @@ public slots:
 	void selectedDivesChanged(const QList<int> &);
 	void coordinatesChanged(struct dive_site *ds, const location_t &);
 	void doneLoading(QQuickWidget::Status status);
-	void divesChanged(dive_trip *, const QVector<dive *> &, DiveField field);
+	void divesChanged(const QVector<dive *> &, DiveField field);
 
 private:
 	static MapWidget *m_instance;

desktop-widgets/tab-widgets/TabDiveInformation.cpp

@@ -5,7 +5,6 @@
 #include "core/units.h"
 #include "core/dive.h"
 #include "desktop-widgets/command.h"
-#include "core/trip.h" // TODO: Needed because divesChanged uses a trip parameter -> remove that!
 #include "core/qthelper.h"
 #include "core/statistics.h"
 #include "core/display.h"
@@ -131,7 +130,7 @@ void TabDiveInformation::updateData()
 
 // This function gets called if a field gets updated by an undo command.
 // Refresh the corresponding UI field.
-void TabDiveInformation::divesChanged(dive_trip *trip, const QVector<dive *> &dives, DiveField field)
+void TabDiveInformation::divesChanged(const QVector<dive *> &dives, DiveField field)
 {
 	// If the current dive is not in list of changed dives, do nothing
 	if (!current_dive || !dives.contains(current_dive))

desktop-widgets/tab-widgets/TabDiveInformation.h

@@ -17,7 +17,7 @@ public:
 	void updateData() override;
 	void clear() override;
 private slots:
-	void divesChanged(dive_trip *trip, const QVector<dive *> &dives, DiveField field);
+	void divesChanged(const QVector<dive *> &dives, DiveField field);
 	void on_atmPressVal_editingFinished();
 	void on_atmPressType_currentIndexChanged(int index);
 private:

desktop-widgets/tab-widgets/maintab.cpp

@@ -281,7 +281,7 @@ static void profileFromDive(struct dive *d)
 
 // This function gets called if a field gets updated by an undo command.
 // Refresh the corresponding UI field.
-void MainTab::divesChanged(dive_trip *trip, const QVector<dive *> &dives, DiveField field)
+void MainTab::divesChanged(const QVector<dive *> &dives, DiveField field)
 {
 	// If the current dive is not in list of changed dives, do nothing
 	if (!current_dive || !dives.contains(current_dive))

desktop-widgets/tab-widgets/maintab.h

@@ -46,7 +46,7 @@ public:
 
 public
 slots:
-	void divesChanged(dive_trip *trip, const QVector<dive *> &dives, DiveField field);
+	void divesChanged(const QVector<dive *> &dives, DiveField field);
 	void diveSiteEdited(dive_site *ds, int field);
 	void tripChanged(dive_trip *trip, TripField field);
 	void updateDiveInfo();