add some asserts in SwFormatAnchor

Which flushes out some dodgy behaviour.

Particularly in SwUndoFormatAttr, which was storing a sal_Int32 content offset in a sal_uInt16 page field.
In this case, rather just store the value on SwUndoFormatAttr itself, the same way it does for other things.

Change-Id: I5890353f5d51d82037bf7e0e77a16cf3b0dff565
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/149764
Tested-by: Noel Grandin <noel.grandin@collabora.co.uk>
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>

7 files changed, 51 insertions, 36 deletions

diff --git a/sw/source/core/inc/UndoAttribute.hxx b/sw/source/core/inc/UndoAttribute.hxx
index 178a17f5a2fd..7fbbb5b522cd 100644
--- a/sw/source/core/inc/UndoAttribute.hxx
+++ b/sw/source/core/inc/UndoAttribute.hxx
@@ -87,6 +87,7 @@ class SwUndoFormatAttr final : public SwUndo
     friend class SwUndoDefaultAttr;
     OUString m_sFormatName;
     std::optional<SfxItemSet> m_oOldSet;      // old attributes
+    sal_Int32 m_nAnchorContentOffset;
     SwNodeOffset m_nNodeIndex;
     const sal_uInt16 m_nFormatWhich;
     const bool m_bSaveDrawPt;
diff --git a/sw/source/core/layout/atrfrm.cxx b/sw/source/core/layout/atrfrm.cxx
index b53683e780e2..b976f6ee1377 100644
--- a/sw/source/core/layout/atrfrm.cxx
+++ b/sw/source/core/layout/atrfrm.cxx
@@ -1560,20 +1560,27 @@ void SwFormatHoriOrient::dumpAsXml(xmlTextWriterPtr pWriter) const
     (void)xmlTextWriterEndElement(pWriter);
 }
 
-// Partially implemented inline in hxx
 SwFormatAnchor::SwFormatAnchor( RndStdIds nRnd, sal_uInt16 nPage )
     : SfxPoolItem( RES_ANCHOR ),
     m_eAnchorId( nRnd ),
     m_nPageNumber( nPage ),
     // OD 2004-05-05 #i28701# - get always new increased order number
     m_nOrder( ++s_nOrderCounter )
-{}
+{
+    assert( m_eAnchorId == RndStdIds::FLY_AT_PARA
+        || m_eAnchorId == RndStdIds::FLY_AS_CHAR
+        || m_eAnchorId == RndStdIds::FLY_AT_PAGE
+        || m_eAnchorId == RndStdIds::FLY_AT_FLY
+        || m_eAnchorId == RndStdIds::FLY_AT_CHAR);
+    // only FLY_AT_PAGE should have a valid page
+    assert( m_eAnchorId == RndStdIds::FLY_AT_PAGE || nPage == 0 );
+}
 
 SwFormatAnchor::SwFormatAnchor( const SwFormatAnchor &rCpy )
     : SfxPoolItem( RES_ANCHOR )
     , m_oContentAnchor( rCpy.m_oContentAnchor )
-    , m_eAnchorId( rCpy.GetAnchorId() )
-    , m_nPageNumber( rCpy.GetPageNum() )
+    , m_eAnchorId( rCpy.m_eAnchorId )
+    , m_nPageNumber( rCpy.m_nPageNumber )
     // OD 2004-05-05 #i28701# - get always new increased order number
     , m_nOrder( ++s_nOrderCounter )
 {
@@ -1585,22 +1592,23 @@ SwFormatAnchor::~SwFormatAnchor()
 
 void SwFormatAnchor::SetAnchor( const SwPosition *pPos )
 {
+    if (!pPos)
+    {
+        m_oContentAnchor.reset();
+        return;
+    }
     // anchor only to paragraphs, or start nodes in case of RndStdIds::FLY_AT_FLY
     // also allow table node, this is used when a table is selected and is converted to a frame by the UI
-    assert(!pPos
-            || (RndStdIds::FLY_AT_FLY == m_eAnchorId && pPos->GetNode().GetStartNode())
+    assert((RndStdIds::FLY_AT_FLY == m_eAnchorId && pPos->GetNode().GetStartNode())
             || (RndStdIds::FLY_AT_PARA == m_eAnchorId && pPos->GetNode().GetTableNode())
             || pPos->GetNode().GetTextNode());
-    if (pPos)
-        m_oContentAnchor.emplace(*pPos);
-    else
-        m_oContentAnchor.reset();
+    // verify that the SwPosition being passed to us is not screwy
+    assert(!pPos->nContent.GetContentNode()
+            || &pPos->nNode.GetNode() == pPos->nContent.GetContentNode());
+    m_oContentAnchor.emplace(*pPos);
     // Flys anchored AT paragraph should not point into the paragraph content
-    if (m_oContentAnchor &&
-        ((RndStdIds::FLY_AT_PARA == m_eAnchorId) || (RndStdIds::FLY_AT_FLY == m_eAnchorId)))
-    {
+    if ((RndStdIds::FLY_AT_PARA == m_eAnchorId) || (RndStdIds::FLY_AT_FLY == m_eAnchorId))
         m_oContentAnchor->nContent.Assign( nullptr, 0 );
-    }
 }
 
 SwNode* SwFormatAnchor::GetAnchorNode() const
@@ -1633,8 +1641,8 @@ SwFormatAnchor& SwFormatAnchor::operator=(const SwFormatAnchor& rAnchor)
 {
     if (this != &rAnchor)
     {
-        m_eAnchorId  = rAnchor.GetAnchorId();
-        m_nPageNumber   = rAnchor.GetPageNum();
+        m_eAnchorId  = rAnchor.m_eAnchorId;
+        m_nPageNumber   = rAnchor.m_nPageNumber;
         // OD 2004-05-05 #i28701# - get always new increased order number
         m_nOrder = ++s_nOrderCounter;
         m_oContentAnchor  = rAnchor.m_oContentAnchor;
@@ -1647,8 +1655,8 @@ bool SwFormatAnchor::operator==( const SfxPoolItem& rAttr ) const
     assert(SfxPoolItem::operator==(rAttr));
     SwFormatAnchor const& rFormatAnchor(static_cast<SwFormatAnchor const&>(rAttr));
     // OD 2004-05-05 #i28701# - Note: <mnOrder> hasn't to be considered.
-    return ( m_eAnchorId == rFormatAnchor.GetAnchorId() &&
-             m_nPageNumber == rFormatAnchor.GetPageNum()   &&
+    return ( m_eAnchorId == rFormatAnchor.m_eAnchorId &&
+             m_nPageNumber == rFormatAnchor.m_nPageNumber   &&
                 // compare anchor: either both do not point into a textnode or
                 // both do (valid m_oContentAnchor) and the positions are equal
              (m_oContentAnchor == rFormatAnchor.m_oContentAnchor) );
@@ -1674,7 +1682,7 @@ bool SwFormatAnchor::QueryValue( uno::Any& rVal, sal_uInt8 nMemberId ) const
         case MID_ANCHOR_ANCHORTYPE:
 
             text::TextContentAnchorType eRet;
-            switch (GetAnchorId())
+            switch (m_eAnchorId)
             {
                 case  RndStdIds::FLY_AT_CHAR:
                     eRet = text::TextContentAnchorType_AT_CHARACTER;
@@ -1749,10 +1757,12 @@ bool SwFormatAnchor::PutValue( const uno::Any& rVal, sal_uInt8 nMemberId )
                 case  text::TextContentAnchorType_AT_CHARACTER:
                     eAnchor = RndStdIds::FLY_AT_CHAR;
                     break;
-                //case  text::TextContentAnchorType_AT_PARAGRAPH:
-                default:
+                case text::TextContentAnchorType_AT_PARAGRAPH:
                     eAnchor = RndStdIds::FLY_AT_PARA;
                     break;
+                default:                    
+                    eAnchor = RndStdIds::FLY_AT_PARA; // just to keep some compilers happy
+                    assert(false);
             }
             SetType( eAnchor );
         }
@@ -1762,9 +1772,9 @@ bool SwFormatAnchor::PutValue( const uno::Any& rVal, sal_uInt8 nMemberId )
             sal_Int16 nVal = 0;
             if((rVal >>= nVal) && nVal > 0)
             {
-                SetPageNum( nVal );
-                if (RndStdIds::FLY_AT_PAGE == GetAnchorId())
+                if (RndStdIds::FLY_AT_PAGE == m_eAnchorId)
                 {
+                    SetPageNum( nVal );
                     // If the anchor type is page and a valid page number
                     // is set, the content position has to be deleted to not
                     // confuse the layout (frmtool.cxx). However, if the
@@ -1772,6 +1782,11 @@ bool SwFormatAnchor::PutValue( const uno::Any& rVal, sal_uInt8 nMemberId )
                     // be kept.
                     m_oContentAnchor.reset();
                 }
+                else
+                {
+                    assert(false && "cannot set page number on this anchor type");
+                    bRet = false;
+                }
             }
             else
                 bRet = false;
diff --git a/sw/source/core/undo/unattr.cxx b/sw/source/core/undo/unattr.cxx
index ac57fe8adcba..ca6a53f71e99 100644
--- a/sw/source/core/undo/unattr.cxx
+++ b/sw/source/core/undo/unattr.cxx
@@ -383,24 +383,23 @@ void SwUndoFormatAttr::SaveFlyAnchor( const SwFormat * pFormat, bool bSvDrwPt )
 
     const SwFormatAnchor& rAnchor =
         m_oOldSet->Get( RES_ANCHOR, false );
-    if( !rAnchor.GetAnchorNode() )
+    if( !rAnchor.GetAnchorNode() || rAnchor.GetAnchorId() == RndStdIds::FLY_AT_PAGE)
         return;
 
-    sal_Int32 nContent = 0;
     switch( rAnchor.GetAnchorId() ) {
     case RndStdIds::FLY_AS_CHAR:
     case RndStdIds::FLY_AT_CHAR:
-        nContent = rAnchor.GetAnchorContentOffset();
+        m_nAnchorContentOffset = rAnchor.GetAnchorContentOffset();
         [[fallthrough]];
     case RndStdIds::FLY_AT_PARA:
     case RndStdIds::FLY_AT_FLY:
         m_nNodeIndex = rAnchor.GetAnchorNode()->GetIndex();
         break;
     default:
-        return;
+        assert(false);
     }
 
-    SwFormatAnchor aAnchor( rAnchor.GetAnchorId(), nContent );
+    SwFormatAnchor aAnchor( rAnchor.GetAnchorId(), 0 );
     m_oOldSet->Put( aAnchor );
 }
 
@@ -431,7 +430,7 @@ bool SwUndoFormatAttr::RestoreFlyAnchor(::sw::UndoRedoContext & rContext)
         SwPosition aPos( *pNd );
         if ((RndStdIds::FLY_AS_CHAR == rAnchor.GetAnchorId()) ||
             (RndStdIds::FLY_AT_CHAR == rAnchor.GetAnchorId())) {
-            aPos.SetContent( rAnchor.GetPageNum() );
+            aPos.SetContent( m_nAnchorContentOffset );
             if ( aPos.GetContentIndex() > pNd->GetTextNode()->GetText().getLength()) {
                 // #i35443# - invalid position.
                 // Thus, anchor attribute not restored
diff --git a/sw/source/core/unocore/unoframe.cxx b/sw/source/core/unocore/unoframe.cxx
index a55e4d976a7c..ff9e06e93219 100644
--- a/sw/source/core/unocore/unoframe.cxx
+++ b/sw/source/core/unocore/unoframe.cxx
@@ -183,12 +183,12 @@ bool BaseFrameProperties_Impl::FillBaseProperties(SfxItemSet& rToSet, const SfxI
     // always add an anchor to the set
     SwFormatAnchor aAnchor ( rFromSet.Get ( RES_ANCHOR ) );
     {
-        const ::uno::Any* pAnchorPgNo;
-        if(GetProperty(RES_ANCHOR, MID_ANCHOR_PAGENUM, pAnchorPgNo))
-            bRet &= static_cast<SfxPoolItem&>(aAnchor).PutValue(*pAnchorPgNo, MID_ANCHOR_PAGENUM);
         const ::uno::Any* pAnchorType;
         if(GetProperty(RES_ANCHOR, MID_ANCHOR_ANCHORTYPE, pAnchorType))
             bRet &= static_cast<SfxPoolItem&>(aAnchor).PutValue(*pAnchorType, MID_ANCHOR_ANCHORTYPE);
+        const ::uno::Any* pAnchorPgNo;
+        if(GetProperty(RES_ANCHOR, MID_ANCHOR_PAGENUM, pAnchorPgNo))
+            bRet &= static_cast<SfxPoolItem&>(aAnchor).PutValue(*pAnchorPgNo, MID_ANCHOR_PAGENUM);
     }
 
     rToSet.Put(aAnchor);
diff --git a/sw/source/ui/frmdlg/frmpage.cxx b/sw/source/ui/frmdlg/frmpage.cxx
index 94c86e95df5e..74ccc1a51809 100644
--- a/sw/source/ui/frmdlg/frmpage.cxx
+++ b/sw/source/ui/frmdlg/frmpage.cxx
@@ -1099,7 +1099,7 @@ bool SwFramePage::FillItemSet(SfxItemSet *rSet)
             OSL_ENSURE( pSh , "shell not found");
             if (pSh)
             {
-                SwFormatAnchor aAnc( eAnchorId, pSh->GetPhyPageNum() );
+                SwFormatAnchor aAnc( eAnchorId, eAnchorId == RndStdIds::FLY_AT_PAGE ? pSh->GetPhyPageNum() : 0 );
                 bRet = nullptr != rSet->Put( aAnc );
             }
         }
@@ -1767,7 +1767,7 @@ DeactivateRC SwFramePage::DeactivatePage(SfxItemSet * _pSet)
             if (pSh)
             {
                 RndStdIds eAnchorId = GetAnchor();
-                SwFormatAnchor aAnc( eAnchorId, pSh->GetPhyPageNum() );
+                SwFormatAnchor aAnc( eAnchorId, eAnchorId == RndStdIds::FLY_AT_PAGE ? pSh->GetPhyPageNum() : 0 );
                 _pSet->Put( aAnc );
             }
         }
diff --git a/sw/source/uibase/frmdlg/frmmgr.cxx b/sw/source/uibase/frmdlg/frmmgr.cxx
index d135f5f2d24b..729892398997 100644
--- a/sw/source/uibase/frmdlg/frmmgr.cxx
+++ b/sw/source/uibase/frmdlg/frmmgr.cxx
@@ -227,7 +227,7 @@ void SwFlyFrameAttrMgr::SetAnchor( RndStdIds eId )
     sal_uInt16 nPhyPageNum, nVirtPageNum;
     m_pOwnSh->GetPageNum( nPhyPageNum, nVirtPageNum );
 
-    m_aSet.Put( SwFormatAnchor( eId, nPhyPageNum ) );
+    m_aSet.Put( SwFormatAnchor( eId, RndStdIds::FLY_AT_PAGE == eId ? nPhyPageNum : 0 ) );
     if ((RndStdIds::FLY_AT_PAGE == eId) || (RndStdIds::FLY_AT_PARA == eId) || (RndStdIds::FLY_AT_CHAR == eId)
         || (RndStdIds::FLY_AT_FLY == eId))
     {
diff --git a/sw/source/uibase/shells/basesh.cxx b/sw/source/uibase/shells/basesh.cxx
index 07a44e6d78be..c4154b00de7e 100644
--- a/sw/source/uibase/shells/basesh.cxx
+++ b/sw/source/uibase/shells/basesh.cxx
@@ -1474,7 +1474,7 @@ void SwBaseShell::Execute(SfxRequest &rReq)
                 rSh.ChgAnchor(eSet);
             else if (rSh.IsFrameSelected())
             {
-                SwFormatAnchor aAnc(eSet, rSh.GetPhyPageNum());
+                SwFormatAnchor aAnc(eSet, eSet == RndStdIds::FLY_AT_PAGE ? rSh.GetPhyPageNum() : 0);
                 SfxItemSet aSet(SwFEShell::makeItemSetFromFormatAnchor(GetPool(), aAnc));
                 rSh.SetFlyFrameAttr(aSet);
             }