Calling wxGrid::AutoSize() shows previously hidden rows/columns

[DoltAlya]

wxGrid::HideRow/ShowRow?doesn't work. The height of 0 is ignored by SetRowHeight.

Method SetRowHeight() need some corrections:

1) line

    if (height < GetRowMinimalAcceptableHeight())
     return;

replace with

     if ( height>0 && height < GetRowMinimalAcceptableHeight())
       return;

2) at the end of method

    if ( !GetBatchCount() )
        CalcDimensions();

replace with

    if ( !GetBatchCount() )
    {
        CalcDimensions();
        Refresh();
    }

But this corrections not solving the whole problem. After restoring the height of row is setting to default (15px), not to previous state. That problem threaded to SetColSize() too.

[vadz]

We'd need "real" support for hiding rows/columns to solve the second (and the only remaining as I'll check in a fix for the first one soon) problem and not emulate them by setting the row size to 0. This would require more extensive changes, of course...

The simplest would probably be to use negative width for such rows/columns and then interpret anything negative as meaning "hidden". Then showing the row/column again would be as simple as changing the sign of its height/width.

[VZ]

(In [70991]) Allow hiding rows in wxGrid with HideRow().

HideRow() was simply ignored as SetRowSize(row, 0) didn't do anything. Check
for the special value of 0 and allow it, just as it was done for the columns
since r57336.

See #14133.

[hackish]

Upon encountering the same problem I can see this defect was partially fixed in 2.9.4.

If you create a grid of size 5,5, call HideRow(0) then ShowRow(0) the grid will cease to be mount selectable.

[hackish]

typo in previous... it will cease to be mouse selectable...

[hackish]

I'm looking at fixing this defect for all.

I like the solution proposed by vadz as it incurs no space penalty but I can see a small problem. Although not documented, calling SetRowSize or SetColSize with any negative size will automatically determine the minimum possible size as by the contents. There are already facilities in place to do this so anyone misusing these functions will see their implementation fail.

If there are no serious reservations I can develop and test the necessary patch and submit it.

[hackish]

See proof of concept below. Now only does this show this bug but it also exposes another bug. At least on MSW with the row 0 hidden certain cells become unclickable. I'm also working to correct this problem at the same time.

#include <wx/wx.h>
#include <wx/grid.h>
#include <wx/sizer.h>

class MyFrame : public wxFrame
{
public:

MyFrame(const wxString& title, const wxPoint& pos, const wxSize& size);

void OnHideCol(wxCommandEvent& event);
void OnShowCol(wxCommandEvent& event);
void OnHideRow(wxCommandEvent& event);
void OnShowRow(wxCommandEvent& event);

protected:

wxGrid *grid;

};

enum
{

ID_Quit=1,
ID_Go

};

class MyApp : public wxApp
{

virtual bool OnInit();

};

IMPLEMENT_APP(MyApp)

bool MyApp::OnInit()
{

MyFrame *frame = new MyFrame( _("Grid Test"), wxPoint(50, 50),

wxSize(450, 350));

frame->Show(true);
SetTopWindow(frame);
return true;

}

MyFrame::MyFrame(const wxString& title, const wxPoint& pos, const wxSize& size)

: wxFrame( NULL, -1, title, pos, size ) {

wxBoxSizer *sizer=new wxBoxSizer(wxVERTICAL);
wxButton *button=new wxButton(this,wxID_ANY,wxT("Hide Row 0"));
button->Connect(wxEVT_COMMAND_BUTTON_CLICKED,(wxObjectEventFunction)&MyFrame::OnHideRow,NULL,this);
sizer->Add(button,0);
button=new wxButton(this,wxID_ANY,wxT("Show Row 0"));
sizer->Add(button,0);
button->Connect(wxEVT_COMMAND_BUTTON_CLICKED,(wxObjectEventFunction)&MyFrame::OnShowRow,NULL,this);
button=new wxButton(this,wxID_ANY,wxT("Hide Col 0"));
sizer->Add(button,0);
button->Connect(wxEVT_COMMAND_BUTTON_CLICKED,(wxObjectEventFunction)&MyFrame::OnHideCol,NULL,this);
button=new wxButton(this,wxID_ANY,wxT("Show Col 0"));
sizer->Add(button,0);
button->Connect(wxEVT_COMMAND_BUTTON_CLICKED,(wxObjectEventFunction)&MyFrame::OnShowCol,NULL,this);
grid=new wxGrid(this,wxID_ANY);
grid->CreateGrid(6,6);

grid->AutoSizeRowLabelSize(3);
sizer->Add(grid,1,wxEXPAND);
SetSizer(sizer);
Fit();

}

