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>

3 files changed, 94 insertions, 46 deletions

diff --git a/sfx2/source/view/lokhelper.cxx b/sfx2/source/view/lokhelper.cxx
index 864523e50fd6..4928dd3102e9 100644
--- a/sfx2/source/view/lokhelper.cxx
+++ b/sfx2/source/view/lokhelper.cxx
@@ -55,7 +55,10 @@ public:
         --m_nDisabled;
     }
 
-    static bool disabled() { return m_nDisabled != 0; }
+    static inline bool disabled()
+    {
+        return !comphelper::LibreOfficeKit::isActive() || m_nDisabled != 0;
+    }
 
 private:
     static int m_nDisabled;
@@ -70,28 +73,57 @@ static LanguageTag g_defaultLanguageTag("en-US", true);
 static LOKDeviceFormFactor g_deviceFormFactor = LOKDeviceFormFactor::UNKNOWN;
 }
 
-int SfxLokHelper::createView()
+int SfxLokHelper::createView(SfxViewFrame* pViewFrame, ViewShellDocId docId)
 {
-    SfxViewFrame* pViewFrame = SfxViewFrame::GetFirst();
+    assert(docId >= ViewShellDocId(0) && "Cannot createView for invalid (negative) DocId.");
+
     if (pViewFrame == nullptr)
         return -1;
-    SfxViewShell* pPrevViewShell = SfxViewShell::Current();
-    ViewShellDocId nId;
-    if (pPrevViewShell)
-        nId = pPrevViewShell->GetDocId();
+
+    SfxViewShell::SetCurrentDocId(docId);
     SfxRequest aRequest(pViewFrame, SID_NEWWINDOW);
     pViewFrame->ExecView_Impl(aRequest);
     SfxViewShell* pViewShell = SfxViewShell::Current();
     if (pViewShell == nullptr)
         return -1;
-    if (pPrevViewShell)
-        pViewShell->SetDocId(nId);
+
+    assert(pViewShell->GetDocId() == docId && "DocId must be already set!");
     return static_cast<sal_Int32>(pViewShell->GetViewShellId());
 }
 
+int SfxLokHelper::createView()
+{
+    // Assumes a single document, or at least that the
+    // current view belongs to the document on which the
+    // view will be created.
+    SfxViewShell* pViewShell = SfxViewShell::Current();
+    if (pViewShell == nullptr)
+        return -1;
+
+    return createView(pViewShell->GetViewFrame(), pViewShell->GetDocId());
+}
+
+int SfxLokHelper::createView(int nDocId)
+{
+    const SfxApplication* pApp = SfxApplication::Get();
+    if (pApp == nullptr)
+        return -1;
+
+    // Find a shell with the given DocId.
+    const ViewShellDocId docId(nDocId);
+    for (const SfxViewShell* pViewShell : pApp->GetViewShells_Impl())
+    {
+        if (pViewShell->GetDocId() == docId)
+            return createView(pViewShell->GetViewFrame(), docId);
+    }
+
+    // No frame with nDocId found.
+    return -1;
+}
+
 void SfxLokHelper::destroyView(int nId)
 {
-    SfxApplication* pApp = SfxApplication::Get();
+    const SfxApplication* pApp = SfxApplication::Get();
     if (pApp == nullptr)
         return;
 
@@ -156,56 +188,52 @@ int SfxLokHelper::getView(const SfxViewShell* pViewShell)
     return static_cast<sal_Int32>(pViewShell->GetViewShellId());
 }
 
-std::size_t SfxLokHelper::getViewsCount()
+std::size_t SfxLokHelper::getViewsCount(int nDocId)
 {
+    assert(nDocId != -1 && "Cannot getViewsCount for invalid DocId -1");
+
     SfxApplication* pApp = SfxApplication::Get();
     if (!pApp)
         return 0;
 
-    const SfxViewShell* const pCurrentViewShell = SfxViewShell::Current();
-    const ViewShellDocId nCurrentDocId = pCurrentViewShell ? pCurrentViewShell->GetDocId() : ViewShellDocId(-1);
+    const ViewShellDocId nCurrentDocId(nDocId);
     std::size_t n = 0;
     SfxViewShell* pViewShell = SfxViewShell::GetFirst();
     while (pViewShell)
     {
-        if (pViewShell->GetDocId() == nCurrentDocId)
-            n++;
+        n += (pViewShell->GetDocId() == nCurrentDocId);
         pViewShell = SfxViewShell::GetNext(*pViewShell);
     }
+
     return n;
 }
 
