[Libreoffice-commits] core.git: sw/source

[Luboš Luňák (via logerrit)]

 sw/source/core/bastyp/swregion.cxx |   92 +++++++++++++++++++++----------------
 1 file changed, 54 insertions(+), 38 deletions(-)

New commits:
commit d13b63a9859d653d1aba688a56590aa0ef24b89c
Author:     Luboš Luňák <l.lunak at collabora.com>
AuthorDate: Wed Sep 15 13:49:11 2021 +0200
Commit:     Luboš Luňák <l.lunak at collabora.com>
CommitDate: Thu Sep 16 11:29:19 2021 +0200

    optimize SwRegionRects::Compress()
    
    This function gets called on modifying documents, and it shows up
    noticeably e.g. when profiling editing in Online.
    The code repeatedly restarts the whole process on most changes,
    but in practice this seems to only lead to repeated checks with
    the same outcome. So finish the whole compression and try again
    only at the end, if deemed needed. And even there I'm not sure
    if it's actually ever needed, since I couldn't trigger such a case
    in practice.
    
    Change-Id: I15b0a83c6865bad21fe60a7e9ca0b87ceea5b489
    Reviewed-on: https://gerrit.libreoffice.org/c/core/+/122156
    Tested-by: Luboš Luňák <l.lunak at collabora.com>
    Reviewed-by: Luboš Luňák <l.lunak at collabora.com>

diff --git a/sw/source/core/bastyp/swregion.cxx b/sw/source/core/bastyp/swregion.cxx
index ea59d25d532e..54b237c3756d 100644
--- a/sw/source/core/bastyp/swregion.cxx
+++ b/sw/source/core/bastyp/swregion.cxx
@@ -135,7 +135,7 @@ void SwRegionRects::Invert()
     swap( aInvRegion );
 }
 
-static SwTwips CalcArea( const SwRect &rRect )
+static inline SwTwips CalcArea( const SwRect &rRect )
 {
     return rRect.Width() * rRect.Height();
 }
@@ -143,51 +143,67 @@ static SwTwips CalcArea( const SwRect &rRect )
 // combine all adjacent rectangles
 void SwRegionRects::Compress()
 {
-    for (size_type i = 0; i < size(); )
+    bool bAgain;
+    do
     {
-        bool bRestart(false);
-        for ( size_type j = i+1; j < size(); ++j )
+        bAgain = false;
+        for (size_type i = 0; i < size(); ++i )
         {
-            // If one rectangle contains a second completely than the latter
-            // does not need to be stored and can be deleted
-            if ( (*this)[i].IsInside( (*this)[j] ) )
+            for ( size_type j = i+1; j < size(); ++j )
             {
-                erase( begin() + j );
-                --j;
-            }
-            else if ( (*this)[j].IsInside( (*this)[i] ) )
-            {
-                (*this)[i] = (*this)[j];
-                erase( begin() + j );
-                bRestart = true;
-                break;
-            }
-            else
-            {
-                // If two rectangles have the same area of their union minus the
-                // intersection then one of them can be deleted.
-                // For combining as much as possible (and for having less single
-                // paints), the area of the union can be a little bit larger:
-                // ( 9622 * 141.5 = 1361513 ~= a quarter (1/4) centimeter wider
-                // than the width of an A4 page
-                const tools::Long nFuzzy = 1361513;
-                SwRect aUnion( (*this)[i] );
-                aUnion.Union( (*this)[j] );
-                SwRect aInter( (*this)[i] );
-                aInter.Intersection( (*this)[j] );
-                if ( (::CalcArea( (*this)[i] ) +
-                      ::CalcArea( (*this)[j] ) + nFuzzy) >=
-                     (::CalcArea( aUnion ) - CalcArea( aInter )) )
+                // If one rectangle contains a second completely than the latter
+                // does not need to be stored and can be deleted
+                if ( (*this)[i].IsInside( (*this)[j] ) )
+                {
+                    erase( begin() + j );
+                    --j;
+                }
+                else if ( (*this)[j].IsInside( (*this)[i] ) )
                 {
-                    (*this)[i] = aUnion;
+                    (*this)[i] = (*this)[j];
                     erase( begin() + j );
-                    bRestart = true;
-                    break;
+                    --j;
+                    bAgain = true;
+                }
+                else
+                {
+                    // TODO: I think the comment below and the code are partially incorrect.
+                    // An obvious mistake is the comment saying that one rectangle can be deleted,
+                    // while it's the union that gets used instead of the two rectangles.
+                    // I think this code is supposed to merge adjacent rectangles (possibly
+                    // overlapping), and such rectangles can be detected by their merged areas
+                    // being equal to the area of the union (which is obviously the case if they
+                    // share one side, and using the nFuzzy extra allow merging also rectangles
+                    // that do not quite cover the entire union but it's close enough).
+                    // So another mistake seems to be using '- CalcArea( aInter )',
+                    // it should be on the other side of the comparison to subtract shared area
+                    // counted twice. In practice it seems rectangles here do not share areas,
+                    // so the error is irrelevant.
+
+                    // If two rectangles have the same area of their union minus the
+                    // intersection then one of them can be deleted.
+                    // For combining as much as possible (and for having less single
+                    // paints), the area of the union can be a little bit larger:
+                    // ( 9622 * 141.5 = 1361513 ~= a quarter (1/4) centimeter wider
+                    // than the width of an A4 page
+                    const tools::Long nFuzzy = 1361513;
+                    SwRect aUnion( (*this)[i] );
+                    aUnion.Union( (*this)[j] );
+                    SwRect aInter( (*this)[i] );
+                    aInter.Intersection( (*this)[j] );
+                    if ( (::CalcArea( (*this)[i] ) +
+                          ::CalcArea( (*this)[j] ) + nFuzzy) >=
+                         (::CalcArea( aUnion ) - CalcArea( aInter )) )
+                    {
+                        (*this)[i] = aUnion;
+                        erase( begin() + j );
+                        --j;
+                        bAgain = true;
+                    }
                 }
             }
         }
-        i = bRestart ? 0 : i+1;
-    }
+    } while(bAgain);
 }
 
 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */