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 #577 (closed defect: fixed)

Opened 13 years ago

Last modified 13 years ago

[PROPOSAL] normalize nested widget structures automatically

Reported by: alberto Owned by: anonymous
Priority: low Milestone: 0.9
Component: Installation Version: 0.9a4
Severity: critical Keywords: <Default>
Cc: <Default>

Description

I've been working much lately with forms and widgets and validators and have noticed some inconsistent behaviour:

having a form declared like this:

class MyForm(TableForm):
    fields = [
        FieldSet("p_data", legend = "Personal Data", fields = [
            TextField("name"),
            TextField("age"),
        ],
        FieldSet("o_data", legend = "Other Data", fields = [
            TextField("whatever"),
            TextField("anything"),
        ],
    ]

When it is submitted, the input dict looks like this:

input_data = {'name':'alberto', 'age':'25', 'whatever':'you want', 'anything':'at all'}

So far so good... the problem is that when you want to prepopulate the form using this input as output, it doesn't work as expected, values are not prepopulated. It works if you do:

value = dict(
    p_data = dict(name="peter", age="99"),
    o_data = dict(whatever="nothing", anything="the same"),
)
$form.display(value=value)

I think we should normalize this so nested widgets return nested dictoinaries and viceversa, both for input and for output. It makes more sense to me and offers the possibility of building more complex forms, manage related joins, etc..

I think it shouldn't be too difficult to implement: All that is needed is to make a formencode.schema.Schema subclass with a  NestedVariables pre_validator and make this the default validator for CompoundWidgets. *Maybe* a little tweaking is also needed with the code that prepopulates the form... but I haven't tested anything yet

One drawback I can think of is that it will break things for people who depend on the current behaviour which, hopefully, won't be too many...

Opinions anyone? I'll give it a wirl sometimes this afternoon if i get a green (or orange ;) light. Regards, Alberto

Attachments

nested.patch Download (4.7 KB) - added by alberto 13 years ago.
poc
partial-solution.patch Download (4.5 KB) - added by michele 13 years ago.
partial.patch Download (8.5 KB) - added by anonymous 13 years ago.
nested.2.patch Download (12.3 KB) - added by alberto 13 years ago.
nested.3.patch Download (12.4 KB) - added by alberto 13 years ago.
nested.4.patch Download (13.6 KB) - added by anonymous 13 years ago.
nested.5.patch Download (20.2 KB) - added by alberto 13 years ago.
almost there
nested.6.patch Download (24.3 KB) - added by anonymous 13 years ago.
michele-nested.patch Download (8.5 KB) - added by michele 13 years ago.
my second attempt
michele-nested2.patch Download (10.2 KB) - added by michele 13 years ago.
small cleanup
michele-nested-w-tests.patch Download (15.1 KB) - added by anonymous 13 years ago.
michele-nested-w-tests.2.patch Download (15.2 KB) - added by anonymous 13 years ago.
the previous patch + a little modification at adjust_value which made file uploads barf (checking "value is None" instead of "not value")
michele-nested-w-tests-plus-schema.patch Download (16.0 KB) - added by anonymous 13 years ago.
michele-nested3_tests_and_field_id.patch Download (13.6 KB) - added by michele 13 years ago.
New patch with field_id management
michele-nested-w-tests-plus-schema-plus-id.patch Download (23.6 KB) - added by alberto 13 years ago.
michele's last patch with tests for the id stuff too
michele-nested-w-tests-plus-schema-plus-id.2.patch Download (18.9 KB) - added by alberto 13 years ago.
Yikes! Subversion does weird stuff when repatching uncommitted files... :/ fixed test_nested_widgets
combined-patch-with-controller-test.patch Download (20.9 KB) - added by alberto 13 years ago.
michele-nested3k.patch Download (25.0 KB) - added by michele 13 years ago.
Cool!
final_nested.patch Download (22.6 KB) - added by alberto 13 years ago.
cleanead up, 2.3ized nested-widgets-mega-patch :)
final_nested.2.patch Download (23.8 KB) - added by anonymous 13 years ago.
fixed the automplete's instance name
michele-nested-yet-another.patch Download (26.5 KB) - added by michele 13 years ago.
py2.3, uses "_" instead of "." for ids, some fixes here and there
getting-there-with-the-filter.patch Download (27.0 KB) - added by michele 13 years ago.
The name says it all
getting-there-with-the-filter.2.patch Download (27.3 KB) - added by alberto 13 years ago.

Change History

comment:1 Changed 13 years ago by michele

Mmm, that's an hot topic. :)

The reason we get this is because FieldSet? is not an input element for HTML but just a layout element so if you give it a name it doesn't matter and what you get is just the flat dict of input_values.

Anyway it's true that this can be inconsistent regarding the controller side, and it could be nice to receive a dict of its input values and just using the name in the method, so for your example:

def save(self, p_data, o_data)

where p_data and o_data are dict.

instead of:

def save(self, name, age, whatever, anything)

I actually like this, it seems a pretty useful behavior.

I've taken a look at the nested dict validator back when I've redone the API, I don't like the way it manages names, anyway it works for now and we can rewrite a different one.

For example for javascript it could be useful to use names like this:

p_data[name]
p_data[age]

to get:

p_data = {'name':value, 'age':value}

instead of:

p_data-age
p_data-name

that's what you get with Nested from formencode.

If you can prepare a patch it would be nice to see this in action before doing any further comment.

a useful link:  http://groups.google.com/group/turbogears/browse_frm/thread/9a77dfe1aa2c6dad/f812ab4c384ffe0b?q=nested+karl&rnum=3#f812ab4c384ffe0b

comment:2 Changed 13 years ago by anonymous

Mmm, I haven't tried the NestedVariables? really so I might be wrong... but IIRC formencode's docs, dictionaries where separated by dots, that is:

a[b] => a.b a[b][c] = a.b.c

dashes were used to encode lists.

Anyway, I think it's probably better to try and patch NestedVariables? into turbogears.validators to use brackets, undersocres, etc... than reinventing formencode's wheel (and bugs).

Well, nuff talk :D, let's see the patches! I'll try to post back later...

Alberto

Changed 13 years ago by alberto

poc

comment:3 Changed 13 years ago by alberto

  • Type changed from defect to enhancement

Hi Michele,

This is what i've got, minimally tested and probably broken... but it's a start. Now I have to go but i'll have more time to discuss it later this afternoon Regards, Alberto

comment:4 Changed 13 years ago by michele

Hi Alberto,

Unfortunately I won't have time to look into this more deeply until Saturday I think. :-(

Anyway just some things:

  • Do we really need the parent parameter?
  • I think we should use only python structures on the controller side, this means that instead of using 'p_data.name' I prefer (even if more verbose) to use a nested dict 'p.data': {'name'}, that's to do thing symmetrically, if we provide a way to receive input values from a compound widget like the FieldSet? in the way I described above (that's also how it works with the Nested validator) we receive in input a dict like the one I described, why should we prepare values in a ad-hoc way that's not python related?

