sw: revert ToX hyperlinks to LO 4.3 behaviour

Commit 94b296d5416dd71d721ad16216b50bce79e3dc04 changed this to
potentially insert multiple overlapping hyperlink items.

Unfortunately Writer can only have one RES_TXTATR_INETFMT on any given
position in the paragraph, so the hyperlink hints inevitably overwrite
each other.

Revert this to do it the same way as the old code in LO 4.3 did: match
the last unmatched link-start with the first link-end, and ignore all
the link-start before the matching one, as well as the link-end after
the matching one.

This should prevent surprising formatting changes on index update,
including entries spontaneously turning green, and there is no reason
why the result of the new way is objectively better than the old one.

Change-Id: I55be9c212c473908428fa8bd6487d136343fe852
(cherry picked from commit 68f7be01a78a5bad56ddd94c04764131676d6cc0)
Reviewed-on: https://gerrit.libreoffice.org/33552
Tested-by: Jenkins <ci@libreoffice.org>
Reviewed-by: Caolán McNamara <caolanm@redhat.com>
Tested-by: Caolán McNamara <caolanm@redhat.com>

3 files changed, 27 insertions, 31 deletions

diff --git a/sw/inc/ToxLinkProcessor.hxx b/sw/inc/ToxLinkProcessor.hxx
index 7fceef1050d5..78bb7fb30d49 100644
--- a/sw/inc/ToxLinkProcessor.hxx
+++ b/sw/inc/ToxLinkProcessor.hxx
@@ -82,7 +82,7 @@ private:
 
     std::vector<std::unique_ptr<ClosedLink>> m_ClosedLinks;
 
-    std::vector<std::unique_ptr<StartedLink>> m_StartedLinks;
+    std::unique_ptr<StartedLink> m_pStartedLink;
 
     friend class ::ToxLinkProcessorTest;
 };
