fix docx redline import (bnc#821804)

This is a squash of commits b6969634..fd26de3b.

Change-Id: I8984001d7f85c00eb9b9943b25b8abb6e2ee28d3
Reviewed-on: https://gerrit.libreoffice.org/11789
Reviewed-by: Miklos Vajna <vmiklos@collabora.co.uk>
Tested-by: Miklos Vajna <vmiklos@collabora.co.uk>

5 files changed, 107 insertions, 93 deletions

diff --git a/writerfilter/source/dmapper/DomainMapper.cxx b/writerfilter/source/dmapper/DomainMapper.cxx
index d97f4715417e..90ae6e94ae69 100644
--- a/writerfilter/source/dmapper/DomainMapper.cxx
+++ b/writerfilter/source/dmapper/DomainMapper.cxx
@@ -2169,9 +2169,10 @@ void DomainMapper::sprmWithProps( Sprm& rSprm, PropertyMapPtr rContext )
     }
     break;
     case NS_ooxml::LN_endtrackchange:
-        m_pImpl->RemoveCurrentRedline( );
+        m_pImpl->RemoveTopRedline();
     break;
     case NS_ooxml::LN_CT_RPrChange_rPr:
+    {
         // Push all the current 'Character' properties to the stack, so that we don't store them
         // as 'tracked changes' by mistake
         m_pImpl->PushProperties(CONTEXT_CHARACTER);
@@ -2179,19 +2180,19 @@ void DomainMapper::sprmWithProps( Sprm& rSprm, PropertyMapPtr rContext )
         // Resolve all the properties that are under the 'rPrChange'->'rPr' XML node
         resolveSprmProps(*this, rSprm );
 
-        if (m_pImpl->GetTopContext())
-        {
-            // Get all the properties that were processed in the 'rPrChange'->'rPr' XML node
-            uno::Sequence< beans::PropertyValue > currentRedlineRevertProperties = m_pImpl->GetTopContext()->GetPropertyValues();
-
-            // Store these properties in the current redline object
-            m_pImpl->SetCurrentRedlineRevertProperties( currentRedlineRevertProperties );
-        }
+        // Get all the properties that were processed in the 'rPrChange'->'rPr' XML node
+        uno::Sequence< beans::PropertyValue > currentRedlineRevertProperties = m_pImpl->GetTopContext()->GetPropertyValues();
 
         // Pop back out the character properties that were on the run
         m_pImpl->PopProperties(CONTEXT_CHARACTER);
+
+        // Store these properties in the current redline object (do it after the PopProperties() above, since
+        // otherwise it'd be stored in the content dropped there).
+        m_pImpl->SetCurrentRedlineRevertProperties( currentRedlineRevertProperties );
+    }
     break;
     case NS_ooxml::LN_CT_PPrChange_pPr:
+    {
         // Push all the current 'Paragraph' properties to the stack, so that we don't store them
         // as 'tracked changes' by mistake
         m_pImpl->PushProperties(CONTEXT_PARAGRAPH);
@@ -2199,17 +2200,16 @@ void DomainMapper::sprmWithProps( Sprm& rSprm, PropertyMapPtr rContext )
         // Resolve all the properties that are under the 'pPrChange'->'pPr' XML node
         resolveSprmProps(*this, rSprm );
 
-        if (m_pImpl->GetTopContext())
-        {
-            // Get all the properties that were processed in the 'pPrChange'->'pPr' XML node
-            uno::Sequence< beans::PropertyValue > currentRedlineRevertProperties = m_pImpl->GetTopContext()->GetPropertyValues();
-
-            // Store these properties in the current redline object
-            m_pImpl->SetCurrentRedlineRevertProperties( currentRedlineRevertProperties );
-        }
+        // Get all the properties that were processed in the 'pPrChange'->'pPr' XML node
+        uno::Sequence< beans::PropertyValue > currentRedlineRevertProperties = m_pImpl->GetTopContext()->GetPropertyValues();
 
         // Pop back out the character properties that were on the run
         m_pImpl->PopProperties(CONTEXT_PARAGRAPH);
+
+        // Store these properties in the current redline object (do it after the PopProperties() above, since
+        // otherwise it'd be stored in the content dropped there).
+        m_pImpl->SetCurrentRedlineRevertProperties( currentRedlineRevertProperties );
+    }
     break;
     case NS_ooxml::LN_object:
     {
@@ -3288,7 +3288,7 @@ void DomainMapper::HandleRedline( Sprm& rSprm )
 {
     sal_uInt32 nSprmId = rSprm.getId();
 
-    m_pImpl->AddNewRedline( );
+    m_pImpl->AddNewRedline( nSprmId );
 
     if (nSprmId == NS_ooxml::LN_CT_PPr_pPrChange)
     {
@@ -3328,6 +3328,7 @@ void DomainMapper::HandleRedline( Sprm& rSprm )
         default: OSL_FAIL( "redline token other than mod, ins, del or table row" ); break;
     }
     m_pImpl->EndParaMarkerChange( );
+    m_pImpl->SetCurrentRedlineIsRead();
 }
 
 } //namespace dmapper
