sw/qa/core/layout/data/floattable-move-master.docx |binary
sw/qa/core/layout/flycnt.cxx | 42
++++++++++
sw/source/core/inc/rootfrm.hxx | 1
sw/source/core/inc/txtfrm.hxx | 2
sw/source/core/layout/calcmove.cxx | 13 +++
sw/source/core/layout/flycnt.cxx | 27
++++++
sw/source/core/objectpositioning/tocntntanchoredobjectposition.cxx | 6 +
sw/source/core/text/itratr.cxx | 10 +-
8 files changed, 95 insertions(+), 6 deletions(-)
New commits:
commit 5dc4ab7e3a071133b08db1c234eac6e6c253516e
Author: Miklos Vajna <vmik...@collabora.com>
AuthorDate: Thu Sep 21 08:31:01 2023 +0200
Commit: Caolán McNamara <caolan.mcnam...@collabora.com>
CommitDate: Fri Sep 22 14:27:46 2023 +0200
tdf#157119 sw floattable: fix moving master of split fly to next page
The problem with the bugdoc is that in case you split the text frame
hosting the start of the anchor text, then the fly will be split in a
broken way on pages 1 -> 4 -> 2, while we want a split on just pages 2
-> 3.
There were several problems here:
1) We created an unnecessary follow fly in SwFrame::GetNextFlyLeaf():
when page 1 wanted to split, we discarded the flys on page 2 and 3,
because the original anchor was in the body text and the flys on page
2/3 was in a fly, so that's rejected. This resulted in a follow fly on
page 4, which is not correct. Fix this by using an existing follow if
possible, this is similar to what SwFrame::GetNextSctLeaf() does.
2) SwFlyAtContentFrame::DelEmpty() broke the invariant that in case the
fly is split to N pieces then the anchor chain's first N frame is
matching that. Fix this by joining the unwanted anchor with its
follow right before unlinking the to-be-deleted fly from the fly chain.
3) SwToContentAnchoredObjectPosition::CalcOverlap() tried to shift down
flys due to overlapping with the to-be-deleted frames, fix this by
ignoring flys which are already in the to-delete list.
4) SwContentFrame::ShouldBwdMoved() tried to move the master fly back
from page 2 to page 1, which makes no sense, since there is not
enough space there to lay out the fly master correctly there.
Normally we only check if the paragraph fits the remaining space on
the previous page, and this is wanted: if there is a normal to-para
anchored image that would not fit, we simply shift up the image. But
this is not wanted to floating tables, since here the anchor's only
purpose is to host the start of the fly chain at a correct position.
Fix this by checking not only for the text frame height vs the
available space, but also the height of our (only) anchored object.
Note that 3) is not a huge problem, it just causes some extra cycles for
the layout (erase the to-delete flys, then position flys once more), do
it because things are complicated enough already.
(cherry picked from commit cfe9c68a7a19dd77d1fcbde3a7dd75730634becc)
Change-Id: I6f2c9d479125309d16b95df0236715c9353e8ba0
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/157167
Tested-by: Jenkins CollaboraOffice <jenkinscollaboraoff...@gmail.com>
Reviewed-by: Caolán McNamara <caolan.mcnam...@collabora.com>
diff --git a/sw/qa/core/layout/data/floattable-move-master.docx
b/sw/qa/core/layout/data/floattable-move-master.docx
new file mode 100644
index 000000000000..03087eb358a8
Binary files /dev/null and b/sw/qa/core/layout/data/floattable-move-master.docx
differ
diff --git a/sw/qa/core/layout/flycnt.cxx b/sw/qa/core/layout/flycnt.cxx
index 102a78afc0b7..0124ede8b792 100644
--- a/sw/qa/core/layout/flycnt.cxx
+++ b/sw/qa/core/layout/flycnt.cxx
@@ -1103,6 +1103,48 @@ CPPUNIT_TEST_FIXTURE(Test, testSplitFlyNestedOverlap)
SwSortedObjs& rPage2Objs = *pPage2->GetSortedObjs();
CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(2), rPage2Objs.size());
}
+
+CPPUNIT_TEST_FIXTURE(Test, testSplitFlyMoveMaster)
+{
+ // Given a document with a multi-page floating table on pages 1 -> 2 -> 3:
+ createSwDoc("floattable-move-master.docx");
+
+ // When adding an empty para before the table, so the table gets shifted
to pages 2 -> 3:
+ SwWrtShell* pWrtShell = getSwDocShell()->GetWrtShell();
+ pWrtShell->SttEndDoc(/*bStt=*/true);
+ pWrtShell->Down(/*bSelect=*/false, /*nCount=*/4);
+ pWrtShell->SplitNode();
+
+ // Then make sure page 1 has no flys, page 2 and 3 has the split fly and
no flys on page 4:
+ SwDoc* pDoc = getSwDoc();
+ SwRootFrame* pLayout = pDoc->getIDocumentLayoutAccess().GetCurrentLayout();
+ auto pPage1 = pLayout->Lower()->DynCastPageFrame();
+ CPPUNIT_ASSERT(pPage1);
+ // Without the accompanying fix in place, this test would have failed, the
start of the fly was
+ // still on page 1 instead of page 2.
+ CPPUNIT_ASSERT(!pPage1->GetSortedObjs());
+ auto pPage2 = pPage1->GetNext()->DynCastPageFrame();
+ CPPUNIT_ASSERT(pPage2);
+ CPPUNIT_ASSERT(pPage2->GetSortedObjs());
+ SwSortedObjs& rPage2Objs = *pPage2->GetSortedObjs();
+ CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(1), rPage2Objs.size());
+ auto pPage2Fly =
rPage2Objs[0]->DynCastFlyFrame()->DynCastFlyAtContentFrame();
+ CPPUNIT_ASSERT(pPage2Fly);
+ CPPUNIT_ASSERT(!pPage2Fly->GetPrecede());
+ CPPUNIT_ASSERT(pPage2Fly->HasFollow());
+ auto pPage3 = pPage2->GetNext()->DynCastPageFrame();
+ CPPUNIT_ASSERT(pPage3);
+ CPPUNIT_ASSERT(pPage3->GetSortedObjs());
+ SwSortedObjs& rPage3Objs = *pPage3->GetSortedObjs();
+ CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(1), rPage3Objs.size());
+ auto pPage3Fly =
rPage3Objs[0]->DynCastFlyFrame()->DynCastFlyAtContentFrame();
+ CPPUNIT_ASSERT(pPage3Fly);
+ CPPUNIT_ASSERT(pPage3Fly->GetPrecede());
+ CPPUNIT_ASSERT(!pPage3Fly->GetFollow());
+ auto pPage4 = pPage3->GetNext()->DynCastPageFrame();
+ CPPUNIT_ASSERT(pPage4);
+ CPPUNIT_ASSERT(!pPage4->GetSortedObjs());
+}
}
/* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/sw/source/core/inc/rootfrm.hxx b/sw/source/core/inc/rootfrm.hxx
index 7c2ba11bd449..f9f2fcada368 100644
--- a/sw/source/core/inc/rootfrm.hxx
+++ b/sw/source/core/inc/rootfrm.hxx
@@ -393,6 +393,7 @@ public:
#ifdef DBG_UTIL
bool IsInDelList( SwSectionFrame* pSct ) const;
#endif
+ bool IsInFlyDelList( SwFlyFrame* pFly ) const;
void SetCallbackActionEnabled( bool b ) { mbCallbackActionEnabled = b; }
bool IsCallbackActionEnabled() const { return mbCallbackActionEnabled; }
diff --git a/sw/source/core/inc/txtfrm.hxx b/sw/source/core/inc/txtfrm.hxx
index 9fd305c5562d..2b7f31b098d3 100644
--- a/sw/source/core/inc/txtfrm.hxx
+++ b/sw/source/core/inc/txtfrm.hxx
@@ -792,7 +792,7 @@ public:
/// This text frame may have a split fly frames anchored to it. Is any of
them a frame that has
/// a follow, i.e. not the last in a master -> follow 1 -> ... -> last
follow chain?
- bool HasNonLastSplitFlyDrawObj() const;
+ SwFlyAtContentFrame* HasNonLastSplitFlyDrawObj() const;
/// This text frame has a follow and the text frame don't contain text.
Additionally one split
/// fly is anchored to the text frame.
diff --git a/sw/source/core/layout/calcmove.cxx
b/sw/source/core/layout/calcmove.cxx
index 73b72d03305c..4f5ad202104c 100644
--- a/sw/source/core/layout/calcmove.cxx
+++ b/sw/source/core/layout/calcmove.cxx
@@ -162,6 +162,19 @@ bool SwContentFrame::ShouldBwdMoved( SwLayoutFrame
*pNewUpper, bool & )
!pNewUpper->GetUpper()->GetNext() ) ) ) )
nSpace += pNewUpper->Grow( LONG_MAX, true );
+ auto pTextFrame = DynCastTextFrame();
+ if (pTextFrame)
+ {
+ // This is a text frame. Check if it's an anchor for a
non-last element in a split
+ // fly chain. If so, we can only move back in case not only
the text frame itself,
+ // but also its fly fits nSpace.
+ SwFlyAtContentFrame* pFly =
pTextFrame->HasNonLastSplitFlyDrawObj();
+ if (pFly && pFly->getFrameArea().Height() > nSpace)
+ {
+ return false;
+ }
+ }
+
if ( nMoveAnyway < 3 )
{
if ( nSpace )
diff --git a/sw/source/core/layout/flycnt.cxx b/sw/source/core/layout/flycnt.cxx
index 13388e378867..95fcfd04efcc 100644
--- a/sw/source/core/layout/flycnt.cxx
+++ b/sw/source/core/layout/flycnt.cxx
@@ -1554,6 +1554,12 @@ SwLayoutFrame *SwFrame::GetNextFlyLeaf( MakePageType
eMakePage )
assert(pFly && "GetNextFlyLeaf: missing fly frame");
assert(pFly->IsFlySplitAllowed() && "GetNextFlyLeaf: fly split not
allowed");
+ if (pFly->HasFollow())
+ {
+ // If we already have a follow, then no need to create a new one, just
use it.
+ return pFly->GetFollow();
+ }
+
SwTextFrame* pFlyAnchor = pFly->FindAnchorCharFrame();
if (!pFlyAnchor)
@@ -1701,6 +1707,16 @@ void SwRootFrame::DeleteEmptyFlys_()
}
}
+bool SwRootFrame::IsInFlyDelList( SwFlyFrame* pFly ) const
+{
+ if (!mpFlyDestroy)
+ {
+ return false;
+ }
+
+ return mpFlyDestroy->find(pFly) != mpFlyDestroy->end();
+}
+
const SwFlyAtContentFrame* SwFlyAtContentFrame::GetPrecede() const
{
return static_cast<const SwFlyAtContentFrame*>(SwFlowFrame::GetPrecede());
@@ -1721,6 +1737,17 @@ void SwFlyAtContentFrame::DelEmpty()
// The anchor has a precede: invalidate it so that JoinFrame() is
called on it.
pAnchorPrecede->GetFrame().InvalidateSize();
}
+ else if (SwTextFrame* pAnchorFollow = pAnchor->GetFollow())
+ {
+ // No precede, but has a follow. Then move the master just before
the follow and join.
+ // This way the anchor chain and the fly chain will match even
after clearing this
+ // frame's follow pointer.
+ if (pAnchorFollow != pAnchor->GetNext())
+ {
+ pAnchor->MoveSubTree(pAnchorFollow->GetUpper(), pAnchorFollow);
+ pAnchor->JoinFrame();
+ }
+ }
}
SwFlyAtContentFrame* pMaster = IsFollow() ? GetPrecede() : nullptr;
diff --git a/sw/source/core/objectpositioning/tocntntanchoredobjectposition.cxx
b/sw/source/core/objectpositioning/tocntntanchoredobjectposition.cxx
index b83d979d31b7..69951ba08a2c 100644
--- a/sw/source/core/objectpositioning/tocntntanchoredobjectposition.cxx
+++ b/sw/source/core/objectpositioning/tocntntanchoredobjectposition.cxx
@@ -1255,6 +1255,12 @@ void
SwToContentAnchoredObjectPosition::CalcOverlap(const SwTextFrame* pAnchorFr
continue;
}
+ if
(pAnchoredObjFly->getRootFrame()->IsInFlyDelList(pAnchoredObjFly))
+ {
+ // A fly overlapping with a to-be-deleted fly is fine.
+ continue;
+ }
+
SwFrame* pAnchoredObjFlyAnchor =
pAnchoredObjFly->GetAnchorFrameContainingAnchPos();
if (pAnchoredObjFlyAnchor && pAnchoredObjFlyAnchor->IsInFly())
{
diff --git a/sw/source/core/text/itratr.cxx b/sw/source/core/text/itratr.cxx
index 5de5467f3075..6ad06999a1d0 100644
--- a/sw/source/core/text/itratr.cxx
+++ b/sw/source/core/text/itratr.cxx
@@ -1480,17 +1480,17 @@ std::vector<SwFlyAtContentFrame*>
SwTextFrame::GetSplitFlyDrawObjs() const
return aObjs;
}
-bool SwTextFrame::HasNonLastSplitFlyDrawObj() const
+SwFlyAtContentFrame* SwTextFrame::HasNonLastSplitFlyDrawObj() const
{
const SwTextFrame* pFollow = GetFollow();
if (!pFollow)
{
- return false;
+ return nullptr;
}
if (mnOffset != pFollow->GetOffset())
{
- return false;
+ return nullptr;
}
// At this point we know what we're part of a chain that is an anchor for
split fly frames, but
@@ -1513,11 +1513,11 @@ bool SwTextFrame::HasNonLastSplitFlyDrawObj() const
}
if (pFly->GetFollow())
{
- return true;
+ return pFly;
}
}
- return false;
+ return nullptr;
}
bool SwTextFrame::IsEmptyMasterWithSplitFly() const
