Lang: set id name as attribute of root widget #812

Merged
merged 2 commits into from Mar 31, 2013

Conversation

Projects
None yet
5 participants
Owner

akshayaurora commented Nov 21, 2012

I was hoping to get a discussion started about this. There have been a few requests asking for easy access of id's in .python code. The way I'm implementing this may not be desirable due to it's hackish nature. However I'd like to get your opinion on the right approach for this.

This code; for a id: my_id creates a attribute my_id in root widget that can be accessed in python like
root_widget.my_id

I used the following code to test

UPDATE: updated example below to show usage in python as well
UPDATE2: updated example to reflect the use of ids

from kivy.app import App
from kivy.uix.tabbedpanel import TabbedPanel
from kivy.lang import Builder

Builder.load_string('''
<MyClass>:
    do_default_tab: False
    TabbedPanelItem:
        text: 'test'
        id: tpitem
        Button:
            id: my_button
            on_release:
                print 'from kv', root.ids.my_button, root.ids.tpitem
                root.do_release()
''')

class MyClass(TabbedPanel):
    def do_release(self):
        print 'from py', self.ids.my_button, self.ids.tpitem

class MainApp(App):

    def build(self):
        return MyClass()

if __name__ == '__main__':
    MainApp().run()

Owner

tshirtman commented Nov 21, 2012

That's a nice trick, but i'm not sure putting that in root widget attribute is a good idea, there is no documented limitation on the id names, and we could have name collisions, i think it would be better just to keep that in lang.ids or something like this.

Owner

tito commented Nov 21, 2012

Agreed about the idea, but same thoughts as Gabriel, storing the id in the root widget will do introduce collision in the current app.

Then, we could add an "ids" DictProperty() in the Widget class, and jump from it?

Owner

akshayaurora commented Nov 21, 2012

should it not be the responsibility of the user to provide unique id's ? Just like in normal Python?

Owner

tito commented Nov 21, 2012

Yes of course, but doing it after is dangerous. I already know that some of our example will fail :)

Owner

tshirtman commented Nov 21, 2012

@akshayaurora yes, but what if someone want to give "parent" as id, or some other existing property of the widget, you get a collision.

Contributor

hansent commented Nov 21, 2012

Does it just gave you access or create an ObjectProperty ?

usually the we we've done it, is to use the id in kivy to then assign to an objectproperty of the root (but this, I agree violates DRY and is confusing)

when we added the app keyword in KV, tito also did a small change for on_parent event so you could actually do this now:

TabbedPanel:
    TabbedPanelItem:
        on_parent: root.tpitem = self
        Button:
            on_parent: root.my_button = self
            on_release: print root.my_button, root.tpitem

This will just set a regular python attribute usually, but if TabbedPanelItem has an objectproperty called e.g. tpitem, it ends up as a proper ObjectProperty and you can use root.tpitem iside the rule and all teh events will get hooked up properly

Owner

akshayaurora commented Nov 21, 2012

@hansent Thanks for reminding me about that procedure :).

The use of root_widget.ids.<id_name> for accessing instead of root_widget.<id_name> would avoid the conflicts mentioned and still allow for use of Properties for event propagations like before.

Owner

akshayaurora commented Nov 21, 2012

@hansent So, I re-read our comment carefully.

This implementation just provides a ref in python code for the widgets that have a id in kv lang.

Is there a advantage of providing a ObjectProperty versus just providing a ref in this situation? I understand the need for using ObjectProperty when the Object is changing, but in case of kvlang when you define a id for a Widget you kind of expect it to not change right?

I'm probably missing a use case that I havn't thought of yet.

Owner

tito commented Nov 21, 2012

I was wondering the impact of it, and the side-cases:

  1. id is per-rule. But you might have multiple rule for the same class: just have a look at Button: it use a Label, so the Label class is applied before the Button. So the current implementation will erase the ids (if exists) to the latest applied rule. Maybe we could find a way to merge all of them.
  2. using directly the dict from the rule might not be cool: it will contain "root" and "self": it's redundant, we already know it's us. It would be better to not include them.
  3. in term of performance, i have an idea: in our case, it would be nice to not create the dictionary if we don't have ids. But in case the user access to it, it must have a dict. So what about extending the DictProperty to allow:
    3.1 usage of the dictproperty same as the QueryDict
    3.2 by default, don't create a dict, but create it the first time we access it.
  4. ensure that adding a reference in the dict, attached to the widget instance (or in a DictProperty) will not break the GC collection.
Contributor

hansent commented Nov 21, 2012

Is there a advantage of providing a ObjectProperty versus just providing a ref in this situation?
well if you use just a ref, then if you had a label somewhere else in the same rule with:
text: root.tpitem.text

