Undo: update dive list after edit command

The dive list was not updated automatically when an edit command was
executed. There was already a signal to do that, viz. divesChanged().
But that signal worked by-trip and didn't have a dive-field specifier.

The edit-commands used the divesEdited() signal that isn't by-trip
but has a dive-field specifier.

Unify these two signals to be by-trip and with dive-field specifier.
This needs common code to generate the by-trip list that is moved to
a command_private.h header.

Since there might now be multiple signals (one per trip) actually
check in the main-tab whether the current trip is affected to
avoid multiple update of fields. This has the positive(?) effect
of not doing any update if the current dive isn't changed.

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

core/subsurface-qt/DiveListNotifier.h

@@ -13,6 +13,7 @@
 // Dive fields that can be edited.
 // Use "enum class" to not polute the global name space.
 enum class DiveField {
+	NR,
 	DATETIME,
 	DEPTH,
 	DURATION,
@@ -46,7 +47,7 @@ signals:
 	// - The "trip" arguments are null for top-level-dives.
 	void divesAdded(dive_trip *trip, bool addTrip, const QVector<dive *> &dives);
 	void divesDeleted(dive_trip *trip, bool deleteTrip, const QVector<dive *> &dives);
-	void divesChanged(dive_trip *trip, const QVector<dive *> &dives);
+	void divesChanged(dive_trip *trip, const QVector<dive *> &dives, DiveField field);
 	void divesMovedBetweenTrips(dive_trip *from, dive_trip *to, bool deleteFrom, bool createTo, const QVector<dive *> &dives);
 	void divesTimeChanged(dive_trip *trip, timestamp_t delta, const QVector<dive *> &dives);
 
@@ -69,9 +70,6 @@ signals:
 	void diveSiteDiveCountChanged(dive_site *ds);
 	void diveSiteChanged(dive_site *ds, int field); // field according to LocationInformationModel
 	void diveSiteDivesChanged(dive_site *ds); // The dives associated with that site changed
-
-	// Signals emitted when dives are edited.
-	void divesEdited(const QVector<dive *> &dives, DiveField);
 public:
 	// Desktop uses the QTreeView class to present the list of dives. The layout
 	// of this class gives us a very fundamental problem, as we can not easily

desktop-widgets/command_divelist.cpp

@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 
 #include "command_divelist.h"
+#include "command_private.h"
 #include "desktop-widgets/mainwindow.h"
 #include "desktop-widgets/divelistview.h"
 #include "core/divelist.h"
@@ -13,36 +14,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.
-// 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 std::tie(e1.first, e1.second->when) < std::tie(e2.first, e2.second->when); });
-
-	// 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 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 that the indices are correctly
@@ -194,15 +165,8 @@ DivesAndSitesToRemove DiveListBase::addDives(DivesAndTripsToAdd &toAdd)
 	}
 	toAdd.sites.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.
-	std::vector<std::pair<dive_trip *, dive *>> dives;
-	dives.reserve(res.size());
-	for (dive *d: res)
-		dives.push_back({ d->divetrip, d });
-
-	// Send signals.
-	processByTrip(dives, [&](dive_trip *trip, const QVector<dive *> &divesInTrip) {
+	// Send signals by trip.
+	processByTrip(res, [&](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
@@ -235,7 +199,7 @@ static void renumberDives(QVector<QPair<dive *, int>> &divesToRenumber)
 
 	// Send signals.
 	processByTrip(dives, [&](dive_trip *trip, const QVector<dive *> &divesInTrip) {
-		emit diveListNotifier.divesChanged(trip, divesInTrip);
+		emit diveListNotifier.divesChanged(trip, divesInTrip, DiveField::NR);
 	});
 }
 
@@ -733,20 +697,13 @@ void ShiftTime::redoit()
 	sort_dive_table(&dive_table);
 	sort_trip_table(&trip_table);
 
-	// We send one time changed 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(diveList.size());
-	for (dive *d: diveList)
-		dives.push_back({ d->divetrip, d });
-
-	// Send signals and sort tables.
-	processByTrip(dives, [&](dive_trip *trip, const QVector<dive *> &divesInTrip) {
+	// 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);
 	});
-	emit diveListNotifier.divesEdited(diveList, DiveField::DATETIME);
 
 	// Negate the time-shift so that the next call does the reverse
 	timeChanged = -timeChanged;

desktop-widgets/command_divesite.cpp

@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 
 #include "command_divesite.h"
+#include "command_private.h"
 #include "core/divesite.h"
 #include "core/subsurface-qt/DiveListNotifier.h"
 #include "core/qthelper.h"
@@ -312,7 +313,7 @@ void MergeDiveSites::redo()
 	sitesToAdd = std::move(removeDiveSites(sitesToRemove));
 
 	// Remember which dives changed so that we can send a single dives-edited signal
-	QVector<dive *> divesChanged;
+	std::vector<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
@@ -320,28 +321,32 @@ void MergeDiveSites::redo()
 	for (const OwningDiveSitePtr &site: sitesToAdd) {
 		for (int i = 0; i < site->dives.nr; ++i) {
 			add_dive_to_dive_site(site->dives.dives[i], ds);
-			divesChanged.append(site->dives.dives[i]);
+			divesChanged.push_back(site->dives.dives[i]);
 		}
 	}
-	emit diveListNotifier.divesEdited(divesChanged, DiveField::DIVESITE);
+	processByTrip(divesChanged, [&](dive_trip *trip, const QVector<dive *> &divesInTrip) {
+		emit diveListNotifier.divesChanged(trip, divesInTrip, DiveField::DIVESITE);
+	});
 }
 
 void MergeDiveSites::undo()
 {
 	// Remember which dives changed so that we can send a single dives-edited signal
-	QVector<dive *> divesChanged;
+	std::vector<dive *> divesChanged;
 
 	// Before readding the dive sites, unregister the corresponding dives so that they can be
 	// readded to their old dive sites.
 	for (const OwningDiveSitePtr &site: sitesToAdd) {
 		for (int i = 0; i < site->dives.nr; ++i) {
 			unregister_dive_from_dive_site(site->dives.dives[i]);
-			divesChanged.append(site->dives.dives[i]);
+			divesChanged.push_back(site->dives.dives[i]);
 		}
 	}
 
 	sitesToRemove = std::move(addDiveSites(sitesToAdd));
-	emit diveListNotifier.divesEdited(divesChanged, DiveField::DIVESITE);
+	processByTrip(divesChanged, [&](dive_trip *trip, const QVector<dive *> &divesInTrip) {
+		emit diveListNotifier.divesChanged(trip, divesInTrip, DiveField::DIVESITE);
+	});
 }
 
 } // namespace Command

desktop-widgets/command_edit.cpp

@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 
 #include "command_edit.h"
+#include "command_private.h"
 #include "core/divelist.h"
 #include "core/qthelper.h" // for copy_qstring
 #include "desktop-widgets/mapwidget.h" // TODO: Replace desktop-dependency by signal
@@ -59,7 +60,11 @@ void EditBase<T>::undo()
 
 	std::swap(old, value);
 
-	emit diveListNotifier.divesEdited(QVector<dive *>::fromStdVector(dives), fieldId());
+	// Send signals.
+	DiveField id = fieldId();
+	processByTrip(dives, [&](dive_trip *trip, const QVector<dive *> &divesInTrip) {
+		emit diveListNotifier.divesChanged(trip, divesInTrip, id);
+	});
 
 	mark_divelist_changed(true);
 }
