-
-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ColorProperty: Use ColorProperty instead of ListProperty for color property #6918
ColorProperty: Use ColorProperty instead of ListProperty for color property #6918
Conversation
Should be merged after #6917 . |
d0f12cd
to
9bdfa41
Compare
9bdfa41
to
ec255b0
Compare
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 looks ok, although it makes me nervous to change code on masse like this. I'd want at least one other person to look it over.
One thing, it would be ideal if you could add versionchanged
tag for these properties to say it was changed to a color property.
Added Regression problem I can see is setting of individual components for color (with |
How do you mean? |
Setting like
won't dispatch event that |
Oh, that's definitely bad. I wouldn't assume it's not a common usage, I'm pretty sure I used that with slicing. But even if it's not common, we can't break it like that. The solution is to change |
How can we ensure that iterable of 3 or 4 components are used when setting a slice? obj.color[:] = [0.1, 0.2, 0.3, 1.0, 1.0]
|
The current implementation has the same problem though!? That would be user error and we don't have to protect against it. Trying to protect against that would make things much more complicated. |
Ok. I'll add a note to |
Cool. Although it's not just slicing. Presumably doing |
Added note under |
@matham Are we waiting for another developer to approve or I can squash and merge? |
Yeah, because it's such a large change I'd like it if someone like @tshirtman would also approve. |
looks good to me, as long the change to ColorProperty now means we can use it exactly like any ListProperty would be in this context, yes it changes quite a few pieces of the code, but the change every time is pretty trivial. |
For all color properties except:
GestureSurface.color
Label.outline_color
Label.disabled_outline_color
as for those properties format
rgb
is used and additional changes are needed to enable using ofColorProperty