If you can wait until saturday I hope to clarify my points a little more, anyway my first reply it's how I envision this thing.

comment:5 Changed 13 years ago by alberto

Hi Michele,

I've been thinking about this and it's not as easy as I first thought :/ The parent parameter was a quick hack to build the widget's "path", something is needed in this direction, as we need to build a path to each widget to put it into the name parameter so the variable names are named in a way NestedVariables? (or whatever comes up) understands. Something has to be done with the way adjust_values works and how the validation errors are saved (we need a nested dict here too).

I'm going to look closer at this tomorrow, if you're working on it too make sure you pop up here and we can have a sort of "online sprint" ;)

bye, Alberto

comment:6 Changed 13 years ago by michele

Hi Alberto,

Now I will attach a small patch that uses your parent method to to the things, it's working almost right the only lacking thing is the construction of a nested error dict (I've made a small attempt at it) to collect errors.

Note that the parent thing is now a list this allows us to keep track of multiple parent in a easy way, for example I'm testing this stuffs against a form with a fieldset that embeds another fieldset and validation is working right (returning a nested dict as we want) and also input_values are conserved and reused in the right way for form redisplay (by modifying adjust_value).

As I said I think the only thing left is preparing the error dict (and also updating field_id according to the parent name) in the right way, I haven't put much tought on it but it's only a matter of working on validate for CompoundWidget?, I'm posting this here even if not complete so that you can take a closer look it's really late here.

Sorry no tests, just use a form like the one I mentioned. ;-)

Time to sleep.

Changed 13 years ago by michele

comment:7 Changed 13 years ago by michele

I have an idea for the errors problem, going to post a new patch soon.

comment:8 Changed 13 years ago by alberto

Wow! man, you should get some sleep sometime... ;)

I'm going to get a closer look at your patch tought at first sight it looks pretty good. great work! If you need any help just ask. BTW, I forgot to mention it yesterday (couldn't imagine you were going to be that fast :), have you taken a look at Ian's  repeating forms? I was thinking of creating a new widget, pehaps a RepeatingForm? or something which subclassed CompundWidget?, that could take a count parameter and repeat it's content that many times, should do it in the field-1.name, field-2.name, format that NestedVariables? (or any markup decided later) accepts and probably throw in all the js neccesary to add/remove subforms. This would be GREAT, IMO, for inline forms like Django does. As you've already done almost all that needs to be done for the nested dict stuff I'm going to take a chance on this later this afternoon.

Great work and thanks! :) Alberto

comment:9 Changed 13 years ago by michele

You posted the reply before me so I've got an mind air collision by trac. :-)

