Opened 5 months ago

Closed 3 months ago

Last modified 3 months ago

#16254 closed defect (fixed)

wxDC does not translate coordinates for RTL layout

Reported by: plorkyeran Owned by: VZ
Priority: normal Milestone: 3.1.0
Component: wxMSW Version: dev-latest
Keywords: Cc:
Blocked By: Blocking:
Patch: yes

Description

{
    wxBitmap bmp(wxSize(100, 100));
    wxMemoryDC dc(bmp);
    dc.SetLayoutDirection(wxLayout_LeftToRight);
    dc.SetBackground(*wxRED_BRUSH);
    dc.Clear();
    dc.SetBrush(*wxBLUE_BRUSH);
    dc.DrawRectangle(10, 10, 10, 10);
    dc.SetBrush(*wxGREEN_BRUSH);
    dc.DrawRectangle(-10, 20, 10, 10);
    bmp.ConvertToImage().SaveFile("c:/temp/ltr.bmp");
}

{
    wxBitmap bmp(wxSize(100, 100));
    wxMemoryDC dc(bmp);
    dc.SetLayoutDirection(wxLayout_RightToLeft);
    dc.SetBackground(*wxRED_BRUSH);
    dc.Clear();
    dc.SetBrush(*wxBLUE_BRUSH);
    dc.DrawRectangle(10, 10, 10, 10);
    dc.SetBrush(*wxGREEN_BRUSH);
    dc.DrawRectangle(-10, 20, 10, 10);
    bmp.ConvertToImage().SaveFile("c:/temp/rtl.bmp");
}

The LTR and RTL versions should be mirror images of each other, but in the RTL version the green rectangle at -10, 10 is drawn in the image and the blue rectangle is offscreen. It appears to be correctly inverting the x-axis, but the origin is left at the top-left corner rather than moved to the top-right.

This appears to apply to all DC types, and in addition wxBufferedPaintDC doesn't even draw the background (presumably because the buffer is being blitted to the wrong place).

Adding if (dc.GetLayoutDirection() == wxLayout_RightToLeft) dc.SetLogicalOrigin(dc.GetSize().GetWidth(), 0); fixes this for everything but wxAutoBufferedPaintDC, but breaks Clear(). I assume some combination of logical and device origin is needed, but I failed to find something that made both Clear() and everything else work at the same time.

Attachments (11)

ltr.bmp download (29.3 KB) - added by plorkyeran 5 months ago.
rtl.bmp download (29.3 KB) - added by plorkyeran 5 months ago.
ltr-rtl.png download (13.0 KB) - added by awi 3 months ago.
LTR & RTL test case
win-ltr.png download (16.1 KB) - added by awi 3 months ago.
Win API and paint DC (LTR layout).
win-rtl.png download (16.0 KB) - added by awi 3 months ago.
Win API and paint DC (RTL layout).
paint-ltr.png download (17.0 KB) - added by awi 3 months ago.
wxPaintDC (LTR layout).
paint-rtl.png download (16.4 KB) - added by awi 3 months ago.
wxPaintDC (RTL layout).
Reduce-window-and-viewport-extent.patch download (2.0 KB) - added by awi 3 months ago.
Reduce the fracion of window and viewport extents using GCD.
Implement-greatest-common-divisor-algorithm.patch download (2.1 KB) - added by awi 3 months ago.
Implememts GCD algorithm.
Reduce-GDI-logical-and-device-extents.patch download (743 bytes) - added by awi 3 months ago.
Reduces GDI extents.
memDC-ltr-rtl.png download (17.2 KB) - added by awi 3 months ago.
wxMemoryDC tests.

Download all attachments as: .zip

Change History (25)

Changed 5 months ago by plorkyeran

Changed 5 months ago by plorkyeran

comment:1 Changed 3 months ago by AW

In 76932:

Fix for drawing check box in the wxPG edit mode when RTL layout direction is set under wxMSW.

Check box isn't drawn correctly in the edit mode under wxMSW due to the problems with RTL handling in wxAutoBufferedPaintDC and wxPaintDC (see #16254).
We need to only draw the image, no text, so we can work around the problem by overriding layout direction to LTR.

