Cleaned up widget API
Kevin Dangoor
API is looking a lot better. I wanted to open up for comments,
suggestions and patches.
First of all, you can "activate" the new widgets code like this:
from turbogears import widgets
widgets.use_new_widgets()
So, what's different?
* create_dict is gone! it has been replaced by adjust_value and
update_data. Subclassing is much cleaner now, I think
* all of those crazy parameters to create_dict, insert and render have
been eliminated. Just pass in appropriate keywords for your widgets.
(the value parameter for the first parameter is still supported, of
course...)
* widget.insert has been renamed widget.display
* widget.input has been renamed widget.validate
* display() now returns an Element (you'll want to update TurboKid,
but it'll probably still work even if you don't). render() works off
of display, so if you're trying to generate HTML in some non-Kid form,
you really only need to implement display.
* No more label widget associated with a field. form
fields now just have label text. Use of "labeltext"
previously should now be "label"
* form.widgets is now form.widgets["fields"]
* disabled_fields on forms has become disabled_widgets
* as mentioned earlier, widgets can no longer be modified after
they've been displayed, to help ensure threadsafe use of the widget
The widget sample and source functionality is going to be broken out
into a new class. It will become more powerful and useful in the
process (ultimately leading to a better Toolbox Widget Browser).
Limitations:
* the select widgets are untested, as far as I know. they're probably
broken and, at the very least, they don't support callables as they
should
* CalendarDatePicker and AutoCompleteField are just plain missing
* the widget browser in the toolbox is *completely* broken
I'm going to work on those 3 things that I list as limitations above.
Anything else is fair game for patches.
I hope people find this rearrangement pleasant... it shouldn't take
long to convert widgets from the old style to the new.
Kevin
--
Kevin Dangoor
Author of the Zesty News RSS newsreader
email: k...@blazingthings.com
company: http://www.BlazingThings.com
blog: http://www.BlueSkyOnMars.com
Kevin Dangoor
widget implementation is based on code by Michele Cella. I've also
blended in input from David Bernard who had some proposals up on the
wiki.
A couple more things of note:
* the widget templates are much simpler (code has productively moved
into Python)
* error display is now the responsibility of the form and not the
widget (though you certainly can make widgets that include the error
display)
* most of the "form" related widgets have moved into the "newforms"
module. This represents a cleaner separation between widgets that just
display things and widgets that actually get input
* widgets that combine other widgets now have a common ancestor
(CompoundWidget) to help out. Karl Guertin had a proposal floated for
one of those, and I'm not sure if all of his ideas made it in.
Kevin
Kevin Dangoor
to the template namespace. This encourages two things:
1) threadsafe use (since you can't just dump things in the widget and
grab them from the template)
2) moving more computation to the Python code (since you'll likely
need to create an update_data method to give data to the template
anyhow)
Kevin
On 2/2/06, Kevin Dangoor <dan...@gmail.com> wrote:
Michele Cella
> I'm not completely done with the widgets work, but I think the main
> API is looking a lot better. I wanted to open up for comments,
> suggestions and patches.
>
> First of all, you can "activate" the new widgets code like this:
> from turbogears import widgets
> widgets.use_new_widgets()
>
> So, what's different?
>
> * create_dict is gone! it has been replaced by adjust_value and
> update_data. Subclassing is much cleaner now, I think
Hurra. :-)
>
> * all of those crazy parameters to create_dict, insert and render have
> been eliminated. Just pass in appropriate keywords for your widgets.
> (the value parameter for the first parameter is still supported, of
> course...)
>
Regarding this, since we support options at the constructor I think we
should also add options here at display time (and fix the problem with
_collect_options making options a property see #490).
That's just to provide a default suggested way of passing options (of
any type, choices for a selectfield) along from a form to it's fields
as the actual form I made does. What do you think?
I mean:
def display(self, value=None, options=None, convert=True, **kw):
Just to be coherent:
default -> value
options -> options
>
> * form.widgets is now form.widgets["fields"]
>
> * disabled_fields on forms has become disabled_widgets
I left it to be disabled_fields AFAIK. :-/
>
> Limitations:
>
> * the select widgets are untested, as far as I know. they're probably
> broken and, at the very least, they don't support callables as they
> should
>
I noticed they don't work anymore as on my initial implementation (that
Alberto tested), anyway I will left the selection field to you so you
can check things, the CheckBox is now broken also (still using
create_dict and not respecting the default value), I'm cooking a patch
(with a test to show the supposed behavior).
Great work by the way.
Ciao
Michele
Karl Guertin
> * widgets that combine other widgets now have a common ancestor
> (CompoundWidget) to help out. Karl Guertin had a proposal floated for
> one of those, and I'm not sure if all of his ideas made it in.
I've been holding off on messing around with compound widgets until
the majority of the widgets updates had landed. I'll definitely be
looking into this and the rest of the widgets changes over the
weekend.
Matthew Bevan
> the majority of the widgets updates had landed. I'll definitely be
> looking into this and the rest of the widgets changes over the
> weekend.
Until widgets have calmed down, my CMS has no editing capability. ^_^;
I want to use stored sets of widgets to automatically update all of my CMS
objects from one central controller.
Michele Cella
>
> I noticed they don't work anymore as on my initial implementation (that
> Alberto tested), anyway I will left the selection field to you so you
> can check things, the CheckBox is now broken also (still using
> create_dict and not respecting the default value), I'm cooking a patch
> (with a test to show the supposed behavior).
>
I've attached the patch to ticket 490, it adds a bunch of tests (for
CheckBox, SelectFields, ...), I've also ported all the widget in
newforms.py to use update_data and fixed some things here and there.
Finally I've integrated Alberto's patch for callable options and added
a test for this.
All SelectionField seems to work just fine, at least from the tests
I've made. :-)
Ciao
Michele
Alberto
> I hope people find this rearrangement pleasant... it shouldn't take
> long to convert widgets from the old style to the new.
>
Great! Looks as the widget-waters are calming down, aren't they? I'll
take this words as an invitation to update #502 to the new-new-style
and the rest of my apps widgets... (please stop me before it's too
late... ;)
Nice work both of you! I'll post anything I find
broken/strange/whatever at the Trac.
Regards, Alberto
Kevin Dangoor
As noted below, the widget is no longer in the namespace of the
template. What I didn't mention is that I simplified a lot of the
variable names. Since these templates are very special purpose and
just for widgets, things like "widget_value" just became "value". At
this point, using simpler names seemed to make sense, and it didn't
seem to me that name collisions were going to be a giant problem.
Kevin Dangoor
>
> Kevin Dangoor wrote:
> > * all of those crazy parameters to create_dict, insert and render have
> > been eliminated. Just pass in appropriate keywords for your widgets.
> > (the value parameter for the first parameter is still supported, of
> > course...)
> >
>
> Regarding this, since we support options at the constructor I think we
> should also add options here at display time (and fix the problem with
> _collect_options making options a property see #490).
I think "options" should only exist for select fields. Otherwise, a
widget should use named parameters for the things that are of
interest/value to it.
One thing that may be useful to do is allow you to put a list of
widget attributes that you'd like to make available to the template
and have that do the equivalent of:
d[that_attribute] = d.get(that_attribute, getattr(self, that_attribute))
That would make all of these types of things overrideable at display()
time. We could, if it's desirable, make that automatically call a
callable, but I'm a bit less certain on that one.
> > * disabled_fields on forms has become disabled_widgets
>
> I left it to be disabled_fields AFAIK. :-/
I think I may have actually written that note backwards :(
It's disabled_fields.
> I noticed they don't work anymore as on my initial implementation (that
> Alberto tested), anyway I will left the selection field to you so you
> can check things, the CheckBox is now broken also (still using
> create_dict and not respecting the default value), I'm cooking a patch
> (with a test to show the supposed behavior).
Thanks for writing those tests! That was a big help! Always nice to
know that more of the functions are tested.
> Great work by the way.
Thanks, and same to you!
Kevin
Michele Cella
> Man, my notes on these changes were awful.
>
> As noted below, the widget is no longer in the namespace of the
> template. What I didn't mention is that I simplified a lot of the
> variable names. Since these templates are very special purpose and
> just for widgets, things like "widget_value" just became "value". At
> this point, using simpler names seemed to make sense, and it didn't
> seem to me that name collisions were going to be a giant problem.
>
What name collisions are you refering to? I have not noticed them or
something strange going on.
Tests are all running fine.
Ciao
Michele
Kevin Dangoor
I was referring to the name collisions that don't exist :)
If memory serves, I originally passed "widget_value" to the template
instead of "value" because I didn't want to collide with variable
names used by user-created widgets. Those are the name collisiions
that i think are not much of an issue.
Kevin
Michele Cella
>
> I was referring to the name collisions that don't exist :)
>
> If memory serves, I originally passed "widget_value" to the template
> instead of "value" because I didn't want to collide with variable
> names used by user-created widgets. Those are the name collisiions
> that i think are not much of an issue.
Ah, all ok then.
I interpreted your words in the opposite way. :D
Ciao
Michele
Michele Cella
>
> I think "options" should only exist for select fields. Otherwise, a
> widget should use named parameters for the things that are of
> interest/value to it.
>
The problem is passing them along the way from a form container to it's
fields at display time, if we know that we can always rely upon "value"
and "options" (as it is now) being passed by a form (or another
compound widget) to it's fields we can just use them in a general way
for passing dynamics things (in this case a SelectField is a special
case that uses them for collecting possible choices).
field.display(value=value.get(field.name), options.get(field.name,
{}))"
That's what ended up on my last patch, r629 (maybe you didn't notice it
:P).
In this way you need to construct a options dictionary along with the
value one that you pass to the template from your controller.
Other solutions?
> One thing that may be useful to do is allow you to put a list of
> widget attributes that you'd like to make available to the template
> and have that do the equivalent of:
>
> d[that_attribute] = d.get(that_attribute, getattr(self, that_attribute))
>
> That would make all of these types of things overrideable at display()
> time. We could, if it's desirable, make that automatically call a
> callable, but I'm a bit less certain on that one.
>
Yep, that could be useful, I've done something similar for the last
patch on #490 (attributes support), but we should also take care of
mutable (list/dict) instance attribute.
A shortcut like the one you mentioned could be useful.
>
> Thanks for writing those tests! That was a big help! Always nice to
> know that more of the functions are tested.
I'm getting addicted to write tests, it's a nice feeling when they
pass. :-)
>
> > Great work by the way.
>
> Thanks, and same to you!
>
:-)
Ciao
Michele
Max Ischenko
Traceback (most recent call last):
File "D:\Projects\www.knigoman.com.ua\start-bookswap.py", line 55, in
?
from bookswap.controllers import Root
File "D:\Projects\www.knigoman.com.ua\bookswap\controllers.py", line
211, in ?
validator=validators.Number(default=None)),
File
"d:\projects\3rd-party\turbogears\turbogears\widgets\newbase.py", line
42, in widgetinit
func(self, *args, **kw)
TypeError: __init__() takes at least 2 non-keyword arguments (1 given)
The code:
add_book_form = TableForm(name='addbook',
submittext=_(u'Добавить книгу'),
widgets=[
TextField(name='location', label=_(u'xx'), default=_(u'x')),
TextField(name='price', label=_(u'x'),
validator=validators.Number(default=None)),
])
Michele Cella
>
> The code:
> add_book_form = TableForm(name='addbook',
> submittext=_(u'Добавить книгу'),
> widgets=[
Change from "widgets" to "fields".
Ciao
Michele
Kevin Dangoor
> >
> > I think "options" should only exist for select fields. Otherwise, a
> > widget should use named parameters for the things that are of
> > interest/value to it.
> >
>
> The problem is passing them along the way from a form container to it's
> fields at display time, if we know that we can always rely upon "value"
> and "options" (as it is now) being passed by a form (or another
> compound widget) to it's fields we can just use them in a general way
> for passing dynamics things (in this case a SelectField is a special
> case that uses them for collecting possible choices).
>
> field.display(value=value.get(field.name), options.get(field.name,
> {}))"
Actually, here's what's in the form right now:
field.display(value=value.get(field.name),
**options.get(field.name, {}))
So *forms* still have options allowed for each field, but they get
passed as keyword arguments to each individual field's display method.
> That's what ended up on my last patch, r629 (maybe you didn't notice it
> :P).
>
> In this way you need to construct a options dictionary along with the
> value one that you pass to the template from your controller.
>
> Other solutions?
I think it's fine for forms and select widgets to retain "options" for
their particular uses. I don't think the other widgets can just use
plain keyword arguments though.
> > One thing that may be useful to do is allow you to put a list of
> > widget attributes that you'd like to make available to the template
> > and have that do the equivalent of:
> >
> > d[that_attribute] = d.get(that_attribute, getattr(self, that_attribute))
> >
> > That would make all of these types of things overrideable at display()
> > time. We could, if it's desirable, make that automatically call a
> > callable, but I'm a bit less certain on that one.
> >
>
> Yep, that could be useful, I've done something similar for the last
> patch on #490 (attributes support), but we should also take care of
> mutable (list/dict) instance attribute.
>
> A shortcut like the one you mentioned could be useful.
>
> >
> > Thanks for writing those tests! That was a big help! Always nice to
> > know that more of the functions are tested.
>
> I'm getting addicted to write tests, it's a nice feeling when they
> pass. :-)
Being test-infected is a good thing :)
Kevin
Max Ischenko
Thanks, it worked.
Another question, unrelated.
I wrote a new thread-safe and new-style compatible version of DataGrid.
I'm not sure how to proceed about it. Should I commit as NewDataGrid or
something? Or wait till newbase will be merged?
Max Ischenko
> Another thing: the widget itself is no longer automatically
> provided to the template namespace.
-1 on this change. I think widget.something is both cleaner (makes it
clear to the template's author where the variable come from) and
simpler (doesn't force developer to write trivial update_data method).
But on the other hand, if it's needed for threadsafety...so be it.
Max Ischenko
Still I think my version is more "correct". ;-)
Kevin, would you mind to review it, I added it as NewDataGrid to
newfastdata/datawidgets.py.
Alberto Valverde
I've tried to test NewDataGrid on my controllers (by adding
list_widget = NewDataGrid() to my subclasses) but I can't seem to
make it work. I get the following exception:
Traceback (most recent call last):
File "./start-n50cpanel.py", line 25, in ?
from n50cpanel.controllers import Root
File "/Users/alberto/src/n50cpanel/n50cpanel/controllers.py", line
61, in ?
class PanelDataController(DataController):
File "/Users/alberto/src/n50cpanel/n50cpanel/controllers.py", line
65, in PanelDataController
list_widget = NewDataGrid()
File "/Users/alberto/src/turbogears/turbogears/widgets/
newbase.py", line 42, in widgetinit
func(self, *args, **kw)
File "/Users/alberto/src/turbogears/turbogears/newfastdata/
datawidgets.py", line 152, in __init__
raise ValueError, "no fields specified and model is null"
ValueError: no fields specified and model is null
Is it backwards compatible with the current DataController code?
It seems to me by reading the code that the NewDataGrid is less
flexible than DataGrid as it forces you to specify the fields (or
model) at construction time, while DataGrid let's you pass a new
SelectResults (from different sqlobject classes) on each render (in a
thread-safe way). It also preserves the original interface (and tries
to confirm deletion ;)
Regards, Alberto
Kevin Dangoor
Actually, because of the instance locking mechanism, threadsafety
probably isn't a valid enough concern.
The other plus that came out of this plan (which is not yet
implemented, but also not hard) is the ability to override everything
at display() time.
Of course, many things simply don't need to be overridden at display()
time. I haven't been certain about this change all along, so I'm
willing to put widget back in the namespace.
Michele Cella
It's not only for theadsafety but also to keep the logic outside of
templates, just compare the new form template with the old one if you
need an example. :-)
Ciao
Michele
Max Ischenko
Yes, it's not backward compatible in a way that it requires an
SQLObject model beforehand instead of reading it from SelectResults.
This is intentional and I think now is a good time to make this change.
I probably should have indicated that explicitly, sorry for confusion.
My non-authoritative opinion that in this case DataGrid flexibility of
adapting to different model classes is actually harmful. Passing the
model explicitly is IMO better and safer. Plus, only a trivial change
required to DataController to init list_widget that way (it has
sql_class available at ctor).
OTOH, may be a good route could be to have a streamlined DataGrid class
that knows nothing about SQLObject's model and/or SelectResults (and
takes a single required ctor arg: fields) and then add, say,
FastDataGrid subclass that can derive fields from model (like my
NewDataGrid version did) or compute them at runtime (like yours version
did).
Max Ischenko
> templates, just compare the new form template with the old one if you
> need an example. :-)
Well, I don't think availability of 'widget' in template namespace is
the reason.
Plus, as history teaches us, assuming a user is less competent then the
author and forcing particular behaviour upon him is no good. ;-)
To summarize:
1. I don't like been forced to write boilerplate code like this:
def update_data(self, d):
d['headers'] = self.headers
d['collist'] = self.collist
d['getcol'] = self.getcol
2. I think using "widget.collist" or "widget.headers" from within
template is clearer then simply "collist" or "headers". No need to
wonder where's particular variable come from. And less cluttering going
on.
Just my opinion.
Max.
Jorge Godoy
> My non-authoritative opinion that in this case DataGrid flexibility of
> adapting to different model classes is actually harmful. Passing the
> model explicitly is IMO better and safer. Plus, only a trivial change
> required to DataController to init list_widget that way (it has
> sql_class available at ctor).
So, if we pass it directly, how can we filter results? Lets suppose I want to
filter data by sex or age or something else. If I pass a SelectResult then I
can filter by any field and using anything supported by SQLObject.
If we pass the model directly and it isn't possible to filter results before
using them on the grid one would have to create several views and put those on
the model...
> OTOH, may be a good route could be to have a streamlined DataGrid class
> that knows nothing about SQLObject's model and/or SelectResults (and
> takes a single required ctor arg: fields) and then add, say,
> FastDataGrid subclass that can derive fields from model (like my
> NewDataGrid version did) or compute them at runtime (like yours version
> did).
I think that subclassing for filtering results is not a good design here,
specially because it is very simple to have these results filtered and in the
form of an iterator.
--
Jorge Godoy <jgo...@gmail.com>
Jorge Godoy
> 1. I don't like been forced to write boilerplate code like this:
> def update_data(self, d):
> d['headers'] = self.headers
> d['collist'] = self.collist
> d['getcol'] = self.getcol
>
> 2. I think using "widget.collist" or "widget.headers" from within
> template is clearer then simply "collist" or "headers". No need to
> wonder where's particular variable come from. And less cluttering going
> on.
I'm not, yet, writing my own widgets -- I may do something like that soon,
though -- but I agree with Max here. It looks more obvious to what is being
used on the template and the update_data method seems unnecessary... But
then, I'm new to all this web stuff and I don't know the repercussions of this
change, though.
--
Jorge Godoy <jgo...@gmail.com>
Alberto Valverde
On Feb 5, 2006, at 1:02 PM, Max Ischenko wrote:
> My non-authoritative opinion that in this case DataGrid flexibility of
> adapting to different model classes is actually harmful. Passing the
> model explicitly is IMO better and safer. Plus, only a trivial change
> required to DataController to init list_widget that way (it has
> sql_class available at ctor).
I don't see why it can be harmful. Computing headers, etc... at
runtime opens the possibility to a non-model-specific grid which can
be used for displaying any kind of SelectResults with the same
controller/grid. Anyway, I haven't got a real use-case for this right
now so I can't back up my opinion any further.
My intention when porting it was to keep the interface and behaviour
intact, so no functionallity I wasn't using (but somene might be,
like non-SO grids) was lost. This way migration wouldn't be so
painful (probably Randall doesn't think alike, though ;)
>
> OTOH, may be a good route could be to have a streamlined DataGrid
> class
> that knows nothing about SQLObject's model and/or SelectResults (and
> takes a single required ctor arg: fields) and then add, say,
> FastDataGrid subclass that can derive fields from model (like my
> NewDataGrid version did) or compute them at runtime (like yours
> version
> did).
I agree with this. I can see that DataGrid as it's currently
impemented could be used also for a non SO grid, which isn't very
specific... I favor a refactoring out of the grid basic stuff into a
separate subclass that could be used for displaying any kind of
"gridded" data structure (like a parsed logfile, for example) and
have a different grid for SO SelectResults.
Anyway, I *believe* the current behaviour (overriding fields at
display time) should be preserved, as I think would keep in line with
the new widgets' capabilities, quoting Kevin previuously in this thread:
"The other plus that came out of this plan (which is not yet
implemented, but also not hard) is the ability to override everything
at display() time. "
Regards,
Alberto
Alberto Valverde
> "Max Ischenko" <isch...@gmail.com> writes:
>
>> 1. I don't like been forced to write boilerplate code like this:
>> def update_data(self, d):
>> d['headers'] = self.headers
>> d['collist'] = self.collist
>> d['getcol'] = self.getcol
>>
>> 2. I think using "widget.collist" or "widget.headers" from within
>> template is clearer then simply "collist" or "headers". No need to
>> wonder where's particular variable come from. And less cluttering
>> going
>> on.
>
I agree that the boiler-plate code is unnecesary and ugly.
But Michele made a good point on this: it encourages coding logic at
the python side (at update_data) and just sending "placeholder-
fillers" and iterables (selectresults, option lists, etc..) to the
template... it's more MVCish.
On Feb 5, 2006, at 1:47 PM, Jorge Godoy wrote:
> I'm not, yet, writing my own widgets -- I may do something like
> that soon,
> though -- but I agree with Max here. It looks more obvious to what
> is being
> used on the template and the update_data method seems
> unnecessary... But
> then, I'm new to all this web stuff and I don't know the
> repercussions of this
> change, though.
>
> --
> Jorge Godoy <jgo...@gmail.com>
The update_data method *IS* needed. Without it we wouldn't be able to
override widget attributes at display time (which I think is
essential) or to pass new variables to the template for each render
(like passing a new css class for changing a textfield's background
color, etc).
It's what makes the new widgets thread-safer too: a new dictionary is
built at each render for the values needed for EACH request. And
gives more control of what actually is being sent to the template
(think of it as a "mini-controller" for a "mini-template" :)
I still think something must be done to reduce the boiler-plate code
though, maybe something like this:
def update_data(self, d):
self.update_data_from_self(d, ['headers', 'colist', 'getcol'])
...
It's still, boilerplate, but shorter...
this idea could even be factored out into the Widget class so it
does it implicitly on every update_data, something like:
class MyWidget(Widget):
update_template_with = ['headers', 'colist', 'getcol']
.....
the code handling this could be something like (at Widget.display():
d = {}
for attr in self.update_template_with: # or
self.__class__.update_template_with
d[attr] = getattr(self, attr, None)
self.update_data(kw)
return view.transform(kw, template=self.template)
This last approach would reduce the boilerplate while preserving the
all-goodness ;) of update_data.
If full qualification is preferred (a là widget.name) it shouldn't be
too difficult to implement either.
What I'm positive about is that the widget instance should NOT be
passed to the template, just to emphasize that the template variables
are updated at every render (plus all the, now diminished, thread-
safety issues).
Regards, Alberto
Jorge Godoy
> The update_data method *IS* needed. Without it we wouldn't be able to
> override widget attributes at display time (which I think is essential) or to
> pass new variables to the template for each render (like passing a new css
> class for changing a textfield's background color, etc).
>
> It's what makes the new widgets thread-safer too: a new dictionary is built
> at each render for the values needed for EACH request. And gives more control
> of what actually is being sent to the template (think of it as a
> "mini-controller" for a "mini-template" :)
This is exactly what I needed to know ;-) Thanks for your explanation.
> I still think something must be done to reduce the boiler-plate code though,
Reduce: yes! But please make it simple and legible as well. I'd rather have
10 lines of code that are easy to maintain than having to decypher 3 or 4
lines. It will also make it clear for newcomers.
> What I'm positive about is that the widget instance should NOT be passed to
> the template, just to emphasize that the template variables are updated at
> every render (plus all the, now diminished, thread- safety issues).
Now that I understand it, this sounds like it is the better, indeed. And
would also avoid having people - like me ;-) - hurting themselves. :-)
--
Jorge Godoy <jgo...@gmail.com>
Alberto Valverde
On Feb 5, 2006, at 2:58 PM, Alberto Valverde wrote:
> the code handling this could be something like (at Widget.display():
>
> d = {}
> for attr in self.update_template_with: # or
> self.__class__.update_template_with
> d[attr] = getattr(self, attr, None)
> self.update_data(kw)
> return view.transform(kw, template=self.template)
should be:
> the code handling this could be something like (at Widget.display():
>
> d = {}
> for attr in self.update_template_with: # or
> self.__class__.update_template_with
> d[attr] = getattr(self, attr, None)
d.update(kw)
> self.update_data(d)
> return view.transform(d, template=self.template)
>
Alberto Valverde
avoid it:
http://trac.turbogears.org/turbogears/attachment/ticket/490/
newbase_widget_vars.2.patch
The discussion is at:
http://trac.turbogears.org/turbogears/ticket/490
Hope i helps,
Alberto.
Kevin Dangoor
>
> > It's not only for theadsafety but also to keep the logic outside of
> > templates, just compare the new form template with the old one if you
> > need an example. :-)
>
> Well, I don't think availability of 'widget' in template namespace is
> the reason.
>
> Plus, as history teaches us, assuming a user is less competent then the
> author and forcing particular behaviour upon him is no good. ;-)
I agree with this, in general. The argument could be made that the new
"locking" behavior of widgets that prevents you from setting self.foo
after you've started using a widget could be viewed as forcing
behavior, but I view it more as preventing a bug.
The availability of "widget" in the template namespace is different
because that's getting more into good design vs. bad design, which is
a more "slushy" (soft) topic.
From the standpoint of widget authoring, just dropping widget into the
namespace is appealing: it's the least amount of work possible.
However, from the standpoint of widget *users*, there is an advantage
to explicitly listing out the variables from the widget that should be
available: *all* variables that the widget uses and defines in the
constructor can also be overridden at display() time. For
consistency's sake, it's nice to think that people wouldn't have to
play a guessing game about whether they can override something.
>
> To summarize:
>
> 1. I don't like been forced to write boilerplate code like this:
> def update_data(self, d):
> d['headers'] = self.headers
> d['collist'] = self.collist
> d['getcol'] = self.getcol
I'm proposing something similar to what was suggested elsewhere in the
thread: a class variable that lists the attributes that should be
copied from the widget to the template.
template_vars = ["headers", "collist", "getcol"]
It's a little more typing than just having widget available, but it
provides the ability to override. There would be a method that copies
those over to the dict that is called from display()... so nothing
else is required if you don't override display().
> 2. I think using "widget.collist" or "widget.headers" from within
> template is clearer then simply "collist" or "headers". No need to
> wonder where's particular variable come from. And less cluttering going
> on.
Granted with just "collist" you don't know if the value came from the
widget, or from the template which called display(). But, there should
only be a couple places that people need to check to figure out where
the value they're seeing came from, and the override-per-request
capability from display() overrides is important in a number of cases.
Kevin
Kevin Dangoor
>
> I've posted a patch for the new widgets API which you can use to
> avoid it:
>
> http://trac.turbogears.org/turbogears/attachment/ticket/490/
> newbase_widget_vars.2.patch
Err... that's interesting. In the email I just wrote, I suggested
"template_vars" and that's exactly what you named it!
Kevin
Alberto Valverde
Scary.... :/
Mike Orr
> From the standpoint of widget authoring, just dropping widget into the
> namespace is appealing: it's the least amount of work possible.
> However, from the standpoint of widget *users*, there is an advantage
> to explicitly listing out the variables from the widget that should be
> available: *all* variables that the widget uses and defines in the
> constructor can also be overridden at display() time. For
> consistency's sake, it's nice to think that people wouldn't have to
> play a guessing game about whether they can override something.
This is how I know TurboGears is in good hands. The widget author
only has to write the code once. The widget user has to use the API
again and again in every site. The more concise the better, provided
it doesn't get cryptic.
I don't understand the threading issues so I'm staying out of that.
Reusing a widget instance has just never been an issue in Quixote. A
widget instance is specific to a particular widget on the screen in
one transaction. If two identical forms are on the page, they'd use
different form instances with different widget instances. It's not
smart enough to choose the correct form from the input, but you can do
that manually by examining a hidden field or submit value and then
telling the code which form object to use. Quixote is multiprocess
rather than multithreaded so threading issues don't come up. But I
still don't see why you'd want to share a widget instance between
threads, which would imply it's being used on multiple forms, and thus
must be thread safe. In Cheetah we purposely made Template instances
not threadsafe because it's much more convenient to store state in the
instance. And the overhead of instantiating extra instances is so
small it doesn't matter.
--
Mike Orr <slugg...@gmail.com>
(m...@oz.net address is semi-reliable)
Alberto Valverde
On Feb 6, 2006, at 2:49 AM, Mike Orr wrote:
> In Cheetah we purposely made Template instances
> not threadsafe because it's much more convenient to store state in the
> instance. And the overhead of instantiating extra instances is so
> small it doesn't matter.
>
> --
> Mike Orr <slugg...@gmail.com>
> (m...@oz.net address is semi-reliable)
I think one of the main reasons to instantiate widgets only once is
compiled template reusal. With the current API (withe the formrer
too), templates are compiled just once.
Alberto
Kevin Dangoor
> I don't understand the threading issues so I'm staying out of that.
> Reusing a widget instance has just never been an issue in Quixote. A
> widget instance is specific to a particular widget on the screen in
> one transaction. If two identical forms are on the page, they'd use
> different form instances with different widget instances. It's not
> smart enough to choose the correct form from the input, but you can do
> that manually by examining a hidden field or submit value and then
> telling the code which form object to use. Quixote is multiprocess
> rather than multithreaded so threading issues don't come up. But I
> still don't see why you'd want to share a widget instance between
> threads, which would imply it's being used on multiple forms, and thus
> must be thread safe. In Cheetah we purposely made Template instances
> not threadsafe because it's much more convenient to store state in the
> instance. And the overhead of instantiating extra instances is so
> small it doesn't matter.
It ultimately wasn't an overhead issue. It was more to do with how the
widgets are used.
@turbogears.expose()
def foo(self):
return dict(someform = myform)
@turbogears.expose()
@turbogears.validate(form=myform)
def save(self, blah, blah2, blah3):
pass
That same form instance is used at class initialization time in two
different methods (at least in this style of forms usage). There are
various ways that the widgets could potentially be instantiated that
don't have threadsafety issues, but they were less pleasant to work
with. The revised widget API makes it more natural to write a widget
in a threadsafe manner (in addition to raising an exception if the
user attempts use that is not threadsafe), so I think we end up with a
pleasing API and no real threadsafety worries.
Saving state in a controller isn't threadsafe, either, but i think
that is a less-natural thing for people to try to do.
Kevin
Mike Orr
> It ultimately wasn't an overhead issue. It was more to do with how the
> widgets are used.
>
> @turbogears.expose()
> def foo(self):
> return dict(someform = myform)
>
> @turbogears.expose()
> @turbogears.validate(form=myform)
> def save(self, blah, blah2, blah3):
> pass
>
> That same form instance is used at class initialization time in two
> different methods (at least in this style of forms usage).
OK, the difference is that in Quixote the form is instantiated inside
the method. I use a factory function for each form.
What if you passed the form class or a factory function instead of the
instance? Then each thread would have its own instance even though
it's a decorator wrapper, right? What would be lost in terms of TG
programming elegance?
Kevin Dangoor
My example was probably not full enough. The form itself is not the
problem... the problem was more to do with the widgets on the form:
class Foo(WidgetDeclaration):
name = TextField(size=10)
age = TextField(validator=Int())
myform = TableForm(widgets=Foo)
The need for a factory function extends to the widgets themselves.
What *could* be done here is to change from using a widget constructor
directly to currying (I think that's the term I want) the constructor
call and actually doing the instantiation at request time. That may
have been a bit less clear when we were also struggling with the
declarative format for widgets on a form.
It's likely possible to keep the widget user API the same or very
similar, but I'm not sure if there are truly benefits for the widget
creator. Setting whatever you need access to in update_data is easy,
and the safeguard is there for threadsafety.
Max Ischenko
I appreciate your time and efforts on educating me (and others) on this
issue.
I think I finally "get" your arguments but please also check out a
separate thread regarding DataGrid refactoring.
Mike Orr
I'll have an opportunity to test the form API in the next two weeks,
so I'll see how it goes. Yes, the structure is very different if you
have widget instances in the form class.
Not sure where you see currying. Currying is like a bound method but
more generalized. The leftmost args are fixed, so you only supply the
rightmost args on each call. My decorator-writing ability sucks but
it's something like:
def curried(func, *bound_args):
def curried_func(*args, **kw):
complete_args = bound_args + args
return func(complete_args, **kw)
return curried_func
@curried('arg1', 'arg2')
def my_func(arg1, arg2, arg3, arg4):
return "my_func was called with", `(arg1, arg2, arg3, arg4)`
print my_func('arg3', 'arg4')
Kevin Dangoor
> I'll have an opportunity to test the form API in the next two weeks,
> so I'll see how it goes. Yes, the structure is very different if you
> have widget instances in the form class.
>
> Not sure where you see currying. Currying is like a bound method but
> more generalized. The leftmost args are fixed, so you only supply the
> rightmost args on each call. My decorator-writing ability sucks but
> it's something like:
What I meant was that the constructor call would be curried (the
parameters kept track of in a new callable that would return the
instance as needed).
I'd definitely be interested in hearing your opinions when you give
the API a whirl.
Kevin
Tavis Rudd
> must be thread safe. In Cheetah we purposely made Template instances
> not threadsafe because it's much more convenient to store state in the
> instance. And the overhead of instantiating extra instances is so
> small it doesn't matter.
This is misleading. I've been sharing Cheetah template instances
between threads for years. You just need to turn off the searchList
and pass in anything threadlocal as arguments to the template methods
you define. Thread safety is a usage issue, not something inherent in
Cheetah template instances.