-bool SfxLokHelper::getViewIds(int* pArray, size_t nSize)
+bool SfxLokHelper::getViewIds(int nDocId, int* pArray, size_t nSize)
 {
+    assert(nDocId != -1 && "Cannot getViewsIds for invalid DocId -1");
+
     SfxApplication* pApp = SfxApplication::Get();
     if (!pApp)
         return false;
 
-    const SfxViewShell* const pCurrentViewShell = SfxViewShell::Current();
-    const ViewShellDocId nCurrentDocId = pCurrentViewShell ? pCurrentViewShell->GetDocId() : ViewShellDocId(-1);
+    const ViewShellDocId nCurrentDocId(nDocId);
     std::size_t n = 0;
     SfxViewShell* pViewShell = SfxViewShell::GetFirst();
     while (pViewShell)
     {
-        if (n == nSize)
-            return false;
         if (pViewShell->GetDocId() == nCurrentDocId)
         {
+            if (n == nSize)
+                return false;
+
             pArray[n] = static_cast<sal_Int32>(pViewShell->GetViewShellId());
             n++;
         }
+
         pViewShell = SfxViewShell::GetNext(*pViewShell);
     }
-    return true;
-}
 
-void SfxLokHelper::setDocumentIdOfView(int nId)
-{
-    SfxViewShell* pViewShell = SfxViewShell::Current();
-    assert(pViewShell);
-    if (!pViewShell)
-        return;
-    pViewShell->SetDocId(ViewShellDocId(nId));
+    return true;
 }
 
 int SfxLokHelper::getDocumentIdOfView(int nViewId)
