tdf#76683 writerfilter: TwipMeasure must be positive

...and the column width must not be negative.

The previous column logic ensured that the total width was
larger than the reference by adding a fake buffer
and then subtracted the difference from the final column.
In the case of a zero-width final column, it could become negative.
The current logic ensures that the total width is less than
the reference width, and then adds the difference
(which should be a smaller difference now) to the final column.

Regression potential - early columns that need every single twip
of bonus space might not fit anymore. On the other hand,
ending columns might be fixed...

Change-Id: Ie75d455e8ed62dbec5a1b9c901417df8d842ace8
Reviewed-on: https://gerrit.libreoffice.org/59400
Tested-by: Jenkins
Reviewed-by: Justin Luth <justin_luth@sil.org>

7 files changed, 59 insertions, 12 deletions

diff --git a/sw/qa/extras/ooxmlexport/data/tdf76683_negativeTwipsMeasure.docx b/sw/qa/extras/ooxmlexport/data/tdf76683_negativeTwipsMeasure.docx
new file mode 100644
index 000000000000..eb769fdcc3bf
--- /dev/null
+++ b/sw/qa/extras/ooxmlexport/data/tdf76683_negativeTwipsMeasure.docx
diff --git a/sw/qa/extras/ooxmlexport/ooxmlexport11.cxx b/sw/qa/extras/ooxmlexport/ooxmlexport11.cxx
index db52d019439b..adb0dc6f4264 100644
--- a/sw/qa/extras/ooxmlexport/ooxmlexport11.cxx
+++ b/sw/qa/extras/ooxmlexport/ooxmlexport11.cxx
@@ -160,6 +160,17 @@ DECLARE_OOXMLEXPORT_TEST(testTdf82065_Ind_start_strict, "tdf82065_Ind_start_stri
     CPPUNIT_ASSERT_EQUAL_MESSAGE("IndentAt defined", true, bFoundIndentAt);
 }
 
