sfx2: lok: reliably support multi-documents

Instead of using the current view to set
the DocId, we instead make sure that the ShellView
object has the DocId set at construction time.
This turned out to be necessary in at least one
case (which has a unit-test that failed), which
is when events fired during the creation of a
new view. The cursor position is notified
before we have a chance to set the DocId and
because of that we miss the notifications (or
worse, we end up sending them to all other
documents' views in an effort to fix this bug).

This approach is clean and always guarantees that
all views have the correct DocId set as soon as
possible and that all notifications are sent
as expected.

A unit-test is added to exercise mult-document
usage, which exposed a number of bugs and issues
that have been addressed in this patch.

Change-Id: Icf5145fb1dabd0d029368310c2b9bf73ae927ccc
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/99975
Tested-by: Jenkins CollaboraOffice <jenkinscollaboraoffice@gmail.com>
Reviewed-by: Michael Meeks <michael.meeks@collabora.com>
Reviewed-by: Ashod Nakashian <ash@collabora.com>
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/106252
Tested-by: Jenkins
Reviewed-by: Tor Lillqvist <tml@collabora.com>

3 files changed, 124 insertions, 15 deletions

diff --git a/desktop/inc/lib/init.hxx b/desktop/inc/lib/init.hxx
index a71ecae79f83..84d9d5299fd4 100644
--- a/desktop/inc/lib/init.hxx
+++ b/desktop/inc/lib/init.hxx
@@ -150,7 +150,8 @@ namespace desktop {
         std::map<size_t, std::shared_ptr<CallbackFlushHandler>> mpCallbackFlushHandlers;
         const int mnDocumentId;
 
-        explicit LibLODocument_Impl(const css::uno::Reference <css::lang::XComponent> &xComponent, int nDocumentId = -1);
+        explicit LibLODocument_Impl(const css::uno::Reference<css::lang::XComponent>& xComponent,
+                                    int nDocumentId);
         ~LibLODocument_Impl();
     };
 
diff --git a/desktop/qa/desktop_lib/test_desktop_lib.cxx b/desktop/qa/desktop_lib/test_desktop_lib.cxx
index a02347450a38..0a19d5582e80 100644
--- a/desktop/qa/desktop_lib/test_desktop_lib.cxx
+++ b/desktop/qa/desktop_lib/test_desktop_lib.cxx
@@ -195,6 +195,7 @@ public:
     void testCalcSaveAs();
     void testControlState();
     void testMetricField();
+    void testMultiDocuments();
     void testABI();
 
     CPPUNIT_TEST_SUITE(DesktopLOKTest);
@@ -257,6 +258,7 @@ public:
     CPPUNIT_TEST(testCalcSaveAs);
     CPPUNIT_TEST(testControlState);
     CPPUNIT_TEST(testMetricField);
+    CPPUNIT_TEST(testMultiDocuments);
     CPPUNIT_TEST(testABI);
     CPPUNIT_TEST_SUITE_END();
 
@@ -322,10 +324,13 @@ DesktopLOKTest::loadDocUrlImpl(const OUString& rFileURL, LibreOfficeKitDocumentT
         break;
     }
 
+    static int nDocumentIdCounter = 0;
+    SfxViewShell::SetCurrentDocId(ViewShellDocId(nDocumentIdCounter));
     uno::Reference<lang::XComponent> xComponent = loadFromDesktop(rFileURL, aService);
     CPPUNIT_ASSERT(xComponent.is());
 
-    std::unique_ptr<LibLODocument_Impl> pDocument(new LibLODocument_Impl(xComponent));
+    std::unique_ptr<LibLODocument_Impl> pDocument(new LibLODocument_Impl(xComponent, nDocumentIdCounter));
+    ++nDocumentIdCounter;
 
     return std::make_pair(std::move(pDocument), xComponent);
 }
@@ -2933,6 +2938,99 @@ void DesktopLOKTest::testSpellcheckerMultiView()
     CPPUNIT_ASSERT_EQUAL(1, pDocument->m_pDocumentClass->getViewsCount(pDocument));
 }
 