diff --git a/writerfilter/source/dmapper/DomainMapper_Impl.cxx b/writerfilter/source/dmapper/DomainMapper_Impl.cxx
index 23b976bee9ee..ffa4bae80dfd 100644
--- a/writerfilter/source/dmapper/DomainMapper_Impl.cxx
+++ b/writerfilter/source/dmapper/DomainMapper_Impl.cxx
@@ -1625,7 +1625,7 @@ void DomainMapper_Impl::PushFootOrEndnote( bool bIsFootnote )
     }
 }
 
-void DomainMapper_Impl::CreateRedline( uno::Reference< text::XTextRange > xRange, RedlineParamsPtr& pRedline )
+void DomainMapper_Impl::CreateRedline( uno::Reference< text::XTextRange > xRange, RedlineParamsPtr pRedline )
 {
     if ( pRedline.get( ) )
     {
@@ -1679,21 +1679,25 @@ void DomainMapper_Impl::CheckParaMarkerRedline( uno::Reference< text::XTextRange
 
 void DomainMapper_Impl::CheckRedline( uno::Reference< text::XTextRange > xRange )
 {
-    vector<RedlineParamsPtr>::iterator pIt = m_aRedlines.top().begin( );
-    vector< RedlineParamsPtr > aCleaned;
+    // Writer core "officially" does not like overlapping redlines, and its UNO interface is stupid enough
+    // to not prevent that. However, in practice in fact everything appears to work fine (except for the debug warnings
+    // about redline table corruption, which may possibly be harmless in reality). So leave this as it is, since this
+    // is a better representation of how the changes happened. If this will ever become a problem, overlapping redlines
+    // will need to be merged into one, just like doing the changes in the UI does, which will lose some information
+    // (and so if that happens, it may be better to fix Writer).
+    // Create the redlines here from lowest (formats) to highest (inserts/removals) priority, since the last one is
+    // what Writer presents graphically, so this will show deletes as deleted text and not as just formatted text being there.
+    if( GetTopContextOfType(CONTEXT_PARAGRAPH) != NULL )
+        for( std::vector<RedlineParamsPtr>::const_iterator it = GetTopContextOfType(CONTEXT_PARAGRAPH)->Redlines().begin();
+             it != GetTopContextOfType(CONTEXT_PARAGRAPH)->Redlines().end(); ++it )
+            CreateRedline( xRange, *it );
+    if( GetTopContextOfType(CONTEXT_CHARACTER) != NULL )
+        for( std::vector<RedlineParamsPtr>::const_iterator it = GetTopContextOfType(CONTEXT_CHARACTER)->Redlines().begin();
+             it != GetTopContextOfType(CONTEXT_CHARACTER)->Redlines().end(); ++it )
+            CreateRedline( xRange, *it );
+    std::vector<RedlineParamsPtr>::iterator pIt = m_aRedlines.top().begin( );
     for (; pIt != m_aRedlines.top().end( ); ++pIt )
-    {
         CreateRedline( xRange, *pIt );
-
-        // Adding the non-mod redlines to the temporary vector
-        if ( pIt->get( ) )
-        {
-            if (((*pIt)->m_nToken & 0xffff) != OOXML_mod && ((*pIt)->m_nToken & 0xffff) != OOXML_ParagraphFormat)
-                aCleaned.push_back(*pIt);
-        }
-    }
-
-    m_aRedlines.top().swap( aCleaned );
 }
 
 void DomainMapper_Impl::StartParaMarkerChange( )
@@ -1704,6 +1708,7 @@ void DomainMapper_Impl::StartParaMarkerChange( )
 void DomainMapper_Impl::EndParaMarkerChange( )
 {
     m_bIsParaMarkerChange = false;
+    m_currentRedline.reset();
 }
 
 
@@ -4495,36 +4500,43 @@ bool DomainMapper_Impl::ExecuteFrameConversion()
     return bRet;
 }
 
-void DomainMapper_Impl::AddNewRedline(  )
+void DomainMapper_Impl::AddNewRedline( sal_uInt32 sprmId )
 {
     RedlineParamsPtr pNew( new RedlineParams );
     pNew->m_nToken = OOXML_mod;
     if ( !m_bIsParaMarkerChange )
     {
-        m_aRedlines.top().push_back( pNew );
+        // <w:rPrChange> applies to the whole <w:r>, <w:pPrChange> applies to the whole <w:p>,
+        // so keep those two in CONTEXT_CHARACTERS and CONTEXT_PARAGRAPH, which will take
+        // care of their scope (i.e. when they should be used and discarded).
+        // Let's keep the rest the same way they used to be handled (explictly dropped
+        // from a global stack by endtrackchange), but quite possibly they should not be handled
+        // that way either (I don't know).
+        if( sprmId == NS_ooxml::LN_EG_RPrContent_rPrChange )
+            GetTopContextOfType( CONTEXT_CHARACTER )->Redlines().push_back( pNew );
+        else if( sprmId == NS_ooxml::LN_CT_PPr_pPrChange )
+            GetTopContextOfType( CONTEXT_PARAGRAPH )->Redlines().push_back( pNew );
+        else
+            m_aRedlines.top().push_back( pNew );
     }
     else
     {
-        m_pParaMarkerRedline.swap( pNew );
+        m_pParaMarkerRedline = pNew;
     }
+    // Newly read data will go into this redline.
+    m_currentRedline = pNew;
 }
 
-RedlineParamsPtr DomainMapper_Impl::GetTopRedline(  )
+void DomainMapper_Impl::SetCurrentRedlineIsRead()
 {
-    RedlineParamsPtr pResult;
-    if ( !m_bIsParaMarkerChange && m_aRedlines.top().size(  ) > 0 )
-        pResult = m_aRedlines.top().back(  );
-    else if ( m_bIsParaMarkerChange )
-        pResult = m_pParaMarkerRedline;
-    return pResult;
+    m_currentRedline.reset();
 }
 
 sal_Int32 DomainMapper_Impl::GetCurrentRedlineToken(  )
 {
     sal_Int32 nToken = 0;
-    RedlineParamsPtr pCurrent( GetTopRedline(  ) );
-    if ( pCurrent.get(  ) )
-        nToken = pCurrent->m_nToken;
+    assert( m_currentRedline.get());
+    nToken = m_currentRedline->m_nToken;
     return nToken;
 }
 
@@ -4532,9 +4544,8 @@ void DomainMapper_Impl::SetCurrentRedlineAuthor( const OUString& sAuthor )
 {
     if (!m_xAnnotationField.is())
     {
-        RedlineParamsPtr pCurrent( GetTopRedline(  ) );
-        if ( pCurrent.get(  ) )
-            pCurrent->m_sAuthor = sAuthor;
+        assert( m_currentRedline.get());
+        m_currentRedline->m_sAuthor = sAuthor;
     }
     else
         m_xAnnotationField->setPropertyValue("Author", uno::makeAny(sAuthor));
@@ -4550,9 +4561,8 @@ void DomainMapper_Impl::SetCurrentRedlineDate( const OUString& sDate )
 {
     if (!m_xAnnotationField.is())
     {
-        RedlineParamsPtr pCurrent( GetTopRedline(  ) );
-        if ( pCurrent.get(  ) )
-            pCurrent->m_sDate = sDate;
+        assert( m_currentRedline.get());
+        m_currentRedline->m_sDate = sDate;
     }
     else
         m_xAnnotationField->setPropertyValue("DateTimeValue", uno::makeAny(ConversionHelper::ConvertDateStringToDateTime(sDate)));
@@ -4566,46 +4576,46 @@ void DomainMapper_Impl::SetCurrentRedlineId( sal_Int32 sId )
     }
     else
     {
-        RedlineParamsPtr pCurrent( GetTopRedline(  ) );
-        if ( pCurrent.get(  ) )
-            pCurrent->m_nId = sId;
+        // This should be an assert, but somebody had the smart idea to reuse this function also for comments and whatnot,
+        // and in some cases the id is actually not handled, which may be in fact a bug.
+        SAL_WARN( "writerfilter", !m_currentRedline.get());
+        if( m_currentRedline.get())
+            m_currentRedline->m_nId = sId;
     }
 }
 
 void DomainMapper_Impl::SetCurrentRedlineToken( sal_Int32 nToken )
 {
-    RedlineParamsPtr pCurrent( GetTopRedline(  ) );
-    if ( pCurrent.get(  ) )
-        pCurrent->m_nToken = nToken;
+    assert( m_currentRedline.get());
+    m_currentRedline->m_nToken = nToken;
 }
 
 void DomainMapper_Impl::SetCurrentRedlineRevertProperties( const uno::Sequence<beans::PropertyValue>& aProperties )
 {
-    RedlineParamsPtr pCurrent( GetTopRedline(  ) );
-    if ( pCurrent.get(  ) )
-        pCurrent->m_aRevertProperties = aProperties;
+    assert( m_currentRedline.get());
+    m_currentRedline->m_aRevertProperties = aProperties;
 }
 
 
-void DomainMapper_Impl::RemoveCurrentRedline( )
+// This removes only the last redline stored here, those stored in contexts are automatically removed when
+// the context is destroyed.
+void DomainMapper_Impl::RemoveTopRedline( )
 {
-    if ( m_aRedlines.top().size( ) > 0 )
-    {
-        m_aRedlines.top().pop_back( );
-    }
+    assert( m_aRedlines.top().size( ) > 0 );
+    m_aRedlines.top().pop_back( );
+    m_currentRedline.reset();
 }
 
 void DomainMapper_Impl::ResetParaMarkerRedline( )
 {
     if ( m_pParaMarkerRedline.get( ) )
     {
-        RedlineParamsPtr pEmpty;
-        m_pParaMarkerRedline.swap( pEmpty );
+        m_pParaMarkerRedline.reset();
+        m_currentRedline.reset();
     }
 }
 
 
-
 void DomainMapper_Impl::ApplySettingsTable()
 {
     if (m_pSettingsTable && m_xTextFactory.is())
diff --git a/writerfilter/source/dmapper/DomainMapper_Impl.hxx b/writerfilter/source/dmapper/DomainMapper_Impl.hxx
index 3f6212978313..e60eab466fe9 100644
--- a/writerfilter/source/dmapper/DomainMapper_Impl.hxx
+++ b/writerfilter/source/dmapper/DomainMapper_Impl.hxx
@@ -243,20 +243,6 @@ struct AnnotationPosition
 };
 typedef boost::unordered_map< sal_Int32, AnnotationPosition > AnnotationPositions_t;
 
-struct RedlineParams
-{
-    OUString m_sAuthor;
-    OUString m_sDate;
-    sal_Int32       m_nId;
-    sal_Int32       m_nToken;
-
-    /// This can hold properties of runs that had formatted 'track changes' properties
-    css::uno::Sequence<css::beans::PropertyValue> m_aRevertProperties;
-};
-typedef boost::shared_ptr< RedlineParams > RedlineParamsPtr;
-
-
-
 struct LineNumberSettings
 {
     bool        bIsOn;
@@ -377,6 +363,8 @@ private:
 
     // Redline stack
     std::stack< std::vector< RedlineParamsPtr > > m_aRedlines;
+    // The redline currently read, may be also stored by a context instead of m_aRedlines.
+    RedlineParamsPtr                m_currentRedline;
     RedlineParamsPtr                m_pParaMarkerRedline;
     bool                            m_bIsParaMarkerChange;
 
@@ -449,7 +437,7 @@ public:
     }
     void SetDocumentSettingsProperty( const OUString& rPropName, const css::uno::Any& rValue );
 
-    void CreateRedline( ::com::sun::star::uno::Reference< ::com::sun::star::text::XTextRange > xRange, RedlineParamsPtr& pRedline  );
+    void CreateRedline( ::com::sun::star::uno::Reference< ::com::sun::star::text::XTextRange > xRange, RedlineParamsPtr pRedline  );
 
     void CheckParaMarkerRedline( ::com::sun::star::uno::Reference< ::com::sun::star::text::XTextRange > xRange );
 
@@ -704,9 +692,7 @@ public:
         );
     bool ExecuteFrameConversion();
 
-    void AddNewRedline( );
-
-    RedlineParamsPtr GetTopRedline( );
+    void AddNewRedline( sal_uInt32 sprmId );
 
     sal_Int32 GetCurrentRedlineToken( );
     void SetCurrentRedlineAuthor( const OUString& sAuthor );
@@ -714,7 +700,8 @@ public:
     void SetCurrentRedlineId( sal_Int32 nId );
     void SetCurrentRedlineToken( sal_Int32 nToken );
     void SetCurrentRedlineRevertProperties( const css::uno::Sequence<css::beans::PropertyValue>& aProperties );
-    void RemoveCurrentRedline( );
+    void SetCurrentRedlineIsRead();
+    void RemoveTopRedline( );
     void ResetParaMarkerRedline( );
     void SetCurrentRedlineInitials( const OUString& sInitials );
     bool IsFirstRun() { return m_bIsFirstRun;}
diff --git a/writerfilter/source/dmapper/PropertyMap.hxx b/writerfilter/source/dmapper/PropertyMap.hxx
index 5c12df27ff87..58487d678bfb 100644
--- a/writerfilter/source/dmapper/PropertyMap.hxx
+++ b/writerfilter/source/dmapper/PropertyMap.hxx
@@ -71,6 +71,18 @@ enum GrabBagType
     CHAR_GRAB_BAG
 };
 
