Undo: unify selection behavior in dive-list commands

Some commands tried to retain the current selection on undo/redo,
others set the selection to the modified dives.

The latter was introduced because it was easier in some cases, but
it is probably more user-friendly because the user gets feedback
on the change.

Therefore, unify to always select the affected dives on undo()/redo().

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

7 files changed, 48 insertions, 56 deletions

diff --git a/core/subsurface-qt/DiveListNotifier.h b/core/subsurface-qt/DiveListNotifier.h
index 1d16652bd..7f97cbf8c 100644
--- a/core/subsurface-qt/DiveListNotifier.h
+++ b/core/subsurface-qt/DiveListNotifier.h
@@ -53,13 +53,12 @@ signals:
 	void tripChanged(dive_trip *trip, TripField field);
 
 	// Selection-signals come in two kinds:
-	//  - divesSelected, divesDeselected and currentDiveChanged are are used by the dive-list
+	//  - divesSelected and currentDiveChanged are are used by the dive-list
 	//    model and view to correctly highlight the correct dives.
 	//  - selectionChanged() is called once at the end of commands if either the selection
 	//    or the current dive changed. It is used by the main-window / profile to update
 	//    their data.
 	void divesSelected(const QVector<dive *> &dives);
-	void divesDeselected(const QVector<dive *> &dives);
 	void currentDiveChanged();
 	void selectionChanged();
 
diff --git a/desktop-widgets/command_divelist.cpp b/desktop-widgets/command_divelist.cpp
index 4c699fdd6..26708e07d 100644
--- a/desktop-widgets/command_divelist.cpp
+++ b/desktop-widgets/command_divelist.cpp
@@ -614,6 +614,9 @@ void ShiftTime::redoit()
 	emit diveListNotifier.divesTimeChanged(timeChanged, diveList);
 	emit diveListNotifier.divesChanged(diveList, DiveField::DATETIME);
 
+	// Select the changed dives
+	restoreSelection(diveList.toStdVector(), diveList[0]);
+
 	// Negate the time-shift so that the next call does the reverse
 	timeChanged = -timeChanged;
 }
