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

Opened 13 years ago

Last modified 12 years ago

[PROPOSAL] Small forms refactoring to make them more useful and easy to customize

Reported by: alberto Owned by: anonymous
Priority: normal Milestone: 0.9
Component: TG Widgets Version:
Severity: normal Keywords:
Cc: dangoor+turbogears@…

Description

Hi,

Following from this  thread from the ML, I think we should do some little refactoring at the Form/Fieldset?'s update data.

One of the motivations is that they are very similarily coded (Form and FieldSet?) at update_data when building the list of tuples to pass to the template as fields, we can also make the more useful and easier to customize if we make available to the template the following functions:

  • field_for(name) -> Returns the sub field for name's curried display()
  • error_for(name) -> Returns the error for name
  • help_for(name) -> Returns the field text for name
  • label_for(name) -> Returns a Label widget (with computed field id) for name

This should make the form and fieldset labels more intuitive and easier to customize, for example:

<form>
<span py:for"name in [field.name for hidden_fields]" py:replace="field_for(name)()">
<ul>
<li py:for="name in [field.name for visible_fields]">
${label_for(name)} ${field_for(name)(extra_args_to_display)} ${help_for(name)} ${error_for(name)}
</li>
</ul>
</form>

field_for and error_for are easy (see patch at ML), but label_for, error_for and value passing are harder because of the nested error dict, nested value dict and computed-at-display field ids.

I think we could try partial application of some args before sending them to the template, something like:

def get_field_for(self, parent, values, options):
    def _field_for(name):
        field = self.field_for(name)
        def _curried_display(*args, **kw)
            new_opts = kw.pop('options', {})
            options[name].update(new_opts)
            return field.display(value=value[name], options=options[name], parent=parent, *args, **kw)
        return _curried_display
    return _field_for

   def update_data(self, d):
      super(BlaBla, self).update_data(d)
      options = get options for this level of the tree
      parent = update parent
      value = get values for this level
      d["field_for"] = self._get_field_for(parent, value, options)
      d["error_for"] = self._get_error_for(.....)
      ........

error_for and label_for would follow a similar technique... Disclaimer: code not tested

The idea is to get the options, parent, and value we get for this level of the tree out of the template as they really don't belong there, they need to be there right now because we need to pass parameters for child widgets from the error_dict, value_dict and options_dict, but the template designer shouldn't be concerned with this. The way we need to dynamically build this parameters on each request could be easily hidden with this technique.

I think this logic should affect FieldSet? as well, right now it lacks some features as we've failed to keep them in sync (hidden fields for example) so maybe this should go to a superclass and make both inherit from it (CompundWidget??... mmm, not so sure).

I could start trying some POC tomorrow or the next...

Comments? Ideas?

Alberto

Change History

comment:1 Changed 13 years ago by anonymous

  • Cc michele added

comment:2 Changed 13 years ago by simon

Shameless self-promotion: #613 ;)

comment:3 Changed 13 years ago by michele

  • Cc dangoor+turbogears@… added; michele removed

Alberto... a mind that never stops! :D

Ok, not you will kill me but I'm -1 on this, I also thought about it when I've introduced field_for and error_for #523 but here I will explain some of the reasons I'm against this.

You are mixing two different concepts here a Form and a FieldSet?, the latter is only a form field, the field_for and error_for methods are not meant to be used by a field but only to easily build a form, I think that incidentally instead of simplifying things this will only complicate them.

Those methods should not be used inside a widget template but just for having a finer control over widget appearance on the final template (the ticket I mentioned contains an example of this).

Also keep in mind that you can easily retrieve the field help text and whatever field attribute in this way:

!#python
form.field_for("name").help_text

field_for returns the widget instance not the already render template, I know that this could look a little harder from the designer POV but we can't hide everything inside update_data, people that don't use kid can also use this method:

!#python
form.field_for("name").render()

instead of calling display, if we hardcode the display method we are shooting them, also with this approach we should provide a whole load of *_for methods (you're proposing 4 of them ATM) to return every other thing we will add later.

Things get even more difficult since we introduced the parent managing (and I modified the field_for and error_for methods to make it still easy) and will get even hardier to explain the right way of using it, I only see this as a way to display top-level form fields when you know their names and not to build a widget template that should be generic, from my POV the template example you posted is already more complex than what we are using actually.

Finally this method will not be usable from a kid template since it will except the template scope to contain those variables you mentioned, again take a look at #523 and you will notice that my idea with field_for is to be able to easily layout a form (and not it's fields) in a different way but the real main selling point is that this doesn't need any modification on the controller side.

I can see your point on simplifying the widget update_data method, but well there will never be a definitive way that avoids the code duplication, sometimes is just needed and the price to remove could be to high and could end up to force only that behavior and will get into your way.

You mention that FieldSet? has not hidden field management, well I would say that I'm more in favor of totally removing it even from the Form since I doesn't make much sense there, it's only there because people complained that hidden fields on the browser are being displayed with a space but I've never experienced this myself.

Also as I said I don't think our widgets should use those methods in their templates, but I don't see anything wrong in just doing this:

d["field_for"] = self.field_for
d["error_for"] = self.error_for

but just this (and I will also complain that's not the intended way of using a widget since you're assuming that you know it's structure) or field_for will not be usable in our templates as you can use it now, it should just return the field instance associated with this name nothing more than that.

I would also point out that if we can come up with some methods that can help to remove code duplication from Form/FieldSet? and help to do the right thing I'm all in favor of this, I just don't think changing field_for and error_for in this way is the solution. For example in #611 I modified get_dict_by_parent to always return the right dict if you are a compoundwidget or a widget this simplified things on FieldSet?.

cc-ing kevin as I would like to hear his opinion on this.

comment:4 Changed 13 years ago by michele

My english sucks so I'm sure I'm unable to explain all the reasons that are flowing in my mind and that are putting me strongly against this.

I can just say that I don't see the use of field_for, error_for and such methods inside widgets template as the solution to this code duplication thing, hiding parameter to those function makes a template language like Kid (where you can still access python functions) plain useless and will not make the template any simpler, I do think our actual templates are really straightforward now, not because I've made some of them but just because they are so

It's hard to draw the line that separates controller logic and view logic, but I think hiding such things is not the right line and will just obscure the behavior leading to some unwanted side effects, but I can be wrong every one can have a subjective opinion.

Again, I do think that we can study and maybe (if possible) abstract some of the duplication going on here and this will be a big benefit I just think the solution you're proposing is not related to this problem and abstracting in this way will IMHO complicate things instead.

comment:5 Changed 13 years ago by michele

Ah, one last thing.

In my first post I (wrongly) talked about changes to field_for as we know it, your solution doesn't affect it in reality, but anyway I still think what we should do is just providing access to it in form widgets template (for people that want to use it) but not using it in our templates and by hardcoding the display thing.

comment:6 Changed 13 years ago by alberto

Hi michele.

You're example at #523 does not convince me, sorry, really. This is beacuse you're delegating forms resposabilities (laying out it's child widgets) to the template that contains the form, just doesn't make sense to me, especially because it contradicts what the form's template already is doing. The ML proves that at least one person has found this confusing.

Giving access to field_for and error_for to the template (via update_data) is a partial solution. We still need to take care of the fields label (which we already know that the id it point's to is built dynamically) so we also need knowledge of our parent.

Altogether, I want to

what I really want to do with this is:

.1 Unify how CompoundWidgets work (Form and FieldSet? are, after all, compound widgets).

.1 Let compound widgets "know" (if they need to, like for form layout) about theit child widgets in a uniform way (that is, with accessor functions.

  1. Pre bind any information computed dynamically (at the moment, parent) so the template doesn't need to know about it (as it really shouldn't, shouldn't it? I think it's an *artifact* were introducing regarding some info we need to handle the tree structure)

And hey! let's please cool this down, we're getting over-excited (both of us here) :) Alberto

comment:7 Changed 13 years ago by michele

Hi Alberto,

first of all, I'm would like to point out that I'm writing things a bit faster than my english can handle :D so sorry if I sound "hot" it's really not my intention but that can be the impression you get, I just want a civil discussion like we have done before (like for WidgetsDeclaration?).

First of all, the intention of field_for and error_for "is to delegate" the form display work to the template, not to contradict the widget template work but to let the template be more designer friendly (how can someone customize a template that just uses form.display()?) and have a finer control over the final appearance, the example also uses Form that's not providing a template for example, we can't use widgets for every thing but being able to customize a form without touching the controller side is a BIG point IMHO.

The person on ML found this confusing because I haven't pointed him to this example that only resides on this ticket and no one know apart me (and maybe Jorge), moreover he wanted a reusable layout that you can easily achieve using the example I posted or by passing the field_for and error_for thing along the template, again I don't think moving field_for and error_for to the widget is correct from a OOP POV but that's only my opinion.

Now your points:

1) We can't totally unify how a Form and a FieldSet? are working, yes they are CompoundWidget? but they are quite different, a Form is a very special widget (to the point it deserves a custom parameter in the validate decorator) and "the only" one you can use to collect input from the web, a Form can contain a FieldSet? but a FieldSet? can't contain a Form, that's a quite important distinction.

2) We can't predict or impose how every single widget will want to treat it's children widgets, before my modifications to the widgets API there wasn't a CompoundWidget? concept and widgets where being putted into random attributes thus the inability to provide retrieve_css/retrieve_javascript/validate methods working out of the box, now they are all putted into self.widgets but we can't predict any further how a widget writer will categorize them.

3) You probably remember I was the first to criticize the parent attribute being passed along, I still don't like it but I can't see any solution to this, passing half hardcode methods (with some parameters) to the template is not a viable solution to the problem, the best thing I come up with during the parent work we have done was to make it easy to use field_for and error_for without the need of passing parent to it or the ability to just pass a string that's the form name you want to use (you can check it by looking at the Form code), we can't expose the form structure any deeper or things will get even more harder IMHO.