+struct RedlineParams
+{
+    OUString m_sAuthor;
+    OUString m_sDate;
+    sal_Int32       m_nId;
+    sal_Int32       m_nToken;
+
+    /// This can hold properties of runs that had formatted 'track changes' properties
+    css::uno::Sequence<css::beans::PropertyValue> m_aRevertProperties;
+};
+typedef boost::shared_ptr< RedlineParams > RedlineParamsPtr;
+
 class PropValue
 {
     css::uno::Any m_aValue;
@@ -101,6 +113,9 @@ class PropertyMap : public _PropertyMap
     OUString                                                             m_sFootnoteFontName;
     ::com::sun::star::uno::Reference< ::com::sun::star::text::XFootnote >       m_xFootnote;
 
+
+    std::vector< RedlineParamsPtr > m_aRedlines;
+
 protected:
     void Invalidate()
     {
@@ -133,6 +148,9 @@ public:
 
     virtual void insertTableProperties( const PropertyMap* );
 
+    const std::vector< RedlineParamsPtr >& Redlines() const { return m_aRedlines; }
+    std::vector< RedlineParamsPtr >& Redlines() { return m_aRedlines; }
+
 #if OSL_DEBUG_LEVEL > 1
     virtual void dumpXml( const TagLogger::Pointer_t pLogger ) const;
 #endif
diff --git a/writerfilter/source/ooxml/model.xml b/writerfilter/source/ooxml/model.xml
index 36a37f61e736..b3f25afe6002 100644
--- a/writerfilter/source/ooxml/model.xml
+++ b/writerfilter/source/ooxml/model.xml
@@ -24263,19 +24263,16 @@
     </resource>
     <resource name="CT_RPrChange" resource="Properties" tag="character">
       <element name="rPr" tokenid="ooxml:CT_RPrChange_rPr"/>
-      <action name="end" action="tokenproperty"/>
-      <action name="end" action="propagateCharacterPropertiesAsSet" sendtokenid="ooxml:endtrackchange"/>
-      <action name="end" action="clearProps"/>
     </resource>
     <resource name="CT_ParaRPrChange" resource="Properties" tag="character">
       <element name="rPr" tokenid="ooxml:CT_ParaRPrChange_rPr"/>
     </resource>
     <resource name="CT_RunTrackChange" resource="Stream" tag="redlines">
       <action name="start" action="tokenproperty"/>
-      <action name="start" action="propagateCharacterPropertiesAsSet" sendtokenid="ooxml:trackchange"/>
+      <action name="start" action="sendPropertiesWithId" sendtokenid="ooxml:trackchange"/>
       <action name="start" action="clearProps"/>
       <action name="end" action="tokenproperty"/>
-      <action name="end" action="propagateCharacterPropertiesAsSet" sendtokenid="ooxml:endtrackchange"/>
+      <action name="end" action="sendPropertiesWithId" sendtokenid="ooxml:endtrackchange"/>
       <action name="end" action="clearProps"/>
     </resource>
     <resource name="EG_RangeMarkupElements" resource="Properties" tag="redlines">
@@ -24479,6 +24476,7 @@
       <element name="checkBox" tokenid="ooxml:CT_FFData_checkBox"/>
       <element name="ddList" tokenid="ooxml:CT_FFData_ddList"/>
       <element name="textInput" tokenid="ooxml:CT_FFData_textInput"/>
+      <!-- TODO: This is possibly wrong and should be sendPropertiesWithId -->
       <action name="end" action="propagateCharacterPropertiesAsSet" sendtokenid="ooxml:ffdata"/>
       <action name="end" action="clearProps"/>
     </resource>
@@ -24934,7 +24932,7 @@
     </resource>
     <resource name="CT_ParaTrackChange" resource="Properties" tag="redline">
       <action name="start" action="tokenproperty"/>
-      <action name="start" action="propagateCharacterPropertiesAsSet" sendtokenid="ooxml:paratrackchange"/>
+      <action name="start" action="sendPropertiesWithId" sendtokenid="ooxml:paratrackchange"/>
       <action name="start" action="clearProps"/>
     </resource>
     <resource name="ST_RubyAlign" resource="List" generated="yes">