Date: prev next · Thread: first prev next last
2011 Archives by date, by thread · List index


On Monday 31 of October 2011, Timothy Pearson wrote:
 And I don't remember any reply to my questions, and I don't see anything
changing since then, so they still stand: What are the reasons this is
done as a huge copy&paste s/KDE/TDE/ patch?

Primarily so that we can start hacking on the TDE module without breaking
the KDE3 module, and also if you look at the patch the Qt classnames have
all been altered.  The TDE developers are nearing the end of a yearlong
project to convert TDE to use those class names, and as soon as the final
transition is done the KDE3 module will be completely unbuildable, let
alone usable, on TDE.

 If it's really only class name changes, then it's nothing that a couple of 
typedef's or #define's wouldn't solve.

I have been discussing this with Michael Meeks, and basically if
LibreOffice would rather we hack up the KDE3 module to work with TDE that
is fine, but we will not be able to guarantee that our changes do not
seriously break the module when used under KDE3.  I have serious concerns
about the maintainability of such code, including readability,

 I picked a file at random, vcl/unx/kde/salnativewidgets-kde.cxx, compared it 
with the TDE file from the patch, and if I ignore type name changes, the 
differences boil down to the attached a.patch, which is 2 changes of less 
than 20 lines in total, for a file that is 70k big. Second random file 
picked, just in case, fpicker/source/unx/kde/kdefilepicker.cxx , is pretty 
much identical (again minus the type name changes).

 In other words, to me it looks like it would have been much less work to 
create the TDE version by something like:

foo-tde.cxx:
#define BUILDING_TDE
#define QApplication QTApplication
...
#include "foo-kde.cxx"

foo-kde.cxx:
...
#ifdef BUILDING_TDE // or possibly even better "#if TDE_IS_VERSION( 3,7,0 )"
...
#endif
...

speed of patch acceptance (due to offsite KDE3 tests most likely being
mandatory), and how prone the code will be to accidental breakage.

 If you start taking care of the KDE3/TDE code, you'll more or less end up 
being the maintainer of it. People occassionally do a build fix or similar, I 
rarely do a bugfix when I find the time and/or somebody whines a lot, but 
that's about it I expect.

 I would assume that TDE still maintains some kind of backwards 
compatibility[*], so you should generally know whether your changes are safe 
or whether it's something new that needs an #ifdef (in which case the 
TDE_IS_VERSION check is needed anyway). If something slips through, it can 
get fixed when somebody notices.

 So I don't know the extent of the changes you plan in the code, but from what 
I can see there does not seem to be a good reason to have a special copy of 
the sources for TDE.

[*] I still don't see it said anywhere if TDE maintains any API backwards 
compatibility with KDE3 or not. The configure checks in the patch search for 
KDE3/Qt3 names, so I would assume the answer is yes. If not, then at least 
that part of the patch is definitely wrong.

-- 
 Lubos Lunak
 l.lunak@suse.cz
--- /home/llunak/build/src/libo/vcl/unx/kde/salnativewidgets-kde.cxx    2011-10-31 
13:32:50.598155002 +0100
+++ a.cxx       2011-11-01 17:57:00.640561003 +0100
@@ -392,11 +395,7 @@ WidgetPainter::WidgetPainter( void )
       m_pToolBarVert( NULL ),
       m_pToolButton( NULL ),
       m_pMenuBar( NULL ),
-      m_nMenuBarEnabledItem( 0 ),
-      m_nMenuBarDisabledItem( 0 ),
       m_pPopupMenu( NULL ),
-      m_nPopupMenuEnabledItem( 0 ),
-      m_nPopupMenuDisabledItem( 0 ),
       m_pProgressBar( NULL )
 {
 }
@@ -1923,7 +1922,17 @@ void KDESalFrame::UpdateSettings( AllSet
     aStyleSettings.SetFaceColor( aBack );
     aStyleSettings.SetInactiveTabColor( aBack );
     aStyleSettings.SetDialogColor( aBack );
-    aStyleSettings.SetCheckedColorSpecialCase( );
+    if( aBack == COL_LIGHTGRAY )
+        aStyleSettings.SetCheckedColor( Color( 0xCC, 0xCC, 0xCC ) );
+    else
+    {
+        Color aColor2 = aStyleSettings.GetLightColor();
+        aStyleSettings.
+            SetCheckedColor( Color( (BYTE)(((USHORT)aBack.GetRed()+(USHORT)aColor2.GetRed())/2),
+                        (BYTE)(((USHORT)aBack.GetGreen()+(USHORT)aColor2.GetGreen())/2),
+                        (BYTE)(((USHORT)aBack.GetBlue()+(USHORT)aColor2.GetBlue())/2)
+                        ) );
+    }
 
     // Selection
     aStyleSettings.SetHighlightColor( toColor( qColorGroup.highlight() ) );

Context


Privacy Policy | Impressum (Legal Info) | Copyright information: Unless otherwise specified, all text and images on this website are licensed under the Creative Commons Attribution-Share Alike 3.0 License. This does not include the source code of LibreOffice, which is licensed under the Mozilla Public License (MPLv2). "LibreOffice" and "The Document Foundation" are registered trademarks of their corresponding registered owners or are in actual use as trademarks in one or more countries. Their respective logos and icons are also subject to international copyright laws. Use thereof is explained in our trademark policy.