Re: [Libreoffice] Libreoffice Changes

On Mon, 2010-10-11 at 16:16 -0700, Alan Du wrote:
> Sorry about my last whacked email.  I've removed a lot of
> the //CHINA001 comments with these.
>
> The filename is the directory the changes were made in.

Hey Alan,

Thanks for your patch submission.  I've pushed most of it except for the
writer one.  My reasons for not pushing your writer patch is given
below.

Also...

@@ -217,13 +217,13 @@ void SvxShadowTabPage::ActivatePage( const SfxItemSet& rSet )
..
-    //add CHINA001 end
-    if( nDlgType == 0 ) //CHINA001 // Flaechen-Dialogif( *pDlgType == 0 ) // Flaechen-Dialog
-    {
+
+    if( nDlgType == 0 )
+
         if( pColorTab )
         {
             // ColorTable

You removed the opening brace below the if statement, which broke the
build.  Not a big issue but let's be careful there.  Additionally...

@@ -1561,12 +1561,7 @@ IMPL_LINK( SvxSearchDialog, CommandHdl_Impl, Button *, pBtn )
     }
     else if ( pBtn == &aSimilarityBtn )
     {
-//CHINA001             SvxSearchSimilarityDialog* pDlg =
-//CHINA001                     new SvxSearchSimilarityDialog( this,
-//CHINA001                                                                                
pSearchItem->IsLEVRelaxed(),
-//CHINA001                                                                                
pSearchItem->GetLEVOther(),
-//CHINA001                                                                                
pSearchItem->GetLEVShorter(),
-//CHINA001                                                                                
pSearchItem->GetLEVLonger() );
+                                                                          
pSearchItem->GetLEVLonger() );
         SvxAbstractDialogFactory* pFact = SvxAbstractDialogFactory::Create();
         if(pFact)

This hunk also needs a little after-care as well.  I assume the line
added there was not intentional.

Now, I didn't add your patch against the writer repo because of two
reasons.  First one is that you removed references to OOo's bug tracker.
Numbers like #i24453# refers to OOo's bug tracker, which we'd like to
keep.  When you encounter a comment with OOo's bug reference ID, let's
not remove it.  However, numbers like #148498# (without a preceding 'i')
are OK to remove, since they refer to Oracle's internal database which
we have no access to.

The second one is that, your patch removed lots of

-        // --> OD 2008-02-11 #newlistlevelattrs#

but you weren't consistent with the removal of the corresponding

-        // <--

Let's be consistent here.  Lastly (sorry there are 3 reasons),

-    // --> OD 2005-02-04 #118544# - If cell is the first one to be merged,
+
     // a new merge group has to be provided.
     // E.g., it could be that a cell is the first one to be merged, but no
     // new merge group is provided, because the potential other cell to be merged

You removed the first line of a multi-line comments.  This is a no-go.

So, can you review your writer patch and re-submit a new one?

Thanks a lot!

Kohei

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