Fix Shift-Ctrl-arrows handling

Extending the selection with Ctrl-arrows is different from all the other
cases, as we need to combine both the selection anchor and the current
cell coordinates when doing it.

This means that we can't reuse the same PrepareForSelectionExpansion()
helper for this case, so this function is not useful finally and this
commit removes it entirely. It also replaces GetCurrentBlockCornerRow()
and GetCurrentBlockCornerCol() functions with GetExtensionAnchor() which
combines both of them.

Finally, it adds wxGridDirectionOperations::TryToAdvance() helper to
avoid repeating the IsAtBoundary() check which was previously part of
PrepareForSelectionExpansion() in multiple places.

And because the "extending" and normal parts of DoMoveCursorByBlock()
are so different now, it also factors out AdvanceByBlock() helper which
can be used to keep these parts well separate from each other instead of
intermixing them together.

With all these preparatory changes, it's finally possible to implement
the "extending selection by block" logic relatively easily, with the
bulk of this branch actually taken by comments explaining why do we have
to do what we do.

Add unit tests verifying that the functions used by Shift-Ctrl-arrow
work as expected.

include/wx/generic/grid.h

@@ -2767,12 +2767,6 @@ private:
                   wxGridWindow *gridWindow) const;
     int PosToEdgeOfLine(int pos, const wxGridOperations& oper) const;
 
-    // Fill the coords with the cell coordinates to use for the movement
-    // extending the current selection. Return false if, for whatever reason,
-    // we can't expand the selection at all.
-    bool PrepareForSelectionExpansion(wxGridCellCoords& coords,
-                                      const wxGridDirectionOperations& diroper);
-
     void DoMoveCursorFromKeyboard(const wxKeyboardState& kbdState,
                                   const wxGridDirectionOperations& diroper);
     bool DoMoveCursor(const wxKeyboardState& kbdState,
@@ -2782,6 +2776,8 @@ private:
                              const wxGridDirectionOperations& diroper);
     void AdvanceToNextNonEmpty(wxGridCellCoords& coords,
                                const wxGridDirectionOperations& diroper);
+    bool AdvanceByBlock(wxGridCellCoords& coords,
+                        const wxGridDirectionOperations& diroper);
 
     // common part of {Insert,Delete}{Rows,Cols}
     bool DoModifyLines(bool (wxGridTableBase::*funcModify)(size_t, size_t),

include/wx/generic/gridsel.h

@@ -86,12 +86,12 @@ public:
                             const wxKeyboardState& kbd);
 
 
-    // Return the row of the current selection block if it exists and we can
+    // Return the coordinates of the cell from which the selection should
-    // edit the block vertically. Otherwise return -1.
+    // continue to be extended. This is normally the opposite corner of the
-    int GetCurrentBlockCornerRow() const;
+    // last selected block from the current cell coordinates.
-    // Return the column of the current selection block if it exists and we can
+    //
-    // edit the block horizontally. Otherwise return -1.
+    // If there is no selection, just returns the current cell coordinates.
-    int GetCurrentBlockCornerCol() const;
+    wxGridCellCoords GetExtensionAnchor() const;
 
     wxGridCellCoordsArray GetCellSelection() const;
     wxGridCellCoordsArray GetBlockSelectionTopLeft() const;

include/wx/generic/private/grid.h

@@ -807,8 +807,23 @@ public:
     }
 
     // Increment the component of this point in our direction
+    //
+    // Note that this can't be called if IsAtBoundary() is true, use
+    // TryToAdvance() if this might be the case.
     virtual void Advance(wxGridCellCoords& coords) const = 0;
 
+    // Try to advance in our direction, return true if succeeded or false
+    // otherwise, i.e. if the coordinates are already at the grid boundary.
+    bool TryToAdvance(wxGridCellCoords& coords) const
+    {
+        if ( IsAtBoundary(coords) )
+            return false;
+
+        Advance(coords);
+
+        return true;
+    }
+
     // Find the line at the given distance, in pixels, away from this one
     // (this uses clipping, i.e. anything after the last line is counted as the
     // last one and anything before the first one as 0)

src/generic/grid.cpp

@@ -5753,16 +5753,12 @@ void wxGrid::OnKeyDown( wxKeyEvent& event )
                     }
                     else
                     {
-                        int currentBlockRow = -1;
+                        // If we're extending the selection, try to continue in
-                        if ( m_selection )
+                        // the same row, which may well be different from the
-                            currentBlockRow = m_selection->GetCurrentBlockCornerRow();
+                        // one in which we started selecting.
-
+                        if ( m_selection && event.ShiftDown() )
-                        // If we're selecting, continue in the same row, which
-                        // may well be different from the one in which we
-                        // started selecting.
-                        if ( event.ShiftDown() && currentBlockRow != -1 )
                         {
-                            row = currentBlockRow;
+                            row = m_selection->GetExtensionAnchor().GetRow();
                         }
                         else // Just use the current row.
                         {
@@ -7924,34 +7920,6 @@ wxKeyboardState DummyKeyboardState(bool expandSelection)
 
 } // anonymous namespace
 
-bool
-wxGrid::PrepareForSelectionExpansion(wxGridCellCoords& coords,
-                                     const wxGridDirectionOperations& diroper)
-{
-    int row = m_selection->GetCurrentBlockCornerRow();
-    if ( row == -1 )
-        row = m_currentCellCoords.GetRow();
-
-    int col = m_selection->GetCurrentBlockCornerCol();
-    if ( col == -1 )
-        col = m_currentCellCoords.GetCol();
-
-    coords.Set(row, col);
-
-    if ( !diroper.IsValid(coords) )
-    {
-        // The component of the current block corner in our direction
-        // is not valid. This means we can't change the selection block
-        // in this direction.
-        return false;
-    }
-
-    if ( diroper.IsAtBoundary(coords) )
-        return false;
-
-    return true;
-}
-
 void
 wxGrid::DoMoveCursorFromKeyboard(const wxKeyboardState& kbdState,
                                  const wxGridDirectionOperations& diroper)
@@ -7975,12 +7943,10 @@ wxGrid::DoMoveCursor(const wxKeyboardState& kbdState,
         if ( !m_selection )
             return false;
 
-        wxGridCellCoords coords;
+        wxGridCellCoords coords(m_selection->GetExtensionAnchor());
-        if ( !PrepareForSelectionExpansion(coords, diroper) )
+        if ( !diroper.TryToAdvance(coords) )
             return false;
 
-        diroper.Advance(coords);
-
         if ( m_selection->ExtendCurrentBlock(m_currentCellCoords,
                                              coords,
                                              kbdState) )
@@ -7995,11 +7961,9 @@ wxGrid::DoMoveCursor(const wxKeyboardState& kbdState,
     {
         ClearSelection();
 
-        if ( diroper.IsAtBoundary(m_currentCellCoords) )
-            return false;
-
         wxGridCellCoords coords = m_currentCellCoords;
-        diroper.Advance(coords);
+        if ( !diroper.TryToAdvance(coords) )
+            return false;
 
         GoToCell(coords);
     }
@@ -8080,25 +8044,9 @@ wxGrid::AdvanceToNextNonEmpty(wxGridCellCoords& coords,
 }
 
 bool
-wxGrid::DoMoveCursorByBlock(const wxKeyboardState& kbdState,
+wxGrid::AdvanceByBlock(wxGridCellCoords& coords,
-                            const wxGridDirectionOperations& diroper)
+                       const wxGridDirectionOperations& diroper)
 {
-    if ( !m_table )
-        return false;
-
-    wxGridCellCoords coords;
-
-    // Expand selection if Shift is pressed.
-    if ( kbdState.ShiftDown() )
-    {
-        if ( !PrepareForSelectionExpansion(coords, diroper) )
-            return false;
-    }
-    else
-    {
-        coords = m_currentCellCoords;
-    }
-
     if ( m_table->IsEmpty(coords) )
     {
         // we are in an empty cell: find the next block of non-empty cells
@@ -8106,7 +8054,9 @@ wxGrid::DoMoveCursorByBlock(const wxKeyboardState& kbdState,
     }
     else // current cell is not empty
     {
-        diroper.Advance(coords);
+        if ( !diroper.TryToAdvance(coords) )
+            return false;
+
         if ( m_table->IsEmpty(coords) )
         {
             // we started at the end of a block, find the next one
@@ -8128,23 +8078,81 @@ wxGrid::DoMoveCursorByBlock(const wxKeyboardState& kbdState,
         }
     }
 
+    return true;
+}
+
+bool
+wxGrid::DoMoveCursorByBlock(const wxKeyboardState& kbdState,
+                            const wxGridDirectionOperations& diroper)
+{
+    if ( !m_table )
+        return false;
+
+    wxGridCellCoords coords(m_currentCellCoords);
     if ( kbdState.ShiftDown() )
     {
-        if ( m_selection )
+        if ( !m_selection )
+            return false;
+
+        // Extending selection by block is tricky for several reasons. First of
+        // all, we need to combine the coordinates of the current cell and the
+        // anchor point of selection to find our starting point.
+        //
+        // To explain why we need to do this, consider the example of moving by
+        // rows (i.e. vertically). In this case, it's the contents of column
+        // containing the current cell which should determine the destination
+        // of Shift-Ctrl-Up/Down movement, just as it does for Ctrl-Up/Down. In
+        // fact, the column containing the selection anchor might not even be
+        // visible at all, e.g. if a whole row is selected and that column is
+        // the last one, so we definitely don't want to use the contents of
+        // this column to determine the destination row.
+        //
+        // So instead of using the anchor itself here, use only its component
+        // component in "our" direction with the current cell component.
+        const wxGridCellCoords anchor = m_selection->GetExtensionAnchor();
+
+        // This is a really ugly hack that we use to check if we're moving by
+        // rows or columns here, but it's not worth adding a specific method
+        // just for this, so just check if MakeWholeLineCoords() fixes the
+        // column or the row as -1 to determine the direction we're moving in.
+        const bool byRow = diroper.MakeWholeLineCoords(coords).GetCol() == -1;
+
+        if ( byRow )
+            coords.SetRow(anchor.GetRow());
+        else
+            coords.SetCol(anchor.GetCol());
+
+        if ( !AdvanceByBlock(coords, diroper) )
+            return false;
+
+        // Second, now that we've found the destination, we need to copy the
+        // other component, i.e. the one we didn't modify, from the anchor to
+        // do as if we started from the anchor originally.
+        //
+        // Again, to understand why this is necessary, consider the same
+        // example as above: if we didn't do this, we would lose the column of
+        // the original anchor completely and end up with a single column
+        // selection block even if we had started with the full row selection.
+        if ( byRow )
+            coords.SetCol(anchor.GetCol());
+        else
+            coords.SetRow(anchor.GetRow());
+
+        if ( m_selection->ExtendCurrentBlock(m_currentCellCoords,
+                                             coords,
+                                             kbdState) )
         {
-            if ( m_selection->ExtendCurrentBlock(m_currentCellCoords,
+            // We want to show a line (a row or a column), not the end of
-                                                 coords,
+            // the selection block. And do it only if the selection block
-                                                 kbdState) )
+            // was actually changed.
-            {
+            MakeCellVisible(diroper.MakeWholeLineCoords(coords));
-                // We want to show a line (a row or a column), not the end of
-                // the selection block. And do it only if the selection block
-                // was actually changed.
-                MakeCellVisible(diroper.MakeWholeLineCoords(coords));
-            }
         }
     }
     else
     {
+        if ( !AdvanceByBlock(coords, diroper) )
+            return false;
+
         ClearSelection();
         GoToCell(coords);
     }

src/generic/gridsel.cpp

@@ -597,32 +597,25 @@ bool wxGridSelection::ExtendCurrentBlock(const wxGridCellCoords& blockStart,
     return true;
 }
 
-int wxGridSelection::GetCurrentBlockCornerRow() const
+wxGridCellCoords wxGridSelection::GetExtensionAnchor() const
 {
+    wxGridCellCoords coords = m_grid->m_currentCellCoords;
+
     if ( m_selection.empty() )
-        return -1;
+        return coords;
 
     const wxGridBlockCoords& block = *m_selection.rbegin();
-    if ( block.GetTopRow() == m_grid->m_currentCellCoords.GetRow() )
+    if ( block.GetTopRow() == coords.GetRow() )
-        return block.GetBottomRow();
+        coords.SetRow(block.GetBottomRow());
-    if ( block.GetBottomRow() == m_grid->m_currentCellCoords.GetRow() )
+    else if ( block.GetBottomRow() == coords.GetRow() )
-        return block.GetTopRow();
+        coords.SetRow(block.GetTopRow());
 
-    return -1;
+    if ( block.GetLeftCol() == coords.GetCol() )
-}
+        coords.SetCol(block.GetRightCol());
+    else if ( block.GetRightCol() == coords.GetCol() )
+        coords.SetCol(block.GetLeftCol());
 
-int wxGridSelection::GetCurrentBlockCornerCol() const
+    return coords;
-{
-    if ( m_selection.empty() )
-        return -1;
-
-    const wxGridBlockCoords& block = *m_selection.rbegin();
-    if ( block.GetLeftCol() == m_grid->m_currentCellCoords.GetCol() )
-        return block.GetRightCol();
-    if ( block.GetRightCol() == m_grid->m_currentCellCoords.GetCol() )
-        return block.GetLeftCol();
-
-    return -1;
 }
 
 wxGridCellCoordsArray wxGridSelection::GetCellSelection() const

tests/controls/gridtest.cpp

@@ -74,6 +74,17 @@ protected:
 
     void CheckFirstColAutoSize(int expected);
 
+    // Helper to check that the selection is equal to the specified block.
+    void CheckSelection(const wxGridBlockCoords& block)
+    {
+        const wxGridBlocks selected = m_grid->GetSelectedBlocks();
+        wxGridBlocks::iterator it = selected.begin();
+
+        REQUIRE( it != selected.end() );
+        CHECK( *it == block );
+        CHECK( ++it == selected.end() );
+    }
+
     TestableGrid *m_grid;
 
     wxDECLARE_NO_COPY_CLASS(GridTestCase);
@@ -490,6 +501,35 @@ TEST_CASE_METHOD(GridTestCase, "Grid::Cursor", "[grid]")
     CHECK(m_grid->GetGridCursorRow() == 0);
 }
 
+TEST_CASE_METHOD(GridTestCase, "Grid::KeyboardSelection", "[grid][selection]")
+{
+    m_grid->SetCellValue(1, 1, "R2C2");
+    m_grid->SetCellValue(2, 0, "R3C1");
+    m_grid->SetCellValue(3, 0, "R4C1");
+    m_grid->SetCellValue(4, 0, "R5C1");
+    m_grid->SetCellValue(7, 0, "R8C1");
+
+    CHECK(m_grid->GetGridCursorCoords() == wxGridCellCoords(0, 0));
+
+    m_grid->MoveCursorRight(true);
+    CheckSelection(wxGridBlockCoords(0, 0, 0, 1));
+
+    m_grid->MoveCursorDownBlock(true);
+    CheckSelection(wxGridBlockCoords(0, 0, 2, 1));
+
+    m_grid->MoveCursorDownBlock(true);
+    CheckSelection(wxGridBlockCoords(0, 0, 4, 1));
+
+    m_grid->MoveCursorDownBlock(true);
+    CheckSelection(wxGridBlockCoords(0, 0, 7, 1));
+
+    m_grid->MoveCursorUpBlock(true);
+    CheckSelection(wxGridBlockCoords(0, 0, 4, 1));
+
+    m_grid->MoveCursorLeft(true);
+    CheckSelection(wxGridBlockCoords(0, 0, 4, 0));
+}
+
 TEST_CASE_METHOD(GridTestCase, "Grid::Selection", "[grid]")
 {
     m_grid->SelectAll();