Anyway I haven't got a chance to play with the errors thing as I said, it seems a bit tricky to get right, if you want to take a look at it no problem as this afternoon I'm away (football game ;)), I'm not so sure we can do this using only parent at display time I'm starting to think we need a parent attribute on every widget that's automatically updated in the right way by every compound widget after it's initialization by calling a method that works pretty much like the retrieve_jss one and iterates over all widgets by propagating it's parent and it's name (if it's a compound) to children, I we go this way it should be totally transparent to the FieldSet? and will not even require parent at display and passing it to adjust_value and in other places.

Mmm, I don't know django inline forms, anyway I think that if we get this done we could probably push it for the 0.9 alpha release (22/02) as it's a nice way of working, regarding RepeatingForms? it could probably be better to push them after the alpha release, ask Kevin anyway. ;)

Ciao.

comment:10 Changed 13 years ago by alberto

Yeah, it's tricky... and setting a parent attribute is very tricky (was my first attempt at this) because a wiget instance can have multiple parents, example:

class Fields(WD):
    name = TextField()
    age = TextField()

form = TableForm(fields=Fields() + FieldSet(fields = Fields())

as name and age are initialized on Fields declaration, both the form's Fields() and the FieldSet?'s fields share references to the same TextFields? :(

So we must decide if the previous code is acceptable usage or do as it's done now and build the "path" dynamically :/

I'll look into it later...

Have a nice time in your Calcio match and thanks again ;) Alberto

comment:11 Changed 13 years ago by michele

Mmmm, you are right about this.

You can try to elaborate on my patch that uses parent at display time to see if you can come up with the nested error dict, that's probably the best solution at this point.

Thanks, and yes it's a calcio match not a football game. ;-)

comment:12 Changed 13 years ago by alberto

Almost there! :)

Might be unoptimal, might be broken, but looks almost there :)

my table looks like this:

form = widgets.TableForm(fields=[
    widgets.TextField("name"),
    widgets.TextField("age", validator=validators.Int()),
    widgets.FieldSet("sub", fields = [
        widgets.TextField("name"),
        widgets.TextField("age", validator=validators.Int()),
        widgets.FieldSet("sub2", fields = [
            widgets.TextField("name"),
            widgets.TextField("age", validator=validators.Int()),
        ]),
    ]),
])

and it seems to be working! (no tests though, gonna work on that after lunch). Errors are not displayed correctly still but the debug prints seem to give correct results. take a look if you have a chance...

chao (spanish version for ciao ;), alberto

Changed 13 years ago by anonymous

comment:13 Changed 13 years ago by alberto

Hi Michele,

The error dict is built correctly now... (I hope ;) Some tests to prove it. All that is left is to fill in the form with errors correctlly. Probably the whole algorithm needs to be polished aswell as It might be redundant in some places (specially the recursive_update stuff). It's tricky though...

regards, Alberto

Changed 13 years ago by alberto

comment:14 Changed 13 years ago by alberto

I've polished the patch a little so I could understand what was going on ;)

Changed 13 years ago by alberto

comment:15 Changed 13 years ago by alberto

the patch is terribly broken... :( don't even bother... I have an idea on how to fix it and working on it.

We *really* should use fully qualified names for widgets, that is form.fieldset.field, even from a security POV as it would be very easy to trick the validator into not validating some stuff (in _validate_children, if not values.has_key(self,name) then validation should stop, else the dicts get messed up). After validation we can always adjust it so all the root's childs become roots

comment:16 Changed 13 years ago by alberto

Mmmm, This is almost impossible to get right... I've spotted another problem: When a compound widget renders it's subwidgets, for example TableForm?, it tries to print it's label for field.field_id. The problem is that field.field_id could be different to the field's name (which would be a bug in my POV as then we could have DOM elements with the same id (id on different levels of the tree). That means, a widget should have knowledge of who is it's parent.

I think the best solution is to use a parent attribute in the field as you suggested. It will be the easiest to implement, the least hacky and no need to do kungfu at display time (and validation time)

The downside is that we should prevent widgets from changing their parent... It's probably going to break exsting stuff but I do believe it's the *right* thing to do (if we want to properly handle nested widgets, which I personally *really* do).

Kevin should have a say on this so I'm pinging him see what he thinks.

Alberto

comment:17 Changed 13 years ago by michele

Hi Alberto,

yep nested widgets are really tricky, I my mind I have a solution for the error thing now (we can pass to validate the parent like we do for display by constructing it during the validation chain) that should work I think, I will try to make a patch tomorrow maybe we can use it for the moment.

Anyway the parent attribute seems the best solution, but it will suffer from the problem you described that could lead to some bad behaviors.

Let's see Kevin opinion and how it turns out the thing I mentioned.

comment:18 Changed 13 years ago by alberto

Hi Michele,

Take a look at nested.3.patch. It's exactly what I'm doing, passing parent to validate and all the validation chain. It almost works (the tests are passing but they're broken... it's *very* tricky, for example, if the input nested dict is missing subdicts).

I think all this stuff of passing the parent to display and validate is a "highway to hell" ;) and *ugly*. The parent attribute will make things much simpler and we can always code it as a property that barks when used improperly. I really think that "a widget shall only have one parent" ;)

