diff options
author | Andrew den Exter <andrew.den-exter@nokia.com> | 2012-05-23 14:58:13 +1000 |
---|---|---|
committer | Qt by Nokia <qt-info@nokia.com> | 2012-05-24 04:04:33 +0200 |
commit | 2542778d4837143a61dfcf143c32683acc8b998a (patch) | |
tree | d8de075e1a1fc65a8a825eacafed928e1723d388 | |
parent | 5008fc55cf1ab1c776ccd431525fe54b7afeea00 (diff) |
Fix crash in VisualDataModel drag selection example.
The crash was an assert on an invalid pre-condition, and that's simply
been removed. But it did highlight another issue when moving items
due to separate code paths for iterating forwards and backward in
a composited list. Basically when iterating backwards because we're
looking for the first instance of an index it's necessary to overshoot
a little to an index prior and then iterate forwards, and as such
there's little difference between iterating forward or backwards.
Change-Id: I6252e3e0170dc2c72d0204137c69275c8ccc519b
Reviewed-by: Martin Jones <martin.jones@nokia.com>
-rw-r--r-- | src/quick/util/qquicklistcompositor.cpp | 161 | ||||
-rw-r--r-- | src/quick/util/qquicklistcompositor_p.h | 2 | ||||
-rw-r--r-- | tests/auto/qml/qquicklistcompositor/tst_qquicklistcompositor.cpp | 34 |
3 files changed, 99 insertions, 98 deletions
diff --git a/src/quick/util/qquicklistcompositor.cpp b/src/quick/util/qquicklistcompositor.cpp index 2afc6e9384..1a893609e0 100644 --- a/src/quick/util/qquicklistcompositor.cpp +++ b/src/quick/util/qquicklistcompositor.cpp @@ -158,91 +158,47 @@ static bool qt_verifyIntegrity( QQuickListCompositor::iterator &QQuickListCompositor::iterator::operator +=(int difference) { - Q_ASSERT(difference >= 0); - while (!(range->flags & groupFlag)) { - incrementIndexes(range->count - offset); - offset = 0; - range = range->next; - } + // Update all indexes to the start of the range. decrementIndexes(offset); + + // If the iterator group isn't a member of the current range ignore the current offset. + if (!(range->flags & groupFlag)) + offset = 0; + offset += difference; - while (offset >= range->count || !(range->flags & groupFlag)) { - if (range->flags & groupFlag) - offset -= range->count; - incrementIndexes(range->count); - range = range->next; - } - incrementIndexes(offset); - return *this; -} -QQuickListCompositor::iterator &QQuickListCompositor::iterator::operator -=(int difference) -{ - Q_ASSERT(difference >= 0); - while (!(range->flags & groupFlag)) { - decrementIndexes(offset); - range = range->previous; - offset = range->count; - } - decrementIndexes(offset); - offset -= difference; - while (offset < 0) { + // Iterate backwards looking for a range with a positive offset. + while (offset <= 0 && range->previous->flags) { range = range->previous; if (range->flags & groupFlag) offset += range->count; decrementIndexes(range->count); } - incrementIndexes(offset); - return *this; -} -QQuickListCompositor::insert_iterator &QQuickListCompositor::insert_iterator::operator +=(int difference) -{ - Q_ASSERT(difference >= 0); - while (!(range->flags & groupFlag)) { - incrementIndexes(range->count - offset); - offset = 0; - range = range->next; - } - decrementIndexes(offset); - offset += difference; - while (offset > range->count - || (offset == range->count && !range->append() && offset > 0) - || (!(range->flags & groupFlag) && offset > 0)) { - Q_ASSERT(range->flags); + // Iterate forwards looking for the first range which contains both the offset and the + // iterator group. + while (range->flags && (offset >= range->count || !(range->flags & groupFlag))) { if (range->flags & groupFlag) offset -= range->count; incrementIndexes(range->count); range = range->next; } + + // Update all the indexes to inclue the remaining offset. incrementIndexes(offset); + return *this; } -QQuickListCompositor::insert_iterator &QQuickListCompositor::insert_iterator::operator -=(int difference) +QQuickListCompositor::insert_iterator &QQuickListCompositor::insert_iterator::operator +=(int difference) { - Q_ASSERT(difference >= 0); - while (!(range->flags & groupFlag) && range->previous->flags) { - decrementIndexes(offset); - range = range->previous; - offset = (range->flags & (GroupMask | CacheFlag)) ? range->count : 0; - } - decrementIndexes(offset); - offset -= difference; - while (offset < 0) { + iterator::operator +=(difference); + + // If the previous range contains the append flag move the iterator to the tail of the previous + // range so that appended appear after the insert position. + if (offset == 0 && range->previous->append()) { range = range->previous; - if (range->flags & groupFlag) - offset += range->count; - decrementIndexes(range->count); - } - incrementIndexes(offset); - for (Range *previous = range->previous; offset == 0 && previous->prepend(); previous = previous->previous) { - if (previous->append() && previous->inGroup()) { - offset = previous->count; - range = previous; - } else if (!previous->inGroup()) { - break; - } + offset = range->inGroup() ? range->count : 0; } return *this; @@ -303,14 +259,7 @@ QQuickListCompositor::iterator QQuickListCompositor::find(Group group, int index } else { const int offset = index - m_cacheIt.index[group]; m_cacheIt.setGroup(group); - if (offset > 0) { - m_cacheIt += offset; - } else if (offset < 0) { - m_cacheIt -= -offset; - } else if (offset == 0) { - m_cacheIt -= 0; - m_cacheIt += 0; - } + m_cacheIt += offset; } Q_ASSERT(m_cacheIt.index[group] == index); Q_ASSERT(m_cacheIt->inGroup(group)); @@ -335,14 +284,7 @@ QQuickListCompositor::insert_iterator QQuickListCompositor::findInsertPosition(G const int offset = index - m_cacheIt.index[group]; it = m_cacheIt; it.setGroup(group); - if (offset > 0) { - it += offset; - } else if (offset < 0) { - it -= -offset; - } else if (offset == 0) { - it -= 0; - it += 0; - } + it += offset; } Q_ASSERT(it.index[group] == index); return it; @@ -414,7 +356,7 @@ QQuickListCompositor::iterator QQuickListCompositor::insert( void QQuickListCompositor::setFlags( Group fromGroup, int from, int count, Group group, int flags, QVector<Insert> *inserts) { - QT_QML_TRACE_LISTCOMPOSITOR(<< group << index << count << flags) + QT_QML_TRACE_LISTCOMPOSITOR(<< fromGroup << from << count << group << flags) setFlags(find(fromGroup, from), count, group, flags, inserts); } @@ -496,7 +438,7 @@ void QQuickListCompositor::setFlags( void QQuickListCompositor::clearFlags( Group fromGroup, int from, int count, Group group, uint flags, QVector<Remove> *removes) { - QT_QML_TRACE_LISTCOMPOSITOR(<< group << index << count << flags) + QT_QML_TRACE_LISTCOMPOSITOR(<< fromGroup << from << count << group << flags) clearFlags(find(fromGroup, from), count, group, flags, removes); } @@ -631,27 +573,44 @@ bool QQuickListCompositor::verifyMoveTo( return to >= 0 && to + count <= m_end.index[toGroup]; } +/*! + \internal + + Moves \a count items belonging to \a moveGroup from the index \a from in \a fromGroup + to the index \a to in \a toGroup. + + If \a removes and \a inserts are not null they will be populated with per group notifications + of the items moved. + */ + void QQuickListCompositor::move( Group fromGroup, int from, Group toGroup, int to, int count, - Group group, + Group moveGroup, QVector<Remove> *removes, QVector<Insert> *inserts) { QT_QML_TRACE_LISTCOMPOSITOR(<< fromGroup << from << toGroup << to << count) - Q_ASSERT(count != 0); - Q_ASSERT(from >=0 && from + count <= m_end.index[toGroup]); - Q_ASSERT(verifyMoveTo(fromGroup, from, toGroup, to, count, group)); + Q_ASSERT(count > 0); + Q_ASSERT(from >=0); + Q_ASSERT(verifyMoveTo(fromGroup, from, toGroup, to, count, moveGroup)); + // Find the position of the first item to move. iterator fromIt = find(fromGroup, from); - if (fromIt != group) { + + if (fromIt != moveGroup) { + // If the range at the from index doesn't contain items from the move group; skip + // to the next range. fromIt.incrementIndexes(fromIt->count - fromIt.offset); fromIt.offset = 0; *fromIt = fromIt->next; } else if (fromIt.offset > 0) { + // If the range at the from index contains items from the move group and the index isn't + // at the start of the range; split the range at the index and move the iterator to start + // of the second range. *fromIt = insert( *fromIt, fromIt->list, fromIt->index, fromIt.offset, fromIt->flags & ~AppendFlag)->next; fromIt->index += fromIt.offset; @@ -659,32 +618,39 @@ void QQuickListCompositor::move( fromIt.offset = 0; } + // Remove count items belonging to the move group from the list. Range movedFlags; for (int moveId = 0; count > 0;) { - if (fromIt != group) { + if (fromIt != moveGroup) { + // Skip ranges not containing items from the move group. fromIt.incrementIndexes(fromIt->count); *fromIt = fromIt->next; continue; } int difference = qMin(count, fromIt->count); + // Create a new static range containing the moved items from an existing range. new Range( &movedFlags, fromIt->list, fromIt->index, difference, fromIt->flags & ~(PrependFlag | AppendFlag)); + // Remove moved items from the count, the existing range, and a remove notification. if (removes) removes->append(Remove(fromIt, difference, fromIt->flags, moveId++)); count -= difference; fromIt->count -= difference; + // If the existing range contains the prepend flag replace the removed items with + // a placeholder range for new items inserted into the source model. int removeIndex = fromIt->index; if (fromIt->prepend() && fromIt->previous != &m_ranges && fromIt->previous->flags == PrependFlag && fromIt->previous->list == fromIt->list && fromIt->previous->end() == fromIt->index) { + // Grow the previous range instead of creating a new one if possible. fromIt->previous->count += difference; } else if (fromIt->prepend()) { *fromIt = insert(*fromIt, fromIt->list, removeIndex, difference, PrependFlag)->next; @@ -692,10 +658,12 @@ void QQuickListCompositor::move( fromIt->index += difference; if (fromIt->count == 0) { + // If the existing range has no items remaining; remove it from the list. if (fromIt->append()) fromIt->previous->flags |= AppendFlag; *fromIt = erase(*fromIt); + // If the ranges before and after the removed range can be joined, do so. if (*fromIt != m_ranges.next && fromIt->flags == PrependFlag && fromIt->previous != &m_ranges && fromIt->previous->flags == PrependFlag @@ -710,6 +678,7 @@ void QQuickListCompositor::move( } } + // Try and join the range following the removed items to the range preceding it. if (*fromIt != m_ranges.next && *fromIt != &m_ranges && fromIt->previous->list == fromIt->list @@ -723,14 +692,15 @@ void QQuickListCompositor::move( *fromIt = erase(*fromIt)->previous; } + // Find the destination position of the move. insert_iterator toIt = fromIt; toIt.setGroup(toGroup); + const int difference = to - toIt.index[toGroup]; - if (difference > 0) - toIt += difference; - else - toIt -= -difference; + toIt += difference; + // If the insert position is part way through a range; split it and move the iterator to the + // start of the second range. if (toIt.offset > 0) { *toIt = insert(*toIt, toIt->list, toIt->index, toIt.offset, toIt->flags & ~AppendFlag)->next; toIt->index += toIt.offset; @@ -738,6 +708,8 @@ void QQuickListCompositor::move( toIt.offset = 0; } + // Insert the moved ranges before the insert iterator, growing the previous range if that + // is an option. for (Range *range = movedFlags.previous; range != &movedFlags; range = range->previous) { if (*toIt != &m_ranges && range->list == toIt->list @@ -750,6 +722,7 @@ void QQuickListCompositor::move( } } + // Try and join the range after the inserted ranges to the last range inserted. if (*toIt != m_ranges.next && toIt->previous->list == toIt->list && (!toIt->list || (toIt->previous->end() == toIt->index && toIt->previous->flags == (toIt->flags & ~AppendFlag)))) { @@ -758,6 +731,7 @@ void QQuickListCompositor::move( toIt->previous->flags = toIt->flags; *toIt = erase(*toIt)->previous; } + // Create insert notification for the ranges moved. Insert insert(toIt, 0, 0, 0); for (Range *next, *range = movedFlags.next; range != &movedFlags; range = next) { insert.count = range->count; @@ -774,6 +748,7 @@ void QQuickListCompositor::move( } m_cacheIt = toIt; + QT_QML_VERIFY_LISTCOMPOSITOR } diff --git a/src/quick/util/qquicklistcompositor_p.h b/src/quick/util/qquicklistcompositor_p.h index 5c9d679fa4..38ac0a5457 100644 --- a/src/quick/util/qquicklistcompositor_p.h +++ b/src/quick/util/qquicklistcompositor_p.h @@ -135,7 +135,6 @@ public: const Range *operator ->() const { return range; } iterator &operator +=(int difference); - iterator &operator -=(int difference); template<typename T> T *list() const { return static_cast<T *>(range->list); } int modelIndex() const { return range->index + offset; } @@ -170,7 +169,6 @@ public: inline ~insert_iterator() {} insert_iterator &operator +=(int difference); - insert_iterator &operator -=(int difference); }; struct Change diff --git a/tests/auto/qml/qquicklistcompositor/tst_qquicklistcompositor.cpp b/tests/auto/qml/qquicklistcompositor/tst_qquicklistcompositor.cpp index a5d66661f0..f16e287c90 100644 --- a/tests/auto/qml/qquicklistcompositor/tst_qquicklistcompositor.cpp +++ b/tests/auto/qml/qquicklistcompositor/tst_qquicklistcompositor.cpp @@ -258,8 +258,8 @@ void tst_qquicklistcompositor::findInsertPosition_data() << Range(a, 1, 1, int(C::AppendFlag | C::PrependFlag | C::CacheFlag)) << Range(0, 0, 1, int(VisibleFlag| C::CacheFlag))) << Selection << 1 - << 1 << 0 << 1 << 1 - << uint(C::AppendFlag | C::PrependFlag | C::CacheFlag) << 1; + << 1 << 1 << 1 << 3 + << uint(0) << 0; } void tst_qquicklistcompositor::findInsertPosition() @@ -282,6 +282,7 @@ void tst_qquicklistcompositor::findInsertPosition() compositor.append(range.list, range.index, range.count, range.flags); QQuickListCompositor::insert_iterator it = compositor.findInsertPosition(group, index); + QCOMPARE(it.index[C::Cache], cacheIndex); QCOMPARE(it.index[C::Default], defaultIndex); QCOMPARE(it.index[Visible], visibleIndex); @@ -338,7 +339,6 @@ void tst_qquicklistcompositor::insert() C::Default, 0, c, 6, 4, C::DefaultFlag); const int indexes[] = {6,7,8,9,0,1,2,3,4,5,6,7,8,9,10,11,4,5,6,7,2,3}; const int *lists[] = {c,c,c,c,a,a,a,a,a,a,a,a,a,a, a, a,b,b,b,b,c,c}; - QCOMPARE(compositor.count(C::Default), lengthOf(indexes)); for (int i = 0; i < lengthOf(indexes); ++i) { it = compositor.find(C::Default, i); QCOMPARE(it.list<int>(), lists[i]); @@ -951,6 +951,34 @@ void tst_qquicklistcompositor::move_data() << IndexArray(defaultIndexes) << ListArray(defaultLists) << IndexArray() << ListArray() << IndexArray() << ListArray(); + } { static const int cacheIndexes[] = {1,2,3,4,5,6,0,8,9}; + static const void *cacheLists[] = {a,a,a,a,a,a,a,a,a}; + static const int defaultIndexes[] = {1,2,3,4,5,6,0,8,9}; + static const void *defaultLists[] = {a,a,a,a,a,a,a,a,a}; + static const int visibleIndexes[] = {0,7,10}; + static const void *visibleLists[] = {a,a, a}; + QTest::newRow("0, 6, 3") + << (RangeList() + << Range(a, 0, 1, C::PrependFlag) + << Range(a, 1, 6, C::PrependFlag | C::DefaultFlag | C::CacheFlag) + << Range(a, 0, 1, VisibleFlag | C::DefaultFlag | C::CacheFlag) + << Range(a, 7, 1, VisibleFlag) + << Range(a,10, 1, VisibleFlag) + << Range(a, 7, 1, C::PrependFlag) + << Range(a, 8, 2, C::AppendFlag | C::PrependFlag | C::DefaultFlag | C::CacheFlag)) + << Visible << 0 << C::Default << 6 << 3 + << (RemoveList() + << Remove(0, 0, 6, 6, 1, VisibleFlag | C::DefaultFlag | C::CacheFlag, 0) + << Remove(0, 0, 6, 6, 1, VisibleFlag, 1) + << Remove(0, 0, 6, 6, 1, VisibleFlag, 2)) + << (InsertList() + << Insert(0, 0, 6, 6, 1, VisibleFlag | C::DefaultFlag | C::CacheFlag, 0) + << Insert(0, 1, 7, 7, 1, VisibleFlag, 1) + << Insert(0, 2, 7, 7, 1, VisibleFlag, 2)) + << IndexArray(cacheIndexes) << ListArray(cacheLists) + << IndexArray(defaultIndexes) << ListArray(defaultLists) + << IndexArray(visibleIndexes) << ListArray(visibleLists) + << IndexArray() << ListArray(); } } |