+void DesktopLOKTest::testMultiDocuments()
+{
+    // Load a document.
+    uno::Reference<lang::XComponent> xComponent1;
+    std::unique_ptr<LibLODocument_Impl> document1;
+    std::tie(document1, xComponent1) = loadDocImpl("blank_text.odt");
+    LibLODocument_Impl* pDocument1 = document1.get();
+    CPPUNIT_ASSERT_EQUAL(1, pDocument1->m_pDocumentClass->getViewsCount(pDocument1));
+    const int nDocId1 = pDocument1->mnDocumentId;
+
+    const int nDoc1View0 = pDocument1->m_pDocumentClass->getView(pDocument1);
+    CPPUNIT_ASSERT_EQUAL(nDocId1, SfxLokHelper::getDocumentIdOfView(nDoc1View0));
+    const int nDoc1View1 = pDocument1->m_pDocumentClass->createView(pDocument1);
+    CPPUNIT_ASSERT_EQUAL(nDoc1View1, pDocument1->m_pDocumentClass->getView(pDocument1));
+    CPPUNIT_ASSERT_EQUAL(nDocId1, SfxLokHelper::getDocumentIdOfView(nDoc1View1));
+    CPPUNIT_ASSERT_EQUAL(2, pDocument1->m_pDocumentClass->getViewsCount(pDocument1));
+
+    // Validate the views of document 1.
+    std::vector<int> aViewIdsDoc1(2);
+    CPPUNIT_ASSERT(pDocument1->m_pDocumentClass->getViewIds(pDocument1, aViewIdsDoc1.data(), aViewIdsDoc1.size()));
+    CPPUNIT_ASSERT_EQUAL(nDoc1View0, aViewIdsDoc1[0]);
+    CPPUNIT_ASSERT_EQUAL(nDoc1View1, aViewIdsDoc1[1]);
+
+    CPPUNIT_ASSERT_EQUAL(nDoc1View1, pDocument1->m_pDocumentClass->getView(pDocument1));
+    CPPUNIT_ASSERT_EQUAL(nDocId1, SfxLokHelper::getDocumentIdOfView(nDoc1View1));
+    pDocument1->m_pDocumentClass->setView(pDocument1, nDoc1View0);
+    CPPUNIT_ASSERT_EQUAL(nDoc1View0, pDocument1->m_pDocumentClass->getView(pDocument1));
+    CPPUNIT_ASSERT_EQUAL(nDocId1, SfxLokHelper::getDocumentIdOfView(nDoc1View0));
+    pDocument1->m_pDocumentClass->setView(pDocument1, nDoc1View1);
+    CPPUNIT_ASSERT_EQUAL(nDoc1View1, pDocument1->m_pDocumentClass->getView(pDocument1));
+    CPPUNIT_ASSERT_EQUAL(nDocId1, SfxLokHelper::getDocumentIdOfView(nDoc1View1));
+    CPPUNIT_ASSERT_EQUAL(2, pDocument1->m_pDocumentClass->getViewsCount(pDocument1));
+
+    // Load another document.
+    uno::Reference<lang::XComponent> xComponent2;
+    std::unique_ptr<LibLODocument_Impl> document2;
+    std::tie(document2, xComponent2) = loadDocImpl("blank_presentation.odp");
+    LibLODocument_Impl* pDocument2 = document2.get();
+    CPPUNIT_ASSERT_EQUAL(1, pDocument2->m_pDocumentClass->getViewsCount(pDocument2));
+    const int nDocId2 = pDocument2->mnDocumentId;
+
+    const int nDoc2View0 = pDocument2->m_pDocumentClass->getView(pDocument2);
+    CPPUNIT_ASSERT_EQUAL(nDocId2, SfxLokHelper::getDocumentIdOfView(nDoc2View0));
+    const int nDoc2View1 = pDocument2->m_pDocumentClass->createView(pDocument2);
+    CPPUNIT_ASSERT_EQUAL(nDoc2View1, pDocument2->m_pDocumentClass->getView(pDocument2));
+    CPPUNIT_ASSERT_EQUAL(nDocId2, SfxLokHelper::getDocumentIdOfView(nDoc2View1));
+    CPPUNIT_ASSERT_EQUAL(2, pDocument2->m_pDocumentClass->getViewsCount(pDocument2));
+
+    // Validate the views of document 2.
+    std::vector<int> aViewIdsDoc2(2);
+    CPPUNIT_ASSERT(pDocument2->m_pDocumentClass->getViewIds(pDocument2, aViewIdsDoc2.data(), aViewIdsDoc2.size()));
+    CPPUNIT_ASSERT_EQUAL(nDoc2View0, aViewIdsDoc2[0]);
+    CPPUNIT_ASSERT_EQUAL(nDoc2View1, aViewIdsDoc2[1]);
+
+    CPPUNIT_ASSERT_EQUAL(nDoc2View1, pDocument2->m_pDocumentClass->getView(pDocument2));
+    CPPUNIT_ASSERT_EQUAL(nDocId2, SfxLokHelper::getDocumentIdOfView(nDoc2View1));
+    pDocument2->m_pDocumentClass->setView(pDocument2, nDoc2View0);
+    CPPUNIT_ASSERT_EQUAL(nDoc2View0, pDocument2->m_pDocumentClass->getView(pDocument2));
+    CPPUNIT_ASSERT_EQUAL(nDocId2, SfxLokHelper::getDocumentIdOfView(nDoc2View0));
+    pDocument2->m_pDocumentClass->setView(pDocument2, nDoc2View1);
+    CPPUNIT_ASSERT_EQUAL(nDoc2View1, pDocument2->m_pDocumentClass->getView(pDocument2));
+    CPPUNIT_ASSERT_EQUAL(nDocId2, SfxLokHelper::getDocumentIdOfView(nDoc2View1));
+    CPPUNIT_ASSERT_EQUAL(2, pDocument2->m_pDocumentClass->getViewsCount(pDocument2));
+
+    // The views of document1 should be unchanged.
+    CPPUNIT_ASSERT(pDocument1->m_pDocumentClass->getViewIds(pDocument1, aViewIdsDoc1.data(), aViewIdsDoc1.size()));
+    CPPUNIT_ASSERT_EQUAL(nDoc1View0, aViewIdsDoc1[0]);
+    CPPUNIT_ASSERT_EQUAL(nDoc1View1, aViewIdsDoc1[1]);
+    // Switch views in the first doc.
+    CPPUNIT_ASSERT_EQUAL(nDocId1, SfxLokHelper::getDocumentIdOfView(nDoc1View0));
+    pDocument1->m_pDocumentClass->setView(pDocument1, nDoc1View0);
+    CPPUNIT_ASSERT_EQUAL(nDoc1View0, pDocument1->m_pDocumentClass->getView(pDocument1));
+    CPPUNIT_ASSERT_EQUAL(nDocId1, SfxLokHelper::getDocumentIdOfView(nDoc1View1));
+    pDocument1->m_pDocumentClass->destroyView(pDocument1, nDoc1View1);
+    CPPUNIT_ASSERT_EQUAL(1, pDocument1->m_pDocumentClass->getViewsCount(pDocument1));
+
+    // The views of document2 should be unchanged.
+    CPPUNIT_ASSERT(pDocument2->m_pDocumentClass->getViewIds(pDocument2, aViewIdsDoc2.data(), aViewIdsDoc2.size()));
+    CPPUNIT_ASSERT_EQUAL(nDoc2View0, aViewIdsDoc2[0]);
+    CPPUNIT_ASSERT_EQUAL(nDoc2View1, aViewIdsDoc2[1]);
+    // Switch views in the second doc.
+    CPPUNIT_ASSERT_EQUAL(nDocId2, SfxLokHelper::getDocumentIdOfView(nDoc2View0));
+    pDocument2->m_pDocumentClass->setView(pDocument2, nDoc2View0);
+    CPPUNIT_ASSERT_EQUAL(nDoc2View0, pDocument2->m_pDocumentClass->getView(pDocument2));
+    CPPUNIT_ASSERT_EQUAL(nDocId2, SfxLokHelper::getDocumentIdOfView(nDoc2View1));
+    pDocument2->m_pDocumentClass->destroyView(pDocument2, nDoc2View1);
+    CPPUNIT_ASSERT_EQUAL(1, pDocument2->m_pDocumentClass->getViewsCount(pDocument2));
+
+    closeDoc(document2, xComponent2);
+
+    closeDoc(document1, xComponent1);
+}
+
 void DesktopLOKTest::testControlState()
 {
     LibLODocument_Impl* pDocument = loadDoc("search.ods");
diff --git a/desktop/source/lib/init.cxx b/desktop/source/lib/init.cxx
index 6fe7347e9b70..9bcfff524bbf 100644
--- a/desktop/source/lib/init.cxx
+++ b/desktop/source/lib/init.cxx
@@ -1229,6 +1229,8 @@ LibLODocument_Impl::LibLODocument_Impl(const uno::Reference <css::lang::XCompone
     : mxComponent(xComponent)
     , mnDocumentId(nDocumentId)
 {
+    assert(nDocumentId != -1 && "Cannot set mnDocumentId to -1");
+
     m_pDocumentClass = gDocumentClass.lock();
     if (!m_pDocumentClass)
     {
@@ -2292,7 +2294,6 @@ static LibreOfficeKitDocument* lo_documentLoadWithOptions(LibreOfficeKit* pThis,
         LibLODocument_Impl* pDocument = new LibLODocument_Impl(xComponent, nDocumentIdCounter++);
 
         // After loading the document, its initial view is the "current" view.
-        SfxLokHelper::setDocumentIdOfView(pDocument->mnDocumentId);
         if (pLib->mpCallback)
         {
             int nState = doc_getSignatureState(pDocument);
@@ -3358,13 +3359,13 @@ static void doc_registerCallback(LibreOfficeKitDocument* pThis,
 
     LibLODocument_Impl* pDocument = static_cast<LibLODocument_Impl*>(pThis);
 
-    int nView = SfxLokHelper::getView();
+    const int nView = SfxLokHelper::getView();
     if (nView < 0)
         return;
 
+    const size_t nId = nView;
     if (pCallback != nullptr)
     {
-        size_t nId = nView;
         for (auto& pair : pDocument->mpCallbackFlushHandlers)
         {
             if (pair.first == nId)
@@ -3375,7 +3376,6 @@ static void doc_registerCallback(LibreOfficeKitDocument* pThis,
     }
     else
     {
-        size_t nId = nView;
         for (auto& pair : pDocument->mpCallbackFlushHandlers)
         {
             if (pair.first == nId)
@@ -3389,7 +3389,6 @@ static void doc_registerCallback(LibreOfficeKitDocument* pThis,
 
     if (pCallback != nullptr)
     {
-        size_t nId = nView;
         for (const auto& pair : pDocument->mpCallbackFlushHandlers)
         {
             if (pair.first == nId)
@@ -3397,11 +3396,19 @@ static void doc_registerCallback(LibreOfficeKitDocument* pThis,
 
             pDocument->mpCallbackFlushHandlers[nView]->addViewStates(pair.first);
         }
-    }
 
-    if (SfxViewShell* pViewShell = SfxViewShell::Current())
+        if (SfxViewShell* pViewShell = SfxViewShell::Current())
+        {
+            pViewShell->registerLibreOfficeKitViewCallback(
+                CallbackFlushHandler::callback, pDocument->mpCallbackFlushHandlers[nView].get());
+        }
+    }
+    else
     {
-        pViewShell->registerLibreOfficeKitViewCallback(CallbackFlushHandler::callback, pDocument->mpCallbackFlushHandlers[nView].get());
+        if (SfxViewShell* pViewShell = SfxViewShell::Current())
+        {
+            pViewShell->registerLibreOfficeKitViewCallback(nullptr, nullptr);
+        }
     }
 }
 
@@ -5145,7 +5152,8 @@ static int doc_createViewWithOptions(LibreOfficeKitDocument* pThis,
     const OUString aDeviceFormFactor = extractParameter(aOptions, "DeviceFormFactor");
     SfxLokHelper::setDeviceFormFactor(aDeviceFormFactor);
 
-    int nId = SfxLokHelper::createView();
+    LibLODocument_Impl* pDocument = static_cast<LibLODocument_Impl*>(pThis);
+    const int nId = SfxLokHelper::createView(pDocument->mnDocumentId);
 
 #ifdef IOS
     (void) pThis;
@@ -5193,24 +5201,26 @@ static int doc_getView(SAL_UNUSED_PARAMETER LibreOfficeKitDocument* /*pThis*/)
     return SfxLokHelper::getView();
 }
 
-static int doc_getViewsCount(SAL_UNUSED_PARAMETER LibreOfficeKitDocument* /*pThis*/)
+static int doc_getViewsCount(SAL_UNUSED_PARAMETER LibreOfficeKitDocument* pThis)
 {
     comphelper::ProfileZone aZone("doc_getViewsCount");
 
     SolarMutexGuard aGuard;
     SetLastExceptionMsg();
 
-    return SfxLokHelper::getViewsCount();
+    LibLODocument_Impl* pDocument = static_cast<LibLODocument_Impl*>(pThis);
+    return SfxLokHelper::getViewsCount(pDocument->mnDocumentId);
 }
 
-static bool doc_getViewIds(SAL_UNUSED_PARAMETER LibreOfficeKitDocument* /*pThis*/, int* pArray, size_t nSize)
+static bool doc_getViewIds(SAL_UNUSED_PARAMETER LibreOfficeKitDocument* pThis, int* pArray, size_t nSize)
 {
     comphelper::ProfileZone aZone("doc_getViewsIds");
 
     SolarMutexGuard aGuard;
     SetLastExceptionMsg();
 
-    return SfxLokHelper::getViewIds(pArray, nSize);
+    LibLODocument_Impl* pDocument = static_cast<LibLODocument_Impl*>(pThis);
+    return SfxLokHelper::getViewIds(pDocument->mnDocumentId, pArray, nSize);
 }
 
 static void doc_setViewLanguage(SAL_UNUSED_PARAMETER LibreOfficeKitDocument* /*pThis*/, int nId, const char* language)