Implement wxGrid transparent selection (PR #24561)
AliKet
see #22694
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/24561
Commit Summary
- 8221ba5 wxQt: override wxDCImpl::DoDrawPolyPolygon()
- ef9f222 Added convenience functions, RefreshRect() and RefreshBlock(), to wxGrid
- 87d8a03 Added wxGridSelection::MergeAdjacent{Blocks,Rects}() helper functions
- 06e47c6 Added wxGrid::GetSelectedRectangles() member function
- 97fd68c Added transparent selection capability to wxGrid
- 1825354 Refactor wxGrid{Row,Column}HeaderRendererDefault::DrawBorder() common code
- 376c5b3 Added Is{Row,Col}LabelHighlighted() member functions to wxGrid
- 7e111be Added row/col highlight capability to wxGrid
- db98130 Refresh the old highlighted row/col labels for a non-visible selection/current cell
- 64654c5 wxQt: Fix wxGrid::RefreshRect(), which sometimes leave areas unrefreshed
- 76ee9ae Fix rendering transparent selection on external DC (via wxGrid::Render())
- 3334954 Update griddemo sample
File Changes
(9 files)
- M include/wx/generic/grid.h (53)
- M include/wx/generic/gridsel.h (61)
- M include/wx/qt/dc.h (4)
- M samples/grid/griddemo.cpp (24)
- M samples/grid/griddemo.h (2)
- M src/generic/grid.cpp (594)
- M src/generic/gridctrl.cpp (4)
- M src/generic/gridsel.cpp (521)
- M src/qt/dc.cpp (42)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/24561.patch
- https://github.com/wxWidgets/wxWidgets/pull/24561.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
@AliKet pushed 1 commit.
- d8711ae Update Grid::SelectionRange of GridTestCase
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
@AliKet pushed 13 commits.
- 96defec wxQt: override wxDCImpl::DoDrawPolyPolygon()
- 21de6d6 Added convenience functions, RefreshRect() and RefreshBlock(), to wxGrid
- d4a9fe7 Added wxGridSelection::MergeAdjacent{Blocks,Rects}() helper functions
- 4bf13ac Added wxGrid::GetSelectedRectangles() member function
- 9ba4cf4 Added transparent selection capability to wxGrid
- 7439b2e Refactor wxGrid{Row,Column}HeaderRendererDefault::DrawBorder() common code
- be7d0d5 Added Is{Row,Col}LabelHighlighted() member functions to wxGrid
- cb9b551 Added row/col highlight capability to wxGrid
- 7df35d3 Refresh the old highlighted row/col labels for a non-visible selection/current cell
- 3242598 wxQt: Fix wxGrid::RefreshRect(), which sometimes leave areas unrefreshed
- 57686e8 Fix rendering transparent selection on external DC (via wxGrid::Render())
- a4f056e Update griddemo sample
- d8f8e50 Update Grid::SelectionRange of GridTestCase
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
@AliKet pushed 3 commits.
- b77b7e4 Fix rendering transparent selection on external DC (via wxGrid::Render())
- 463e73b Update griddemo sample
- 63bbe4d Update Grid::SelectionRange of GridTestCase
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
@vadz I'm not sure if it's intentional or not, but I noticed that wxGrid::Render() doesn't render frozen borders (if any) at all, and I fixed this in b77b7e4. Just let me know if I should change anything here, TIA
Screenshot.from.2024-05-27.20-31-48.png (view on web)
Screenshot.from.2024-05-27.20-31-04.png (view on web)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
I haven't tried it yet but globally this looks good, thank you for implementing it!
I wonder if there is some better/more common term for this than "transparent selection", would anybody know how it is called in the existing programs? "Overlay selection" maybe?
In any case, whatever its final name, the new wxGrid function for enabling it should be documented.
Finally, ideally, we'd extract (some of?) the new functions in wx/private/grid.h in order to allow testing them as it's slightly worrisome that there is a lot of not really obvious new code here which is not covered by the unit tests...
Thanks again for your work!
In include/wx/generic/gridsel.h:
> @@ -114,6 +114,45 @@ class WXDLLIMPEXP_CORE wxGridSelection
void EndSelecting();
void CancelSelecting();
+ // A simple interface used by wxGrid::DrawSelection() as a helper to draw
+ // the grid selection(s).
+ class PolyPolygon
I wonder if could/should hide this one in a private header to keep the possibility of modifying it later without worrying about ABI. This would require using std::unique_ptr<> rather than the object in the selection, but is this really a big problem?
In fact, it would allow to get rid of IsValid() (it would just be null if invalid), so it could even be an advantage.
In include/wx/generic/gridsel.h:
> + std::vector<int> m_counts; + std::vector<wxPoint> m_points;
I'd appreciate a comment explaining what is stored in these vectors because it's not obvious to me.
> @@ -74,6 +74,16 @@ class CustomColumnHeaderRenderer : public wxGridColumnHeaderRenderer
dc.DrawRectangle(rect);
}
+ virtual void DrawHighlighted(const wxGrid& WXUNUSED(grid),
+ wxDC& dc,
+ wxRect& rect,
+ int WXUNUSED(rowOrCol)) const override
+ {
+ dc.SetPen(*wxTRANSPARENT_PEN);
+ dc.SetBrush(wxBrush(wxColour(240, 200, 15, 180)));
Maybe we should use SelectLightDark() to provide a different colour for the dark theme?
> + std::vector<wxPoint>::const_iterator cItr = points.begin(),
+ cEnd = points.end();
+ while ( cItr != cEnd )
+ {
+ const wxPoint& point = *cItr++;
Why not just
⬇️ Suggested change- std::vector<wxPoint>::const_iterator cItr = points.begin(),
- cEnd = points.end();
- while ( cItr != cEnd )
- {
- const wxPoint& point = *cItr++;
+ for ( const auto& point : points )
+ {
?
> + std::vector<wxPoint>::const_iterator cItr = poly.begin(),
+ cEnd = poly.end();
+ while ( cItr != cEnd )
+ {
+ const wxPoint& pt = *cItr++;
This could be replaced by a range-for too AFAICS.
> +// or a DC that has a native support for drawing with alpha, like standard +// DCs on OSX and GTK3.
Sorry, it's a pity that you've already done it, but I think there might have been a misunderstanding in one of the previous discussions: I never wanted to imply that you should support using wxDC when wxUSE_G_C==0 in these ports. First of all, I don't even this it can be disabled in them and, second, even if it could, it's definitely not worth it because nobdoy would do it in practice.
So I'd just remove the port checks and use only #if wxUSE_G_C (although you might want to #define wxHAS_TRANSPARENT_SELECTION in this case and check for this one below to make these checks more readable).
> + + wxON_BLOCK_EXIT_OBJ2(dc, wxDC::SetLogicalOrigin, oldOrig.x, oldOrig.y); + + clipRect.Offset(offset); + + // Don't draw selection edges on frozen borders. + if ( offset.x > 0 ) + clipRect.x += 1; + if ( offset.y > 0 ) + clipRect.y += 1; + + wxDCClipper clip(dc, clipRect); + + const wxColour colBg = GetSelectionBackground(); + +#ifdef __WXMSW__
I'd appreciate a comment explaining why do we do this only under MSW as it doesn't seem obvious.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
I wonder if there is some better/more common term for this than "transparent selection", would anybody know how it is called in the existing programs? "Overlay selection" maybe?
So according to this page:
- I'll change
IsUsingTransparentSelection()toIsUsingSelectionOverlay()or simplyIsSelectionOverlay()? - Remove
EnableTransparentSelection()because selection overlay will be used by default if supported. But... - If the environment variable
WXGRID_NO_SELECTION_OVERLAYis set, then the old why to draw the selection will be used.
What do you think ?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
@AliKet pushed 14 commits.
- 0f34b2d Added convenience functions, RefreshRect() and RefreshBlock(), to wxGrid
- b527c10 Added wxGridSelection::MergeAdjacent{Blocks,Rects}() helper functions
- 55c6770 Added wxGrid::GetSelectedRectangles() member function
- 6bc6267 Added selection overlay capability to wxGrid
- 0d8ea68 Refactor wxGrid{Row,Column}HeaderRendererDefault::DrawBorder() common code
- 16fb809 Added Is{Row,Col}LabelHighlighted() member functions to wxGrid
- 18975a9 Added row/col highlight capability to wxGrid
- 8c30cfa Refresh the old highlighted row/col labels for a non-visible selection/current cell
- a7f3e76 Fix rendering selection overlay on external DC (via wxGrid::Render())
- fda13d2 Update Grid::SelectionRange of GridTestCase
- 12fa5a0 Rename DrawSelection() to DrawSelectionOverlay()
- caa609d Update griddemo sample
- ace060b Move PolyPolygon & PolyPolygonHelper to private header
- 57e477f Move MergeAdjacent{Blocks,Rects}() to private header
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
@AliKet commented on this pull request.
> @@ -74,6 +74,16 @@ class CustomColumnHeaderRenderer : public wxGridColumnHeaderRenderer
dc.DrawRectangle(rect);
}
+ virtual void DrawHighlighted(const wxGrid& WXUNUSED(grid),
+ wxDC& dc,
+ wxRect& rect,
+ int WXUNUSED(rowOrCol)) const override
+ {
+ dc.SetPen(*wxTRANSPARENT_PEN);
+ dc.SetBrush(wxBrush(wxColour(240, 200, 15, 180)));
The custom header renderer uses hardcoded colours and as such I don't think that's necessary here:
Screenshot.from.2024-05-30.15-08-41.png (view on web)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
@AliKet pushed 2 commits.
- 94568b0 Move PolyPolygon & PolyPolygonHelper to private header
- 0b98239 Move MergeAdjacent{Blocks,Rects}() to private header
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
@AliKet pushed 15 commits.
- 00abf75 wxQt: Added OffsetHelper class to wxGraphicsContext
- 46e043c Added convenience functions, RefreshRect() and RefreshBlock(), to wxGrid
- 494f007 Added wxGridSelection::MergeAdjacent{Blocks,Rects}() helper functions
- 98b5981 Added wxGrid::GetSelectedRectangles() member function
- 95c7c8e Added selection overlay capability to wxGrid
- 136084f Refactor wxGrid{Row,Column}HeaderRendererDefault::DrawBorder() common code
- f3de9ed Added Is{Row,Col}LabelHighlighted() member functions to wxGrid
- b87d6c3 Added row/col highlight capability to wxGrid
- 27e8e1e Refresh the old highlighted row/col labels for a non-visible selection/current cell
- 7000ded Fix rendering selection overlay on external DC (via wxGrid::Render())
- d78a908 Update Grid::SelectionRange of GridTestCase
- d14bdd3 Rename DrawSelection() to DrawSelectionOverlay()
- e15272f Update griddemo sample
- 61c002c Move PolyPolygon & PolyPolygonHelper to private header
- bd1e378 Move MergeAdjacent{Blocks,Rects}() to private header
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
@AliKet commented on this pull request.
> + + wxON_BLOCK_EXIT_OBJ2(dc, wxDC::SetLogicalOrigin, oldOrig.x, oldOrig.y); + + clipRect.Offset(offset); + + // Don't draw selection edges on frozen borders. + if ( offset.x > 0 ) + clipRect.x += 1; + if ( offset.y > 0 ) + clipRect.y += 1; + + wxDCClipper clip(dc, clipRect); + + const wxColour colBg = GetSelectionBackground(); + +#ifdef __WXMSW__
I wanted to make the selection outline more bold but after thinking more about it I think I'll revert this as it doesn't seem necessary.
Here is how it looks without this dirty hack:
In light mode:
grid_light.png (view on web)
In dark mode:
grid_dark.png (view on web)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
I wonder if there is some better/more common term for this than "transparent selection", would anybody know how it is called in the existing programs? "Overlay selection" maybe?
So according to this page:
* I'll change `IsUsingTransparentSelection()` to `IsUsingSelectionOverlay()` or simply `IsSelectionOverlay()` ?
I'd invert the word order as it seems more natural to me (can any native English speakers please comment on whether it's just me?), i.e. would use IsUsingOverlaySelection() or UsesOverlaySelection() or maybe HasOverlaySelection(), but any of these choices are acceptable, so please choose whichever you prefer.
* Remove `EnableTransparentSelection()` because **selection overlay** will be used by default if supported. But... * If the environment variable `WXGRID_NO_SELECTION_OVERLAY` is set, then the old why to draw the selection will be used.
I don't see why should we have an environment variable to control something like this, so I'd strongly prefer to avoid it. But we could (should?) provide DisableOverlaySelection() to revert to the old appearance for people who would want it, for whatever reason.
Thanks again!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
@AliKet pushed 14 commits.
- 8382dd5 Added convenience functions, RefreshRect() and RefreshBlock(), to wxGrid
- 84e93ad Added wxGridSelection::MergeAdjacent{Blocks,Rects}() helper functions
- aa4470c Added wxGrid::GetSelectedRectangles() member function
- 2b0e6d9 Added overlay selection capability to wxGrid
- 482dd56 Refactor wxGrid{Row,Column}HeaderRendererDefault::DrawBorder() common code
- 2f70a24 Added Is{Row,Col}LabelHighlighted() member functions to wxGrid
- 1f10f6c Added row/col highlight capability to wxGrid
- cdcb801 Refresh the old highlighted row/col labels for a non-visible selection/current cell
- 617b12e Fix rendering the overlay selection on external DC (via wxGrid::Render())
- 5eb502e Update Grid::SelectionRange of GridTestCase
- 48e3f08 Rename DrawSelection() to DrawOverlaySelection()
- 94eb9b0 Update griddemo sample
- d67d58d Move PolyPolygon & PolyPolygonHelper to private header
- 3846275 Move MergeAdjacent{Blocks,Rects}() to private header
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
I'd invert the word order as it seems more natural to me (can any native English speakers please comment on whether it's just me?), i.e. would use
IsUsingOverlaySelection()orUsesOverlaySelection()or maybeHasOverlaySelection(), but any of these choices are acceptable, so please choose whichever you prefer.
I chooses UsesOverlaySelection() and added DisableOverlaySelection() as you suggested. also added documentation for them.
I think this is ready now, but as always I didn't tests this under wxOSX. so if you could please do it (or anyone interested) before merging.
TIA!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
Thanks again for all this work, but I still wonder if it can't be simplified a bit more. I'm sorry for all the comments, but wxGrid code is already awfully complicated and so I'd really like to make an effort to reduce additional increase of its complexity as much as possible.
In include/wx/generic/gridsel.h:
> @@ -151,6 +162,16 @@ class WXDLLIMPEXP_CORE wxGridSelection
wxGrid *m_grid;
wxGrid::wxGridSelectionModes m_selectionMode;
+ // Used by wxGrid::DrawSelectionOverlay() to draw a:
+ //
+ // - Simple rectangle (using wxDC::DrawRectangle() if it is empty and the bounding box is valid.
+ // - Simple polygon (using wxDC::DrawPolygon()) if it represents a simple polygon.
+ // - Poly-polygon (using wxDC::DrawPolyPolygon()) if it consists of multi-polygons.
Really minor, but I'd say:
⬇️ Suggested change- // - Poly-polygon (using wxDC::DrawPolyPolygon()) if it consists of multi-polygons. + // - Poly-polygon (using wxDC::DrawPolyPolygon()) if it consists of multiple polygons.
In include/wx/generic/gridsel.h:
> @@ -18,6 +18,9 @@
#include "wx/vector.h"
+// Forward declaration
+namespace wxGridPrivate { class PolyPolygon; }
I wonder if we should call this class SelectionShape instead? This name seems to carry more meaning than the current one (especially because it doesn't even always consists of multiple polygons).
In include/wx/generic/gridsel.h:
> @@ -114,6 +117,11 @@ class WXDLLIMPEXP_CORE wxGridSelection
void EndSelecting();
void CancelSelecting();
+ // Return the PolyPolygon object. Call ComputePolyPolygon() if necessary.
This comment should probably mention that it returns the intersection of the shape of the selection with the given rectangle (if I understand what it does correctly).
In include/wx/generic/gridsel.h:
> @@ -139,6 +147,9 @@ class WXDLLIMPEXP_CORE wxGridSelection
void MergeOrAddBlock(wxGridBlockCoordsVector& blocks,
const wxGridBlockCoords& block);
+ // Called each time the selection changed or scrolled to recompute m_polyPolygon.
+ void ComputePolyPolygon(bool refreshLabelWindows = false, const wxRect& renderExtent = {});
Sorry, I still think that this function shouldn't have refreshLabelWindows argument and that the case of refreshLabelWindows = true should be handled in the single place where it's used. I realize that this means we're going to potentially call Refresh() twice but I don't think this is a problem in practice, the refresh rectangles should be coalesced anyhow.
And it would make this function much easier to understand.
In include/wx/generic/gridsel.h:
> @@ -151,6 +162,16 @@ class WXDLLIMPEXP_CORE wxGridSelection
wxGrid *m_grid;
wxGrid::wxGridSelectionModes m_selectionMode;
+ // Used by wxGrid::DrawSelectionOverlay() to draw a:
+ //
+ // - Simple rectangle (using wxDC::DrawRectangle() if it is empty and the bounding box is valid.
+ // - Simple polygon (using wxDC::DrawPolygon()) if it represents a simple polygon.
+ // - Poly-polygon (using wxDC::DrawPolyPolygon()) if it consists of multi-polygons.
+ //
+ std::unique_ptr<wxGridPrivate::PolyPolygon> m_polyPolygon;
+
+ size_t m_isAnyLabelHighlighted = 0;
I'd also appreciate a comment about the meaning of this one as its name (isXXX) implies bool type for me, but here it is size_t, so clearly there is something else going on here, but I don't really know what.
In include/wx/generic/gridsel.h:
> @@ -151,6 +162,16 @@ class WXDLLIMPEXP_CORE wxGridSelection
wxGrid *m_grid;
wxGrid::wxGridSelectionModes m_selectionMode;
+ // Used by wxGrid::DrawSelectionOverlay() to draw a:
+ //
+ // - Simple rectangle (using wxDC::DrawRectangle() if it is empty and the bounding box is valid.
+ // - Simple polygon (using wxDC::DrawPolygon()) if it represents a simple polygon.
+ // - Poly-polygon (using wxDC::DrawPolyPolygon()) if it consists of multi-polygons.
+ //
+ std::unique_ptr<wxGridPrivate::PolyPolygon> m_polyPolygon;
We should #include <memory> in this file instead of relying on it being done implicitly in some other header.
> + static bool UsesOverlaySelection(); + + static void DisableOverlaySelection();
Sorry, but why do these functions have to be global, i.e. class-static? I thought they would be per-grid, so that you could customize the selection of a particular grid without affecting anything else.
Maybe it would make sense to have some global DisableOverlaySelectionByDefault() that would set gs_useOverlaySelection to false, but I'd still add m_useOverlaySelection, that would be initialized with gs_useOverlaySelection but could be set to false using non-static DisableOverlaySelection() without affecting any other grids.
In include/wx/generic/private/grid.h:
> @@ -1227,6 +1236,11 @@ wxGetContentRect(wxSize contentSize,
int hAlign,
int vAlign);
+inline bool operator<(const wxPoint& pt1, const wxPoint& pt2)
I'd prefer to define a custom comparator and pass it to std::{set,map} explicitly. Comparing points lexicographically doesn't always make sense and while it's unlikely to affect anything right now, when this is declared in a private header, it still might and it could be very annoying to run into problems due to this later.
In include/wx/generic/private/grid.h:
> + const int* GetCounts() const { return m_counts.data(); }
+
+ const wxPoint* GetPoints() const { return m_points.data(); }
+
+ void Append(const std::vector<wxPoint>& points);
+
+ // Return the bounding box of the visible selected blocks. Return empty
+ // rectangle if there is no selection.
+ wxRect GetBoundingBox() const;
+
+ void SetBoundingBox(const wxRect& rect);
+
+private:
+ void CalcBoundingBox(const std::vector<wxPoint>& points);
+
+ // m_counts contains the number of points in each of the polygons in m_points.
So the data structure here is a vector of points in all polygons stored as a single continuous vector? I.e. if we have 2 polygons P1 and P2, m_points contains P1_first, P1_second, ..., P1_last, P2_first, ..., P2_last?
Is this done for efficiency reasons? Because for me a natural approach would be to just have vector<vector<wxPoint>> m_polygons and I wonder why is it not being used here?
In include/wx/generic/private/grid.h:
> + + void SetBoundingBox(const wxRect& rect); + +private: + void CalcBoundingBox(const std::vector<wxPoint>& points); + + // m_counts contains the number of points in each of the polygons in m_points. + + std::vector<int> m_counts; + std::vector<wxPoint> m_points; + + int m_minX = 0, m_maxX = 0, + m_minY = 0, m_maxY = 0; +}; + +// This class is just a helper that simply converts (using the sweep line algorithm)
Sorry but why do we need a class for what looks like just a function? I had a lot of trouble understanding what it does, but now that I (think I) understand it, it looks like it could be just a function of PolyPolygon, why do we have to make a separate class for it?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
I've tested this PR under Mac, seems to work great, thanks!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
@AliKet commented on this pull request.
In include/wx/generic/gridsel.h:
> @@ -18,6 +18,9 @@
#include "wx/vector.h"
+// Forward declaration
+namespace wxGridPrivate { class PolyPolygon; }
Done
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
> @@ -139,6 +147,9 @@ class WXDLLIMPEXP_CORE wxGridSelection
void MergeOrAddBlock(wxGridBlockCoordsVector& blocks,
const wxGridBlockCoords& block);
+ // Called each time the selection changed or scrolled to recompute m_polyPolygon.
+ void ComputePolyPolygon(bool refreshLabelWindows = false, const wxRect& renderExtent = {});
Done
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
> @@ -151,6 +162,16 @@ class WXDLLIMPEXP_CORE wxGridSelection
wxGrid *m_grid;
wxGrid::wxGridSelectionModes m_selectionMode;
+ // Used by wxGrid::DrawSelectionOverlay() to draw a:
+ //
+ // - Simple rectangle (using wxDC::DrawRectangle() if it is empty and the bounding box is valid.
+ // - Simple polygon (using wxDC::DrawPolygon()) if it represents a simple polygon.
+ // - Poly-polygon (using wxDC::DrawPolyPolygon()) if it consists of multi-polygons.
+ //
+ std::unique_ptr<wxGridPrivate::PolyPolygon> m_polyPolygon;
Done
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
@AliKet commented on this pull request.
In include/wx/generic/private/grid.h:
> @@ -1227,6 +1236,11 @@ wxGetContentRect(wxSize contentSize,
int hAlign,
int vAlign);
+inline bool operator<(const wxPoint& pt1, const wxPoint& pt2)
Done
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
> + const int* GetCounts() const { return m_counts.data(); }
+
+ const wxPoint* GetPoints() const { return m_points.data(); }
+
+ void Append(const std::vector<wxPoint>& points);
+
+ // Return the bounding box of the visible selected blocks. Return empty
+ // rectangle if there is no selection.
+ wxRect GetBoundingBox() const;
+
+ void SetBoundingBox(const wxRect& rect);
+
+private:
+ void CalcBoundingBox(const std::vector<wxPoint>& points);
+
+ // m_counts contains the number of points in each of the polygons in m_points.
So the data structure here is a vector of points in all polygons stored as a single continuous vector? I.e. if we have 2 polygons P1 and P2,
m_pointscontains P1_first, P1_second, ..., P1_last, P2_first, ..., P2_last?
Yes.
Is this done for efficiency reasons? Because for me a natural approach would be to just have
vector<vector<wxPoint>> m_polygonsand I wonder why is it not being used here?
Well, the data structure is modeled in such a way that we can use it to draw a poly-polygon using DrawPolyPolygon() in a simple and straightforward way. I don't see how a vector<vector<wxPoint>> would simplify things here to be honest?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
@AliKet commented on this pull request.
> + static bool UsesOverlaySelection(); + + static void DisableOverlaySelection();
Done without adding DisableOverlaySelectionByDefault().
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
@AliKet pushed 15 commits.
- 995ba9d Added overlay selection capability to wxGrid
- 7b54e8d Refactor wxGrid{Row,Column}HeaderRendererDefault::DrawBorder() common code
- f5567ca Added Is{Row,Col}LabelHighlighted() member functions to wxGrid
- 7d658d4 Added row/col highlight capability to wxGrid
- 156b62d Refresh the old highlighted row/col labels for a non-visible selection/current cell
- bc570b7 Fix rendering the overlay selection on external DC (via wxGrid::Render())
- 909f822 Rename DrawSelection() to DrawOverlaySelection()
- 8eab3f6 Move PolyPolygon & PolyPolygonHelper to private header
- 6fac958 Move MergeAdjacent{Blocks,Rects}() to private header
- d1324f6 Remove refreshLabelWindows parameter from wxGridSelection::ComputePolyPolygon()
- 2588ecb Use SelectionShape instead of PolyPolygon to refer to the selection shape
- 8ebe02c Get rid of PolyPolygonHelper class
- a928103 Update griddemo sample
- 2b23d52 Update Grid::SelectionRange of GridTestCase
- 3003d3e Use custom comparator to compare wxPoints in SelectionShape class
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
@AliKet commented on this pull request.
In include/wx/generic/gridsel.h:
> @@ -139,6 +147,9 @@ class WXDLLIMPEXP_CORE wxGridSelection
void MergeOrAddBlock(wxGridBlockCoordsVector& blocks,
const wxGridBlockCoords& block);
+ // Called each time the selection changed or scrolled to recompute m_polyPolygon.
+ void ComputePolyPolygon(bool refreshLabelWindows = false, const wxRect& renderExtent = {});
ComputePolyPolygon() has been renamed to ComputeSelectionShape(). Because the new name seems strange to me now! I'm thinking of renaming it to UpdateSelectionShape() instead, what do you think ?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
In include/wx/generic/private/grid.h:
> + const int* GetCounts() const { return m_counts.data(); }
+
+ const wxPoint* GetPoints() const { return m_points.data(); }
+
+ void Append(const std::vector<wxPoint>& points);
+
+ // Return the bounding box of the visible selected blocks. Return empty
+ // rectangle if there is no selection.
+ wxRect GetBoundingBox() const;
+
+ void SetBoundingBox(const wxRect& rect);
+
+private:
+ void CalcBoundingBox(const std::vector<wxPoint>& points);
+
+ // m_counts contains the number of points in each of the polygons in m_points.
Sorry, I forgot about this DrawPolyPolygon() convention, I thought the polygons would be drawn one by one, but if we use this function then storing it like this makes sense, thanks, even if using a vector of vectors would still definitely be simpler (because a polygon is a vector of points, and so many polygons is a vector of such vectors).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
In include/wx/generic/gridsel.h:
> @@ -139,6 +147,9 @@ class WXDLLIMPEXP_CORE wxGridSelection
void MergeOrAddBlock(wxGridBlockCoordsVector& blocks,
const wxGridBlockCoords& block);
+ // Called each time the selection changed or scrolled to recompute m_polyPolygon.
+ void ComputePolyPolygon(bool refreshLabelWindows = false, const wxRect& renderExtent = {});
Both Compute and Update seem fine to me, with maybe a slight preference for the latter, but mostly I just think that SelectionShape() is much better than PolyPolygon here.
Thanks!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
@vadz Unrelated to this PR but I just noticed this function wxGrid::DrawAllGridLines() which is undocumented and not used or referenced anywhere and I'm wondering if it's safe to get rid of it?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz Unrelated to this PR but I just noticed this function wxGrid::DrawAllGridLines() which is undocumented and not used or referenced anywhere and I'm wondering if it's safe to get rid of it?
Yes, it seems pretty useless to me, so I think we can remove it, thanks.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Just to confirm: this is ok to merge already, right?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
Just to confirm: this is ok to merge already, right?
Yes.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Merged #24561 into master.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Dietmar Schwertberger
DietmarSchwertberger left a comment (wxWidgets/wxWidgets#24561)@AliKet
Hi!
I'm just testing the last wxPython version which now tracks the wxWidgets master branch.
Thanks for the overlay mode.
It looks good, but I would prefer to have a finer grained control on this feature.
E.g. when complete row(s) are selected, I don't want all the column headers to be highlighted.
For some grids I would like to have the overlay mode for the selected cell(s), but I don't want the headers to be highlighted for the current cell or the selection.
For some grids, I would like the current row and col label being highlighted, but only when the grid actually has the focus.
Maybe the active cell should be in blue as well if there is no selection. (I find it a bit inconsistent that the row and col header are blue like for a selection, but the cell is not.)
As of now, I find the implementation being a bit too distracting from other contents and I have to disable it.
Traditionally 'blue' means 'focused' to me (I'm on Windows). Maybe, the overlay colour should switch from blue to grey while not focused?
I'm not sure what would be the best way:
- add arguments to
DisableOverlaySelection - add style flags?
What do you think?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.