I'm working on the parent attribute fo the moment taking ideas from your patch to see how it goes, I'll post a patch in a couple of hours see what u think.

comment:19 Changed 13 years ago by michele

Well, indeed without the parent attribute things are turning out to be *horrible* like you said. :D

I think you are right regarding the fact that a widget should have just a parent, I will take a look at your patch tomorrow morning as now I'm going away.

One important thing to note is that the parent management is a bit tricky and should act in a reverse way by iterating over all children widgets starting from the form, otherwise the first compound widget will pass a wrong parent to its widgets because it doesn't yet know its parent.

Once the parent list attribute works in the right way things will get really simpler and cleaner. ;)

Thanks Alberto.

Ciao

comment:20 Changed 13 years ago by anonymous

Hi Michele,

I've finally relialized that I was running in the wrong direction...

The parent can, and should be passed at display/validation time... that way no ugly things can happen later. I was stubborn to take a recursive approach and it just can't work the way I thought it. Your trick for finding the "route" to the correct value at adjust_values is the key: that is, letting each widget find it's way on the error dict by traversing the dict from the root following their path., no recursion neccesary.

I've tried a "parent" attribute approach and I just hit the same wall as before but from another angle, it can be avoided I :)

I'll give it another try tomorrow where you left it at the partial-solution patch (though I might find it implemented before I get up ;). I think I finally see it clear now...

Alberto

comment:21 Changed 13 years ago by kevin

Hi guys,

This seems like a very interesting discussion, but at the moment my brain doesn't seem to be awake enough to parse it properly. I'll need to take a look at the patch and the tests and think about the use cases that this is trying to cover. I might not get a chance to chime in on this until Monday.

comment:22 Changed 13 years ago by alberto

Hi,

The following patch works (as far as unittests can tell). No parent attribute used, just a parent parameter to validate() and display().

However, it's backwards incompatible because it *needs* to have the names of the widget's fully qualified like: form.fieldset.name I cannot manage to get it working without it, specially if the values are not submitted from a real form as the thing would go crazy with missing values. Many of the other widget tests fail for ths reason...

I think the form validator can be fixed to make the values dict backwards compatible, so it doesn't break current controllers. though another tweak should be made to the code that prepopulates the form to take it into account (now it expects a full path).

It's too late here and I need some sleep, take a look see what you think. I can polish it tomorrow and get the id's and the rest of the tests updated... and discuss this further :)

Alberto

P.S got a mind air collition and missed kevin's post while writting this :/ ll try to describe the use cases briefly:

One future use case would be to make this parse Ian's repeating forms or something similar (there's a link somewhere in this ticket). Now it can handle "inline forms" for "related items" (sorry, my head is not up right now with better vocabulary, 5am here ;)

Changed 13 years ago by anonymous

comment:23 Changed 13 years ago by michele

Great work, but you should really sleep! :D

Regarding the fully qualified thing, can you explain it better? in what way this is backward incompatible?

Anyway this can have a nice side effect and I really like it in general, in the first version of my widgets I was doing something very similar but for ids using something like "comment_text" where comment is the form name and text the field name, now if we decide to use fully qualified names we should also use them for ids by the way (this will also avoid id collisions for sure).

I think once you sleep a bit more we can find a better solution for the recursive update function.

Great work Alberto.

comment:24 Changed 13 years ago by alberto

Hi,

Backwards incompatible? Did I really say that? :D What I meant is that the widgets are pre-pended with their container's path, not only FieldSets? but forms too. So this might break something somewhere (specially CSS stylesheets when the field id gets populated properly), nothing too serious though.

I've fixed a couple of things, like the Schema validation, and adjusted all the widget tests to the new style of input values (this should be totally transparent to the user, it just makes a difference with tests). I've also removed the values' tree root so all it's siblings become new roots, sort of like taking cuttings from a plant :D this way, the input values to the controller will be the same format as before if no nested widgets are involved.

that is:

    form = widgets.TableForm(name = "myform", fields=[                          
        widgets.TextField("name"),                                              
        widgets.TextField("age", validator=validators.Int(if_empty=None)),      
        widgets.FieldSet("sub", fields = [                                      
            widgets.TextField("name"),                                          
            widgets.TextField("age", validator=validators.Int(if_empty=None)),  
            widgets.FieldSet("sub2", fields = [                                 
                widgets.TextField("name"),                                      
                widgets.TextField("age",                                        
                    validator=validators.Int(if_empty=None)),                   
            ]),                                                                 
        ]),                                                                     
    ]) 

Will produce:

def controllermethod(self, **kw):
    print kw

{'age':20, 'name':'peter', 'sub':{'name':'susan', 'age':29, 'sub2':{'name':'kathy','age':25} } }

