Lines 1-355
Link Here
|
1 |
commit 0df92439241a76c6a67efa9485bd95c3c25d63a0 |
|
|
2 |
Author: Christian Mollekopf <chrigi_1@fastmail.fm> |
3 |
Date: Thu Jan 22 15:04:16 2015 +0100 |
4 |
|
5 |
KRecursiveFilterProxyModel: Fixed the model |
6 |
|
7 |
The model was not working properly and didn't include all items under |
8 |
some circumstances. |
9 |
This patch fixes the following scenarios in particular: |
10 |
|
11 |
* The change in sourceDataChanged is required to fix the shortcut condition. |
12 |
The idea is that if the parent is already part of the model (it must be if acceptRow returns true), |
13 |
we can directly invoke dataChanged on the parent, resulting in the changed index |
14 |
getting reevaluated. However, because the recursive filterAcceptsRow version was used |
15 |
the shortcut was also used when only the current index matches the filter and |
16 |
the parent index is in fact not yet in the model. In this case we failed to call |
17 |
dataChanged on the right index and thus the complete branch was never added to the model. |
18 |
|
19 |
* The change in refreshAscendantMapping is required to include indexes that were |
20 |
included by descendants. The intended way how this was supposed to work is that we |
21 |
traverse the tree upwards and find the last index that is not yet part of the model. |
22 |
We would then call dataChanged on that index causing it and its descendants to get reevaluated. |
23 |
However, acceptRow does not reflect wether an index is already in the model or not. |
24 |
Consider the following model: |
25 |
|
26 |
- A |
27 |
- B |
28 |
- C |
29 |
- D |
30 |
|
31 |
If C is included in the model by default but D not, and A & B only get included due to C, we have the following model: |
32 |
|
33 |
- A |
34 |
- B |
35 |
- C |
36 |
|
37 |
If we then call refreshAscendantsMapping on D it will not consider B as already being part of the model. |
38 |
This results in the toplevel index A being considered lastAscendant, and a call to dataChanged on A results in |
39 |
a reevaluation of A only, which is already in the model. Thus D never gets added to the model. |
40 |
|
41 |
Unfortunately there is no way to probe QSortFilterProxyModel for indexes that are |
42 |
already part of the model. Even the const mapFromSource internally creates a mapping when called, |
43 |
and thus instead of revealing indexes that are not yet part of the model, it silently |
44 |
creates a mapping (without issuing the relevant signals!). |
45 |
|
46 |
As the only possible workaround we have to issues dataChanged for all ancestors |
47 |
which is ignored for indexes that are not yet mapped, and results in a rowsInserted |
48 |
signal for the correct indexes. It also results in superfluous dataChanged signals, |
49 |
since we don't know when to stop, but at least we have a properly behaving model |
50 |
this way. |
51 |
|
52 |
REVIEW: 120119 |
53 |
BUG: 338950 |
54 |
|
55 |
--- kdeui/itemviews/krecursivefilterproxymodel.cpp |
56 |
+++ kdeui/itemviews/krecursivefilterproxymodel.cpp |
57 |
@@ -108,12 +108,9 @@ public: |
58 |
void sourceRowsRemoved(const QModelIndex &source_parent, int start, int end); |
59 |
|
60 |
/** |
61 |
- Given that @p index does not match the filter, clear mappings in the QSortFilterProxyModel up to and excluding the |
62 |
- first ascendant that does match, and remake the mappings. |
63 |
- |
64 |
- If @p refreshAll is true, this method also refreshes intermediate mappings. This is significant when removing rows. |
65 |
+ Given that @p index does not match the filter, clear mappings in the QSortFilterProxyModel up to roo, and remake the mappings. |
66 |
*/ |
67 |
- void refreshAscendantMapping(const QModelIndex &index, bool refreshAll = false); |
68 |
+ void refreshAscendantMapping(const QModelIndex &index); |
69 |
|
70 |
bool ignoreRemove; |
71 |
bool completeInsert; |
72 |
@@ -126,7 +123,7 @@ void KRecursiveFilterProxyModelPrivate::sourceDataChanged(const QModelIndex &sou |
73 |
|
74 |
QModelIndex source_parent = source_top_left.parent(); |
75 |
|
76 |
- if (!source_parent.isValid() || q->filterAcceptsRow(source_parent.row(), source_parent.parent())) |
77 |
+ if (!source_parent.isValid() || q->acceptRow(source_parent.row(), source_parent.parent())) |
78 |
{ |
79 |
invokeDataChanged(source_top_left, source_bottom_right); |
80 |
return; |
81 |
@@ -146,27 +143,20 @@ void KRecursiveFilterProxyModelPrivate::sourceDataChanged(const QModelIndex &sou |
82 |
refreshAscendantMapping(source_parent); |
83 |
} |
84 |
|
85 |
-void KRecursiveFilterProxyModelPrivate::refreshAscendantMapping(const QModelIndex &index, bool refreshAll) |
86 |
+void KRecursiveFilterProxyModelPrivate::refreshAscendantMapping(const QModelIndex &index) |
87 |
{ |
88 |
Q_Q(KRecursiveFilterProxyModel); |
89 |
- |
90 |
Q_ASSERT(index.isValid()); |
91 |
- QModelIndex lastAscendant = index; |
92 |
- QModelIndex sourceAscendant = index.parent(); |
93 |
+ |
94 |
+ QModelIndex sourceAscendant = index; |
95 |
// We got a matching descendant, so find the right place to insert the row. |
96 |
// We need to tell the QSortFilterProxyModel that the first child between an existing row in the model |
97 |
// has changed data so that it will get a mapping. |
98 |
- while(sourceAscendant.isValid() && !q->acceptRow(sourceAscendant.row(), sourceAscendant.parent())) |
99 |
+ while(sourceAscendant.isValid()) |
100 |
{ |
101 |
- if (refreshAll) |
102 |
- invokeDataChanged(sourceAscendant, sourceAscendant); |
103 |
- |
104 |
- lastAscendant = sourceAscendant; |
105 |
+ invokeDataChanged(sourceAscendant, sourceAscendant); |
106 |
sourceAscendant = sourceAscendant.parent(); |
107 |
} |
108 |
- |
109 |
- // Inform the model that its data changed so that it creates new mappings and finds the rows which now match the filter. |
110 |
- invokeDataChanged(lastAscendant, lastAscendant); |
111 |
} |
112 |
|
113 |
void KRecursiveFilterProxyModelPrivate::sourceRowsAboutToBeInserted(const QModelIndex &source_parent, int start, int end) |
114 |
@@ -261,7 +251,7 @@ void KRecursiveFilterProxyModelPrivate::sourceRowsRemoved(const QModelIndex &sou |
115 |
// This is needed because QSFPM only invalidates the mapping for the |
116 |
// index range given to dataChanged, not its children. |
117 |
if (source_parent.isValid()) |
118 |
- refreshAscendantMapping(source_parent, true); |
119 |
+ refreshAscendantMapping(source_parent); |
120 |
} |
121 |
|
122 |
KRecursiveFilterProxyModel::KRecursiveFilterProxyModel(QObject* parent) |
123 |
--- kdeui/tests/CMakeLists.txt |
124 |
+++ kdeui/tests/CMakeLists.txt |
125 |
@@ -82,6 +82,7 @@ KDEUI_PROXYMODEL_TESTS( |
126 |
kdescendantsproxymodeltest |
127 |
kselectionproxymodeltest |
128 |
testmodelqueuedconnections |
129 |
+ krecursivefilterproxymodeltest |
130 |
) |
131 |
|
132 |
KDEUI_EXECUTABLE_TESTS( |
133 |
--- /dev/null |
134 |
+++ kdeui/tests/krecursivefilterproxymodeltest.cpp |
135 |
@@ -0,0 +1,220 @@ |
136 |
+/* |
137 |
+ Copyright (c) 2014 Christian Mollekopf <mollekopf@kolabsys.com> |
138 |
+ |
139 |
+ This library is free software; you can redistribute it and/or modify it |
140 |
+ under the terms of the GNU Library General Public License as published by |
141 |
+ the Free Software Foundation; either version 2 of the License, or (at your |
142 |
+ option) any later version. |
143 |
+ |
144 |
+ This library is distributed in the hope that it will be useful, but WITHOUT |
145 |
+ ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or |
146 |
+ FITNESS FOR A PARTICULAR PURPOSE. See the GNU Library General Public |
147 |
+ License for more details. |
148 |
+ |
149 |
+ You should have received a copy of the GNU Library General Public License |
150 |
+ along with this library; see the file COPYING.LIB. If not, write to the |
151 |
+ Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA |
152 |
+ 02110-1301, USA. |
153 |
+*/ |
154 |
+ |
155 |
+ |
156 |
+#include <qtest_kde.h> |
157 |
+ |
158 |
+#include <krecursivefilterproxymodel.h> |
159 |
+#include <QStandardItemModel> |
160 |
+ |
161 |
+class ModelSignalSpy : public QObject { |
162 |
+ Q_OBJECT |
163 |
+public: |
164 |
+ explicit ModelSignalSpy(QAbstractItemModel &model) { |
165 |
+ connect(&model, SIGNAL(rowsInserted(QModelIndex, int, int)), this, SLOT(onRowsInserted(QModelIndex,int,int))); |
166 |
+ connect(&model, SIGNAL(rowsRemoved(QModelIndex, int, int)), this, SLOT(onRowsRemoved(QModelIndex,int,int))); |
167 |
+ connect(&model, SIGNAL(rowsMoved(QModelIndex, int, int, QModelIndex, int)), this, SLOT(onRowsMoved(QModelIndex,int,int, QModelIndex, int))); |
168 |
+ connect(&model, SIGNAL(dataChanged(QModelIndex,QModelIndex)), this, SLOT(onDataChanged(QModelIndex,QModelIndex))); |
169 |
+ connect(&model, SIGNAL(layoutChanged()), this, SLOT(onLayoutChanged())); |
170 |
+ connect(&model, SIGNAL(modelReset()), this, SLOT(onModelReset())); |
171 |
+ } |
172 |
+ |
173 |
+ QStringList mSignals; |
174 |
+ QModelIndex parent; |
175 |
+ int start; |
176 |
+ int end; |
177 |
+ |
178 |
+public Q_SLOTS: |
179 |
+ void onRowsInserted(QModelIndex p, int s, int e) { |
180 |
+ mSignals << QLatin1String("rowsInserted"); |
181 |
+ parent = p; |
182 |
+ start = s; |
183 |
+ end = e; |
184 |
+ } |
185 |
+ void onRowsRemoved(QModelIndex p, int s, int e) { |
186 |
+ mSignals << QLatin1String("rowsRemoved"); |
187 |
+ parent = p; |
188 |
+ start = s; |
189 |
+ end = e; |
190 |
+ } |
191 |
+ void onRowsMoved(QModelIndex,int,int,QModelIndex,int) { |
192 |
+ mSignals << QLatin1String("rowsMoved"); |
193 |
+ } |
194 |
+ void onDataChanged(QModelIndex,QModelIndex) { |
195 |
+ mSignals << QLatin1String("dataChanged"); |
196 |
+ } |
197 |
+ void onLayoutChanged() { |
198 |
+ mSignals << QLatin1String("layoutChanged"); |
199 |
+ } |
200 |
+ void onModelReset() { |
201 |
+ mSignals << QLatin1String("modelReset"); |
202 |
+ } |
203 |
+}; |
204 |
+ |
205 |
+class TestModel : public KRecursiveFilterProxyModel |
206 |
+{ |
207 |
+ Q_OBJECT |
208 |
+public: |
209 |
+ virtual bool acceptRow(int sourceRow, const QModelIndex &sourceParent) const |
210 |
+ { |
211 |
+ // qDebug() << sourceModel()->index(sourceRow, 0, sourceParent).data().toString() << sourceModel()->index(sourceRow, 0, sourceParent).data(Qt::UserRole+1).toBool(); |
212 |
+ return sourceModel()->index(sourceRow, 0, sourceParent).data(Qt::UserRole+1).toBool(); |
213 |
+ } |
214 |
+}; |
215 |
+ |
216 |
+static QModelIndex getIndex(const char *string, const QAbstractItemModel &model) |
217 |
+{ |
218 |
+ QModelIndexList list = model.match(model.index(0, 0), Qt::DisplayRole, QString::fromLatin1(string), 1, Qt::MatchRecursive); |
219 |
+ if (list.isEmpty()) { |
220 |
+ return QModelIndex(); |
221 |
+ } |
222 |
+ return list.first(); |
223 |
+} |
224 |
+ |
225 |
+class KRecursiveFilterProxyModelTest : public QObject |
226 |
+{ |
227 |
+ Q_OBJECT |
228 |
+private: |
229 |
+ |
230 |
+private slots: |
231 |
+ // Test that we properly react to a data-changed signal in a descendant and include all required rows |
232 |
+ void testDataChange() |
233 |
+ { |
234 |
+ QStandardItemModel model; |
235 |
+ TestModel proxy; |
236 |
+ proxy.setSourceModel(&model); |
237 |
+ |
238 |
+ QStandardItem *index1 = new QStandardItem("1"); |
239 |
+ index1->setData(false); |
240 |
+ model.appendRow(index1); |
241 |
+ |
242 |
+ QVERIFY(!getIndex("1", proxy).isValid()); |
243 |
+ |
244 |
+ QStandardItem *index1_1_1 = new QStandardItem("1.1.1"); |
245 |
+ index1_1_1->setData(false); |
246 |
+ QStandardItem *index1_1 = new QStandardItem("1.1"); |
247 |
+ index1_1->setData(false); |
248 |
+ index1_1->appendRow(index1_1_1); |
249 |
+ index1->appendRow(index1_1); |
250 |
+ |
251 |
+ ModelSignalSpy spy(proxy); |
252 |
+ index1_1_1->setData(true); |
253 |
+ |
254 |
+ QVERIFY(getIndex("1", proxy).isValid()); |
255 |
+ QVERIFY(getIndex("1.1", proxy).isValid()); |
256 |
+ QVERIFY(getIndex("1.1.1", proxy).isValid()); |
257 |
+ |
258 |
+ QCOMPARE(spy.mSignals, QStringList() << QLatin1String("rowsInserted")); |
259 |
+ } |
260 |
+ |
261 |
+ void testInsert() |
262 |
+ { |
263 |
+ QStandardItemModel model; |
264 |
+ TestModel proxy; |
265 |
+ proxy.setSourceModel(&model); |
266 |
+ |
267 |
+ QStandardItem *index1 = new QStandardItem("index1"); |
268 |
+ index1->setData(false); |
269 |
+ model.appendRow(index1); |
270 |
+ |
271 |
+ QStandardItem *index1_1 = new QStandardItem("index1_1"); |
272 |
+ index1_1->setData(false); |
273 |
+ index1->appendRow(index1_1); |
274 |
+ |
275 |
+ QStandardItem *index1_1_1 = new QStandardItem("index1_1_1"); |
276 |
+ index1_1_1->setData(false); |
277 |
+ index1_1->appendRow(index1_1_1); |
278 |
+ |
279 |
+ QVERIFY(!getIndex("index1", proxy).isValid()); |
280 |
+ QVERIFY(!getIndex("index1_1", proxy).isValid()); |
281 |
+ QVERIFY(!getIndex("index1_1_1", proxy).isValid()); |
282 |
+ |
283 |
+ ModelSignalSpy spy(proxy); |
284 |
+ { |
285 |
+ QStandardItem *index1_1_1_1 = new QStandardItem("index1_1_1_1"); |
286 |
+ index1_1_1_1->setData(true); |
287 |
+ index1_1_1->appendRow(index1_1_1_1); |
288 |
+ } |
289 |
+ |
290 |
+ QVERIFY(getIndex("index1", proxy).isValid()); |
291 |
+ QVERIFY(getIndex("index1_1", proxy).isValid()); |
292 |
+ QVERIFY(getIndex("index1_1_1", proxy).isValid()); |
293 |
+ QVERIFY(getIndex("index1_1_1_1", proxy).isValid()); |
294 |
+ QCOMPARE(spy.mSignals, QStringList() << QLatin1String("rowsInserted")); |
295 |
+ QCOMPARE(spy.parent, QModelIndex()); |
296 |
+ } |
297 |
+ |
298 |
+ |
299 |
+ // We want to get index1_1_1_1 into the model which is a descendant of index1_1. |
300 |
+ // index1_1 is already in the model from the neighbor2 branch. We must ensure dataChange is called on index1_1, |
301 |
+ // so index1_1_1_1 is included in the model. |
302 |
+ void testNeighborPath() |
303 |
+ { |
304 |
+ QStandardItemModel model; |
305 |
+ TestModel proxy; |
306 |
+ proxy.setSourceModel(&model); |
307 |
+ |
308 |
+ QStandardItem *index1 = new QStandardItem("index1"); |
309 |
+ index1->setData(false); |
310 |
+ model.appendRow(index1); |
311 |
+ |
312 |
+ QStandardItem *index1_1 = new QStandardItem("index1_1"); |
313 |
+ index1_1->setData(false); |
314 |
+ index1->appendRow(index1_1); |
315 |
+ |
316 |
+ QStandardItem *index1_1_1 = new QStandardItem("index1_1_1"); |
317 |
+ index1_1_1->setData(false); |
318 |
+ index1_1->appendRow(index1_1_1); |
319 |
+ |
320 |
+ { |
321 |
+ QStandardItem *nb1 = new QStandardItem("neighbor"); |
322 |
+ nb1->setData(false); |
323 |
+ index1_1->appendRow(nb1); |
324 |
+ |
325 |
+ QStandardItem *nb2 = new QStandardItem("neighbor2"); |
326 |
+ nb2->setData(true); |
327 |
+ nb1->appendRow(nb2); |
328 |
+ } |
329 |
+ |
330 |
+ //These tests affect the test. It seems without them the mapping is not created in qsortfilterproxymodel, resulting in the item |
331 |
+ //simply getting added later on. With these the model didn't react to the added index1_1_1_1 as it should. |
332 |
+ QVERIFY(!getIndex("index1_1_1", proxy).isValid()); |
333 |
+ QVERIFY(getIndex("index1_1", proxy).isValid()); |
334 |
+ QVERIFY(getIndex("neighbor", proxy).isValid()); |
335 |
+ QVERIFY(getIndex("neighbor2", proxy).isValid()); |
336 |
+ |
337 |
+ ModelSignalSpy spy(proxy); |
338 |
+ |
339 |
+ { |
340 |
+ QStandardItem *index1_1_1_1 = new QStandardItem("index1_1_1_1"); |
341 |
+ index1_1_1_1->setData(true); |
342 |
+ index1_1_1->appendRow(index1_1_1_1); |
343 |
+ } |
344 |
+ |
345 |
+ QVERIFY(getIndex("index1_1_1", proxy).isValid()); |
346 |
+ QVERIFY(getIndex("index1_1_1_1", proxy).isValid()); |
347 |
+ //The dataChanged signals are not intentional and caused by refreshAscendantMapping. Unfortunately we can't avoid them. |
348 |
+ QCOMPARE(spy.mSignals, QStringList() << QLatin1String("rowsInserted") << QLatin1String("dataChanged") << QLatin1String("dataChanged")); |
349 |
+ } |
350 |
+ |
351 |
+}; |
352 |
+ |
353 |
+QTEST_KDEMAIN(KRecursiveFilterProxyModelTest, NoGUI) |
354 |
+ |
355 |
+#include "krecursivefilterproxymodeltest.moc" |