void MyFrame::OnHideCol(wxCommandEvent& event) {

grid->HideCol(0);

}
void MyFrame::OnShowCol(wxCommandEvent& event) {

grid->ShowCol(0);

}

void MyFrame::OnHideRow(wxCommandEvent& event) {

grid->HideRow(0);

}
void MyFrame::OnShowRow(wxCommandEvent& event) {

grid->ShowRow(0);

}

[hackish]

Index: src/generic/grid.cpp
===================================================================
--- src/generic/grid.cpp (revision 72422)
+++ src/generic/grid.cpp (working copy)
@@ -2,7 +2,7 @@

// Name: src/generic/grid.cpp
// Purpose: wxGrid and related classes
// Author: Michael Bedward (based on code by Julian Smart, Robin Dunn)

-// Modified by: Robin Dunn, Vadim Zeitlin, Santiago Palacios
+// Modified by: Robin Dunn, Vadim Zeitlin, Santiago Palacios, Michael Richards

// Created: 1/08/1999
// RCS-ID: $Id$
// Copyright: (c) Michael Bedward (mbedward@…)

@@ -2564,13 +2564,19 @@

int wxGrid::GetColWidth(int col) const
{

- return m_colWidths.IsEmpty() ? m_defaultColWidth : m_colWidths[col];
+ if ( m_colWidths.IsEmpty() )
+ return m_defaultColWidth;
+ // a negative width indicates a hidden column
+ return ( m_colWidths[col] > 0) ? m_colWidths[col] : 0;

}

int wxGrid::GetColLeft(int col) const
{

- return m_colRights.IsEmpty() ? GetColPos( col ) * m_defaultColWidth
- : m_colRights[col] - m_colWidths[col];
+ if ( m_colRights.IsEmpty() )
+ return GetColPos( col ) * m_defaultColWidth;
+
+ // a negative width indicates a hidden column
+ return ( m_colWidths[col] > 0 ) ? m_colRights[col] - m_colWidths[col] : 0;

}

int wxGrid::GetColRight(int col) const

@@ -2581,13 +2587,26 @@

int wxGrid::GetRowHeight(int row) const
{

- return m_rowHeights.IsEmpty() ? m_defaultRowHeight : m_rowHeights[row];
+ // no custom heights / hidden rows
+ if ( m_rowHeights.IsEmpty() ) {
+ return m_defaultRowHeight;
+ }
+ int height=m_rowHeights[row];
+
+ // a height less than 1 really means the row is hidden
+ return ( height < 1 ) ? 0 : height;

}

int wxGrid::GetRowTop(int row) const
{

- return m_rowBottoms.IsEmpty() ? row * m_defaultRowHeight
- : m_rowBottoms[row] - m_rowHeights[row];
+ if ( m_rowBottoms.IsEmpty() )
+ {
+ return row * m_defaultRowHeight;
+ }
+ int height = m_rowHeights[row];
+
+ // a height less than 1 really means the row is hidden
+ return ( height < 1 ) ? m_rowBottoms[row] : m_rowBottoms[row] - height;

}

int wxGrid::GetRowBottom(int row) const

@@ -6502,8 +6521,8 @@