diff --git a/sw/qa/core/test_ToxLinkProcessor.cxx b/sw/qa/core/test_ToxLinkProcessor.cxx
index eb4618e1e8c8..d2d4721dae70 100644
--- a/sw/qa/core/test_ToxLinkProcessor.cxx
+++ b/sw/qa/core/test_ToxLinkProcessor.cxx
@@ -30,17 +30,15 @@ using namespace sw;
 class ToxLinkProcessorTest : public test::BootstrapFixture
 {
     void NoExceptionIsThrownIfTooManyLinksAreClosed();
-    void AddingAndClosingTwoLinksResultsInTwoClosedLinks();
+    void AddingAndClosingTwoOverlappingLinksResultsInOneClosedLink();
     void LinkIsCreatedCorrectly();
     void LinkSequenceIsPreserved();
-    void StandardOpenLinkIsAddedWhenMoreLinksThanAvaiableAreClosed();
 
     CPPUNIT_TEST_SUITE(ToxLinkProcessorTest);
     CPPUNIT_TEST(NoExceptionIsThrownIfTooManyLinksAreClosed);
-    CPPUNIT_TEST(AddingAndClosingTwoLinksResultsInTwoClosedLinks);
+    CPPUNIT_TEST(AddingAndClosingTwoOverlappingLinksResultsInOneClosedLink);
     CPPUNIT_TEST(LinkIsCreatedCorrectly);
     CPPUNIT_TEST(LinkSequenceIsPreserved);
-    CPPUNIT_TEST(StandardOpenLinkIsAddedWhenMoreLinksThanAvaiableAreClosed);
     CPPUNIT_TEST_SUITE_END();
 public:
     void setUp() override {
@@ -71,30 +69,28 @@ ToxLinkProcessorTest::NoExceptionIsThrownIfTooManyLinksAreClosed()
     sut.CloseLink(1, URL_1);
     // fdo#85872 actually it turns out the UI does something like this
     // so an exception must not be thrown!
-    sut.CloseLink(1, URL_1);
+    // should not succeed either (for backward compatibility)
+    sut.CloseLink(2, URL_1);
+    CPPUNIT_ASSERT_EQUAL(1u, static_cast<unsigned>(sut.m_ClosedLinks.size()));
+    CPPUNIT_ASSERT_EQUAL(1u, static_cast<unsigned>(sut.m_ClosedLinks.at(0)->mEndTextPos));
+    CPPUNIT_ASSERT_MESSAGE("no links are open", sut.m_pStartedLink == nullptr);
 }
 
 void
-ToxLinkProcessorTest::StandardOpenLinkIsAddedWhenMoreLinksThanAvaiableAreClosed()
-{
-    ToxLinkProcessor sut;
-    sut.StartNewLink(0, STYLE_NAME_1);
-    sut.CloseLink(1, URL_1);
-    sut.CloseLink(1, URL_1);
-    CPPUNIT_ASSERT_EQUAL(2u, static_cast<unsigned>(sut.m_ClosedLinks.size()));
-    CPPUNIT_ASSERT_EQUAL(1u, static_cast<unsigned>(sut.m_ClosedLinks.at(1)->mEndTextPos));
-}
-
-void
-ToxLinkProcessorTest::AddingAndClosingTwoLinksResultsInTwoClosedLinks()
+ToxLinkProcessorTest::AddingAndClosingTwoOverlappingLinksResultsInOneClosedLink()
 {
     ToxLinkProcessor sut;
     sut.StartNewLink(0, STYLE_NAME_1);
     sut.StartNewLink(0, STYLE_NAME_2);
     sut.CloseLink(1, URL_1);
+    // this should not cause an error, and should not succeed either
+    // (for backward compatibility)
     sut.CloseLink(1, URL_2);
-    CPPUNIT_ASSERT_EQUAL(2u, static_cast<unsigned>(sut.m_ClosedLinks.size()));
-    CPPUNIT_ASSERT_MESSAGE("no links are open", sut.m_StartedLinks.empty());
+    CPPUNIT_ASSERT_EQUAL(1u, static_cast<unsigned>(sut.m_ClosedLinks.size()));
+    CPPUNIT_ASSERT_MESSAGE("no links are open", sut.m_pStartedLink == nullptr);
+    // backward compatibility: the last start is closed by the first end
+    CPPUNIT_ASSERT_EQUAL(STYLE_NAME_2, sut.m_ClosedLinks[0]->mINetFormat.GetINetFormat());
+    CPPUNIT_ASSERT_EQUAL(URL_1, sut.m_ClosedLinks[0]->mINetFormat.GetValue());
 }
 
 class ToxLinkProcessorWithOverriddenObtainPoolId : public ToxLinkProcessor {
@@ -131,10 +127,10 @@ ToxLinkProcessorTest::LinkSequenceIsPreserved()
     // obtainpoolid needs to be overridden to check what we are
     ToxLinkProcessorWithOverriddenObtainPoolId sut;
 
-    sut.StartNewLink(0, STYLE_NAME_1);
     sut.StartNewLink(0, STYLE_NAME_2);
     sut.CloseLink(1, URL_2);
-    sut.CloseLink(1, URL_1);
+    sut.StartNewLink(1, STYLE_NAME_1);
+    sut.CloseLink(2, URL_1);
 
     // check first closed element
     CPPUNIT_ASSERT_EQUAL_MESSAGE("Style is stored correctly in link",
diff --git a/sw/source/core/tox/ToxLinkProcessor.cxx b/sw/source/core/tox/ToxLinkProcessor.cxx
index 1ef40d271625..f9fa4c1a6238 100644
--- a/sw/source/core/tox/ToxLinkProcessor.cxx
+++ b/sw/source/core/tox/ToxLinkProcessor.cxx
@@ -21,19 +21,18 @@ namespace sw {
 void
 ToxLinkProcessor::StartNewLink(sal_Int32 startPosition, const OUString& characterStyle)
 {
-    m_StartedLinks.push_back(o3tl::make_unique<StartedLink>(
-                startPosition, characterStyle));
+    SAL_INFO_IF(m_pStartedLink, "sw.core", "ToxLinkProcessor: LS without LE");
+    m_pStartedLink = o3tl::make_unique<StartedLink>(
+                startPosition, characterStyle);
 }
 
 void
 ToxLinkProcessor::CloseLink(sal_Int32 endPosition, const OUString& url)
 {
-    StartedLink const startedLink( (m_StartedLinks.empty())
-        ? StartedLink(0, SW_RES(STR_POOLCHR_TOXJUMP))
-        : *m_StartedLinks.back() );
-    if (!m_StartedLinks.empty())
+    if (m_pStartedLink == nullptr)
     {
-        m_StartedLinks.pop_back();
+        SAL_INFO("sw.core", "ToxLinkProcessor: LE without LS");
+        return;
     }
 
     if (url.isEmpty()) {
@@ -41,14 +40,15 @@ ToxLinkProcessor::CloseLink(sal_Int32 endPosition, const OUString& url)
     }
 
     std::unique_ptr<ClosedLink> pClosedLink(
-            new ClosedLink(url, startedLink.mStartPosition, endPosition));
+            new ClosedLink(url, m_pStartedLink->mStartPosition, endPosition));
 
-    const OUString& characterStyle = startedLink.mCharacterStyle;
+    const OUString& characterStyle = m_pStartedLink->mCharacterStyle;
     sal_uInt16 poolId = ObtainPoolId(characterStyle);
     pClosedLink->mINetFormat.SetVisitedFormatAndId(characterStyle, poolId);
     pClosedLink->mINetFormat.SetINetFormatAndId(characterStyle, poolId);
 
     m_ClosedLinks.push_back(std::move(pClosedLink));
+    m_pStartedLink.reset();
 }
 
 sal_uInt16