+DECLARE_OOXMLEXPORT_TEST(testTdf76683_negativeTwipsMeasure, "tdf76683_negativeTwipsMeasure.docx")
+{
+    xmlDocPtr pXmlDoc = parseExport("word/document.xml");
+    if (!pXmlDoc)
+        return;
+    assertXPath(pXmlDoc, "/w:document/w:body/w:sectPr/w:cols/w:col", 2);
+    sal_uInt32 nColumn1 = getXPath(pXmlDoc, "/w:document/w:body/w:sectPr/w:cols/w:col[1]", "w").toUInt32();
+    sal_uInt32 nColumn2 = getXPath(pXmlDoc, "/w:document/w:body/w:sectPr/w:cols/w:col[2]", "w").toUInt32();
+    CPPUNIT_ASSERT( nColumn1 > nColumn2 );
+}
+
 DECLARE_OOXMLEXPORT_TEST(testTdf112694, "tdf112694.docx")
 {
     uno::Any aPageStyle = getStyles("PageStyles")->getByName("Standard");
diff --git a/writerfilter/source/dmapper/PropertyMap.cxx b/writerfilter/source/dmapper/PropertyMap.cxx
index 691f1a00a474..4048080e03c5 100644
--- a/writerfilter/source/dmapper/PropertyMap.cxx
+++ b/writerfilter/source/dmapper/PropertyMap.cxx
@@ -716,13 +716,18 @@ uno::Reference< text::XTextColumns > SectionPropertyMap::ApplyColumnProperties(
             nColSum = 0;
             for ( sal_Int32 nCol = 0; nCol <= m_nColumnCount; ++nCol )
             {
-                pColumn[nCol].LeftMargin = nCol ? m_aColDistance[nCol - 1] / 2 : 0;
-                pColumn[nCol].RightMargin = nCol == m_nColumnCount ? 0 : m_aColDistance[nCol] / 2;
-                pColumn[nCol].Width = sal_Int32( (double( m_aColWidth[nCol] + pColumn[nCol].RightMargin + pColumn[nCol].LeftMargin ) + 0.5) * fRel );
+                const double fLeft = nCol ? m_aColDistance[nCol - 1] / 2 : 0;
+                pColumn[nCol].LeftMargin = fLeft;
+                const double fRight = nCol == m_nColumnCount ? 0 : m_aColDistance[nCol] / 2;
+                pColumn[nCol].RightMargin = fRight;
+                const double fWidth = m_aColWidth[nCol];
+                pColumn[nCol].Width = (fWidth + fLeft + fRight) * fRel;
                 nColSum += pColumn[nCol].Width;
             }
             if ( nColSum != nRefValue )
-                pColumn[m_nColumnCount].Width -= (nColSum - nRefValue);
+                pColumn[m_nColumnCount].Width += (nRefValue - nColSum);
+            assert( pColumn[m_nColumnCount].Width >= 0 );
+
             xColumns->setColumns( aColumns );
         }
         else
diff --git a/writerfilter/source/ooxml/OOXMLFactory.cxx b/writerfilter/source/ooxml/OOXMLFactory.cxx
index 4c41684cd594..d98d22db485b 100644
--- a/writerfilter/source/ooxml/OOXMLFactory.cxx
+++ b/writerfilter/source/ooxml/OOXMLFactory.cxx
@@ -102,10 +102,16 @@ void OOXMLFactory::attributes(OOXMLFastContextHandler * pHandler,
                 pFactory->attributeAction(pHandler, nToken, xValue);
             }
             break;
-        case ResourceType::TwipsMeasure:
+        case ResourceType::TwipsMeasure_asSigned:
+        case ResourceType::TwipsMeasure_asZero:
             {
                 const char *pValue = pAttribs->getAsCharByIndex(nAttrIndex);
                 OOXMLValue::Pointer_t xValue(new OOXMLTwipsMeasureValue(pValue));
+                if ( xValue->getInt() < 0 )
+                {
+                    if ( pAttr->m_nResource == ResourceType::TwipsMeasure_asZero )
+                        xValue = OOXMLIntegerValue::Create(0);
+                }
                 pHandler->newProperty(nId, xValue);
                 pFactory->attributeAction(pHandler, nToken, xValue);
             }
diff --git a/writerfilter/source/ooxml/OOXMLFactory.hxx b/writerfilter/source/ooxml/OOXMLFactory.hxx
index f1432869f4e2..04a71bbc92f4 100644
--- a/writerfilter/source/ooxml/OOXMLFactory.hxx
+++ b/writerfilter/source/ooxml/OOXMLFactory.hxx
@@ -52,7 +52,8 @@ enum class ResourceType {
     PropertyTable,
     Math,
     Any,
-    TwipsMeasure,
+    TwipsMeasure_asSigned,
+    TwipsMeasure_asZero,
     HpsMeasure,
     MeasurementOrPercent
 };
diff --git a/writerfilter/source/ooxml/factoryimpl.py b/writerfilter/source/ooxml/factoryimpl.py
index 2d54ee8ff6b8..5f397ab9cbad 100644
--- a/writerfilter/source/ooxml/factoryimpl.py
+++ b/writerfilter/source/ooxml/factoryimpl.py
@@ -38,7 +38,8 @@ def createFastChildContextFromFactory(model):
             switch (nResource)
             {""")
     resources = [
-        "List", "Integer", "Hex", "HexColor", "String", "TwipsMeasure",
+        "List", "Integer", "Hex", "HexColor", "String",
+        "TwipsMeasure_asSigned", "TwipsMeasure_asZero",
         "HpsMeasure", "Boolean", "MeasurementOrPercent",
     ]
     for resource in [r.getAttribute("resource") for r in model.getElementsByTagName("resource")]:
diff --git a/writerfilter/source/ooxml/model.xml b/writerfilter/source/ooxml/model.xml
index e1640c44e54d..953010eb020b 100644
--- a/writerfilter/source/ooxml/model.xml
+++ b/writerfilter/source/ooxml/model.xml
@@ -8236,7 +8236,7 @@
     <resource name="CT_OMathJc" resource="Value">
       <attribute name="val" tokenid="ooxml:CT_OMathJc_val" action="setValue"/>
     </resource>
-    <resource name="ST_TwipsMeasure" resource="TwipsMeasure"/>
+    <resource name="ST_TwipsMeasure" resource="TwipsMeasure_asSigned"/>
     <resource name="CT_TwipsMeasure" resource="Value">
       <attribute name="val" tokenid="ooxml:CT_TwipsMeasure_val" action="setValue"/>
       <action name="start" action="setDefaultIntegerValue"/>
@@ -10683,9 +10683,25 @@
       <define name="ST_UnsignedDecimalNumber">
         <data type="unsignedLong"/>
       </define>
+<!-- MS documentation states that TwipsMeasure is a positive value
+     but it doesn't indicate how to handle an invalid negative
+     value. So, ST_TwipsMeasure matches the documented type,
+     and the extension (_asSigned, _asZero, or perhaps _asAbsolute)
+     indicates how MS handles it in practice.
+
+     Historically, LO has treated TwipsMeasure as signed,
+     i.e. that negative numbers are allowed and treated as negative,
+     so that is the default handling.
+-->
       <define name="ST_TwipsMeasure">
         <data type="unsignedLong"/>
       </define>
+      <define name="ST_TwipsMeasure_asSigned">
+        <data type="unsignedLong"/>
+      </define>
+      <define name="ST_TwipsMeasure_asZero">
+        <data type="unsignedLong"/>
+      </define>
       <define name="CT_TwipsMeasure">
         <attribute name="val">
           <ref name="ST_TwipsMeasure"/>
@@ -13013,10 +13029,10 @@
       </define>
       <define name="CT_Column">
         <attribute name="w">
-          <ref name="ST_TwipsMeasure"/>
+          <ref name="ST_TwipsMeasure_asZero"/>
         </attribute>
         <attribute name="space">
-          <ref name="ST_TwipsMeasure"/>
+          <ref name="ST_TwipsMeasure_asZero"/>
         </attribute>
       </define>
       <define name="CT_Columns">
@@ -16656,12 +16672,19 @@
       <action name="start" action="setDefaultIntegerValue"/>
     </resource>
     <resource name="ST_UnsignedDecimalNumber" resource="Integer"/>
-    <resource name="ST_TwipsMeasure" resource="TwipsMeasure"/>
+<!--
+     Historically, LO has treated TwipsMeasure as signed,
+     i.e. that negative numbers are allowed and treated as negative,
+     so that is the default handling.
+-->
+    <resource name="ST_TwipsMeasure" resource="TwipsMeasure_asSigned"/>
+    <resource name="ST_TwipsMeasure_asSigned" resource="TwipsMeasure_asSigned"/>
+    <resource name="ST_TwipsMeasure_asZero" resource="TwipsMeasure_asZero"/>
     <resource name="CT_TwipsMeasure" resource="Value">
       <attribute name="val" tokenid="ooxml:CT_TwipsMeasure_val" action="setValue"/>
       <action name="start" action="setDefaultIntegerValue"/>
     </resource>
-    <resource name="ST_SignedTwipsMeasure" resource="TwipsMeasure"/>
+    <resource name="ST_SignedTwipsMeasure" resource="TwipsMeasure_asSigned"/>
     <resource name="CT_SignedTwipsMeasure" resource="Value">
       <attribute name="val" tokenid="ooxml:CT_SignedTwipsMeasure_val" action="setValue"/>
       <action name="start" action="setDefaultIntegerValue"/>