What still remains to be done is to correctly generate the field ids and preparing changing Form's update_data method to get the errors from the nested dict.

I'll make that change tomorrow morning at work (if I't doesn't get done before ;)

Regards Alberto

P.S. I would appreciate some testing of this patch on "real" apps just in case I'm missing something... Tests are passing fine (after some tweaking to take into account the changes). Any comments or ways to improve this are always welcomed. :) Thanks!

Changed 13 years ago by alberto

almost there

comment:25 Changed 13 years ago by alberto

Another patch polishing things a bit and fixing some things after running all tests. I've modified test_form_controllers to send the GET params in the new notation (ie: form.name intead of just name) but I've noticed something weird:

def controllermethod(self, name, age):
   self.name = name
   self.age = age

does not work, but

def controllermethod(self, **kw):
    self.name = kw['name']
    self.age = kw['age']

Does.

Fixed the test so it works though this change should *not* go in but looked closer at and fixed.

I have a feeling that the way Form's validate() rebinds kw args will only work if the controller does not use keyword or positional parameters and relies solely on the extra keywords dict... :( Maybe a hack can be done at validate with some dark introspective magic to rebind the applicable args.... but I'm not sure of all the consequences this might have in the decorator stack (if any) or how to best implement it.

Alberto

Changed 13 years ago by anonymous

comment:26 Changed 13 years ago by anonymous

Please ignore that comment at the form controller's test regarding to_kw as it was a first hypothesis but found out it has nothing to do.

comment:27 Changed 13 years ago by michele

Hi Alberto,

I have taken a quick look, some things to note:

  • the kw problem is related to this ticket? #554
  • we can move the empty subdict removal to the form after validation so it's done at validation end and only once
  • do we really need all this logic inside the validate method of a form? or this can be abstracted to Compound?

I really hope to get a chance to take a closer look ASAP but I can't promise it. :-(

Thanks again for the great work.

comment:28 Changed 13 years ago by michele

Ok, I couldn't sleep until now so I tried to work on this and see if things could be made in an easier way and... it seems so.

I'm going to attach the patch that shows what I've come up with.

Some things to note:

  • I haven't used fully qualified names as things seems to work nice even without them, anyway they can easily be added
  • No need to check for empty dicts in validation_errors, they are avoided before doing any computation
  • Little modifications to FieldSet? and Forms as in my first patch
  • The error dict is constructed in the right way this time
  • Schema validation should be fixed cause I'm a little slow now and I haven't done it so I don't want to break it (your work Alberto)
  • No tests ATM, but FieldSet? shows errors in the right way, anyway we may want to change the new functions name to be more generalized

Well, I must admit we have made a little duplication effort but that way we can check different solutions to find the right and easier one, and be sure we cover every problem. ;)

I'm particularly interested to hear your opinion on what I've done, I'm sure (but I hope that's not the case :D) that I probably missed some problems you have found during your work.

Ciao!

Changed 13 years ago by michele

my second attempt

comment:29 Changed 13 years ago by michele

Mmm, I also think your problem with the kw thing could be related to how you modified the form validate method, I'm not experiencing it here.

One last thing to add, tests are all passing apart from Schema validation for Compound widgets and a failure related to the widget name I think IIRC.

Buona notte.

comment:30 Changed 13 years ago by michele

Regarding fully qualified names, if possible after thinking a bit I would like to avoid using them.

Some of the reasons:

  • It could be a little unintuitive to have names like form.name but receiving on your controllers parameters like name without the form thing
  • It would complicate the field_for and error_for methods I added to forms
  • Other reasons that ATM don't come to my mind... :D

Changed 13 years ago by michele

small cleanup

comment:31 Changed 13 years ago by michele

Regarding the kw problem, we can't use ATM widgets names when we have nested structures or we get an argument error, we may want to ask Simon about this.

comment:32 Changed 13 years ago by alberto

Hi Michele,

Nice work, and seems much simpler too... great! I'm going to adapt the tests I wrote to the naming schmema you use and give it a try see what happens...

Thie FQN was something I did to simplify the algorithm, don't remember exactly why but had somthing to do with reduicing the number of special cases. If it can be avoided, better avoid it as to keep the interface as close to the original as possible.

Well, haven't really tried you patch so I cannot really comment it, I'm giving it a try right now and post back.

Alberto

comment:33 Changed 13 years ago by anonymous

Wow! The nested tests are all passing perfectly! No I can really say... great work! :D

I'm going to see what can be done with the schema validation...

I'm attaching you last patch with my tests so you can work with them too...

Changed 13 years ago by anonymous

Changed 13 years ago by anonymous

the previous patch + a little modification at adjust_value which made file uploads barf (checking "value is None" instead of "not value")

comment:34 Changed 13 years ago by alberto

Another patch with the widget tests tewaked.... Fixed the schema validation. Every test is passing OK. One question, shouldn't we leave "name" as a template_var for Widget?

