various theme and complex color related fixes

Most of changes are a results of code reviews.

Added comments and clarifications, removed unneeded methods,
renamed variables to be more clear, protection against nullptr
dereference,...

Change-Id: Iae2b6abfa90b3bbd3f28328ca7fcf429765cbe9d
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/156203
Tested-by: Jenkins
Reviewed-by: Tomaž Vajngerl <quikee@gmail.com>

8 files changed, 53 insertions, 46 deletions

diff --git a/include/docmodel/color/ComplexColor.hxx b/include/docmodel/color/ComplexColor.hxx
index 4cec9e972503..0fc99a12617b 100644
--- a/include/docmodel/color/ComplexColor.hxx
+++ b/include/docmodel/color/ComplexColor.hxx
@@ -180,8 +180,6 @@ public:
         meType = ColorType::HSL;
     }
 
-    void setPlaceholder() { meType = ColorType::Placeholder; }
-
     void setSystemColor(SystemColorType eSystemColorType, sal_Int32 nRGB)
     {
         maLastColor = ::Color(ColorTransparency, nRGB);
diff --git a/include/docmodel/theme/FormatScheme.hxx b/include/docmodel/theme/FormatScheme.hxx
index 96c8afc48214..d6812e749aba 100644
--- a/include/docmodel/theme/FormatScheme.hxx
+++ b/include/docmodel/theme/FormatScheme.hxx
@@ -474,19 +474,19 @@ public:
         {
             FillStyle* pFillStyle = pThis->addFillStyle();
             auto pFill = std::make_shared<SolidFill>();
-            pFill->maColor.setPlaceholder();
+            pFill->maColor.setThemePlaceholder();
             pFillStyle->mpFill = pFill;
         }
         {
             FillStyle* pFillStyle = pThis->addFillStyle();
             auto pFill = std::make_shared<SolidFill>();
-            pFill->maColor.setPlaceholder();
+            pFill->maColor.setThemePlaceholder();
             pFillStyle->mpFill = pFill;
         }
         {
             FillStyle* pFillStyle = pThis->addFillStyle();
             auto pFill = std::make_shared<SolidFill>();
-            pFill->maColor.setPlaceholder();
+            pFill->maColor.setThemePlaceholder();
             pFillStyle->mpFill = pFill;
         }
     }
@@ -517,7 +517,7 @@ public:
             pLineStyle->maLineDash.mePresetType = PresetDashType::Solid;
             pLineStyle->maLineJoin.meType = LineJoinType::Miter;
             auto pFill = std::make_shared<SolidFill>();
-            pFill->maColor.setPlaceholder();
+            pFill->maColor.setThemePlaceholder();
             pLineStyle->maLineFillStyle.mpFill = pFill;
         }
         {
@@ -529,7 +529,7 @@ public:
             pLineStyle->maLineDash.mePresetType = PresetDashType::Solid;
             pLineStyle->maLineJoin.meType = LineJoinType::Miter;
             auto pFill = std::make_shared<SolidFill>();
-            pFill->maColor.setPlaceholder();
+            pFill->maColor.setThemePlaceholder();
             pLineStyle->maLineFillStyle.mpFill = pFill;
         }
         {
@@ -541,7 +541,7 @@ public:
             pLineStyle->maLineDash.mePresetType = PresetDashType::Solid;
             pLineStyle->maLineJoin.meType = LineJoinType::Miter;
             auto pFill = std::make_shared<SolidFill>();
-            pFill->maColor.setPlaceholder();
+            pFill->maColor.setThemePlaceholder();
             pLineStyle->maLineFillStyle.mpFill = pFill;
         }
     }
