Re: [Libreoffice] [PATCH] Fix rendering of GTK combobox in RTL

Oops, the patches :)

On Fri, Dec 02, 2011 at 11:43:37PM +0200, Khaled Hosny wrote:
> On Wed, Nov 30, 2011 at 01:12:08PM +0000, Michael Meeks wrote:
> > Hi Khaled,
> >
> >     Thanks for your patch ! :-)
> >
> > On Wed, 2011-11-30 at 09:11 +0200, Khaled Hosny wrote:
> > > Here is a little patch that fixes a rendering buglet that annoyed me
> > > since ever. Native GTK applications swap the position of the button and
> > > editing area of comboboxes in RTL and themese expect that, but LO does
> > > not.
> >
> >     :-)
> >
> > > The same issue happens with spin buttons and other similar widget, I'm
> > > just making sure I'm doing it right before sending other patches.
> >
> >     Sure - it'd be great to have this all fixed up.
> >
> > > Also I'm not sure how GTK3 relates to this (is it affected/need to be
> > > fixed separately etc.) since I can't build with it here.
> >
> >     Sadly it'd be need to be fixed separately, that is one bit of code that
> > cannot be shared; but gtk3 will be highly experimental for 3.5 so ...
> > don't worry :-)
>
> OK, someone will have to remember fixing them when GTK3 is the default :)
>
> > > BTW, Application::GetSettings().GetLayoutRTL() is now used in several
> > > places (and may be more will come), would it be more rideable/save a few
> > > microsocends to have a 
> >
> >     Ho hum; that method is a bit distressing, I'd hope that it would cache
> > the value itself, and get notifications when / if it changes.
> >
> > > bool isLayoutRTL = Application::GetSettings().GetLayoutRTL()
> >
> >     Updating that can be a pain if/as/when the user tweaks it in the
> > settings; clearly if we can do this at the top of the method, or
> > (better) accelerate the GetLayoutRTL impl. itself ;-)
> >
> >     Having said that we hsould prolly be using:
> >
> > inc/vcl/outdev.hxx:    sal_Bool                IsRTLEnabled() const
> > { return mbEnableRTL; }
> >
> >     on the OutputDevice if we have one around to improve efficiency.
>
> That is too complex for me :p so I'll keep doing what seems to work.
>
> On a, not so, different note, I found that setting LibreOffice language
> to one different from system language, e.g. LO in Arabic but LANGUAGE is
> set to "en" and vice versa, GTK will be using the directionally of the
> system language, so stuff that get reversed in RTL will not, while it is
> reversed in LibreOffice resulting in the reverse of the bugs being
> fixed. GTK takes the directionally from its translation catalog, so it
> seems the gettext domain used by GTK is the one from system language
> regardless of the actual LibreOffice language, may be there is a way to
> fix this?
>
> >     Another minor nit; it looks like the combo doesn't join up very
> > elegantly with the drop-down in some themes [ cf. the attached image ]
> > when in LTR mode - any thoughts on that ?
>
> No idea, those control are broken in different way in my machine. I'm
> attaching two new patches now really fixing compoboxes (in your
> screenshot, what I fixed earlier with the one with entry box) and spin
> buttons.
>
> This is really a huge improvements in the RTL UI, and case I didn't say
> it before, LibreOffice is really the best thing that happened to OOo
> since it was leashed upon the world (and I like the name, BTW).
>
> Regards,
>  Khaled
From bc3e8081c2b679518735ac4013cedde0c96e9db0 Mon Sep 17 00:00:00 2001
From: Khaled Hosny <khaledhosny@eglug.org>
Date: Fri, 2 Dec 2011 14:10:32 +0200
Subject: [PATCH 1/2] gtk: fix RTL spin button rendering

---
 vcl/unx/gtk/gdi/salnativewidgets-gtk.cxx |   22 +++++++++++++++++-----
 1 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/vcl/unx/gtk/gdi/salnativewidgets-gtk.cxx b/vcl/unx/gtk/gdi/salnativewidgets-gtk.cxx
index 6afba20..cdd388d 100644
--- a/vcl/unx/gtk/gdi/salnativewidgets-gtk.cxx
+++ b/vcl/unx/gtk/gdi/salnativewidgets-gtk.cxx
@@ -2026,8 +2026,11 @@ sal_Bool GtkSalGraphics::NWPaintGTKSpinBox( ControlType nType, ControlPart 
nPart
     {
         // Draw an edit field for SpinBoxes and ComboBoxes
         Rectangle aEditBoxRect( pixmapRect );
-        aEditBoxRect.SetSize( Size( upBtnRect.Left() - pixmapRect.Left(), aEditBoxRect.GetHeight() 
) );
-        aEditBoxRect.setX( 0 );
+        aEditBoxRect.SetSize( Size( pixmapRect.GetWidth() - upBtnRect.GetWidth(), 
aEditBoxRect.GetHeight() ) );
+        if( Application::GetSettings().GetLayoutRTL() )
+            aEditBoxRect.setX( upBtnRect.GetWidth() );
+        else
+            aEditBoxRect.setX( 0 );
         aEditBoxRect.setY( 0 );
 
         NWPaintOneEditBox( m_nScreen, pixmap, NULL, nType, nPart, aEditBoxRect, nState, aValue, 
rCaption );
@@ -2080,7 +2083,10 @@ static Rectangle NWGetSpinButtonRect( int nScreen,
     buttonSize -= buttonSize % 2 - 1; /* force odd */
     buttonRect.SetSize( Size( buttonSize + 2 * 
gWidgetData[nScreen].gSpinButtonWidget->style->xthickness,
                               buttonRect.GetHeight() ) );
-    buttonRect.setX( aAreaRect.Left() + (aAreaRect.GetWidth() - buttonRect.GetWidth()) );
+    if( Application::GetSettings().GetLayoutRTL() )
+        buttonRect.setX( aAreaRect.Left() );
+    else
+        buttonRect.setX( aAreaRect.Left() + (aAreaRect.GetWidth() - buttonRect.GetWidth()) );
     if ( nPart == PART_BUTTON_UP )
     {
         buttonRect.setY( aAreaRect.Top() );
@@ -2093,8 +2099,14 @@ static Rectangle NWGetSpinButtonRect( int nScreen,
     }
     else
     {
-        buttonRect.Right()  = buttonRect.Left()-1;
-        buttonRect.Left()   = aAreaRect.Left();
+        if( Application::GetSettings().GetLayoutRTL() ) {
+            buttonRect.Left()   = buttonRect.Right()+1;
+            buttonRect.Right()  = aAreaRect.Right();
+        } else {
+            printf("%ld\n", buttonRect.GetWidth());
+            buttonRect.Right()  = buttonRect.Left()-1;
+            buttonRect.Left()   = aAreaRect.Left();
+        }
         buttonRect.Top()    = aAreaRect.Top();
         buttonRect.Bottom() = aAreaRect.Bottom();
     }
-- 
1.7.0.4

From c44cc47832ec12e5d215ea9995dddb0e5e54ccfc Mon Sep 17 00:00:00 2001
From: Khaled Hosny <khaledhosny@eglug.org>
Date: Fri, 2 Dec 2011 22:27:05 +0200
Subject: [PATCH 2/2] gtk: fix RTL list box rendering

---
 vcl/unx/gtk/gdi/salnativewidgets-gtk.cxx |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/vcl/unx/gtk/gdi/salnativewidgets-gtk.cxx b/vcl/unx/gtk/gdi/salnativewidgets-gtk.cxx
index cdd388d..f9e5906 100644
--- a/vcl/unx/gtk/gdi/salnativewidgets-gtk.cxx
+++ b/vcl/unx/gtk/gdi/salnativewidgets-gtk.cxx
@@ -3240,7 +3240,10 @@ static Rectangle NWGetListBoxButtonRect( int nScreen,
 
         case PART_SUB_EDIT:
             aPartSize.Width() = aAreaRect.GetWidth() - nButtonAreaWidth - xthickness;
-            aPartPos.X() = aAreaRect.Left() + xthickness;
+            if( Application::GetSettings().GetLayoutRTL() )
+                aPartPos.X() = aAreaRect.Left() + nButtonAreaWidth;
+            else
+                aPartPos.X() = aAreaRect.Left() + xthickness;
             break;
 
         default:
@@ -3274,6 +3277,7 @@ static Rectangle NWGetListBoxIndicatorRect( int nScreen,
     gint            width = 13;    // GTK+ default
     gint            height = 13;    // GTK+ default
     gint            right = 5;    // GTK+ default
+    gint            x;
 
     NWEnsureGTKOptionMenu( nScreen );
 
@@ -3291,8 +3295,11 @@ static Rectangle NWGetListBoxIndicatorRect( int nScreen,
         right = pIndicatorSpacing->right;
 
     aIndicatorRect.SetSize( Size( width, height ) );
-    aIndicatorRect.SetPos( Point( aAreaRect.Left() + aAreaRect.GetWidth() - width - right - 
gWidgetData[nScreen].gOptionMenuWidget->style->xthickness,
-                                  aAreaRect.Top() + ((aAreaRect.GetHeight() - height) / 2) ) );
+    if( Application::GetSettings().GetLayoutRTL() )
+        x = aAreaRect.Left() + right;
+    else
+        x = aAreaRect.Left() + aAreaRect.GetWidth() - width - right - 
gWidgetData[nScreen].gOptionMenuWidget->style->xthickness;
+    aIndicatorRect.SetPos( Point( x, aAreaRect.Top() + ((aAreaRect.GetHeight() - height) / 2) ) );
 
     // If height is odd, move the indicator down 1 pixel
     if ( aIndicatorRect.GetHeight() % 2 )
-- 
1.7.0.4