Re: [Libreoffice] Trinity Desktop Environment (TDE) desktop integraton for LibreOffice

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() ) );