-
Notifications
You must be signed in to change notification settings - Fork 823
fix: Panic caused by the absence of LastClr #847
base: master
Are you sure you want to change the base?
Conversation
|
"Applications shall use the @lastClr attribute to determine the absolute value of the last color used if system colors are not |
|
Basically it seems we have two problems here:
Defaulting to the empty string here is likely to have odd results for some people. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments inline.
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| if tint == 0 { | ||
| return "FF" + baseColor | ||
| } else { | ||
| } else if baseColor != "" { |
There was a problem hiding this comment.
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.
ECMA-376-1:2016 states that lastClr is optional

<xsd:attribute name="lastClr" type="s:ST_HexColorRGB" use="optional"/>and example:
When lastClr does not exist, it will result in empty strings in the theme ("lt1", "dk1" ).
However, I am not sure if returning an empty string is the best option for
themeColor()in this case.