From d83a82d872417a0c50d38c54f95af512136f6d94 Mon Sep 17 00:00:00 2001 From: Vasily Melenchuk Date: Mon, 27 Dec 2021 13:54:23 +0300 Subject: tdf#143703 sw: always assign name for fly section Previously generated name was assigned only if not in doc reading mode. But there is no guarantee that it will be assigned later. Better to insert any name in SwDoc::MakeFlySection_() and later it can be overwritten, but fly will definitely have any unique name. * Some test failed because GraphicImport_Impl::applyName() overwrote the name with a different generated one. * This breaks chaining of VML shapes, see test testTDF87348. The code introduced in commit 091fe76b6329b4bb974987554369cbfadd8f2401 in DomainMapper_Impl::ChainTextFrames() breaks if the text frame already has a name; it's a bit confusing which names there come from the file and which come from the API, and it also mixes 2 different cases of VML chaining and DrawingML chaining that look like they should be using different data. * This also breaks moving flys anchored at-char in flys into them in SwXText::convertToTextFrame(), see ooxmlexport13 testFlyInFly. This kind of worked by accident before: the fly is copied and then the original deleted, keeping the same name (with help of SwDoc::mbCopyIsMove); with no name it would compare the SdrObject pointer, which is different for the new copy, now the name is the same. Fix this by only moving flys anchored at the edge of the selection back inoto the body; it turns out that Word actually supports at-char anchors in text frames, but only if it's a VML shape or Compatibility Mode or whatever; i wasn't able to do it in a document created from scratch. This is a bit tricky to ignore the nodes added for floating tables as seen in ooxmlexport10 testFloatingTablesAnchor. * Another change is required in SwDoc::SetFlyName() because of testTdf127732, as it would rename a frame named "Frame1" to "Frame2" when called to rename it to "Frame1". * Some tests failed because after MakeFlySection_() assigns a name it is immediately unconditionally overwritten; replace that with asserts Reviewed-on: https://gerrit.libreoffice.org/c/core/+/127556 Tested-by: Jenkins Reviewed-by: Michael Stahl (cherry picked from commit 4d6243693c228703394c00164276f8326447beb9) Change-Id: I46752a4413ba3a9e981eccd1e153b3aaf8053781 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/137766 Tested-by: Thorsten Behrens Reviewed-by: Thorsten Behrens --- sw/qa/extras/ooxmlexport/ooxmlexport13.cxx | 4 +- sw/source/core/doc/doclay.cxx | 11 +++-- sw/source/core/doc/textboxhelper.cxx | 3 +- sw/source/core/unocore/unotext.cxx | 21 +++++++--- writerfilter/source/dmapper/DomainMapper_Impl.cxx | 49 ++++++++++++----------- writerfilter/source/dmapper/GraphicImport.cxx | 11 ++--- 6 files changed, 58 insertions(+), 41 deletions(-) diff --git a/sw/qa/extras/ooxmlexport/ooxmlexport13.cxx b/sw/qa/extras/ooxmlexport/ooxmlexport13.cxx index 8f457f89bc16..62ad331a8c64 100644 --- a/sw/qa/extras/ooxmlexport/ooxmlexport13.cxx +++ b/sw/qa/extras/ooxmlexport/ooxmlexport13.cxx @@ -44,11 +44,11 @@ DECLARE_SW_EXPORT_TEST(testFlyInFly, "ooo39250-1-min.rtf", nullptr, Test) // check that anchor of text frame is in other text frame uno::Reference const xAnchored(getShape(3), uno::UNO_QUERY); CPPUNIT_ASSERT(xAnchored.is()); - CPPUNIT_ASSERT_EQUAL(OUString(""), uno::Reference(xAnchored, uno::UNO_QUERY_THROW)->getName()); + CPPUNIT_ASSERT_EQUAL(OUString("Frame1")/*generated name*/, uno::Reference(xAnchored, uno::UNO_QUERY_THROW)->getName()); uno::Reference const xAnchorText(xAnchored->getAnchor()->getText()); uno::Reference const xAnchorFrame(xAnchorText, uno::UNO_QUERY); CPPUNIT_ASSERT(xAnchorFrame.is()); - CPPUNIT_ASSERT_EQUAL(OUString("Frame2"), uno::Reference(xAnchorFrame, uno::UNO_QUERY_THROW)->getName()); + CPPUNIT_ASSERT_EQUAL(OUString("Frame3"), uno::Reference(xAnchorFrame, uno::UNO_QUERY_THROW)->getName()); } DECLARE_OOXMLEXPORT_TEST(testTdf125778_lostPageBreakTOX, "tdf125778_lostPageBreakTOX.docx") diff --git a/sw/source/core/doc/doclay.cxx b/sw/source/core/doc/doclay.cxx index ec4861fe39b2..5b3dc0ef3687 100644 --- a/sw/source/core/doc/doclay.cxx +++ b/sw/source/core/doc/doclay.cxx @@ -157,13 +157,12 @@ SwFlyFrameFormat* SwDoc::MakeFlySection_( const SwPosition& rAnchPos, pFrameFormat = getIDocumentStylePoolAccess().GetFrameFormatFromPool( RES_POOLFRM_FRAME ); OUString sName; - if( !mbInReading ) - switch( rNode.GetNodeType() ) - { + switch( rNode.GetNodeType() ) + { case SwNodeType::Grf: sName = GetUniqueGrfName(); break; case SwNodeType::Ole: sName = GetUniqueOLEName(); break; default: sName = GetUniqueFrameName(); break; - } + } SwFlyFrameFormat* pFormat = MakeFlyFrameFormat( sName, pFrameFormat ); // Create content and connect to the format. @@ -1408,6 +1407,10 @@ const SwFlyFrameFormat* SwDoc::FindFlyByName( const OUString& rName, SwNodeType void SwDoc::SetFlyName( SwFlyFrameFormat& rFormat, const OUString& rName ) { + if (rFormat.GetName() == rName) + { + return; + } OUString sName( rName ); if( sName.isEmpty() || FindFlyByName( sName ) ) { diff --git a/sw/source/core/doc/textboxhelper.cxx b/sw/source/core/doc/textboxhelper.cxx index ea57aa58319d..40d1bb607c1e 100644 --- a/sw/source/core/doc/textboxhelper.cxx +++ b/sw/source/core/doc/textboxhelper.cxx @@ -90,7 +90,8 @@ void SwTextBoxHelper::create(SwFrameFormat* pShape) xPropertySet->setPropertyValue(UNO_NAME_SURROUND, uno::makeAny(text::WrapTextMode_THROUGH)); uno::Reference xNamed(xTextFrame, uno::UNO_QUERY); - xNamed->setName(pShape->GetDoc()->GetUniqueFrameName()); + assert(!xNamed->getName().isEmpty()); + (void)xNamed; // Link its text range to the original shape. uno::Reference xTextBox(xTextFrame, uno::UNO_QUERY_THROW); diff --git a/sw/source/core/unocore/unotext.cxx b/sw/source/core/unocore/unotext.cxx index 3887a11191d7..a4dfe5422af1 100644 --- a/sw/source/core/unocore/unotext.cxx +++ b/sw/source/core/unocore/unotext.cxx @@ -1566,6 +1566,8 @@ SwXText::convertToTextFrame( } bool bParaAfterInserted = false; bool bParaBeforeInserted = false; + ::std::optional oAnchorCheckPam; + oAnchorCheckPam.emplace(*pStartPam->Start(), *pEndPam->End()); if ( pStartStartNode && pEndStartNode && (pStartStartNode != pEndStartNode || pStartStartNode != GetStartNode()) @@ -1646,6 +1648,7 @@ SwXText::convertToTextFrame( bParaAfterInserted = GetDoc()->getIDocumentContentOperations().AppendTextNode( aEnd ); pEndPam->DeleteMark(); *pEndPam->GetPoint() = aEnd; + *oAnchorCheckPam->End() = aEnd; } pStartPam->SetMark(); *pStartPam->End() = *pEndPam->End(); @@ -1660,10 +1663,17 @@ SwXText::convertToTextFrame( { const SwFrameFormat* pFrameFormat = (*m_pImpl->m_pDoc->GetSpzFrameFormats())[i]; const SwFormatAnchor& rAnchor = pFrameFormat->GetAnchor(); - if ( !isGraphicNode(pFrameFormat) && - (RndStdIds::FLY_AT_PARA == rAnchor.GetAnchorId() || RndStdIds::FLY_AT_CHAR == rAnchor.GetAnchorId()) && - pStartPam->Start()->nNode.GetIndex() <= rAnchor.GetContentAnchor()->nNode.GetIndex() && - pStartPam->End()->nNode.GetIndex() >= rAnchor.GetContentAnchor()->nNode.GetIndex()) + // note: Word can do at-char anchors in text frames - sometimes! + // see testFlyInFly for why this checks only the edges of the selection, + // and testFloatingTablesAnchor for why it excludes pre/post table + // added nodes + if (!isGraphicNode(pFrameFormat) + && ( (RndStdIds::FLY_AT_PARA == rAnchor.GetAnchorId() + && ( oAnchorCheckPam->Start()->nNode.GetIndex() == rAnchor.GetContentAnchor()->nNode.GetIndex() + || oAnchorCheckPam->End()->nNode.GetIndex() == rAnchor.GetContentAnchor()->nNode.GetIndex())) + || (RndStdIds::FLY_AT_CHAR == rAnchor.GetAnchorId() + && ( *oAnchorCheckPam->Start() == *rAnchor.GetContentAnchor() + || *oAnchorCheckPam->End() == *rAnchor.GetContentAnchor())))) { if (pFrameFormat->GetName().isEmpty()) { @@ -1675,6 +1685,7 @@ SwXText::convertToTextFrame( } } } + oAnchorCheckPam.reset(); // clear SwIndex before deleting nodes const uno::Reference xNewFrame( SwXTextFrame::CreateXTextFrame(*m_pImpl->m_pDoc, nullptr)); @@ -1692,7 +1703,7 @@ SwXText::convertToTextFrame( new SwXTextRange(*pStartPam, this); assert(rNewFrame.IsDescriptor()); rNewFrame.attachToRange(xInsertTextRange, pStartPam.get()); - rNewFrame.setName(m_pImpl->m_pDoc->GetUniqueFrameName()); + assert(!rNewFrame.getName().isEmpty()); } SwTextNode *const pTextNode(pStartPam->GetNode().GetTextNode()); diff --git a/writerfilter/source/dmapper/DomainMapper_Impl.cxx b/writerfilter/source/dmapper/DomainMapper_Impl.cxx index 329999ca18ce..0aec8482e121 100644 --- a/writerfilter/source/dmapper/DomainMapper_Impl.cxx +++ b/writerfilter/source/dmapper/DomainMapper_Impl.cxx @@ -3309,14 +3309,15 @@ void DomainMapper_Impl::ChainTextFrames() sal_Int32 nId; sal_Int32 nSeq; OUString s_mso_next_textbox; - bool bShapeNameSet; - TextFramesForChaining(): nId(0), nSeq(0), bShapeNameSet(false) {} + OUString shapeName; + TextFramesForChaining() : nId(0), nSeq(0) {} } ; typedef std::map ChainMap; try { ChainMap aTextFramesForChainingHelper; + ::std::vector chainingWPS; OUString sChainNextName("ChainNextName"); //learn about ALL of the textboxes and their chaining values first - because frames are processed in no specific order. @@ -3354,19 +3355,22 @@ void DomainMapper_Impl::ChainTextFrames() //Sometimes the shape names have not been imported. If not, we may have a fallback name. //Set name later, only if required for linking. - if( sShapeName.isEmpty() ) - aChainStruct.bShapeNameSet = false; - else - { - aChainStruct.bShapeNameSet = true; - sLinkChainName = sShapeName; - } + aChainStruct.shapeName = sShapeName; - if( !sLinkChainName.isEmpty() ) + if (!sLinkChainName.isEmpty()) { aChainStruct.xShape = rTextFrame; aTextFramesForChainingHelper[sLinkChainName] = aChainStruct; } + if (aChainStruct.s_mso_next_textbox.isEmpty()) + { // no VML chaining => try to chain DrawingML via IDs + aChainStruct.xShape = rTextFrame; + if (!sLinkChainName.isEmpty()) + { // for member of group shapes, TestTdf73499 + aChainStruct.shapeName = sLinkChainName; + } + chainingWPS.emplace_back(aChainStruct); + } } //if mso-next-textbox tags are provided, create those vml-style links first. Afterwards we will make dml-style id/seq links. @@ -3381,22 +3385,22 @@ void DomainMapper_Impl::ChainTextFrames() if( nextFinder != aTextFramesForChainingHelper.end() ) { //if the frames have no name yet, then set them. LinkDisplayName / ChainName are read-only. - if( !msoItem.second.bShapeNameSet ) + if (msoItem.second.shapeName.isEmpty()) { uno::Reference< container::XNamed > xNamed( msoItem.second.xShape, uno::UNO_QUERY ); if ( xNamed.is() ) { xNamed->setName( msoItem.first ); - msoItem.second.bShapeNameSet = true; + msoItem.second.shapeName = msoItem.first; } } - if( !nextFinder->second.bShapeNameSet ) + if (nextFinder->second.shapeName.isEmpty()) { uno::Reference< container::XNamed > xNamed( nextFinder->second.xShape, uno::UNO_QUERY ); if ( xNamed.is() ) { xNamed->setName( nextFinder->first ); - nextFinder->second.bShapeNameSet = true; + nextFinder->second.shapeName = msoItem.first; } } @@ -3404,7 +3408,7 @@ void DomainMapper_Impl::ChainTextFrames() uno::Reference xPropertySet(xTextContent, uno::UNO_QUERY); //The reverse chaining happens automatically, so only one direction needs to be set - xPropertySet->setPropertyValue(sChainNextName, uno::makeAny(nextFinder->first)); + xPropertySet->setPropertyValue(sChainNextName, uno::makeAny(nextFinder->second.shapeName)); //the last item in an mso-next-textbox chain is indistinguishable from id/seq items. Now that it is handled, remove it. if( nextFinder->second.s_mso_next_textbox.isEmpty() ) @@ -3417,26 +3421,23 @@ void DomainMapper_Impl::ChainTextFrames() const sal_Int32 nDirection = 1; //Finally - go through and attach the chains based on matching ID and incremented sequence number (dml-style). - for (const auto& rOuter : aTextFramesForChainingHelper) + for (const auto& rOuter : chainingWPS) { - if( rOuter.second.s_mso_next_textbox.isEmpty() ) //non-empty ones already handled earlier - so skipping them now. - { - for (const auto& rInner : aTextFramesForChainingHelper) + for (const auto& rInner : chainingWPS) { - if ( rInner.second.nId == rOuter.second.nId ) + if (rInner.nId == rOuter.nId) { - if ( rInner.second.nSeq == ( rOuter.second.nSeq + nDirection ) ) + if (rInner.nSeq == (rOuter.nSeq + nDirection)) { - uno::Reference xTextContent(rOuter.second.xShape, uno::UNO_QUERY_THROW); + uno::Reference const xTextContent(rOuter.xShape, uno::UNO_QUERY_THROW); uno::Reference xPropertySet(xTextContent, uno::UNO_QUERY); //The reverse chaining happens automatically, so only one direction needs to be set - xPropertySet->setPropertyValue(sChainNextName, uno::makeAny(rInner.first)); + xPropertySet->setPropertyValue(sChainNextName, uno::makeAny(rInner.shapeName)); break ; //there cannot be more than one next frame } } } - } } m_vTextFramesForChaining.clear(); //clear the vector } diff --git a/writerfilter/source/dmapper/GraphicImport.cxx b/writerfilter/source/dmapper/GraphicImport.cxx index b69f4565fb13..ea369ae858fc 100644 --- a/writerfilter/source/dmapper/GraphicImport.cxx +++ b/writerfilter/source/dmapper/GraphicImport.cxx @@ -367,11 +367,12 @@ public: { try { - // Ask the graphic naming helper to find out the name for this - // object: It's around till the end of the import, so it remembers - // what's the first free name. - uno::Reference< container::XNamed > xNamed( xGraphicObjectProperties, uno::UNO_QUERY_THROW ); - xNamed->setName(rDomainMapper.GetGraphicNamingHelper().NameGraphic(sName)); + if (!sName.isEmpty()) + { + uno::Reference const xNamed(xGraphicObjectProperties, uno::UNO_QUERY_THROW); + xNamed->setName(sName); + } + // else: name is automatically generated by SwDoc::MakeFlySection_() if ( sHyperlinkURL.getLength() > 0 ) xGraphicObjectProperties->setPropertyValue(getPropertyName( PROP_HYPER_LINK_U_R_L ), -- cgit v1.2.3