@@ -638,6 +641,13 @@ RenumberDives::RenumberDives(const QVector<QPair<dive *, int>> &divesToRenumberI
 void RenumberDives::undoit()
 {
 	renumberDives(divesToRenumber);
+
+	// Select the changed dives
+	std::vector<dive *> dives;
+	dives.reserve(divesToRenumber.size());
+	for (const QPair<dive *, int> &item: divesToRenumber)
+		dives.push_back(item.first);
+	restoreSelection(dives, dives[0]);
 }
 
 bool RenumberDives::workToBeDone()
@@ -660,6 +670,13 @@ void TripBase::redoit()
 {
 	moveDivesBetweenTrips(divesToMove);
 	sort_trip_table(&trip_table); // Though unlikely, moving dives may reorder trips
+
+	// Select the moved dives
+	std::vector<dive *> dives;
+	dives.reserve(divesToMove.divesToMove.size());
+	for (const DiveToTrip &item: divesToMove.divesToMove)
+		dives.push_back(item.dive);
+	restoreSelection(dives, dives[0]);
 }
 
 void TripBase::undoit()
diff --git a/desktop-widgets/command_private.cpp b/desktop-widgets/command_private.cpp
index 7e604dfc9..6dac645b5 100644
--- a/desktop-widgets/command_private.cpp
+++ b/desktop-widgets/command_private.cpp
@@ -47,12 +47,13 @@ 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.
 	QVector<dive *> divesToSelect;
-	QVector<dive *> divesToDeselect;
+	divesToSelect.reserve(selection.size());
 
 	// TODO: We might want to keep track of selected dives in a more efficient way!
 	int i;
 	dive *d;
 	amount_selected = 0; // We recalculate amount_selected
+	bool selectionChanged = false;
 	for_each_dive(i, d) {
 		// We only modify dives that are currently visible.
 		if (d->hidden_by_filter) {
@@ -65,24 +66,21 @@ bool setSelection(const std::vector<dive *> &selection, dive *currentDive)
 		// TODO: By sorting the list in the same way as the backend, this could be made more efficient.
 		bool newState = std::find(selection.begin(), selection.end(), d) != selection.end();
 
+		if (newState) {
+			++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.
-		if (newState && !d->selected) {
-			d->selected = true;
-			++amount_selected;
-			divesToSelect.push_back(d);
-		} else if (!newState && d->selected) {
-			d->selected = false;
-			divesToDeselect.push_back(d);
-		}
+		selectionChanged |= d->selected != newState;
+		d->selected = newState;
 	}
 
 	// Send the select and deselect signals
 	emit diveListNotifier.divesSelected(divesToSelect);
-	emit diveListNotifier.divesDeselected(divesToDeselect);
 
 	bool currentDiveChanged = false;
 	if (!currentDive) {
@@ -105,7 +103,7 @@ bool setSelection(const std::vector<dive *> &selection, dive *currentDive)
 	}
 
 	// return true if selection of current dive changed
-	return !divesToSelect.empty() || !divesToDeselect.empty() || currentDiveChanged;
+	return selectionChanged || currentDiveChanged;
 }
 
 // Turn current selection into a vector.
diff --git a/desktop-widgets/divelistview.cpp b/desktop-widgets/divelistview.cpp
index 055737252..c010c0010 100644
--- a/desktop-widgets/divelistview.cpp
+++ b/desktop-widgets/divelistview.cpp
@@ -192,13 +192,11 @@ void DiveListView::reset()
 }
 
 // If items were selected, inform the selection model
-void DiveListView::diveSelectionChanged(const QVector<QModelIndex> &indexes, bool select)
+void DiveListView::diveSelectionChanged(const QVector<QModelIndex> &indexes)
 {
+	clearSelection();
 	MultiFilterSortModel *m = MultiFilterSortModel::instance();
 	QItemSelectionModel *s = selectionModel();
-	auto flags = select ?
-		QItemSelectionModel::Rows | QItemSelectionModel::Select :
-		QItemSelectionModel::Rows | QItemSelectionModel::Deselect;
 	for (const QModelIndex &index: indexes) {
 		// We have to transform the indices into local indices, since
 		// there might be sorting or filtering in effect.
@@ -210,10 +208,10 @@ void DiveListView::diveSelectionChanged(const QVector<QModelIndex> &indexes, boo
 		if (!localIndex.isValid())
 			continue;
 
-		s->select(localIndex, flags);
+		s->select(localIndex, QItemSelectionModel::Rows | QItemSelectionModel::Select);
 
 		// If an item of a not-yet expanded trip is selected, expand the trip.
-		if (select && localIndex.parent().isValid() && !isExpanded(localIndex.parent())) {
+		if (localIndex.parent().isValid() && !isExpanded(localIndex.parent())) {
 			setAnimated(false);
 			expand(localIndex.parent());
 			setAnimated(true);
diff --git a/desktop-widgets/divelistview.h b/desktop-widgets/divelistview.h
index 7a3757adb..abf9ce8a2 100644
--- a/desktop-widgets/divelistview.h
+++ b/desktop-widgets/divelistview.h
@@ -63,7 +63,7 @@ slots:
 	void shiftTimes();
 	void loadImages();
 	void loadWebImages();
-	void diveSelectionChanged(const QVector<QModelIndex> &indexes, bool select);
+	void diveSelectionChanged(const QVector<QModelIndex> &indexes);
 	void currentDiveChanged(QModelIndex index);
 	void filterFinished();
 	void tripChanged(dive_trip *trip, TripField);
diff --git a/qt-models/divetripmodel.cpp b/qt-models/divetripmodel.cpp
index e15d5d0fd..a0aa1f5ba 100644
--- a/qt-models/divetripmodel.cpp
+++ b/qt-models/divetripmodel.cpp
@@ -417,16 +417,6 @@ bool DiveTripModelBase::setData(const QModelIndex &index, const QVariant &value,
 	return true;
 }
 
-void DiveTripModelBase::divesSelected(const QVector<dive *> &dives)
-{
-	changeDiveSelection(dives, true);
-}
-
-void DiveTripModelBase::divesDeselected(const QVector<dive *> &dives)
-{
-	changeDiveSelection(dives, false);
-}
-
 // Find a range of matching elements in a vector.
 // Input parameters:
 //	v: vector to be searched
@@ -561,7 +551,6 @@ DiveTripModelTree::DiveTripModelTree(QObject *parent) : DiveTripModelBase(parent
 	connect(&diveListNotifier, &DiveListNotifier::divesMovedBetweenTrips, this, &DiveTripModelTree::divesMovedBetweenTrips);
 	connect(&diveListNotifier, &DiveListNotifier::divesTimeChanged, this, &DiveTripModelTree::divesTimeChanged);
 	connect(&diveListNotifier, &DiveListNotifier::divesSelected, this, &DiveTripModelTree::divesSelected);
-	connect(&diveListNotifier, &DiveListNotifier::divesDeselected, this, &DiveTripModelTree::divesDeselected);
 	connect(&diveListNotifier, &DiveListNotifier::currentDiveChanged, this, &DiveTripModelTree::currentDiveChanged);
 	connect(&diveListNotifier, &DiveListNotifier::tripChanged, this, &DiveTripModelTree::tripChanged);
 
@@ -1037,7 +1026,6 @@ void DiveTripModelTree::divesMovedBetweenTrips(dive_trip *from, dive_trip *to, b
 	QVector<dive *> selectedDives = filterSelectedDives(dives);
 	divesAdded(to, createTo, dives);
 	divesDeleted(from, deleteFrom, dives);
-	changeDiveSelectionTrip(to, dives, true);
 }
 
 void DiveTripModelTree::divesTimeChanged(timestamp_t delta, const QVector<dive *> &dives)
@@ -1061,22 +1049,23 @@ void DiveTripModelTree::divesTimeChangedTrip(dive_trip *trip, timestamp_t delta,
 	QVector<dive *> selectedDives = filterSelectedDives(dives);
 	divesDeleted(trip, false, dives);
 	divesAdded(trip, false, dives);
-	changeDiveSelectionTrip(trip, selectedDives, true);
-}
-
-void DiveTripModelTree::changeDiveSelection(const QVector<dive *> &dives, bool select)
-{
-	processByTrip(dives, [this, select] (dive_trip *trip, const QVector<dive *> &divesInTrip)
-		      { changeDiveSelectionTrip(trip, divesInTrip, select); });
 }
 
-void DiveTripModelTree::changeDiveSelectionTrip(dive_trip *trip, const QVector<dive *> &dives, bool select)
+void DiveTripModelTree::divesSelected(const QVector<dive *> &dives)
 {
 	// We got a number of dives that have been selected. Turn this into QModelIndexes and
 	// emit a signal, so that views can change the selection.
 	QVector<QModelIndex> indexes;
 	indexes.reserve(dives.count());
 
+	processByTrip(dives, [this, &indexes] (dive_trip *trip, const QVector<dive *> &divesInTrip)
+		      { divesSelectedTrip(trip, divesInTrip, indexes); });
+
+	emit selectionChanged(indexes);
+}
+
+void DiveTripModelTree::divesSelectedTrip(dive_trip *trip, const QVector<dive *> &dives, QVector<QModelIndex> &indexes)
+{
 	if (!trip) {
 		// This is at the top level.
 		// Since both lists are sorted, we can do this linearly. Perhaps a binary search
@@ -1095,7 +1084,7 @@ void DiveTripModelTree::changeDiveSelectionTrip(dive_trip *trip, const QVector<d
 		if (idx < 0) {
 			// We don't know the trip - this shouldn't happen. We seem to have
 			// missed some signals!
-			qWarning() << "DiveTripModelTree::changeDiveSelection(): unknown trip";
+			qWarning() << "DiveTripModelTree::diveSelectedTrip(): unknown trip";
 			return;
 		}
 		// Locate the indices inside the trip.
@@ -1111,8 +1100,6 @@ void DiveTripModelTree::changeDiveSelectionTrip(dive_trip *trip, const QVector<d
 			indexes.append(createIndex(j, 0, idx));
 		}
 	}
-
-	emit selectionChanged(indexes, select);
 }
 
 void DiveTripModelTree::currentDiveChanged()
@@ -1182,7 +1169,6 @@ DiveTripModelList::DiveTripModelList(QObject *parent) : DiveTripModelBase(parent
 	//connect(&diveListNotifier, &DiveListNotifier::divesMovedBetweenTrips, this, &DiveTripModelList::divesMovedBetweenTrips);
 	connect(&diveListNotifier, &DiveListNotifier::divesTimeChanged, this, &DiveTripModelList::divesTimeChanged);
 	connect(&diveListNotifier, &DiveListNotifier::divesSelected, this, &DiveTripModelList::divesSelected);
-	connect(&diveListNotifier, &DiveListNotifier::divesDeselected, this, &DiveTripModelList::divesDeselected);
 	connect(&diveListNotifier, &DiveListNotifier::currentDiveChanged, this, &DiveTripModelList::currentDiveChanged);
 
 	// Fill model
@@ -1302,7 +1288,7 @@ void DiveTripModelList::divesTimeChanged(timestamp_t delta, const QVector<dive *
 	divesSelected(selectedDives);
 }
 
-void DiveTripModelList::changeDiveSelection(const QVector<dive *> &dives, bool select)
+void DiveTripModelList::divesSelected(const QVector<dive *> &dives)
 {
 	// We got a number of dives that have been selected. Turn this into QModelIndexes and
 	// emit a signal, so that views can change the selection.
@@ -1320,7 +1306,7 @@ void DiveTripModelList::changeDiveSelection(const QVector<dive *> &dives, bool s
 		indexes.append(createIndex(j, 0, noParent));
 	}
 
-	emit selectionChanged(indexes, select);
+	emit selectionChanged(indexes);
 }
 
 void DiveTripModelList::currentDiveChanged()
diff --git a/qt-models/divetripmodel.h b/qt-models/divetripmodel.h
index 8903cb0c1..734612ff7 100644
--- a/qt-models/divetripmodel.h
+++ b/qt-models/divetripmodel.h
@@ -89,19 +89,13 @@ signals:
 	// into QModelIndexes according to the current view (tree/list). Finally, the DiveListView transforms these
 	// indexes into local indexes according to current sorting/filtering and instructs the QSelectionModel to
 	// perform the appropriate actions.
-	void selectionChanged(const QVector<QModelIndex> &indexes, bool select);
+	void selectionChanged(const QVector<QModelIndex> &indexes);
 	void newCurrentDive(QModelIndex index);
-protected slots:
-	void divesSelected(const QVector<dive *> &dives);
-	void divesDeselected(const QVector<dive *> &dives);
 protected:
 	// Access trip and dive data
 	static QVariant diveData(const struct dive *d, int column, int role);
 	static QVariant tripData(const dive_trip *trip, int column, int role);
 
-	// Select or deselect dives
-	virtual void changeDiveSelection(const QVector<dive *> &dives, bool select) = 0;
-
 	virtual dive *diveOrNull(const QModelIndex &index) const = 0;	// Returns a dive if this index represents a dive, null otherwise
 };
 
@@ -114,6 +108,7 @@ public slots:
 	void divesMovedBetweenTrips(dive_trip *from, dive_trip *to, bool deleteFrom, bool createTo, const QVector<dive *> &dives);
 	void divesChanged(const QVector<dive *> &dives);
 	void divesTimeChanged(timestamp_t delta, const QVector<dive *> &dives);
+	void divesSelected(const QVector<dive *> &dives);
 	void currentDiveChanged();
 	void tripChanged(dive_trip *trip, TripField);
 
@@ -126,8 +121,7 @@ private:
 	QVariant data(const QModelIndex &index, int role) const override;
 	void filterFinished() override;
 	bool lessThan(const QModelIndex &i1, const QModelIndex &i2) const override;
-	void changeDiveSelection(const QVector<dive *> &dives, bool select) override;
-	void changeDiveSelectionTrip(dive_trip *trip, const QVector<dive *> &dives, bool select);
+	void divesSelectedTrip(dive_trip *trip, const QVector<dive *> &dives, QVector<QModelIndex> &);
 	dive *diveOrNull(const QModelIndex &index) const override;
 	bool setShown(const QModelIndex &idx, bool shown);
 	void divesChangedTrip(dive_trip *trip, const QVector<dive *> &dives);
@@ -181,6 +175,7 @@ public slots:
 	void divesTimeChanged(timestamp_t delta, const QVector<dive *> &dives);
 	// Does nothing in list view.
 	//void divesMovedBetweenTrips(dive_trip *from, dive_trip *to, bool deleteFrom, bool createTo, const QVector<dive *> &dives);
+	void divesSelected(const QVector<dive *> &dives);
 	void currentDiveChanged();
 
 public:
@@ -192,7 +187,6 @@ private:
 	QVariant data(const QModelIndex &index, int role) const override;
 	void filterFinished() override;
 	bool lessThan(const QModelIndex &i1, const QModelIndex &i2) const override;
-	void changeDiveSelection(const QVector<dive *> &dives, bool select) override;
 	dive *diveOrNull(const QModelIndex &index) const override;
 	bool setShown(const QModelIndex &idx, bool shown);