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

RecycleView: Add behavior to set RV data using kv ids #6897

Merged
merged 4 commits into from
Jun 20, 2020

Conversation

matham
Copy link
Member

@matham matham commented May 26, 2020

Adds the RecycleKVIDsDataViewBehavior class. Fixes #6878.

It's similar to the existing RecycleDataViewBehavior class, except that the data can signify properties of objects named with an id in KV. E.g. given a KV rule::

<MyRule@RecycleKVIDsDataViewBehavior+BoxLayout>:
    Label:
        id: name
    Label:
        id: value

Then setting the data list with rv.data = [{'name.text': 'Kivy user', 'value.text': '12'}] would automatically set the corresponding labels.

Copy link
Member

@tshirtman tshirtman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks fine, though i think maybe it can be improved, see comment.

kivy/uix/recycleview/views.py Show resolved Hide resolved
sizing_attrs = RecycleDataAdapter._sizing_attrs
for key, value in data.items():
if key not in sizing_attrs:
name, *ids = key.split('.', maxsplit=1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the star? the maxsplit makes it a 2-tuple, so we should be able to directly use the value without indexing on the next line.

Also, i think like ids is not a great name here, as name actually refer to a kv id, and ids here would be a chain of one or more attributes.
And why the maxsplit actually? Do we really want to set an attribute with dots in it? wouldn't it be better to iterate on the names, getting the next value until the penultimate one and use setattr on this one?

wid, *attrs = key.split('.')
obj = self
for attr in attrs[:-1]:
    obj = getattr(obj, attr)
setattr(obj, attrs[-1], value)

or would that cause issues i can't think of right now?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the star? the maxsplit makes it a 2-tuple, so we should be able to directly use the value without indexing on the next line.

Because the maxsplit is 1 we can have at most 2 items. But it could also be only 1. In the latter case, ids will be empty.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, i think like ids is not a great name here, as name actually refer to a kv id, and ids here would be a chain of one or more attributes.

I feel like that's nitpicking because in the normal case it is the property name, except when it's interpreted as an id :) But also, I think you may have misunderstood what this is doing. Specifically, with the following KV examples:

MyRule:
    street: ''
    Label:
        id: address

<RootRule@RecycleKVIDsDataViewBehavior>:
    country: ''
    Label:
        id: phone
    MyRule:
        id: my_rule

Setting data to [{'country': 'US', 'phone.text': '123456', 'my_rule.street': 'Home ave', 'my_rule.address.text': '555'}]. Under the current code, my_rule will have a attribute named address.text created and set to 555. That's because we don't recursively go through rules like your example code. We only allow accessing the ids at the first level. Hence the only 1 split by period. Under your code, it would set the text property of the address widget to 555.

The reason is that I don't think it would be common in kivy to set properties this way for sub rules and sub-sub rules etc. So I don't think we should complicate the API by adding this. Because once we add that it's hard to remove such functionality and I'd prefer to wait until this specifically is requested. And even then I think we can just add such recursive ids access a doc recipe. For now, this API only allows setting the property of a widget named in the root rule of the class.

Copy link
Member

@tshirtman tshirtman Jun 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, i did think a recursive definition could be useful, but i agree it might be a strong commitment that we might not need to make.

Under the current code, my_rule will have a attribute named address.text created and set to 555

I think that's what i meant with "Do we really want to set an attribute with dots in it?", that seems like a weird requirement to me, and i think i'd rather produce an error, saying that recursive setting (what i would expect by putting dots in the key) is not supported, than create attributes with dots in them, that can only be accessed through getattr, and won't properly bind in kvlang.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose we can raise an exception if there is more than one dot, in case users think that it does work recursively.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I added that.

@matham matham requested a review from tshirtman June 19, 2020 22:03
Copy link
Member

@tshirtman tshirtman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i feel the exception message could be improved, i'm not sure i would immediately understand why it's a problem, but reading the doc of the class should help, and it's a cool feature.

@matham
Copy link
Member Author

matham commented Jun 20, 2020

Yeah, I agree the message can be improved. But I couldn't think of a way to state it without writing a essay so I figured explaining it in the docs will have to be sufficient :)

@matham matham merged commit 6aacc50 into kivy:master Jun 20, 2020
@matham matham deleted the kv_names branch June 20, 2020 04:13
@chavarinbonbyn
Copy link

Which kivy version will contain this change ?

@tshirtman
Copy link
Member

next RC or release of 2.0.

@matham matham added this to the 2.0.0 milestone Oct 28, 2020
@matham matham changed the title Add class that sets RV data using kv ids. RecycleView: Add behavior to set RV data using kv ids 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow using kv ids in RecycleView data keys
3 participants