@@ -591,19 +591,19 @@ public:
         {
             FillStyle* pFillStyle = pThis->addBackgroundFillStyle();
             auto pFill = std::make_shared<SolidFill>();
-            pFill->maColor.setPlaceholder();
+            pFill->maColor.setThemePlaceholder();
             pFillStyle->mpFill = pFill;
         }
         {
             FillStyle* pFillStyle = pThis->addBackgroundFillStyle();
             auto pFill = std::make_shared<SolidFill>();
-            pFill->maColor.setPlaceholder();
+            pFill->maColor.setThemePlaceholder();
             pFillStyle->mpFill = pFill;
         }
         {
             FillStyle* pFillStyle = pThis->addBackgroundFillStyle();
             auto pFill = std::make_shared<SolidFill>();
-            pFill->maColor.setPlaceholder();
+            pFill->maColor.setThemePlaceholder();
             pFillStyle->mpFill = pFill;
         }
     }
diff --git a/include/oox/export/ColorExportUtils.hxx b/include/oox/export/ColorExportUtils.hxx
index f9dafe260b46..01b3503e00a3 100644
--- a/include/oox/export/ColorExportUtils.hxx
+++ b/include/oox/export/ColorExportUtils.hxx
@@ -14,9 +14,6 @@
 namespace model
 {
 class ComplexColor;
-}
-namespace model
-{
 enum class ThemeColorType : sal_Int32;
 }
 
diff --git a/oox/source/export/ColorExportUtils.cxx b/oox/source/export/ColorExportUtils.cxx
index 2b9f7baabc53..d46ab493b5ab 100644
--- a/oox/source/export/ColorExportUtils.cxx
+++ b/oox/source/export/ColorExportUtils.cxx
@@ -45,11 +45,16 @@ sal_Int32 convertThemeColorTypeToExcelThemeNumber(model::ThemeColorType eType)
     if (eType == model::ThemeColorType::Unknown)
         return -1;
 
+    // Change position of text1 and text2 and background1 and background2 - needed because of an bug in excel, where
+    // the text and background index positions are switched.
+    // 0 -> 1, 1 -> 0
+    // 2 -> 3, 3 -> 2
+    // everything else stays the same
     static constexpr std::array<sal_Int32, 12> constThemeColorMapToXmlMap
         = { 1, 0, 3, 2, 4, 5, 6, 7, 8, 9, 10, 11 };
 
     return constThemeColorMapToXmlMap[sal_Int32(eType)];
 }
-}
+} // end oox
 
 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/sc/qa/unit/ucalc_DocumentThemes.cxx b/sc/qa/unit/ucalc_DocumentThemes.cxx
index 664b06a879f9..d0bd29052c16 100644
--- a/sc/qa/unit/ucalc_DocumentThemes.cxx
+++ b/sc/qa/unit/ucalc_DocumentThemes.cxx
@@ -71,6 +71,7 @@ CPPUNIT_TEST_FIXTURE(DocumentThemesTest, testChangeTheme)
         aNewPattern.GetItemSet().Put(SvxColorItem(aColor, aComplexColor, ATTR_FONT_COLOR));
     }
 
+    // Apply the pattern to cells C3:E5 (2,2 - 4,4) on the first sheet
     m_pDoc->ApplyPatternAreaTab(2, 2, 4, 4, 0, aNewPattern);
 
     {
diff --git a/sc/source/filter/excel/xestyle.cxx b/sc/source/filter/excel/xestyle.cxx
index f7f24c17d223..a68d6408edd9 100644
--- a/sc/source/filter/excel/xestyle.cxx
+++ b/sc/source/filter/excel/xestyle.cxx
@@ -1845,7 +1845,9 @@ static const char* ToLineStyle( sal_uInt8 nLineStyle )
     return "*unknown*";
 }
 