Im really amazed... :)

Changed 13 years ago by anonymous

comment:35 Changed 13 years ago by alberto

Man, you're patch looks perfect from my POV:

  • The kw problem doesn't seem to be occuring anymore (test_form_controllers proves it).
  • Much more efficient than my walking and rewalking of the whole error_dict tree.
  • errors are displayed right beside their appropiate fields
  • All tests are passing
  • Schema validation is working

The only thing is the field_id issue. How shall it be fixed? Maybe the parent parameter sent to display could be a tuple (name, id). My thinking behind this is that the id might be overriden at display and therefore be != to name. I think that when this is fixed I can commit all this changes see how it goes... I'll need to post something at the ML explaining the new behaviour for people using fieldsets, etc...

BTW, the new semantics for managing forms is veeeery neat :)

I'm going to think about how to implement the repeating forms for a while see if I come up with something.

Thanks! Alberto

comment:36 Changed 13 years ago by kevin

Now that I'm slightly more awake... I can understand the repeating forms use case, though there are likely to be other ways to implement that. I'm not completely sure I understand the nesting of related items use case - is the goal to keep them separate from a namespace perspective (avoid name collisions), or is it to make it easier to populate with different objects, or something else entirely? Sorry if I'm being dim, but I'm just trying to see where this would be applied in a real form.

comment:37 Changed 13 years ago by michele

Hi Alberto,

thanks for all for adding back your tests, regarding the field_id problem, my idea is to just move it from being an attribute to be a function that you can call with the parent parameter, that's the only thing that comes to mind ATM and the only way to make it work.

I'm going to post an updated patch for this in just two minutes. :)

Ah, regarding adding name to template_vars I don't think so since we are constructive our dname? taking into account the parent attribute, using template_vars to just replace it seems a computation waste.

Kevin: This is needed to make forms managaments more consistent, if we are declaring a form in this way:

class FormFields(...):
   name = TextField()
   comment = TextArea()
   other = FieldSet()

I would expect to manage it by using a method like this:

def save(self, name, comment, other):
   ...

where other is a dictionary.

ATM you will need to list also every name used by other but to populate it you will need a nested dict as Alberto said on his first post.

def save(self, name, comment, hello, bye, hi):
   ...

where hello, bye, hi are the FieldSet? fields, this means giving CompoundWidgets a more helpful role where ATM they simply are layout widgets. For example this will help to manage big forms like this one used by Trac that contains many fieldset by just receiving, name, comment, properties, action.

Changed 13 years ago by michele

New patch with field_id management

comment:38 Changed 13 years ago by michele

  • Cc simon.belak@… added

The only problem I'm experiencing with this patch is that I can't use the nice form we want to use. :D

I think this needs a trivial fix here or somewhere in controllers.py.

Problem is that, if I have this form:

class OtherFields(widgets.WidgetsDeclaration):
    hello = widgets.TextField(validator=validators.Int())
    bye = widgets.TextField()

class CommentFields(widgets.WidgetsDeclaration):
    name = widgets.TextField()
    email = widgets.TextField(validator=validators.Int(), attrs={'size':30})
    many = widgets.FieldSet(fields=OtherFields())

my save methods should be:

    @expose()
    @validate(form=comment_form)
    @error_handler(add)
    def save(self, **kw):
        raise redirect("index")

but if I want to use (and I should be able to) a method like this one:

    @expose()
    @validate(form=comment_form)
    @error_handler(add)
    def save(self, name, email, many):
        raise redirect("index")

I get this error:

Page handler: <bound method Root.save of <formstutorial.controllers.Root object at 0xb7646f2c>>
Traceback (most recent call last):
  File "/usr/local/lib/python2.4/site-packages/CherryPy-2.2.0beta-py2.4.egg/cherrypy/_cphttptools.py", line 99, in _run
    self.main()
  File "/usr/local/lib/python2.4/site-packages/CherryPy-2.2.0beta-py2.4.egg/cherrypy/_cphttptools.py", line 247, in main
    body = page_handler(*virtual_path, **self.params)
TypeError: save() takes at least 4 non-keyword arguments (3 given)

Another thing I noticed is that now (I guess from some on the last revisions #795) input_values also contains self ie the reference to the method being decorated, I guess is would be better to leave it out.

I'm pinging (yes, again :D) Simon so that we can hear his opinion.

Note to Kevin, this thing is completely backward compatible but you may want to a look at the patch anyway. ;)

comment:39 Changed 13 years ago by michele

  • Cc simon.belak@… removed

Last thing, my patch seems to not contain the new tests, can you add them back Alberto? now I have to go offline. :-)

comment:40 Changed 13 years ago by anonymous

Hi Kevin,

The use cse I'm most interested in is to simplify controller code when you want to insert related items in the same form. Example from a real small app i'm writing:

