Fix clearing selection in a grid with reordered columns

Since the changes of 04f7f1fd32 (frozen
rows/columns implementation), RefreshBlock() could call GetColPos() with
an invalid index. This didn't matter most of the time as the function
simply returned the same index as long as the columns were using their
natural order, but resulted in a crash due to an out of bound access to
m_colAt array as soon as they were reordered.

Fix this by avoiding using invalid indices in RefreshBlock() and, more
generally, improving its precondition check and making the assumptions
about the input parameters more clear. Also add a defensive check to
GetColPos() itself.

Finally, add a unit test exercising this code.

Closes https://github.com/wxWidgets/wxWidgets/pull/1536

include/wx/generic/grid.h

@@ -1131,6 +1131,8 @@ public:
                          const wxArrayString& lines,
                          long *width, long *height ) const;
 
+    // If bottomRight is invalid, i.e. == wxGridNoCellCoords, it defaults to
+    // topLeft. If topLeft itself is invalid, the function simply returns.
     void RefreshBlock(const wxGridCellCoords& topLeft,
                       const wxGridCellCoords& bottomRight);
 
@@ -1522,6 +1524,8 @@ public:
     // reverse mapping currently
     int GetColPos(int idx) const
     {
+        wxASSERT_MSG( idx >= 0 && idx < m_numCols, "invalid column index" );
+
         if ( m_colAt.IsEmpty() )
             return idx;
 

src/generic/grid.cpp

@@ -5277,6 +5277,33 @@ void wxGrid::RefreshBlock(const wxGridCellCoords& topLeft,
 void wxGrid::RefreshBlock(int topRow, int leftCol,
                           int bottomRow, int rightCol)
 {
+    // Note that it is valid to call this function with wxGridNoCellCoords as
+    // either or even both arguments, but we can't have a mix of valid and
+    // invalid columns/rows for each corner coordinates.
+    const bool noTopLeft = topRow == -1 || leftCol == -1;
+    const bool noBottomRight = bottomRow == -1 || rightCol == -1;
+
+    if ( noTopLeft )
+    {
+        // So check that either both or none of the components are valid.
+        wxASSERT( topRow == -1 && leftCol == -1 );
+
+        // And specifying bottom right corner when the top left one is not
+        // specified doesn't make sense neither.
+        wxASSERT( noBottomRight );
+
+        return;
+    }
+
+    if ( noBottomRight )
+    {
+        wxASSERT( bottomRow == -1 && rightCol == -1 );
+
+        bottomRow = topRow;
+        rightCol = leftCol;
+    }
+
+
     int row = topRow;
     int col = leftCol;
 

tests/controls/gridtest.cpp

@@ -49,6 +49,7 @@ private:
     CPPUNIT_TEST_SUITE( GridTestCase );
         WXUISIM_TEST( CellEdit );
         NONGTK_TEST( CellClick );
+        NONGTK_TEST( ReorderedColumnsCellClick );
         NONGTK_TEST( CellSelect );
         NONGTK_TEST( LabelClick );
         NONGTK_TEST( SortClick );
@@ -83,6 +84,7 @@ private:
 
     void CellEdit();
     void CellClick();
+    void ReorderedColumnsCellClick();
     void CellSelect();
     void LabelClick();
     void SortClick();
@@ -232,6 +234,35 @@ void GridTestCase::CellClick()
 #endif
 }
 
+void GridTestCase::ReorderedColumnsCellClick()
+{
+#if wxUSE_UIACTIONSIMULATOR
+    EventCounter click(m_grid, wxEVT_GRID_CELL_LEFT_CLICK);
+
+    wxUIActionSimulator sim;
+
+    wxArrayInt neworder;
+    neworder.push_back(1);
+    neworder.push_back(0);
+
+    m_grid->SetColumnsOrder(neworder);
+
+    wxRect rect = m_grid->CellToRect(0, 1);
+    wxPoint point = m_grid->CalcScrolledPosition(rect.GetPosition());
+    point = m_grid->ClientToScreen(point + wxPoint(m_grid->GetRowLabelSize(),
+        m_grid->GetColLabelSize())
+        + wxPoint(2, 2));
+
+    sim.MouseMove(point);
+    wxYield();
+
+    sim.MouseClick();
+    wxYield();
+
+    CPPUNIT_ASSERT_EQUAL(1, click.GetCount());
+#endif
+}
+
 void GridTestCase::CellSelect()
 {
 #if wxUSE_UIACTIONSIMULATOR