From 1c394490f4efe6055426257a8dfcec4cdf3e12af Mon Sep 17 00:00:00 2001 From: Andrew den Exter Date: Fri, 14 Oct 2011 16:44:46 +1000 Subject: [PATCH] Fix incorrect cache indexes when consecutive list items are removed. When cached model items are removed listItemsRemoved will try and append a new cache only range onto a preceding one where possible. Previously it would then re-run that range so the current indexes in the list were incremented before moving onto the next range, which meant that the current indexes were incremented by size of the previous range twice. Instead of processing the range a second time increment the cache indexes directly when inserting a cache only range and move directly onto the next range in the list. Change-Id: I63418c4397f911cefb521c5a5b0dd25faf66e08b Reviewed-by: Martin Jones --- .../util/qdeclarativelistcompositor.cpp | 43 +----- .../util/qdeclarativelistcompositor_p.h | 2 - .../tst_qdeclarativelistcompositor.cpp | 153 +++++++++++++++++++++ 3 files changed, 159 insertions(+), 39 deletions(-) diff --git a/src/declarative/util/qdeclarativelistcompositor.cpp b/src/declarative/util/qdeclarativelistcompositor.cpp index be0d543..0618ed2 100644 --- a/src/declarative/util/qdeclarativelistcompositor.cpp +++ b/src/declarative/util/qdeclarativelistcompositor.cpp @@ -154,22 +154,6 @@ static bool qt_verifyIntegrity( //#define QT_DECLARATIVE_TRACE_LISTCOMPOSITOR(args) qDebug() << m_end.index[1] << m_end.index[0] << Q_FUNC_INFO args; #define QT_DECLARATIVE_TRACE_LISTCOMPOSITOR(args) -QDeclarativeListCompositor::iterator &QDeclarativeListCompositor::iterator::operator ++() -{ - while (!(range->flags & groupFlag)) { - incrementIndexes(range->count - offset); - offset = 0; - range = range->next; - } - incrementIndexes(1); - if (++offset == range->count) { - while (!((range = range->next)->flags & groupFlag)) - incrementIndexes(range->count); - offset = 0; - } - return *this; -} - QDeclarativeListCompositor::iterator &QDeclarativeListCompositor::iterator::operator +=(int difference) { Q_ASSERT(difference >= 0); @@ -210,27 +194,10 @@ QDeclarativeListCompositor::iterator &QDeclarativeListCompositor::iterator::oper return *this; } -QDeclarativeListCompositor::insert_iterator &QDeclarativeListCompositor::insert_iterator::operator ++() -{ - while (!(range->flags & groupFlag)) { - incrementIndexes(range->count - offset); - offset = 0; - range = range->next; - } - incrementIndexes(1); - if (++offset == range->count && !range->append()) { - while (!((range = range->next)->flags & groupFlag) ){ - incrementIndexes(range->count); - } - offset = 0; - } - return *this; -} - QDeclarativeListCompositor::insert_iterator &QDeclarativeListCompositor::insert_iterator::operator +=(int difference) { Q_ASSERT(difference >= 0); - while (!(range->flags & groupFlag) && range->flags & GroupMask) { + while (!(range->flags & groupFlag) && (range->flags & (GroupMask | CacheFlag))) { incrementIndexes(range->count - offset); offset = 0; range = range->next; @@ -252,7 +219,7 @@ QDeclarativeListCompositor::insert_iterator &QDeclarativeListCompositor::insert_ QDeclarativeListCompositor::insert_iterator &QDeclarativeListCompositor::insert_iterator::operator -=(int difference) { Q_ASSERT(difference >= 0); - while (!(range->flags & groupFlag) && range->flags & GroupMask) { + while (!(range->flags & groupFlag) && (range->flags & (GroupMask | CacheFlag))) { decrementIndexes(offset); range = range->previous; offset = range->count; @@ -359,7 +326,7 @@ QDeclarativeListCompositor::insert_iterator QDeclarativeListCompositor::findInse Q_ASSERT(index >=0 && index <= count(group)); insert_iterator it; if (m_cacheIt == m_end) { - m_cacheIt = iterator(m_ranges.next, 0, group, m_groupCount); + it = iterator(m_ranges.next, 0, group, m_groupCount); it += index; } else { const int offset = index - m_cacheIt.index[group]; @@ -976,6 +943,7 @@ void QDeclarativeListCompositor::listItemsRemoved( } else { *it = insert(*it, it->list, -1, removeCount, CacheFlag)->next; } + it.index[Cache] += removeCount; } if (removeFlags & GroupMask) translatedRemovals->append(translatedRemoval); @@ -994,10 +962,11 @@ void QDeclarativeListCompositor::listItemsRemoved( && it->list == it->next->list && (it->flags == CacheFlag || it->end() == it->next->index) && it->flags == (it->next->flags & ~AppendFlag)) { + + it.index[Cache] += it->next->count; it->count += it->next->count; it->flags = it->next->flags; erase(it->next); - *it = it->previous; } else if (!removed) { it.incrementIndexes(it->count); } diff --git a/src/declarative/util/qdeclarativelistcompositor_p.h b/src/declarative/util/qdeclarativelistcompositor_p.h index 60df819..c808f6f 100644 --- a/src/declarative/util/qdeclarativelistcompositor_p.h +++ b/src/declarative/util/qdeclarativelistcompositor_p.h @@ -132,7 +132,6 @@ public: Range *operator ->() { return range; } const Range *operator ->() const { return range; } - iterator &operator ++(); iterator &operator +=(int difference); iterator &operator -=(int difference); @@ -168,7 +167,6 @@ public: inline insert_iterator(Range *, int, Group, int); inline ~insert_iterator() {} - insert_iterator &operator ++(); insert_iterator &operator +=(int difference); insert_iterator &operator -=(int difference); }; diff --git a/tests/auto/declarative/qdeclarativelistcompositor/tst_qdeclarativelistcompositor.cpp b/tests/auto/declarative/qdeclarativelistcompositor/tst_qdeclarativelistcompositor.cpp index 67bced6..94bb40e 100644 --- a/tests/auto/declarative/qdeclarativelistcompositor/tst_qdeclarativelistcompositor.cpp +++ b/tests/auto/declarative/qdeclarativelistcompositor/tst_qdeclarativelistcompositor.cpp @@ -146,6 +146,10 @@ class tst_qdeclarativelistcompositor : public QObject } private slots: + void find_data(); + void find(); + void findInsertPosition_data(); + void findInsertPosition(); void insert(); void clearFlags_data(); void clearFlags(); @@ -164,6 +168,131 @@ private slots: void listItemsChanged(); }; +void tst_qdeclarativelistcompositor::find_data() +{ + QTest::addColumn("ranges"); + QTest::addColumn("startGroup"); + QTest::addColumn("startIndex"); + QTest::addColumn("group"); + QTest::addColumn("index"); + QTest::addColumn("selectionIndex"); + QTest::addColumn("visibleIndex"); + QTest::addColumn("defaultIndex"); + QTest::addColumn("cacheIndex"); + QTest::addColumn("rangeFlags"); + QTest::addColumn("rangeIndex"); + + int listA; void *a = &listA; + + QTest::newRow("Start") + << (RangeList() + << Range(a, 0, 1, int(C::PrependFlag | SelectionFlag | C::DefaultFlag | C::CacheFlag)) + << Range(a, 1, 1, int(C::AppendFlag | C::PrependFlag | C::CacheFlag)) + << Range(0, 0, 1, int(VisibleFlag| C::CacheFlag))) + << C::Cache << 2 + << Selection << 0 + << 0 << 0 << 0 << 0 + << int(C::PrependFlag | SelectionFlag | C::DefaultFlag | C::CacheFlag) << 0; +} + +void tst_qdeclarativelistcompositor::find() +{ + QFETCH(RangeList, ranges); + QFETCH(C::Group, startGroup); + QFETCH(int, startIndex); + QFETCH(C::Group, group); + QFETCH(int, index); + QFETCH(int, cacheIndex); + QFETCH(int, defaultIndex); + QFETCH(int, visibleIndex); + QFETCH(int, selectionIndex); + QFETCH(int, rangeFlags); + QFETCH(int, rangeIndex); + + QDeclarativeListCompositor compositor; + compositor.setGroupCount(4); + compositor.setDefaultGroups(VisibleFlag | C::DefaultFlag); + + foreach (const Range &range, ranges) + compositor.append(range.list, range.index, range.count, range.flags); + + compositor.find(startGroup, startIndex); + + QDeclarativeListCompositor::iterator it = compositor.find(group, index); + QCOMPARE(it.index[C::Cache], cacheIndex); + QCOMPARE(it.index[C::Default], defaultIndex); + QCOMPARE(it.index[Visible], visibleIndex); + QCOMPARE(it.index[Selection], selectionIndex); + QCOMPARE(it->flags, rangeFlags); + QCOMPARE(it->index, rangeIndex); +} + +void tst_qdeclarativelistcompositor::findInsertPosition_data() +{ + QTest::addColumn("ranges"); + QTest::addColumn("startGroup"); + QTest::addColumn("startIndex"); + QTest::addColumn("group"); + QTest::addColumn("index"); + QTest::addColumn("selectionIndex"); + QTest::addColumn("visibleIndex"); + QTest::addColumn("defaultIndex"); + QTest::addColumn("cacheIndex"); + QTest::addColumn("rangeFlags"); + QTest::addColumn("rangeIndex"); + + int listA; void *a = &listA; + + QTest::newRow("Start") + << (RangeList() + << Range(a, 0, 1, int(C::PrependFlag | SelectionFlag | C::DefaultFlag | C::CacheFlag)) + << Range(a, 1, 1, int(C::AppendFlag | C::PrependFlag | C::CacheFlag)) + << Range(0, 0, 1, int(VisibleFlag| C::CacheFlag))) + << C::Cache << 2 + << Selection << 0 + << 0 << 0 << 0 << 0 + << int(C::PrependFlag | SelectionFlag | C::DefaultFlag | C::CacheFlag) << 0; + QTest::newRow("1") + << (RangeList() + << Range(a, 0, 1, int(C::PrependFlag | SelectionFlag | C::DefaultFlag | C::CacheFlag)) + << Range(a, 1, 1, int(C::AppendFlag | C::PrependFlag | C::CacheFlag)) + << Range(0, 0, 1, int(VisibleFlag| C::CacheFlag))) + << C::Cache << 2 + << Selection << 1 + << 1 << 0 << 1 << 1 + << int(C::AppendFlag | C::PrependFlag | C::CacheFlag) << 1; +} + +void tst_qdeclarativelistcompositor::findInsertPosition() +{ + QFETCH(RangeList, ranges); + QFETCH(C::Group, startGroup); + QFETCH(int, startIndex); + QFETCH(C::Group, group); + QFETCH(int, index); + QFETCH(int, cacheIndex); + QFETCH(int, defaultIndex); + QFETCH(int, visibleIndex); + QFETCH(int, selectionIndex); + QFETCH(int, rangeFlags); + QFETCH(int, rangeIndex); + + QDeclarativeListCompositor compositor; + compositor.setGroupCount(4); + compositor.setDefaultGroups(VisibleFlag | C::DefaultFlag); + + foreach (const Range &range, ranges) + compositor.append(range.list, range.index, range.count, range.flags); + + QDeclarativeListCompositor::insert_iterator it = compositor.findInsertPosition(group, index); + QCOMPARE(it.index[C::Cache], cacheIndex); + QCOMPARE(it.index[C::Default], defaultIndex); + QCOMPARE(it.index[Visible], visibleIndex); + QCOMPARE(it.index[Selection], selectionIndex); + QCOMPARE(it->flags, rangeFlags); + QCOMPARE(it->index, rangeIndex); +} + void tst_qdeclarativelistcompositor::insert() { QDeclarativeListCompositor compositor; @@ -1067,6 +1196,30 @@ void tst_qdeclarativelistcompositor::listItemsRemoved_data() << IndexArray(defaultIndexes) << IndexArray() << IndexArray(); + } { static const int cacheIndexes[] = {/*A*/-1,-1,0,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,1}; + static const int defaultIndexes[] = {/*A*/0,1,2,3,4,5,6}; + QTest::newRow("Sparse remove") + << (RangeList() + << Range(a, 0, 2, C::CacheFlag) + << Range(a, 0, 1, C::DefaultFlag | C::CacheFlag) + << Range(a, 0, 1, C::CacheFlag) + << Range(a, 1, 5, C::DefaultFlag | C::CacheFlag) + << Range(a, 0, 1, C::CacheFlag) + << Range(a, 6, 2, C::DefaultFlag | C::CacheFlag) + << Range(a, 0, 1, C::CacheFlag) + << Range(a, 8, 3, C::DefaultFlag | C::CacheFlag) + << Range(a, 0, 1, C::CacheFlag) + << Range(a, 11, 1, C::DefaultFlag | C::CacheFlag) + << Range(a, 12, 5, C::DefaultFlag)) + << a << 1 << 10 + << (RemoveList() + << Remove(0, 0, 1, 4, 5, C::DefaultFlag | C::CacheFlag) + << Remove(0, 0, 1,10, 2, C::DefaultFlag | C::CacheFlag) + << Remove(0, 0, 1,13, 3, C::DefaultFlag | C::CacheFlag)) + << IndexArray(cacheIndexes) + << IndexArray(defaultIndexes) + << IndexArray() + << IndexArray(); } } -- 2.7.4