-static void lcl_WriteBorder(XclExpXmlStream& rStrm, sal_Int32 nElement, sal_uInt8 nLineStyle, const Color& rColor, model::ComplexColor const& rComplexColor)
+namespace
+{
+void lcl_WriteBorder(XclExpXmlStream& rStrm, sal_Int32 nElement, sal_uInt8 nLineStyle, const Color& rColor, model::ComplexColor const& rComplexColor)
 {
     sax_fastparser::FSHelperPtr& rStyleSheet = rStrm.GetCurrentStream();
     if( nLineStyle == EXC_LINE_NONE )
@@ -1853,7 +1855,7 @@ static void lcl_WriteBorder(XclExpXmlStream& rStrm, sal_Int32 nElement, sal_uInt
         rStyleSheet->singleElement(nElement);
         return;
     }
-    else if (rColor == Color(0, 0, 0))
+    else if (rColor == Color(0, 0, 0) && !rComplexColor.isValidThemeType())
     {
         rStyleSheet->singleElement(nElement, XML_style, ToLineStyle(nLineStyle));
         return;
@@ -1863,6 +1865,7 @@ static void lcl_WriteBorder(XclExpXmlStream& rStrm, sal_Int32 nElement, sal_uInt
     oox::xls::writeComplexColor(rStyleSheet, XML_color, rComplexColor, rColor);
     rStyleSheet->endElement(nElement);
 }
+} // end anonymous namespace
 
 void XclExpCellBorder::SaveXml(XclExpXmlStream& rStream) const
 {
@@ -1998,7 +2001,7 @@ void XclExpCellArea::SaveXml( XclExpXmlStream& rStrm ) const
         {
             {
                 Color aColor = rPalette.GetColor(mnForeColor);
-                if (maForegroundComplexColor.isValidThemeType())
+                if (maForegroundComplexColor.isValidThemeType() || mnForeColor != 0)
                     oox::xls::writeComplexColor(rStyleSheet, XML_fgColor, maForegroundComplexColor, aColor);
                 else if (mnForeColor != 0)
                     oox::xls::writeComplexColor(rStyleSheet, XML_fgColor, maForegroundComplexColor, aColor);
@@ -2006,9 +2009,7 @@ void XclExpCellArea::SaveXml( XclExpXmlStream& rStrm ) const
 
             {
                 Color aColor = rPalette.GetColor(mnBackColor);
-                if (maBackgroundComplexColor.isValidThemeType())
-                    oox::xls::writeComplexColor(rStyleSheet, XML_bgColor, maBackgroundComplexColor, aColor);
-                else if (mnForeColor != 0)
+                if (maBackgroundComplexColor.isValidThemeType() || mnBackColor != 0)
                     oox::xls::writeComplexColor(rStyleSheet, XML_bgColor, maBackgroundComplexColor, aColor);
             }
         }
diff --git a/sc/source/ui/drawfunc/drawsh.cxx b/sc/source/ui/drawfunc/drawsh.cxx
index fde34e9d6686..2369ad0884c9 100644
--- a/sc/source/ui/drawfunc/drawsh.cxx
+++ b/sc/source/ui/drawfunc/drawsh.cxx
@@ -260,9 +260,9 @@ void ScDrawShell::ExecDrawAttr( SfxRequest& rReq )
 
                 if( pView->AreObjectsMarked() )
                 {
-                    std::unique_ptr<SfxItemSet> aNewArgs = rReq.GetArgs()->Clone();
-                    lcl_convertStringArguments(*aNewArgs);
-                    pView->SetAttrToMarked(*aNewArgs, false);
+                    std::unique_ptr<SfxItemSet> pNewArgs = rReq.GetArgs()->Clone();
+                    lcl_convertStringArguments(*pNewArgs);
+                    pView->SetAttrToMarked(*pNewArgs, false);
                 }
                 else
                     pView->SetDefaultAttr( *rReq.GetArgs(), false);
diff --git a/sc/source/ui/theme/ThemeColorChanger.cxx b/sc/source/ui/theme/ThemeColorChanger.cxx
index 8af2d4885abf..c9b88652dcc3 100644
--- a/sc/source/ui/theme/ThemeColorChanger.cxx
+++ b/sc/source/ui/theme/ThemeColorChanger.cxx
@@ -117,7 +117,7 @@ bool changeCellItems(SfxItemSet& rItemSet, model::ColorSet const& rColorSet)
     return bChanged;
 }
 
-bool changeStyles(ScDocShell& rDocShell, std::shared_ptr<model::ColorSet> const& pColorSet)
+bool changeStyles(ScDocShell& rDocShell, model::ColorSet const& rColorSet)
 {
     ScDocument& rDocument = rDocShell.GetDocument();
     ScStyleSheetPool* pPool = rDocument.GetStyleSheetPool();
@@ -132,7 +132,7 @@ bool changeStyles(ScDocShell& rDocShell, std::shared_ptr<model::ColorSet> const&
         aOldData.InitFromStyle(pStyle);
 
         auto rItemSet = pStyle->GetItemSet();
-        if (changeCellItems(rItemSet, *pColorSet))
+        if (changeCellItems(rItemSet, rColorSet))
         {
             if (rDocument.IsUndoEnabled())
             {
@@ -152,7 +152,7 @@ bool changeStyles(ScDocShell& rDocShell, std::shared_ptr<model::ColorSet> const&
 }
 
 bool changeSheets(ScDocShell& rDocShell, ScTabViewShell* pViewShell, ScDrawLayer* pModel,
-                  std::shared_ptr<model::ColorSet> const& pColorSet)
+                  model::ColorSet const& rColorSet)
 {
     ScDocument& rDocument = rDocShell.GetDocument();
     bool bChanged = false;
@@ -174,7 +174,7 @@ bool changeSheets(ScDocShell& rDocShell, ScTabViewShell* pViewShell, ScDrawLayer
 
                 ScPatternAttr aNewPattern(*pPattern);
                 auto& rItemSet = aNewPattern.GetItemSet();
-                bool bItemChanged = changeCellItems(rItemSet, *pColorSet);
+                bool bItemChanged = changeCellItems(rItemSet, rColorSet);
                 bChanged = bChanged || bItemChanged;
 
                 if (bItemChanged && rDocument.IsUndoEnabled())
@@ -217,7 +217,7 @@ bool changeSheets(ScDocShell& rDocShell, ScTabViewShell* pViewShell, ScDrawLayer
             SdrObjListIter aIter(pPage, SdrIterMode::DeepNoGroups);
             for (SdrObject* pObject = aIter.Next(); pObject; pObject = aIter.Next())
             {
-                svx::theme::updateSdrObject(*pColorSet, pObject, pView, rDocShell.GetUndoManager());
+                svx::theme::updateSdrObject(rColorSet, pObject, pView, rDocShell.GetUndoManager());
             }
 
             std::unique_ptr<SdrUndoGroup> pUndo = pModel->GetCalcUndo();
@@ -235,19 +235,19 @@ bool changeSheets(ScDocShell& rDocShell, ScTabViewShell* pViewShell, ScDrawLayer
 }
 
 model::ComplexColor modifyComplexColor(model::ComplexColor const& rComplexColor,
-                                       std::shared_ptr<model::ColorSet> const& pColorSet)
+                                       model::ColorSet const& rColorSet)
 {
     model::ComplexColor aComplexColor(rComplexColor);
 
     if (aComplexColor.isValidThemeType())
     {
-        Color aColor = pColorSet->resolveColor(aComplexColor);
+        Color aColor = rColorSet.resolveColor(aComplexColor);
         aComplexColor.setFinalColor(aColor);
     }
     return aComplexColor;
 }
 
-void changeSparklines(ScDocShell& rDocShell, std::shared_ptr<model::ColorSet> const& pColorSet)
+void changeSparklines(ScDocShell& rDocShell, model::ColorSet const& rColorSet)
 {
     ScDocument& rDocument = rDocShell.GetDocument();
     auto& rDocFunc = rDocShell.GetDocFunc();
@@ -261,18 +261,18 @@ void changeSparklines(ScDocShell& rDocShell, std::shared_ptr<model::ColorSet> co
             {
                 auto aAttributes = rSparklineGroup->getAttributes();
 
-                aAttributes.setColorAxis(modifyComplexColor(aAttributes.getColorAxis(), pColorSet));
+                aAttributes.setColorAxis(modifyComplexColor(aAttributes.getColorAxis(), rColorSet));
                 aAttributes.setColorSeries(
-                    modifyComplexColor(aAttributes.getColorSeries(), pColorSet));
+                    modifyComplexColor(aAttributes.getColorSeries(), rColorSet));
                 aAttributes.setColorNegative(
-                    modifyComplexColor(aAttributes.getColorNegative(), pColorSet));
+                    modifyComplexColor(aAttributes.getColorNegative(), rColorSet));
                 aAttributes.setColorMarkers(
-                    modifyComplexColor(aAttributes.getColorMarkers(), pColorSet));
-                aAttributes.setColorHigh(modifyComplexColor(aAttributes.getColorHigh(), pColorSet));
-                aAttributes.setColorLow(modifyComplexColor(aAttributes.getColorLow(), pColorSet));
+                    modifyComplexColor(aAttributes.getColorMarkers(), rColorSet));
+                aAttributes.setColorHigh(modifyComplexColor(aAttributes.getColorHigh(), rColorSet));
+                aAttributes.setColorLow(modifyComplexColor(aAttributes.getColorLow(), rColorSet));
                 aAttributes.setColorFirst(
-                    modifyComplexColor(aAttributes.getColorFirst(), pColorSet));
-                aAttributes.setColorLast(modifyComplexColor(aAttributes.getColorLast(), pColorSet));
+                    modifyComplexColor(aAttributes.getColorFirst(), rColorSet));
+                aAttributes.setColorLast(modifyComplexColor(aAttributes.getColorLast(), rColorSet));
                 rDocFunc.ChangeSparklineGroupAttributes(rSparklineGroup, aAttributes);
             }
         }
@@ -292,7 +292,8 @@ std::shared_ptr<model::Theme> getTheme(ScDocShell& rDocShell)
     return pTheme;
 }
 
-void changeTheTheme(ScDocShell& rDocShell, std::shared_ptr<model::ColorSet> const& pColorSet)
+void changeThemeColorInTheDocModel(ScDocShell& rDocShell,
+                                   std::shared_ptr<model::ColorSet> const& pColorSet)
 {
     auto pTheme = getTheme(rDocShell);
     std::shared_ptr<model::ColorSet> pNewColorSet = pColorSet;
@@ -312,6 +313,10 @@ void changeTheTheme(ScDocShell& rDocShell, std::shared_ptr<model::ColorSet> cons
 
 void ThemeColorChanger::apply(std::shared_ptr<model::ColorSet> const& pColorSet)
 {
+    // Can't change to an empty color set
+    if (!pColorSet)
+        return;
+
     m_rDocShell.MakeDrawLayer();
 
     ScDocShellModificator aModificator(m_rDocShell);
@@ -333,12 +338,12 @@ void ThemeColorChanger::apply(std::shared_ptr<model::ColorSet> const& pColorSet)
     }
 
     bool bChanged = false;
-    bChanged = changeStyles(m_rDocShell, pColorSet) || bChanged;
+    bChanged = changeStyles(m_rDocShell, *pColorSet) || bChanged;
     bChanged
-        = changeSheets(m_rDocShell, pViewShell, rDocument.GetDrawLayer(), pColorSet) || bChanged;
-    changeSparklines(m_rDocShell, pColorSet);
+        = changeSheets(m_rDocShell, pViewShell, rDocument.GetDrawLayer(), *pColorSet) || bChanged;
+    changeSparklines(m_rDocShell, *pColorSet);
 
-    changeTheTheme(m_rDocShell, pColorSet);
+    changeThemeColorInTheDocModel(m_rDocShell, pColorSet);
 
     if (bUndo)
     {
@@ -349,6 +354,6 @@ void ThemeColorChanger::apply(std::shared_ptr<model::ColorSet> const& pColorSet)
     aModificator.SetDocumentModified();
 }
 
-} // end sw namespace
+} // end sc namespace
 
 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */