Toolkit independent color class for Pyface #608
Conversation
Codecov Report
@@ Coverage Diff @@
## master #608 +/- ##
==========================================
+ Coverage 38.97% 39.21% +0.23%
==========================================
Files 487 491 +4
Lines 26798 26916 +118
Branches 4066 4069 +3
==========================================
+ Hits 10445 10555 +110
- Misses 15897 15904 +7
- Partials 456 457 +1
Continue to review full report at Codecov.
|
|
FTR, I went through the older/larger PR - #534 - and there aren't any comments on that PR which are relevant here that hadn't been addressed. |
|
PR still LGTM |
|
@kitchoi can you make time to review this PR over the next couple of days? |
Yep can do. |
| Colors implement equality testing, but are not hashable as they are | ||
| mutable, and so are not suitable for use as dictionary keys. If you |
There was a problem hiding this comment.
Do we actually need Color to be mutable? e.g. in all our use cases, could changing colors be done by supplying a new instance of Color instead of mutating its traits?
We can make Color immutable by making rgba read-only and required at instantiation. This can be achieved by using ReadOnly trait type, or setting the ctrait_type to readonly.
There was a problem hiding this comment.
The objects that this is intended to replace (QColor and wx.Colour values of the Color trait) are mutable.
If we were to make this immutable, the design would be a lot different (probably a namedtuple rather than a HasTraits)
There was a problem hiding this comment.
I guess it depends on how this object is to be used. If we only ever interface it with the GUI toolkit using from_toolkit and to_toolkit, then we might not need to mutate the traits on this object at all.
Immutables are much easier to work with (and are much easier to maintain), especially in asynchronous context (GUI is such context). It is probably also easier to start with an immutable, and then turn it into a mutable later. And if later we run into a situation where this object really needs to be mutated, we have a reason to do it. On the other hand, it would be harder to make a mutable an immutable later.
I will leave that up to you whether you want to consider turning this an immutable.
There was a problem hiding this comment.
No, I expect that the to/from toolkit functions will be used at the point of need internally by toolkit-specific code and not exposed to typical users. One goal of this exercise is that eventually we should never have someone have to use a QColour or wx.Colour unless they are writing toolkit-specific code.
| return NotImplemented | ||
|
|
||
| def __str__(self): | ||
| return "({:0.5}, {:0.5}, {:0.5}, {:0.5})".format(*self.rgba) |
There was a problem hiding this comment.
What is this __str__ used for?
This reminds me of Python 2 behaviour where str(float) does not roundstrip, and it has caused issues because it was not at all obvious (e.g. marshmallow Float uses str, and it causes issue for people who try to use marshmallow for serializing data in JSON and deserialized back, and expecting the data to stay the same... and then be surprised when they don't).
That Python 2 has since changed in Python 3 such that str(float) does round trip. I am wondering whatever that had bitten people before with str(float) not roundtripping might bite us here.
To be safe, could we use {!r} here?
There was a problem hiding this comment.
The point of __str__ is always a human-friendly representation of an object, suitable for use in a print statement or, in the case of TraitsUI, the default representation in a UI. Round-tripping is always a secondary consideration.
In particular, the motivation for this is what will appear in a TraitsUI colour editor's text field.
Given that in practice the colours are going to be converted to bytes, they key thing is that the round-trip byte -> float -> str -> float -> byte works (and it does) so that the actual color doesn't change.
Actually, we can even get away with {0:3} here.
If there is a specialist case which requires more precision, the user can override the editor's format function to give full precision.
There was a problem hiding this comment.
Ah - I remember why 5. The CSS styles allow you to specify with 4-digit hex values if you want. 5 places allows round-tripping all of those successfully.
There was a problem hiding this comment.
I am wondering if this is actually a view layer that gets put on the model. Perhaps this conversion could be implemented in the view instead of here?
There was a problem hiding this comment.
It could, but you want a default, and since this default also makes sense for other situations where you want a human-friendly representation, I think it makes sense to have a __str__ method.
Otherwise why does Python have a __str__ at all?
There was a problem hiding this comment.
For a library, it allows more room for modifications later if we are more conservative about the functionality exposed. The tools are there but we may not choose to use it, e.g. namedtuple is there but we would be exposing more capability than we need if we use namedtuple for something where we only needed named attribute access, making it hard to back out later. Anyway, it was just a thought.
pyface/tests/test_color.py
Outdated
|
|
||
| from unittest import TestCase | ||
|
|
||
| from traits.testing.unittest_tools import UnittestTools |
There was a problem hiding this comment.
Nit: It is available from traits.testing.api, it would be preferred to import from api.
| ) | ||
|
|
||
|
|
||
| def channels_to_ints(channels, maximum=255): |
There was a problem hiding this comment.
Looks like we never use a different value for maximum. When do we need to set a different value?
There was a problem hiding this comment.
We do in the parsing code in the other PR.
|
I think everything is addressed. Will merge in a little while. |
This is the color class from #534 pulled out into a separate PR.
This does not include any of the string parsing, trait type, or dialog classes.