QHeaderView: fixing selection of reordered rows and columns

The old code incorrectly calculated visual region for a selection (specified by
QItemSelection object) in case of reordered / swapped rows or columns. It used
the leftmost and rightmost (upmost and downmost for vertical mode) logical
indexes directly. However some middle logical index (in case of reorder) may be
the leftmost or rightmost visual index. In such cases the repainting didn't
work properly. This fix first checks whether reordering / swapping is in use.
If it isn't (ie visual=logical) we use code similar to the old code. Otherwise
the new code scans all selected logical indexes, translates them into visual
ones and gets the correct leftmost and rightmost indexes.

[ChangeLog][QtWidgets][QHeaderView] Fixed a repainting issue when items had been reordered.

Task-number: QTBUG-50171
Change-Id: If6afabebf445cf32a8e0afe262b6ee149e7c4b2b
Reviewed-by: Thorbjørn Lund Martsum <tmartsum@gmail.com>

2 files changed, 94 insertions, 48 deletions

diff --git a/src/widgets/itemviews/qheaderview.cpp b/src/widgets/itemviews/qheaderview.cpp
index f04d580030..46684d325c 100644
--- a/src/widgets/itemviews/qheaderview.cpp
+++ b/src/widgets/itemviews/qheaderview.cpp
@@ -2957,31 +2957,43 @@ QRegion QHeaderView::visualRegionForSelection(const QItemSelection &selection) c
 {
     Q_D(const QHeaderView);
     const int max = d->modelSectionCount();
-    if (d->orientation == Qt::Horizontal) {
-        int left = max;
-        int right = 0;
-        int rangeLeft, rangeRight;
 
-        for (int i = 0; i < selection.count(); ++i) {
-            QItemSelectionRange r = selection.at(i);
-            if (r.parent().isValid() || !r.isValid())
-                continue; // we only know about toplevel items and we don't want invalid ranges
-            // FIXME an item inside the range may be the leftmost or rightmost
-            rangeLeft = visualIndex(r.left());
-            if (rangeLeft == -1) // in some cases users may change the selections
-                continue;        // before we have a chance to do the layout
-            rangeRight = visualIndex(r.right());
-            if (rangeRight == -1) // in some cases users may change the selections
-                continue;         // before we have a chance to do the layout
-            if (rangeLeft < left)
-                left = rangeLeft;
-            if (rangeRight > right)
-                right = rangeRight;
+    if (d->orientation == Qt::Horizontal) {
+        int logicalLeft = max;
+        int logicalRight = 0;
+
+        if (d->visualIndices.empty()) {
+            // If no reordered sections, skip redundant visual-to-logical transformations
+            for (int i = 0; i < selection.count(); ++i) {
+                const QItemSelectionRange &r = selection.at(i);
+                if (r.parent().isValid() || !r.isValid())
+                    continue; // we only know about toplevel items and we don't want invalid ranges
+                if (r.left() < logicalLeft)
+                    logicalLeft = r.left();
+                if (r.right() > logicalRight)
+                    logicalRight = r.right();
+            }
+        } else {
+            int left = max;
+            int right = 0;
+            for (int i = 0; i < selection.count(); ++i) {
+                const QItemSelectionRange &r = selection.at(i);
+                if (r.parent().isValid() || !r.isValid())
+                    continue; // we only know about toplevel items and we don't want invalid ranges
+                for (int k = r.left(); k <= r.right(); ++k) {
+                    int visual = visualIndex(k);
+                    if (visual == -1)   // in some cases users may change the selections
+                        continue;       // before we have a chance to do the layout
+                    if (visual < left)
+                        left = visual;
+                    if (visual > right)
+                        right = visual;
+                }
+            }
+            logicalLeft = logicalIndex(left);
+            logicalRight = logicalIndex(right);
         }
 
-        int logicalLeft = logicalIndex(left);
-        int logicalRight = logicalIndex(right);
-
         if (logicalLeft < 0  || logicalLeft >= count() ||
             logicalRight < 0 || logicalRight >= count())
             return QRegion();
@@ -2992,32 +3004,46 @@ QRegion QHeaderView::visualRegionForSelection(const QItemSelection &selection) c
         return QRect(leftPos, 0, rightPos - leftPos, height());
     }
     // orientation() == Qt::Vertical
-    int top = max;
-    int bottom = 0;
-    int rangeTop, rangeBottom;
-
-    for (int i = 0; i < selection.count(); ++i) {
-        QItemSelectionRange r = selection.at(i);
-        if (r.parent().isValid() || !r.isValid())
-            continue; // we only know about toplevel items
-        // FIXME an item inside the range may be the leftmost or rightmost
-        rangeTop = visualIndex(r.top());
-        if (rangeTop == -1) // in some cases users may change the selections
-            continue;       // before we have a chance to do the layout
-        rangeBottom = visualIndex(r.bottom());
-        if (rangeBottom == -1) // in some cases users may change the selections
-            continue;          // before we have a chance to do the layout
-        if (rangeTop < top)
-            top = rangeTop;
-        if (rangeBottom > bottom)
-            bottom = rangeBottom;
-    }
-
-    int logicalTop = logicalIndex(top);
-    int logicalBottom = logicalIndex(bottom);
-
-    if (logicalTop == -1 || logicalBottom == -1)
-        return QRect();
+    int logicalTop = max;
+    int logicalBottom = 0;
+
+    if (d->visualIndices.empty()) {
+        // If no reordered sections, skip redundant visual-to-logical transformations
+        for (int i = 0; i < selection.count(); ++i) {
+            const QItemSelectionRange &r = selection.at(i);
+            if (r.parent().isValid() || !r.isValid())
+                continue; // we only know about toplevel items and we don't want invalid ranges
+            if (r.top() < logicalTop)
+                logicalTop = r.top();
+            if (r.bottom() > logicalBottom)
+                logicalBottom = r.bottom();
+        }
+    } else {
+        int top = max;
+        int bottom = 0;
+
+        for (int i = 0; i < selection.count(); ++i) {
+            const QItemSelectionRange &r = selection.at(i);
+            if (r.parent().isValid() || !r.isValid())
+                continue; // we only know about toplevel items and we don't want invalid ranges
+            for (int k = r.top(); k <= r.bottom(); ++k) {
+                int visual = visualIndex(k);
+                if (visual == -1)   // in some cases users may change the selections
+                    continue;       // before we have a chance to do the layout
+                if (visual < top)
+                    top = visual;
+                if (visual > bottom)
+                    bottom = visual;
+            }
+        }
+
+        logicalTop = logicalIndex(top);
+        logicalBottom = logicalIndex(bottom);
+    }
+
+    if (logicalTop < 0 || logicalTop >= count() ||
+        logicalBottom < 0 || logicalBottom >= count())
+        return QRegion();
 
     int topPos = sectionViewportPosition(logicalTop);
     int bottomPos = sectionViewportPosition(logicalBottom) + sectionSize(logicalBottom);
diff --git a/tests/auto/widgets/itemviews/qheaderview/tst_qheaderview.cpp b/tests/auto/widgets/itemviews/qheaderview/tst_qheaderview.cpp
index 55fcf04846..4736aa5c12 100644
--- a/tests/auto/widgets/itemviews/qheaderview/tst_qheaderview.cpp
+++ b/tests/auto/widgets/itemviews/qheaderview/tst_qheaderview.cpp
@@ -77,6 +77,7 @@ public:
     void testEvent();
     void testhorizontalOffset();
     void testverticalOffset();
+    void testVisualRegionForSelection();
     friend class tst_QHeaderView;
 };
 
@@ -211,6 +212,7 @@ private slots:
     void QTBUG8650_crashOnInsertSections();
     void QTBUG12268_hiddenMovedSectionSorting();
     void QTBUG14242_hideSectionAutoSize();
+    void QTBUG50171_visualRegionForSwappedItems();
     void ensureNoIndexAtLength();
     void offsetConsistent();
 
@@ -2282,6 +2284,24 @@ void tst_QHeaderView::QTBUG14242_hideSectionAutoSize()
     QCOMPARE(calced_length, afterlength);
 }
 
+void tst_QHeaderView::QTBUG50171_visualRegionForSwappedItems()
+{
+    protected_QHeaderView headerView(Qt::Horizontal);
+    QtTestModel model;
+    model.rows = 2;
+    model.cols = 3;
+    headerView.setModel(&model);
+    headerView.swapSections(1, 2);
+    headerView.hideSection(0);
+    headerView.testVisualRegionForSelection();
+}
+
+void protected_QHeaderView::testVisualRegionForSelection()
+{
+    QRegion r = visualRegionForSelection(QItemSelection(model()->index(1, 0), model()->index(1, 2)));
+    QCOMPARE(r.boundingRect().contains(QRect(1, 1, length() - 2, 1)), true);
+}
+
 void tst_QHeaderView::ensureNoIndexAtLength()
 {
     QTableView qtv;