Warning: Can't synchronize with repository "(default)" (Unsupported version control system "svn": No module named svn). Look in the Trac log for more information.

Ticket #607 (closed defect: fixed)

Opened 12 years ago

Last modified 10 years ago

WidgetsDeclaration behaves unexpectedly

Reported by: bjourne Owned by: anonymous
Priority: normal Milestone: 0.9a2
Component: TG Widgets Version:
Severity: normal Keywords:
Cc:

Description

>>> from turbogears.widgets import WidgetsDeclaration, TextField
>>> class Foo(WidgetsDeclaration):
...     name = TextField()
...
>>> Foo().name
Traceback (most recent call last):
  File "<stdin>", line 1, in ?
AttributeError: 'Foo' object has no attribute 'name'

This is because MetaWidgetsDeclaration? deletes the declared widgets before adding them to its list. Clearly, that is not good and in this case Foo().name should be an alias to Foo()[0].

Change History

comment:1 Changed 12 years ago by alberto

Looks like a nice shortcut to add, I'm not so sure though as WidgetsDeclaration? is supposed to be a "special" list, that is, a list you can initialize in a declaratiive way. Nothing more, nothing less.

This could be accomplished with getattribute scanning the whole list for a widget named "name" but that O(N) behaviour for attribute lookup doesn't seem too nice. A separate dict for fast access to attributes could be done but would complicate things when the list is modified as this dict should be kept in sync :/ If someone has a better idea it will be gladly heard about :)

Alberto

comment:2 Changed 12 years ago by Karl G

MetaWidgetsDeclaration? has to delete the attributes in order for inheritance to work properly.

They can be re-aliased using a getattr but then you run into naming problems. The declaration gets translated into a list. How much more confusing would it be, for example, when you have a field named extend or one of the other list methods? You can put the widgets into their own attribute (e.g. Foo.w.name) but that still causes the declaration to behave unexpectedly.

comment:3 Changed 12 years ago by michele

That's not a bug but a design decision I've made, if you sum two list you will loose this attribute, a WidgetsDeclaration? is just a convenient method of declaring a list, nothing more than that and it should be kept so simple for this reason.

Also remember that this design decision allows you to not get any name conflict as Karl said.

You can easily achieve the thing you're trying to do by doing:

>>> class Foo(WidgetsDeclaration):
...     name = TextField()
...
>>> Foo()[0]
TextField(field_class='textfield', attrs={})
>>>

The bottom line for me is that we should make it clear that a WidgetsDeclaration? instance is just a list, anyway that should be clear as you're passing it to the fields argument, and from the source code:

class WidgetsDeclaration(list):

I know the WidgetsDeclaration? thing involves a small amount of logic but that's far less than the previous solution or what happens with other similar solutions.

comment:4 Changed 12 years ago by michele

For further reference on the problems I mentioned take a look at #454.

comment:5 Changed 12 years ago by michele

In my first comment I wanted to say "a small amount of magic".

comment:6 Changed 12 years ago by Karl G

Here's a quick (i.e. untested) implementation for a dict based getattr:

class MetaWidgetsDeclaration(type):
    def __new__(meta, class_name, bases, class_dict):
        declared_widgets = []
        widget_attrs = {}
        for name, value in class_dict.items():
            if isinstance(value, Widget):
                if not value.is_named:
                    value.name = name
                declared_widgets.append(value)
                widget_attrs[name] = value
                del class_dict[name]
        declared_widgets.sort(lambda a, b: cmp(a._declaration_counter,
                                               b._declaration_counter))
        cls = type.__new__(meta, class_name, bases, class_dict)
        cls.declared_widgets = declared_widgets
        if hasattr(cls,'widget_attrs'):
            cls.widget_attrs.update(widget_attrs)
        else:
            cls.widget_attrs = widget_attrs
        return cls

class WidgetsDeclaration(list):
    __metaclass__ = MetaWidgetsDeclaration

    def __init__(self):
        super(WidgetsDeclaration, self).__init__()
        self.extend(self.declared_widgets)

    def __getattr__(self,name):
        try:
            return self.widget_attrs[name]
        except KeyError:
            raise AttributeError, name #makes pickling work
    def __setattr___(self,name,value):
        return self.widget_attrs[name] = value

    def __delattr__(self,name):
        dval = self.widget_attrs.get(name,None)
        if dval:
            self.remove(dval)
            return del self.widget_attrs[name]
        else:
            raise AttributeError, name

    def __setitem__(self,index,value):
        try:
            name = self[index].name
            return self.widget_attrs[name] = value
        except:
            raise AttributeError, name

    def __delitem__(self,index):
        try:
            name = self[index].name
            del self.widget_attrs[name]
        except:
            raise AttributeError, name

The attr parts I'm pretty sure about, but I'm not positive that the item parts are correct (I've never overridden a list). I'm not fond of the approach, however, and agree with michele.

comment:7 Changed 12 years ago by bjourne

Sorry, I disagree it is not clear to me that it is a list. If it was, this bug report wouldn't exist :-P. My use case when the problem occured was this:

class DetailsFields(WidgetsDeclaration):
    name = TextField(validator=validators.NotEmpty, label = _("Name"))
    email = TextField(attrs={"size" : 40}, label = _("Email"))
    phone = TextField(label = _("Phone"))
    address = TextField(label = _("Address"))
self.form = Tableform(fields = DetailsFields(), submit_text = _("Apply"))

# And then in some other part of the code
def fillForm(self, person):
    self.form.fields.name = person.name
    self.form.fields.email = person.email
    self.form.fields.phone = person.phone
    self.form.fields.address = person.address

IMHO, that is code that should work - principle of least astonishment.

comment:8 Changed 12 years ago by michele

Thanks for providing a use case, is much more useful for a discussion. ;-)

Actually you can do:

class DetailsFields(WidgetsDeclaration):
    name = TextField(validator=validators.NotEmpty, label = _("Name"))
    email = TextField(attrs={"size" : 40}, label = _("Email"))
    phone = TextField(label = _("Phone"))
    address = TextField(label = _("Address"))
self.form = Tableform(fields = DetailsFields(), submit_text = _("Apply"))

# And then in some other part of the code
def fillForm(self, person):
    self.form.field_for('name').name = person.name
    self.form.field_for('email').name = person.email
    self.form.field_for('phone').name = person.phone
    self.form.field_for('address').name = person.address

I should note that you shouldn't modify widgets attribute in other points than their initialization since this is not threadsafe, so we can even say that WidgetsDeclaration? enforces good use of them. :P

Again, I'm sorry but as I said we can't make this thing work in reliable way, we should clearly state that WidgetsDeclaration? is a magical way of declaring a list nothing more and you can't access those attributes.

Even by providing the type of access you want we can't make it work since:

self.form = Tableform(fields = DetailsFields() + [TextField(name="hello")], submit_text = _("Apply"))

will cause the creation of a new list that looses all attributes:

>>> class Foo(WidgetsDeclaration):
...     name = TextField()
...
>>> c = Foo()
>>> dir(c)
['__add__', '__class__', '__contains__', '__delattr__', '__delitem__', '__delslice__', '__dict__', '__doc__', '__eq__', '__ge__', '__getattribute__', '__getitem__', '__getslice__', '__gt__', '__hash__', '__iadd__', '__imul__', '__init__', '__iter__', '__le__', '__len__', '__lt__', '__metaclass__', '__module__', '__mul__', '__ne__', '__new__', '__reduce__', '__reduce_ex__', '__repr__', '__reversed__', '__rmul__', '__setattr__', '__setitem__', '__setslice__', '__str__', '__weakref__', 'append', 'count', 'declared_widgets', 'extend', 'index', 'insert', 'pop', 'remove', 'reverse', 'sort']
>>> c = c + [TextArea(name="hello")]
>>> dir(c)
['__add__', '__class__', '__contains__', '__delattr__', '__delitem__', '__delslice__', '__doc__', '__eq__', '__ge__', '__getattribute__', '__getitem__', '__getslice__', '__gt__', '__hash__', '__iadd__', '__imul__', '__init__', '__iter__', '__le__', '__len__', '__lt__', '__mul__', '__ne__', '__new__', '__reduce__', '__reduce_ex__', '__repr__', '__reversed__', '__rmul__', '__setattr__', '__setitem__', '__setslice__', '__str__', 'append', 'count', 'extend', 'index', 'insert', 'pop', 'remove', 'reverse', 'sort']
>>>

Note that declared_widgets is not there anymore.

comment:9 Changed 12 years ago by bjourne

Thanks for telling me about field_for(). I haven't tested it but I guess doing stuff like self.form.fields.field_for("name").default = "Kalle" works? Still it isn't very obvious. Adding a WidgetsDeclaration? and a list can be fixed in this way:

class Foo(WidgetsDeclaration):
    def __add__(self, other):
        self.extend(other)
        return copy.copy(self)

r = Foo() + [1, 2, 3]
assert isinstance(r, Foo)

Other then that a getattr and setattr like Alerto suggested while keeping the class a list and doing O(N) lookup should work fine AFAICT. O(N) doesn't matter much when your dataset is less than 10 items. IMHO the naming conflict is a much less severe problem than the disappearing attributes. SQLObject for example, has the same type of naming conflict:

class Foo(SQLObject):
    select = StringCol()
assert isinstance(Foo.select, property)

Yet that doesn't bother anyone.

comment:10 Changed 12 years ago by michele

Well the name conflict does bother me, even in SQLObject, that's why we come up with WidgetsDeclaration?.

I'm not against a getattrs and setattrs solution, but this will not work if you have for some reason a field named "extend" or "append", that's a BIG problem for me and the one we avoided thanks to this solution I don't want it coming back.

Also keep in mind that you shouldn't declare or modify fields attribute in this way because it's not threadsafe so I'm against making it more straightforward.

Anyway we should ask what Kevin thinks about this...

comment:11 Changed 12 years ago by kevin

This would appear to be partly a documentation issue. One thing we can do that may clear it up would be to renamed WidgetsDeclaration? to WidgetList. This may make it clearer that the object in question is, first and foremost, a list.

The documentation part of this is making sure that the introductory documentation for widgets really stresses the re-entrant nature of widgets and the threadsafety requirements that results in.

I think the only change I'd be willing to support at this point is renaming the WidgetsDeclaration? class.

comment:12 Changed 12 years ago by godoy

+1 for renaming it. It makes things clearer as well as keeping the actual syntax, minimizing problems with changes.

comment:13 Changed 12 years ago by michele

+1 also from me, as I said the first time I proposed this the name was not the best most probably, I do agree with Kevin that we shouldn't change other things but just the name...

comment:14 Changed 12 years ago by michele

  • Status changed from new to closed
  • Resolution set to fixed
  • Milestone set to 0.9a2

Done on r952.

Note: See TracTickets for help on using tickets.