Skip to content
This repository was archived by the owner on Aug 13, 2025. It is now read-only.

fix: Panic caused by the absence of LastClr #847

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: Panic caused by the absence of LastClr #847

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions theme.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ func newTheme(themeXml xlsxTheme) *theme {
for _, scheme := range clrSchemes {
var rgbColor string
if scheme.SysClr != nil {
rgbColor = scheme.SysClr.LastClr
rgbColor = scheme.SysClr.LastClr // LastClr is optional, so rgbColor may be an empty string
Copy link
Owner

@tealeg tealeg Apr 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice comment. I feel like we really need to make the underlying options for a colour transparent to the public interface of the library. At the moment all of this code is pointing towards a use case where we need to know a concrete aRGB value for the colour - the only case where you need that is if you're rendering the document somewhere and this seems to be a less likely use case than generating or modifying a document. In that case we'd be better off just preserving the stylistic information as it is. Sadly fixing that means creating a v4 and I don't have the time or inclination to tackle it.

Copy link
Owner

@tealeg tealeg Apr 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the way we intend this code to be used, the right fix for this situation is to define the values for the systemColors and use those values when lastClr is omitted.

} else {
rgbColor = scheme.SrgbClr.Val
}
Expand All @@ -31,7 +31,7 @@ func (t *theme) themeColor(index int64, tint float64) string {
baseColor := t.colors[index]
if tint == 0 {
return "FF" + baseColor
} else {
} else if baseColor != "" {
Copy link
Owner

@tealeg tealeg Apr 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear that this won't create unexpected errors. See my comment above. The way in v3 would be to define the system colours in a lookup table and insert those values when lastClr is missing.

r, _ := strconv.ParseInt(baseColor[0:2], 16, 64)
g, _ := strconv.ParseInt(baseColor[2:4], 16, 64)
b, _ := strconv.ParseInt(baseColor[4:6], 16, 64)
Expand All @@ -44,4 +44,5 @@ func (t *theme) themeColor(index int64, tint float64) string {
br, bg, bb := HSLToRGB(h, s, l)
return fmt.Sprintf("FF%02X%02X%02X", br, bg, bb)
}
return ""
}