comment:2 Changed 3 months ago by awi

I can confirm that when for wxMemoryDC layout direction is set to RTL then drawings are displayed only if origin is set manually (via SetLogicalOrigin). But another strange effect can be also observed. It seems that origin is shifted to the right by several pixels in RTL mode (3 pixels in the test case).
It seems that internal GDI parameters are OK (DC handle, layout flag), but everything looks wrong.

LTR & RTL test case

Test case:

  • samples/minimal/minimal.cpp

    a b MyFrame::MyFrame(const wxString& title) 
    172172    CreateStatusBar(2); 
    173173    SetStatusText("Welcome to wxWidgets!"); 
    174174#endif // wxUSE_STATUSBAR 
     175    wxBitmap bmp1(wxSize(100, 100)); 
     176    { 
     177        wxMemoryDC dc(bmp1); 
     178        dc.SetLayoutDirection(wxLayout_LeftToRight); 
     179        dc.SetBackground(*wxGREEN_BRUSH); 
     180        dc.Clear(); 
     181        dc.DrawText(wxT("+LTR-"), 20, 0); 
     182        dc.SetBrush(*wxBLUE_BRUSH); 
     183        dc.DrawRectangle(10, 20, 20, 10); 
     184        dc.SetBrush(*wxBLACK_BRUSH); 
     185        dc.DrawRectangle(-10, 40, 20, 10); 
     186        dc.SetBrush(*wxYELLOW_BRUSH); 
     187        dc.DrawRectangle(80, 80, 20, 10); 
     188        dc.DrawLine(0,0,99,99); 
     189    } 
     190 
     191    wxBitmap bmp2(wxSize(100, 100)); 
     192    { 
     193        wxMemoryDC dc(bmp2); 
     194        dc.SetLayoutDirection(wxLayout_RightToLeft); 
     195        wxLayoutDirection ld = dc.GetLayoutDirection(); 
     196        dc.SetBackground(*wxGREEN_BRUSH); 
     197        dc.Clear(); 
     198        dc.SetLogicalOrigin(dc.GetSize().GetWidth(), 0); 
     199        dc.DrawText(wxT("*RTL="), 20, 0); 
     200        dc.SetBrush(*wxBLUE_BRUSH); 
     201        dc.DrawRectangle(10, 20, 20, 10); 
     202        dc.SetBrush(*wxBLACK_BRUSH); 
     203        dc.DrawRectangle(-10, 40, 20, 10); 
     204        dc.SetBrush(*wxYELLOW_BRUSH); 
     205        dc.DrawRectangle(80, 80, 20, 10); 
     206        dc.DrawLine(0,0,99,99); 
     207        dc.DrawPoint(102,99); 
     208    } 
     209 
     210    new wxStaticBitmap(this, wxID_ANY, bmp1, wxPoint(10,10)); 
     211    new wxStaticBitmap(this, wxID_ANY, bmp2, wxPoint(115,10)); 
    175212} 

Changed 3 months ago by awi

LTR & RTL test case

comment:3 Changed 3 months ago by vadz

Really strange, I don't see anything wrong here neither. And I could understand if we were off by 1 pixel somewhere (e.g. in the origin setting?), but by 3?

I guess the next thing to do would be test this in a raw Win32 API application to really determine whether this is a wx bug or Windows (mis)feature.

Also, we probably should still call SetLogicalOrigin() in wxDC::SetLayoutDirection(), even if it's not perfect, it's arguably better than not mirroring at all...

comment:4 Changed 3 months ago by plorkyeran

FWIW I'm currently using the following in place of wxAutoBufferedPaintDC, which makes everything I'm using work correctly (I don't think it's off by 3 pixels, but it's plausible that I may have just not noticed):

#ifdef __WXMSW__
class PaintDC : public wxBufferedDC {
        wxPaintDC dc;

public:
        PaintDC(wxWindow *window) : dc(window) {
                dc.SetLayoutDirection(wxLayout_LeftToRight);
                Init(&dc, window->GetClientSize(), 0);
                if (window->GetLayoutDirection() == wxLayout_RightToLeft) {
                        SetLayoutDirection(wxLayout_RightToLeft);
                        SetLogicalOrigin(GetSize().GetWidth(), 0);
                }
        }

        ~PaintDC() {
                SetLayoutDirection(wxLayout_LeftToRight);
                SetLogicalOrigin(0, 0);
                UnMask();
        }

        void Clear() {
                auto origin = GetLogicalOrigin();
                SetLogicalOrigin(0, 0);
                wxBufferedDC::Clear();
                SetLogicalOrigin(origin.x, origin.y);
        }
};
#else
typedef wxAutoBufferedPaintDC PaintDC;
#endif

The switching between LTR and RTL are to make it so that painting to the backing bitmap is done in RTL mode, but painting the bitmap to the screen is done in LTR mode to avoid double-flipping things.

comment:5 Changed 3 months ago by awi

I did quick tests with RTL layout in pure Win API application with standard WM_PAINT handling (using BeginPaint/EndPaint). In this mode (equivalent to wxPaintDC) everything works fine both for LTR and RTL window layout. Paint DC inherits layout flag and output is as expected.
Win API and paint DC (LTR layout).
Win API and paint DC (RTL layout).

In RTL mode, some raw DC paramateres, like window origin, viewport origin observed in this sample were the same as parameters taken from DC encapsulated in wxPaintDC. But some parameters were different: window extent and viewport extent.
For pure Win API, paint DC both extents were equal to 1 (one) both for x and y.
For wxPaintDC both extents are set by default to very large numbers (227-1) both for x and y.
When for testing purposes I assigned 1 (one) to VIEWPORT_EXTENT constant then wxPaintDC started to work fine in RTL mode (as well as wxClientDC).
wxPaintDC (LTR layout).
wxPaintDC (RTL layout).

It seems that the reason of the problem with wxPaintDC and wxClientDC in RTL mode is arithmetic overflow. It is not clearly stated in MSDN but maybe max extent value is the same as max window size (32767)?
I see two possible options for wxPaintDC/wxClientDC:

  1. Set VIEWPORT_EXTENT constant to 32767.
  2. Take advantage of the fact that absolute extent values don't matter in ansisotropic GDI mode (MM_ANISOTROPIC) but only their ratio is important and assign in wxMSWDCImpl::RealizeScaleAndOrigin() their reduced values instead of large unreduced numbers.

Unfortunately, problem with wxMemoryDC is not solved by applying above fixes. In this case the reason must be different (probably something with CreateCompatibleDC).

Test case:

  • samples/minimal/minimal.cpp

    a b  
    6767    // event handlers (these functions should _not_ be virtual) 
    6868    void OnQuit(wxCommandEvent& event); 
    6969    void OnAbout(wxCommandEvent& event); 
     70    void OnPaint( wxPaintEvent& event ); 
    7071 
    7172private: 
    7273    // any class wishing to process wxWidgets events must use this macro 
     
    99100wxBEGIN_EVENT_TABLE(MyFrame, wxFrame) 
    100101    EVT_MENU(Minimal_Quit,  MyFrame::OnQuit) 
    101102    EVT_MENU(Minimal_About, MyFrame::OnAbout) 
     103    EVT_PAINT(MyFrame::OnPaint) 
    102104wxEND_EVENT_TABLE() 
    103105 
    104106// Create a new application object: this macro will allow wxWidgets to create 
     
    143145 
    144146// frame constructor 
    145147MyFrame::MyFrame(const wxString& title) 
    146        : wxFrame(NULL, wxID_ANY, title) 
     148       : wxFrame(NULL, wxID_ANY, title, wxDefaultPosition, wxSize(350,300), 
     149                 wxDEFAULT_FRAME_STYLE|wxFULL_REPAINT_ON_RESIZE) 
    147150{ 
    148151    // set the frame icon 
    149152    SetIcon(wxICON(sample)); 
     153    SetLayoutDirection(wxLayout_RightToLeft); 
    150154 
    151155#if wxUSE_MENUS 
    152156    // create a menu bar 
     
    197201                 "About wxWidgets minimal sample", 
    198202                 wxOK | wxICON_INFORMATION, 
    199203                 this); 
     204} 
     205 
     206void MyFrame::OnPaint( wxPaintEvent& WXUNUSED(event) ) 
     207{ 
     208    wxPaintDC dc(this); 
     209    PrepareDC( dc ); 
     210 
     211    dc.SetBackground(*wxGREEN_BRUSH); 
     212    dc.Clear(); 
     213 
     214    dc.SetPen(*wxBLUE_PEN); 
     215    dc.DrawText("Text ABCDEFGHIKL", 40, 20); 
     216    dc.SetBrush(*wxBLUE_BRUSH); 
     217    dc.DrawRectangle(50,50,30,60); 
     218    dc.DrawLine(10,10,40,40); 
    200219} 

