wxMSW: A better (IMHO) RTL fixes for wx{Auto}BufferedDC (PR #26398)
AliKet
Also fix custom drawing where wxDC::DeviceToLogical() and related functions need to be used.
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/26398
Commit Summary
- 2e0d08c wxDC::LogicalToDevice{Rel}() now always return unmirrored coordinates under wxMSW
- 5827b4c Remove unnecessary hack from wxMSWDCImpl::DoSetClippingRegion()
- ada3d84 Really fix using wx{Auto}BufferedDC in RTL layout under wxMSW
- af13c88 Fix rendering GridFrame (from griddemo) in RTL layout
- dadf3ab Update OneDevRegionRTL test after recent changes
File Changes
(3 files)
- M samples/grid/griddemo.cpp (2)
- M src/msw/dc.cpp (74)
- M tests/graphics/clippingbox.cpp (7)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/26398.patch
- https://github.com/wxWidgets/wxWidgets/pull/26398.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.
- da9c92e Try to fix OneDevRegionRTL test failures
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
AliKet left a comment (wxWidgets/wxWidgets#26398)I don't have a real explanation of why we need this commit da9c92e to fix tests failures in OneDevRegionRTL, Just some weired rounding errors? I'm guessing...
—
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#26398)Micro-optimisation:
diff --git a/src/msw/dc.cpp b/src/msw/dc.cpp index dc56fa3e53..dda510a168 100644 --- a/src/msw/dc.cpp +++ b/src/msw/dc.cpp @@ -2379,10 +2379,12 @@ bool wxMSWDCImpl::DoStretchBlit(wxCoord xdest, wxCoord ydest, // automatically as it doesn't even work with the source HDC. // So do this manually to ensure that the coordinates are // interpreted in the same way here as in all the other cases. - xsrc = source->LogicalToDeviceX(xsrcOrig); - ysrc = source->LogicalToDeviceY(ysrcOrig); - srcWidth = source->LogicalToDeviceXRel(srcWidth); - srcHeight = source->LogicalToDeviceYRel(srcHeight); + const auto devPos = implSrc->LogicalToDevice(xsrcOrig, ysrcOrig); + const auto devSize = implSrc->LogicalToDeviceRel(srcWidth, srcHeight); + xsrc = devPos.x; + ysrc = devPos.y; + srcWidth = devSize.x; + srcHeight = devSize.y; // Figure out what co-ordinate system we're supposed to specify // ysrc in.
LogicalToDevice{X,Y}[Rel]() now internally call SetLayout() to force LTR before calling LPtoDP() as a pre-condition and restore the old layout when the function returns as a post-condition. So there will be 8 calls to SetLayout() with the old code, but 4 with the patch above. I didn't try to profile this to see whether the patch makes any difference, but I still prefer to apply it, though.
—
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#26398)@vslavik I would be grateful if you could test this and see whether it breaks something I’m not aware of. Thanks in advance!
—
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.
Sorry, I'm still very confused by this stuff. It would be great to have an explanation in a comment somewhere about how RTL works in wxMSW, i.e. which coordinates take into account and which don't. Right now I don't understand why is is it correct to disable RTL when converting between device and logical, hopefully it would become clearer with a comment like this...
In src/msw/dc.cpp:
> @@ -68,7 +69,7 @@ wxIMPLEMENT_ABSTRACT_CLASS(wxMSWDCImpl, wxDCImpl); // The device space in Win32 GDI measures 2^27*2^27 , so we use 2^27-1 as the // maximal possible view port extent. -static const int VIEWPORT_EXTENT = 134217727; +static const int VIEWPORT_EXTENT = 134217726;
I'm not sure why did this change, but the comment above needs to be updated as it's now 2^27-2 and not -1. Ideal, of course, would be to also explain why is an extra -1 needed.
In src/msw/dc.cpp:
> @@ -101,6 +102,19 @@ static const int VIEWPORT_EXTENT = 134217727;
#define XDEV2LOG(x) ((x) - (m_deviceOriginX*m_signX / m_scaleX))
#define YDEV2LOG(y) ((y) - (m_deviceOriginY*m_signY / m_scaleY))
+// ----------------------------------------------------------------------------
+// macro to temporarily disable RTL layout if already set on the device context
+// ----------------------------------------------------------------------------
+
+// Mainly used with the LogicalToDevice{Rel}()/DeviceToLogical{Rel}() functions
+// to force them to return unmirrored coordinates if the LAYOUT_RTL flag is set
+// on the device context. This avoids double-mirroring problems when the result
+// is passed to GDI drawing functions. And also to be consistent with wxGTK3.
+
+#define wxSCOPED_DC_RTL_DISABLER(hdc) \
I'd prefer to replace this macro with wxScopedRTLDisabler class that would be created with the HDC to disable RTL layout for.
In src/msw/dc.cpp:
> @@ -2381,6 +2393,22 @@ bool wxMSWDCImpl::DoStretchBlit(wxCoord xdest, wxCoord ydest,
ysrc = hDIB - (ysrc + srcHeight);
}
+ if ( GetLayoutDirection() == wxLayout_RightToLeft )
+ {
+ // Unlike BitBlt() and StretchBlt(), StretchDIBits() doesn't
+ // honor the LAYOUT_RTL flag set on the DC, so we have to apply
+ // mirroring ourselves (see SetLayout() documentation in MSDN).
+ const LONG wDIB = ds.dsBmih.biWidth;
+ if ( wDIB > 0 )
Can it really be 0 here? It might be clearer/simpler to return early from this function if the width (or height) is 0.
In tests/graphics/clippingbox.cpp:
> @@ -1030,7 +1030,7 @@ static void OneDevRegionRTL(wxDC& dc, const wxBitmap& bmp, bool useTransformMatr
// Setting one clipping region in device coordinates
// inside transformed DC area.
- const int x = 11;
+ const int x = 10;
Curious why changing this still works for wxGTK?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Václav Slavík
vslavik left a comment (wxWidgets/wxWidgets#26398)@vslavik I would be grateful if you could test this and see whether it breaks something I’m not aware of. Thanks in advance!
Sorry, I wanted to, but ran out of time and won’t be able to until May 12-ish.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
AliKet
@AliKet pushed 13 commits.
- 0f94d2d Define WX_ATTRIBUTE_UNUSED correctly for clang-cl
- 4b8c418 Remove "extern" from wxGlobalSEInformation definition
- e40bf73 Use WinStruct<> instead of manually initializing HIGHCONTRAST
- cd54827 Make wxToastEventHandler final to avoid warnings when deleting it
- 345a15d Add a cast to suppress signed/unsigned comparison warning
- 1145bbf Use "__declspec(nothrow)" when overriding IUnknown functions
- 2e030a7 wxDC::LogicalToDevice{Rel}() now return unmirrored coordinates in RTL under wxMSW
- 26ea02d Remove unnecessary hack from wxMSWDCImpl::DoSetClippingRegion()
- 241b990 Really fix using wx{Auto}BufferedDC in RTL layout under wxMSW
- f133967 Change viewport extent to avoid overflow when computing clip extents in RTL
- 5710320 Reduce the scope of VIEWPORT_EXTENT constant
- 2c39cce Fix rendering GridFrame (in griddemo) in RTL layout
- b4ba89f Update OneDevRegionRTL test after recent changes
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26398)I'm still not sure if it this is correct, it seems wrong to disable RTL when doing device/logical conversions. But if this is how the other ports work and if there is no good way to fix this otherwise, we could still merge this...
@vslavik Please let me know what do you think when you get back.
—
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 7 commits.
- 61ab3bf wxDC::LogicalToDevice{Rel}() now return unmirrored coordinates in RTL under wxMSW
- 60a4fd0 Remove unnecessary hack from wxMSWDCImpl::DoSetClippingRegion()
- 005afa8 Really fix using wx{Auto}BufferedDC in RTL layout under wxMSW
- 8d0c94a Change viewport extent to avoid overflow when computing clip extents in RTL
- f3333e7 Reduce the scope of VIEWPORT_EXTENT constant
- a7b81ad Fix rendering GridFrame (in griddemo) in RTL layout
- e607f6b Update OneDevRegionRTL test after recent changes
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
@AliKet pushed 1 commit.
- e468462 Update OneDevRegionRTL test after recent changes
—
View it on GitHub or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
AliKet
@AliKet pushed 1 commit.
- 526a148 Update OneDevRegionRTL test after recent changes
—
View it on GitHub or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.