[Libreoffice] [REVIEW] fix for fdo#36690

Hi there,

The attached patch fixes

https://bugs.freedesktop.org/show_bug.cgi?id=36690

and I would like to push this to the -3-4 branch and possibly to the
-3-4-0 branch as well.  The patch is against vcl.

Calc races during print preview when the sheet contains buttons with
colors.  The reason is as follows:

1. Print preview, which is the output device, tries to paint its own
window which draws the buttons.  (ScPreview implements the print preview
window for Calc.)
2. PushButton::Draw() updates the style setting of the output device in
order to change the face color by calling SetSettings() of the output
device.
3. SetSettings() is virtual (!), and Window::SetSettings() receives the
call instead of OutputDevice::SetSettings().
4. Window::SetSettings() calls DataChanged() virtual method to broadcast
the setting change.
5. ScPreView::DataChanged() receives this, sees the style has changed,
and tries to re-paint the whole thing.
6. The cycle continues. :-(

When the PushButton::Draw changes the style setting, I'm pretty sure it
never intends to broadcast the change to anyone, yet it does because
SetSettings is virtual.  So what my patch does is to allow it to simply
change the style setting without broadcasting it, and that's enough to
end this infinite re-paint cycle.

As an aside, it really bugs me to have a simple accessor method being
virtual, but that's another story...

Reviews appreciated.

Kohei

-- 
Kohei Yoshida, LibreOffice hacker, Calc
<kyoshida@novell.com>

From 530449e89eb76b536aeb1a49582cee2d882b0075 Mon Sep 17 00:00:00 2001
From: Kohei Yoshida <kyoshida@novell.com>
Date: Fri, 20 May 2011 00:15:01 -0400
Subject: [PATCH] fdo#36690: Don't broadcast setting changes during painting of button.

Calling SetSettings() when the output device is Window causes it to
broadcast data change.  PushButton::Draw() in fact calls this method
when the button contains colors, which ends up broadcasting its change
via Window::DataChanged call.  But depending on the output device
this DataChanged call may end up painting it again, which basically
causes a recursive loop.

The solution is to provide a way to only change the settings without
doing anything else, and use that when settings need to be changed
during the painting of a control (like PushButton).
---
 vcl/inc/vcl/outdev.hxx        |    7 +++++++
 vcl/source/control/button.cxx |    2 +-
 vcl/source/gdi/outdev.cxx     |    7 ++++++-
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/vcl/inc/vcl/outdev.hxx b/vcl/inc/vcl/outdev.hxx
index b624cd9..f4f7475 100644
--- a/vcl/inc/vcl/outdev.hxx
+++ b/vcl/inc/vcl/outdev.hxx
@@ -917,6 +917,13 @@ public:
     void                SetTextAlign( TextAlign eAlign );
     TextAlign           GetTextAlign() const { return maFont.GetAlign(); }
 
+    /**
+     * Unlike SetSettings() which is virtual & may do more than just updating
+     * the settings, this method only sets the new settings and do nothing
+     * extra.  This method should be called instead of SetSettings() when
+     * changing the settings during painting of controls.
+     */
+    void                SetSettingsOnly( const AllSettings& rSettings );
     virtual void        SetSettings( const AllSettings& rSettings );
     const AllSettings&  GetSettings() const { return maSettings; }
 
diff --git a/vcl/source/control/button.cxx b/vcl/source/control/button.cxx
index c77d14a..b0ce14f 100644
--- a/vcl/source/control/button.cxx
+++ b/vcl/source/control/button.cxx
@@ -1522,7 +1522,7 @@ void PushButton::Draw( OutputDevice* pDev, const Point& rPos, const Size& 
rSize,
         else
             aStyleSettings.SetFaceColor( GetSettings().GetStyleSettings().GetFaceColor() );
         aSettings.SetStyleSettings( aStyleSettings );
-        pDev->SetSettings( aSettings );
+        pDev->SetSettingsOnly( aSettings );
     }
     pDev->SetTextFillColor();
 
diff --git a/vcl/source/gdi/outdev.cxx b/vcl/source/gdi/outdev.cxx
index da9ddc3..96a5e1d 100644
--- a/vcl/source/gdi/outdev.cxx
+++ b/vcl/source/gdi/outdev.cxx
@@ -2558,7 +2558,7 @@ void OutputDevice::EnableOutput( sal_Bool bEnable )
 
 // -----------------------------------------------------------------------
 
-void OutputDevice::SetSettings( const AllSettings& rSettings )
+void OutputDevice::SetSettingsOnly( const AllSettings& rSettings )
 {
     maSettings = rSettings;
 
@@ -2566,6 +2566,11 @@ void OutputDevice::SetSettings( const AllSettings& rSettings )
         mpAlphaVDev->SetSettings( rSettings );
 }
 
+void OutputDevice::SetSettings( const AllSettings& rSettings )
+{
+    SetSettingsOnly(rSettings);
+}
+
 // -----------------------------------------------------------------------
 
 sal_uInt16 OutputDevice::GetBitCount() const
-- 
1.7.3.4