From c4f9629c130d507c9536d93467ee76eb5fa30539 2013-04-24 07:22:35 From: Miikka Heikkinen Date: 2013-04-24 07:22:35 Subject: [PATCH] Fix crash when adding/removing points during animation Adding/removing points during the animation of the previous add/remove operation resulted in a mismatch between visible point count and the actual point count, leading to crashes in code that assumed both to be the same. Added necessary safeguards and improved autotests to detect these cases. Task-number: QTRD-1984 Change-Id: I544d10a69e760a40d4c90a4f02de61d4d1bb974f Reviewed-by: Tomi Korpipää --- diff --git a/src/animations/xyanimation.cpp b/src/animations/xyanimation.cpp index 9f66b4a..e8ad280 100644 --- a/src/animations/xyanimation.cpp +++ b/src/animations/xyanimation.cpp @@ -59,15 +59,21 @@ void XYAnimation::setup(const QVector &oldPoints, const QVector= 0 && y > 0) { + int diff = x - y; + int requestedDiff = oldPoints.count() - y; + + // m_oldPoints can be whatever between 0 and actual points count if new animation setup + // interrupts a previous animation, so only do remove and add animations if both + // stored diff and requested diff indicate add or remove. Also ensure that index is not + // invalid. + if (diff == 1 && requestedDiff == 1 && index >= 0 && y > 0 && index <= y) { //remove point m_newPoints.insert(index, index > 0 ? newPoints[index - 1] : newPoints[index]); m_index = index; m_type = RemovePointAnimation; } - if (x - y == -1 && index >= 0) { + if (diff == -1 && requestedDiff == -1 && index >= 0 && index <= x) { //add point m_oldPoints.insert(index, index > 0 ? newPoints[index - 1] : newPoints[index]); m_index = index; diff --git a/src/linechart/linechartitem.cpp b/src/linechart/linechartitem.cpp index dd5f540..ef2a350 100644 --- a/src/linechart/linechartitem.cpp +++ b/src/linechart/linechartitem.cpp @@ -113,6 +113,9 @@ void LineChartItem::updateGeometry() qreal rightMarginLine = centerPoint.x() + margin; qreal horizontal = centerPoint.y(); + // See ScatterChartItem::updateGeometry() for explanation why seriesLastIndex is needed + const int seriesLastIndex = m_series->count() - 1; + for (int i = 1; i < points.size(); i++) { // Interpolating line fragments would be ugly when thick pen is used, // so we work around it by utilizing three separate @@ -126,7 +129,7 @@ void LineChartItem::updateGeometry() // degrees and both of the points are within the margin, one in the top half and one in the // bottom half of the chart, the bottom one gets clipped incorrectly. // However, this should be rare occurrence in any sensible chart. - currentSeriesPoint = m_series->pointAt(i); + currentSeriesPoint = m_series->pointAt(qMin(seriesLastIndex, i)); currentGeometryPoint = points.at(i); pointOffGrid = (currentSeriesPoint.x() < minX || currentSeriesPoint.x() > maxX); diff --git a/src/scatterchart/scatterchartitem.cpp b/src/scatterchart/scatterchartitem.cpp index fecbeb1..6889fdc 100644 --- a/src/scatterchart/scatterchartitem.cpp +++ b/src/scatterchart/scatterchartitem.cpp @@ -127,12 +127,20 @@ void ScatterChartItem::updateGeometry() QRectF clipRect(QPointF(0,0),domain()->size()); QVector offGridStatus = offGridStatusVector(); + const int seriesLastIndex = m_series->count() - 1; for (int i = 0; i < points.size(); i++) { QGraphicsItem *item = items.at(i); const QPointF &point = points.at(i); const QRectF &rect = item->boundingRect(); - m_markerMap[item] = m_series->pointAt(i); + // During remove/append animation series may have different number of points, + // so ensure we don't go over the index. Animation handling itself ensures that + // if there is actually no points in the series, then it won't generate a fake point, + // so we can be assured there is always at least one point in m_series here. + // Note that marker map values can be technically incorrect during the animation, + // if it was caused by an insert, but this shouldn't be a problem as the points are + // fake anyway. + m_markerMap[item] = m_series->pointAt(qMin(seriesLastIndex, i)); item->setPos(point.x() - rect.width() / 2, point.y() - rect.height() / 2); if (!m_visible || offGridStatus.at(i)) diff --git a/src/splinechart/splinechartitem.cpp b/src/splinechart/splinechartitem.cpp index 4a46a72..4f1162c 100644 --- a/src/splinechart/splinechartitem.cpp +++ b/src/splinechart/splinechartitem.cpp @@ -141,6 +141,9 @@ void SplineChartItem::updateGeometry() qreal rightMarginLine = centerPoint.x() + margin; qreal horizontal = centerPoint.y(); + // See ScatterChartItem::updateGeometry() for explanation why seriesLastIndex is needed + const int seriesLastIndex = m_series->count() - 1; + for (int i = 1; i < points.size(); i++) { // Interpolating spline fragments accurately is not trivial, and would anyway be ugly // when thick pen is used, so we work around it by utilizing three separate @@ -154,7 +157,7 @@ void SplineChartItem::updateGeometry() // degrees and both of the points are within the margin, one in the top half and one in the // bottom half of the chart, the bottom one gets clipped incorrectly. // However, this should be rare occurrence in any sensible chart. - currentSeriesPoint = m_series->pointAt(i); + currentSeriesPoint = m_series->pointAt(qMin(seriesLastIndex, i)); currentGeometryPoint = points.at(i); pointOffGrid = (currentSeriesPoint.x() < minX || currentSeriesPoint.x() > maxX); diff --git a/src/xychart/xychart.cpp b/src/xychart/xychart.cpp index 3d442c5..a018119 100644 --- a/src/xychart/xychart.cpp +++ b/src/xychart/xychart.cpp @@ -73,9 +73,13 @@ QVector XYChart::offGridStatusVector() QVector returnVector; returnVector.resize(m_points.size()); + // During remove/append animation series may have different number of points, + // so ensure we don't go over the index. No need to check for zero points, this + // will not be called in such a situation. + const int seriesLastIndex = m_series->count() - 1; for (int i = 0; i < m_points.size(); i++) { - const QPointF &seriesPoint = m_series->pointAt(i); + const QPointF &seriesPoint = m_series->pointAt(qMin(seriesLastIndex, i)); if (seriesPoint.x() < minX || seriesPoint.x() > maxX || seriesPoint.y() < minY diff --git a/tests/auto/qxyseries/tst_qxyseries.cpp b/tests/auto/qxyseries/tst_qxyseries.cpp index 7988e9b..d3c15a1 100644 --- a/tests/auto/qxyseries/tst_qxyseries.cpp +++ b/tests/auto/qxyseries/tst_qxyseries.cpp @@ -84,8 +84,13 @@ void tst_QXYSeries::seriesOpacity() void tst_QXYSeries::append_data() { QTest::addColumn< QList >("points"); - QTest::newRow("0,0 1,1 2,2 3,3") << (QList() << QPointF(0,0) << QPointF(1,1) << QPointF(2,2) << QPointF(3,3)); - QTest::newRow("0,0 -1,-1 -2,-2 -3,-3") << (QList() << QPointF(0,0) << QPointF(-1,-1) << QPointF(-2,-2) << QPointF(-3,-3)); + QTest::addColumn< QList >("otherPoints"); + QTest::newRow("0,0 1,1 2,2 3,3") + << (QList() << QPointF(0,0) << QPointF(1,1) << QPointF(2,2) << QPointF(3,3)) + << (QList() << QPointF(4,4) << QPointF(5,5) << QPointF(6,6) << QPointF(7,7)); + QTest::newRow("0,0 -1,-1 -2,-2 -3,-3") + << (QList() << QPointF(0,0) << QPointF(-1,-1) << QPointF(-2,-2) << QPointF(-3,-3)) + << (QList() << QPointF(-4,-4) << QPointF(-5,-5) << QPointF(-6,-6) << QPointF(-7,-7)); } @@ -97,12 +102,19 @@ void tst_QXYSeries::append_raw_data() void tst_QXYSeries::append_raw() { QFETCH(QList, points); + QFETCH(QList, otherPoints); QSignalSpy spy0(m_series, SIGNAL(clicked(QPointF))); QSignalSpy addedSpy(m_series, SIGNAL(pointAdded(int))); m_series->append(points); TRY_COMPARE(spy0.count(), 0); TRY_COMPARE(addedSpy.count(), points.count()); QCOMPARE(m_series->points(), points); + + // Process events between appends + foreach (const QPointF &point, otherPoints) { + m_series->append(point); + QApplication::processEvents(); + } } void tst_QXYSeries::chart_append_data() @@ -196,6 +208,29 @@ void tst_QXYSeries::remove_raw() m_series->remove(points[i]); } QCOMPARE(m_series->points().count(), 0); + + QApplication::processEvents(); + + // Process events between removes + m_series->append(points); + QCOMPARE(m_series->points(), points); + foreach (const QPointF &point, points) { + m_series->remove(point); + QApplication::processEvents(); + } + + // Actual meaningful delay between removes, but still shorter than animation duration + // (simulate e.g. spamming a hypothetical "remove last point"-button) + QList bunchOfPoints; + for (int i = 0; i < 10; i++) + bunchOfPoints.append(QPointF(i, (qreal) rand() / (qreal) RAND_MAX)); + m_series->replace(bunchOfPoints); + QCOMPARE(m_series->points(), bunchOfPoints); + QTest::qWait(1500); // Wait for append animations to be over + for (int i = bunchOfPoints.count() - 1; i >= 0; i--) { + m_series->remove(bunchOfPoints.at(i)); + QTest::qWait(50); + } } void tst_QXYSeries::remove_chart_data() @@ -238,6 +273,8 @@ void tst_QXYSeries::clear_raw() m_series->clear(); TRY_COMPARE(spy0.count(), 0); QCOMPARE(m_series->points().count(), 0); + + QApplication::processEvents(); } void tst_QXYSeries::clear_chart_data() @@ -272,6 +309,7 @@ void tst_QXYSeries::replace_raw_data() void tst_QXYSeries::replace_raw() { QFETCH(QList, points); + QFETCH(QList, otherPoints); QSignalSpy pointReplacedSpy(m_series, SIGNAL(pointReplaced(int))); QSignalSpy pointsReplacedSpy(m_series, SIGNAL(pointsReplaced())); m_series->append(points); @@ -303,6 +341,31 @@ void tst_QXYSeries::replace_raw() m_series->replace(allPoints); TRY_COMPARE(pointReplacedSpy.count(), points.count()); TRY_COMPARE(pointsReplacedSpy.count(), 1); + + m_series->replace(points); + QApplication::processEvents(); + + // Process events between replaces + for (int i = 0; i < points.count(); ++i) { + m_series->replace(points.at(i), otherPoints.at(i)); + QApplication::processEvents(); + } + + newPoints = m_series->points(); + QCOMPARE(newPoints.count(), points.count()); + for (int i = 0; i < points.count(); ++i) { + QCOMPARE(otherPoints.at(i).x(), newPoints.at(i).x()); + QCOMPARE(otherPoints.at(i).y(), newPoints.at(i).y()); + } + + // Append followed by a replace shouldn't crash + m_series->clear(); + m_series->append(QPointF(22,22)); + m_series->append(QPointF(23,23)); + QApplication::processEvents(); + m_series->replace(QPointF(23,23), otherPoints.at(1)); + QCOMPARE(m_series->points().at(1).x(), otherPoints.at(1).x()); + QCOMPARE(m_series->points().at(1).y(), otherPoints.at(1).y()); }