// compute row or column from some (unscrolled) coordinate value, using either
// m_defaultRowHeight/m_defaultColWidth or binary search on array of

-// m_rowBottoms/m_colRights to do it quickly (linear search shouldn't be used
-// for large grids)
+// m_rowBottoms/m_colRights to do it quickly in O(log n) time.
+// NOTE: This may not work correctly for reordered columns.

int wxGrid::PosToLinePos(int coord,

bool clipToMinMax,
const wxGridOperations& oper) const

@@ -6531,27 +6550,12 @@

}

- // adjust maxPos before starting the binary search
- if ( maxPos >= numLines )
- {
- maxPos = numLines - 1;
- }
- else
- {
- if ( coord >= lineEnds[oper.GetLineAt(this, maxPos)])
- {
- minPos = maxPos;
- const int minDist = oper.GetMinimalAcceptableLineSize(this);
- if ( minDist )
- maxPos = coord / minDist;
- else
- maxPos = numLines - 1;
- }
-
- if ( maxPos >= numLines )
- maxPos = numLines - 1;
- }
-
+ // binary search is quite efficient and we can't really make any assumptions
+ // on where to start here since row and columns could be of size 0 if they are
+ // hidden. While this could be made more efficient, some profiling will be
+ // necessary to determine if it really is a performance bottleneck
+ maxPos = numLines - 1;
+

// check if the position is beyond the last column
const int lineAtMaxPos = oper.GetLineAt(this, maxPos);
if ( coord >= lineEnds[lineAtMaxPos] )

@@ -6562,7 +6566,6 @@

if ( coord < lineEnds[lineAt0] )

return 0;

-

