Timeline: Add exception for parenting ranges with equal start times

Also, in contrast to the 5 previous times this piece of code has been
broken and fixed, this time add an elaborate comment about the problem
and a test case to check for future regressions.

Change-Id: I2babf7c6e98d7ab12ee53362229f30f6d9e5d7d5
Reviewed-by: Joerg Bornemann <joerg.bornemann@theqtcompany.com>

src/libs/timeline/timelinemodel.cpp

@@ -80,14 +80,27 @@ void TimelineModel::computeNesting()
                 parentIt = parents.erase(parentIt);
             } else if (parentEnd >= current.start + current.duration) {
                 // Current range is completely inside the parent range: no need to insert
-                current.parent = *parentIt;
+                current.parent = (parent.parent == -1 ? *parentIt : parent.parent);
                 break;
             } else if (parent.start == current.start) {
                 // The parent range starts at the same time but ends before the current range.
-                // We need to switch them.
+                // We could switch them but that would violate the order requirements. When
-                QTC_CHECK(parent.parent == -1);
+                // searching for ranges between two timestamps we'd skip the ranges between the
-                parent.parent = range;
+                // current range and the parent range if the start timestamp points into the parent
-                *parentIt = range;
+                // range. firstIndex() would then return the current range, which has an id greater
+                // than the parent. The parent could not be found then. To deal with this corner
+                // case, we assign the parent the "wrong" way around, so that on firstIndex() we
+                // always end up with the smallest id of any ranges starting at the same time.
+
+                // The other way to deal with this would be fixing up the ordering on insert. In
+                // fact we do that on insertStart().
+                // However, in order to rely on this we would also have to move the start index if
+                // on insertEnd() it turns out that the range just being ended is shorter than a
+                // previous one starting at the same time. We don't want to do that as client code
+                // could not find out about the changes in the IDs for range starts then.
+
+                current.parent = *parentIt;
+                parents.append(range);
                 break;
             } else {
                 ++parentIt;
@@ -315,8 +328,9 @@ int TimelineModel::firstIndex(qint64 startTime) const
 }
 
 /*!
-    Looks up the first range with an end time later than the specified \a startTime and
+    Of all indexes of ranges starting at the same time as the first range with an end time later
-    returns its index. If no such range is found, it returns -1.
+    than the specified \a startTime returns the lowest one. If no such range is found, it returns
+    -1.
 */
 int TimelineModel::TimelineModelPrivate::firstIndexNoParents(qint64 startTime) const
 {
@@ -434,11 +448,10 @@ int TimelineModel::insert(qint64 startTime, qint64 duration, int selectionId)
     Q_D(TimelineModel);
     /* Doing insert-sort here is preferable as most of the time the times will actually be
      * presorted in the right way. So usually this will just result in appending. */
-    int index = d->insertSorted(d->ranges,
+    int index = d->insertStart(TimelineModelPrivate::Range(startTime, duration, selectionId));
-                                TimelineModelPrivate::Range(startTime, duration, selectionId));
     if (index < d->ranges.size() - 1)
         d->incrementStartIndices(index);
-    d->insertSorted(d->endTimes, TimelineModelPrivate::RangeEnd(index, startTime + duration));
+    d->insertEnd(TimelineModelPrivate::RangeEnd(index, startTime + duration));
     return index;
 }
 
@@ -451,7 +464,7 @@ int TimelineModel::insert(qint64 startTime, qint64 duration, int selectionId)
 int TimelineModel::insertStart(qint64 startTime, int selectionId)
 {
     Q_D(TimelineModel);
-    int index = d->insertSorted(d->ranges, TimelineModelPrivate::Range(startTime, 0, selectionId));
+    int index = d->insertStart(TimelineModelPrivate::Range(startTime, 0, selectionId));
     if (index < d->ranges.size() - 1)
         d->incrementStartIndices(index);
     return index;
@@ -464,8 +477,7 @@ void TimelineModel::insertEnd(int index, qint64 duration)
 {
     Q_D(TimelineModel);
     d->ranges[index].duration = duration;
-    d->insertSorted(d->endTimes, TimelineModelPrivate::RangeEnd(index,
+    d->insertEnd(TimelineModelPrivate::RangeEnd(index, d->ranges[index].start + duration));
-            d->ranges[index].start + duration));
 }
 
 bool TimelineModel::expanded() const

src/libs/timeline/timelinemodel_p.h

@@ -82,16 +82,31 @@ public:
         }
     }
 
