SelectField Widget Bug

[Sean De La Torre]

I've been experimenting with the SelectField widget, and I found that
it does not render itself correctly when the option data is passed to
it in the render/insert call:

>>> from turbogears.widgets import *
>>> select = SelectField('test')
>>> select.render([(x,x) for x in range(10)])
'\n <SELECT CLASS="select_field" NAME="test" ID="test">\n \n
</SELECT>'

I found the source of the problem in turbogears/widgets/base.py. The
render/insert functions do not properly update the OptionWidget with
the input data. I believe this happens because the OptionWidget uses
an "options" field to store its data, while the other widgets use
"widget_value". I was able to fix the issue by changing the following
in base.py (starting at line 92):

def render(self, value=None, input_values={}, error=None,
format="html", convert=True, **kw):
if not self.template:
return None

+ if isinstance(self, OptionWidget) and value:
+ self.options = value

return view.render(self.create_dict(value, input_values,
error, convert=convert,
**kw),
template=self.template, fragment=1,
format=format)

def insert(self, value=None, input_values={}, error=None,
convert=True, **kw):
if not self.template:
return None

+ if isinstance(self, OptionWidget) and value:
+ self.options = value

return view.transform(self.create_dict(value,
input_values, error,
convert=convert, **kw),
self.template)

(New results after the patch):
>>> from turbogears.widgets import *
>>> select = SelectField('test')
>>> select.render([(x,x) for x in range(10)])
'\n <SELECT CLASS="select_field" NAME="test" ID="test">\n \n
\n
<OPTION VALUE="0">0</OPTION>\n \n \n
<OPTION VALUE=
"1">1</OPTION>\n \n \n <OPTION
VALUE="2">2</OPTION>\n
\n \n <OPTION VALUE="3">3</OPTION>\n \n
\n
<OPTION VALUE="4">4</OPTION>\n \n \n <OPTION
VALUE="5">5</O
PTION>\n \n \n <OPTION VALUE="6">6</OPTION>\n
\n
\n <OPTION VALUE="7">7</OPTION>\n \n \n
<OPTION
VALUE="8">8</OPTION>\n \n \n <OPTION
VALUE="9">9</OPTION>\n
\n </SELECT>'
>>>

This is the output I originally expected. Am I doing something wrong,
or is this a bug?

Sean

[Kevin Dangoor]

The way that widget is implemented, it assumes that the options are
static and the widget value is the selected value. Being able to
change the options at rendertime seems reasonable, but you can't
change "self" attributes in render/insert/create_dict because that's
not threadsafe.

Kevin

--
Kevin Dangoor
Author of the Zesty News RSS newsreader

email: k...@blazingthings.com
company: http://www.BlazingThings.com
blog: http://www.BlueSkyOnMars.com

[Sean De La Torre]

This may be a dumb question, but I'll ask anyway. Why is it not
threadsafe?

[Bob Ippolito]

On Dec 30, 2005, at 2:42 PM, Sean De La Torre wrote:

> This may be a dumb question, but I'll ask anyway. Why is it not
> threadsafe?

It's not threadsafe because the same widget instance is used across
requests which means that all of its instance variables should be
immutable, or that gratuitous locking and/or copying should be used
so that a given thread has a consistent view of the instance.

-bob

[Sean De La Torre]

That makes sense. Thanks!

[jpel...@gmail.com]

I just came across this thread, and was really suprised by this:

Bob Ippolito wrote:
> On Dec 30, 2005, at 2:42 PM, Sean De La Torre wrote:
>
> > This may be a dumb question, but I'll ask anyway. Why is it not
> > threadsafe?
>
> It's not threadsafe because the same widget instance is used across
> requests

I hope I'm missing something, because I don't see how this can be the
case-- or if it is the case, I don't see how it could be workable. If I
use a widget, I'm not able to change its properties after
instantiation? In other words, this is not ok (excuse any misspelling,
I'm away from my own machine):

def whatever(self, **kw):
foo = widgets.TextField('bar')
if kw.get('something'):
foo.attr['class'] = 'one'
else:
foo.attr['class'] = 'two'
...

Is it really the case that the above is not threadsafe? If not, where
in the request cycle does the widget become (potentially) shared --
only inside of the template? Or am I just misunderstanding something?

If I'm not just on crack here, I'd argue that shared widgets anyplace
is bad; not being able to manipulate widgets per-request is going to
trip a lot of people up, especially since it's going to cause the sorts
of bugs that only appear in real-world use and cause developers to say
"well, I can't reproduce it [with my single-thread dev config]...."

JP

[Kevin Dangoor]

On 1/2/06, jpel...@gmail.com <jpel...@gmail.com> wrote:
> def whatever(self, **kw):
> foo = widgets.TextField('bar')
> if kw.get('something'):
> foo.attr['class'] = 'one'
> else:
> foo.attr['class'] = 'two'
> ...
>
> Is it really the case that the above is not threadsafe? If not, where
> in the request cycle does the widget become (potentially) shared --
> only inside of the template? Or am I just misunderstanding something?

It's really very similar to a cherrypy controller. You can't use
"self" attributes in a cherrypy controller because the same controller
instance is used for each request. Consider how a form is used:

myform = TableForm([TextField("name"), TextField("address")])

class MyController(controllers.Controller):
@turbogears.expose(html="my.template")
def index(self):
return dict(form=myform)

@turbogears.expose(inputform=myform)
def save(self, name, address):
...

You don't get a new form for each request. So, as it stands now,
widgets are stateless. The parameters that define the basic properties
of a widget that are the same over and over are sent in via the
constructor. The options that can change from request to request are
sent in via render() or insert().

This is something that I've seen trip people up a couple of times
already. That may partly be because there's no widget documentation
yet. But, we can think about the implications of making widgets get
instantiated for each rendering.

Kevin

[jpel...@gmail.com]

Ah, I see. Thank you for the explanation.

As I said, I think this is going to cause a lot of trouble, docs or no
-- it's just so counter-intuitive. I'd suggest at least adding some
kind of thread-local storage to the base widget, so that the docs can
give a simple solution ("to set per-request properties in a widget, use
widget.request [or whatever]").

A greater investment that I think would be worthwhile would be to
profile the two cases (current, vs instantiate/copy per request) and
see how expensive the (to me at least) intuitive scenario is. If you or
other folks are interested, I can take a crack at that next week -- my
access this week is still pretty lousy. Also, I have the flu. Ah,
vacation! ;)

JP

[Kevin Dangoor]

Bummer. Sad way to spend vacation!

I can't imagine that it's super expensive to instiantiate the needed
widgets for a given request. It just didn't seem important to do so,
to me.

Here's why: for something to be meaningful to a request, it needs to
end up in the dictionary that goes to the template. It's already
possible to pass things in to that dictionary via insert() and
render() and the widget itself can use update_dict to modify the
dictionary on the way in.

I looked at it very much like a type of controller. You give it a
baseline set of parameters that don't vary and send in the variable
stuff at render time.

that said, it *has* tripped people up, so eliminating that trip-up
spot is a worthy goal.

Kevin

>
> JP