Make selection rect rounded as per OS X look (PR #26084)
RobertRoeb
- So far only do this for tree control single line
- Up for discussion, it is a matter of native look vs taste
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/26084
Commit Summary
- 93d0d56 Make selection rect rounded as per OS X look
File Changes
(1 file)
- M src/osx/cocoa/renderer.mm (11)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/26084.patch
- https://github.com/wxWidgets/wxWidgets/pull/26084.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz requested changes on this pull request.
Please change this to drive the behaviour from the caller instead of using RTTI. TIA!
> @@ -603,6 +604,14 @@ wxRect rect( (int) splitterRect.origin.x, (int) splitterRect.origin.y, (int) spl
wxDCPenChanger setPen(dc, *wxTRANSPARENT_PEN);
wxDCBrushChanger setBrush(dc, selBrush);
+ if (win->IsKindOf(wxCLASSINFO(wxTreeCtrl)))
Sorry but let's please not do this. AFAICS this function doesn't use wxCONTROL_SPECIAL so the simplest alternative would be to add it when using multiple selections. Or, if this doesn't work, we could add wxCONTROL_MULTIPLE. But using RTTI for this is just wrong for the reasons which I think are pretty clear.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
RobertRoeb
@RobertRoeb pushed 1 commit.
- 1cac580 Added wxRENDER_SELECTION_SINGLE_ROUNDED for round macOS selection rect
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
RobertRoeb
@RobertRoeb commented on this pull request.
> @@ -603,6 +604,14 @@ wxRect rect( (int) splitterRect.origin.x, (int) splitterRect.origin.y, (int) spl
wxDCPenChanger setPen(dc, *wxTRANSPARENT_PEN);
wxDCBrushChanger setBrush(dc, selBrush);
+ if (win->IsKindOf(wxCLASSINFO(wxTreeCtrl)))
I didn't want to re-use an unspecific flag, so I introduced new ones. Otherwise did as you suggested and moved the test to the tree control.
—
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 for the changes but I still have the natural questions about the new flags. Please let me know if I'm missing something which explains why was this approach taken instead of the obvious one.
> @@ -603,7 +604,14 @@ wxRect rect( (int) splitterRect.origin.x, (int) splitterRect.origin.y, (int) spl
wxDCPenChanger setPen(dc, *wxTRANSPARENT_PEN);
wxDCBrushChanger setBrush(dc, selBrush);
- dc.DrawRectangle( rect );
+ if ((flags & wxRENDER_SELECTION_SINGLE_ROUND) != 0)
+ {
+ dc.DrawRoundedRectangle( rect, 8 );
I wonder if there is some way to get this 8 from the system?
> +// control state flags for different selection rectangle (on e.g. macOS)
+enum
+{
+ wxRENDER_SELECTION_SINGLE_ROUND = 0x00001000, // single round selection
+ wxRENDER_SELECTION_FIRST_ROUND = 0x00002000, // top of multiple round selection
+ wxRENDER_SELECTION_MIDDLE_ROUND = 0x00004000, // mid of multiple round selection
+ wxRENDER_SELECTION_LAST_ROUND = 0x00008000, // bottom of multiple round selection
+ wxRENDER_SELECTION_MASK = 0x0000F000, // bottom of multiple round selection
+};
+
Sorry, I don't understand why:
- Is this separate from the main enum, when it's passed inside the same
int. - Do we need all the unused flags.
I'd like to keep just the first one and move it into the enum above and call it wxCONTROL_MULTIPLE as proposed before because it's not the fact that it's rounded which is important (this could be different in implementations for the other platforms or even in the future versions of macOS itself), but that it's the selection style used for multiple selection.
I can do this myself if you agree. And if you don't, please explain why.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
RobertRoeb
@RobertRoeb commented on this pull request.
> +// control state flags for different selection rectangle (on e.g. macOS)
+enum
+{
+ wxRENDER_SELECTION_SINGLE_ROUND = 0x00001000, // single round selection
+ wxRENDER_SELECTION_FIRST_ROUND = 0x00002000, // top of multiple round selection
+ wxRENDER_SELECTION_MIDDLE_ROUND = 0x00004000, // mid of multiple round selection
+ wxRENDER_SELECTION_LAST_ROUND = 0x00008000, // bottom of multiple round selection
+ wxRENDER_SELECTION_MASK = 0x0000F000, // bottom of multiple round selection
+};
+
I don't care in which enum you move it, but one day, we will need all of these as the first selected item (of a continuous group of selected items) only has rounded corners on top, the last selected item (of the same continuous group of selected items) only has rounded corners at the bottom and all middle ones have no rounded corners, but may look different than classical non-rounded ones (maybe not).
MultipleSelectionsOSX.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.
> +// control state flags for different selection rectangle (on e.g. macOS)
+enum
+{
+ wxRENDER_SELECTION_SINGLE_ROUND = 0x00001000, // single round selection
+ wxRENDER_SELECTION_FIRST_ROUND = 0x00002000, // top of multiple round selection
+ wxRENDER_SELECTION_MIDDLE_ROUND = 0x00004000, // mid of multiple round selection
+ wxRENDER_SELECTION_LAST_ROUND = 0x00008000, // bottom of multiple round selection
+ wxRENDER_SELECTION_MASK = 0x0000F000, // bottom of multiple round selection
+};
+
There is no way we're going to handle this by calling DrawItemSelectionRect() with different flags for the first, middle and last items, it clearly should be called just once for each contiguous selection fragment to draw the entire rectangle at once, so there is definitely not a good reason to have these elements.
Also, while I definitely see it, I can't help wondering why you don't care about which enum is used when there is obviously the natural and, dare I say, only reasonable, choice of using a single enum for all the flags passed as a single parameter and whatever you did. Could you please care a bit more about either choosing the dull and obvious solution or explaining why you don't do it instead of leaving me questioning my sanity?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
RobertRoeb
@RobertRoeb commented on this pull request.
> +// control state flags for different selection rectangle (on e.g. macOS)
+enum
+{
+ wxRENDER_SELECTION_SINGLE_ROUND = 0x00001000, // single round selection
+ wxRENDER_SELECTION_FIRST_ROUND = 0x00002000, // top of multiple round selection
+ wxRENDER_SELECTION_MIDDLE_ROUND = 0x00004000, // mid of multiple round selection
+ wxRENDER_SELECTION_LAST_ROUND = 0x00008000, // bottom of multiple round selection
+ wxRENDER_SELECTION_MASK = 0x0000F000, // bottom of multiple round selection
+};
+
It is because of the name wxCONTROL_SOMETHING. I need different selection drawing based on items position in the same control
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
RobertRoeb
@RobertRoeb commented on this pull request.
> +// control state flags for different selection rectangle (on e.g. macOS)
+enum
+{
+ wxRENDER_SELECTION_SINGLE_ROUND = 0x00001000, // single round selection
+ wxRENDER_SELECTION_FIRST_ROUND = 0x00002000, // top of multiple round selection
+ wxRENDER_SELECTION_MIDDLE_ROUND = 0x00004000, // mid of multiple round selection
+ wxRENDER_SELECTION_LAST_ROUND = 0x00008000, // bottom of multiple round selection
+ wxRENDER_SELECTION_MASK = 0x0000F000, // bottom of multiple round selection
+};
+
There is no way we're going to handle this by calling
DrawItemSelectionRect()with different flags for the first, middle and last items,
Well, that would have been easy to do. Not sure what the problem is
—
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.
> +// control state flags for different selection rectangle (on e.g. macOS)
+enum
+{
+ wxRENDER_SELECTION_SINGLE_ROUND = 0x00001000, // single round selection
+ wxRENDER_SELECTION_FIRST_ROUND = 0x00002000, // top of multiple round selection
+ wxRENDER_SELECTION_MIDDLE_ROUND = 0x00004000, // mid of multiple round selection
+ wxRENDER_SELECTION_LAST_ROUND = 0x00008000, // bottom of multiple round selection
+ wxRENDER_SELECTION_MASK = 0x0000F000, // bottom of multiple round selection
+};
+
It is because of the name wxCONTROL_SOMETHING. I need different selection drawing based on items position in the same control
What's wrong with wxCONTROL_SELECTION_FIRST etc if you still think you need this?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
paulcor
paulcor left a comment (wxWidgets/wxWidgets#26084)DrawHeaderButton() uses wxCONTROL_SPECIAL to indicate the first button, and wxCONTROL_DIRTY to indicate the last.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
RobertRoeb
RobertRoeb left a comment (wxWidgets/wxWidgets#26084)
DrawHeaderButton()useswxCONTROL_SPECIALto indicate
the first button, andwxCONTROL_DIRTYto indicate the last.
That does not seem very intuitive to me, so may I propose this:
// used to indicate a round selection on e.g. macOS
wxCONTROL_SELECTION_ROUND = 0x40000000,
// used to indicate a sub item within e.g. selections or header controls parts
wxCONTROL_ITEM_FIRST = wxCONTROL_SPECIAL,
wxCONTROL_ITEM_MIDDLE = 0x20000000,
wxCONTROL_ITEM_LAST = wxCONTROL_DIRTY,
wxCONTROL_ITEM_MASK = wxCONTROL_ITEM_FIRST | wxCONTROL_ITEM_MIDDLE | wxCONTROL_ITEM_LAST
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
RobertRoeb
@RobertRoeb pushed 2 commits.
- ade66f5 Renamed selection constants and moved into wxCONTROL_ enum
- 3312eb0 OSX: implement multirow rounded selection
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
RobertRoeb
RobertRoeb left a comment (wxWidgets/wxWidgets#26084)This is what how it can look now, side by side with native:
Multi.wx.png (view on web)
Multi.Native.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.
Is there really no native function for drawing the selection rectangle? I'm absolutely sure that this code will result in the selection looking out of place in the next macOS version as this is exactly the kind of things they like tweaking.
> @@ -690,6 +690,9 @@ void wxListLineData::ApplyAttributes(wxDC *dc,
flags |= wxCONTROL_FOCUSED;
if (current)
flags |= wxCONTROL_CURRENT;
+ if (listctrl->HasFlag(wxLC_SINGLE_SEL)) {
Isn't this backwards?
> @@ -74,7 +74,16 @@ enum
// this is a pseudo flag not used directly by wxRenderer but rather by some
// controls internally
- wxCONTROL_DIRTY = 0x80000000
+ wxCONTROL_DIRTY = 0x80000000,
+
+ // used to indicate a round selection on e.g. macOS
+ wxCONTROL_SELECTION_ROUND = 0x40000000,
Sorry but I'd still really, really like to call it wxCONTROL_MULTIPLE (or _SINGLE if you prefer) because the flags should indicate the semantics, not the appearance.
> + // Allow macOS renderer to draw a rounded rect, but only + // when no horizontal scrolling is needed
Can we check this in the renderer instead? Again, this is clearly just not the right place/layer to do it.
> @@ -601,9 +602,56 @@ wxRect rect( (int) splitterRect.origin.x, (int) splitterRect.origin.y, (int) spl
}
wxBrush selBrush( col );
+ // macOS NSTableView has whitespace left and right of the first and last
+ // data columns and only draws the border where the actual columns begins.
+ // wxWidgets generic controls don't do this, so add a bit of distance to
+ // the border
+ int distanceFromBorder = 8;
⬇️ Suggested change
- int distanceFromBorder = 8; + constexpr int distanceFromBorder = 8;
> wxDCPenChanger setPen(dc, *wxTRANSPARENT_PEN);
wxDCBrushChanger setBrush(dc, selBrush);
- dc.DrawRectangle( rect );
+ if ((flags & wxCONTROL_SELECTION_ROUND) != 0)
+ {
+ dc.DrawRoundedRectangle( rect, 8 );
Is this a different 8? Or should it be distanceFromBorder?
Same question for all the other 8s below.
> + else + if ((flags & wxCONTROL_ITEM_FIRST) != 0)⬇️ Suggested change
- else - if ((flags & wxCONTROL_ITEM_FIRST) != 0) + else if ((flags & wxCONTROL_ITEM_FIRST) != 0)
> + else + if ((flags & wxCONTROL_ITEM_LAST) != 0)⬇️ Suggested change
- else - if ((flags & wxCONTROL_ITEM_LAST) != 0) + else if ((flags & wxCONTROL_ITEM_LAST) != 0)
> + else + if ((flags & wxCONTROL_ITEM_MIDDLE) != 0)⬇️ Suggested change
- else - if ((flags & wxCONTROL_ITEM_MIDDLE) != 0) + else if ((flags & wxCONTROL_ITEM_MIDDLE) != 0)
> @@ -27,6 +27,7 @@ #include "wx/graphics.h" #include "wx/dcgraph.h" #include "wx/splitter.h" +#include "wx/treectrl.h"⬇️ Suggested change
-#include "wx/treectrl.h"
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
paulcor
@paulcor commented on this pull request.
> @@ -74,7 +74,16 @@ enum
// this is a pseudo flag not used directly by wxRenderer but rather by some
// controls internally
- wxCONTROL_DIRTY = 0x80000000
+ wxCONTROL_DIRTY = 0x80000000,
+
+ // used to indicate a round selection on e.g. macOS
+ wxCONTROL_SELECTION_ROUND = 0x40000000,
+
+ // used to indicate a sub item within e.g. selections or header controls parts
+ wxCONTROL_ITEM_FIRST = wxCONTROL_SPECIAL,
+ wxCONTROL_ITEM_MIDDLE = 0x20000000,
It seems like "middle" can be inferred as the lack of "first" or "last"?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
RobertRoeb
@RobertRoeb commented on this pull request.
> @@ -690,6 +690,9 @@ void wxListLineData::ApplyAttributes(wxDC *dc,
flags |= wxCONTROL_FOCUSED;
if (current)
flags |= wxCONTROL_CURRENT;
+ if (listctrl->HasFlag(wxLC_SINGLE_SEL)) {
No, this tests for the easy case, single selection (and possibly no horizontal scrolling) in which case it is round in macOS. I excluded multiple selection mode in a previous variant of the PR because it patch could not handle multiple row yet.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
RobertRoeb
@RobertRoeb commented on this pull request.
> + // Allow macOS renderer to draw a rounded rect, but only + // when no horizontal scrolling is needed
The renderer does not know anything about the scrolling state of the parent window. As you wrote further up, this should be done in the caller.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
RobertRoeb
@RobertRoeb commented on this pull request.
> @@ -74,7 +74,16 @@ enum
// this is a pseudo flag not used directly by wxRenderer but rather by some
// controls internally
- wxCONTROL_DIRTY = 0x80000000
+ wxCONTROL_DIRTY = 0x80000000,
+
+ // used to indicate a round selection on e.g. macOS
+ wxCONTROL_SELECTION_ROUND = 0x40000000,
I cannot promise I have found all scenarios. There is also full row highlight vs. partial, scrolled vs. fits on client area. Tree vs list.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
RobertRoeb
@RobertRoeb commented on this pull request.
> wxDCPenChanger setPen(dc, *wxTRANSPARENT_PEN);
wxDCBrushChanger setBrush(dc, selBrush);
- dc.DrawRectangle( rect );
+ if ((flags & wxCONTROL_SELECTION_ROUND) != 0)
+ {
+ dc.DrawRoundedRectangle( rect, 8 );
One is the radius of the selection rectangle that I figured out testing all if individual numbers and using a 4x magnification, the other is the white space that the native control leaves left of the actual data. Looking at this again right one, this white space might be the unused space from rows that have no chevron. Not easy to deduce the logic...
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.