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>
(cherry picked from commit 03bc5199f92e70b6168e4f79600ac288aa7b26ec)
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/156458
Tested-by: Jenkins CollaboraOffice <jenkinscollaboraoffice@gmail.com>
Reviewed-by: Miklos Vajna <vmiklos@collabora.com>

4 files changed, 38 insertions, 31 deletions

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 2a3d12d89938..3759efc23494 100644
--- a/sc/source/filter/excel/xestyle.cxx
+++ b/sc/source/filter/excel/xestyle.cxx
@@ -1846,7 +1846,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 )
@@ -1854,7 +1856,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;
@@ -1864,6 +1866,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
 {
@@ -1999,7 +2002,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);
@@ -2007,9 +2010,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 652e25083090..549d193db089 100644
--- a/sc/source/ui/drawfunc/drawsh.cxx
+++ b/sc/source/ui/drawfunc/drawsh.cxx
@@ -289,9 +289,9 @@ void ScDrawShell::ExecDrawAttr( SfxRequest& rReq )
 
                 if( pView->AreObjectsMarked() )
                 {
-                    std::unique_ptr<SfxItemSet> aNewArgs = rReq.GetArgs()->Clone();
-                    lcl_convertStringArguments(rReq.GetSlot(), *aNewArgs);
-                    pView->SetAttrToMarked(*aNewArgs, false);
+                    std::unique_ptr<SfxItemSet> pNewArgs = rReq.GetArgs()->Clone();
+                    lcl_convertStringArguments(rReq.GetSlot(), *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: */