diff --git a/include/wx/generic/grid.h b/include/wx/generic/grid.h index cbd76d749c..e97d5a1f7d 100644 --- a/include/wx/generic/grid.h +++ b/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), diff --git a/include/wx/generic/gridsel.h b/include/wx/generic/gridsel.h index 3e078653e5..18377431ef 100644 --- a/include/wx/generic/gridsel.h +++ b/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 - // edit the block vertically. Otherwise return -1. - int GetCurrentBlockCornerRow() const; - // Return the column of the current selection block if it exists and we can - // edit the block horizontally. Otherwise return -1. - int GetCurrentBlockCornerCol() const; + // Return the coordinates of the cell from which the selection should + // continue to be extended. This is normally the opposite corner of the + // last selected block from the current cell coordinates. + // + // If there is no selection, just returns the current cell coordinates. + wxGridCellCoords GetExtensionAnchor() const; wxGridCellCoordsArray GetCellSelection() const; wxGridCellCoordsArray GetBlockSelectionTopLeft() const; diff --git a/include/wx/generic/private/grid.h b/include/wx/generic/private/grid.h index 82703c4b9d..318e88293b 100644 --- a/include/wx/generic/private/grid.h +++ b/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) diff --git a/src/generic/grid.cpp b/src/generic/grid.cpp index 754bca5f16..58e25b56fa 100644 --- a/src/generic/grid.cpp +++ b/src/generic/grid.cpp @@ -5753,16 +5753,12 @@ void wxGrid::OnKeyDown( wxKeyEvent& event ) } else { - int currentBlockRow = -1; - if ( m_selection ) - currentBlockRow = m_selection->GetCurrentBlockCornerRow(); - - // 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 ) + // If we're extending the selection, try to continue in + // the same row, which may well be different from the + // one in which we started selecting. + if ( m_selection && event.ShiftDown() ) { - 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; - if ( !PrepareForSelectionExpansion(coords, diroper) ) + wxGridCellCoords coords(m_selection->GetExtensionAnchor()); + 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, - const wxGridDirectionOperations& diroper) +wxGrid::AdvanceByBlock(wxGridCellCoords& coords, + 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, - coords, - kbdState) ) - { - // 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)); - } + // 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); } diff --git a/src/generic/gridsel.cpp b/src/generic/gridsel.cpp index 2d473c0ede..cf54fbb7c1 100644 --- a/src/generic/gridsel.cpp +++ b/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() ) - return block.GetBottomRow(); - if ( block.GetBottomRow() == m_grid->m_currentCellCoords.GetRow() ) - return block.GetTopRow(); + if ( block.GetTopRow() == coords.GetRow() ) + coords.SetRow(block.GetBottomRow()); + else if ( block.GetBottomRow() == coords.GetRow() ) + 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 -{ - 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; + return coords; } wxGridCellCoordsArray wxGridSelection::GetCellSelection() const diff --git a/tests/controls/gridtest.cpp b/tests/controls/gridtest.cpp index 5aa431a623..d61ba77d21 100644 --- a/tests/controls/gridtest.cpp +++ b/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();