selection: avoid select_dive() and deselect_dive calls in dive list

Each of these calls recalculates the current dive and divecomputer.
Instead, collect the dives to be selected/deselected and (de)select
them at once.

This needs some code refactoring in the core, because we need a
function that
1) doesn't send a signal by itself.
2) doesn't clear the trip-selection.

This contains some reorganization of the selection functions
signatures: The filter code is the only caller that keeps the
selected dive and the only caller that cares about whether the
current dive changed. So let only the function that keeps the
selected dive return whether the current dive changed.

It's all very fragile.

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

core/divefilter.cpp

@@ -67,7 +67,7 @@ ShownChange DiveFilter::update(const QVector<dive *> &dives) const
 		updateDiveStatus(d, newStatus, res, removeFromSelection);
 	}
 	updateSelection(selection, std::vector<dive *>(), removeFromSelection);
-	res.currentChanged = setSelection(selection);
+	res.currentChanged = setSelectionKeepCurrent(selection);
 	return res;
 }
 
@@ -107,7 +107,7 @@ ShownChange DiveFilter::updateAll() const
 		}
 	}
 	updateSelection(selection, std::vector<dive *>(), removeFromSelection);
-	res.currentChanged = setSelection(selection);
+	res.currentChanged = setSelectionKeepCurrent(selection);
 	return res;
 }
 

core/selection.cpp

@@ -177,21 +177,17 @@ static void setClosestCurrentDive(timestamp_t when, const std::vector<dive *> &s
 	}
 }
 
-// Reset the selection to the dives of the "selection" vector and send the appropriate signals.
+
+// Reset the selection to the dives of the "selection" vector.
 // Set the current dive to "currentDive". "currentDive" must be an element of "selection" (or
-// null if "selection" is empty). Return true if the current dive changed.
+// null if "selection" is empty).
-bool setSelection(const std::vector<dive *> &selection, dive *currentDive, int currentDc)
+// Does not send signals or clear the trip selection.
+QVector<dive *> setSelectionCore(const std::vector<dive *> &selection, dive *currentDive, int currentDc)
 {
 	// 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.
 	QVector<dive *> divesToSelect;
 	divesToSelect.reserve(selection.size());
-	const dive *oldCurrent = current_dive;
-
-	// Since we select only dives, there are no selected trips!
-	amount_trips_selected = 0;
-	for (int i = 0; i < divelog.trips->nr; ++i)
-		divelog.trips->trips[i]->selected = false;
 
 	// TODO: We might want to keep track of selected dives in a more efficient way!
 	int i;
@@ -213,11 +209,6 @@ bool setSelection(const std::vector<dive *> &selection, dive *currentDive, int c
 			++amount_selected;
 			divesToSelect.push_back(d);
 		}
-		// TODO: Instead of using select_dive() and deselect_dive(), we set selected directly.
-		// The reason is that deselect() automatically sets a new current dive, which we
-		// don't want, as we set it later anyway.
-		// There is other parts of the C++ code that touches the innards directly, but
-		// ultimately this should be pushed down to C.
 		d->selected = newState;
 	}
 
@@ -232,18 +223,48 @@ bool setSelection(const std::vector<dive *> &selection, dive *currentDive, int c
 	if (currentDc >= 0)
 		dc_number = currentDc;
 	fixup_current_dc();
-
+	return divesToSelect;
-	// Send the new selection
-	emit diveListNotifier.divesSelected(divesToSelect);
-	return current_dive != oldCurrent;
 }
 
-bool setSelection(const std::vector<dive *> &selection)
+static void clear_trip_selection()
 {
+	amount_trips_selected = 0;
+	for (int i = 0; i < divelog.trips->nr; ++i)
+		divelog.trips->trips[i]->selected = false;
+}
+
+// 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 "selection" is empty).
+// If "currentDc" is negative, an attempt will be made to keep the current computer number.
+void setSelection(const std::vector<dive *> &selection, dive *currentDive, int currentDc)
+{
+	// Since we select only dives, there are no selected trips!
+	clear_trip_selection();
+
+	auto selectedDives = setSelectionCore(selection, currentDive, currentDc);
+
+	// Send the new selection to the UI.
+	emit diveListNotifier.divesSelected(selectedDives);
+}
+
+// Set selection, but try to keep the current dive. If current dive is not in selection,
+// find the nearest current dive in the selection
+// Returns true if the current dive changed.
+// Do not send a signal.
+bool setSelectionKeepCurrent(const std::vector<dive *> &selection)
+{
+	// Since we select only dives, there are no selected trips!
+	clear_trip_selection();
+
+	const dive *oldCurrent = current_dive;
+
 	dive *newCurrent = current_dive;
 	if (current_dive && std::find(selection.begin(), selection.end(), current_dive) == selection.end())
 		newCurrent = closestInSelection(current_dive->when, selection);
-	return setSelection(selection, newCurrent, -1);
+	setSelectionCore(selection, newCurrent, -1);
+
+	return current_dive != oldCurrent;
 }
 
 extern "C" void select_single_dive(dive *d)

core/selection.h

@@ -40,18 +40,24 @@ extern void dump_selection(void);
 
 #ifdef __cplusplus
 #include <vector>
+#include <QVector>
 
 // Reset the selection to the dives of the "selection" vector and send the appropriate signals.
 // Set the current dive to "currentDive" and the current dive computer to "currentDc".
 // "currentDive" must be an element of "selection" (or null if "seletion" is empty).
 // If "currentDc" is negative, an attempt will be made to keep the current computer number.
+// Returns the list of selected dives
+QVector<dive *> setSelectionCore(const std::vector<dive *> &selection, dive *currentDive, int currentDc);
+
+// As above, but sends a signal to inform the frontend of the changed selection.
 // Returns true if the current dive changed.
