342 lines
14 KiB
Diff
342 lines
14 KiB
Diff
From a932980cc7babe69613b9c6ad98faa4ec368258e Mon Sep 17 00:00:00 2001
|
|
From: Christian Mollekopf <chrigi_1@fastmail.fm>
|
|
Date: Tue, 9 Sep 2014 18:16:37 +0200
|
|
Subject: [PATCH] KRecursiveFilterProxyModel: Fixed the model
|
|
|
|
The model was not working properly and didn't include all items under
|
|
some circumstances.
|
|
This patch fixes the following scenarios in particular:
|
|
|
|
* The change in sourceDataChanged is required to fix the shortcut condition.
|
|
The idea is that if the parent is already part of the model (it must be if acceptRow returns true),
|
|
we can directly invoke dataChanged on the parent, resulting in the changed index
|
|
getting reevaluated. However, because the recursive filterAcceptsRow version was used
|
|
the shortcut was also used when only the current index matches the filter and
|
|
the parent index is in fact not yet in the model. In this case we failed to call
|
|
dataChanged on the right index and thus the complete branch was never added to the model.
|
|
|
|
* The change in refreshAscendantMapping is required to include indexes that were
|
|
included by descendants. The intended way how this was supposed to work is that we
|
|
traverse the tree upwards and find the last index that is not yet part of the model.
|
|
We would then call dataChanged on that index causing it and its descendants to get reevaluated.
|
|
However, acceptRow does not reflect wether an index is already in the model or not.
|
|
Consider the following model:
|
|
|
|
- A
|
|
- B
|
|
- C
|
|
- D
|
|
|
|
|
|
If C is include in the model by default but D not and A & B only gets included due to C, we have the following model:
|
|
- A
|
|
- B
|
|
- C
|
|
- D
|
|
|
|
If we then call refreshAscendantsMapping on D it will not consider B as already being part of the model.
|
|
This results in the toplevel index A being considered lastAscendant, and a call to dataChanged on A results in
|
|
a reevaluation of A only, which is already in the model. Thus D never gets added to the model.
|
|
|
|
Unfortunately there is no way to probe QSortFilterProxyModel for indexes that are
|
|
already part of the model. Even the const mapFromSource internally creates a mapping when called,
|
|
and thus instead of revealing indexes that are not yet part of the model, it silently
|
|
creates a mapping (without issuing the relevant signals!).
|
|
|
|
As the only possible workaround we have to issues dataChanged for all ancestors
|
|
which is ignored for indexes that are not yet mapped, and results in a rowsInserted
|
|
signal for the correct indexes. It also results in superfluous dataChanged signals,
|
|
since we don't know when to stop, but at least we have a properly behaving model
|
|
this way.
|
|
---
|
|
kdeui/itemviews/krecursivefilterproxymodel.cpp | 17 +-
|
|
kdeui/tests/CMakeLists.txt | 1 +
|
|
kdeui/tests/krecursivefilterproxymodeltest.cpp | 221 +++++++++++++++++++++++++
|
|
3 files changed, 227 insertions(+), 12 deletions(-)
|
|
create mode 100644 kdeui/tests/krecursivefilterproxymodeltest.cpp
|
|
|
|
diff --git a/kdeui/itemviews/krecursivefilterproxymodel.cpp b/kdeui/itemviews/krecursivefilterproxymodel.cpp
|
|
index 6d65631..089af79 100644
|
|
--- a/kdeui/itemviews/krecursivefilterproxymodel.cpp
|
|
+++ b/kdeui/itemviews/krecursivefilterproxymodel.cpp
|
|
@@ -126,7 +126,7 @@ void KRecursiveFilterProxyModelPrivate::sourceDataChanged(const QModelIndex &sou
|
|
|
|
QModelIndex source_parent = source_top_left.parent();
|
|
|
|
- if (!source_parent.isValid() || q->filterAcceptsRow(source_parent.row(), source_parent.parent()))
|
|
+ if (!source_parent.isValid() || q->acceptRow(source_parent.row(), source_parent.parent()))
|
|
{
|
|
invokeDataChanged(source_top_left, source_bottom_right);
|
|
return;
|
|
@@ -149,24 +149,17 @@ void KRecursiveFilterProxyModelPrivate::sourceDataChanged(const QModelIndex &sou
|
|
void KRecursiveFilterProxyModelPrivate::refreshAscendantMapping(const QModelIndex &index, bool refreshAll)
|
|
{
|
|
Q_Q(KRecursiveFilterProxyModel);
|
|
-
|
|
Q_ASSERT(index.isValid());
|
|
- QModelIndex lastAscendant = index;
|
|
- QModelIndex sourceAscendant = index.parent();
|
|
+
|
|
+ QModelIndex sourceAscendant = index;
|
|
// We got a matching descendant, so find the right place to insert the row.
|
|
// We need to tell the QSortFilterProxyModel that the first child between an existing row in the model
|
|
// has changed data so that it will get a mapping.
|
|
- while(sourceAscendant.isValid() && !q->acceptRow(sourceAscendant.row(), sourceAscendant.parent()))
|
|
+ while(sourceAscendant.isValid())
|
|
{
|
|
- if (refreshAll)
|
|
- invokeDataChanged(sourceAscendant, sourceAscendant);
|
|
-
|
|
- lastAscendant = sourceAscendant;
|
|
+ invokeDataChanged(sourceAscendant, sourceAscendant);
|
|
sourceAscendant = sourceAscendant.parent();
|
|
}
|
|
-
|
|
- // Inform the model that its data changed so that it creates new mappings and finds the rows which now match the filter.
|
|
- invokeDataChanged(lastAscendant, lastAscendant);
|
|
}
|
|
|
|
void KRecursiveFilterProxyModelPrivate::sourceRowsAboutToBeInserted(const QModelIndex &source_parent, int start, int end)
|
|
diff --git a/kdeui/tests/CMakeLists.txt b/kdeui/tests/CMakeLists.txt
|
|
index f661b91..948516b 100644
|
|
--- a/kdeui/tests/CMakeLists.txt
|
|
+++ b/kdeui/tests/CMakeLists.txt
|
|
@@ -81,6 +81,7 @@ KDEUI_PROXYMODEL_TESTS(
|
|
kdescendantsproxymodeltest
|
|
kselectionproxymodeltest
|
|
testmodelqueuedconnections
|
|
+ krecursivefilterproxymodeltest
|
|
)
|
|
|
|
KDEUI_EXECUTABLE_TESTS(
|
|
diff --git a/kdeui/tests/krecursivefilterproxymodeltest.cpp b/kdeui/tests/krecursivefilterproxymodeltest.cpp
|
|
new file mode 100644
|
|
index 0000000..a336116
|
|
--- /dev/null
|
|
+++ b/kdeui/tests/krecursivefilterproxymodeltest.cpp
|
|
@@ -0,0 +1,221 @@
|
|
+/*
|
|
+ Copyright (c) 2014 Christian Mollekopf <mollekopf@kolabsys.com>
|
|
+
|
|
+ This library is free software; you can redistribute it and/or modify it
|
|
+ under the terms of the GNU Library General Public License as published by
|
|
+ the Free Software Foundation; either version 2 of the License, or (at your
|
|
+ option) any later version.
|
|
+
|
|
+ This library is distributed in the hope that it will be useful, but WITHOUT
|
|
+ ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
|
|
+ FITNESS FOR A PARTICULAR PURPOSE. See the GNU Library General Public
|
|
+ License for more details.
|
|
+
|
|
+ You should have received a copy of the GNU Library General Public License
|
|
+ along with this library; see the file COPYING.LIB. If not, write to the
|
|
+ Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
|
|
+ 02110-1301, USA.
|
|
+*/
|
|
+
|
|
+
|
|
+#include <qtest_kde.h>
|
|
+
|
|
+#include <krecursivefilterproxymodel.h>
|
|
+#include <QStandardItemModel>
|
|
+
|
|
+class ModelSignalSpy : public QObject {
|
|
+ Q_OBJECT
|
|
+public:
|
|
+ explicit ModelSignalSpy(QAbstractItemModel &model) {
|
|
+ connect(&model, SIGNAL(rowsInserted(QModelIndex, int, int)), this, SLOT(onRowsInserted(QModelIndex,int,int)));
|
|
+ connect(&model, SIGNAL(rowsRemoved(QModelIndex, int, int)), this, SLOT(onRowsRemoved(QModelIndex,int,int)));
|
|
+ connect(&model, SIGNAL(rowsMoved(QModelIndex, int, int, QModelIndex, int)), this, SLOT(onRowsMoved(QModelIndex,int,int, QModelIndex, int)));
|
|
+ connect(&model, SIGNAL(dataChanged(QModelIndex,QModelIndex)), this, SLOT(onDataChanged(QModelIndex,QModelIndex)));
|
|
+ connect(&model, SIGNAL(layoutChanged()), this, SLOT(onLayoutChanged()));
|
|
+ connect(&model, SIGNAL(modelReset()), this, SLOT(onModelReset()));
|
|
+ }
|
|
+
|
|
+ QStringList mSignals;
|
|
+ QModelIndex parent;
|
|
+ int start;
|
|
+ int end;
|
|
+
|
|
+public Q_SLOTS:
|
|
+ void onRowsInserted(QModelIndex p, int s, int e) {
|
|
+ mSignals << QLatin1String("rowsInserted");
|
|
+ parent = p;
|
|
+ start = s;
|
|
+ end = e;
|
|
+ }
|
|
+ void onRowsRemoved(QModelIndex p, int s, int e) {
|
|
+ mSignals << QLatin1String("rowsRemoved");
|
|
+ parent = p;
|
|
+ start = s;
|
|
+ end = e;
|
|
+ }
|
|
+ void onRowsMoved(QModelIndex,int,int,QModelIndex,int) {
|
|
+ mSignals << QLatin1String("rowsMoved");
|
|
+ }
|
|
+ void onDataChanged(QModelIndex,QModelIndex) {
|
|
+ mSignals << QLatin1String("dataChanged");
|
|
+ }
|
|
+ void onLayoutChanged() {
|
|
+ mSignals << QLatin1String("layoutChanged");
|
|
+ }
|
|
+ void onModelReset() {
|
|
+ mSignals << QLatin1String("modelReset");
|
|
+ }
|
|
+};
|
|
+
|
|
+class TestModel : public KRecursiveFilterProxyModel
|
|
+{
|
|
+ Q_OBJECT
|
|
+public:
|
|
+ virtual bool acceptRow(int sourceRow, const QModelIndex &sourceParent) const
|
|
+ {
|
|
+ // qDebug() << sourceModel()->index(sourceRow, 0, sourceParent).data().toString() << sourceModel()->index(sourceRow, 0, sourceParent).data(Qt::UserRole+1).toBool();
|
|
+ return sourceModel()->index(sourceRow, 0, sourceParent).data(Qt::UserRole+1).toBool();
|
|
+ }
|
|
+};
|
|
+
|
|
+static QModelIndex getIndex(char *string, const QAbstractItemModel &model)
|
|
+{
|
|
+ QModelIndexList list = model.match(model.index(0, 0), Qt::DisplayRole, QString::fromLatin1(string), 1, Qt::MatchRecursive);
|
|
+ if (list.isEmpty()) {
|
|
+ return QModelIndex();
|
|
+ }
|
|
+ return list.first();
|
|
+}
|
|
+
|
|
+class KRecursiveFilterProxyModelTest : public QObject
|
|
+{
|
|
+ Q_OBJECT
|
|
+private:
|
|
+
|
|
+private slots:
|
|
+ // Requires the acceptRow fix in sourceDataChanged to pass
|
|
+ // Test that we properly react to a data-changed signal in a descendant and include all required rows
|
|
+ void testDataChange()
|
|
+ {
|
|
+ QStandardItemModel model;
|
|
+ TestModel proxy;
|
|
+ proxy.setSourceModel(&model);
|
|
+
|
|
+ QStandardItem *row1 = new QStandardItem("row1");
|
|
+ row1->setData(false);
|
|
+ model.appendRow(row1);
|
|
+
|
|
+ QCOMPARE(getIndex("row1", proxy).isValid(), false);
|
|
+
|
|
+ QStandardItem *subchild = new QStandardItem("subchild");
|
|
+ subchild->setData(false);
|
|
+ {
|
|
+ QStandardItem *child = new QStandardItem("child");
|
|
+ child->setData(false);
|
|
+ child->appendRow(subchild);
|
|
+ row1->appendRow(child);
|
|
+ }
|
|
+
|
|
+ ModelSignalSpy spy(proxy);
|
|
+ subchild->setData(true);
|
|
+
|
|
+ QCOMPARE(getIndex("row1", proxy).isValid(), true);
|
|
+ QCOMPARE(getIndex("child", proxy).isValid(), true);
|
|
+ QCOMPARE(getIndex("subchild", proxy).isValid(), true);
|
|
+
|
|
+ QCOMPARE(spy.mSignals, QStringList() << QLatin1String("rowsInserted"));
|
|
+ }
|
|
+
|
|
+ void testInsert()
|
|
+ {
|
|
+ QStandardItemModel model;
|
|
+ TestModel proxy;
|
|
+ proxy.setSourceModel(&model);
|
|
+
|
|
+ QStandardItem *row1 = new QStandardItem("row1");
|
|
+ row1->setData(false);
|
|
+ model.appendRow(row1);
|
|
+
|
|
+ QStandardItem *child = new QStandardItem("child");
|
|
+ child->setData(false);
|
|
+ row1->appendRow(child);
|
|
+
|
|
+ QStandardItem *child2 = new QStandardItem("child2");
|
|
+ child2->setData(false);
|
|
+ child->appendRow(child2);
|
|
+
|
|
+ QCOMPARE(getIndex("row1", proxy).isValid(), false);
|
|
+ QCOMPARE(getIndex("child", proxy).isValid(), false);
|
|
+ QCOMPARE(getIndex("child2", proxy).isValid(), false);
|
|
+
|
|
+ ModelSignalSpy spy(proxy);
|
|
+ {
|
|
+ QStandardItem *subchild = new QStandardItem("subchild");
|
|
+ subchild->setData(true);
|
|
+ child2->appendRow(subchild);
|
|
+ }
|
|
+
|
|
+ QCOMPARE(getIndex("row1", proxy).isValid(), true);
|
|
+ QCOMPARE(spy.mSignals, QStringList() << QLatin1String("rowsInserted"));
|
|
+ QCOMPARE(spy.parent, QModelIndex());
|
|
+ }
|
|
+
|
|
+
|
|
+ // We want to get child2 into the model which is a descendant of child.
|
|
+ // child is already in the model from the neighbor2 branch. We must ensure dataChange is called on child,
|
|
+ // so child2 is included in the model.
|
|
+ void testNeighborPath()
|
|
+ {
|
|
+ QStandardItemModel model;
|
|
+ TestModel proxy;
|
|
+ proxy.setSourceModel(&model);
|
|
+
|
|
+ QStandardItem *row1 = new QStandardItem("row1");
|
|
+ row1->setData(false);
|
|
+ model.appendRow(row1);
|
|
+
|
|
+ QStandardItem *child = new QStandardItem("child");
|
|
+ child->setData(false);
|
|
+ row1->appendRow(child);
|
|
+
|
|
+ QStandardItem *child2 = new QStandardItem("child2");
|
|
+ child2->setData(false);
|
|
+ child->appendRow(child2);
|
|
+
|
|
+ {
|
|
+ QStandardItem *nb1 = new QStandardItem("neighbor");
|
|
+ nb1->setData(false);
|
|
+ child->appendRow(nb1);
|
|
+
|
|
+ QStandardItem *nb2 = new QStandardItem("neighbor2");
|
|
+ nb2->setData(true);
|
|
+ nb1->appendRow(nb2);
|
|
+ }
|
|
+
|
|
+ //These tests affect the test. It seems without them the mapping is not created in qsortfilterproxymodel, resulting in the item
|
|
+ //simply getting added later on. With these the model doesn't react to the added subchild as it should. Piece of crap.
|
|
+ QCOMPARE(getIndex("child2", proxy).isValid(), false);
|
|
+ QCOMPARE(getIndex("child", proxy).isValid(), true);
|
|
+ QCOMPARE(getIndex("neighbor", proxy).isValid(), true);
|
|
+ QCOMPARE(getIndex("neighbor2", proxy).isValid(), true);
|
|
+
|
|
+ ModelSignalSpy spy(proxy);
|
|
+
|
|
+ {
|
|
+ qDebug() << "inserting";
|
|
+ QStandardItem *subchild = new QStandardItem("subchild");
|
|
+ subchild->setData(true);
|
|
+ child2->appendRow(subchild);
|
|
+ }
|
|
+
|
|
+ QCOMPARE(getIndex("child2", proxy).isValid(), true);
|
|
+ QCOMPARE(getIndex("subchild", proxy).isValid(), true);
|
|
+ //The dataChanged signals are not intentional and cause by refreshAscendantMapping. Unfortunately we can't avoid them.
|
|
+ QCOMPARE(spy.mSignals, QStringList() << QLatin1String("rowsInserted") << QLatin1String("dataChanged") << QLatin1String("dataChanged"));
|
|
+ }
|
|
+
|
|
+};
|
|
+
|
|
+QTEST_KDEMAIN(KRecursiveFilterProxyModelTest, NoGUI)
|
|
+
|
|
+#include "krecursivefilterproxymodeltest.moc"
|
|
--
|
|
1.8.2.3
|
|
|