LOK: conditionally include part number in invalidation payload

Since desktop/ code queues, compresses and only emits callbacks on idle,
it's possible that two invalidations are in the queue, and there was a
setPart() call between them. In this case it's impossible to tell what
part the invalidation was sent for.

Fix this by conditionally including the part number in the invalidation
payload. It's off by default, a new feature flag is added to request
this behavior.

gtktiledviewer enables this feature flag by default, though just to show
the part number in the debug output. Android doesn't enable it.

Change-Id: I73e6def848c0eb61d64e71026002c7a0e750aab4

comphelper/source/misc/lok.cxx

@@ -17,7 +17,7 @@ namespace LibreOfficeKit
 
 static bool g_bActive(false);
 
-static bool g_bViewCallback(true);
+static bool g_bPartInInvalidation(false);
 
 void setActive(bool bActive)
 {
@@ -29,14 +29,14 @@ bool isActive()
     return g_bActive;
 }
 
-void setViewCallback(bool bViewCallback)
+void setPartInInvalidation(bool bPartInInvalidation)
 {
-    g_bViewCallback = bViewCallback;
+    g_bPartInInvalidation = bPartInInvalidation;
 }
 
-bool isViewCallback()
+bool isPartInInvalidation()
 {
-    return g_bViewCallback;
+    return g_bPartInInvalidation;
 }
 
 static bool g_bLocalRendering(false);

desktop/qa/desktop_lib/test_desktop_lib.cxx

@@ -33,6 +33,7 @@
 #include <sfx2/viewfrm.hxx>
 #include <sfx2/bindings.hxx>
 #include <comphelper/string.hxx>
+#include <comphelper/scopeguard.hxx>
 #include <cairo.h>
 
 #include <lib/init.hxx>
@@ -97,6 +98,7 @@ public:
     void testContextMenuWriter();
     void testContextMenuImpress();
     void testNotificationCompression();
+    void testPartInInvalidation();
     void testRedlineWriter();
     void testTrackChanges();
     void testRedlineCalc();
@@ -129,6 +131,7 @@ public:
     CPPUNIT_TEST(testContextMenuWriter);
     CPPUNIT_TEST(testContextMenuImpress);
     CPPUNIT_TEST(testNotificationCompression);
+    CPPUNIT_TEST(testPartInInvalidation);
     CPPUNIT_TEST(testRedlineWriter);
     CPPUNIT_TEST(testTrackChanges);
     CPPUNIT_TEST(testRedlineCalc);
@@ -1427,6 +1430,77 @@ void DesktopLOKTest::testNotificationCompression()
     CPPUNIT_ASSERT_EQUAL(std::string("1"), std::get<1>(notifs[i++]));
 }
 
+void DesktopLOKTest::testPartInInvalidation()
+{
+    LibLODocument_Impl* pDocument = loadDoc("blank_text.odt");
+    // No part in invalidation: merge.
+    {
+        std::vector<std::tuple<int, std::string>> notifs;
+        std::unique_ptr<CallbackFlushHandler> handler(new CallbackFlushHandler(pDocument, callbackCompressionTest, &notifs));
+
+        handler->queue(LOK_CALLBACK_INVALIDATE_TILES, "10, 10, 20, 10");
+        handler->queue(LOK_CALLBACK_INVALIDATE_TILES, "20, 10, 20, 10");
+
+        Scheduler::ProcessEventsToIdle();
+
+        CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(1), notifs.size());
+
+        CPPUNIT_ASSERT_EQUAL((int)LOK_CALLBACK_INVALIDATE_TILES, (int)std::get<0>(notifs[0]));
+        CPPUNIT_ASSERT_EQUAL(std::string("10, 10, 30, 10"), std::get<1>(notifs[0]));
+    }
+    // No part in invalidation: don't merge.
+    {
+        std::vector<std::tuple<int, std::string>> notifs;
+        std::unique_ptr<CallbackFlushHandler> handler(new CallbackFlushHandler(pDocument, callbackCompressionTest, &notifs));
+
+        handler->queue(LOK_CALLBACK_INVALIDATE_TILES, "10, 10, 20, 10");
+        handler->queue(LOK_CALLBACK_INVALIDATE_TILES, "40, 10, 20, 10");
+
+        Scheduler::ProcessEventsToIdle();
+
+        CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(2), notifs.size());
+    }
+
+    // Part in invalidation, intersection and parts match -> merge.
+    {
+        comphelper::LibreOfficeKit::setPartInInvalidation(true);
+        comphelper::ScopeGuard aGuard([]()
+        {
+            comphelper::LibreOfficeKit::setPartInInvalidation(false);
+        });
+
+        std::vector<std::tuple<int, std::string>> notifs;
+        std::unique_ptr<CallbackFlushHandler> handler(new CallbackFlushHandler(pDocument, callbackCompressionTest, &notifs));
+
+        handler->queue(LOK_CALLBACK_INVALIDATE_TILES, "10, 10, 20, 10, 0");
+        handler->queue(LOK_CALLBACK_INVALIDATE_TILES, "20, 10, 20, 10, 0");
+
+        Scheduler::ProcessEventsToIdle();
+
+        CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(1), notifs.size());
+    }
+    // Part in invalidation, intersection and parts don't match -> don't merge.
+    {
+        comphelper::LibreOfficeKit::setPartInInvalidation(true);
+        comphelper::ScopeGuard aGuard([]()
+        {
+            comphelper::LibreOfficeKit::setPartInInvalidation(false);
+        });
+
+        std::vector<std::tuple<int, std::string>> notifs;
+        std::unique_ptr<CallbackFlushHandler> handler(new CallbackFlushHandler(pDocument, callbackCompressionTest, &notifs));
+
+        handler->queue(LOK_CALLBACK_INVALIDATE_TILES, "10, 10, 20, 10, 0");
+        handler->queue(LOK_CALLBACK_INVALIDATE_TILES, "20, 10, 20, 10, 1");
+
+        Scheduler::ProcessEventsToIdle();
+
+        // This failed as RectangleAndPart::Create() always assumed no part in
+        // payload, so this was merged -> it was 1.
+        CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(2), notifs.size());
+    }
+}
+
 void DesktopLOKTest::testRedlineWriter()
 {
     // Load a Writer document, enable change recording and press a key.

desktop/source/lib/init.cxx

@@ -326,15 +326,42 @@ static boost::property_tree::ptree unoAnyToPropertyTree(const uno::Any& anyItem)
 
 namespace {
 
-Rectangle lcl_ParseRect(const std::string& payload)
+/// Represents an invalidated rectangle inside a given document part.
+struct RectangleAndPart
 {
-    std::istringstream iss(payload);
-    long left, top, right, bottom;
-    char comma;
-    iss >> left >> comma >> top >> comma >> right >> comma >> bottom;
-    Rectangle rc(left, top, left + right, top + bottom);
-    return rc;
-}
+    Rectangle m_aRectangle;
+    int m_nPart;
+
+    RectangleAndPart()
+        : m_nPart(-1)
+    {
+    }
+
+    OString toString() const
+    {
+        std::stringstream ss;
+        ss << m_aRectangle.toString().getStr();
+        if (m_nPart != -1)
+            ss << ", " << m_nPart;
+        return ss.str().c_str();
+    }
+
+    static RectangleAndPart Create(const std::string& rPayload)
+    {
+        std::istringstream aStream(rPayload);
+        long nLeft, nTop, nRight, nBottom;
+        long nPart = -1;
+        char nComma;
+        if (comphelper::LibreOfficeKit::isPartInInvalidation())
+            aStream >> nLeft >> nComma >> nTop >> nComma >> nRight >> nComma >> nBottom >> nComma >> nPart;
+        else
+            aStream >> nLeft >> nComma >> nTop >> nComma >> nRight >> nComma >> nBottom;
+        RectangleAndPart aRet;
+        aRet.m_aRectangle = Rectangle(nLeft, nTop, nLeft + nRight, nTop + nBottom);
+        aRet.m_nPart = nPart;
+        return aRet;
+    }
+};
 
 bool lcl_isViewCallbackType(const int type)
 {
@@ -700,7 +727,7 @@ void CallbackFlushHandler::queue(const int type, const char* data)
 
             case LOK_CALLBACK_INVALIDATE_TILES:
             {
-                Rectangle rcNew = lcl_ParseRect(payload);
+                RectangleAndPart rcNew = RectangleAndPart::Create(payload);
                 //SAL_WARN("lok", "New: " << rcNew.toString());
                 const auto rcOrig = rcNew;
 
@@ -708,15 +735,17 @@ void CallbackFlushHandler::queue(const int type, const char* data)
                     [type, &rcNew] (const queue_type::value_type& elem) {
                         if (elem.first == type)
                         {
-                            const Rectangle rcOld = lcl_ParseRect(elem.second);
+                            const RectangleAndPart rcOld = RectangleAndPart::Create(elem.second);
+                            if (rcOld.m_nPart != rcNew.m_nPart)
+                                return false;
                             //SAL_WARN("lok", "#" << i << " Old: " << rcOld.toString());
-                            const Rectangle rcOverlap = rcNew.GetIntersection(rcOld);
+                            const Rectangle rcOverlap = rcNew.m_aRectangle.GetIntersection(rcOld.m_aRectangle);
                             //SAL_WARN("lok", "#" << i << " Overlap: " << rcOverlap.toString());
                             bool bOverlap = (rcOverlap.GetWidth() > 0 && rcOverlap.GetHeight() > 0);
                             if (bOverlap)
                             {
                                 //SAL_WARN("lok", rcOld.toString() << " U " << rcNew.toString());
-                                rcNew.Union(rcOld);
+                                rcNew.m_aRectangle.Union(rcOld.m_aRectangle);
                             }
                             return bOverlap;
                         }
@@ -727,7 +756,7 @@ void CallbackFlushHandler::queue(const int type, const char* data)
                     }
                 );
 
-                if (rcNew != rcOrig)
+                if (rcNew.m_aRectangle != rcOrig.m_aRectangle)
                 {
                     SAL_WARN("lok", "Replacing: " << rcOrig.toString() << " by " << rcNew.toString());
                     payload = rcNew.toString().getStr();
@@ -2442,6 +2471,8 @@ static void lo_setOptionalFeatures(LibreOfficeKit* pThis, uint64_t const feature
 {
     LibLibreOffice_Impl *const pLib = static_cast<LibLibreOffice_Impl*>(pThis);
     pLib->mOptionalFeatures = features;
+    if (features & LOK_FEATURE_PART_IN_INVALIDATION_CALLBACK)
+        comphelper::LibreOfficeKit::setPartInInvalidation(true);
 }
 
 static void lo_setDocumentPassword(LibreOfficeKit* pThis,

include/LibreOfficeKit/LibreOfficeKitEnums.h

@@ -63,6 +63,12 @@ typedef enum
      * @see lok::Office::setDocumentPassword().
      */
     LOK_FEATURE_DOCUMENT_PASSWORD_TO_MODIFY = (1ULL << 1),
+
+    /**
+     * Request to have the part number as an 5th value in the
+     * LOK_CALLBACK_INVALIDATE_TILES payload.
+     */
+    LOK_FEATURE_PART_IN_INVALIDATION_CALLBACK = (1ULL << 2),
 }
 LibreOfficeKitOptionalFeatures;
 
@@ -84,6 +90,8 @@ typedef enum
      * Rectangle format: "x, y, width, height", where all numbers are document
      * coordinates, in twips. When all tiles are supposed to be dropped, the
      * format is the "EMPTY" string.
+     *
+     * @see LOK_FEATURE_PART_IN_INVALIDATION_CALLBACK.
      */
     LOK_CALLBACK_INVALIDATE_TILES,
     /**

include/comphelper/lok.hxx

@@ -43,10 +43,10 @@ COMPHELPER_DLLPUBLIC bool isActive();
 COMPHELPER_DLLPUBLIC void setLocalRendering(bool bLocalRendering = true);
 COMPHELPER_DLLPUBLIC bool isLocalRendering();
 
-/// Check whether clients register a callback for each view.
-COMPHELPER_DLLPUBLIC bool isViewCallback();
-/// Set whether clients register a callback for each view.
-COMPHELPER_DLLPUBLIC void setViewCallback(bool bViewCallback);
+/// Check whether clients want a part number in an invalidation payload.
+COMPHELPER_DLLPUBLIC bool isPartInInvalidation();
+/// Set whether clients want a part number in an invalidation payload.
+COMPHELPER_DLLPUBLIC void setPartInInvalidation(bool bPartInInvalidation);
 
 // Status indicator handling. Even if in theory there could be several status indicators active at
 // the same time, in practice there is only one at a time, so we don't handle any identification of

include/sfx2/lokhelper.hxx

@@ -36,6 +36,8 @@ public:
     static void notifyOtherViews(SfxViewShell* pThisView, int nType, const OString& rKey, const OString& rPayload);
     /// Same as notifyOtherViews(), but works on a selected "other" view, not on all of them.
     static void notifyOtherView(SfxViewShell* pThisView, SfxViewShell* pOtherView, int nType, const OString& rKey, const OString& rPayload);
+    /// Emits a LOK_CALLBACK_INVALIDATE_TILES, but tweaks it according to setOptionalFeatures() if needed.
+    static void notifyInvalidation(SfxViewShell* pThisView, const OString& rPayload);
 };
 
 #endif

libreofficekit/source/gtk/lokdocview.cxx

@@ -1039,7 +1039,8 @@ payloadToRectangle (LOKDocView* pDocView, const char* pPayload)
 {
     LOKDocViewPrivate& priv = getPrivate(pDocView);
     GdkRectangle aRet;
-    gchar** ppCoordinates = g_strsplit(pPayload, ", ", 4);
+    // x, y, width, height, part number.
+    gchar** ppCoordinates = g_strsplit(pPayload, ", ", 5);
     gchar** ppCoordinate = ppCoordinates;
 
     aRet.width = aRet.height = aRet.x = aRet.y = 0;
@@ -2622,6 +2623,8 @@ static gboolean lok_doc_view_initable_init (GInitable *initable, GCancellable* /
                      priv->m_aLOPath);
         return FALSE;
     }
+    priv->m_nLOKFeatures |= LOK_FEATURE_PART_IN_INVALIDATION_CALLBACK;
+    priv->m_pOffice->pClass->setOptionalFeatures(priv->m_pOffice, priv->m_nLOKFeatures);
 
     return TRUE;
 }

sc/source/ui/view/gridwin4.cxx

@@ -31,6 +31,7 @@
 
 #include <LibreOfficeKit/LibreOfficeKitEnums.h>
 #include <comphelper/lok.hxx>
+#include <sfx2/lokhelper.hxx>
 
 #include <svx/svdview.hxx>
 #include "tabvwsh.hxx"
@@ -1154,7 +1155,7 @@ void ScGridWindow::LogicInvalidate(const Rectangle* pRectangle)
     }
 
     ScTabViewShell* pViewShell = pViewData->GetViewShell();
-    pViewShell->libreOfficeKitViewCallback(LOK_CALLBACK_INVALIDATE_TILES, sRectangle.getStr());
+    SfxLokHelper::notifyInvalidation(pViewShell, sRectangle);
 }
 
 void ScGridWindow::SetCellSelectionPixel(int nType, int nPixelX, int nPixelY)

sc/source/ui/view/tabview3.cxx

@@ -65,6 +65,7 @@
 #include <formula/FormulaCompiler.hxx>
 #include <comphelper/lok.hxx>
 #include <LibreOfficeKit/LibreOfficeKitEnums.h>
+#include <sfx2/lokhelper.hxx>
 
 #include <com/sun/star/chart2/data/HighlightedRange.hpp>
 
@@ -340,13 +341,13 @@ void ScTabView::SetCursor( SCCOL nPosX, SCROW nPosY, bool bNew )
                     // Only invalidate if spreadsheet extended to the right
                     if (aNewColArea.getWidth())
                     {
-                        aViewData.GetViewShell()->libreOfficeKitViewCallback(LOK_CALLBACK_INVALIDATE_TILES, aNewColArea.toString().getStr());
+                        SfxLokHelper::notifyInvalidation(aViewData.GetViewShell(), aNewColArea.toString());
                     }
 
                     // Only invalidate if spreadsheet extended to the bottom
                     if (aNewRowArea.getHeight())
                     {
-                        aViewData.GetViewShell()->libreOfficeKitViewCallback(LOK_CALLBACK_INVALIDATE_TILES, aNewRowArea.toString().getStr());
+                        SfxLokHelper::notifyInvalidation(aViewData.GetViewShell(), aNewRowArea.toString());
                     }
                 }
             }

sd/source/ui/view/sdwindow.cxx

@@ -44,6 +44,7 @@
 #include <vcl/settings.hxx>
 #include <LibreOfficeKit/LibreOfficeKitEnums.h>
 #include <comphelper/lok.hxx>
+#include <sfx2/lokhelper.hxx>
 
 namespace sd {
 
@@ -1008,7 +1009,7 @@ void Window::LogicInvalidate(const Rectangle* pRectangle)
         sRectangle = aRectangle.toString();
     }
     SfxViewShell& rSfxViewShell = mpViewShell->GetViewShellBase();
-    rSfxViewShell.libreOfficeKitViewCallback(LOK_CALLBACK_INVALIDATE_TILES, sRectangle.getStr());
+    SfxLokHelper::notifyInvalidation(&rSfxViewShell, sRectangle);
 }
 
 FactoryFunction Window::GetUITestFactory() const

sfx2/source/view/lokhelper.cxx

@@ -17,6 +17,8 @@
 #include <sfx2/viewsh.hxx>
 #include <sfx2/request.hxx>
 #include <sfx2/viewfrm.hxx>
+#include <LibreOfficeKit/LibreOfficeKitEnums.h>
+#include <comphelper/lok.hxx>
 
 #include <shellimpl.hxx>
 
@@ -136,4 +138,14 @@ void SfxLokHelper::notifyOtherViews(SfxViewShell* pThisView, int nType, const OS
     }
 }
 
+void SfxLokHelper::notifyInvalidation(SfxViewShell* pThisView, const OString& rPayload)
+{
+    std::stringstream ss;
+    ss << rPayload.getStr();
+    if (comphelper::LibreOfficeKit::isPartInInvalidation())
+        ss << ", " << pThisView->getPart();
+    OString aPayload = ss.str().c_str();
+    pThisView->libreOfficeKitViewCallback(LOK_CALLBACK_INVALIDATE_TILES, aPayload.getStr());
+}
+
 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */

svx/source/svdraw/svdmrkv.cxx

@@ -228,7 +228,7 @@ void SdrMarkView::ModelHasChanged()
         }
 
         if(SfxViewShell* pViewShell = GetSfxViewShell())
-            pViewShell->libreOfficeKitViewCallback(LOK_CALLBACK_INVALIDATE_TILES, sSelection.getStr());
+            SfxLokHelper::notifyInvalidation(pViewShell, sSelection);
     }
 }
 

sw/source/uibase/docvw/SidebarScrollBar.cxx

@@ -11,6 +11,7 @@
 
 #include <LibreOfficeKit/LibreOfficeKitEnums.h>
 #include <comphelper/lok.hxx>
+#include <sfx2/lokhelper.hxx>
 
 #include <SidebarWin.hxx>
 #include <view.hxx>
@@ -57,7 +58,7 @@ void SidebarScrollBar::LogicInvalidate(const Rectangle* pRectangle)
 
     OString sRectangle = aRectangle.toString();
     SwWrtShell& rWrtShell = m_rView.GetWrtShell();
-    rWrtShell.GetSfxViewShell()->libreOfficeKitViewCallback(LOK_CALLBACK_INVALIDATE_TILES, sRectangle.getStr());
+    SfxLokHelper::notifyInvalidation(rWrtShell.GetSfxViewShell(), sRectangle);
 }
 
 void SidebarScrollBar::MouseButtonUp(const MouseEvent& /*rMouseEvent*/)