-bool setSelection(const std::vector<dive *> &selection, dive *currentDive, int currentDc);
+void setSelection(const std::vector<dive *> &selection, dive *currentDive, int currentDc);
 
 // Set selection, but try to keep the current dive. If current dive is not in selection,
 // find the nearest current dive in the selection
 // Returns true if the current dive changed.
-bool setSelection(const std::vector<dive *> &selection);
+// Does not send a signal.
+bool setSelectionKeepCurrent(const std::vector<dive *> &selection);
 
 // Get currently selected dives
 std::vector<dive *> getDiveSelection();

core/trip.c

@@ -287,26 +287,6 @@ dive_trip_t *get_dives_to_autogroup(struct dive_table *table, int start, int *fr
 	return NULL;
 }
 
-void deselect_dives_in_trip(struct dive_trip *trip)
-{
-	if (!trip)
-		return;
-	for (int i = 0; i < trip->dives.nr; ++i)
-		deselect_dive(trip->dives.dives[i]);
-}
-
-void select_dives_in_trip(struct dive_trip *trip)
-{
-	struct dive *dive;
-	if (!trip)
-		return;
-	for (int i = 0; i < trip->dives.nr; ++i) {
-		dive = trip->dives.dives[i];
-		if (!dive->hidden_by_filter)
-			select_dive(dive);
-	}
-}
-
 /* Out of two strings, copy the string that is not empty (if any). */
 static char *copy_non_empty_string(const char *a, const char *b)
 {

core/trip.h

@@ -49,9 +49,6 @@ extern dive_trip_t *get_trip_for_new_dive(struct dive *new_dive, bool *allocated
 extern dive_trip_t *get_trip_by_uniq_id(int tripId);
 extern bool trips_overlap(const struct dive_trip *t1, const struct dive_trip *t2);
 
-extern void select_dives_in_trip(struct dive_trip *trip);
-extern void deselect_dives_in_trip(struct dive_trip *trip);
-
 extern dive_trip_t *combine_trips(struct dive_trip *trip_a, struct dive_trip *trip_b);
 extern bool is_trip_before_after(const struct dive *dive, bool before);
 extern bool trip_is_single_day(const struct dive_trip *trip);

desktop-widgets/divelistview.cpp

@@ -548,17 +548,18 @@ void DiveListView::selectionChanged(const QItemSelection &selected, const QItemS
 
 	QItemSelection newSelected = selected.size() ? selected : selectionModel()->selection();
 
+	std::vector<dive *> addToSelection, removeFromSelection;
 	Q_FOREACH (const QModelIndex &index, newDeselected.indexes()) {
 		if (index.column() != 0)
 			continue;
 		const QAbstractItemModel *model = index.model();
 		struct dive *dive = model->data(index, DiveTripModelBase::DIVE_ROLE).value<struct dive *>();
-		if (!dive) { // it's a trip!
+		if (dive) {
-			dive_trip *trip = model->data(index, DiveTripModelBase::TRIP_ROLE).value<dive_trip *>();
+			removeFromSelection.push_back(dive);
+		} else if (dive_trip *trip = model->data(index, DiveTripModelBase::TRIP_ROLE).value<dive_trip *>()) {
 			deselect_trip(trip);
-			deselect_dives_in_trip(trip);
+			for (int i = 0; i < trip->dives.nr; ++i)
-		} else {
+				removeFromSelection.push_back(trip->dives.dives[i]);
-			deselect_dive(dive);
 		}
 	}
 	Q_FOREACH (const QModelIndex &index, newSelected.indexes()) {
@@ -567,10 +568,12 @@ void DiveListView::selectionChanged(const QItemSelection &selected, const QItemS
 
 		const QAbstractItemModel *model = index.model();
 		struct dive *dive = model->data(index, DiveTripModelBase::DIVE_ROLE).value<struct dive *>();
-		if (!dive) { // it's a trip!
+		if (dive) {
-			dive_trip *trip = model->data(index, DiveTripModelBase::TRIP_ROLE).value<dive_trip *>();
+			addToSelection.push_back(dive);
+		} else if (dive_trip *trip = model->data(index, DiveTripModelBase::TRIP_ROLE).value<dive_trip *>()) {
 			select_trip(trip);
-			select_dives_in_trip(trip);
+			for (int i = 0; i < trip->dives.nr; ++i)
+				addToSelection.push_back(trip->dives.dives[i]);
 			if (model->rowCount(index)) {
 				QItemSelection selection;
 				selection.select(model->index(0, 0, index), model->index(model->rowCount(index) - 1, 0, index));
@@ -579,11 +582,23 @@ void DiveListView::selectionChanged(const QItemSelection &selected, const QItemS
 				if (!isExpanded(index))
 					expand(index);
 			}
-		} else {
-			select_dive(dive);
 		}
 	}
 
+	// Funny: It can happen that dives were added to the add and remove lists.
+	// For example, when switching from a trip to a single dive in the trip.
+	// To avoid ill-define situations, clean that up.
+	for (dive *d: addToSelection) {
+		auto it = std::find(removeFromSelection.begin(), removeFromSelection.end(), d);
+		if (it != removeFromSelection.end())
+			removeFromSelection.erase(it);
+	}
+
+	std::vector<dive *> selection = getDiveSelection();
+	updateSelection(selection, addToSelection, removeFromSelection);
+	dive *newCurrent = selection.empty() ? nullptr : selection.front();
+	setSelectionCore(selection, newCurrent, -1);
+
 	// Display the new, processed, selection
 	QTreeView::selectionChanged(selectionModel()->selection(), newDeselected);
 }