Changed 3 months ago by awi

Win API and paint DC (LTR layout).

Changed 3 months ago by awi

Win API and paint DC (RTL layout).

Changed 3 months ago by awi

wxPaintDC (LTR layout).

Changed 3 months ago by awi

wxPaintDC (RTL layout).

comment:6 Changed 3 months ago by awi

  • Patch set

Attached patch contains the code to reducing the fraction of viewportExtent/windowExtent values calculated in wxMSWDCImpl::RealizeScaleAndOrigin().
This should help to avoid possibility of arithmetic overflow (especially when RTL layout is enabled) when coordinates are translated internally in Win API.
This patch solves issues with wxPaintDC (and family) ans wxClientDC in RTL mode.
attachment:Reduce-window-and-viewport-extent.patch

Changed 3 months ago by awi

Reduce the fracion of window and viewport extents using GCD.

comment:7 Changed 3 months ago by vadz

  • Milestone set to 3.1.0

Wow, great find, I would have never thought that the problems were due to this. Thanks a lot for finding this!

I think you should just go ahead and apply this patch and probably apply it to 3.0 branch too (at least I don't see how could it break anything).

In the trunk I wonder if we shouldn't put wxGCD() in wx/math.h -- it's not something I think wx really should provide, but if we have it anyhow, why not? But this is totally up to you.

Thanks again!

comment:8 Changed 3 months ago by awi

Here are two patches:

  1. To introduce wxGCD function implementing GCD algorithm:

(attachment:Implement-greatest-common-divisor-algorithm.patch)

  1. To reduce GDI logical and device extents:

(attachment:Reduce-GDI-logical-and-device-extents.patch)

My latest tests revealed that this patch solves also issues with wxMemoryDC:
wxMemoryDC tests.

Changed 3 months ago by awi

Implememts GCD algorithm.

Changed 3 months ago by awi

Reduces GDI extents.

Changed 3 months ago by awi

wxMemoryDC tests.

comment:9 Changed 3 months ago by vadz

Thanks again, applying this right now!

@plorkyeran: Could you please retest your code with the latest wx snapshot and let us know if you still have any problems? TIA!

comment:10 Changed 3 months ago by VZ

In 77019:

Add wxGCD() helper function.

It is needed in wxMSW code and it looks like it could be useful to the library
users, so make it public.

See #16254.

comment:11 Changed 3 months ago by VZ

  • Owner set to VZ
  • Resolution set to fixed
  • Status changed from new to closed

In 77020:

Fix drawing on wxDC when using right-to-left layout in wxMSW.

Avoid integer overflow when setting wxDC scale. This affected (i.e. broke)
drawing in RTL but probably not only that.

Closes #16254.

comment:12 Changed 3 months ago by VZ

In 77022:

Fix drawing on wxDC when using right-to-left layout in wxMSW.

Avoid integer overflow when setting wxDC scale. This affected (i.e. broke)
drawing in RTL but probably not only that.

Closes #16254.

comment:13 Changed 3 months ago by plorkyeran

That commit did fix all of my RTL issues that could plausibly be blamed on wx (wxSTC still has a few issues, but Scite has the same issues so it's probably Scintilla's fault). I can't say I would have ever guessed that the problem was int overflow...

comment:14 Changed 3 months ago by AW

In 77025:

Remove temporary fix for drawing check box in the wxPG edit mode with RTL layout

Since the issue is fixed (r77020) this hack is no longer necessary (see #16254).

Note: See TracTickets for help on using tickets.