@@ -335,14 +363,25 @@ void SfxLokHelper::notifyOtherView(const SfxViewShell* pThisView, SfxViewShell c
 void SfxLokHelper::notifyOtherViews(const SfxViewShell* pThisView, int nType, const OString& rKey,
                                     const OString& rPayload)
 {
+    assert(pThisView != nullptr && "pThisView must be valid");
     if (DisableCallbacks::disabled())
         return;
 
+    // Cache the payload so we only have to generate it once, at most.
+    OString aPayload;
+
+    const ViewShellDocId nCurrentDocId = pThisView->GetDocId();
     SfxViewShell* pViewShell = SfxViewShell::GetFirst();
     while (pViewShell)
     {
-        if (pViewShell != pThisView && (pThisView->GetDocId() == ViewShellDocId(-1) || pViewShell->GetDocId() == pThisView->GetDocId()))
-            notifyOtherView(pThisView, pViewShell, nType, rKey, rPayload);
+        if (pViewShell != pThisView && nCurrentDocId == pViewShell->GetDocId())
+        {
+            // Payload is only dependent on pThisView.
+            if (aPayload.isEmpty())
+                aPayload = lcl_generateJSON(pThisView, rKey, rPayload);
+
+            pViewShell->libreOfficeKitViewCallback(nType, aPayload.getStr());
+        }
 
         pViewShell = SfxViewShell::GetNext(*pViewShell);
     }
@@ -351,17 +390,25 @@ void SfxLokHelper::notifyOtherViews(const SfxViewShell* pThisView, int nType, co
 void SfxLokHelper::notifyOtherViews(const SfxViewShell* pThisView, int nType,
                                     const boost::property_tree::ptree& rTree)
 {
-    if (SfxLokHelper::getViewsCount() <= 1 || DisableCallbacks::disabled())
+    assert(pThisView != nullptr && "pThisView must be valid");
+    if (DisableCallbacks::disabled())
         return;
 
-    // Payload is only dependent on pThisView.
-    OString aPayload = lcl_generateJSON(pThisView, rTree);
+    // Cache the payload so we only have to generate it once, at most.
+    OString aPayload;
 
+    const ViewShellDocId nCurrentDocId = pThisView->GetDocId();
     SfxViewShell* pViewShell = SfxViewShell::GetFirst();
     while (pViewShell)
     {
-        if (pViewShell != pThisView)
+        if (pViewShell != pThisView && nCurrentDocId == pViewShell->GetDocId())
+        {
+            // Payload is only dependent on pThisView.
+            if (aPayload.isEmpty())
+                aPayload = lcl_generateJSON(pThisView, rTree);
+
             pViewShell->libreOfficeKitViewCallback(nType, aPayload.getStr());
+        }
 
         pViewShell = SfxViewShell::GetNext(*pViewShell);
     }
@@ -415,7 +462,7 @@ void SfxLokHelper::notifyWindow(const SfxViewShell* pThisView,
 {
     assert(pThisView != nullptr && "pThisView must be valid");
 
-    if (SfxLokHelper::getViewsCount() <= 0 || nLOKWindowId == 0 || DisableCallbacks::disabled())
+    if (nLOKWindowId == 0 || DisableCallbacks::disabled())
         return;
 
     OStringBuffer aPayload;
@@ -454,7 +501,7 @@ void SfxLokHelper::notifyInvalidation(SfxViewShell const* pThisView, const OStri
 
 void SfxLokHelper::notifyDocumentSizeChanged(SfxViewShell const* pThisView, const OString& rPayload, vcl::ITiledRenderable* pDoc, bool bInvalidateAll)
 {
-    if (!pDoc || pDoc->isDisposed() || !comphelper::LibreOfficeKit::isActive() || DisableCallbacks::disabled())
+    if (!pDoc || pDoc->isDisposed() || DisableCallbacks::disabled())
         return;
 
     if (bInvalidateAll)
@@ -471,7 +518,7 @@ void SfxLokHelper::notifyDocumentSizeChanged(SfxViewShell const* pThisView, cons
 
 void SfxLokHelper::notifyDocumentSizeChangedAllViews(vcl::ITiledRenderable* pDoc, bool bInvalidateAll)
 {
-    if (!comphelper::LibreOfficeKit::isActive() || DisableCallbacks::disabled())
+    if (DisableCallbacks::disabled())
         return;
 
     // FIXME: Do we know whether it is the views for the document that is in the "current" view that has changed?
diff --git a/sfx2/source/view/viewimp.hxx b/sfx2/source/view/viewimp.hxx
index d89a49c79154..d2c39965a225 100644
--- a/sfx2/source/view/viewimp.hxx
+++ b/sfx2/source/view/viewimp.hxx
@@ -58,9 +58,9 @@ struct SfxViewShell_Impl
     bool m_bTiledSearching;
     static sal_uInt32 m_nLastViewShellId;
     const ViewShellId m_nViewShellId;
-    ViewShellDocId m_nDocId;
+    const ViewShellDocId m_nDocId;
 
-    explicit SfxViewShell_Impl(SfxViewShellFlags const nFlags);
+    explicit SfxViewShell_Impl(SfxViewShellFlags const nFlags, ViewShellDocId nDocId);
     ~SfxViewShell_Impl();
 
     std::vector< SfxInPlaceClient* >* GetIPClients_Impl(bool bCreate = true) const;
diff --git a/sfx2/source/view/viewsh.cxx b/sfx2/source/view/viewsh.cxx
index b9abc8833573..14cbd2c10a08 100644
--- a/sfx2/source/view/viewsh.cxx
+++ b/sfx2/source/view/viewsh.cxx
@@ -231,7 +231,7 @@ void SAL_CALL SfxClipboardChangeListener::changedContents( const datatransfer::c
 
 sal_uInt32 SfxViewShell_Impl::m_nLastViewShellId = 0;
 
-SfxViewShell_Impl::SfxViewShell_Impl(SfxViewShellFlags const nFlags)
+SfxViewShell_Impl::SfxViewShell_Impl(SfxViewShellFlags const nFlags, ViewShellDocId nDocId)
 : aInterceptorContainer( aMutex )
 ,   m_bHasPrintOptions(nFlags & SfxViewShellFlags::HAS_PRINTOPTIONS)
 ,   m_nFamily(0xFFFF)   // undefined, default set by TemplateDialog
@@ -239,7 +239,7 @@ SfxViewShell_Impl::SfxViewShell_Impl(SfxViewShellFlags const nFlags)
 ,   m_pLibreOfficeKitViewData(nullptr)
 ,   m_bTiledSearching(false)
 ,   m_nViewShellId(SfxViewShell_Impl::m_nLastViewShellId++)
-,   m_nDocId(-1)
+,   m_nDocId(nDocId)
 {
 }
 
@@ -1069,6 +1069,7 @@ void SfxViewShell::SetWindow
     //SfxGetpApp()->GrabFocus( pWindow );
 }
 
+ViewShellDocId SfxViewShell::mnCurrentDocId(0);
 
 SfxViewShell::SfxViewShell
 (
@@ -1078,7 +1079,7 @@ SfxViewShell::SfxViewShell
 )
 
 :   SfxShell(this)
-,   pImpl( new SfxViewShell_Impl(nFlags) )
+,   pImpl( new SfxViewShell_Impl(nFlags, SfxViewShell::mnCurrentDocId) )
 ,   pFrame(pViewFrame)
 ,   pWindow(nullptr)
 ,   bNoNewWindow( nFlags & SfxViewShellFlags::NO_NEWWINDOW )
@@ -1553,14 +1554,14 @@ ViewShellId SfxViewShell::GetViewShellId() const
     return pImpl->m_nViewShellId;
 }
 
-void SfxViewShell::SetDocId(ViewShellDocId nId)
+void SfxViewShell::SetCurrentDocId(ViewShellDocId nId)
 {
-    assert(static_cast<int>(pImpl->m_nDocId) == -1);
-    pImpl->m_nDocId = nId;
+    mnCurrentDocId = nId;
 }
 
 ViewShellDocId SfxViewShell::GetDocId() const
 {
+    assert(pImpl->m_nDocId >= ViewShellDocId(0) && "m_nDocId should have been initialized, but it is invalid.");
     return pImpl->m_nDocId;
 }