Refactor color traits#924
Conversation
This combines the two color traits in Enable: ColorTrait (which is mapped) and RGBAColorTrait (which is not), updating them so that they don't use the old `Trait` factory and are simple TraitTypes instead. It combines validation so that they accept the same wide set of values, and makes use of the code from Pyface for parsing color values and the standard color names (Pyface's values were originally taken from Enable). It also unifies the editors, so that both traits have the same editors.
mdickinson
left a comment
mdickinson
left a comment
There was a problem hiding this comment.
Mostly LGTM; a few nitpicks, suggestions and questions in the inline comments.
| from traitsui.api import toolkit as traits_toolkit | ||
| from pyface.toolkit import toolkit | ||
|
|
||
| from enable.trait_defs.rgba_color_trait import ( |
There was a problem hiding this comment.
Is it worth adding a # noqa: F401 here, just to make it clear to the code reader that this is an import for re-export, not for use within the module?
| convert_to_color_tuple as convert_to_color, | ||
| transparent_color_trait, white_color_trait | ||
| ) | ||
| from enable.trait_defs.ui.rgba_color_editor import RGBAColorEditor as ColorEditorFactory |
There was a problem hiding this comment.
Nitpick: Long line (but I'm not sure what conventions are in place for code style here).
| from traitsui.wx.constants import WindowColor | ||
|
|
||
| color_table["sys_window"] = ( | ||
| color_table["syswindow"] = ( |
There was a problem hiding this comment.
On a first pass I was confused by the fact that "sys_window" still appears elsewhere in the codebase and docs (notably in the AbstractWindow and ComponentEditor definitions). I realise now that the color name parsing strips underscores, so it doesn't make a difference in the end, but would it be worth updating those references too?
| "(r,g,b,a)" | ||
| ) | ||
|
|
||
| # XXX to-do: this will not work if dark mode changes |
There was a problem hiding this comment.
Please could you open an issue for this and add the issue reference to the comment?
| from enable.trait_defs.rgba_color_trait import RGBAColor | ||
|
|
||
| rgba_float_dtype = np.dtype([ | ||
| ('red', float), |
There was a problem hiding this comment.
Nitpick-level: use "float64" instead of float, for consistency and to avoid the implicit conversion from NumPy?
| try: | ||
| default_value = convert_to_color_tuple(value) | ||
| except Exception: | ||
| self.error(None, None, value) |
There was a problem hiding this comment.
The generated error message isn't friendly here: for something like RGBAColor("octarine") we end up with the exception traits.trait_errors.TraitError: None. Proposal: leave out the try / except altogether, and just let the convert_to_color_tuple exception propagate. That would mean that the reported exception for an invalid Trait declaration would have type TypeError or ValueError instead of TraitError, but I don't see a problem with that (in fact, I think it's an improvement).
There was a problem hiding this comment.
That may break use in TraitsUI - TraitsUI editors often treat TraitError as "the user typed something that is invalid for the underlying trait but otherwise OK" (and may be flagged in the UI by color changes, etc.) but treats other exceptions as being more fundamental.
So I think we want TraitError. I will look into whether the error message can be made better. I know that the existing color traits often give an unhelpful laundry list of all the possible color names.
There was a problem hiding this comment.
But this is in the trait definition, not trait validation. We definitely want TraitError in the validation - that's what TraitError is for. I'm suggesting that we don't need (or indeed want) it in the trait definition.
I think it would be extremely rare that we're trying to catch TraitError when defining a trait in a class.
There was a problem hiding this comment.
More generally, TraitError has a very specific purpose, namely allowing users (like TraitsUI editors, exactly as you describe) to catch traited attribute validation errors. But there's been a tendency to broaden that scope and use TraitError for anything vaguely traits-related. That's something I'm actively trying to reverse, since it increases the possibility that people who actually want to catch the "invalid attribute" case (the intended use-case) end up catching other errors as well.
There was a problem hiding this comment.
Oh, this is in the __init__. Yep, that's better to just let it propagate.
|
|
||
| def __init__(self, value="white", **metadata): | ||
| default_value = self.validate(None, None, value) | ||
| try: |
There was a problem hiding this comment.
Can we add a class-level default_value_type = DefaultValue.constant to prevent Traits from doing its own default value type inference?
Co-authored-by: Mark Dickinson <mdickinson@enthought.com>
|
I think this now addresses all the comments. |
|
@corranwebster Looks like there's a missing import causing CI to fail. |
2 participants
This combines the two color traits in Enable: ColorTrait (which is mapped) and RGBAColorTrait (which is not), updating them so that they don't use the old
Traitfactory and are simple TraitTypes instead.It combines validation so that they accept the same wide set of values, and makes use of the code from Pyface for parsing color values and the standard color names (Pyface's values were originally taken from Enable). It also unifies the editors, so that both traits have the same editors.
Fixes #895 (at least as much as we can right now).