Merge pull request #34 from nacnudus/fix-32

 Safely get RGB from theme colour nodes (fixes #32)

NEWS.md

@@ -6,6 +6,7 @@
     the underscore is followed by a date-ish character like `M` (#24).
 * Optionally omits blank cells with `include_blank_cells = FALSE` in
     `xlsx_cells()` (#25).
+* Doesn't crash reading files with certain colour themes (#34 @dan-fahey).
 
 # tidyxl 1.0.1
 

src/xlsxstyles.cpp

@@ -64,6 +64,18 @@ xlsxstyles::xlsxstyles(const std::string& path) {
   local_ = zipFormats(local_formats_, false);
 }
 
+std::string xlsxstyles::rgb_string(rapidxml::xml_node<>* node) {
+  rapidxml::xml_node<>* child = node->first_node();
+  std::string name = child->name();
+  std::string out;
+  if (name == "a:sysClr") {
+    out = child->first_attribute("lastClr")->value();
+  } else { // assume the name is "srgbClr"
+    out = child->first_attribute("val")->value();
+  }
+  return(out);
+}
+
 void xlsxstyles::cacheThemeRgb(const std::string& path) {
   theme_name_ =
     CharacterVector::create("background1",
@@ -88,24 +100,22 @@ void xlsxstyles::cacheThemeRgb(const std::string& path) {
     rapidxml::xml_node<>* themeElements = theme->first_node("a:themeElements");
     rapidxml::xml_node<>* clrScheme = themeElements->first_node("a:clrScheme");
 
-    // First, two sysClr nodes in the wrong order
+    // First, four nodes in the wrong order
     rapidxml::xml_node<>* color = clrScheme->first_node();
-    theme_[1] = FF + color->first_node()->first_attribute("lastClr")->value();
+    theme_[1] = FF + rgb_string(color);
     color = color->next_sibling();
-    theme_[0] = FF + color->first_node()->first_attribute("lastClr")->value();
-
-    // Then, two srgbClr nodes in the wrong order
+    theme_[0] = FF + rgb_string(color);
     color = color->next_sibling();
-    theme_[3] = FF + color->first_node()->first_attribute("val")->value();
+    theme_[3] = FF + rgb_string(color);
     color = color->next_sibling();
-    theme_[2] = FF + color->first_node()->first_attribute("val")->value();
+    theme_[2] = FF + rgb_string(color);
 
-    // Finally, eight more srgbClr nodes in the correct order
+    // Then, eight more nodes in the correct order
     // Can't reuse 'color' here, so use 'nextcolor'
     int i = 4;
     for (rapidxml::xml_node<>* nextcolor = color->next_sibling();
         nextcolor; nextcolor = nextcolor->next_sibling()) {
-      theme_[i] = FF + nextcolor->first_node()->first_attribute("val")->value();
+      theme_[i] = FF + rgb_string(nextcolor);
       i++;
     }
   }

src/xlsxstyles.h

@@ -68,6 +68,8 @@ class xlsxstyles {
     void applyFormats(); // Build each style on top of the normal style
     Rcpp::List zipFormats(std::vector<xf> styles, bool is_style); // Turn the formats inside-out to return to R
 
+    std::string rgb_string(rapidxml::xml_node<>* node); // Extract the RGB string from a node that references a colour
+
 };
 
 #endif

tests/testthat/test-formats.R

@@ -235,3 +235,7 @@ test_that("protections are parsed correctly", {
   expect_equal(locals$protection$hidden[local_id("A187")], FALSE)
   expect_equal(locals$protection$hidden[local_id("A188")], TRUE)
 })
+
+test_that("themes from an Excel 2016 file don't cause crashes", {
+  expect_error(xlsx_formats("themes-2016.xlsx"), NA)
+})