class CamposGrupo(widgets.WidgetsDeclaration):
    nombre = widgets.TextField(validator=validators.NotEmpty())
    web = widgets.TextField(
        validator = validators.String(if_empty=None),
        default ="http://",
    )
    ubicacion = widgets.AutoCompleteField(
        search_controller = turbogears.url("/ubicaciones"),
        search_param = "q",
        result_name = "ubicaciones",
    )

class CamposContacto(widgets.WidgetsDeclaration):
    nombre = widgets.TextField(validator=validators.NotEmpty())
    email = widgets.TextField(validator=validators.Email())
    telefono = widgets.TextField(validator=validators.Regex(r'^6[0-9]{8}$'))

class CamposEnvio(widgets.WidgetsDeclaration):
    fichero = widgets.FileField()

class CamposFormulario(widgets.WidgetsDeclaration):
    grupo = FieldSet(fields=CamposGrupo(), legend="Datos del grupo")
    contacto = FieldSet(fields=CamposContacto(), legend="Contacto")
    envio = FieldSet(fields=CamposEnvio(), legend="Las maquetas")

formulario = widgets.TableForm(
    name = "form",
    submit_text = "Enviar",
    fields = CamposFormulario()
)

# The model that this form should manage

class Grupo(SQLObject):
    nombre = UnicodeCol(alternateID = True)
    web    = StringCol(default=None)
    contacto = SingleJoin("Contacto")
    envios = MultipleJoin("Envio")
    contactos = MultipleJoin("Envio")
    ubicacion = ForeignKey("Ubicacion")

class Contacto(SQLObject):
    nombre = UnicodeCol(notNone=True)
    email = StringCol(alternateID = True)
    telefono = StringCol()
    grupo = ForeignKey("Grupo")

class Envio(SQLObject):
    nombre = UnicodeCol(notNone=True)
    fecha = DateTimeCol(default=datetime.now)
    fichero = StringCol(unique=True)
    grupo = ForeignKey("Grupo")
    tamanio = IntCol()

class Ubicacion(SQLObject):
    nombre = UnicodeCol(alternateID=True)
    grupos = MultipleJoin("Grupo")

# now the controller code that handles this form's submission

class EnvioController(controllers.RootController):
    @turbogears.validate(form=formulario)
    @turbogears.error_handler(index)
    @turbogears.expose(template=".templates.fichero_enviado")
    def registrar(self, *args, **kw):
        """ Aqui crean los registros en la base y se guarda el fichero """
        
        grupo = kw.get("grupo", None)
        if grupo:
            grupo = crea_grupo(**grupo)
        else:
            raise Exception

        envio = kw.get("envio", None)
        if envio:
            envio = procesa_envio(envio, grupo)
        else:
            raise Exception
            
        contacto = kw.get("contacto", None)
        if contacto:
            contacto = Contacto(grupo=grupo, **contacto)
        else:
            raise Exception

As you can see, The form displays fields for filling in 4 related objects and the controller code can handle each related object in a clean way (No need to get the fields for each related object from a flat dict and building another to create or upadte the related object).

This is a silly example but you can get an idea... the advantages for more complex forms will be much greater.

Apart from this, name collisions are also avoided :)

This only introduces a change if the form is using FieldSets? or any future container/compound widget, if none are used the mechanics are exactly the same as they are now (a flat dict).

It seems much more clean to me to receive the results of a structured form in the same structured manner.

Alberto

comment:41 Changed 13 years ago by michele

Now I will really go, anyway Alberto is this use case the same Karl was talking about here #326? I Hope, so we can close also that ticket. :-)

Changed 13 years ago by alberto

michele's last patch with tests for the id stuff too

comment:42 Changed 13 years ago by alberto

Yep, I think it addresses more or less the same problem as #326, that's two ducks for one shot :) (though I shouldn't be joking with ducks with all that Flu stuff... :/ )

I think this stuff is now ready to go in the trunk if Kevin approves, I'll try to document it too in the Widget wiki and post to the ML so broken forms know the reason of their infortune :)

Thanks for your work Michele, really.

Alberto

comment:43 Changed 13 years ago by anonymous

Michele,

Regarding your problem with the controller... Are you sure about this? test_form_controller has a similar method and doesn't fail, maybe you can extend this unittest so it fails with your problem when you have a chance... this way it'll be easier for Simon to spot, we'll make sure it doesn't pop up again and all those test-goodies :)

Thanks, Alberto

P.S... As I wrote this I realized that test_form_controllers doesn't use nested dicts. I'll try to write the test myself see if I hit it too.

Changed 13 years ago by alberto

Yikes! Subversion does weird stuff when repatching uncommitted files... :/ fixed test_nested_widgets

comment:44 Changed 13 years ago by alberto

Yep, I've spotted the problem Michele is describing. I'm adding a patch that includes all of the progress made to this point plus test at turbogears.widgets.tests.test_nested_form_controllers which fails for the reason described by Michele.

