aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorGravatar Berthold Stoeger <bstoeger@mail.tuwien.ac.at>2019-06-22 20:48:59 +0200
committerGravatar bstoeger <32835590+bstoeger@users.noreply.github.com>2019-06-23 20:08:46 +0200
commitcbcddaa396f6668fef7750eb2721bc70ca11d0e4 (patch)
tree5d0d7abcafbf99c31f4b24a20c94ce1ca4c7e87e
parent4b0f90ced33569d50913eb9826a8d96fbc4d9f3a (diff)
downloadsubsurface-cbcddaa396f6668fef7750eb2721bc70ca11d0e4.tar.gz
Dive list: cache shown flag in model (quick-fix for undo-crash)
We have a very fundamental problem with data-duplication in core and qt-models. In a particular case, this led to an easily reproducible crash: 1) An undo command moved the last dive of a trip to another. 2) When an undo-command removed the last dive of a trip to a different trip, the dive was removed from the trip in the core. Then, the model was updated. 3) That lead at first to a rearrangement of the trips, because the trip with the added dive is moved before the trip with the removed dive. 4) In such a case, the filter-model checks the visibility of the trip. 5) Since the trip with the removed dive has no dives in the core, visibility was determined as false. 6) From this point on the mappings of the QSortFilterProxyModel were messed up. Accesses led to crashes. It is unclear whether this is a Qt bug or only a QOI issue. As a quick-fix, cache the visibility flag of trips directly in the Qt-models. Don't set the visibility directly in the core, but go via the Qt-models. Thus, a more clear layering is achieved. In the long run, we can hopefully get rid of the data-duplication in the models. Signed-off-by: Berthold Stoeger <bstoeger@mail.tuwien.ac.at>
-rw-r--r--qt-models/divetripmodel.cpp59
-rw-r--r--qt-models/divetripmodel.h5
-rw-r--r--qt-models/filtermodels.cpp63
3 files changed, 97 insertions, 30 deletions
diff --git a/qt-models/divetripmodel.cpp b/qt-models/divetripmodel.cpp
index ca1e4f73f..9fd323bcf 100644
--- a/qt-models/divetripmodel.cpp
+++ b/qt-models/divetripmodel.cpp
@@ -389,6 +389,9 @@ 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)
@@ -636,16 +639,19 @@ QModelIndex DiveTripModelTree::parent(const QModelIndex &index) const
}
DiveTripModelTree::Item::Item(dive_trip *t, const QVector<dive *> &divesIn) : d_or_t{nullptr, t},
- dives(divesIn.toStdVector())
+ dives(divesIn.toStdVector()),
+ shown(std::any_of(dives.begin(), dives.end(), [](dive *d){ return !d->hidden_by_filter; }))
{
}
DiveTripModelTree::Item::Item(dive_trip *t, dive *d) : d_or_t{nullptr, t},
- dives({ d })
+ dives({ d }),
+ shown(!d->hidden_by_filter)
{
}
-DiveTripModelTree::Item::Item(dive *d) : d_or_t{d, nullptr}
+DiveTripModelTree::Item::Item(dive *d) : d_or_t{d, nullptr},
+ shown(!d->hidden_by_filter)
{
}
@@ -683,8 +689,44 @@ 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)
+{
+ 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);
+ }
+ } 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;
+}
+
QVariant DiveTripModelTree::data(const QModelIndex &index, int role) const
{
+ if (role == SHOWN_ROLE) {
+ QModelIndex parent = index.parent();
+ // An invalid parent means that we're at the top-level
+ if (!parent.isValid())
+ return items[index.row()].shown;
+ return !items[parent.row()].dives[index.row()]->hidden_by_filter;
+ }
+
// Set the font for all items alike
if (role == Qt::FontRole)
return defaultModelFont();
@@ -1135,6 +1177,15 @@ dive *DiveTripModelList::diveOrNull(const QModelIndex &index) const
return items[row];
}
+bool DiveTripModelList::setShown(const QModelIndex &idx, bool shown)
+{
+ dive *d = diveOrNull(idx);
+ if (!d)
+ return false;
+ filter_dive(d, shown);
+ return true;
+}
+
QVariant DiveTripModelList::data(const QModelIndex &index, int role) const
{
// Set the font for all items alike
@@ -1142,6 +1193,8 @@ QVariant DiveTripModelList::data(const QModelIndex &index, int role) const
return defaultModelFont();
dive *d = diveOrNull(index);
+ if (role == SHOWN_ROLE)
+ return d && !d->hidden_by_filter;
return d ? diveData(d, index.column(), role) : QVariant();
}
diff --git a/qt-models/divetripmodel.h b/qt-models/divetripmodel.h
index 236dbb1d2..592234917 100644
--- a/qt-models/divetripmodel.h
+++ b/qt-models/divetripmodel.h
@@ -52,6 +52,7 @@ public:
DIVE_ROLE,
TRIP_ROLE,
DIVE_IDX,
+ SHOWN_ROLE,
SELECTED_ROLE
};
enum Layout {
@@ -74,6 +75,7 @@ public:
DiveTripModelBase(QObject *parent = 0);
int columnCount(const QModelIndex&) const;
virtual void filterFinished() = 0;
+ virtual bool setShown(const QModelIndex &idx, bool shown) = 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!
@@ -126,6 +128,7 @@ private:
bool lessThan(const QModelIndex &i1, const QModelIndex &i2) const override;
void changeDiveSelection(dive_trip *trip, const QVector<dive *> &dives, bool select) override;
dive *diveOrNull(const QModelIndex &index) const override;
+ bool setShown(const QModelIndex &idx, bool shown);
// The tree model has two levels. At the top level, we have either trips or dives
// that do not belong to trips. Such a top-level item is represented by the "Item"
@@ -138,6 +141,7 @@ private:
struct Item {
dive_or_trip d_or_t;
std::vector<dive *> dives; // std::vector<> instead of QVector for insert() with three iterators
+ bool shown;
Item(dive_trip *t, const QVector<dive *> &dives);
Item(dive_trip *t, dive *d); // Initialize a trip with one dive
Item(dive *d); // Initialize a top-level dive
@@ -187,6 +191,7 @@ private:
bool lessThan(const QModelIndex &i1, const QModelIndex &i2) const override;
void changeDiveSelection(dive_trip *trip, const QVector<dive *> &dives, bool select) override;
dive *diveOrNull(const QModelIndex &index) const override;
+ bool setShown(const QModelIndex &idx, bool shown);
std::vector<dive *> items; // TODO: access core data directly
};
diff --git a/qt-models/filtermodels.cpp b/qt-models/filtermodels.cpp
index 8e7239e35..9ef910def 100644
--- a/qt-models/filtermodels.cpp
+++ b/qt-models/filtermodels.cpp
@@ -194,25 +194,9 @@ bool MultiFilterSortModel::showDive(const struct dive *d) const
bool MultiFilterSortModel::filterAcceptsRow(int source_row, const QModelIndex &source_parent) const
{
- QModelIndex index0 = sourceModel()->index(source_row, 0, source_parent);
- struct dive *d = sourceModel()->data(index0, DiveTripModelBase::DIVE_ROLE).value<struct dive *>();
-
- // For dives, simply check the hidden_by_filter flag
- if (d)
- return !d->hidden_by_filter;
-
- // Since this is not a dive, it must be a trip
- dive_trip *trip = sourceModel()->data(index0, DiveTripModelBase::TRIP_ROLE).value<dive_trip *>();
-
- if (!trip)
- return false; // Oops. Neither dive nor trip, something is seriously wrong.
-
- // Show the trip if any dive is visible
- for (int i = 0; i < trip->dives.nr; ++i) {
- if (!trip->dives.dives[i]->hidden_by_filter)
- return true;
- }
- return false;
+ QAbstractItemModel *m = sourceModel();
+ QModelIndex index0 = m->index(source_row, 0, source_parent);
+ return m->data(index0, DiveTripModelBase::SHOWN_ROLE).value<bool>();
}
void MultiFilterSortModel::filterChanged(const QModelIndex &from, const QModelIndex &to, const QVector<int> &roles)
@@ -225,8 +209,9 @@ void MultiFilterSortModel::filterChanged(const QModelIndex &from, const QModelIn
void MultiFilterSortModel::myInvalidate()
{
- int i;
- struct dive *d;
+ QAbstractItemModel *m = sourceModel();
+ if (!m)
+ return;
{
// This marker prevents the UI from getting notifications on selection changes.
@@ -240,12 +225,36 @@ void MultiFilterSortModel::myInvalidate()
divesDisplayed = 0;
- // Apply filter for each dive
- for_each_dive (i, d) {
- bool show = showDive(d);
- filter_dive(d, show);
- if (show)
- divesDisplayed++;
+ for (int i = 0; i < m->rowCount(QModelIndex()); ++i) {
+ QModelIndex idx = m->index(i, 0, QModelIndex());
+
+ dive_trip *trip = m->data(idx, DiveTripModelBase::TRIP_ROLE).value<dive_trip *>();
+ if (trip) {
+ // 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 = showDive(d);
+ if (show) {
+ divesDisplayed++;
+ 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 = showDive(d);
+ if (show)
+ divesDisplayed++;
+ m->setData(idx, show, DiveTripModelBase::SHOWN_ROLE);
+ }
}
invalidateFilter();