Skip to content
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

Merged
merged 2 commits into from Jun 14, 2020

Conversation

pythonic64
Copy link
Contributor

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 of ColorProperty

@pythonic64
Copy link
Contributor Author

Should be merged after #6917 .

@pythonic64 pythonic64 force-pushed the use_color_instead_of_list_property branch from 9bdfa41 to ec255b0 Compare June 3, 2020 19:13
Copy link
Member

@matham matham left a 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.

@pythonic64
Copy link
Contributor Author

Added versionchanged tag.

Regression problem I can see is setting of individual components for color (with __setitem__) or setting of slice, but those are rare cases in my opinion.

@matham
Copy link
Member

matham commented Jun 9, 2020

Regression problem I can see is setting of individual components for color (with setitem) or setting of slice,

How do you mean? ColorProperty still uses a list to represent the color, doesn't it?

@pythonic64
Copy link
Contributor Author

Setting like

obj.color[0] = 0.5

won't dispatch event that color changed.

@matham
Copy link
Member

matham commented Jun 9, 2020

won't dispatch event that color changed.

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 ColorProperty to use a ObservableList instead of a normal list https://github.com/kivy/kivy/blob/master/kivy/properties.pyx#L710.

@pythonic64
Copy link
Contributor Author

How can we ensure that iterable of 3 or 4 components are used when setting a slice?
Example:

obj.color[:] = [0.1, 0.2, 0.3, 1.0, 1.0]

ColorProperty should ensure that 4 components or 3 with 1.0 alpha by default.

@matham
Copy link
Member

matham commented Jun 9, 2020

How can we ensure that iterable of 3 or 4 components are used when setting a slice?

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.

@pythonic64
Copy link
Contributor Author

Ok. I'll add a note to ColorProperty about slicing and that should cover it.

@matham
Copy link
Member

matham commented Jun 9, 2020

Cool. Although it's not just slicing. Presumably doing obj.color.extend([1, 2, 3, 4, 5, 6]) would also be a problem.

@pythonic64
Copy link
Contributor Author

Added note under versionchanged tag because indexing and slicing are now supported. See #6930.

@pythonic64
Copy link
Contributor Author

@matham Are we waiting for another developer to approve or I can squash and merge?

@matham
Copy link
Member

matham commented Jun 10, 2020

Yeah, because it's such a large change I'd like it if someone like @tshirtman would also approve.

@tshirtman
Copy link
Member

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.

@matham matham merged commit e91e768 into kivy:master Jun 14, 2020
@matham matham added the Notes: API-break API was broken with backwards incompatibality label Jun 14, 2020
@matham matham added this to the 2.0.0 milestone Oct 28, 2020
@matham matham changed the title Use ColorProperty instead of ListProperty for color property ColorProperty: Use ColorProperty instead of ListProperty for color property Dec 9, 2020
@matham matham added the Component: Widgets kivy/uix, style.kv label Dec 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Widgets kivy/uix, style.kv Notes: API-break API was broken with backwards incompatibality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants