wxRichTextCtrl: Fix selection jitter on macOS (PR #26105)
Joshua Lee Ockert
On macOS, selecting text in wxRichTextCtrl caused visible horizontal jitter as the text shifted left and right during selection. This was caused by splitting DrawText() calls at selection boundaries, which made each text chunk get shaped independently with different kerning.
This commit introduces DrawTabbedStringWithPartialSelection() for macOS, which renders text in a single pass (splitting only at tabs), eliminating the kerning discontinuities that caused jitter.
The implementation matches native macOS behavior where selected text maintains its original styling (like TextEdit in Rich Text mode), with the selection highlight drawn as a background rectangle that completely overrides any text background colors in the selected range.
See #22771 (unable to reproduce on Windows, but confirmed on macOS)
Selection in TextEdit:
PR.915e340.TextEdit.Selection.png (view on web)Selection in wxRichTextCtrl with this commit:
PR.915e340.wxRTC.Selection.png (view on web)Demonstration of lack of left-right jitter:
https://github.com/user-attachments/assets/754bd6e8-ce9a-4b24-8ea9-72b5d38573cf
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/26105
Commit Summary
- 915e340 wxRichTextCtrl: Fix selection jitter on macOS
File Changes
(2 files)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/26105.patch
- https://github.com/wxWidgets/wxWidgets/pull/26105.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Joshua Lee Ockert
@torstenvl pushed 1 commit.
- 5fdd1b7 wxRichTextCtrl: Fix selection jitter on macOS
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Joshua Lee Ockert
torstenvl left a comment (wxWidgets/wxWidgets#26105)Two force-pushed updates:
- To remove a single trailing space in a comment, which caused a failure in a code check.
- No changes, force-pushed only to force restart of checks after the appveyor build failed due to (what appears to be) a transient network issue installing the Golang MSI.
—
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 your work on this! We definitely need to merge this to fix the user-visible problem, but it would be nice to clean this up a bit before merging, notably to reduce the number of preprocessor checks.
Please let me know if you think you could do it. TIA!
In src/richtext/richtextbuffer.cpp:
> +{
+ ///////////////////////////////////////////////////////////////////////////
+ // This function assumes str has uniform formatting (attr applies to the //
+ // entire string). The calling code should split text into style runs //
+ // before calling this function. //
+ ///////////////////////////////////////////////////////////////////////////
+
+ wxColour highlightBackgroundColor(wxSystemSettings::GetColour(wxSYS_COLOUR_HIGHLIGHT));
+ wxColour standardTextColor = (attr.HasTextColour() && attr.GetTextColour().IsOk()) ? attr.GetTextColour() : wxSystemSettings::GetColour(wxSYS_COLOUR_WINDOWTEXT);
+ wxCoord originx = x;
+ wxArrayInt startpos; // Character starting positions
+ wxArrayInt endpos; // Character ending positions (cumulative widths)
+ wxArrayInt tabstops; // Array of tab stop positions
+ wxPen savedpen;
+
+ int strikethrough = (attr.HasTextEffects() && (attr.GetTextEffects() & wxTEXT_ATTR_EFFECT_STRIKETHROUGH)) ? 1 : 0;
This looks like a bool, why does it have int type?
- int strikethrough = (attr.HasTextEffects() && (attr.GetTextEffects() & wxTEXT_ATTR_EFFECT_STRIKETHROUGH)) ? 1 : 0; + const bool strikethrough = (attr.GetTextEffects() & wxTEXT_ATTR_EFFECT_STRIKETHROUGH)) != 0;
In src/richtext/richtextbuffer.cpp:
> + /////////////////////////////////////////////////////////////////////////// + // This function assumes str has uniform formatting (attr applies to the // + // entire string). The calling code should split text into style runs // + // before calling this function. // + /////////////////////////////////////////////////////////////////////////// + + wxColour highlightBackgroundColor(wxSystemSettings::GetColour(wxSYS_COLOUR_HIGHLIGHT)); + wxColour standardTextColor = (attr.HasTextColour() && attr.GetTextColour().IsOk()) ? attr.GetTextColour() : wxSystemSettings::GetColour(wxSYS_COLOUR_WINDOWTEXT); + wxCoord originx = x; + wxArrayInt startpos; // Character starting positions + wxArrayInt endpos; // Character ending positions (cumulative widths) + wxArrayInt tabstops; // Array of tab stop positions + wxPen savedpen; + + int strikethrough = (attr.HasTextEffects() && (attr.GetTextEffects() & wxTEXT_ATTR_EFFECT_STRIKETHROUGH)) ? 1 : 0; + int strikeheight = (int) (rect.y + rect.GetHeight()/2.0 + 0.5); // Cast truncates; +0.5 forces rounding
Or could just use wxRound():
- int strikeheight = (int) (rect.y + rect.GetHeight()/2.0 + 0.5); // Cast truncates; +0.5 forces rounding + const int strikeheight = wxRound(rect.y + rect.GetHeight()/2.0);
In src/richtext/richtextbuffer.cpp:
> + int strikeheight = (int) (rect.y + rect.GetHeight()/2.0 + 0.5); // Cast truncates; +0.5 forces rounding + int tabcount; + int effectivetabstop; + int posadjustment; + int printstart; + int i = 0; // initialization iterator + int si = 0; // string iterator + int ti = 0; // tabstop iterator + int pi = 0; // position iterator + + + /////////////////////////////////////////////////////////////////////////// + // Initial setup // + /////////////////////////////////////////////////////////////////////////// + + if (str.Len() == 0) return true; // Nonsensical but it's what the original does
If it's really nonsensical, i.e. if we can guarantee that the string is never empty, I'd just remove it. Otherwise
⬇️ Suggested change- if (str.Len() == 0) return true; // Nonsensical but it's what the original does + if (str.empty()) + return true; // Nonsensical but it's what the original does
In src/richtext/richtextbuffer.cpp:
> + int ti = 0; // tabstop iterator
+ int pi = 0; // position iterator
+
+
+ ///////////////////////////////////////////////////////////////////////////
+ // Initial setup //
+ ///////////////////////////////////////////////////////////////////////////
+
+ if (str.Len() == 0) return true; // Nonsensical but it's what the original does
+
+ // Get character ending positions
+ dc.GetPartialTextExtents(str, endpos);
+ startpos.SetCount(endpos.GetCount());
+
+ startpos[0] = originx;
+ for (i = 0; i < endpos.GetCount() - 1; i++) {
wx style mandates putting { on its own line, i.e.
- for (i = 0; i < endpos.GetCount() - 1; i++) {
+ for (i = 0; i < endpos.GetCount() - 1; i++)
+ {
(here and below).
In include/wx/richtext/richtextbuffer.h:
> @@ -4766,6 +4766,9 @@ class WXDLLIMPEXP_RICHTEXT wxRichTextPlainText: public wxRichTextObject
private:
bool DrawTabbedString(wxDC& dc, const wxRichTextAttr& attr, const wxRect& rect, wxString& str, wxCoord& x, wxCoord& y, bool selected);
+#ifdef __WXMAC__
+ bool DrawTabbedStringWithPartialSelection(wxDC& dc, const wxRichTextAttr& attr, const wxRect& rect, wxString& str, wxCoord& x, wxCoord& y, int selStart, int selEnd);
AFAICS the return value is always ignored, why have it?
In src/richtext/richtextbuffer.cpp:
> + dc.GetTextExtent(str, &w, &h); // Get actual text height + wxRect bgRect(startpos[0], y, backgroundWidth, h);
GetTextExtent() overload return wxSize is more convenient and concise to use:
- dc.GetTextExtent(str, &w, &h); // Get actual text height - wxRect bgRect(startpos[0], y, backgroundWidth, h); + wxRect bgRect(startpos[0], y, backgroundWidth, dc.GetTextExtent(str).y);
(and w and h don't need to be declared any more).
In src/richtext/richtextbuffer.cpp:
> + wxCheckSetBrush(dc, wxBrush(attr.GetBackgroundColour()));
+ wxCheckSetPen(dc, wxPen(attr.GetBackgroundColour()));
+ dc.GetTextExtent(str, &w, &h); // Get actual text height
+ wxRect bgRect(startpos[0], y, backgroundWidth, h);
+ dc.DrawRectangle(bgRect);
+ wxCheckSetPen(dc, savedpen);
+ }
+
+ ///////////////////////////////////////////////////////////////////////////
+ // Draw selection rectangle //
+ ///////////////////////////////////////////////////////////////////////////
+
+ if (selStart >=0 && selStart < endpos.GetCount()) {
+ if (selEnd >= selStart && selEnd < endpos.GetCount()) {
+ int selectionWidth = endpos[selEnd] - startpos[selStart] + 1; // Avoid kerning gaps
+ savedpen = dc.GetPen();
This really ought to be done using wxDCPenChanger, i.e.
- savedpen = dc.GetPen(); + wxDCPenChanger changePen(dc, highlightBackgroundColor);
(and remove wxCheckSetPen() below).
This is not completely identical because we lose the checks done in wxCheckSetPen() but they don't seem relevant anyhow here because the pen colour does change.
In src/richtext/richtextbuffer.cpp:
> + wxRect selRect(startpos[selStart], rect.y, selectionWidth, rect.GetHeight()); + dc.DrawRectangle(selRect);
This could also be simplified to just
⬇️ Suggested change- wxRect selRect(startpos[selStart], rect.y, selectionWidth, rect.GetHeight()); - dc.DrawRectangle(selRect); + dc.DrawRectangle(startpos[selStart], rect.y, selectionWidth, rect.GetHeight());
(unless you want to have this variable to show that it's the selection rect? but this seems clear enough anyhow...)
In src/richtext/richtextbuffer.cpp:
> + for (si = 0; si < str.Len(); si++) {
+ if (str[si] == '\t') {
+ if (si - printstart > 0) {
+ dc.DrawText(str.Mid(printstart, si-printstart), startpos[printstart], y);
+ if (strikethrough) dc.DrawLine(startpos[printstart], strikeheight, startpos[si], strikeheight);
+ }
+ if (selStart <= si && si <= selEnd && strikethrough) {
+ // Matching original behavior of only showing strikethrough on tabs when selected
+ dc.DrawLine(startpos[si], strikeheight, endpos[si], strikeheight);
+ }
+ printstart = si + 1;
+ }
+ }
+ if (printstart < str.Len()) {
+ dc.DrawText(str.Mid(printstart), startpos[printstart], y);
+ if (strikethrough) dc.DrawLine(startpos[printstart], strikeheight, endpos[endpos.GetCount()-1], strikeheight);
Please don't put compound statements on the same line.
⬇️ Suggested change- if (strikethrough) dc.DrawLine(startpos[printstart], strikeheight, endpos[endpos.GetCount()-1], strikeheight); + if (strikethrough) + dc.DrawLine(startpos[printstart], strikeheight, endpos[endpos.GetCount()-1], strikeheight);
In include/wx/richtext/richtextbuffer.h:
> @@ -4766,6 +4766,9 @@ class WXDLLIMPEXP_RICHTEXT wxRichTextPlainText: public wxRichTextObject
private:
bool DrawTabbedString(wxDC& dc, const wxRichTextAttr& attr, const wxRect& rect, wxString& str, wxCoord& x, wxCoord& y, bool selected);
+#ifdef __WXMAC__
It's not great to have checks for Mac, especially in the headers. Could you please change this to:
- Keep the current function name ("WithPartialSelection" is not really useful because it is also called when there is no selection and everything is selected).
- Replace its
bool selectedparameter withint selStart, int selEnd(alternatively, you could usewxRange selection, seewx/range.h). - Only use
#ifdef __WXMAC__to selection between the implementations in the source file.
I think this would make things a bit tidier.
In src/richtext/richtextbuffer.cpp:
> }
else
{
+#ifdef __WXMAC__
+ int selStart = wxMax(selectionRange.GetStart(), range.GetStart()) - range.GetStart();
+ int selEnd = wxMin(selectionRange.GetEnd(), range.GetEnd()) - range.GetStart();
+ DrawTabbedStringWithPartialSelection(dc, textAttr, rect, stringChunk, x, y, selStart, selEnd);
+#else
All the code in this #else branch would need to be moved to the new version of DrawTabbedString().
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Joshua Lee Ockert
torstenvl left a comment (wxWidgets/wxWidgets#26105)Not a problem! As I'm a new contributor, I was conservative about existing code in a lot of places, but I can absolutely do some refactoring to make the end result nicer.
Especially with the existing code's line 6937 // TODO: new selection code (from Julian Smart, 16 years ago) 😂🤣
Understand the style preference not to have single line if statements and I can work with that.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Joshua Lee Ockert
@torstenvl commented on this pull request.
In src/richtext/richtextbuffer.cpp:
> + /////////////////////////////////////////////////////////////////////////// + // This function assumes str has uniform formatting (attr applies to the // + // entire string). The calling code should split text into style runs // + // before calling this function. // + /////////////////////////////////////////////////////////////////////////// + + wxColour highlightBackgroundColor(wxSystemSettings::GetColour(wxSYS_COLOUR_HIGHLIGHT)); + wxColour standardTextColor = (attr.HasTextColour() && attr.GetTextColour().IsOk()) ? attr.GetTextColour() : wxSystemSettings::GetColour(wxSYS_COLOUR_WINDOWTEXT); + wxCoord originx = x; + wxArrayInt startpos; // Character starting positions + wxArrayInt endpos; // Character ending positions (cumulative widths) + wxArrayInt tabstops; // Array of tab stop positions + wxPen savedpen; + + int strikethrough = (attr.HasTextEffects() && (attr.GetTextEffects() & wxTEXT_ATTR_EFFECT_STRIKETHROUGH)) ? 1 : 0; + int strikeheight = (int) (rect.y + rect.GetHeight()/2.0 + 0.5); // Cast truncates; +0.5 forces rounding
No issues, I just used the existing code (and added the comment since I kept forgetting why they did it this way). Original in DrawTabbedString is dc.DrawLine(x, (int) (y+(h/2)+0.5), x+w, (int) (y+(h/2)+0.5));
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Joshua Lee Ockert
> + int strikeheight = (int) (rect.y + rect.GetHeight()/2.0 + 0.5); // Cast truncates; +0.5 forces rounding + int tabcount; + int effectivetabstop; + int posadjustment; + int printstart; + int i = 0; // initialization iterator + int si = 0; // string iterator + int ti = 0; // tabstop iterator + int pi = 0; // position iterator + + + /////////////////////////////////////////////////////////////////////////// + // Initial setup // + /////////////////////////////////////////////////////////////////////////// + + if (str.Len() == 0) return true; // Nonsensical but it's what the original does
The original doesn't use partial text extents, so there's no subscripting of the character positions, so they don't early return. However, it returns true under all code paths, so I borrowed that behavior. As you point out, the original is a bool but the return value is never used (and it's a private method, so the end-user won't use the return value either). Will change to void during refactor.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Joshua Lee Ockert
@torstenvl pushed 1 commit.
- 3239342 wxRichTextCtrl: Refactor selection changes
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Joshua Lee Ockert
torstenvl left a comment (wxWidgets/wxWidgets#26105)Did a complete refactor of wxRichTextPlainText::DrawTabbedString to minimize platform-specific code and localize it in one place (with an explanation for why we took different, platform-specific approaches). Verified that selection now works correctly on Mac, Windows, and Linux/GTK, with no left-right jitter bug.
Mac:
https://github.com/user-attachments/assets/3f360e0e-b150-49ae-a2e9-5e093b58fc63
Windows:
https://github.com/user-attachments/assets/7bb9285d-55da-419b-9f21-1c26c1ccdad8
Linux:
https://github.com/user-attachments/assets/0c872bc2-1d50-497e-8819-e875cc1d2c51
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26105)Thanks for the update!
I've (squash) merged it now with only just some very minor formatting changes (to avoid really overlong lines) and use of std::string-like length() instead of old Len() for string lengths.
Thanks again for fixing this!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.