Simon, can you please take a look at it when you have a chance? :) Thanks!

Alberto

Changed 13 years ago by alberto

comment:45 Changed 13 years ago by michele

Just to make things clear to Simon, what we are trying to achieve is this.

On input values we get something like:

{'many.other': '2', 'many.hello': '4', 'name': 'michele', 'email': '12'}

then we use formencode NestedVariables? to make a nested dict from this and end up with something like:

{'many' {'other': '2', 'hello': '4'}, 'name': 'michele', 'email': '12'}

so we want to be able to use an handler method like this:

def save(self, name, email, many)

Can this be done (hope so)? Note that ATM the NestedVariables? validator is triggered by calling the validate method on a form.

I should also note that using something like:

def save(self, name, email, many={})

works but we always get a validation error if we have a validator on one of the nested fields since many defaults to an empty dict always, so I guess that maybe we can get around this problem if we manage default value even in the case of nested dicts.

Simon, where are you!? :D

Changed 13 years ago by michele

Cool!

Changed 13 years ago by alberto

cleanead up, 2.3ized nested-widgets-mega-patch :)

Changed 13 years ago by anonymous

fixed the automplete's instance name

Changed 13 years ago by michele

py2.3, uses "_" instead of "." for ids, some fixes here and there

Changed 13 years ago by michele

The name says it all

comment:46 Changed 13 years ago by michele

This is getting longer than #490. :D

Anyway that's the actual status:

michele@ionic:~/Progetti/TurboGears/svn/turbogears/widgets$ nosetests
....................................................
----------------------------------------------------------------------
Ran 52 tests in 2.440s

OK
michele@ionic:~/Progetti/TurboGears/svn/turbogears/widgets$

comment:47 Changed 13 years ago by alberto

I must take off my hat :)

Great work!

This should be comitted ASAP... and this ticket closed! ;)

Changed 13 years ago by alberto

comment:48 Changed 13 years ago by alberto

Couple of tweaks:

  • Made tesform_new_style really new style (was using tg_errors instead of error_handler).
  • Changed the filter so it only instantiates the decoder once as a little speed boost for something that potentially get's done in every request. nothing a proper tree traversal wouldn't outperform :D

Once again... thanks! ;) Alberto

comment:49 Changed 13 years ago by michele

Thanks to you Alberto, two minds are better than one! ;-)

comment:50 Changed 13 years ago by simon

Another thing I noticed is that now (I guess from some on the last revisions #795) input_values also contains self ie the reference to the method being decorated, I guess is would be better to leave it out.

I have talked to Kevin and we decided it should stay, as removing it, just adds complexity to the code.

The only problem I'm experiencing with this patch is that I can't use the nice form we want to use. :D

I think this needs a trivial fix here or somewhere in controllers.py.

AFAIK CP does not fold arguments in the form of name.attribute to a dict. I will open a ticket on this and eventually enchant expose with this functionality.

comment:51 Changed 13 years ago by michele

Hey Simon, don't mind we already fixed it by using a filter that runs NestedVariables?() before anything with Kevin approval. ;-)

It was not a fault on your code, regarding self well I was under the impression the problem was coming from it but that's not the case so use the code you feel is more suited and easier you're the "decorator guru". ;-)

Can I ask you a question? How are default arguments managed ATM? from the decorator? or CP?

comment:52 Changed 13 years ago by simon

Great!

When possible, default arguments as defined in the controller method (with decorator just preserving them) are used. Decorator just adds an "argument sink" (*, ) to prevent errors where to many inputs are passed in and to allow various lower-level arguments to be passed around (e.g. tg_flash).

comment:53 Changed 13 years ago by michele

Mmm, now I have to go anyway.

Anyway I think we should do something to manage default arguments when they are dict, for example ATM you can't provide the default only for one field because the whole input dict is used, the right solution it to just update the default dict with the input value one and do it recursively because we could have nested dicts, this should be easy, we just need to walk every dict key if it's a dict and update it (I do something similar in the update_errors_dict_from_parent method that's inside the patch).

Maybe we can do this when this patch hits svn.

Thanks again Simon.

comment:54 Changed 13 years ago by alberto

  • Status changed from new to closed
  • Resolution set to fixed

Committed at r804. Great work, both of you ;)

Alberto

P.S. Another soap-opera ticket to miss :D P.P.S Mind air collision! The ticket has hit svn. Should we reopen this ticket or open another one for default argument stuff? For the time being, I'll close it.

comment:55 Changed 13 years ago by simon

#605 -- default argument stuff :)

comment:56 Changed 13 years ago by Main page

  • Severity changed from minor to critical
  • Cc <Default> added
  • Component changed from Widgets to Installation
  • Priority changed from normal to low
  • Version set to 0.9a4
  • Keywords <Default> added
  • Type changed from enhancement to task
Note: See TracTickets for help on using tickets.