sw/qa/extras/uiwriter/data/tdf147006.rtf | 24 ++++++++++++++ sw/qa/extras/uiwriter/uiwriter2.cxx | 43 ++++++++++++++++++++++++++ sw/source/core/crsr/bookmark.cxx | 19 +++++++---- sw/source/core/doc/DocumentRedlineManager.cxx | 3 + sw/source/core/doc/docbm.cxx | 8 +--- sw/source/core/txtnode/modeltoviewhelper.cxx | 2 - sw/source/core/undo/rolbck.cxx | 8 ++-- 7 files changed, 89 insertions(+), 18 deletions(-)
New commits: commit bdf1d9b8151476531f2fbe06f66db260efcbc529 Author: Michael Stahl <[email protected]> AuthorDate: Tue Feb 1 21:35:46 2022 +0100 Commit: Michael Stahl <[email protected]> CommitDate: Wed Feb 2 09:47:36 2022 +0100 tdf#147006 sw_fieldmarkhide: fix crash when deleting fly with fieldmark The problem is similar to commit eef10be20a4c5108c68b19ccdda263c5ca852386, there is a fieldmark in a fly and this results in UpdateFramesForRemoveDeleteRedline() re-creating fly frames that have already been deleted in SwUndoFlyBase::DelFly(), and then the SwFlyAtContentFrame::SwClientNotify() crashes on a null anchor position in the SwFormat::ResetFormatAttr(RES_ANCHOR). This time the passed rPam is empty, after removing the dummy characters of the fieldmark; there isn't really anything to do in this case so just return. Change-Id: I475b8fcb0bcf94be58ff553454c261d75076303b Reviewed-on: https://gerrit.libreoffice.org/c/core/+/129308 Tested-by: Jenkins Reviewed-by: Michael Stahl <[email protected]> diff --git a/sw/qa/extras/uiwriter/data/tdf147006.rtf b/sw/qa/extras/uiwriter/data/tdf147006.rtf new file mode 100644 index 000000000000..462c8dc4d52d --- /dev/null +++ b/sw/qa/extras/uiwriter/data/tdf147006.rtf @@ -0,0 +1,24 @@ +{\rtf1\ansi\deff3\adeflang1025 +{\fonttbl{\f0\froman\fprq2\fcharset0 Times New Roman;}{\f1\froman\fprq2\fcharset2 Symbol;}{\f2\fswiss\fprq2\fcharset0 Arial;}{\f3\froman\fprq2\fcharset0 Liberation Serif{\*\falt Times New Roman};}{\f4\fswiss\fprq2\fcharset0 Liberation Sans{\*\falt Arial};}{\f5\fnil\fprq2\fcharset0 Source Han Sans CN;}{\f6\fnil\fprq2\fcharset0 Lohit Devanagari;}{\f7\fnil\fprq0\fcharset128 Lohit Devanagari;}} +{\colortbl;\red0\green0\blue0;\red0\green0\blue255;\red0\green255\blue255;\red0\green255\blue0;\red255\green0\blue255;\red255\green0\blue0;\red255\green255\blue0;\red255\green255\blue255;\red0\green0\blue128;\red0\green128\blue128;\red0\green128\blue0;\red128\green0\blue128;\red128\green0\blue0;\red128\green128\blue0;\red128\green128\blue128;\red192\green192\blue192;} +{\stylesheet{\s0\snext0\rtlch\af6\afs24\alang1081 \ltrch\lang1031\langfe2052\hich\af3\loch\widctlpar\hyphpar0\ltrpar\cf0\f3\fs24\lang1031\kerning1\dbch\af8\langfe2052 Normal;} +{\s20\sbasedon0\snext20 Frame Contents;} +}{\*\generator LibreOfficeDev/7.4.0.0.alpha0$Linux_X86_64 LibreOffice_project/086efd30b2f5857d2b155099ec06c522d57ad81f}{\info{\creatim\yr2022\mo2\dy1\hr21\min9}{\revtim\yr2022\mo2\dy1\hr21\min10}{\printim\yr0\mo0\dy0\hr0\min0}}{\*\userprops}\deftab709 +\hyphauto1\viewscale100 +{\*\pgdsctbl +{\pgdsc0\pgdscuse451\pgwsxn11906\pghsxn16838\marglsxn1134\margrsxn1134\margtsxn1134\margbsxn1134\pgdscnxt0 Default Page Style;}} +\formshade\paperh16838\paperw11906\margl1134\margr1134\margt1134\margb1134\sectd\sbknone\pgndec\sftnnar\saftnnrlc\sectunlocked1\pgwsxn11906\pghsxn16838\marglsxn1134\margrsxn1134\margtsxn1134\margbsxn1134\ftnbj\ftnstart1\ftnrstcont\ftnnar\aenddoc\aftnrstcont\aftnstart1\aftnnrlc +{\*\ftnsep\chftnsep}\pard\plain \s0\rtlch\af6\afs24\alang1081 \ltrch\lang1031\langfe2052\hich\af3\loch\widctlpar\hyphpar0\ltrpar\cf0\f3\fs24\lang1031\kerning1\dbch\af8\langfe2052\loch\ql\ltrpar\loch +{\shp{\*\shpinst\shpwr2\shpwrk0\shpbypara\shpbyignore\shptop114\shpbottom1248\shpbxcolumn\shpbxignore\shpleft4252\shpright5386\shpz0{\sp{\sn shapeType}{\sv 202}}{\sp{\sn dxWrapDistLeft}{\sv 72390}}{\sp{\sn dxWrapDistRight}{\sv 72390}}{\sp{\sn dyWrapDistTop}{\sv 72390}}{\sp{\sn dyWrapDistBottom}{\sv 72390}}{\sp{\sn posrelv}{\sv 2}}{\sp{\sn posv}{\sv 1}}{\sp{\sn posrelh}{\sv 2}}{\sp{\sn posh}{\sv 2}}{\sp{\sn dxTextLeft}{\sv 53975}}{\sp{\sn dyTextTop}{\sv 53975}}{\sp{\sn dxTextRight}{\sv 53975}}{\sp{\sn dyTextBottom}{\sv 53975}}{\sp{\sn lineColor}{\sv 0}}{\sp{\sn lineWidth}{\sv 635}}{\shptxt\pgndec\s20\loch\ql{ +{\*\shppict{\pict{\*\picprop{\sp{\sn wzDescription}{\sv }}{\sp{\sn wzName}{\sv }}}\picscalex100\picscaley100\piccropl0\piccropr0\piccropt0\piccropb0\picw19\pich19\picwgoal380\pichgoal380\pngblip +89504e470d0a1a0a0000000d4948445200000013000000130806000000725036cc000000017352474200aece1ce900000006624b474400ff00ff00ffa0bda793 +000000097048597300000b1300000b1301009a9c180000000774494d4507db0906123403a1d7aeb2000000654944415438cb63fcffff3f03b50013318a2497b0 +fca78a6130838831908914171132908954afe13390899c30c225cf446e606353c7448e41b8d433e133e879cc1f467c7c747d3823009b467ce20c0c0c0c2cc428 +22d64026062a8251c38693618cd42c6901c5e32de14bf6d21e0000000049454e44ae426082}} +}{\loch +{\field{\*\fldinst { FORMTEXT }{\loch +}}{\fldrslt {}{\loch +}}}} +\par \pard}}} + +\par } diff --git a/sw/qa/extras/uiwriter/uiwriter2.cxx b/sw/qa/extras/uiwriter/uiwriter2.cxx index e0897a6f350f..a02191c3f8a4 100644 --- a/sw/qa/extras/uiwriter/uiwriter2.cxx +++ b/sw/qa/extras/uiwriter/uiwriter2.cxx @@ -3667,6 +3667,41 @@ CPPUNIT_TEST_FIXTURE(SwUiWriterTest2, testMixedFormFieldInsertion) CPPUNIT_ASSERT_EQUAL(sal_Int32(3), pMarkAccess->getAllMarksCount()); } +CPPUNIT_TEST_FIXTURE(SwUiWriterTest2, testTdf147006) +{ + SwDoc* const pDoc = createSwDoc(DATA_DIRECTORY, "tdf147006.rtf"); + + IDocumentMarkAccess& rIDMA(*pDoc->getIDocumentMarkAccess()); + CPPUNIT_ASSERT_EQUAL(size_t(1), pDoc->GetFlyCount(FLYCNTTYPE_FRM)); + CPPUNIT_ASSERT_EQUAL(size_t(1), pDoc->GetFlyCount(FLYCNTTYPE_GRF)); + CPPUNIT_ASSERT_EQUAL(sal_Int32(1), rIDMA.getAllMarksCount()); + CPPUNIT_ASSERT_EQUAL(std::iterator_traits<IDocumentMarkAccess::iterator>::difference_type(1), + std::distance(rIDMA.getFieldmarksBegin(), rIDMA.getFieldmarksEnd())); + + dispatchCommand(mxComponent, ".uno:SelectAll", {}); + // this crashed + dispatchCommand(mxComponent, ".uno:Delete", {}); + CPPUNIT_ASSERT_EQUAL(size_t(0), pDoc->GetFlyCount(FLYCNTTYPE_FRM)); + CPPUNIT_ASSERT_EQUAL(size_t(0), pDoc->GetFlyCount(FLYCNTTYPE_GRF)); + CPPUNIT_ASSERT_EQUAL(sal_Int32(0), rIDMA.getAllMarksCount()); + dispatchCommand(mxComponent, ".uno:Undo", {}); + CPPUNIT_ASSERT_EQUAL(size_t(1), pDoc->GetFlyCount(FLYCNTTYPE_FRM)); + CPPUNIT_ASSERT_EQUAL(size_t(1), pDoc->GetFlyCount(FLYCNTTYPE_GRF)); + CPPUNIT_ASSERT_EQUAL(sal_Int32(1), rIDMA.getAllMarksCount()); + dispatchCommand(mxComponent, ".uno:Redo", {}); + CPPUNIT_ASSERT_EQUAL(size_t(0), pDoc->GetFlyCount(FLYCNTTYPE_FRM)); + CPPUNIT_ASSERT_EQUAL(size_t(0), pDoc->GetFlyCount(FLYCNTTYPE_GRF)); + CPPUNIT_ASSERT_EQUAL(sal_Int32(0), rIDMA.getAllMarksCount()); + dispatchCommand(mxComponent, ".uno:Undo", {}); + CPPUNIT_ASSERT_EQUAL(size_t(1), pDoc->GetFlyCount(FLYCNTTYPE_FRM)); + CPPUNIT_ASSERT_EQUAL(size_t(1), pDoc->GetFlyCount(FLYCNTTYPE_GRF)); + CPPUNIT_ASSERT_EQUAL(sal_Int32(1), rIDMA.getAllMarksCount()); + dispatchCommand(mxComponent, ".uno:Redo", {}); + CPPUNIT_ASSERT_EQUAL(size_t(0), pDoc->GetFlyCount(FLYCNTTYPE_FRM)); + CPPUNIT_ASSERT_EQUAL(size_t(0), pDoc->GetFlyCount(FLYCNTTYPE_GRF)); + CPPUNIT_ASSERT_EQUAL(sal_Int32(0), rIDMA.getAllMarksCount()); +} + CPPUNIT_TEST_FIXTURE(SwUiWriterTest2, testTdf124261) { #if !defined(_WIN32) diff --git a/sw/source/core/doc/DocumentRedlineManager.cxx b/sw/source/core/doc/DocumentRedlineManager.cxx index 69bbf3d7ac67..a3de2c9143fb 100644 --- a/sw/source/core/doc/DocumentRedlineManager.cxx +++ b/sw/source/core/doc/DocumentRedlineManager.cxx @@ -246,7 +246,8 @@ void UpdateFramesForAddDeleteRedline(SwDoc & rDoc, SwPaM const& rPam) void UpdateFramesForRemoveDeleteRedline(SwDoc & rDoc, SwPaM const& rPam) { - if (rDoc.IsClipBoard()) + // tdf#147006 fieldmark command may be empty => do not call AppendAllObjs() + if (rDoc.IsClipBoard() || *rPam.GetPoint() == *rPam.GetMark()) { return; } commit ea06852ee87531794f07710de496734a647a9062 Author: Michael Stahl <[email protected]> AuthorDate: Tue Feb 1 13:39:19 2022 +0100 Commit: Michael Stahl <[email protected]> CommitDate: Wed Feb 2 09:47:23 2022 +0100 tdf#147008 sw_fieldmarkhide: fix invalid NonTextFieldmark positions Commit ab6176e88f78d0b3aa2490fbc7858304c2d4a437 introduced a crash in ModelToViewHelper when the positions of a NonTextFieldmark are invalid. The NonTextFieldmark must always contain 1 CH_TXT_ATR_FORMELEMENT but after SplitNode() the position is (rr) p *pFieldMark->m_pPos1 $2 = SwPosition (node 10, offset 1) (rr) p *pFieldMark->m_pPos2 $3 = SwPosition (node 9, offset 0) This is because in ContentIdxStoreImpl::SaveBkmks() there is an asymmetry where the m_pPos2 is recorded to be wrongly corrected to node 9, but if the positions were swapped so that m_pPos1 is the start position, then it will not be recorded and remain in node 10. So fix this by changing the NonTextFieldmark to insert its CH_TXT_ATR_FORMELEMENT differently. There is some very subtle code in SwTextNode::Update() that is again asymmetric and (non-obviously) prefers to move m_pPos2 and leave m_pPos1 alone (by moving it to aTmpIdxReg) in case the positions are equal. But then the fieldmark code increments "rEnd" (which is really the m_pPos1 i.e. the start after InsertString() returns), and then decrements m_pPos2. So avoid the problem by removing these 2 pointless adjustments. Then it turns a bunch of tests fail because other code assumes that m_pPos1 is the end of the NonTextFieldmark, so fix MarkManager::changeFormFieldmarkType(), ModelToViewHelper and SwHistoryNoTextFieldmark to use GetMarkStart(). Change-Id: I7c82f9a67661121662c95727e0f8f15e06d85a3a Reviewed-on: https://gerrit.libreoffice.org/c/core/+/129289 Tested-by: Jenkins Reviewed-by: Michael Stahl <[email protected]> diff --git a/sw/qa/extras/uiwriter/uiwriter2.cxx b/sw/qa/extras/uiwriter/uiwriter2.cxx index 5ef95215178b..e0897a6f350f 100644 --- a/sw/qa/extras/uiwriter/uiwriter2.cxx +++ b/sw/qa/extras/uiwriter/uiwriter2.cxx @@ -3589,6 +3589,14 @@ CPPUNIT_TEST_FIXTURE(SwUiWriterTest2, testCheckboxFormFieldInsertion) pFieldmark = dynamic_cast<::sw::mark::IFieldmark*>(*aIter); CPPUNIT_ASSERT(pFieldmark); CPPUNIT_ASSERT_EQUAL(OUString(ODF_FORMCHECKBOX), pFieldmark->GetFieldname()); + + // tdf#147008 this would crash + SwWrtShell* pWrtShell = pDoc->GetDocShell()->GetWrtShell(); + pWrtShell->StartOfSection(false); + pWrtShell->SplitNode(); + CPPUNIT_ASSERT_EQUAL(pFieldmark->GetMarkPos().nNode, pFieldmark->GetOtherMarkPos().nNode); + CPPUNIT_ASSERT_EQUAL(sal_Int32(pFieldmark->GetMarkPos().nContent.GetIndex() + 1), + pFieldmark->GetOtherMarkPos().nContent.GetIndex()); } CPPUNIT_TEST_FIXTURE(SwUiWriterTest2, testDropDownFormFieldInsertion) diff --git a/sw/source/core/crsr/bookmark.cxx b/sw/source/core/crsr/bookmark.cxx index 2945b018bb8b..417558aad130 100644 --- a/sw/source/core/crsr/bookmark.cxx +++ b/sw/source/core/crsr/bookmark.cxx @@ -154,6 +154,10 @@ namespace SwPosition const sepPos(sw::mark::FindFieldSep(rField)); assert(sepPos.nNode.GetNode().GetTextNode()->GetText()[sepPos.nContent.GetIndex()] == CH_TXT_ATR_FIELDSEP); (void) sepPos; } + else + { // must be m_pPos1 < m_pPos2 because of asymmetric SplitNode update + assert(rField.GetMarkPos().nContent.GetIndex() + 1 == rField.GetOtherMarkPos().nContent.GetIndex()); + } SwPosition const& rEnd(rField.GetMarkEnd()); assert(rEnd.nNode.GetNode().GetTextNode()->GetText()[rEnd.nContent.GetIndex() - 1] == aEndMark); (void) rEnd; } @@ -207,7 +211,14 @@ namespace { SwPaM aEndPaM(rEnd); io_rDoc.getIDocumentContentOperations().InsertString(aEndPaM, OUString(aEndMark)); - ++rEnd.nContent; + if (aEndMark != CH_TXT_ATR_FORMELEMENT) + { + ++rEnd.nContent; // InsertString didn't move non-empty mark + } + else + { // InsertString moved the mark's end, not its start + assert(rField.GetMarkPos().nContent.GetIndex() + 1 == rField.GetOtherMarkPos().nContent.GetIndex()); + } } lcl_AssertFieldMarksSet(rField, aStartMark, aEndMark); @@ -590,12 +601,6 @@ namespace sw::mark if (eMode == sw::mark::InsertMode::New) { lcl_SetFieldMarks(*this, io_rDoc, CH_TXT_ATR_FIELDSTART, CH_TXT_ATR_FORMELEMENT, pSepPos); - - // For some reason the end mark is moved from 1 by the Insert: - // we don't want this for checkboxes - SwPosition aNewEndPos = GetMarkEnd(); - aNewEndPos.nContent--; - SetMarkEndPos( aNewEndPos ); } else { diff --git a/sw/source/core/doc/docbm.cxx b/sw/source/core/doc/docbm.cxx index 5ed676719622..c15c1dd3fcea 100644 --- a/sw/source/core/doc/docbm.cxx +++ b/sw/source/core/doc/docbm.cxx @@ -1419,8 +1419,7 @@ namespace sw::mark void MarkManager::deleteFieldmarkAt(const SwPosition& rPos) { auto const pFieldmark = dynamic_cast<Fieldmark*>(getFieldmarkAt(rPos)); - if (!pFieldmark) - return; + assert(pFieldmark); // currently all callers require it to be there deleteMark(lcl_FindMark(m_vAllMarks, pFieldmark)); } @@ -1455,12 +1454,11 @@ namespace sw::mark // Store attributes needed to create the new fieldmark OUString sName = pFieldmark->GetName(); - SwPaM aPaM(pFieldmark->GetMarkPos()); + SwPaM const aPaM(pFieldmark->GetMarkStart()); // Remove the old fieldmark and create a new one with the new type - if(aPaM.GetPoint()->nContent > 0 && (rNewType == ODF_FORMDROPDOWN || rNewType == ODF_FORMCHECKBOX)) + if (rNewType == ODF_FORMDROPDOWN || rNewType == ODF_FORMCHECKBOX) { - --aPaM.GetPoint()->nContent; SwPosition aNewPos (aPaM.GetPoint()->nNode, aPaM.GetPoint()->nContent); deleteFieldmarkAt(aNewPos); return makeNoTextFieldBookmark(aPaM, sName, rNewType); diff --git a/sw/source/core/txtnode/modeltoviewhelper.cxx b/sw/source/core/txtnode/modeltoviewhelper.cxx index 2572cf7d1a74..0ddcbcd8e545 100644 --- a/sw/source/core/txtnode/modeltoviewhelper.cxx +++ b/sw/source/core/txtnode/modeltoviewhelper.cxx @@ -294,7 +294,7 @@ ModelToViewHelper::ModelToViewHelper(const SwTextNode &rNode, for (sw::mark::IFieldmark *const pMark : aNoTextFieldmarks) { - const sal_Int32 nDummyCharPos = pMark->GetMarkPos().nContent.GetIndex()-1; + const sal_Int32 nDummyCharPos = pMark->GetMarkStart().nContent.GetIndex(); if (aHiddenMulti.IsSelected(nDummyCharPos)) continue; std::vector<block>::iterator aFind = std::find_if(aBlocks.begin(), aBlocks.end(), diff --git a/sw/source/core/undo/rolbck.cxx b/sw/source/core/undo/rolbck.cxx index 85f92be47670..49c4d9b1d946 100644 --- a/sw/source/core/undo/rolbck.cxx +++ b/sw/source/core/undo/rolbck.cxx @@ -729,8 +729,8 @@ bool SwHistoryBookmark::IsEqualBookmark(const ::sw::mark::IMark& rBkmk) SwHistoryNoTextFieldmark::SwHistoryNoTextFieldmark(const ::sw::mark::IFieldmark& rFieldMark) : SwHistoryHint(HSTRY_NOTEXTFIELDMARK) , m_sType(rFieldMark.GetFieldname()) - , m_nNode(rFieldMark.GetMarkPos().nNode.GetIndex()) - , m_nContent(rFieldMark.GetMarkPos().nContent.GetIndex()) + , m_nNode(rFieldMark.GetMarkStart().nNode.GetIndex()) + , m_nContent(rFieldMark.GetMarkStart().nContent.GetIndex()) { } @@ -760,8 +760,8 @@ void SwHistoryNoTextFieldmark::ResetInDoc(SwDoc& rDoc) std::optional<SwPaM> pPam; const SwContentNode* pContentNd = rNds[m_nNode]->GetContentNode(); - if(pContentNd) - pPam.emplace(*pContentNd, m_nContent-1); + assert(pContentNd); + pPam.emplace(*pContentNd, m_nContent); if (pPam) {