// finally do perform the binary search
while ( minPos < maxPos )
{

@@ -8036,19 +8039,6 @@

{

wxCHECK_RET( row >= 0 && row < m_numRows, wxT("invalid row index") );

- // if < 0 then calculate new height from label
- if ( height < 0 )
- {
- long w, h;
- wxArrayString lines;
- wxClientDC dc(m_rowLabelWin);
- dc.SetFont(GetLabelFont());
- StringToLines(GetRowLabelValue( row ), lines);
- GetTextBoxSize( dc, lines, &w, &h );
- //check that it is not less than the minimal height
- height = wxMax(h, GetRowMinimalAcceptableHeight());
- }
-

// See comment in SetColSize
if ( height > 0 && height < GetRowMinimalAcceptableHeight())

return;

@@ -8059,10 +8049,50 @@

InitRowHeights();

}

- int h = wxMax( 0, height );
- int diff = h - m_rowHeights[row];
+ // get the current height. If hidden it will be lastheight*-1
+ int curHeight=m_rowHeights[row];

- m_rowHeights[row] = h;
+ // we shouldn't restore a row's height if it's not hidden
+ // we also shouldn't do anything if it's already hidden and they're asking us to hide it

+ if ( ( height < 0 && curHeight > 0 ) ( height == 0 && curHeight < 1 ) )
+ return;
+
+ // the height we're proposing for this row
+ int newHeight=height;
+
+ // a special case, if height is -1 it means restore a hidden row to the last visible height
+ if ( height < 0 )
+ {
+ // restore the height from the saved one
+ newHeight=curHeight*-1;
+ curHeight=0;
+ }
+
+ // if the height is 0 then they must be hiding this row, save the last height as -1*current height
+ if (height==0)
+ {
+ m_rowHeights[row]*=-1;
+ newHeight=0;
+ }
+ else
+ {
+ // not hiding it we can update the height
+ m_rowHeights[row] = newHeight;
+ }
+
+ // if the row is already that size we don't need to do anything
+ if ( curHeight == newHeight )
+ return;
+
+
+ // a hidden row really has a current height of 0
+ if (curHeight<0)
+ {
+ curHeight=0;
+ }
+
+ int diff = newHeight - curHeight;
+

for ( int i = row; i < m_numRows; i++ )
{

m_rowBottoms[i] += diff;

@@ -8097,23 +8127,6 @@

{

wxCHECK_RET( col >= 0 && col < m_numCols, wxT("invalid column index") );

- // if < 0 then calculate new width from label
- if ( width < 0 )
- {
- long w, h;
- wxArrayString lines;
- wxClientDC dc(m_colWindow);
- dc.SetFont(GetLabelFont());
- StringToLines(GetColLabelValue(col), lines);
- if ( GetColLabelTextOrientation() == wxHORIZONTAL )
- GetTextBoxSize( dc, lines, &w, &h );
- else
- GetTextBoxSize( dc, lines, &h, &w );
- width = w + 6;
- //check that it is not less than the minimal width
- width = wxMax(width, GetColMinimalAcceptableWidth());
- }
-

// we intentionally don't test whether the width is less than
// GetColMinimalWidth() here but we do compare it with
// GetColMinimalAcceptableWidth() as otherwise things currently break (see

@@ -8128,8 +8141,50 @@

InitColWidths();

}

- const int diff = width - m_colWidths[col];
- m_colWidths[col] = width;
+ // get the current width. If hidden it will be lastwidth*-1
+ int curWidth=m_colWidths[col];
+
+ // we shouldn't restore a column's width if it's not hidden
+ // we also shouldn't do anything if it's already hidden and they're asking us to hide it

+ if ( ( width < 0 && curWidth > 0 ) ( width == 0 && curWidth < 1 ) )
+ return;
+
+ // the width we're proposing for this column
+ int newWidth=width;
+
+ // a special case, if width is -1 it means restore a hidden column to the last visible width
+ if ( width < 0 )
+ {
+ // restore the height from the saved one
+ newWidth=curWidth*-1;
+ curWidth=0;
+ }
+
+ // if the width is 0 then they must be hiding this column, save the last width as -1*current width
+ if (width==0)
+ {
+ m_colWidths[col]*=-1;
+ newWidth=0;
+ }
+ else
+ {
+ // not hiding it we can update the height
+ m_colWidths[col] = newWidth;
+ }
+
+ // if the row is already that size we don't need to do anything
+ if ( curWidth == newWidth )
+ return;
+
+
+ // a hidden row really has a current height of 0
+ if (curWidth<0)
+ {
+ curWidth=0;
+ }
+
+ int diff = newWidth - curWidth;
+

if ( m_useNativeHeader )

GetGridColHeader()->UpdateColumn(col);

//else: will be refreshed when the header is redrawn

@@ -8498,7 +8553,17 @@

}

// autosize row height depending on label text

- SetRowSize(row, -1);
+ long w, h;
+ wxArrayString lines;
+ wxClientDC dc(m_rowLabelWin);
+ dc.SetFont(GetLabelFont());
+ StringToLines(GetRowLabelValue( row ), lines);
+ GetTextBoxSize( dc, lines, &w, &h );
+ //check that it is not less than the minimal height
+ int height = wxMax(h, GetRowMinimalAcceptableHeight());
+ // set the row height
+ SetRowSize(row, height);
+

ForceRefresh();

}

@@ -8513,7 +8578,21 @@

}

// autosize column width depending on label text

- SetColSize(col, -1);
+ long w, h;
+ wxArrayString lines;
+ wxClientDC dc(m_colWindow);
+ dc.SetFont(GetLabelFont());
+ StringToLines(GetColLabelValue(col), lines);
+ if ( GetColLabelTextOrientation() == wxHORIZONTAL )
+ GetTextBoxSize( dc, lines, &w, &h );
+ else
+ GetTextBoxSize( dc, lines, &h, &w );
+ int width = w + 6;
+ //check that it is not less than the minimal width
+ width = wxMax(width, GetColMinimalAcceptableWidth());
+ // set the column width
+ SetColSize(col, width);
+

ForceRefresh();

}

@@ -8928,8 +9007,15 @@