@@ -460,7 +465,11 @@ void EditTagsBase::undo()
 
 	std::swap(tagsToAdd, tagsToRemove);
 
-	emit diveListNotifier.divesEdited(QVector<dive *>::fromStdVector(dives), fieldId());
+	// Send signals.
+	DiveField id = fieldId();
+	processByTrip(dives, [&](dive_trip *trip, const QVector<dive *> &divesInTrip) {
+		emit diveListNotifier.divesChanged(trip, divesInTrip, id);
+	});
 
 	mark_divelist_changed(true);
 }

desktop-widgets/command_private.h

@@ -0,0 +1,61 @@
+// SPDX-License-Identifier: GPL-2.0
+// Private definitions for the command-objects
+
+#ifndef COMMAND_PRIVATE_H
+#define COMMAND_PRIVATE_H
+
+#include <vector>
+#include <utility>
+#include <QVector>
+
+struct dive;
+struct dive_trip;
+
+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.
+// 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 std::tie(e1.first, e1.second->when) < std::tie(e2.first, e2.second->when); });
+
+	// 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);
+
+}
+
+} // namespace Command
+
+#endif // COMMAND_PRIVATE_H

desktop-widgets/tab-widgets/maintab.cpp

@@ -82,7 +82,7 @@ MainTab::MainTab(QWidget *parent) : QTabWidget(parent),
 	ui.weights->setModel(weightModel);
 	closeMessage();
 
-	connect(&diveListNotifier, &DiveListNotifier::divesEdited, this, &MainTab::divesEdited);
+	connect(&diveListNotifier, &DiveListNotifier::divesChanged, this, &MainTab::divesChanged);
 	connect(ui.editDiveSiteButton, &QToolButton::clicked, MainWindow::instance(), &MainWindow::startDiveSiteEdit);
 	connect(ui.location, &DiveLocationLineEdit::entered, MapWidget::instance(), &MapWidget::centerOnIndex);
 	connect(ui.location, &DiveLocationLineEdit::currentChanged, MapWidget::instance(), &MapWidget::centerOnIndex);
@@ -340,10 +340,10 @@ 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::divesEdited(const QVector<dive *> &, DiveField field)
+void MainTab::divesChanged(dive_trip *trip, const QVector<dive *> &dives, DiveField field)
 {
-	// If there is no current dive, no point in updating anything
-	if (!current_dive)
+	// If the current dive is not in list of changed dives, do nothing
+	if (!current_dive || !dives.contains(current_dive))
 		return;
 
 	switch(field) {

desktop-widgets/tab-widgets/maintab.h

@@ -62,7 +62,7 @@ signals:
 	void diveSiteChanged();
 public
 slots:
-	void divesEdited(const QVector<dive *> &dives, DiveField field);
+	void divesChanged(dive_trip *trip, const QVector<dive *> &dives, DiveField field);
 	void addCylinder_clicked();
 	void addWeight_clicked();
 	void updateDiveInfo(bool clear = false);