Package: kde4libs / 4:4.14.2-5+deb8u2

KRecursiveFilterProxyModel-Fixed-the-model.diff Patch series | download
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
261
262
263
264
265
266
267
268
269
270
271
272
273
274
275
276
277
278
279
280
281
282
283
284
285
286
287
288
289
290
291
292
293
294
295
296
297
298
299
300
301
302
303
304
305
306
307
308
309
310
311
312
313
314
315
316
317
318
319
320
321
322
323
324
325
326
327
328
329
330
331
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

--- a/kdeui/itemviews/krecursivefilterproxymodel.cpp
+++ b/kdeui/itemviews/krecursivefilterproxymodel.cpp
@@ -126,7 +126,7 @@ void KRecursiveFilterProxyModelPrivate::
 
   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::
 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)
--- a/kdeui/tests/CMakeLists.txt
+++ b/kdeui/tests/CMakeLists.txt
@@ -82,6 +82,7 @@ KDEUI_PROXYMODEL_TESTS(
   kdescendantsproxymodeltest
   kselectionproxymodeltest
   testmodelqueuedconnections
+  krecursivefilterproxymodeltest
 )
 
 KDEUI_EXECUTABLE_TESTS(
--- /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"