int wxGridSizesInfo::GetSize(unsigned pos) const
{

wxUnsignedToIntHashMap::const_iterator it = m_customSizes.find(pos);

+ // if it's not found return the default
+ if ( it == m_customSizes.end() )
+ return m_sizeDefault;
+ // otherwise return 0 if it's hidden. NOTE: We should add an additional
+ // method to return the hidden size.
+ if ( it->second < 0 )
+ return 0;

- return it == m_customSizes.end() ? m_sizeDefault : it->second;
+ return it->second;

}

// ----------------------------------------------------------------------------

[hackish]

Description of the above patch...

As per vadz's suggestion this deals with hidden rows by flipping the sign and then re-interprets the size as 0 if it is stored as negative. It also attempts to be smart by catching resizes of equal height/width and ignoring hiding an already hidden row/column (so as to preserve the saved size).

The patch also fixes a bug in wxGrid::PosToLinePos where it was unable to consider find a cell when hidden rows/columns were present. This was due to an optimization of the binary search. While my implementation can possibly be improved I do not believe it to be necessary given an already O(log n) complexity.

The patch changes an undocumented feature where calling SetRowSize or SetColSize to -1 would have resized the row or column to it's header's size. That functionality should instead be found using AutoSizeRowLabelSize or AutoSizeColLabelSize and those functions have also been updated and tested to work correctly. SetRowSize and SetColSize now interpret a -1 in size to mean restore a hidden row/column to its original size.

[vadz]

This probably was closed accidentally.

