Add RefreshArea() member function to wxGrid #22826

AliKet · 2022-09-27T14:19:11Z

The idea here is to issue a refresh request on the wxGrid object itself and rely on the underlying toolkit to dispatch the request to the child windows intersecting the refreshed rectangle. Notice that there is no performance penalty here because the paint handler of the grid itself is trivial (does nothing in fact) and the number of its children is nine (09) at most, and this is not a problem at all (in practice) to know which part needs to be refreshed from the request.

Tested and works correctly under Windows 10 and Linux. but i have no Mac machine to test this on, sorry

This will make refreshing frozen grid windows much easier and a lot of functions much more clear an concise

This is more neat and less error prone

The affected functions already accept NULL as a valid argument

vadz

Sorry, I find the new function very confusing. Maybe it's just me, but it also looks like it's only used in 2 cases:

Refresh several areas completely.
Refresh part of a single area.

And this seems to indicate to me that we need 2 different functions here and not one doing everything. Am I missing some reason we need to do all in a single function?

include/wx/generic/grid.h

src/generic/grid.cpp

include/wx/generic/grid.h

AliKet · 2022-09-27T17:37:44Z

Sorry, I find the new function very confusing. Maybe it's just me, but it also looks like it's only used in 2 cases:
1. Refresh several areas completely.

2. Refresh part of a single area.

First, thanks for the review.
Second, here is how RefreshArea() works illustrated below:

In the above image, the nine (09) child windows are all there:
m_cornerLabelWin, m_{row,col}LabelWin, m_{row,col}FrozenLabelWin, m_gridwin, m_frozenCornerGridWin, m_frozen{Row,Col}GridWin

Suppose now that the selection rectangle rect surrounded by the green box is already calculated, then calling RefreshArea(wxGridArea::Cells, &rect) will correctly refreshes m_gridwin, m_frozenCornerGridWin, m_frozen{Row,Col}GridWin windows, but only the intersection areas between these windows and rect.
Notice that the rect as passed to RefreshArea has the big black dot as origin and what we should refresh is the offset of this rectangle, i.e. rect.Offset(GetRowLabelSize(), GetColLabelSize())

In addition, if the labels need to be refreshed to reflect the selected state, the following RefreshArea(wxGridArea::Cells | wxGridArea::Labels, &rect) will also refreshes the red box (which means two windows will be refreshed m_colLabelWin and m_colFrozenLabelWin). The same for the blue box (m_rowLabelWin, m_rowFrozenLabelWin will be refreshed).

For me i find this feature extremely useful, i.e. no need to test for any frozen part at all.

One that refreshes part of one area of the grid and takes a wxRect as parameter. the other one can refresh one or more areas entirely. This makes sense and less confusing because a rectangle cannot be shared with two separate/unrelated areas. Notice that this commit undoes this 3bb8613 (Move wxGridArea enum to private header)

… windows Also, don't do anything if the corresponding label window is hidden

vadz

Thanks for the changes, they make things clearer, but now it seems even more obvious that we don't actually need a special function for a single area refresh: it's basically only used once and it's more clear to just do it there.

The other overload is, of course, nice and useful to have.

src/generic/grid.cpp

include/wx/generic/grid.h

Also make wxGA_Corner part of wxGA_Labels so it will be refreshed when the row and col labels are refreshed

This function expects the rectangle to be calculated relative to the wxGrid window itself (with the origin (0, 0) in the top left corner of the window) which is the caller's responsibility to make sure. Additionally, wxScrolledCanvas::Refresh() will properly refresh the intersection of subwindows with the rectangle rect. so no need to do anything special inside the function which is at best an unnecessary complication.

AliKet · 2022-10-01T14:23:40Z

src/generic/grid.cpp

+        return true;
+    }
+
+    return false;
 }


Oops! we can just return labelArea != -1;

No need to worry about frozen windows anymore as wxGrid::Refresh() will do the right job for them anyhow.

Do not refresh the unaffected label window unnecessarily due to the last call to Refresh(). In fact, we can do even better by refreshing the grid window the same way wxGrid::DoSet{Row,Col}Size() do that. but for now let's continue with the simple and straightforward way to refresh the grid.

No need to do any refresh inside AutoSizeColOrRow() as it is already done inside DoSet{Row,Col}Size() when Set{Row,Col}Size() is called.

Calling Refresh() with no explicit rectangle will refresh the entire window anyhow.

AliKet · 2022-10-03T19:37:54Z

@vadz @paulcor

FYI in this function wxGrid::DoSetRowSize() (also true for wxGrid::DoSetColSize())
the call to CalcDimensions() has a side effect of generating a refresh request on the entire widget (under GTK 3 only) due to this call (as it calls gtk_widget_queue_resize() internally) which makes this call: if ( ShouldRefresh() ) useless there (under GTK 3)!

IMHO this is not a problem (i.e. to execute the refresh code resulting in another refresh request to be queued) because GTK 3 is optimized in such a way the drawing is done only once. but the problem is in the user not expecting a refresh if ShouldRefresh() returns false !!

Personally, I haven't had any issues with this, and just wanted to let you know so it can be documented.

Thank you.

vadz · 2022-10-03T21:38:13Z

So should we just move CalcDimensions() call inside if ( ShouldRefresh() ) check? I don't think we need to do it for the currently hidden window (but this needs to be double checked...) and CalcDimensions() already does nothing if the updates are disabled anyhow.

AliKet · 2022-10-04T11:40:48Z

So should we just move CalcDimensions() call inside if ( ShouldRefresh() ) check? I don't think we need to do it for the currently hidden window (but this needs to be double checked...) and CalcDimensions() already does nothing if the updates are disabled anyhow.

What I was thinking is to apply something like this:

diff --git a/src/generic/grid.cpp b/src/generic/grid.cpp
index bca2f1899e..ea1c294f50 100644
--- a/src/generic/grid.cpp
+++ b/src/generic/grid.cpp
@@ -9939,6 +9939,11 @@ void wxGrid::DoSetRowSize( int row, int height )
 
     CalcDimensions();
 
+    // Notice that if ShouldRefresh() returns true, a refresh request is already
+    // queued for the entire grid under GTK3 by CalcDimensions() above. So save
+    // some valuable CPU cycles and do nothing there.
+#ifndef __WXGTK3__
+
     if ( ShouldRefresh() )
     {
         // We need to check the size of all the currently visible cells and
@@ -9978,6 +9983,8 @@ void wxGrid::DoSetRowSize( int row, int height )
         const wxRect updateRect(0, y, cw, ch - y);
         Refresh(true, &updateRect);
     }
+
+#endif // __WXGTK3__
 }
 
 void wxGrid::SetDefaultColSize( int width, bool resizeExistingCols )
@@ -10082,6 +10089,10 @@ void wxGrid::DoSetColSize( int col, int width )
 
     CalcDimensions();
 
+    // See the comment in DoSetRowSize() for why the following code
+    // is not needed under GTK 3
+#ifndef __WXGTK3__
+
     if ( ShouldRefresh() )
     {
         // This code is symmetric with DoSetRowSize(), see there for more
@@ -10119,6 +10130,8 @@ void wxGrid::DoSetColSize( int col, int width )
         const wxRect updateRect(x, 0, cw - x, ch);
         Refresh(true, &updateRect);
     }
+
+#endif // __WXGTK3__
 }
 
 void wxGrid::SetColMinimalWidth( int col, int width )

What do you think ?

vadz · 2022-10-04T23:12:33Z

What do you think ?

I'd rather avoid having platform checks in the generic code.

I also have to wonder how important is it to try to refresh just parts of the window here. If we're using double buffering (and we do), is it really any better than just refreshing everything? And if we do this, we could do it unconditionally and it shouldn't do any harm in wxGTK as 2 refreshes should be coalesced anyhow.

AliKet · 2022-10-05T13:07:43Z

What do you think ?

I'd rather avoid having platform checks in the generic code.

I hate platform checks in the generic code too.

I also have to wonder how important is it to try to refresh just parts of the window here. If we're using double buffering (and we do), is it really any better than just refreshing everything? And if we do this, we could do it unconditionally and it shouldn't do any harm in wxGTK as 2 refreshes should be coalesced anyhow.

I did some measurments on wxGrid::DoSetColSize() (and wxGrid::DoSetRowSize()) performence (using a simple glib timer) with and without the following patch:

diff --git a/src/generic/grid.cpp b/src/generic/grid.cpp
index bca2f1899e..eba6c4b0ab 100644
--- a/src/generic/grid.cpp
+++ b/src/generic/grid.cpp
@@ -9941,42 +9941,7 @@ void wxGrid::DoSetRowSize( int row, int height )
 
     if ( ShouldRefresh() )
     {
-        // We need to check the size of all the currently visible cells and
-        // decrease the row to cover the start of the multirow cells, if any,
-        // because we need to refresh such cells entirely when resizing.
-        int topRow = row;
-
-        // Note that we don't care about the cells in frozen windows here as
-        // they can't have multiple rows currently.
-        const wxRect rect = m_gridWin->GetRect();
-        int left, right;
-        CalcUnscrolledPosition(rect.GetLeft(), 0, &left, NULL);
-        CalcUnscrolledPosition(rect.GetRight(), 0, &right, NULL);
-
-        const int posLeft = XToPos(left, m_gridWin);
-        const int posRight = XToPos(right, m_gridWin);
-        for ( int pos = posLeft; pos <= posRight; ++pos )
-        {
-            int col = GetColAt(pos);
-
-            int numRows, numCols;
-            if ( GetCellSize(row, col, &numRows, &numCols) == CellSpan_Inside )
-            {
-                // Notice that numRows here is negative.
-                if ( row + numRows < topRow )
-                    topRow = row + numRows;
-            }
-        }
-
-        int y;
-        CalcScrolledPosition(0, GetRowTop(topRow), NULL, &y);
-
-        // Refresh the lower part of the window below the y position
-        int cw, ch;
-        GetClientSize(&cw, &ch);
-
-        const wxRect updateRect(0, y, cw, ch - y);
-        Refresh(true, &updateRect);
+        RefreshArea(wxGA_RowLabels | wxGA_Cells);
     }
 }
 
@@ -10084,40 +10049,7 @@ void wxGrid::DoSetColSize( int col, int width )
 
     if ( ShouldRefresh() )
     {
-        // This code is symmetric with DoSetRowSize(), see there for more
-        // comments.
-
-        int leftCol = col;
-
-        const wxRect rect = m_gridWin->GetRect();
-        int top, bottom;
-        CalcUnscrolledPosition(0, rect.GetTop(), NULL, &top);
-        CalcUnscrolledPosition(0, rect.GetBottom(), NULL, &bottom);
-
-        const int posTop = YToPos(top, m_gridWin);
-        const int posBottom = YToPos(bottom, m_gridWin);
-        for ( int pos = posTop; pos <= posBottom; ++pos )
-        {
-            int row = GetRowAt(pos);
-
-            int numRows, numCols;
-            if ( GetCellSize(row, col, &numRows, &numCols) == CellSpan_Inside )
-            {
-                if ( col + numCols < leftCol )
-                    leftCol = col + numCols;
-            }
-        }
-
-        int x;
-        CalcScrolledPosition(GetColLeft(leftCol), 0, &x, NULL);
-
-        // Refresh the further part of the window to the right (LTR)
-        // or to the left (RTL) of the x position
-        int cw, ch;
-        GetClientSize(&cw, &ch);
-
-        const wxRect updateRect(x, 0, cw - x, ch);
-        Refresh(true, &updateRect);
+        RefreshArea(wxGA_ColLabels | wxGA_Cells);
     }
 }

and guess what ?
there is no much gain to try to refresh just parts of the grid compared with
just refreshing the entire cells area plus the affected label (row or col) window.
moreover, the gain is really lost (or vanished) if we are resizing the first row/col labels

So if you (or someone else) don't object to apply the patch above I'll do that ?
(Note that we don't have to change these functions later if/when merged cells in frozen windows are supported.)

vadz · 2022-10-06T00:26:22Z

I think partial refreshing is just generally not that useful when using double buffering as the entire window is going to be blitted to the screen anyhow. I guess there could be some exceptions when drawing itself takes a relatively long time and we want to optimize away redrawing of the unchanged parts even off screen, but normally this shouldn't be the case.

So yes, let's simplify the code here by just refreshing everything.

AliKet · 2022-10-18T11:00:50Z

Will be replaced with another one soon!

ali kettab added 4 commits September 27, 2022 14:14

Add RefreshArea() member function to wxGrid

02228dd

This will make refreshing frozen grid windows much easier and a lot of functions much more clear an concise

Let RefreshArea() handle frozen windows for us

dc4300d

This is more neat and less error prone

Remove unnecessary dummy variable from wxGrid impl.

4cef1aa

The affected functions already accept NULL as a valid argument

Make wxGrid::SetCellValue() works correctly with frozen grid windows

4a98ec5

AliKet mentioned this pull request Sep 27, 2022

Fix wxGrid::SetCellvalue() incorrectly refreshes the wrong rectangle #22824

Closed

vadz reviewed Sep 27, 2022

View reviewed changes

ali kettab added 7 commits September 27, 2022 19:14

Move wxGridArea enum to private header

3bb8613

Add wxGA_ prefix to wxGridArea enums

3bb2d71

RefreshArea: erasebg parameter is already defaulted to true

3da84b1

Minor fixup

11b68bb

Always erase the background when calling wxGrid::RefreshArea

3dffe72

Really make wxGrid::Set{Row,Col}LabelValue work correctly with frozen…

c3e97b3

… windows Also, don't do anything if the corresponding label window is hidden

vadz requested changes Sep 28, 2022

View reviewed changes

ali kettab and others added 7 commits September 29, 2022 12:48

Move wxGridArea enum to private header again

d85025d

Also make wxGA_Corner part of wxGA_Labels so it will be refreshed when the row and col labels are refreshed

Change RefreshArea() signature and implementation

041a831

Fixup!

5ad0662

Replace wxScrolledCanvas::Refresh() with wxGrid::Refresh()

d8a2649

Get rid of RefreshArea(area, rect) overload

62aebbe

Fixup! ClearGrid() doesn't clear frozen grid windows

2476719

AliKet commented Oct 1, 2022

View reviewed changes

src/generic/grid.cpp

return true;

}

return false;

}

Copy link

Contributor Author

AliKet Oct 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops! we can just return labelArea != -1;

AliKet force-pushed the grid-refresharea branch from ddd8725 to 2b848f6 Compare October 2, 2022 16:00

ali kettab added 2 commits October 2, 2022 20:56

Simplify wxGrid::DoSet{Row,Col}Size() implementations

44f53d4

No need to worry about frozen windows anymore as wxGrid::Refresh() will do the right job for them anyhow.

AliKet force-pushed the grid-refresharea branch from 2b848f6 to a2f039e Compare October 2, 2022 20:01

ali kettab added 2 commits October 3, 2022 12:13

wxGrid::AutoSizeColOrRow() cleanup

f162f0f

No need to do any refresh inside AutoSizeColOrRow() as it is already done inside DoSet{Row,Col}Size() when Set{Row,Col}Size() is called.

Simplify wxGrid::SetCornerLabelValue() implementation

5e8bac3

Calling Refresh() with no explicit rectangle will refresh the entire window anyhow.

AliKet closed this Oct 18, 2022

This was referenced Oct 18, 2022

wxGrid refresh logic revisited #22890

Closed

wxGrid refresh logic revisited #22893

Merged

AliKet deleted the grid-refresharea branch October 20, 2022 12:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add RefreshArea() member function to wxGrid #22826

Add RefreshArea() member function to wxGrid #22826

AliKet commented Sep 27, 2022 •

edited

vadz left a comment

AliKet commented Sep 27, 2022

vadz left a comment

AliKet Oct 1, 2022

AliKet commented Oct 3, 2022

vadz commented Oct 3, 2022

AliKet commented Oct 4, 2022 •

edited

vadz commented Oct 4, 2022

AliKet commented Oct 5, 2022

vadz commented Oct 6, 2022

AliKet commented Oct 18, 2022

Add RefreshArea() member function to wxGrid #22826

Add RefreshArea() member function to wxGrid #22826

Conversation

AliKet commented Sep 27, 2022 • edited

vadz left a comment

Choose a reason for hiding this comment

AliKet commented Sep 27, 2022

vadz left a comment

Choose a reason for hiding this comment

AliKet Oct 1, 2022

Choose a reason for hiding this comment

AliKet commented Oct 3, 2022

vadz commented Oct 3, 2022

AliKet commented Oct 4, 2022 • edited

vadz commented Oct 4, 2022

AliKet commented Oct 5, 2022

vadz commented Oct 6, 2022

AliKet commented Oct 18, 2022

AliKet commented Sep 27, 2022 •

edited

AliKet commented Oct 4, 2022 •

edited