wxQt RTL fixes (PR #25822)
AliKet
Some screenshots of the result of this PR:
Widgets demo:
widgets_rtl.png (view on web)
Grid demo:
griddemo_rtl.png (view on web)
AUI demo:
auidemo_rtl.png (view on web)
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/25822
Commit Summary
- 9c868a8 wxQt: Fix window positioning in RTL layout.
- f47a042 wxQt: Fix wxWindow::Refresh() not working correctly in RTL layout.
- 9a0faac wxQt: Fix window scrolling in RTL layout
- cbe1bd0 wxQt: Fix ClientToScreen() and ScreenToClient() to work correctly in RTL layout.
- 1ce42a7 wxQt: Fix popup menu position in RTL layout
- 6919cd1 wxQt: Override wxDC::{Get,Set}LayoutDirection()
- 955bb3a wxQt: Apply custom mirroring to wxDC in RTL layout.
- 8f64804 wxQt: Fix wxDC::GetSize() for wxMemoryDC
- 37b20aa wxQt: Fix wxAuiManager::CalculateHintRect() in RTL layout
- e1a527f wxQt: Fix drawing wxGrid lines in RTL layout
- 40e7903 wxQt: Fix drawing wxTreeCtrl icons in RTL layout
- 59c1788 wxQt: Fix wxOverlay in RTL layout
- a7713e7 wxQt: Fix some native control rendering in RTL layout
- 16083e7 Fix clippingbox's OneDevRegionRTL test case
File Changes
(12 files)
- M include/wx/qt/dc.h (15)
- M include/wx/qt/dcmemory.h (2)
- M include/wx/qt/window.h (7)
- M src/aui/framemanager.cpp (2)
- M src/generic/grid.cpp (2)
- M src/qt/dc.cpp (137)
- M src/qt/dcmemory.cpp (16)
- M src/qt/overlay.cpp (14)
- M src/qt/renderer.cpp (87)
- M src/qt/treectrl.cpp (6)
- M src/qt/window.cpp (181)
- M tests/graphics/clippingbox.cpp (5)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/25822.patch
- https://github.com/wxWidgets/wxWidgets/pull/25822.diff
—
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#25822)Amazing!
—
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 10 commits.
- 137006d wxQt: Override wxDC::{Get,Set}LayoutDirection()
- 9413bde wxQt: Override wxDC::GetLayoutDirection() in wxMemoryDC
- 5b0c369 wxQt: Apply custom mirroring to wxDC in RTL layout.
- 6888227 wxQt: Fix wxDC::GetSize() for wxMemoryDC
- cb65081 wxQt: Fix wxAuiManager::CalculateHintRect() in RTL layout
- 6f0062b wxQt: Fix drawing wxGrid lines in RTL layout
- 6686207 wxQt: Fix drawing wxTreeCtrl icons in RTL layout
- a30936a wxQt: Fix wxOverlay in RTL layout
- 4b5f389 wxQt: Fix some native control rendering in RTL layout
- fd7c422 Fix clippingbox's OneDevRegionRTL test case
—
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, this looks good and could be merged AFAICS, I'm not sure why is this in the "draft" state yet?
Also, unfortunately I have no idea why did SliderTestCase start consistently failing with Qt 6.8 just recently, but this is unrelated to this change.
> @@ -117,6 +122,14 @@ class WXDLLIMPEXP_CORE wxQtDCImpl : public wxDCImpl
virtual void* GetHandle() const override { return (void*) m_qtPainter; }
+ // LRT/RTL related functions
Typo:
⬇️ Suggested change- // LRT/RTL related functions + // LTR/RTL related functions
> @@ -3478,11 +3478,13 @@ wxRect wxAuiManager::CalculateHintRect(wxWindow* pane_window,
m_frame->ClientToScreen(&rect.x, &rect.y);
+#ifndef __WXQT__
I was always suspicious of this piece of code, if it's not needed for wxQt, it seems to confirm that it shouldn't be necessary for the other ports as well... Do you have any idea what could be wrong in/with them to require it?
In src/qt/dc.cpp:
> @@ -462,10 +496,10 @@ void wxQtDCImpl::Clear()
int width, height;
DoGetSize(&width, &height);
- m_qtPainter->eraseRect( DeviceToLogicalX(0),
- DeviceToLogicalY(0),
- DeviceToLogicalXRel(width),
- DeviceToLogicalYRel(height) );
+ const wxPoint pos = DeviceToLogical(0, 0);
Just to confirm: this doesn't really change anything, does it? I.e. DeviceToLogicalX() etc work too?
> + // For correct result in RTL layout, the drawing is done in a non-mirrored + // DC and the mirroring is done in paintEvent() above.
Sorry, I feel dumb, but I don't really understand why do we need to do this. I.e. what is wrong with using RTL layout for the DC itself instead of mirroring it later?
> + default: + wxFALLTHROUGH; +
I'd remove this to get a warning in the (unlikely) event of more elements being added to this enum.
In tests/graphics/clippingbox.cpp:
> @@ -1033,9 +1033,9 @@ static void OneDevRegionRTL(wxDC& dc, const wxBitmap& bmp, bool useTransformMatr
return;
}
-#ifdef __WXGTK__
+#if defined(__WXGTK__) || defined(__WXMSW__)
Could we perhaps add a check for __WXMSW__ below instead of disabling this test entirely for it?
—
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 19 commits.
- 95d3fb9 wxQt: Fix window positioning in RTL layout.
- ce427d5 wxQt: Fix wxWindow::Refresh() not working correctly in RTL layout.
- 6f5cbdd wxQt: Fix window scrolling in RTL layout
- 9365edb wxQt: Fix ClientToScreen() and ScreenToClient() to work correctly in RTL layout.
- ba72753 wxQt: Fix popup menu position in RTL layout
- fee7fd7 wxQt: Override wxDC::{Get,Set}LayoutDirection()
- 4b86491 wxQt: Override wxDC::SetLayoutDirection() in wxMemoryDC
- 883f95b wxQt: Apply custom mirroring to wxDC in RTL layout.
- 69243bd wxQt: Simplify wxDC::DoDraw{Rotated}Text() implementations
- 57a9461 wxQt: Fix drawing multiline text with wxDC::DrawText()
- d838e46 wxQt: Fix drawing (rotated) text in RTL layout
- 6d730db wxQt: Fix drawing (rotated) text with GC in RTL layout
- 8f835d4 wxQt: Fix wxDC::GetSize() for wxMemoryDC
- 44f178f wxQt: Fix wxAuiManager::CalculateHintRect() in RTL layout
- c23bca6 wxQt: Fix drawing wxGrid lines in RTL layout
- 594c884 wxQt: Fix drawing wxTreeCtrl icons in RTL layout
- e1ecd45 wxQt: Fix wxOverlay in RTL layout
- 3f82398 wxQt: Fix some native control rendering in RTL layout
- 0174e8c Fix clippingbox's OneDevRegionRTL test case
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
@AliKet pushed 12 commits.
- 269829e wxQt: Apply custom mirroring to wxDC in RTL layout.
- e09ea4f wxQt: Simplify wxDC::DoDraw{Rotated}Text() implementations
- 39238fc wxQt: Fix drawing multiline text with wxDC::DrawText()
- e6675e6 wxQt: Fix drawing (rotated) text in RTL layout
- da92795 wxQt: Fix drawing (rotated) text with GC in RTL layout
- 7b98f2c wxQt: Fix wxDC::GetSize() for wxMemoryDC
- 08b2160 wxQt: Fix wxAuiManager::CalculateHintRect() in RTL layout
- ee9aea5 wxQt: Fix drawing wxGrid lines in RTL layout
- 0ed0bcb wxQt: Fix drawing wxTreeCtrl icons in RTL layout
- 3e34e2f wxQt: Fix wxOverlay in RTL layout
- adfe916 wxQt: Fix some native control rendering in RTL layout
- 14dd4a0 Fix clippingbox's OneDevRegionRTL test case
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
@AliKet commented on this pull request.
> @@ -3478,11 +3478,13 @@ wxRect wxAuiManager::CalculateHintRect(wxWindow* pane_window,
m_frame->ClientToScreen(&rect.x, &rect.y);
+#ifndef __WXQT__
This code is also not needed when this PR is complete. see this commit
—
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.
> + // For correct result in RTL layout, the drawing is done in a non-mirrored + // DC and the mirroring is done in paintEvent() above.
If we don't do this (i.e.: forcing LTR on the DC) the rubberbanding is not drawn correctly, i.e.: the rubberband is missing near the right side of the window (image 01) and is drawn over the vertical scrollbar (image 02):
Image 01:
drawing_1.png (view on web)
Image 02:
drawing_2.png (view on web)
I haven't found the reason for this problem. I'll add a comment stating that this is to correct drawing the rubberband on a scrolled window.
—
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 tests/graphics/clippingbox.cpp:
> @@ -1033,9 +1033,9 @@ static void OneDevRegionRTL(wxDC& dc, const wxBitmap& bmp, bool useTransformMatr
return;
}
-#ifdef __WXGTK__
+#if defined(__WXGTK__) || defined(__WXMSW__)
The test is enabled for both wxMSW and wxGTK3 in this PR (see 4f49395 and 403e9f2)
I think I should merge the changes done here in that PR and close this one! 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 commented on this pull request.
In src/qt/dc.cpp:
> @@ -462,10 +496,10 @@ void wxQtDCImpl::Clear()
int width, height;
DoGetSize(&width, &height);
- m_qtPainter->eraseRect( DeviceToLogicalX(0),
- DeviceToLogicalY(0),
- DeviceToLogicalXRel(width),
- DeviceToLogicalYRel(height) );
+ const wxPoint pos = DeviceToLogical(0, 0);
DeviceToLogical{X,Y,...}() return different values than their counterpart in the other ports (i.e.: wxMSW and wxGTK3), which makes OneDevRegionRTL test case in wxQt fail!
—
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 tests/graphics/clippingbox.cpp:
> @@ -1033,9 +1033,9 @@ static void OneDevRegionRTL(wxDC& dc, const wxBitmap& bmp, bool useTransformMatr
return;
}
-#ifdef __WXGTK__
+#if defined(__WXGTK__) || defined(__WXMSW__)
Yes, probably, but whatever you decide please let me know how do you want me to process them, i.e. in which order to merge them if you don't combine them both. 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.
In src/qt/dc.cpp:
> @@ -462,10 +496,10 @@ void wxQtDCImpl::Clear()
int width, height;
DoGetSize(&width, &height);
- m_qtPainter->eraseRect( DeviceToLogicalX(0),
- DeviceToLogicalY(0),
- DeviceToLogicalXRel(width),
- DeviceToLogicalYRel(height) );
+ const wxPoint pos = DeviceToLogical(0, 0);
Hmm, but which one is correct?
—
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 20 commits.
- de8fb02 wxQt: Fix window positioning in RTL layout.
- e515f0c wxQt: Fix SetClientSize() not working correctly in RTL layout
- 43d3eb5 wxQt: Fix wxWindow::Refresh() not working correctly in RTL layout.
- 400830f wxQt: Fix window scrolling in RTL layout
- bfe7071 wxQt: Fix ClientToScreen() and ScreenToClient() to work correctly in RTL layout.
- f2fb0b2 wxQt: Fix popup menu position in RTL layout
- fb347c0 wxQt: Override wxDC::{Get,Set}LayoutDirection()
- 8f4f809 wxQt: Override wxDC::SetLayoutDirection() in wxMemoryDC
- 9d97c91 wxQt: Apply custom mirroring to wxDC in RTL layout.
- 52478a5 wxQt: Simplify wxDC::DoDraw{Rotated}Text() implementations
- 4a4011c wxQt: Fix drawing multiline text with wxDC::DrawText()
- f11441d wxQt: Fix drawing (rotated) text in RTL layout
- 55e803e wxQt: Fix drawing (rotated) text with GC in RTL layout
- 480994f wxQt: Fix wxDC::GetSize() for wxMemoryDC
- 515a484 wxQt: Fix wxAuiManager::CalculateHintRect() in RTL layout
- 7f71bd3 wxQt: Fix drawing wxGrid lines in RTL layout
- 07818dc wxQt: Fix drawing wxTreeCtrl icons in RTL layout
- c40b308 wxQt: Fix wxOverlay in RTL layout
- 9ef274a wxQt: Fix rendering some native control in RTL layout
- 8fc31fd Fix clippingbox's OneDevRegionRTL test case
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
@AliKet pushed 20 commits.
- 230e253 wxQt: Fix window positioning in RTL layout.
- 1b50d4e wxQt: Fix SetClientSize() not working correctly in RTL layout
- ba5dd92 wxQt: Fix wxWindow::Refresh() not working correctly in RTL layout.
- 18e89dc wxQt: Fix window scrolling in RTL layout
- a1d130c wxQt: Fix ClientToScreen() and ScreenToClient() to work correctly in RTL layout.
- fa58743 wxQt: Fix popup menu position in RTL layout
- 5d15d64 wxQt: Override wxDC::{Get,Set}LayoutDirection()
- e1830cb wxQt: Override wxDC::SetLayoutDirection() in wxMemoryDC
- 2411ae2 wxQt: Apply custom mirroring to wxDC in RTL layout.
- 937ea25 wxQt: Simplify wxDC::DoDraw{Rotated}Text() implementations
- ba9be4a wxQt: Fix drawing multiline text with wxDC::DrawText()
- 6e2fd6d wxQt: Fix drawing (rotated) text in RTL layout
- 4ec8dce wxQt: Fix drawing (rotated) text with GC in RTL layout
- 5c36902 wxQt: Fix wxDC::GetSize() for wxMemoryDC
- 3b7de51 wxQt: Fix drawing wxGrid lines in RTL layout
- 2bb1248 wxQt: Fix drawing wxTreeCtrl icons in RTL layout
- 68f53b2 wxQt: Fix wxOverlay in RTL layout
- 6b9309c wxQt: Fix rendering some native control in RTL layout
- 55f5433 wxAUI: Don't mirror hint rectangle in RTL layout
- b9997a7 Relaxe checking clip box in OneDevRegionRTL test case
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
AliKet left a comment (wxWidgets/wxWidgets#25822)@vadz Should I completely remove the code in 55f5433 because it seems unnecessary at least under wxMSW, wxGTK3 and wxQt?
—
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#25822)Ideal would be to check if this code is needed under Mac and, if it is, and assuming the bug can't be fixed in wxOSX itself, replace the preprocessor condition with a check for __WXOSX__. TIA!
—
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#25822)Ideal would be to check if this code is needed under Mac and, if it is, and assuming the bug can't be fixed in wxOSX itself, replace the preprocessor condition with a check for
__WXOSX__. TIA!
I don't have a Mac device to test this on it. so I'll leave the check as is then.
—
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#25822)FWIW we have a wxMac which is specifically there to be used for testing by people without access to a Mac. Please contact Stefan to get access to it.
In the meanwhile, let me know what do I need to do to test for it myself and I could do it here.
—
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#25822)In the meanwhile, let me know what do I need to do to test for it myself and I could do it here.
1- Just hardcode the wxAppBase::GetLayoutDirection() return value (for testing purposes) in src/common/appcmn.cpp
wxLayoutDirection wxAppBase::GetLayoutDirection() const { return wxLayout_RightToLeft; }
2- Run the auidemo and see if the hint rectangle is correctly position in RTL layout.
aui.png (view on web) aui2.png (view on web)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 left a comment (wxWidgets/wxWidgets#25822)wxOSX doesn't seem to be affected by changing the return value of wxAppBase::GetLayoutDirection() at all, i.e. everything still shows up the same. Unsurprisingly, removing the code in question doesn't affect it either.
I think you should remove it and when/if anybody fixes RTL support in wxOSX, they could fix the bug that this worked around, if it's still there by then.
Thanks!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz pushed 1 commit.
- 8f21c51 Remove probably unnecessary RTL workaround from wxAuiManager
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Merged #25822 into master.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.