sw/qa/extras/layout/data/tdf170477.docx |binary sw/qa/extras/layout/layout6.cxx | 7 +++++++ sw/source/core/layout/tabfrm.cxx | 9 ++++----- 3 files changed, 11 insertions(+), 5 deletions(-)
New commits: commit e23029a24cb2e4bcdb49c1065c9a8afd19593861 Author: Mike Kaganski <[email protected]> AuthorDate: Mon Jan 26 15:29:24 2026 +0500 Commit: Mike Kaganski <[email protected]> CommitDate: Thu Jan 29 11:54:05 2026 +0100 tdf#170477 follow-up: reimplement the fix by cleaning up code This is an alternative fix to the bug. The idea is, that we shouldn't call MoveFwd in SwTabFrame::MakeAll in case of floating tables. In commit 45574624ff05673d44f11cdbbbb49e1af599133e (tdf#120262 sw floattable: no split when none of first row fits the vert space, 2023-07-19) a code was added, that made sure that for floating tables MoveFwd was called with bMoveAlways = true. Obviously, for that bug, this code path was used at that time. However, the unit test for that but doesn't pass the code path these days. What happens inside MoveFwd for a floating table is: 1. MoveFwd calls GetLeaf, which in turn calls GetNextFlyLeaf; 2. GetNextFlyLeaf splits the anchor (the new frame isn't yet moved), and creates a follow fly, which is still on the same page; 3. MoveFwd moves the whole table to the new fly, making the old fly empty; 4. The old fly calls its DelEmpty, breaking precede/follow chain and making the new fly standalone. In the end, the situation after the call is fundamentally the same as before, with exception of new garbage (empty fly) to clean later, and the split anchor that needs a join. This change avoids calling MoveFwd for floating tables in this case. It keeps all unit tests working, so hopefully this cleanup could be an improvement by simplifying code, and by avoiding a call to SwLayouter::InsertMovedFwdFrame, which itself is manual override of the normal layout process (IMO, the less it's used, the better). If it turns out problematic, it may be simply reverted (that's the idea of merging it only after the more conservative fix). Change-Id: I606976de629bb5514edafd1430704901241876cc Reviewed-on: https://gerrit.libreoffice.org/c/core/+/198135 Tested-by: Jenkins Reviewed-by: Mike Kaganski <[email protected]> Reviewed-on: https://gerrit.libreoffice.org/c/core/+/198344 Tested-by: Jenkins CollaboraOffice <[email protected]> diff --git a/sw/source/core/layout/tabfrm.cxx b/sw/source/core/layout/tabfrm.cxx index 69b37c37a47d..9f25d14641e0 100644 --- a/sw/source/core/layout/tabfrm.cxx +++ b/sw/source/core/layout/tabfrm.cxx @@ -3277,58 +3277,34 @@ void SwTabFrame::MakeAll(vcl::RenderContext* pRenderContext) // #i29771# Reset bTryToSplit flag on change of upper const SwFrame* pOldUpper = GetUpper(); - const SwPageFrame* pOldUpperPage = nullptr; //Let's see if we find some place anywhere... if (!bMovedFwd) { - bool bMoveAlways = false; + bool bSkipMoveFwd = false; SwFrame* pUpper = GetUpper(); if (pUpper && pUpper->IsFlyFrame()) { auto pFlyFrame = static_cast<SwFlyFrame*>(pUpper); if (pFlyFrame->IsFlySplitAllowed()) { - // If the anchor of the split has a previous frame, MoveFwd() is allowed to move - // forward. - bMoveAlways = true; - pOldUpperPage = pFlyFrame->FindPageFrame(); + // Don't try to move floating table forward. It's done elsewhere moving its fly. + bSkipMoveFwd = true; } } // don't make the effort to move fwd if its known // conditions that are known not to work if (IsInFootnote() && ForbiddenForFootnoteCntFwd()) bMakePage = false; - else if (!MoveFwd(bMakePage, false, bMoveAlways)) + else if (bSkipMoveFwd || !MoveFwd(bMakePage, false)) bMakePage = false; } // #i29771# Reset bSplitError flag on change of upper if ( GetUpper() != pOldUpper ) { - bool bResetSplit = true; - if (GetUpper() && GetUpper()->IsFlyFrame() && pOldUpperPage - && GetUpper()->FindPageFrame() == pOldUpperPage - && static_cast<const SwFlyFrame*>(GetUpper())->IsFlySplitAllowed()) - { - // MoveFwd created a split (follow) fly on the same page, that was intended to move - // to the next page together with its anchor. Then the content was moved to the new - // fly, and the old fly became empty; it called its DelEmpty(). The new fly is now - // basically a copy of the old one; move to the next page and prevent oscillation. - SwTextFrame* pAnchor = static_cast<SwFlyFrame*>(GetUpper())->FindAnchorCharFrame(); - if (pAnchor) - { - bResetSplit = false; - SwPageFrame* pPage = pAnchor->FindPageFrame(); - SwLayouter::InsertMovedFwdFrame(pPage->GetFormat()->GetDoc(), *pAnchor, - pPage->GetPhyPageNum() + 1); - } - } - if (bResetSplit) - { - bTryToSplit = true; - nUnSplitted = 5; - } + bTryToSplit = true; + nUnSplitted = 5; } aRectFnSet.Refresh(*this); commit 771c50039bebc920837907d58a32a1417c69f440 Author: Mike Kaganski <[email protected]> AuthorDate: Mon Jan 26 10:46:04 2026 +0500 Commit: Mike Kaganski <[email protected]> CommitDate: Thu Jan 29 11:53:54 2026 +0100 tdf#170477: detect MoveFwd oscillation in SwTabFrame::MakeAll In the specific bug document, some constellation of factors created the case where the last inner floating table tries to move forward, in the process creates a split fly on the same page (subject to move forward later, when outer table formats), moves to it (leaving the old fly empty), which recreates the same layout as before; and that oscillated infinitely. The fix implemented here is to detect when MoveFwd call created the described situation, and then use SwLayouter::InsertMovedFwdFrame to break the oscillation. I was not able to reproduce this problem anew; test document was simplified from the original, with the aim to minimize instability (but still, the resulting layout in Writer differs from Word's, which makes the test likely to stop testing the intended code path in the future - sigh). Change-Id: Iaa796a169cb9f97d5a6b3dd7ebf1cf5b726ca568 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/198116 Reviewed-by: Mike Kaganski <[email protected]> Tested-by: Jenkins Reviewed-on: https://gerrit.libreoffice.org/c/core/+/198343 Tested-by: Jenkins CollaboraOffice <[email protected]> diff --git a/sw/qa/extras/layout/data/tdf170477.docx b/sw/qa/extras/layout/data/tdf170477.docx new file mode 100644 index 000000000000..646d7191c92d Binary files /dev/null and b/sw/qa/extras/layout/data/tdf170477.docx differ diff --git a/sw/qa/extras/layout/layout6.cxx b/sw/qa/extras/layout/layout6.cxx index e69f42a197ab..88fd08d5113e 100644 --- a/sw/qa/extras/layout/layout6.cxx +++ b/sw/qa/extras/layout/layout6.cxx @@ -2042,6 +2042,13 @@ CPPUNIT_TEST_FIXTURE(SwLayoutWriter6, testTdf170381_split_float_table_in_float_t assertCellLines(2, r + 1, page2cells[r]); } +CPPUNIT_TEST_FIXTURE(SwLayoutWriter6, testTdf170477) +{ + // This document must not hang on layout: + createSwDoc("tdf170477.docx"); + calcLayout(); +} + } // end of anonymous namespace CPPUNIT_PLUGIN_IMPLEMENT(); diff --git a/sw/source/core/layout/tabfrm.cxx b/sw/source/core/layout/tabfrm.cxx index a5bbd2b3710b..69b37c37a47d 100644 --- a/sw/source/core/layout/tabfrm.cxx +++ b/sw/source/core/layout/tabfrm.cxx @@ -3277,6 +3277,7 @@ void SwTabFrame::MakeAll(vcl::RenderContext* pRenderContext) // #i29771# Reset bTryToSplit flag on change of upper const SwFrame* pOldUpper = GetUpper(); + const SwPageFrame* pOldUpperPage = nullptr; //Let's see if we find some place anywhere... if (!bMovedFwd) @@ -3291,6 +3292,7 @@ void SwTabFrame::MakeAll(vcl::RenderContext* pRenderContext) // If the anchor of the split has a previous frame, MoveFwd() is allowed to move // forward. bMoveAlways = true; + pOldUpperPage = pFlyFrame->FindPageFrame(); } } // don't make the effort to move fwd if its known @@ -3304,8 +3306,29 @@ void SwTabFrame::MakeAll(vcl::RenderContext* pRenderContext) // #i29771# Reset bSplitError flag on change of upper if ( GetUpper() != pOldUpper ) { - bTryToSplit = true; - nUnSplitted = 5; + bool bResetSplit = true; + if (GetUpper() && GetUpper()->IsFlyFrame() && pOldUpperPage + && GetUpper()->FindPageFrame() == pOldUpperPage + && static_cast<const SwFlyFrame*>(GetUpper())->IsFlySplitAllowed()) + { + // MoveFwd created a split (follow) fly on the same page, that was intended to move + // to the next page together with its anchor. Then the content was moved to the new + // fly, and the old fly became empty; it called its DelEmpty(). The new fly is now + // basically a copy of the old one; move to the next page and prevent oscillation. + SwTextFrame* pAnchor = static_cast<SwFlyFrame*>(GetUpper())->FindAnchorCharFrame(); + if (pAnchor) + { + bResetSplit = false; + SwPageFrame* pPage = pAnchor->FindPageFrame(); + SwLayouter::InsertMovedFwdFrame(pPage->GetFormat()->GetDoc(), *pAnchor, + pPage->GetPhyPageNum() + 1); + } + } + if (bResetSplit) + { + bTryToSplit = true; + nUnSplitted = 5; + } } aRectFnSet.Refresh(*this);