-    template<typename RangeDelimiter>
+    inline int insertStart(const Range &start)
-    static inline int insertSorted(QVector<RangeDelimiter> &container, const RangeDelimiter &item)
     {
-        for (int i = container.count();;) {
+        for (int i = ranges.count();;) {
             if (i == 0) {
-                container.prepend(item);
+                ranges.prepend(start);
                 return 0;
             }
-            if (container[--i].timestamp() <= item.timestamp()) {
+            const Range &range = ranges[--i];
-                container.insert(++i, item);
+            if (range.start < start.start || (range.start == start.start &&
+                                              range.duration >= start.duration)) {
+                ranges.insert(++i, start);
+                return i;
+            }
+        }
+    }
+
+    inline int insertEnd(const RangeEnd &end)
+    {
+        for (int i = endTimes.count();;) {
+            if (i == 0) {
+                endTimes.prepend(end);
+                return 0;
+            }
+            if (endTimes[--i].end <= end.end) {
+                endTimes.insert(++i, end);
                 return i;
             }
         }

tests/auto/timeline/timelinemodel/tst_timelinemodel.cpp

@@ -102,6 +102,10 @@ DummyModel::DummyModel(QString displayName, QObject *parent) :
 
 void DummyModel::loadData()
 {
+    // TimelineModel is clever enough to sort the longer events before the short ones with the same
+    // start time to avoid unnecessary complications when assigning parents. So, if you do e.g.
+    // firstIndex(0) you get the first event insered in the third iteration of the loop.
+
     for (int i = 0; i < NumItems / 4; ++i) {
         insert((i / 3) * ItemSpacing, ItemDuration * (i + 1) + 2, 4);
         insert((i / 3) * ItemSpacing + 1, ItemDuration * (i + 1), 5);
@@ -248,8 +252,8 @@ void tst_TimelineModel::times()
     DummyModel dummy;
     dummy.loadData();
     QCOMPARE(dummy.startTime(0), 0);
-    QCOMPARE(dummy.duration(0), ItemDuration + 2);
+    QCOMPARE(dummy.duration(0), ItemDuration * 3 + 2);
-    QCOMPARE(dummy.endTime(0), ItemDuration + 2);
+    QCOMPARE(dummy.endTime(0), ItemDuration * 3 + 2);
 
 }
 
@@ -272,8 +276,8 @@ void tst_TimelineModel::firstLast()
     QCOMPARE(dummy.lastIndex(5), 0);
     dummy.clear();
     dummy.loadData();
-    QCOMPARE(dummy.firstIndex(0), 2);
+    QCOMPARE(dummy.firstIndex(0), 0);
-    QCOMPARE(dummy.firstIndex(ItemSpacing + 1), 2);
+    QCOMPARE(dummy.firstIndex(ItemSpacing + 1), 0);
     QCOMPARE(dummy.lastIndex(0), -1);
     QCOMPARE(dummy.lastIndex(ItemSpacing + 1), 14);
     QCOMPARE(dummy.firstIndex(ItemDuration * 5000), -1);
@@ -362,7 +366,7 @@ void tst_TimelineModel::colorBySelectionId()
 {
     DummyModel dummy;
     dummy.loadData();
-    QCOMPARE(dummy.colorBySelectionId(5), QColor::fromHsl(5 * 25, 150, 166));
+    QCOMPARE(dummy.colorBySelectionId(5), QColor::fromHsl(6 * 25, 150, 166));
 }
 
 void tst_TimelineModel::colorByFraction()
@@ -388,6 +392,14 @@ void tst_TimelineModel::insertStartEnd()
     dummy.insertEnd(id2, 10);
     QCOMPARE(dummy.startTime(id2), 5);
     QCOMPARE(dummy.endTime(id2), 15);
+
+    int id3 = dummy.insertStart(10, 1);
+    QVERIFY(id3 > id);
+    dummy.insertEnd(id3, 20);
+
+    // Make sure that even though the new range is larger than the one pointed to by id, we still
+    // get to search from id when looking up a timestamp which is in both ranges.
+    QCOMPARE(dummy.firstIndex(11), id);
 }
 
 void tst_TimelineModel::rowCount()
@@ -426,7 +438,7 @@ void tst_TimelineModel::prevNext()
     QCOMPARE(dummy.nextItemBySelectionId(5, 10, -1), 15);
     QCOMPARE(dummy.prevItemBySelectionId(5, 10, -1), 6);
     QCOMPARE(dummy.nextItemBySelectionId(5, 10, 5), 6);
-    QCOMPARE(dummy.prevItemBySelectionId(5, 10, 5), 3);
+    QCOMPARE(dummy.prevItemBySelectionId(5, 10, 5), 4);
     QCOMPARE(dummy.nextItemByTypeId(-1, 10, 5), 6);
     QCOMPARE(dummy.prevItemByTypeId(-1, 10, 5), 4);
 }