Also, why should we provide a method to retrieve the widget label to be used? if you need to use field_for it means you already know the final form structure and you know where you want to put this and that field (for example on the same row), thus you also already know the label that this field will use. If that's not the case it means you don't know the form structure -> you can't make any assumption on it and it's fields names -> you don't need and can't use the field_for method that works by receiving a field name -> you need a generalized widget that works by using some general rules you want (for example, every row the label, the field, it's error) -> our templates are the right answer to this problem.

It all boils down to this, you can't make any assumption on the fields name a form will use so it doesn't make sense having a template that uses fields name to build itself, if you have such a form that's not a form anymore but a specialized form, for example what you're trying to achieve does make a lot of sense for something like this:

class GmailLoginForm(Form):
    fields = [TextField(name="username"), 
              TextField(name="password"), 
              CheckBox(name="remember")]

    template = """
    <form action="login">
    <div>
    <span>Username: field_for("username")</span>
    <span>Password: field_for("password")</span>
    <div>Remember me on my next login: field_for("remember")</div>
    <input type="submit" value="Login">
    </div>
    </form>
    """

this is a specialized form, a form you use in this way:

gmail_form = GmailLoginForm()

and never by passing the fields parameter (it's not used), that's the reason it makes sense to use field_for, that's what that user asked for on the ML because that's a reusable form layout ready to be used, and finally I would just add that if you just need this login form on a single page (not to be used on many pages) and you want to make it more customizable by a designer using field_for inside your template makes sense.

For generalized forms (like the one we are providing with TG) I don't think something like this:

<li py:for="name in [field.name for visible_fields]">
${label_for(name)} ${field_for(name)(extra_args_to_display)} ${help_for(name)} ${error_for(name)}
</li>

is easier than this (what we have now):

<li py:for="field, value, options, error in visible_fields">
${field.label} ${field.display(value, options, parent)} ${field.help} ${error}
</li>

(that's a huge reply... D'oh! :D)

comment:8 Changed 13 years ago by michele

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

Fixed on r912.

I will fix the field_for thing in another iteration after discussing about it on the group.

comment:9 Changed 13 years ago by michele

That was r913.

comment:10 Changed 13 years ago by alberto

Great work Michele! :)

You even took care of the options stuff which is great, however, it now conflicts with SelectionField?'s options :( I guess I can fix it by renaming these to "select_options" or the current "options" to "extra_params" or something. What do you think?

Thanks many! :)

Alberto,

P.S: As I've told you by email, I'll take care of #613 as soon as I can sneak some time from work, I'm really busy this week :( Just to justify my "disappearence" from the community :)

comment:11 Changed 13 years ago by alberto

Doh! forget the "options" thing I've mentioned... was a non-related bug in my app... works flawlessly. Think I need another coffee :) Alberto

comment:12 Changed 13 years ago by michele

Hi Alberto,

thanks, and yes options are working just fine but now I've made the mechanism generalized so that you can pass any kw along without special casing the options name as we were doing before, this let's any widget define the options it wants with the name it wants, to make an example you can also pass along attrs using the nested dict.

comment:13 Changed 13 years ago by alberto

Hi Michele,

I'm migrating some forms to the new style (you broke my apps, bastard ;) and I've got something to point out, it has a comment that says it's going to be deprecated, but anyway...

Shouldn't be "field_for" (or whatever replaces it) in FormFieldsContainer? so it works for any kind of container?

I read somewhere that you were thinking about a widget.display_field_for("form.bla.blou") syntax. Seems like a good idea to me, as it'll let us have total control over the layout. Do you want me to give it a shot or are you planning to do it somewhen soon?

Thanks again :)

Adeu, Alberto

comment:14 Changed 13 years ago by michele

Hi Alberto,

I have the patch almost ready for that feature, I will add it only to form since that's the only point where it makes sense IMHO.

Ciao Michele

comment:15 Changed 13 years ago by alberto

Great! But i'm not so sure about it being only in the form... I would really like to make some fieldsets and have control over inner field's layout from the fieldset's template as it's being placed in at least 3 different forms and I would like to factor all the layout into one template. Shouldn't this be a fairly common use case? or am i just weird? :D

Alberto

P.S. I've just been picking up in the ML and read Kevin's comments regarding our discussions in the Trac (no wonder... ;) Should this go to the brand shiny new turbogears-trunk maybe?

comment:16 Changed 13 years ago by michele

Well, having it on FormFieldsContainer? would not hurt either so we can go for it, if you want to work on it I can give you the partial patch I'm doing since until tomorrow or later I will not have any time to work on this most probably.

Ciao

comment:17 Changed 13 years ago by alberto

Ok, I was giving #613 a try but I might have some time for this, post it just in case... Adeu Alberto

Note: See TracTickets for help on using tickets.