Dive list view: replace signal-magic by flag

In DiveListView, we have a very fundamental problem: When
On the one hand, we get informed of user-selection in the
DiveListView::selectionChanged() slot. This has to set the
correct flags in the C-backend.

On the other hand, sometimes we have to set the selection
programatically, e.g. when selecting a trip. This is done
by calling QItemSelectionModel::select().

But: this will *also* call into the above slot, in which
we can't tell whether it was a user interaction or an
internal call. This can lead to either infinite loops or
very inefficient behavior, because the current dive
is set numerous times.

The current code is aware of that and disconnects the
corresponding signal. This is scary, as these signals are
set internally by the model and view. Replace this
by a global "command executing" flag in DiveListNotifier.
The flag is set using a "marker" class, which resets the flag
once it goes out of scope (cf. RAII pattern).

In DiveListView, only process a selection if the flag is not
set. Otherwise simply call the QTreeView base class, to reflect
the new selection in the UI.

To have a common point for notifications of selection changes,
add such a signal to DiveListNotifier. This signal will be
used by the DiveListView as well as the Command-objects.

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

6 files changed, 99 insertions, 27 deletions

diff --git a/core/subsurface-qt/DiveListNotifier.h b/core/subsurface-qt/DiveListNotifier.h
index 94b0063ea..fe5ea0e22 100644
--- a/core/subsurface-qt/DiveListNotifier.h
+++ b/core/subsurface-qt/DiveListNotifier.h
@@ -30,10 +30,69 @@ signals:
 	void divesChanged(dive_trip *trip, const QVector<dive *> &dives);
 	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);
+
+	// This signal is sent if the selection of dives and/or the current dive changed
+	void selectionChanged();
+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
+	// distinguish between user-initiated changes of the selection and changes
+	// that are due to actions of the Command-classes. To solve this problem,
+	// the frontend can use this function to query whether a dive list-modifying
+	// command is currently executed. If this function returns true, the
+	// frontend is supposed to not modify the selection.
+	bool inCommand() const;
+
+	// The following class and function are used by divelist-modifying commands
+	// to signalize that are in-flight. If the returned object goes out of scope,
+	// the command-in-flight status is reset to its previous value. Thus, the
+	// function can be called recursively.
+	class InCommandMarker {
+		DiveListNotifier &notifier;
+		bool oldValue;
+		InCommandMarker(DiveListNotifier &);
+		friend DiveListNotifier;
+	public:
+		~InCommandMarker();
+	};
+
+	// Usage:
+	// void doWork()
+	// {
+	// 	auto marker = diveListNotifier.enterCommand();
+	// 	... do work ...
+	// }
+	InCommandMarker enterCommand();
+private:
+	friend InCommandMarker;
+	bool commandExecuting;
 };
 
-// The DiveListNotifier class has no state and no constructor.
+// The DiveListNotifier class has only trivial state.
 // We can simply define it as a global object.
 extern DiveListNotifier diveListNotifier;
 
+// InCommandMarker is so trivial that the functions can be inlined.
+// TODO: perhaps move this into own header-file.
+inline DiveListNotifier::InCommandMarker::InCommandMarker(DiveListNotifier &notifierIn) : notifier(notifierIn),
+	oldValue(notifier.commandExecuting)
+{
+	notifier.commandExecuting = true;
+}
+
+inline DiveListNotifier::InCommandMarker::~InCommandMarker()
+{
+	notifier.commandExecuting = oldValue;
+}
+
+inline bool DiveListNotifier::inCommand() const
+{
+	return commandExecuting;
+}
+
+inline DiveListNotifier::InCommandMarker DiveListNotifier::enterCommand()
+{
+	return InCommandMarker(*this);
+}
+
 #endif
diff --git a/desktop-widgets/command_divelist.cpp b/desktop-widgets/command_divelist.cpp
index 17d5f00e4..a75004bea 100644
--- a/desktop-widgets/command_divelist.cpp
+++ b/desktop-widgets/command_divelist.cpp
@@ -323,6 +323,8 @@ bool AddDive::workToBeDone()
 
 void AddDive::redo()
 {
+	auto marker = diveListNotifier.enterCommand();
+
 	int idx = divesToAdd[0].idx;
 	divesToRemove = addDives(divesToAdd);
 	mark_divelist_changed(true);
@@ -338,6 +340,8 @@ void AddDive::redo()
 
 void AddDive::undo()
 {
+	auto marker = diveListNotifier.enterCommand();
+
 	// Simply remove the dive that was previously added
 	divesToAdd = removeDives(divesToRemove);
 
@@ -358,12 +362,14 @@ bool DeleteDive::workToBeDone()
 
 void DeleteDive::undo()
 {
+	auto marker = diveListNotifier.enterCommand();
 	divesToDelete = addDives(divesToAdd);
 	mark_divelist_changed(true);
 }
 
 void DeleteDive::redo()
 {
+	auto marker = diveListNotifier.enterCommand();
 	divesToAdd = removeDives(divesToDelete);
 	mark_divelist_changed(true);
 }
@@ -377,6 +383,7 @@ ShiftTime::ShiftTime(const QVector<dive *> &changedDives, int amount)
 
 void ShiftTime::redo()
 {
+	auto marker = diveListNotifier.enterCommand();
 	for (dive *d: diveList)
 		d->when -= timeChanged;
 
@@ -420,6 +427,7 @@ RenumberDives::RenumberDives(const QVector<QPair<dive *, int>> &divesToRenumberI
 
 void RenumberDives::undo()
 {
+	auto marker = diveListNotifier.enterCommand();
 	renumberDives(divesToRenumber);
 	mark_divelist_changed(true);
 }
@@ -442,6 +450,7 @@ bool TripBase::workToBeDone()
 
 void TripBase::redo()
 {
+	auto marker = diveListNotifier.enterCommand();
 	moveDivesBetweenTrips(divesToMove);
 
 	mark_divelist_changed(true);
@@ -554,8 +563,7 @@ bool SplitDives::workToBeDone()
 
 void SplitDives::redo()
 {
-	if (!diveToSplit)
-		return;
+	auto marker = diveListNotifier.enterCommand();
 	divesToUnsplit[0] = addDive(splitDives[0]);
 	divesToUnsplit[1] = addDive(splitDives[1]);
 	unsplitDive = removeDive(diveToSplit);
@@ -564,9 +572,8 @@ void SplitDives::redo()
 
 void SplitDives::undo()
 {
-	if (!unsplitDive.dive)
-		return;
 	// Note: reverse order with respect to redo()
+	auto marker = diveListNotifier.enterCommand();
 	diveToSplit = addDive(unsplitDive);
 	splitDives[1] = removeDive(divesToUnsplit[1]);
 	splitDives[0] = removeDive(divesToUnsplit[0]);
@@ -659,6 +666,7 @@ bool MergeDives::workToBeDone()
 
 void MergeDives::redo()
 {
+	auto marker = diveListNotifier.enterCommand();
 	renumberDives(divesToRenumber);
 	diveToUnmerge = addDive(mergedDive);
 	unmergedDives = removeDives(divesToMerge);
@@ -666,6 +674,7 @@ void MergeDives::redo()
 
 void MergeDives::undo()
 {
+	auto marker = diveListNotifier.enterCommand();
 	divesToMerge = addDives(unmergedDives);
 	mergedDive = removeDive(diveToUnmerge);
 	renumberDives(divesToRenumber);
diff --git a/desktop-widgets/divelistview.cpp b/desktop-widgets/divelistview.cpp
index ebeb1f6ef..2a0d7028c 100644
--- a/desktop-widgets/divelistview.cpp
+++ b/desktop-widgets/divelistview.cpp
@@ -24,6 +24,7 @@
 #include "desktop-widgets/divelistview.h"
 #include "qt-models/divepicturemodel.h"
 #include "core/metrics.h"
+#include "core/subsurface-qt/DiveListNotifier.h"
 
 DiveListView::DiveListView(QWidget *parent) : QTreeView(parent), mouseClickSelection(false), sortColumn(0),
 	currentOrder(Qt::DescendingOrder), dontEmitDiveChangedSignal(false), selectionSaved(false),
@@ -267,19 +268,17 @@ void DiveListView::selectTrip(dive_trip_t *trip)
 // works as expected
 void DiveListView::clearTripSelection()
 {
-	// we want to make sure no trips are selected
-	disconnect(selectionModel(), SIGNAL(selectionChanged(QItemSelection, QItemSelection)), this, SLOT(selectionChanged(QItemSelection, QItemSelection)));
-	disconnect(selectionModel(), SIGNAL(currentChanged(QModelIndex, QModelIndex)), this, SLOT(currentChanged(QModelIndex, QModelIndex)));
+	// This marks the selection change as being internal - ie. we don't process it further.
+	// TODO: This should probably be sold differently.
+	auto marker = diveListNotifier.enterCommand();
 
+	// we want to make sure no trips are selected
 	Q_FOREACH (const QModelIndex &index, selectionModel()->selectedRows()) {
 		dive_trip_t *trip = static_cast<dive_trip_t *>(index.data(DiveTripModel::TRIP_ROLE).value<void *>());
 		if (!trip)
 			continue;
 		selectionModel()->select(index, QItemSelectionModel::Deselect);
 	}
-
-	connect(selectionModel(), SIGNAL(selectionChanged(QItemSelection, QItemSelection)), this, SLOT(selectionChanged(QItemSelection, QItemSelection)));
-	connect(selectionModel(), SIGNAL(currentChanged(QModelIndex, QModelIndex)), this, SLOT(currentChanged(QModelIndex, QModelIndex)));
 }
 
 void DiveListView::unselectDives()
@@ -374,7 +373,7 @@ void DiveListView::selectDives(const QList<int> &newDiveSelection)
 		scrollTo(idx);
 	}
 	// now that everything is up to date, update the widgets
-	emit currentDiveChanged();
+	emit diveListNotifier.selectionChanged();
 	dontEmitDiveChangedSignal = false;
 	return;
 }
@@ -545,11 +544,20 @@ void DiveListView::currentChanged(const QModelIndex &current, const QModelIndex&
 
 void DiveListView::selectionChanged(const QItemSelection &selected, const QItemSelection &deselected)
 {
-	QItemSelection newSelected = selected.size() ? selected : selectionModel()->selection();
-	QItemSelection newDeselected = deselected;
+	if (diveListNotifier.inCommand()) {
+		// This is a programmatical change of the selection.
+		// Call the QTreeView base function to reflect the selection in the display,
+		// but don't process it any further.
+		QTreeView::selectionChanged(selected, deselected);
+		return;
+	}
 
-	disconnect(selectionModel(), SIGNAL(selectionChanged(QItemSelection, QItemSelection)), this, SLOT(selectionChanged(QItemSelection, QItemSelection)));
-	disconnect(selectionModel(), SIGNAL(currentChanged(QModelIndex, QModelIndex)), this, SLOT(currentChanged(QModelIndex, QModelIndex)));
+	// This is a manual selection change. This means that the core does not yet know
+	// of the new selection. Update the core-structures accordingly and select/deselect
+	// all dives of a trip if a trip is selected/deselected.
+	const QItemSelection &newDeselected = deselected;
+
+	QItemSelection newSelected = selected.size() ? selected : selectionModel()->selection();
 
 	Q_FOREACH (const QModelIndex &index, newDeselected.indexes()) {
 		if (index.column() != 0)
@@ -581,11 +589,11 @@ void DiveListView::selectionChanged(const QItemSelection &selected, const QItemS
 			select_dive(get_divenr(dive));
 		}
 	}
-	QTreeView::selectionChanged(selectionModel()->selection(), newDeselected);
-	connect(selectionModel(), SIGNAL(selectionChanged(QItemSelection, QItemSelection)), this, SLOT(selectionChanged(QItemSelection, QItemSelection)));
-	connect(selectionModel(), SIGNAL(currentChanged(QModelIndex, QModelIndex)), this, SLOT(currentChanged(QModelIndex, QModelIndex)));
 	if (!dontEmitDiveChangedSignal)
-		emit currentDiveChanged();
+		emit diveListNotifier.selectionChanged();
+
+	// Display the new, processed, selection
+	QTreeView::selectionChanged(selectionModel()->selection(), newDeselected);
 }
 
 enum asked_user {NOTYET, MERGE, DONTMERGE};
diff --git a/desktop-widgets/divelistview.h b/desktop-widgets/divelistview.h
index 2d7699d1d..9e6857312 100644
--- a/desktop-widgets/divelistview.h
+++ b/desktop-widgets/divelistview.h
@@ -58,10 +58,6 @@ slots:
 	void shiftTimes();
 	void loadImages();
 	void loadWebImages();
-
-signals:
-	void currentDiveChanged();
-
 private:
 	bool mouseClickSelection;
 	QList<int> expandedRows;
diff --git a/desktop-widgets/mainwindow.cpp b/desktop-widgets/mainwindow.cpp
index 376d2e7f3..5ec086195 100644
--- a/desktop-widgets/mainwindow.cpp
+++ b/desktop-widgets/mainwindow.cpp
@@ -209,7 +209,7 @@ MainWindow::MainWindow() : QMainWindow(),
 	if (!QIcon::hasThemeIcon("window-close")) {
 		QIcon::setThemeName("subsurface");
 	}
-	connect(dive_list(), &DiveListView::currentDiveChanged, this, &MainWindow::current_dive_changed);
+	connect(&diveListNotifier, &DiveListNotifier::selectionChanged, this, &MainWindow::selectionChanged);
 	connect(PreferencesDialog::instance(), SIGNAL(settingsChanged()), this, SLOT(readSettings()));
 	connect(PreferencesDialog::instance(), SIGNAL(settingsChanged()), diveListView, SLOT(update()));
 	connect(PreferencesDialog::instance(), SIGNAL(settingsChanged()), diveListView, SLOT(reloadHeaderActions()));
@@ -528,7 +528,7 @@ void MainWindow::configureToolbar() {
 	}
 }
 
-void MainWindow::current_dive_changed()
+void MainWindow::selectionChanged()
 {
 	graphics()->plotDive(nullptr, false, true);
 	information()->updateDiveInfo();
diff --git a/desktop-widgets/mainwindow.h b/desktop-widgets/mainwindow.h
index 2f2b91a83..ae3b081a4 100644
--- a/desktop-widgets/mainwindow.h
+++ b/desktop-widgets/mainwindow.h
@@ -132,7 +132,7 @@ slots:
 	void on_action_Check_for_Updates_triggered();
 
 	void on_actionDiveSiteEdit_triggered();
-	void current_dive_changed();
+	void selectionChanged();
 	void initialUiSetup();
 
 	void on_actionImportDiveLog_triggered();