sw/source/uibase/docvw/SidebarTxtControl.cxx

@@ -50,6 +50,7 @@
 #include <editeng/flditem.hxx>
 #include <LibreOfficeKit/LibreOfficeKitEnums.h>
 #include <comphelper/lok.hxx>
+#include <sfx2/lokhelper.hxx>
 
 #include <uitool.hxx>
 #include <view.hxx>
@@ -213,7 +214,7 @@ void SidebarTextControl::LogicInvalidate(const Rectangle* pRectangle)
 
     OString sRectangle = aRectangle.toString();
     SwWrtShell& rWrtShell = mrDocView.GetWrtShell();
-    rWrtShell.GetSfxViewShell()->libreOfficeKitViewCallback(LOK_CALLBACK_INVALIDATE_TILES, sRectangle.getStr());
+    SfxLokHelper::notifyInvalidation(rWrtShell.GetSfxViewShell(), sRectangle);
 }
 
 void SidebarTextControl::KeyInput( const KeyEvent& rKeyEvt )

sw/source/uibase/docvw/edtwin.cxx

@@ -71,6 +71,7 @@
 
 #include <LibreOfficeKit/LibreOfficeKitEnums.h>
 #include <comphelper/lok.hxx>
+#include <sfx2/lokhelper.hxx>
 
 #include <editeng/acorrcfg.hxx>
 #include <SwSmartTagMgr.hxx>
@@ -6406,7 +6407,7 @@ void SwEditWin::LogicInvalidate(const Rectangle* pRectangle)
     else
         sRectangle = pRectangle->toString();
 
-    m_rView.libreOfficeKitViewCallback(LOK_CALLBACK_INVALIDATE_TILES, sRectangle.getStr());
+    SfxLokHelper::notifyInvalidation(&m_rView, sRectangle);
 }
 
 void SwEditWin::LogicMouseButtonDown(const MouseEvent& rMouseEvent)