Support Pyface Color in TraitsUI toolkit Color traits.#1812
Conversation
This includes tests for the PyQtColor and WxColor traits which was previously absent.
ColourPtr is not present in WxPython 4.
aaronayres35
left a comment
aaronayres35
left a comment
There was a problem hiding this comment.
LGTM just a few questions
| standard_colors = {} | ||
| for name in QtGui.QColor.colorNames(): | ||
| standard_colors[str(name)] = QtGui.QColor(name) | ||
| for name, rgba in color_table.items(): |
There was a problem hiding this comment.
Is color_table is a super set of the standard SVG color keyword names?
At a quick glance it seems to be but I did not go through all colors
There was a problem hiding this comment.
Yes, it's basically the set that modern HTML understands, plus a couple of extra "transparent" synonyms.
| """ Trait definition for a PyQt-based color. | ||
| """ | ||
|
|
||
| from ast import literal_eval |
There was a problem hiding this comment.
why / when should we prefer literal_eval over eval? / why exactly is literal_eval "safer" - the source is limited to a set of python literal structures but its not immediately obvious to me why that is safer
There was a problem hiding this comment.
literal_eval only allows the evaluation of literal expressions (no operations, function calls, . operators, etc.). This means that all of the sneaky tricks that someone can do to craft an expression that executes arbitrary code are not available. Given that these color names may be parsed from untrusted user input, and are only used to evaluate tuples, literal_eval is the right choice.
For things like enabled_when expressions we have to allow the full power of eval for now.
| return value.to_toolkit() | ||
|
|
||
| elif isinstance(value, wx.Colour): | ||
| return value |
There was a problem hiding this comment.
do WxColor not handle spaces (I'm assuming not since there is no wx test analogous to the qt test_name_string_with_space) / if so why can't they?
Can we not just add a value = value.replace(" ", "") in the elif isinstance(value, str): block below?
Similarly I notice we still use eval rather than literal_eval on line 136. Same question here as above
There was a problem hiding this comment.
I've made these updates, and added some tests around this and what happens if eval doesn't give something reasonable in several different ways.
aaronayres35
left a comment
aaronayres35
left a comment
There was a problem hiding this comment.
LGTM, thanks for the updates!
2 participants
In addition this PR standardizes the set of recognized color names to those that Pyface
Colorunderstands (basically all web colors, which aligns with Enable/Chaco). Previously these were just whatever the backend understood.This includes tests for the
PyQtColorandWxColortraits (which correspond to the traits.apiColortrait) and which were previously absent.Checklist