wxDC::SetDeviceClippingRegion() may create invalid clipping region in RTL (Issue #26348)
AliKet
AliKet created an issue (wxWidgets/wxWidgets#26348)Description
wxDC::SetDeviceClippingRegion() as documented works with physical coordinates and not with the logical ones.
This means that if a rectangle clipRect is expressed in logical coordinates, it needs to be converted to device coordinates before passing it to SetDeviceClippingRegion() like this:
clipRect = wxRect(dc.LogicalToDevice(clipRect.GetPosition()),
dc.LogicalToDeviceRel(clipRect.GetSize()));
dc.SetDeviceClippingRegion(clipRect);The above code will not work correctly under wxMSW and wxQt in RTL and here is why:
SetDeviceClippingRegion()under these platforms just store the clipping region as is (replacing the old one if necessary)- GDI mirrors the clipping region for RTL at drawing time
- The clipRect is already mirrored for RTL due to calling
LogicalToDevice{Rel}()which is just a wrapper aroundLPtoDP - The end result is invalid clipping region
One way to fix this problem is to force LTR when converting the rectangle before calling SetDeviceClippingRegion():
const auto oldLayoutDir = m_dc.GetLayoutDirection(); dc.SetLayoutDirection(wxLayout_LeftToRight); // Force LTR clipRect = wxRect(dc.LogicalToDevice(clipRect.GetPosition()), dc.LogicalToDeviceRel(clipRect.GetSize())); dc.SetLayoutDirection(oldLayoutDir); dc.SetDeviceClippingRegion(clipRect);
This is a reference image in LTR:
m2.png (view on web)
The correct RTL version should look like this:
m1.png (view on web)
But we get this insread:
m3.png (view on web)
Patch (01) to reproduce the above images:
diff --git a/samples/minimal/minimal.cpp b/samples/minimal/minimal.cpp index 5f32257c6b..55493571b1 100644 --- a/samples/minimal/minimal.cpp +++ b/samples/minimal/minimal.cpp @@ -26,6 +26,8 @@ #include "wx/wx.h" #endif +#include "wx/dcbuffer.h" + // ---------------------------------------------------------------------------- // resources // ---------------------------------------------------------------------------- @@ -141,6 +143,10 @@ bool MyApp::OnInit() MyFrame::MyFrame(const wxString& title) : wxFrame(nullptr, wxID_ANY, title) { + // Uncomment the next line to test wxEVT_PAINT with plain wxPaintDC +// SetDoubleBuffered(true); + SetBackgroundStyle(wxBG_STYLE_PAINT); + // set the frame icon SetIcon(wxICON(sample)); @@ -175,6 +181,39 @@ MyFrame::MyFrame(const wxString& title) CreateStatusBar(2); SetStatusText("Welcome to wxWidgets!"); #endif // wxUSE_STATUSBAR + + SetLayoutDirection(wxLayout_RightToLeft); + + Bind(wxEVT_PAINT, [this](wxPaintEvent& ) + { + wxAutoBufferedPaintDC dc(this); + + dc.Clear(); + + wxRect clipRect{10, 10, 100, 100}; + dc.SetClippingRegion(clipRect); + + dc.SetBackground(*wxBLUE_BRUSH); + dc.Clear(); + + { + const auto oldLayoutDir = dc.GetLayoutDirection(); + // Uncomment the next line to fix SetDeviceClippingRegion() +// dc.SetLayoutDirection(wxLayout_LeftToRight); // Force LTR + + clipRect = wxRect(dc.LogicalToDevice(clipRect.GetPosition()), + dc.LogicalToDeviceRel(clipRect.GetSize())); + + dc.SetLayoutDirection(oldLayoutDir); + } + + clipRect.Deflate(2); + + dc.SetDeviceClippingRegion(clipRect); + + dc.SetBackground(*wxYELLOW_BRUSH); + dc.Clear(); + }); }
Simple code with pure GDI calls for testing :
#include <windows.h> #pragma comment(lib, "gdi32") #pragma comment(lib, "user32") LRESULT CALLBACK WndProc(HWND hwnd, UINT msg, WPARAM wParam, LPARAM lParam) { switch (msg) { case WM_PAINT: { PAINTSTRUCT ps; HDC hdc = BeginPaint(hwnd, &ps); // Enable RTL layout SetLayout(hdc, LAYOUT_RTL); RECT rect = {10, 10, 100, 100}; RECT rect2 = rect; InflateRect(&rect2, 1, 1); // This will appear on the RIGHT side in RTL Rectangle(hdc, rect2.left, rect2.top, rect2.right-rect2.left-1, rect2.bottom-rect2.top-1); // LPtoDP(hdc, (POINT*)&rect, 2); // --- 2. Create a clip region in DEVICE coordinates --- // This is ALWAYS interpreted from top-left HRGN hrgn = CreateRectRgn(rect.left, rect.top, rect.right-rect.left, rect.bottom-rect.top); SelectClipRgn(hdc, hrgn); // Fill clipped area HBRUSH hBrush = CreateSolidBrush(RGB(250, 240, 60)); FillRect(hdc, &rect, hBrush); DeleteObject(hBrush); DeleteObject(hrgn); EndPaint(hwnd, &ps); return 0; } case WM_DESTROY: PostQuitMessage(0); return 0; } return DefWindowProc(hwnd, msg, wParam, lParam); } int WINAPI WinMain(HINSTANCE hInstance, HINSTANCE hPrevInstance, LPSTR lpCmdLine, int nCmdShow) { const char CLASS_NAME[] = "RTLClipTest"; WNDCLASS wc = {0}; wc.lpfnWndProc = WndProc; wc.hInstance = hInstance; wc.lpszClassName = CLASS_NAME; RegisterClass(&wc); HWND hwnd = CreateWindowEx( 0, CLASS_NAME, "RTL Clip Test", WS_OVERLAPPEDWINDOW, CW_USEDEFAULT, CW_USEDEFAULT, 400, 300, NULL, NULL, hInstance, NULL ); ShowWindow(hwnd, nCmdShow); MSG msg = {0}; while (GetMessage(&msg, NULL, 0, 0)) { TranslateMessage(&msg); DispatchMessage(&msg); } return 0; }
Platform and version information
- wxWidgets version you use: master
- wxWidgets port you use: wxMSW and wxQt
- OS and its version:
- GTK version:
- Which GDK backend is used:
- Desktop environment :
- Current theme:
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
AliKet left a comment (wxWidgets/wxWidgets#26348)wxDCClipper produces invalid clipping region in RTL when destroyed.
This is the render result (File > Render) of the griddemo (in RTL):
Correct rendering should be:
grid_render_rtl_goor.png (view on web)Patch fixing wxDCClipper:
diff --git a/include/wx/dc.h b/include/wx/dc.h index 5a65ff94d4..4e47b09217 100644 --- a/include/wx/dc.h +++ b/include/wx/dc.h @@ -1630,7 +1630,10 @@ private: void Init(const wxRect& r) { m_restoreOld = m_dc.GetClippingBox(m_oldClipRect); + const auto oldLayoutDir = m_dc.GetLayoutDirection(); + m_dc.SetLayoutDirection(wxLayout_LeftToRight); m_oldClipRect = wxRect(m_dc.LogicalToDevice(m_oldClipRect.GetPosition()), m_dc.LogicalToDeviceRel(m_oldClipRect.GetSize())); + m_dc.SetLayoutDirection(oldLayoutDir); m_dc.SetClippingRegion(r); }
—
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#26348)I think wxMSW/wxQt should undo the mirroring in their SetDeviceClippingRegion() implementation, i.e. mirror the rectangle again if the layout is RTL. This is not optimal, but would allow the existing code to work. Am I missing some reason we can't fix it like this?
Have you tested this in wxGTK, does the existing code work there?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
AliKet left a comment (wxWidgets/wxWidgets#26348)I think wxMSW/wxQt should undo the mirroring in their
SetDeviceClippingRegion()implementation, i.e. mirror the rectangle again if the layout is RTL. This is not optimal, but would allow the existing code to work. Am I missing some reason we can't fix it like this?
The problem is that we don't know from where the coordinates passed to SetDeviceClippingRegion() come from: are they already mirrored as returned by LogicalToDevice() or not.
Have you tested this in wxGTK, does the existing code work there?
Yes, wxGTK works correctly, and LogicalToDevice() and the other functions always return the unmirrored coordinates in RTL under wxGTK. So I changed the wxMSW implementations here #26398 for consistency.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
AliKet left a comment (wxWidgets/wxWidgets#26348)BTW, I'm preparing a PR for wxQt to also make it consistent with wxGTK too (i mean LogicalToDevice() and the other ones should return unmirrored coordinates in RTL)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.