From fefc0dfdcedbd0ef9d32c32be356f8086e756b33 Mon Sep 17 00:00:00 2001 From: Michael Stahl Date: Mon, 12 Oct 2020 17:43:28 +0200 Subject: [PATCH] sw_redlinehide: fix crash in SwTextNode::SplitContentNode() Bug happens in the "top" branch of SplitContentNode(). SwTextNode::CutImpl() sends several hints to the SwTextFrame so that it invalidates itself; a SwInsText and a SwDelText most prominently. The SwInsText won't have an effect because the target node doesn't have text frames yet, but the SwDelText should be sufficient to update the text frames' MergedPara and invalidate the text frames. The MergedPara is re-created by SwTextFrame::RegisterToNode() anyway, but that *doesn't* invalidate the SwTextFrame. Then an additional SwDelText is sent, which will *not* invalidate the SwTextFrame, because its MergedPara is up-to-date so nothing gets deleted by the hint. It's unclear why the LockModify() is done here, since CVS initial import, perhaps it's some premature optimization; try to remove the manual sending of SwDelText and just let CutImpl() do it. Also remove an odd assert in UpdateMergedParaForMove() that i don't understand the reason for now. Change-Id: Iba7196556f614356dba4def74f039a88f392eb0f Reviewed-on: https://gerrit.libreoffice.org/c/core/+/104219 Tested-by: Michael Stahl Reviewed-by: Michael Stahl --- sw/qa/extras/uiwriter/data2/try2.fodt | 66 +++++++++++++++++++++++++++ sw/qa/extras/uiwriter/uiwriter2.cxx | 42 +++++++++++++++++ sw/source/core/text/txtfrm.cxx | 1 - sw/source/core/txtnode/ndtxt.cxx | 26 +---------- 4 files changed, 109 insertions(+), 26 deletions(-) create mode 100644 sw/qa/extras/uiwriter/data2/try2.fodt diff --git a/sw/qa/extras/uiwriter/data2/try2.fodt b/sw/qa/extras/uiwriter/data2/try2.fodt new file mode 100644 index 000000000000..624a75a85ea9 --- /dev/null +++ b/sw/qa/extras/uiwriter/data2/try2.fodt @@ -0,0 +1,66 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + Unknown Author + 2020-10-12T19:26:19 + + + + + + + Unknown Author + 2020-10-12T19:26:26 + + + + + A + xxxxxxxxxxxxxxxxxxx + yy + + + diff --git a/sw/qa/extras/uiwriter/uiwriter2.cxx b/sw/qa/extras/uiwriter/uiwriter2.cxx index 334698e7c693..25015b50e3c5 100644 --- a/sw/qa/extras/uiwriter/uiwriter2.cxx +++ b/sw/qa/extras/uiwriter/uiwriter2.cxx @@ -305,6 +305,48 @@ CPPUNIT_TEST_FIXTURE(SwUiWriterTest2, testRedlineInHiddenSection) CPPUNIT_ASSERT(pNode->GetNodes()[pNode->GetIndex() + 4]->IsEndNode()); } +CPPUNIT_TEST_FIXTURE(SwUiWriterTest2, testRedlineSplitContentNode) +{ + load(DATA_DIRECTORY, "try2.fodt"); + SwXTextDocument* pTextDoc = dynamic_cast(mxComponent.get()); + SwWrtShell* const pWrtShell = pTextDoc->GetDocShell()->GetWrtShell(); + + SwViewOption aViewOptions(*pWrtShell->GetViewOptions()); + // these are required so that IsBlank() is true + aViewOptions.SetBlank(true); + aViewOptions.SetViewMetaChars(true); + pWrtShell->ApplyViewOptions(aViewOptions); + + // enable redlining + dispatchCommand(mxComponent, ".uno:TrackChanges", {}); + // hide + dispatchCommand(mxComponent, ".uno:ShowTrackedChanges", {}); + + SwDocShell* const pDocShell = pTextDoc->GetDocShell(); + SwDoc* const pDoc = pDocShell->GetDoc(); + CPPUNIT_ASSERT_MESSAGE("redlining should be on", + pDoc->getIDocumentRedlineAccess().IsRedlineOn()); + CPPUNIT_ASSERT_MESSAGE( + "redlines should be visible", + IDocumentRedlineAccess::IsShowChanges(pDoc->getIDocumentRedlineAccess().GetRedlineFlags())); + + sw::UndoManager& rUndoManager = pDoc->GetUndoManager(); + + pWrtShell->CalcLayout(); + pWrtShell->Right(CRSR_SKIP_CHARS, /*bSelect=*/false, 18, /*bBasicCall=*/false); + pWrtShell->SplitNode(true); + rUndoManager.Undo(); + // crashed + pWrtShell->SplitNode(true); + rUndoManager.Undo(); + rUndoManager.Redo(); + rUndoManager.Undo(); + rUndoManager.Redo(); + rUndoManager.Undo(); + pWrtShell->SplitNode(true); + rUndoManager.Undo(); +} + CPPUNIT_TEST_FIXTURE(SwUiWriterTest2, testTdf132236) { load(DATA_DIRECTORY, "tdf132236.odt"); diff --git a/sw/source/core/text/txtfrm.cxx b/sw/source/core/text/txtfrm.cxx index 0178f275e540..ed04a3e42c47 100644 --- a/sw/source/core/text/txtfrm.cxx +++ b/sw/source/core/text/txtfrm.cxx @@ -1906,7 +1906,6 @@ void UpdateMergedParaForMove(sw::MergedPara & rMerged, } if (nLastEnd != rNode.Len()) // without nLen, string yet to be removed { - assert(rNode.Len() == 0 || nLastEnd < nSourceEnd); if (nLastEnd < nSourceEnd) { deleted.emplace_back(std::max(nLastEnd, nSourceStart), nSourceEnd); diff --git a/sw/source/core/txtnode/ndtxt.cxx b/sw/source/core/txtnode/ndtxt.cxx index f62ca8ea33ff..00032f256242 100644 --- a/sw/source/core/txtnode/ndtxt.cxx +++ b/sw/source/core/txtnode/ndtxt.cxx @@ -447,9 +447,7 @@ SwTextNode *SwTextNode::SplitContentNode(const SwPosition & rPos, { // optimization for SplitNode: If a split is at the end of a node then // move the frames from the current to the new one and create new ones - // for the current one. As a result, no need for recreating the layout. - - LockModify(); // disable notifications + // for the current one. // If fly frames are moved, they don't need to destroy their layout // frames. Set a flag that is checked in SwTextFlyCnt::SetAnchor. @@ -557,28 +555,6 @@ SwTextNode *SwTextNode::SplitContentNode(const SwPosition & rPos, SetInCache( false ); } - UnlockModify(); // enable notify again - - // If there is an accessible layout we must call modify even - // with length zero, because we have to notify about the changed - // text node. - const SwRootFrame *pRootFrame; - if ( (nTextLen != nSplitPos) || - ( (pRootFrame = pNode->GetDoc().getIDocumentLayoutAccess().GetCurrentLayout()) != nullptr && - pRootFrame->IsAnyShellAccessible() ) ) - { - // tell the frames that something was "deleted" at the end - if( 1 == nTextLen - nSplitPos ) - { - SwDelChr aHint( nSplitPos ); - pNode->NotifyClients( nullptr, &aHint ); - } - else - { - SwDelText aHint( nSplitPos, nTextLen - nSplitPos ); - pNode->NotifyClients( nullptr, &aHint ); - } - } if ( HasHints() ) { MoveTextAttr_To_AttrSet();