Fix margins for right-to-left mode

The behaviour for considering left and right margins was inconsistent
in views with a right to left layout; these values were reversed for
extent calculations but not for general positioning. With this change
the left and right margins are never reversed in a right-to-left layout,
so minXExtent and maxXExtent calculations always use startMargin
and endMargin respectively, regardless of layout direction.

Also fixes calculation of endOffset in trackedPositionChanged() when
in horizontal orientation.

Change-Id: Ie00e3d4c2bd38d8fe6ac0213702206b88bfa895e
Reviewed-by: Martin Jones <martin.jones@nokia.com>

4 files changed, 47 insertions, 23 deletions

diff --git a/src/quick/items/qquickitemview.cpp b/src/quick/items/qquickitemview.cpp
index 50a3216bf0..0d95500860 100644
--- a/src/quick/items/qquickitemview.cpp
+++ b/src/quick/items/qquickitemview.cpp
@@ -1101,9 +1101,9 @@ void QQuickItemView::trackedPositionChanged()
                 if (d->layoutOrientation() == Qt::Vertical)
                     endOffset += d->vData.endMargin;
                 else if (d->isContentFlowReversed())
-                    endOffset += d->hData.endMargin;
-                else
                     endOffset += d->hData.startMargin;
+                else
+                    endOffset += d->hData.endMargin;
                 trackedPos += endOffset;
                 trackedEndPos += endOffset;
                 toItemPos += endOffset;