Also, could you please attach patches as files to this ticket (I'm not sure if they're the same ones as those that you sent me, I'll look at all this in details once O get back from my trip). TIA!

[hackish]

Patch to correct the problem

[hackish]

test case

[vadz]

Unfortunately there is a problem with this patch: using SetColSize(-1) is documented to auto-size the column (and the same for rows, of course), so we can't just change it like this. I'll modify the patch to preserve this behaviour and also, importantly, factor out the row/column size update logic in a separate function because it's really not a good idea to have 2 copies of this not completely trivial code.

I'll also add the asserts to detect attempts to do invalid things and will update the documentation for the new behaviour -- please review all these changes when they're committed.

[VZ]

(In [72490]) Fix wxGrid::PosToLinePos() in presence of hidden rows or columns.

The optimization of the binary search inside this function failed if any
rows/columns were hidden and so were of zero size.

See #14133.

[VZ]

(In [72491]) Preserve size of wxGrid rows/columns when hiding and showing them back.

Previously, if a wxGrid row or column was hidden by setting its size to 0, its
original size was lost. Change this so that it's preserved now and can be
restored by setting the size to the special -1 value later.

Closes #14133.

[hackish]

Replying to vadz:

See comments posted earlier:
"I like the solution proposed by vadz as it incurs no space penalty but I can see a small problem. Although not documented, calling SetRowSize or SetColSize with any negative size will automatically determine the minimum possible size as by the contents. There are already facilities in place to do this so anyone misusing these functions will see their implementation fail."

I am unable to find any documentation that describes SetColSize(-1) and there are already functions in place to provide this functionality. I'm not sure if it's a great idea to document and allow or promote use of SetColSize(-1) when the AutoSizeColumn function would be the correct one to use.

[hackish]

I'll also add the asserts to detect attempts to do invalid things and will update the documentation for the new behaviour -- please review all these changes when they're committed.

Upon review of the asserts I'm not convinced this is a good idea. When a programmer wishes to hide a row they must now check to see if it is hidden, then make the function call to hide it which checks to see if it's hidden anyway. When processing a large grid this imposes a significant performance penalty.

Instead I believe it would be better to simply do nothing if asked to hide an already hidden or show an already visible row/column.

[hackish]

Calling AutoSize() on a table with hidden rows/cols causes them not to be hidden. The issue is AutoSize calling SetColSize with a Given that this relates to the SetColSize(-1) above I'll re-open this rather than create another ticket. I can make and submit a patch for this but it would probably be best to address the -1 issue first.

[vadz]

Replying to hackish:

I am unable to find any documentation that describes SetColSize(-1)

Err, have you looked at  the documentation of `wxGrid::SetColSize()` itself? It quite clearly says "or -1 to fit the column width to its label width.".

I'm not sure if it's a great idea to document and allow or promote

I'm not sure neither but the very fact that we're not absolutely sure that it's a very harmful thing means that we shouldn't break compatibility (especially, and this is the worst case of breakage, silently).

Replying to hackish:

Instead I believe it would be better to simply do nothing if asked to hide an already hidden or show an already visible row/column.

That's a possibility too but I'd prefer to them later if anybody complains rather than foregoing the possibility of helping people find bugs in their code due to unexpected calls to these functions with 0 or negative argument. FWIW I think it's pretty unlikely that you'd want to hide a column without knowing if it had already been hidden or not.

Replying to hackish:

Calling AutoSize() on a table with hidden rows/cols causes them not to be hidden. The issue is AutoSize calling SetColSize with a

Thanks for noticing this! I agree that there is a problem here but there seems to be something missing in your explanation. Anyhow, if you do have a patch fixing this already, it would be very welcome, thanks!

[hackish]

Replying to vadz:

Replying to hackish:

I am unable to find any documentation that describes SetColSize(-1)

Err, have you looked at  the documentation of `wxGrid::SetColSize()` itself? It quite clearly says "or -1 to fit the column width to its label width.".

Sorry you're right I was looking at older docs.

Replying to hackish:

Instead I believe it would be better to simply do nothing if asked to hide an already hidden or show an already visible row/column.

That's a possibility too but I'd prefer to them later if anybody complains rather than foregoing the possibility of helping people find bugs in their code due to unexpected calls to these functions with 0 or negative argument. FWIW I think it's pretty unlikely that you'd want to hide a column without knowing if it had already been hidden or not.

The trouble is that adding the checks here and the requirement that you can no longer hide a hidden row or show a displayed row changes the API - exactly what we're trying to avoid above. I know it breaks all of my code.

I think in the spirit of catching bugs it is better to simply not do something again if it's already been done. By complaining and forcing everything strictly we waste time and CPU cycles and make it more difficult to use. In this example the most common use would be a filter on rows or cols. The easiest way to then un-filter consists of showing all the columns. If you must first check to see if it's displayed (something the function call has to do anyway) you're doing double the work. It is also not very consistent with how the rest of the library behaves. I can enable an already enabled control, or hide an already hidden window without my program failing because of an assert failure.

Replying to hackish:

Calling AutoSize() on a table with hidden rows/cols causes them not to be hidden. The issue is AutoSize calling SetColSize with a

Thanks for noticing this! I agree that there is a problem here but there seems to be something missing in your explanation. Anyhow, if you do have a patch fixing this already, it would be very welcome, thanks!

My patch had originally handled it because the original code was calling SetColSize, it wasn't until I fixed the problem that I realized that VZ's patch is doing it differently. Being consistent with my argument above the patch I've attached (grid1.patch) is silent if you try to auto-size a hidden column.

[vadz]

I think wasting time and CPU cycles is completely negligible compared to wasting programmers time -- and not detecting wrong calls to SetColSize() might lead to the latter. So I'd like to keep it like this -- and change it later if enough people complain about this.

As for auto-sizing, I'm actually not sure why shouldn't we allow fitting even hidden rows/columns. But neither am I sure that we should allow it so it's probably indeed better to keep things simple and not do anything. And here I guess we could do it silently as I don't see any possible danger of calling AutoSize() accidentally.

Thanks for the patch!

[VZ]

(In [72501]) Don't show hidden wxGrid rows/columns when they're auto-sized.

Fix the problem introduced in r72491 which resulted in wxGrid rows/columns
being shown after auto-sizing even if they had been hidden.

Closes #14133.