that text would only be set once, whereas if tpitem is a ObjectProperty of the root widget, the label text will also be updated, when the root.tpitem.text is changed

I understand the need for using ObjectProperty when the Object is changing, but in case of
kvlang when you define a id for a Widget you kind of expect it to not change right?
i agree, but there is still a chance for name collisions because of how the rules inherit / cascade like tito described.

An orthogonal approach to this problem is more of a jquery like functionality. e.g. allow setting id's, or uid (we already have this property anyways right?), which are actually like real ids/names and then have a function like kvquery("widgetid"), which returns the widget with this id. I've done something liek that in a branch once....performance, just like with jquery can be an issue, but i guess it depends on how the user uses it and if the whole tree has to be searched. but it can also be flexible to allow e.g. all_labels = kvquery(class=Label), or all_with_blank_text = kvquery(text="")

opqopq commented Dec 28, 2012

What about at least having the id filled in the widget children ? If I'm not mistaken, in the instanciated widget (Python file) the id of the children are not set, even though they are in the kv file.
Let me explain with an example
kvFile:

: Label: id: label

pyFile:
class frame:
def fun(self):
for child in self.children: print child.id # print nothing and not 'label' ...As of today, this ID is not set.

If the "ids" where set on the child in the instanciated widget (in the python file), we could at least loop on these to find a widget. And then, why not add a content dict to the widget, linking 'id' and 'child'.

akshayaurora reopened this Mar 2, 2013

Owner

akshayaurora commented Mar 2, 2013

I deleted the old branch and created a new one, hopefully this does follow all the guidelines tito suggested.

I am testing with the following code now,

from kivy.uix.gridlayout import GridLayout
from kivy.lang import Builder


Builder.load_string('''
<Label>:
    id: virgin_label
    Image:
        id: lbl_image
        pos: root.pos

<Button>:
    id: virgin_button
    Image:
        id: but_image
        pos:root.pos

<MainView>:
    rows: 1
    Button:
        id: but_1
        text: 'bg'
        on_press:
            self.clear_widgets()
            print self.ids
            print self.ids.lbl_image
    Button:
        text: 'gb'
        on_press:
            print self.ids
            self.ids.lbl_image = None
        on_release:
            print root.ids
            # this raises a error right now, however is this the right approach?
            # i.e should we delete the key if it's set to None?
            print self.ids.lbl_image
''')


class MainView(GridLayout):
    pass

if __name__ == '__main__':
    from kivy.base import runTouchApp
    runTouchApp(MainView(width=800))
  • id is per-rule. But you might have multiple rule for the same class: just have a look at Button: it use a Label, so the Label class is applied before the Button. So the current implementation will erase the ids (if exists) to the latest applied rule. Maybe we could find a way to merge all of them.

Id's should now be merged.

  • using directly the dict from the rule might not be cool: it will contain "root" and "self": it's redundant, we already know it's us. It would be better to not include them.

self and root are now removed from the dict.

  • in term of performance, i have an idea: in our case, it would be nice to not create the dictionary if we don't have ids. But in case the user access to it, it must have a dict. So what about extending the DictProperty to allow:
    3.1 usage of the dictproperty same as the QueryDict

I tried My hand at it , I don't totally understand how the properties work however I made some educated guesses.
It seems to work from the outside. If this is the wrong way, I'm sure you'd let me know :).

3.2 by default, don't create a dict, but create it the first time we access it.

I kind of am totally confused as to how-to go about this, so didn't touch this part yet.

ensure that adding a reference in the dict, attached to the widget instance (or in a DictProperty) will not break the GC collection.

Using weakrefs for the objects now, so shouldn't be a issue. Though still needs testing.

Owner

tito commented Mar 6, 2013

The current ids implementation is not enough: self.ids. will return a weakref, not the final object. So if i really want to use it, i need to self.ids.().
I would not be able to bind it too: "self.ids.().", binding will not work on that case.

I guess that a new "WeakValueDictProperty", based on the http://docs.python.org/2/library/weakref.html#weakref.WeakValueDictionary, could resolve the issue, but i never used it. At least, a simple DictProperty will not work.

Owner

tito commented Mar 6, 2013

Otherwise, it seems ok :)

Owner

tito commented Mar 31, 2013

Looks good, let's merge :)

@tito tito added a commit that referenced this pull request Mar 31, 2013

@tito tito Merge pull request #812 from kivy/lang_id
Lang: set id name as attribute of root widget
577277f

@tito tito merged commit 577277f into master Mar 31, 2013

akshayaurora deleted the lang_id branch Mar 31, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment