Filter: move recalculation of filter from FilterModel to TripModel

The way this was accessed via Qt's model semantics was horrible.
This gives arguably more readable code, since we don't have to
shoehorn things through QVariants.

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

core/divefilter.cpp

@@ -193,7 +193,7 @@ void DiveFilter::startFilterDiveSites(QVector<dive_site *> ds)
 		// When switching into dive site mode, reload the dive sites.
 		// We won't do this in myInvalidate() once we are in dive site mode.
 		MapWidget::instance()->reload();
-		MultiFilterSortModel::instance()->myInvalidate();
+		DiveTripModelBase::instance()->recalculateFilter();
 	}
 }
 
@@ -202,7 +202,7 @@ void DiveFilter::stopFilterDiveSites()
 	if (--diveSiteRefCount > 0)
 		return;
 	dive_sites.clear();
-	MultiFilterSortModel::instance()->myInvalidate();
+	DiveTripModelBase::instance()->recalculateFilter();
 	MapWidget::instance()->reload();
 }
 
@@ -215,7 +215,7 @@ void DiveFilter::setFilterDiveSite(QVector<dive_site *> ds)
 		return;
 	dive_sites = ds;
 
-	MultiFilterSortModel::instance()->myInvalidate();
+	DiveTripModelBase::instance()->recalculateFilter();
 	MapWidget::instance()->setSelected(dive_sites);
 	MainWindow::instance()->diveList->expandAll();
 }
@@ -233,6 +233,6 @@ bool DiveFilter::diveSiteMode() const
 void DiveFilter::setFilter(const FilterData &data)
 {
 	filterData = data;
-	MultiFilterSortModel::instance()->myInvalidate();
+	DiveTripModelBase::instance()->recalculateFilter();
 }
 #endif // SUBSURFACE_MOBILE

core/subsurface-qt/DiveListNotifier.h

@@ -94,6 +94,7 @@ signals:
 
 	// Filter-related signals
 	void numShownChanged();
+	void filterReset();
 
 	// This signal is emited every time a command is executed.
 	// This is used to hide an old multi-dives-edited warning message.

desktop-widgets/divelistview.cpp

@@ -47,7 +47,7 @@ DiveListView::DiveListView(QWidget *parent) : QTreeView(parent), mouseClickSelec
 	resetModel();
 
 	// Update selection if all selected dives were hidden by filter
-	connect(MultiFilterSortModel::instance(), &MultiFilterSortModel::filterFinished, this, &DiveListView::filterFinished);
+	connect(&diveListNotifier, &DiveListNotifier::filterReset, this, &DiveListView::filterFinished);
 
 	connect(&diveListNotifier, &DiveListNotifier::tripChanged, this, &DiveListView::tripChanged);
 

desktop-widgets/mainwindow.cpp

@@ -423,7 +423,7 @@ void MainWindow::refreshDisplay(bool doRecreateDiveList)
 void MainWindow::recreateDiveList()
 {
 	diveList->reload();
-	MultiFilterSortModel::instance()->myInvalidate();
+	DiveTripModelBase::instance()->recalculateFilter();
 }
 
 void MainWindow::configureToolbar()

qt-models/divetripmodel.cpp

@@ -1,6 +1,5 @@
 // SPDX-License-Identifier: GPL-2.0
 #include "qt-models/divetripmodel.h"
-#include "qt-models/filtermodels.h"
 #include "core/divefilter.h"
 #include "core/gettextfromc.h"
 #include "core/metrics.h"
@@ -400,9 +399,6 @@ Qt::ItemFlags DiveTripModelBase::flags(const QModelIndex &index) const
 
 bool DiveTripModelBase::setData(const QModelIndex &index, const QVariant &value, int role)
 {
-	if (role == SHOWN_ROLE)
-		return setShown(index, value.value<bool>());
-
 	// We only support setting of data for dives and there, only the number.
 	dive *d = diveOrNull(index);
 	if (!d)
@@ -688,34 +684,49 @@ dive *DiveTripModelTree::diveOrNull(const QModelIndex &index) const
 	return tripOrDive(index).dive;
 }
 
-// Set the shown flag that marks whether an entry is shown or hidden by the filter.
-// The flag is cached for top-level items (trips and dives outside of trips).
-// For dives that belong to a trip (i.e. non-top-level items), the flag is
-// simply written through to the core.
-bool DiveTripModelTree::setShown(const QModelIndex &idx, bool shown)
+void DiveTripModelTree::recalculateFilter()
 {
-	if (!idx.isValid())
-		return false;
-
-	QModelIndex parent = idx.parent();
-	if (!parent.isValid()) {
-		// An invalid parent means that we're at the top-level
-		Item &item = items[idx.row()];
-		item.shown = shown; // Cache the flag.
-		if (item.d_or_t.dive) {
-			// This is a dive -> also register the flag in the core
-			filter_dive(item.d_or_t.dive, shown);
+	{
+		// This marker prevents the UI from getting notifications on selection changes.
+		// It is active until the end of the scope.
+		// This was actually designed for the undo-commands, so that they can do their work
+		// without having the UI updated.
+		// Here, it is used because invalidating the filter can generate numerous
+		// selection changes, which do full ui reloads. Instead, do that all at once
+		// as a consequence of the filterReset signal right after the local scope.
+		auto marker = diveListNotifier.enterCommand();
+		DiveFilter *filter = DiveFilter::instance();
+		for (Item &item: items) {
+			if (item.d_or_t.dive) {
+				dive *d = item.d_or_t.dive;
+				item.shown = filter->showDive(item.d_or_t.dive);
+				filter_dive(d, item.shown);
+			} else {
+				// Trips are shown if any of the dives is shown
+				bool showTrip = false;
+				for (dive *d: item.dives) {
+					bool shown = filter->showDive(d);
+					filter_dive(d, shown);
+					showTrip |= shown;
+				}
+				item.shown = showTrip;
+			}
 		}
-	} else {
-		// We're not at the top-level. This must be a dive, therefore
-		// simply write the flag through to the core.
-		const Item &parentItem = items[parent.row()];
-		filter_dive(parentItem.dives[idx.row()], shown);
 	}
 
-	return true;
+
+	// Rerender all trip headers. TODO: be smarter about this and only rerender if the number
+	// of shown dives changed.
+	for (int idx = 0; idx < (int)items.size(); ++idx) {
+		QModelIndex tripIndex = createIndex(idx, 0, noParent);
+		dataChanged(tripIndex, tripIndex);
+	}
+
+	emit diveListNotifier.numShownChanged();
+	emit diveListNotifier.filterReset();
 }
 
+
 QVariant DiveTripModelTree::data(const QModelIndex &index, int role) const
 {
 	if (role == SHOWN_ROLE) {
@@ -980,13 +991,23 @@ void DiveTripModelTree::divesChanged(const QVector<dive *> &dives)
 		      { divesChangedTrip(trip, divesInTrip); });
 }
 
+// Update visibility status of dive and return true if visibility changed
+static bool updateShown(const QVector<dive *> &dives)
+{
+	bool changed = false;
+	DiveFilter *filter = DiveFilter::instance();
+	for (dive *d: dives) {
+		bool newStatus = filter->showDive(d);
+		changed |= filter_dive(d, newStatus);
+	}
+	if (changed)
+		emit diveListNotifier.numShownChanged();
+	return changed;
+}
+
 void DiveTripModelTree::divesChangedTrip(dive_trip *trip, const QVector<dive *> &dives)
 {
-	// Update filter flags. TODO: The filter should update the flag by itself when
-	// recieving the signals below.
-	bool diveChanged = false;
-	for (dive *d: dives)
-		diveChanged |= MultiFilterSortModel::instance()->updateDive(d);
+	bool diveChanged = updateShown(dives);
 
 	if (!trip) {
 		// This is outside of a trip. Process top-level items range-wise.
@@ -1182,16 +1203,6 @@ void DiveTripModelTree::divesSelectedTrip(dive_trip *trip, const QVector<dive *>
 	}
 }
 
-void DiveTripModelTree::filterFinished()
-{
-	// If the filter finished, update all trip items to show the correct number of displayed dives
-	// in each trip. Without doing this, only trip headers of expanded trips were updated.
-	for (int idx = 0; idx < (int)items.size(); ++idx) {
-		QModelIndex tripIndex = createIndex(idx, 0, noParent);
-		dataChanged(tripIndex, tripIndex);
-	}
-}
-
 bool DiveTripModelTree::lessThan(const QModelIndex &i1, const QModelIndex &i2) const
 {
 	// In tree mode we don't support any sorting!
@@ -1248,13 +1259,22 @@ dive *DiveTripModelList::diveOrNull(const QModelIndex &index) const
 	return items[row];
 }
 
-bool DiveTripModelList::setShown(const QModelIndex &idx, bool shown)
+void DiveTripModelList::recalculateFilter()
 {
-	dive *d = diveOrNull(idx);
-	if (!d)
-		return false;
-	filter_dive(d, shown);
-	return true;
+	{
+		// This marker prevents the UI from getting notifications on selection changes.
+		// It is active until the end of the scope. See comment in DiveTripModelTree::recalculateFilter().
+		auto marker = diveListNotifier.enterCommand();
+		DiveFilter *filter = DiveFilter::instance();
+
+		for (dive *d: items) {
+			bool shown = filter->showDive(d);
+			filter_dive(d, shown);
+		}
+	}
+
+	emit diveListNotifier.numShownChanged();
+	emit diveListNotifier.filterReset();
 }
 
 QVariant DiveTripModelList::data(const QModelIndex &index, int role) const
@@ -1308,10 +1328,7 @@ void DiveTripModelList::divesChanged(const QVector<dive *> &divesIn)
 	QVector<dive *> dives = divesIn;
 	std::sort(dives.begin(), dives.end(), dive_less_than);
 
-	// Update filter flags. TODO: The filter should update the flag by itself when
-	// recieving the signals below.
-	for (dive *d: dives)
-		MultiFilterSortModel::instance()->updateDive(d);
+	updateShown(dives);
 
 	// Since we know that the dive list is sorted, we will only ever search for the first element
 	// in dives as this must be the first that we encounter. Once we find a range, increase the
@@ -1372,11 +1389,6 @@ void DiveTripModelList::divesSelected(const QVector<dive *> &dives, dive *curren
 	emit newCurrentDive(createIndex(it - items.begin(), 0));
 }
 
-void DiveTripModelList::filterFinished()
-{
-	// In list mode, we don't have to change anything after filter finished.
-}
-
 // Simple sorting helper for sorting against a criterium and if
 // that is undefined against a different criterium.
 // Return true if diff1 < 0, false if diff1 > 0.

qt-models/divetripmodel.h

@@ -78,8 +78,7 @@ public:
 	bool setData(const QModelIndex &index, const QVariant &value, int role = Qt::EditRole) override;
 	DiveTripModelBase(QObject *parent = 0);
 	int columnCount(const QModelIndex&) const;
-	virtual void filterFinished() = 0;
-	virtual bool setShown(const QModelIndex &idx, bool shown) = 0;
+	virtual void recalculateFilter() = 0;
 
 	// Used for sorting. This is a bit of a layering violation, as sorting should be performed
 	// by the higher-up QSortFilterProxyModel, but it makes things so much easier!
@@ -123,11 +122,10 @@ private:
 	QModelIndex index(int row, int column, const QModelIndex &parent) const override;
 	QModelIndex parent(const QModelIndex &index) const override;
 	QVariant data(const QModelIndex &index, int role) const override;
-	void filterFinished() override;
 	bool lessThan(const QModelIndex &i1, const QModelIndex &i2) const override;
 	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 recalculateFilter();
 	void divesChangedTrip(dive_trip *trip, const QVector<dive *> &dives);
 	void divesTimeChangedTrip(dive_trip *trip, timestamp_t delta, const QVector<dive *> &dives);
 
@@ -189,10 +187,9 @@ private:
 	QModelIndex index(int row, int column, const QModelIndex &parent) const override;
 	QModelIndex parent(const QModelIndex &index) const override;
 	QVariant data(const QModelIndex &index, int role) const override;
-	void filterFinished() override;
 	bool lessThan(const QModelIndex &i1, const QModelIndex &i2) const override;
 	dive *diveOrNull(const QModelIndex &index) const override;
-	bool setShown(const QModelIndex &idx, bool shown);
+	void recalculateFilter();
 
 	std::vector<dive *> items;				// TODO: access core data directly
 };

qt-models/filtermodels.cpp

@@ -7,13 +7,6 @@
 #include "core/subsurface-qt/DiveListNotifier.h"
 #include "qt-models/divetripmodel.h"
 
-#if !defined(SUBSURFACE_MOBILE)
-#include "core/divefilter.h"
-#endif
-
-#include <QDebug>
-#include <algorithm>
-
 MultiFilterSortModel *MultiFilterSortModel::instance()
 {
 	static MultiFilterSortModel self;
@@ -22,6 +15,7 @@ MultiFilterSortModel *MultiFilterSortModel::instance()
 
 MultiFilterSortModel::MultiFilterSortModel(QObject *parent) : QSortFilterProxyModel(parent)
 {
+	connect(&diveListNotifier, &DiveListNotifier::filterReset, this, &MultiFilterSortModel::invalidateFilter);
 	setFilterKeyColumn(-1); // filter all columns
 	setFilterCaseSensitivity(Qt::CaseInsensitive);
 }
@@ -41,73 +35,8 @@ bool MultiFilterSortModel::filterAcceptsRow(int source_row, const QModelIndex &s
 	return m->data(index0, DiveTripModelBase::SHOWN_ROLE).value<bool>();
 }
 
-void MultiFilterSortModel::myInvalidate()
-{
-	QAbstractItemModel *m = sourceModel();
-	if (!m)
-		return;
-
-	{
-		// This marker prevents the UI from getting notifications on selection changes.
-		// It is active until the end of the scope.
-		// This is actually meant for the undo-commands, so that they can do their work
-		// without having the UI updated.
-		// Here, it is used because invalidating the filter can generate numerous
-		// selection changes, which do full ui reloads. Instead, do that all at once
-		// as a consequence of the filterFinished signal right after the local scope.
-		auto marker = diveListNotifier.enterCommand();
-
-		DiveFilter *filter = DiveFilter::instance();
-		for (int i = 0; i < m->rowCount(QModelIndex()); ++i) {
-			QModelIndex idx = m->index(i, 0, QModelIndex());
-
-			if (m->data(idx, DiveTripModelBase::IS_TRIP_ROLE).toBool()) {
-				// This is a trip -> loop over all dives and see if any is selected
-
-				bool showTrip = false;
-				for (int j = 0; j < m->rowCount(idx); ++j) {
-					QModelIndex idx2 = m->index(j, 0, idx);
-					dive *d = m->data(idx2, DiveTripModelBase::DIVE_ROLE).value<dive *>();
-					if (!d) {
-						qWarning("MultiFilterSortModel::myInvalidate(): subitem not a dive!?");
-						continue;
-					}
-					bool show = filter->showDive(d);
-					if (show)
-						showTrip = true;
-					m->setData(idx2, show, DiveTripModelBase::SHOWN_ROLE);
-				}
-				m->setData(idx, showTrip, DiveTripModelBase::SHOWN_ROLE);
-			} else {
-				dive *d = m->data(idx, DiveTripModelBase::DIVE_ROLE).value<dive *>();
-				bool show = (d != NULL) && filter->showDive(d);
-				m->setData(idx, show, DiveTripModelBase::SHOWN_ROLE);
-			}
-		}
-
-		invalidateFilter();
-
-		// Tell the dive trip model to update the displayed-counts
-		DiveTripModelBase::instance()->filterFinished();
-		countsChanged();
-	}
-
-	emit filterFinished();
-}
-
-bool MultiFilterSortModel::updateDive(struct dive *d)
-{
-	bool newStatus = DiveFilter::instance()->showDive(d);
-	return filter_dive(d, newStatus);
-}
-
 bool MultiFilterSortModel::lessThan(const QModelIndex &i1, const QModelIndex &i2) const
 {
 	// Hand sorting down to the source model.
 	return DiveTripModelBase::instance()->lessThan(i1, i2);
 }
-
-void MultiFilterSortModel::countsChanged()
-{
-	updateWindowTitle();
-}

qt-models/filtermodels.h

@@ -4,31 +4,18 @@
 
 #include "divetripmodel.h"
 
-#include <QStringListModel>
 #include <QSortFilterProxyModel>
-#include <QDateTime>
-
-#include <stdint.h>
-#include <vector>
 
 class MultiFilterSortModel : public QSortFilterProxyModel {
 	Q_OBJECT
 public:
 	static MultiFilterSortModel *instance();
 	bool filterAcceptsRow(int source_row, const QModelIndex &source_parent) const override;
-	bool updateDive(struct dive *d); // returns true if visibility status changed
 	bool lessThan(const QModelIndex &, const QModelIndex &) const override;
 
 	void resetModel(DiveTripModelBase::Layout layout);
-	void myInvalidate();
-
-signals:
-	void filterFinished();
-
 private:
 	MultiFilterSortModel(QObject *parent = 0);
-	// Dive site filtering has priority over other filters
-	void countsChanged();
 };
 
 #endif