@@ -1204,12 +1204,11 @@ qreal QQuickItemView::minXExtent() const
         return QQuickFlickable::minXExtent();
 
     if (d->hData.minExtentDirty) {
-        d->minExtent = -d->startPosition();
+        d->minExtent = -d->startPosition() + d->hData.startMargin;
         qreal highlightStart;
         qreal highlightEnd;
         qreal endPositionFirstItem = 0;
         if (d->isContentFlowReversed()) {
-            d->minExtent += d->hData.endMargin;
             if (d->model && d->model->count())
                 endPositionFirstItem = d->positionAt(d->model->count()-1);
             else if (d->header)
@@ -1222,7 +1221,6 @@ qreal QQuickItemView::minXExtent() const
             if (d->minExtent < maxX)
                 d->minExtent = maxX;
         } else {
-            d->minExtent += d->hData.startMargin;
             endPositionFirstItem = d->endPositionAt(0);
             highlightStart = d->highlightRangeStart;
             highlightEnd = d->highlightRangeEnd;
@@ -1279,7 +1277,7 @@ qreal QQuickItemView::maxXExtent() const
         if (d->isContentFlowReversed()) {
             if (d->header)
                 d->maxExtent -= d->headerSize();
-            d->maxExtent -= d->hData.startMargin;
+            d->maxExtent -= d->hData.endMargin;
         } else {
             if (d->footer)
                 d->maxExtent -= d->footerSize();
@@ -1314,7 +1312,7 @@ qreal QQuickItemView::xOrigin() const
 {
     Q_D(const QQuickItemView);
     if (d->isContentFlowReversed())
-        return -maxXExtent() + d->size() - d->hData.startMargin;
+        return -maxXExtent() + d->size() - d->hData.endMargin;
     else
         return -minXExtent() + d->hData.startMargin;
 }
diff --git a/tests/auto/quick/qquickgridview/tst_qquickgridview.cpp b/tests/auto/quick/qquickgridview/tst_qquickgridview.cpp
index 88ed94d8bc..c7b5ca6b40 100644
--- a/tests/auto/quick/qquickgridview/tst_qquickgridview.cpp
+++ b/tests/auto/quick/qquickgridview/tst_qquickgridview.cpp
@@ -3691,31 +3691,32 @@ void tst_QQuickGridView::margins()
         QQuickItem *contentItem = gridview->contentItem();
         QTRY_VERIFY(contentItem != 0);
 
-        QCOMPARE(gridview->contentX(), -240+30.);
-        QCOMPARE(gridview->xOrigin(), 0.);
+        QTRY_COMPARE(gridview->contentX(), -240+50.);
+        QTRY_COMPARE(gridview->xOrigin(), 0.);
 
         // check end bound
         gridview->positionViewAtEnd();
         qreal pos = gridview->contentX();
         gridview->setContentX(pos - 80);
         gridview->returnToBounds();
-        QTRY_COMPARE(gridview->contentX(), pos - 50);
+        QTRY_COMPARE(gridview->contentX(), pos - 30);
 
         // remove item before visible and check that left margin is maintained
         // and xOrigin is updated
         gridview->setContentX(-400);
+        QTRY_COMPARE(QQuickItemPrivate::get(gridview)->polishScheduled, false);
         model.removeItems(0, 4);
-        QTest::qWait(100);
+        QTRY_COMPARE(model.count(), gridview->count());
         gridview->setContentX(-240+50);
         gridview->returnToBounds();
         QCOMPARE(gridview->xOrigin(), -100.);
-        QTRY_COMPARE(gridview->contentX(), -240-70.);
+        QTRY_COMPARE(gridview->contentX(), -240-50.);
 
-        // reduce left margin (i.e. right side due to RTL)
+        // reduce right margin
         pos = gridview->contentX();
-        gridview->setLeftMargin(20);
+        gridview->setRightMargin(40);
         QCOMPARE(gridview->xOrigin(), -100.);
-        QTRY_COMPARE(gridview->contentX(), -240-80.);
+        QTRY_COMPARE(gridview->contentX(), -240-100 + 40.);
 
         // check end bound
         gridview->positionViewAtEnd();
@@ -3723,11 +3724,11 @@ void tst_QQuickGridView::margins()
         pos = gridview->contentX();
         gridview->setContentX(pos - 80);
         gridview->returnToBounds();
-        QTRY_COMPARE(gridview->contentX(), pos - 50);
+        QTRY_COMPARE(gridview->contentX(), pos - 30);
 
-        // reduce right margin (i.e. left side due to RTL)
+        // reduce left margin
         pos = gridview->contentX();
-        gridview->setRightMargin(40);
+        gridview->setLeftMargin(20);
         QCOMPARE(gridview->xOrigin(), 0.);
         QTRY_COMPARE(gridview->contentX(), pos+10);
 
diff --git a/tests/auto/quick/qquicklistview/data/margins2.qml b/tests/auto/quick/qquicklistview/data/margins2.qml
index e11c803c4b..4b1f2546bf 100644
--- a/tests/auto/quick/qquicklistview/data/margins2.qml
+++ b/tests/auto/quick/qquicklistview/data/margins2.qml
@@ -9,9 +9,9 @@ Item {
         }
         ListView {
             objectName: "listview"
-            topMargin: 20
+            topMargin: 40
             bottomMargin: 20
-            leftMargin: 20
+            leftMargin: 40
             rightMargin: 20
             anchors.fill: parent
 
diff --git a/tests/auto/quick/qquicklistview/tst_qquicklistview.cpp b/tests/auto/quick/qquicklistview/tst_qquicklistview.cpp
index 202f5164af..461a6b6750 100644
--- a/tests/auto/quick/qquicklistview/tst_qquicklistview.cpp
+++ b/tests/auto/quick/qquicklistview/tst_qquicklistview.cpp
@@ -4290,6 +4290,13 @@ void tst_QQuickListView::marginsResize()
     QFETCH(qreal, start);
     QFETCH(qreal, end);
 
+    QPoint flickStart(20, 20);
+    QPoint flickEnd(20, 20);
+    if (orientation == QQuickListView::Vertical)
+        flickStart.ry() += 180;
+    else
+        flickStart.rx() += (layoutDirection == Qt::LeftToRight) ? 180 : -180;
+
     QQuickView *canvas = getView();
 
     canvas->setSource(testFileUrl("margins2.qml"));
@@ -4316,6 +4323,14 @@ void tst_QQuickListView::marginsResize()
     else
         QTRY_COMPARE(listview->contentX(), end);
 
+    // flick past the end and check content pos still settles on correct extents
+    flick(canvas, flickStart, flickEnd, 180);
+    QTRY_VERIFY(listview->isMoving() == false);
+    if (orientation == QQuickListView::Vertical)
+        QTRY_COMPARE(listview->contentY(), end);
+    else
+        QTRY_COMPARE(listview->contentX(), end);
+
     // back to top - top margin should be visible.
     listview->setCurrentIndex(0);
     if (orientation == QQuickListView::Vertical)
@@ -4323,6 +4338,14 @@ void tst_QQuickListView::marginsResize()
     else
         QTRY_COMPARE(listview->contentX(), start);
 
+    // flick past the beginning and check content pos still settles on correct extents
+    flick(canvas, flickEnd, flickStart, 180);
+    QTRY_VERIFY(listview->isMoving() == false);
+    if (orientation == QQuickListView::Vertical)
+        QTRY_COMPARE(listview->contentY(), start);
+    else
+        QTRY_COMPARE(listview->contentX(), start);
+
     releaseView(canvas);
 }
 
@@ -4333,9 +4356,11 @@ void tst_QQuickListView::marginsResize_data()
     QTest::addColumn<qreal>("start");
     QTest::addColumn<qreal>("end");
 
-    QTest::newRow("vertical") << QQuickListView::Vertical << Qt::LeftToRight << -20.0 << 1020.0;
-    QTest::newRow("horizontal") << QQuickListView::Horizontal << Qt::LeftToRight << -20.0 << 1020.0;
-    QTest::newRow("horizontal, rtl") << QQuickListView::Horizontal << Qt::RightToLeft << -180.0 << -1220.0;
+    // in Right to Left mode, leftMargin still means leftMargin - it doesn't reverse to mean rightMargin
+
+    QTest::newRow("vertical") << QQuickListView::Vertical << Qt::LeftToRight << -40.0 << 1020.0;
+    QTest::newRow("horizontal") << QQuickListView::Horizontal << Qt::LeftToRight << -40.0 << 1020.0;
+    QTest::newRow("horizontal, rtl") << QQuickListView::Horizontal << Qt::RightToLeft << -180.0 << -1240.0;
 }
 
 void tst_QQuickListView::snapToItem_data()